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

Commit Message

Wolfram Sang Sept. 15, 2021, 6:55 a.m. UTC

From: Duc Nguyen <[e-mail]>

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 <[e-mail]>
[wsa: split and reworded commit message]
Signed-off-by: Wolfram Sang <[e-mail]>
---
 drivers/pwm/pwm-renesas-tpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sergei Shtylyov Sept. 15, 2021, 9:36 a.m. UTC | #1

On 15.09.2021 9:55, Wolfram Sang wrote:

> From: Duc Nguyen <[e-mail]>
> 
> ENOTSUP has confused users. EINVAL has been considered clearer. Change
> the errno, we were the only ones using ENOTSUP in this subsystem anyhow.

    It's ENOTSUPP in the code. :-)

> 
> Signed-off-by: Duc Nguyen <[e-mail]>
> [wsa: split and reworded commit message]
> Signed-off-by: Wolfram Sang <[e-mail]>
> ---
>   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;
>   	}
>   
>   	if (duty_ns) {

MBR, Sergei

Uwe Kleine-König Sept. 17, 2021, 8:25 a.m. UTC | #2

On Wed, Sep 15, 2021 at 08:55:40AM +0200, Wolfram Sang wrote:
> From: Duc Nguyen <[e-mail]>
> 
> 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 <[e-mail]>
> [wsa: split and reworded commit message]
> Signed-off-by: Wolfram Sang <[e-mail]>
> ---
>  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

Wolfram Sang Sept. 20, 2021, 9:08 a.m. UTC | #3

Hi Uwe,

thank you for your detailed review, much appreciated! I will look into
your suggestions. However, it will probably not be before October
because it seems some more work and internal discussion is needed
beforehand. I'll get back to you.

Thanks again and happy hacking,

   Wolfram

Patch

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;
 	}
 
 	if (duty_ns) {