Re: [PATCH v13 2/2] pwm: Add support for Xilinx AXI Timer
From: Uwe Kleine-König <hidden>
Date: 2022-03-03 22:25:56
Also in:
linux-devicetree, linux-pwm, lkml
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.
quoted hunk ↗ jump to hunk
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/ |