| Message ID | d1d7d548eba25119397de87211de763c54b5d8cd.1611651611.git.baruch@tkos.co.il |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [5.10-stable] gpio: mvebu: fix pwm .get_state period calculation | expand |
On Tue, Jan 26, 2021 at 11:00:11AM +0200, Baruch Siach wrote: > commit e73b0101ae5124bf7cd3fb5d250302ad2f16a416 upstream. > > The period is the sum of on and off values. That is, calculate period as > > ($on + $off) / clkrate > > instead of > > $off / clkrate - $on / clkrate > > that makes no sense. > > Reported-by: Russell King <linux@armlinux.org.uk> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support") > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Doesn't this need something like: [baruch: Backported to 5.10] plus a new S-o-B? > --- > > This backported patch applies to all kernels between 4.14 and 5.10 > (inclusive). > --- > drivers/gpio/gpio-mvebu.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index 2f245594a90a..ed7c5fc47f52 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -660,9 +660,8 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, > > spin_lock_irqsave(&mvpwm->lock, flags); > > - val = (unsigned long long) > - readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm)); > - val *= NSEC_PER_SEC; > + u = readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm)); > + val = (unsigned long long) u * NSEC_PER_SEC; > do_div(val, mvpwm->clk_rate); > if (val > UINT_MAX) > state->duty_cycle = UINT_MAX; > @@ -671,21 +670,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, > else > state->duty_cycle = 1; > > - val = (unsigned long long) > - readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm)); > + val = (unsigned long long) u; /* on duration */ > + /* period = on + off duration */ > + val += readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm)); > 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) Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks Uwe
Hi Uwe, Thanks for your review. On Tue, Jan 26 2021, Uwe Kleine-König wrote: > On Tue, Jan 26, 2021 at 11:00:11AM +0200, Baruch Siach wrote: >> commit e73b0101ae5124bf7cd3fb5d250302ad2f16a416 upstream. >> >> The period is the sum of on and off values. That is, calculate period as >> >> ($on + $off) / clkrate >> >> instead of >> >> $off / clkrate - $on / clkrate >> >> that makes no sense. >> >> Reported-by: Russell King <linux@armlinux.org.uk> >> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support") >> Signed-off-by: Baruch Siach <baruch@tkos.co.il> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Doesn't this need something like: > > [baruch: Backported to 5.10] > > plus a new S-o-B? I could not find this requirement in the stable-kernel-rules.rst (Option 3) text. Greg, should I resend the patch with another SoB? Thanks, baruch -- ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
On Wed, Jan 27, 2021 at 03:06:55PM +0200, Baruch Siach wrote: > Hi Uwe, > > Thanks for your review. > > On Tue, Jan 26 2021, Uwe Kleine-König wrote: > > On Tue, Jan 26, 2021 at 11:00:11AM +0200, Baruch Siach wrote: > >> commit e73b0101ae5124bf7cd3fb5d250302ad2f16a416 upstream. > >> > >> The period is the sum of on and off values. That is, calculate period as > >> > >> ($on + $off) / clkrate > >> > >> instead of > >> > >> $off / clkrate - $on / clkrate > >> > >> that makes no sense. > >> > >> Reported-by: Russell King <linux@armlinux.org.uk> > >> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > >> Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support") > >> Signed-off-by: Baruch Siach <baruch@tkos.co.il> > >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Doesn't this need something like: > > > > [baruch: Backported to 5.10] > > > > plus a new S-o-B? > > I could not find this requirement in the stable-kernel-rules.rst (Option > 3) text. > > Greg, should I resend the patch with another SoB? Please do! thanks, greg k-h
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c index 2f245594a90a..ed7c5fc47f52 100644 --- a/drivers/gpio/gpio-mvebu.c +++ b/drivers/gpio/gpio-mvebu.c @@ -660,9 +660,8 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, spin_lock_irqsave(&mvpwm->lock, flags); - val = (unsigned long long) - readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm)); - val *= NSEC_PER_SEC; + u = readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm)); + val = (unsigned long long) u * NSEC_PER_SEC; do_div(val, mvpwm->clk_rate); if (val > UINT_MAX) state->duty_cycle = UINT_MAX; @@ -671,21 +670,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip, else state->duty_cycle = 1; - val = (unsigned long long) - readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm)); + val = (unsigned long long) u; /* on duration */ + /* period = on + off duration */ + val += readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm)); 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)