* [PATCH 0/5] gpio: mvebu: pwm fixes and improvements
@ 2021-01-11 11:17 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Andrew Lunn, Baruch Siach, linux-pwm, Gregory Clement,
Russell King, linux-gpio, Chris Packham, Thomas Petazzoni,
Ralph Sennhauser, Sascha Hauer, linux-arm-kernel,
Sebastian Hesselbarth
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 11:17 ` Baruch Siach
-1 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Baruch Siach, Russell King, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
@ 2021-01-11 11:17 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Andrew Lunn, Baruch Siach, linux-pwm, Gregory Clement,
Russell King, linux-gpio, Chris Packham, Thomas Petazzoni,
Ralph Sennhauser, Sascha Hauer, linux-arm-kernel,
Sebastian Hesselbarth
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 20:17 ` Uwe Kleine-König
-1 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:17 UTC (permalink / raw)
To: Baruch Siach, g
Cc: Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Russell King, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
@ 2021-01-11 20:17 ` Uwe Kleine-König
0 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:17 UTC (permalink / raw)
To: Baruch Siach, g
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, Thierry Reding,
Thomas Petazzoni, linux-gpio, Ralph Sennhauser, Lee Jones,
Gregory Clement, linux-arm-kernel, Sebastian Hesselbarth
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
2021-01-11 20:17 ` Uwe Kleine-König
@ 2021-01-13 6:36 ` Baruch Siach
-1 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-13 6:36 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: g, Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Russell King, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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 -
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
@ 2021-01-13 6:36 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-13 6:36 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, g,
Thierry Reding, Thomas Petazzoni, linux-gpio, Ralph Sennhauser,
Lee Jones, Gregory Clement, linux-arm-kernel,
Sebastian Hesselbarth
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 -
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
2021-01-13 6:36 ` Baruch Siach
@ 2021-01-13 8:17 ` Uwe Kleine-König
-1 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-13 8:17 UTC (permalink / raw)
To: Baruch Siach
Cc: g, Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Russell King, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] gpio: mvebu: fix pwm get_state period calculation
@ 2021-01-13 8:17 ` Uwe Kleine-König
0 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-13 8:17 UTC (permalink / raw)
To: Baruch Siach
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, g,
Thierry Reding, Thomas Petazzoni, linux-gpio, Ralph Sennhauser,
Lee Jones, Gregory Clement, linux-arm-kernel,
Sebastian Hesselbarth
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/5] gpio: mvebu: improve pwm period calculation accuracy
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 11:17 ` Baruch Siach
-1 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Baruch Siach, Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/5] gpio: mvebu: improve pwm period calculation accuracy
@ 2021-01-11 11:17 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Andrew Lunn, Baruch Siach, linux-pwm, Gregory Clement,
Russell King, linux-gpio, Chris Packham, Thomas Petazzoni,
Ralph Sennhauser, Sascha Hauer, linux-arm-kernel,
Sebastian Hesselbarth
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/5] gpio: mvebu: improve pwm period calculation accuracy
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 20:18 ` Uwe Kleine-König
-1 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:18 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/5] gpio: mvebu: improve pwm period calculation accuracy
@ 2021-01-11 20:18 ` Uwe Kleine-König
0 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:18 UTC (permalink / raw)
To: Baruch Siach
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, Thierry Reding,
Thomas Petazzoni, linux-gpio, Ralph Sennhauser, Lee Jones,
Gregory Clement, linux-arm-kernel, Sebastian Hesselbarth
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 11:17 ` Baruch Siach
-1 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Baruch Siach, Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent
@ 2021-01-11 11:17 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Andrew Lunn, Baruch Siach, linux-pwm, Gregory Clement,
Russell King, linux-gpio, Chris Packham, Thomas Petazzoni,
Ralph Sennhauser, Sascha Hauer, linux-arm-kernel,
Sebastian Hesselbarth
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 20:20 ` Uwe Kleine-König
-1 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:20 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] gpio: mvebu: make pwm apply/get_state closer to idempotent
@ 2021-01-11 20:20 ` Uwe Kleine-König
0 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:20 UTC (permalink / raw)
To: Baruch Siach
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, Thierry Reding,
Thomas Petazzoni, linux-gpio, Ralph Sennhauser, Lee Jones,
Gregory Clement, linux-arm-kernel, Sebastian Hesselbarth
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 11:17 ` Baruch Siach
-1 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Baruch Siach, Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
@ 2021-01-11 11:17 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Andrew Lunn, Baruch Siach, linux-pwm, Gregory Clement,
Russell King, linux-gpio, Chris Packham, Thomas Petazzoni,
Ralph Sennhauser, Sascha Hauer, linux-arm-kernel,
Sebastian Hesselbarth
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 20:21 ` Uwe Kleine-König
-1 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:21 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
@ 2021-01-11 20:21 ` Uwe Kleine-König
0 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:21 UTC (permalink / raw)
To: Baruch Siach
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, Thierry Reding,
Thomas Petazzoni, linux-gpio, Ralph Sennhauser, Lee Jones,
Gregory Clement, linux-arm-kernel, Sebastian Hesselbarth
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 11:17 ` Baruch Siach
-1 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Baruch Siach, Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation
@ 2021-01-11 11:17 ` Baruch Siach
0 siblings, 0 replies; 28+ messages in thread
From: Baruch Siach @ 2021-01-11 11:17 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones, Linus Walleij,
Bartosz Golaszewski
Cc: Andrew Lunn, Baruch Siach, linux-pwm, Gregory Clement,
Russell King, linux-gpio, Chris Packham, Thomas Petazzoni,
Ralph Sennhauser, Sascha Hauer, linux-arm-kernel,
Sebastian Hesselbarth
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation
2021-01-11 11:17 ` Baruch Siach
@ 2021-01-11 20:24 ` Uwe Kleine-König
-1 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:24 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Lee Jones, Linus Walleij, Bartosz Golaszewski,
Andrew Lunn, Gregory Clement, Russell King,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation
@ 2021-01-11 20:24 ` Uwe Kleine-König
0 siblings, 0 replies; 28+ messages in thread
From: Uwe Kleine-König @ 2021-01-11 20:24 UTC (permalink / raw)
To: Baruch Siach
Cc: Andrew Lunn, Sascha Hauer, linux-pwm, Linus Walleij,
Chris Packham, Russell King, Bartosz Golaszewski, Thierry Reding,
Thomas Petazzoni, linux-gpio, Ralph Sennhauser, Lee Jones,
Gregory Clement, linux-arm-kernel, Sebastian Hesselbarth
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation
2021-01-11 20:24 ` Uwe Kleine-König
@ 2021-01-11 22:43 ` Russell King - ARM Linux admin
-1 siblings, 0 replies; 28+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-11 22:43 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Baruch Siach, Thierry Reding, Lee Jones, Linus Walleij,
Bartosz Golaszewski, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Thomas Petazzoni, Chris Packham,
Sascha Hauer, Ralph Sennhauser, linux-pwm, linux-gpio,
linux-arm-kernel
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!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] gpio: mvebu: document zero pwm duty cycle limitation
@ 2021-01-11 22:43 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 28+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-11 22:43 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Lunn, Baruch Siach, Sascha Hauer, linux-pwm,
Linus Walleij, Chris Packham, Bartosz Golaszewski,
Thierry Reding, Thomas Petazzoni, linux-gpio, Ralph Sennhauser,
Lee Jones, Gregory Clement, linux-arm-kernel,
Sebastian Hesselbarth
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!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread