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

[Uwe Kleine-König]

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@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

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

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

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

> > > +	/*
> > > +	 * 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"
	...

?

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

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature