Re: [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3668 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]