[PATCH v10 3/3] pwm: Add support for Xilinx AXI Timer

[Sean Anderson]

Hi Uwe,

On 11/19/21 3:43 AM, Uwe Kleine-König wrote:
> Hello Sean,
>
> 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.

>> > > +	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. 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.

>> > > +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);

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.

>> > > +	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/

>> 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".

--Sean