[PATCH 0/2] pwm: renesas-tpu: minor improvements

[PATCH 0/2] pwm: renesas-tpu: minor improvements

[Wolfram Sang]

Here is a super-small series with improvements for the Renesas TPU
driver from our BSP. Please look at the patch descriptions for details.
Looking forward to comments or them simply being applied :)

Thanks and happy hacking,

   Wolfram


Duc Nguyen (2):
  pwm: renesas-tpu: better errno for impossible rates
  pwm: renesas-tpu: don't allow no duty if duty_ns is given

 drivers/pwm/pwm-renesas-tpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.30.2

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

[Wolfram Sang]

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

[PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given

[Wolfram Sang]

From: Duc Nguyen <duc.nguyen.ub@renesas.com>

We have special code if duty_ns is 0. But if non-zero is given, then the
calculation should not result in zero duty.

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 754440194650..bb51156e4bda 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -275,7 +275,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
 	if (duty_ns) {
 		duty = clk_rate / prescalers[prescaler]
 		     / (NSEC_PER_SEC / duty_ns);
-		if (duty > period)
+		if (duty > period || duty == 0)
 			return -EINVAL;
 	} else {
 		duty = 0;
-- 
2.30.2

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

[Sergei Shtylyov]

On 15.09.2021 9:55, 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.

    It's ENOTSUPP in the code. :-)

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

MBR, Sergei

Re: [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given

[Uwe Kleine-König]

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

On Wed, Sep 15, 2021 at 08:55:41AM +0200, Wolfram Sang wrote:
> From: Duc Nguyen <duc.nguyen.ub@renesas.com>
> 
> We have special code if duty_ns is 0. But if non-zero is given, then the
> calculation should not result in zero duty.

Why not? Assuming a PWM that supports multiples of say 100 ns for
duty_cycle, rounding a request for 550 ns down to 500 ns isn't worse
than rounding down a request for 50 ns to 0 ns is it?
 
> 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 754440194650..bb51156e4bda 100644
> --- a/drivers/pwm/pwm-renesas-tpu.c
> +++ b/drivers/pwm/pwm-renesas-tpu.c
> @@ -275,7 +275,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
>  	if (duty_ns) {
>  		duty = clk_rate / prescalers[prescaler]
>  		     / (NSEC_PER_SEC / duty_ns);

Unrelated to the change under discussion here: Dividing by the result of
a division is bad. 

Consider:
	clk_rate = 1333333333
	prescalers[prescaler] = 43
	duty_ns = 500000001

The exact result is 15503875.996124031, with the above formula you get
31007751 which is off by nearly a factor of two.

These numbers are probably not relevant, but they show the problem.

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

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

[Uwe Kleine-König]

[-- 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 --]

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

[Wolfram Sang]

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

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


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