[PATCH 0/5] gpio: mvebu: pwm fixes and improvements

[PATCH 0/5] gpio: mvebu: pwm fixes and improvements

[Baruch Siach]

This series adds a few related fixes to the pwm .apply and .get_state 
callbacks.

The first patch was originally part of the series adding Armada 8K/7K pwm 
support. I split it out to a separate series following review comments from 
Uwe Kleine-König who spotted a few more issues. There is no dependency between 
this and the Armada 8K/7K series.

Baruch Siach (5):
  gpio: mvebu: fix pwm get_state period calculation
  gpio: mvebu: improve pwm period calculation accuracy
  gpio: mvebu: make pwm apply/get_state closer to idempotent
  gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
  gpio: mvebu: document zero pwm duty cycle limitation

 drivers/gpio/gpio-mvebu.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

-- 
2.29.2

[PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation

[Baruch Siach]

The period is the sum of on and off values.

Reported-by: Russell King <linux@armlinux.org.uk>
Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 672681a976f5..a912a8fed197 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	else
 		state->duty_cycle = 1;
 
+	val = (unsigned long long) u; /* on duration */
 	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
-	val = (unsigned long long) u * NSEC_PER_SEC;
+	val += (unsigned long long) u; /* period = on + off duration */
+	val *= NSEC_PER_SEC;
 	do_div(val, mvpwm->clk_rate);
-	if (val < state->duty_cycle) {
+	if (val > UINT_MAX)
+		state->period = UINT_MAX;
+	else if (val)
+		state->period = val;
+	else
 		state->period = 1;
-	} else {
-		val -= state->duty_cycle;
-		if (val > UINT_MAX)
-			state->period = UINT_MAX;
-		else if (val)
-			state->period = val;
-		else
-			state->period = 1;
-	}
 
 	regmap_read(mvchip->regs, GPIO_BLINK_EN_OFF + mvchip->offset, &u);
 	if (u)
-- 
2.29.2

[PATCH 2/5] gpio: mvebu: improve pwm period calculation accuracy

[Baruch Siach]

Divide the full period value to reduce rounding error.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index a912a8fed197..c424d88e9e2b 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -715,9 +715,9 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	else
 		on = 1;
 
-	val = (unsigned long long) mvpwm->clk_rate *
-		(state->period - state->duty_cycle);
+	val = (unsigned long long) mvpwm->clk_rate * state->period;
 	do_div(val, NSEC_PER_SEC);
+	val -= on;
 	if (val > UINT_MAX)
 		return -EINVAL;
 	if (val)
-- 
2.29.2

[PATCH 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent

[Baruch Siach]

Round up the result of division in period/duty_cycle calculation to make
the result closer to idempotent.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index c424d88e9e2b..6fc64846eda3 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -668,7 +668,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 
 	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), &u);
 	val = (unsigned long long) u * NSEC_PER_SEC;
-	do_div(val, mvpwm->clk_rate);
+	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
 	if (val > UINT_MAX)
 		state->duty_cycle = UINT_MAX;
 	else if (val)
@@ -680,7 +680,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
 	val += (unsigned long long) u; /* period = on + off duration */
 	val *= NSEC_PER_SEC;
-	do_div(val, mvpwm->clk_rate);
+	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
 	if (val > UINT_MAX)
 		state->period = UINT_MAX;
 	else if (val)
@@ -707,7 +707,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned int on, off;
 
 	val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
-	do_div(val, NSEC_PER_SEC);
+	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);
 	if (val > UINT_MAX)
 		return -EINVAL;
 	if (val)
@@ -716,7 +716,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		on = 1;
 
 	val = (unsigned long long) mvpwm->clk_rate * state->period;
-	do_div(val, NSEC_PER_SEC);
+	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);
 	val -= on;
 	if (val > UINT_MAX)
 		return -EINVAL;
-- 
2.29.2

