On 3/3/22 5:25 PM, Uwe Kleine-König wrote: > 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/. OK >> [...] >> + if (one_timer) >> + return dev_err_probe(dev, -EINVAL, >> + "Two timers required for PWM mode\n"); >> + >> + > > One empty line here would be enough. OK >> 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. OK --Sean
