Messages in this thread Patch in this message | | | Date | Thu, 3 Mar 2022 23:25:42 +0100 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v13 2/2] pwm: Add support for Xilinx AXI Timer |
| |
Hello,
just a few minor things left for me to criticise:
On Fri, Feb 04, 2022 at 01:01:06PM -0500, Sean Anderson wrote: > [...] > + regmap_read(priv->map, TCSR1, &tcsr1); > + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0); > + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); > + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + state->polarity = PWM_POLARITY_NORMAL; > + > + /* > + * 100% duty cycle results in constant low output. This may be slightly > + * wrong if rate >= 1GHz, so fix this if you have such hardware :) > + */
I'd drop "slightly" here. If the bug happens (e.g. tlr0 = 999999999, tlr1 = 999999998, clkrate = 1000000001 Hz) then you diagnose a nearly 100% relative duty cycle as 0%. Also s/>= 1GHz/> 1 GHz/.
> [...] > + if (one_timer) > + return dev_err_probe(dev, -EINVAL, > + "Two timers required for PWM mode\n"); > + > +
One empty line here would be enough.
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h > new file mode 100644 > index 000000000000..1f7757b84a5e > --- /dev/null > +++ b/include/clocksource/timer-xilinx.h > @@ -0,0 +1,91 @@ > [...] > +/** > + * xilinx_timer_common_init() - Perform common initialization for Xilinx > + * AXI timer drivers. > + * @priv: The timer's private data > + * @np: The devicetree node for the timer > + * @one_timer: Set to %1 if there is only one timer > + * > + * This performs common initialization, such as detecting endianness, > + * and parsing devicetree properties. @priv->regs must be initialized > + * before calling this function. This function initializes @priv->read, > + * @priv->write, and @priv->width. > + * > + * Return: 0, or negative errno > + */ > +int xilinx_timer_common_init(struct device_node *np, > + struct xilinx_timer_priv *priv, > + u32 *one_timer); This function is gone, so the prototype should go away, too.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |[unhandled content-type:application/pgp-signature] | |