lkml.org 
[lkml]   [2021]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates
On Wed, Sep 15, 2021 at 08:55:40AM +0200, Wolfram Sang wrote:
> From: Duc Nguyen <duc.nguyen.ub@renesas.com>
>
> ENOTSUP has confused users. EINVAL has been considered clearer. Change
> the errno, we were the only ones using ENOTSUP in this subsystem anyhow.
>
> Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com>
> [wsa: split and reworded commit message]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/pwm/pwm-renesas-tpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
> index 4381df90a527..754440194650 100644
> --- a/drivers/pwm/pwm-renesas-tpu.c
> +++ b/drivers/pwm/pwm-renesas-tpu.c
> @@ -269,7 +269,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
>
> if (prescaler == ARRAY_SIZE(prescalers) || period == 0) {
> dev_err(&tpu->pdev->dev, "clock rate mismatch\n");
> - return -ENOTSUPP;
> + return -EINVAL;
> }

prescaler == ARRAY_SIZE(prescalers) means that period_ns * clk_rate is
too big for the hardware. Given that the driver considers clk_rate as
constant, the interpretation is that period_ns is too big to be
implemented. In this case the expectation for a new driver would be to
round down to the biggest possible rate. period == 0 means the requested
period is too small, in this case -EINVAL is right.

The danger to make the driver behave like new drivers should is that it
ends in regressions, but when we touch the behaviour this might be a
good opportunity to "fix" this driver?

This would look as follows:

max_period_ns = 0xffff * NSEC_PER_SEC * 64 / clk_rate;

period_ns = min(period_ns, max_period_ns);
duty_ns = min(duty_ns, period_ns);

/*
* First assume a prescaler factor of 1 to calculate the period
* value, if this yields a value that doesn't fit into the 16
* bit wide register field, pick a bigger prescaler. The valid
* range for the prescaler register value is [0..3] and yields a
* factor of (1 << (2 * prescaler)).
*/

period = clk_rate * period_ns / NSEC_PER_SEC;
if (period == 0)
return -EINVAL;

if (period <= 0xffff)
prescaler = 0;
else {
prescaler = (ilog2(period) - 14) / 2;
BUG_ON(prescaler > 3);
}

period >>= 2 * prescaler;

duty = clk_rate * duty_ns >> (2 * prescaler) / NSEC_PER_SEC;

(Note: This needs more care to handle overflows, e.g. 0xffff *
NSEC_PER_SEC * 64 doesn't fit into a long, also clk_rate * period_ns
might overflow. I skipped this for the purpose of this mail.)

The code comment "TODO: Pick the highest acceptable prescaler." is
unclear to me, as a smaller prescaler allows more possible settings for
the duty_cycle and I don't see any reason to pick a bigger prescaler.

If we choose to not adapt the behaviour, I suggest to replace

if (duty_ns) {
duty = clk_rate / prescalers[prescaler]
/ (NSEC_PER_SEC / duty_ns);
if (duty > period)
return -EINVAL;
} else {
duty = 0;
}

by:

duty = clk_rate * duty_ns >> (2 * prescaler) / NSEC_PER_SEC;

(probably using u64 math after asserting that period_ns * clk_rate
doesn't overflow a u64. Then given that duty_ns <= period_ns the case
duty > period cannot happen, the special case for duty_ns == 0 doesn't
need to be explicitly handled and precision is improved.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-09-17 10:26    [W:1.081 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site