[PATCH 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX

[Baruch Siach]

PWM on/off registers are limited to UINT_MAX. However the state period
and duty_cycle fields are ns values of type u64. There is no reason to
limit them to UINT_MAX.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 6fc64846eda3..eb7456fa6d86 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -669,9 +669,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), &u);
 	val = (unsigned long long) u * NSEC_PER_SEC;
 	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
-	if (val > UINT_MAX)
-		state->duty_cycle = UINT_MAX;
-	else if (val)
+	if (val)
 		state->duty_cycle = val;
 	else
 		state->duty_cycle = 1;
@@ -681,9 +679,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	val += (unsigned long long) u; /* period = on + off duration */
 	val *= NSEC_PER_SEC;
 	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
-	if (val > UINT_MAX)
-		state->period = UINT_MAX;
-	else if (val)
+	if (val)
 		state->period = val;
 	else
 		state->period = 1;
-- 
2.29.2

[PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation

[Baruch Siach]

Add a comment on why the code never sets the 'on' register to zero.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index eb7456fa6d86..4261e3b22b4e 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -706,6 +706,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);
 	if (val > UINT_MAX)
 		return -EINVAL;
+	/* zero 'on' value does not work as expected for some reason */
 	if (val)
 		on = val;
 	else
-- 
2.29.2

Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation

[Uwe Kleine-König]

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

Hello Baruch,

$Subject ~= s/get_state/.get_state/ ?

On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote:
> The period is the sum of on and off values.
> 
> Reported-by: Russell King <linux@armlinux.org.uk>
> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support")
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 672681a976f5..a912a8fed197 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
>  	else
>  		state->duty_cycle = 1;
>  
> +	val = (unsigned long long) u; /* on duration */
>  	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
> -	val = (unsigned long long) u * NSEC_PER_SEC;
> +	val += (unsigned long long) u; /* period = on + off duration */
> +	val *= NSEC_PER_SEC;
>  	do_div(val, mvpwm->clk_rate);
> -	if (val < state->duty_cycle) {
> +	if (val > UINT_MAX)
> +		state->period = UINT_MAX;
> +	else if (val)
> +		state->period = val;
> +	else
>  		state->period = 1;
> -	} else {
> -		val -= state->duty_cycle;
> -		if (val > UINT_MAX)
> -			state->period = UINT_MAX;
> -		else if (val)
> -			state->period = val;
> -		else
> -			state->period = 1;
> -	}

The patch looks good, the patch description could be a bit more verbose.
Something like:

	Calculate the period as

		($on + $off) / clkrate

	instead of

		$off / clkrate - $on / clkrate

	.

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 2/5] gpio: mvebu: improve pwm period calculation accuracy

[Uwe Kleine-König]

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

On Mon, Jan 11, 2021 at 01:17:03PM +0200, Baruch Siach wrote:
> Divide the full period value to reduce rounding error.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
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 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent

[Uwe Kleine-König]

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

On Mon, Jan 11, 2021 at 01:17:04PM +0200, Baruch Siach wrote:
> Round up the result of division in period/duty_cycle calculation to make
> the result closer to idempotent.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/gpio/gpio-mvebu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index c424d88e9e2b..6fc64846eda3 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -668,7 +668,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
>  
>  	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), &u);
>  	val = (unsigned long long) u * NSEC_PER_SEC;
> -	do_div(val, mvpwm->clk_rate);
> +	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
>  	if (val > UINT_MAX)
>  		state->duty_cycle = UINT_MAX;
>  	else if (val)
> @@ -680,7 +680,7 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
>  	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
>  	val += (unsigned long long) u; /* period = on + off duration */
>  	val *= NSEC_PER_SEC;
> -	do_div(val, mvpwm->clk_rate);
> +	val = DIV_ROUND_UP_ULL(val, mvpwm->clk_rate);
>  	if (val > UINT_MAX)
>  		state->period = UINT_MAX;
>  	else if (val)
> @@ -707,7 +707,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned int on, off;
>  
>  	val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
> -	do_div(val, NSEC_PER_SEC);
> +	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);

