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@xxxxxxxxxxxxxx > > 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@xxxxxxxx > > 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@xxxxxxxxxxxxxx) - 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@xxxxxxxxxxxxxx) 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@xxxxxxxxxxxxxxxxxx/ 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/ |
Attachment:
signature.asc
Description: PGP signature
