[PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Tue Nov 23 00:41:51 PST 2021
Hello Sean,
On Mon, Nov 22, 2021 at 12:32:19PM -0500, Sean Anderson wrote:
> On 11/19/21 3:43 AM, Uwe Kleine-König wrote:
> > On Thu, Nov 18, 2021 at 04:08:45PM -0500, Sean Anderson wrote:
> > > On 11/18/21 4:28 AM, Uwe Kleine-König wrote:
> > > > On Fri, Nov 12, 2021 at 01:55:04PM -0500, Sean Anderson wrote:
> > > > > [...]
> > > > > + /* cycles has a max of 2^32 + 2 */
> > > > > + return DIV64_U64_ROUND_CLOSEST(cycles * NSEC_PER_SEC,
> > > > > + clk_get_rate(priv->clk));
> > > >
> > > > Please round up here.
> > >
> > > Please document the correct rounding mode you expect. The last time we
> > > discussed this (3 months ago), you only said that rounding down was
> > > incorrect...
> >
> > I think you refer to
> > https://lore.kernel.org/linux-pwm/20210817180407.ru4prwu344dxpynu@pengutronix.de
> > here, right? I agree that I could have been a bit more explicit here.
> >
> > .apply should first round down .period to the next achievable setting
> > and then .duty_cycle should be round down to the next achievable setting
> > (in combination with the chosen period).
> >
> > To get .apply ∘ .get_state idempotent (i.e. if I apply the result from
> > get_state there are no changes), .get_state has to round up.
> >
> > After our longer discussion about v4 I would have expected that this was
> > already obvious. There you wrote[1]:
> >
> > > * The apply_state function shall only round the requested period down, and
> > > may do so by no more than one unit cycle. If the requested period is
> > > unrepresentable by the PWM, the apply_state function shall return
> > > -ERANGE.
> > > * The apply_state function shall only round the requested duty cycle
> > > down. The apply_state function shall not return an error unless there
> > > is no duty cycle less than the requested duty cycle which is
> > > representable by the PWM.
> > > * After applying a state returned by round_state with apply_state,
> > > get_state must return that state.
> >
> > The requirement to round up is a direct consequence of these three
> > points, which I confirmed (apart from some wording issues).
> >
> > [1] https://lore.kernel.org/linux-pwm/ddd2ad0c-1dff-c437-17a6-4c7be72c2fce@seco.com
>
> Ok, will fix. But again, a little something in
> Documentation/driver-api/pwm.rst would help a lot.
Ack, will take another attempt to care for that.
> > > > > + period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> > > > > + period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
> > > > > + if (period_cycles < 2 || period_cycles - 2 > priv->max)
> > > > > + return -ERANGE;
> > > >
> > > > if period_cycles - 2 > priv->max the right reaction is to do
> > > >
> > > > period_cycles = priv->max + 2
> > >
> > > It has been 5 months since we last talked about this, and yet you have
> > > not submitted any patches for a "pwm_round_rate" function. Forgive me if
> > > I am reticent to implement forward compatibility for an API which shows
> > > no signs of appearing.
> >
> > This requirement is not only for round_state. It's also to get some
> > common behaviour for at least new drivers. The primary goal here is to
> > make the result for pwm_apply more predictable.
>
> The behavior you specify is *not* common. No drivers currently round in
> the manner you specify here.
Hmm, if this is true I failed during the reviews before.
Looking through the last added drivers:
- visconti: There is no division involved, so there is no rounding
issue. Also period is round down if a too high value is requested.
- ntxec: There is no get_state callback because the hardware state
cannot be read out. Period is reduced as requested.
- raspberrypi-poe: Rounds up in .get_state() and down in .apply(). (A
bit special as this HW has a fixed period.)
- intel-lgm: Rounds up in .get_state() and down in .apply(). Ditto this
is a fixed-period driver and only too small periods are refused.
- dwc: This is indeed wrong. I didn't review the finally merged
version, but pointed out the driver being wrong in v2
(https://lore.kernel.org/linux-pwm/20200524201116.pc7jmffr6jxlwren@pengutronix.de)
- keembay: Rounds up in .get_state and dowan in .apply()
(Though the detection of a too high period might be broken?! Didn't
look into the details.)
- pwm-sl28cpld is aligned to this behaviour (though this is a bit of a
special case as there is no rounding involved). Refusing too big
periods (only) is done right in it.
- iqs620a: Is aligned to my request, too
- pwm-sprd: This is wrong, too. My review comments were not addressed
here either (e.g.
https://lore.kernel.org/linux-pwm/20190814150304.x44lalde3cwp67ge@pengutronix.de)
So while the situation isn't ideal, it's not as worse as you picture it.
> In fact, returning -ERANGE or -EINVAL is
> far more common than attempting to handle this case. If you would like
> new drivers to use a new algorithm, I suggest first converting existing
> drivers. I think it is unreasonable to hold new drivers to a standard
> which no existing driver is held to.
In the past Thierry opposed to adapting existing drivers. :-\
Having said that being more strict for new drivers isn't that uncommon.
For example I expect it wouldn't be possible to get something like
drivers/tty/serial/8250 into mainline today.
> > > > > +static int xilinx_timer_probe(struct platform_device *pdev)
> > > > > +{
> > > > > + int ret;
> > > > > + struct device *dev = &pdev->dev;
> > > > > + struct device_node *np = dev->of_node;
> > > > > + struct xilinx_timer_priv *priv;
> > > > > + struct xilinx_pwm_device *pwm;
> > > >
> > > > The name "pwm" is usually reserved for struct pwm_device pointers. A
> > > > typical name for this would be xlnxpwm or ddata.
> > >
> > > I suppose. However, no variables of struct pwm_device are used in
> > > this driver.
> >
> > Still it provokes wrong expectations when reading
> >
> > platform_set_drvdata(pdev, pwm);
> >
> > in a pwm driver.
>
> The second most common use of this function in drivers/pwm is the above usage.
>
> $ git grep -h platform_set_drvdata v5.15 drivers/pwm/ | sort | uniq -c | sort -n
> 1 platform_set_drvdata(pdev, atmel_pwm);
> 1 platform_set_drvdata(pdev, bpc);
> 1 platform_set_drvdata(pdev, ddata);
> 1 platform_set_drvdata(pdev, ec_pwm);
> 1 platform_set_drvdata(pdev, fpc);
> 1 platform_set_drvdata(pdev, ip);
> 1 platform_set_drvdata(pdev, lpc18xx_pwm);
> 1 platform_set_drvdata(pdev, lpwm);
> 1 platform_set_drvdata(pdev, mdp);
> 1 platform_set_drvdata(pdev, omap);
> 1 platform_set_drvdata(pdev, p);
> 1 platform_set_drvdata(pdev, pwm_chip);
> 1 platform_set_drvdata(pdev, rcar_pwm);
> 1 platform_set_drvdata(pdev, spc);
> 1 platform_set_drvdata(pdev, tcbpwm);
> 1 platform_set_drvdata(pdev, tpm);
> 1 platform_set_drvdata(pdev, tpu);
> 3 platform_set_drvdata(pdev, chip);
> 3 platform_set_drvdata(pdev, priv);
> 4 platform_set_drvdata(pdev, pwm);
> 6 platform_set_drvdata(pdev, pc);
Ack, this are all "old" drivers (i.e. img (2015), stmpe (2016), sun4i
(2015) and tegra(2012); "old" in the sense "before I engaged in pwm
reviews") ISTR that in this question Thierry agrees that only variables
with type struct pwm_chip * should be named pwm.
> With other contenders being "pc", "chip", "pwm_chip", and "p". This used
> to be more common, but several examples have been converted to
> devm_pwmchip_add. To say that such a variable (used once!) "provokes the
> wrong expectations" would be to have expectations misaligned with the
> corpus of existing drivers.
Yeah, I don't oppose to this interpretation. I'm not happy with the code
base. Reworking this is a big effort and difficult with the request to
not break assumption for existing drivers.
> > > > > + u32 pwm_cells, one_timer, width;
> > > > > + void __iomem *regs;
> > > > > +
> > > > > + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> > > > > + if (ret == -EINVAL)
> > > > > + return -ENODEV;
> > > >
> > > > A comment about why this is done would be great.
> > >
> > > OK. How about:
> > >
> > > /* If there are no #pwm-cells, this binding is for a timer and not a PWM */
> >
> > Fine. Does that mean the timer driver won't bind in the presence of the
> > #pwm-cells property, and the timer driver uses the same compatible?
> > Sounds a bit strange to me.
>
> Correct. See below.
>
> > > > > + /*
> > > > > + * The polarity of the generate outputs must be active high for PWM
> > > >
> > > > s/generate/generated/
> > >
> > > The signals I am referring to are called "GenerateOut0" and
> > > "GenerateOut1".
> >
> > Then maybe:
> >
> > The polarity of the outputs "GenerateOut0" and "GenerateOut1"
> > ...
> >
> > ?
>
> The exact wording of the configuration option is
>
> > Active state of Generate Out signal
>
> with a drop-down to select between "Active High" and "Active Low". So
> the most exact way to specify this would be
>
> The polarity of the Generate Out signals must be...
>
> > > > > +static struct platform_driver xilinx_timer_driver = {
> > > > > + .probe = xilinx_timer_probe,
> > > > > + .remove = xilinx_timer_remove,
> > > > > + .driver = {
> > > > > + .name = "xilinx-timer",
> > > >
> > > > Doesn't this give a wrong impression as this is a PWM driver, not a
> > > > timer driver?
> >
> > This directly relates to the fact that the timer driver and the pwm
> > driver (seem to) bind on the same compatible as already mentioned above.
> > The dt people didn't agree to this yet, did they?
>
> Rob Herring has acked the binding. And switching based on the presence
> of #pwm-cells was his idea in the first place:
>
> > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
> > the hint to configure as a PWM.
>
> As I understand it, the compatible should be the same if the hardware is
> the same. Ideally, this sort of thing would be configurable by userspace
> at runtime, but timers get probed so early that we have to use something
> in the devicetree.
>
> [1] https://lore.kernel.org/linux-pwm/20210513021631.GA878860@robh.at.kernel.org/
OK, then my expectation that Rob won't agree was wrong. Fine then.
> > > Perhaps. Though this is the PWM driver for the Xilinx AXI timer, not the
> > > Xilinx AXI PWM.
> >
> > I would be happier with "xilinx-timer-pwm" then.
>
> I've changed it to "xilinx_pwm".
OK.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20211123/a49537b1/attachment.sig>
More information about the linux-arm-kernel
mailing list