.apply must continue to round down.

Best regards
Uwe

>  	if (val > UINT_MAX)
>  		return -EINVAL;
>  	if (val)
> @@ -716,7 +716,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		on = 1;
>  
>  	val = (unsigned long long) mvpwm->clk_rate * state->period;
> -	do_div(val, NSEC_PER_SEC);
> +	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);
>  	val -= on;
>  	if (val > UINT_MAX)
>  		return -EINVAL;
> -- 
> 2.29.2
> 
> 

-- 
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 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX

[Uwe Kleine-König]

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

On Mon, Jan 11, 2021 at 01:17:05PM +0200, Baruch Siach wrote:
> PWM on/off registers are limited to UINT_MAX. However the state period
> and duty_cycle fields are ns values of type u64. There is no reason to
> limit them to UINT_MAX.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
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 5/5] gpio: mvebu: document zero pwm duty cycle limitation

[Uwe Kleine-König]

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

On Mon, Jan 11, 2021 at 01:17:06PM +0200, Baruch Siach wrote:
> Add a comment on why the code never sets the 'on' register to zero.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/gpio/gpio-mvebu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index eb7456fa6d86..4261e3b22b4e 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -706,6 +706,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);
>  	if (val > UINT_MAX)
>  		return -EINVAL;
> +	/* zero 'on' value does not work as expected for some reason */

What does the reference manual say about this? If there is no
information about this, please point this out, too. (Something like: The
reference manual is silent about this issue though.) Also I'd prefer to
read about the behaviour, so maybe mention that there is an occational
peek even when on is configured to 0. Does '$off = 0' has a symmetrical
issue?

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 5/5] gpio: mvebu: document zero pwm duty cycle limitation

[Russell King - ARM Linux admin]

On Mon, Jan 11, 2021 at 09:24:13PM +0100, Uwe Kleine-König wrote:
> On Mon, Jan 11, 2021 at 01:17:06PM +0200, Baruch Siach wrote:
> > Add a comment on why the code never sets the 'on' register to zero.
> > 
> > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  drivers/gpio/gpio-mvebu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > index eb7456fa6d86..4261e3b22b4e 100644
> > --- a/drivers/gpio/gpio-mvebu.c
> > +++ b/drivers/gpio/gpio-mvebu.c
> > @@ -706,6 +706,7 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	val = DIV_ROUND_UP_ULL(val, NSEC_PER_SEC);
> >  	if (val > UINT_MAX)
> >  		return -EINVAL;
> > +	/* zero 'on' value does not work as expected for some reason */
> 
> What does the reference manual say about this? If there is no
> information about this, please point this out, too. (Something like: The
> reference manual is silent about this issue though.) Also I'd prefer to
> read about the behaviour, so maybe mention that there is an occational
> peek even when on is configured to 0. Does '$off = 0' has a symmetrical
> issue?

It isn't a proper PWM block - it's documented as being a "blink
function". It contains two counters, one defines the "on" duration,
and the other defines the "off" duration.

The block is not well documented in the reference manual, so we have
to resort to experimentation - and experimentation reveals that if
we program both registers to zero, then we get about 17s on and 17s
off. That is 2^32 / 250MHz seconds. So, a value of 0 in either register
is interpreted by the hardware as a value of 2^32.

So, let's say we want a 25kHz signal. If we program the "on" duration
to 10000 and the "off" duration to 0, what we actually get a 40us
on duration, and a 17.2s off duration - resulting in a frequency of
0.058Hz!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation

[Baruch Siach]

Hi Uwe,

On Mon, Jan 11 2021, Uwe Kleine-König wrote:
> $Subject ~= s/get_state/.get_state/ ?

Ack.

> On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote:
>> The period is the sum of on and off values.
>> 
>> Reported-by: Russell King <linux@armlinux.org.uk>
>> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support")
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
>>  1 file changed, 8 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
>> index 672681a976f5..a912a8fed197 100644
>> --- a/drivers/gpio/gpio-mvebu.c
>> +++ b/drivers/gpio/gpio-mvebu.c
>> @@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
>>  	else
>>  		state->duty_cycle = 1;
>>  
>> +	val = (unsigned long long) u; /* on duration */
>>  	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
>> -	val = (unsigned long long) u * NSEC_PER_SEC;
>> +	val += (unsigned long long) u; /* period = on + off duration */
>> +	val *= NSEC_PER_SEC;
>>  	do_div(val, mvpwm->clk_rate);
>> -	if (val < state->duty_cycle) {
>> +	if (val > UINT_MAX)
>> +		state->period = UINT_MAX;
>> +	else if (val)
>> +		state->period = val;
>> +	else
>>  		state->period = 1;
>> -	} else {
>> -		val -= state->duty_cycle;
>> -		if (val > UINT_MAX)
>> -			state->period = UINT_MAX;
>> -		else if (val)
>> -			state->period = val;
>> -		else
>> -			state->period = 1;
>> -	}
>
> The patch looks good, the patch description could be a bit more verbose.
> Something like:
>
> 	Calculate the period as
>
> 		($on + $off) / clkrate
>
> 	instead of
>
> 		$off / clkrate - $on / clkrate
>
> 	.

I take this to refer to the next patch (2/5). This patch changes from
buggy

  $on / clkrate

to

  ($on + $off) / clkrate

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation

[Uwe Kleine-König]

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

On Wed, Jan 13, 2021 at 08:36:12AM +0200, Baruch Siach wrote:
> Hi Uwe,
> 
> On Mon, Jan 11 2021, Uwe Kleine-König wrote:
> > $Subject ~= s/get_state/.get_state/ ?
> 
> Ack.
> 
> > On Mon, Jan 11, 2021 at 01:17:02PM +0200, Baruch Siach wrote:
> >> The period is the sum of on and off values.
> >> 
> >> Reported-by: Russell King <linux@armlinux.org.uk>
> >> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support")
> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >> ---
> >>  drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
> >>  1 file changed, 8 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> >> index 672681a976f5..a912a8fed197 100644
> >> --- a/drivers/gpio/gpio-mvebu.c
> >> +++ b/drivers/gpio/gpio-mvebu.c
> >> @@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
> >>  	else
> >>  		state->duty_cycle = 1;
> >>  
> >> +	val = (unsigned long long) u; /* on duration */
> >>  	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
> >> -	val = (unsigned long long) u * NSEC_PER_SEC;
> >> +	val += (unsigned long long) u; /* period = on + off duration */
> >> +	val *= NSEC_PER_SEC;
> >>  	do_div(val, mvpwm->clk_rate);
> >> -	if (val < state->duty_cycle) {
> >> +	if (val > UINT_MAX)
> >> +		state->period = UINT_MAX;
> >> +	else if (val)
> >> +		state->period = val;
> >> +	else
> >>  		state->period = 1;
> >> -	} else {
> >> -		val -= state->duty_cycle;
> >> -		if (val > UINT_MAX)
> >> -			state->period = UINT_MAX;
> >> -		else if (val)
> >> -			state->period = val;
> >> -		else
> >> -			state->period = 1;
> >> -	}
> >
> > The patch looks good, the patch description could be a bit more verbose.
> > Something like:
> >
> > 	Calculate the period as
> >
> > 		($on + $off) / clkrate
> >
> > 	instead of
> >
> > 		$off / clkrate - $on / clkrate
> >
> > 	.
> 
> I take this to refer to the next patch (2/5). This patch changes from
> buggy
> 
>   $on / clkrate

No, the previous calculation had

-		val -= state->duty_cycle;

which accounts for "- $on / clkrate".

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