[PATCH] pwm: pca9685: Fix misuse of regmap_update_bits

[PATCH] pwm: pca9685: Fix misuse of regmap_update_bits

[Florian Vaussard]

Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
(mask & 1), which results in 0 if LSB of mask is 0. Thus the call
regmap_update_bits(..., mask, 1) is in reality equivalent to
regmap_update_bits(..., mask, 0).

In such a case, the correct use is regmap_update_bits(..., mask, mask).

This driver is performing such a mistake with the MODE1_RESTART mask,
which equals (1 << 6). Fix the driver to make it consistent with the
API. Please note that this change is untested, as I do not have this
piece of hardware. Testers are welcome!

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 drivers/pwm/pwm-pca9685.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 117fccf..6b9ff6c 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -124,7 +124,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			 */
 			if (duty_ns == pca->duty_ns) {
 				regmap_update_bits(pca->regmap, PCA9685_MODE1,
-						   MODE1_RESTART, 0x1);
+						   MODE1_RESTART,
+						   MODE1_RESTART);
 				return 0;
 			}
 		} else {
-- 
2.5.5

Re: [PATCH] pwm: pca9685: Fix misuse of regmap_update_bits

[Clemens Gruber]

On Tue, Nov 29, 2016 at 06:02:50PM +0100, Florian Vaussard wrote:
> Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
> and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
> (mask & 1), which results in 0 if LSB of mask is 0. Thus the call
> regmap_update_bits(..., mask, 1) is in reality equivalent to
> regmap_update_bits(..., mask, 0).
> 
> In such a case, the correct use is regmap_update_bits(..., mask, mask).
> 
> This driver is performing such a mistake with the MODE1_RESTART mask,
> which equals (1 << 6). Fix the driver to make it consistent with the
> API. Please note that this change is untested, as I do not have this
> piece of hardware. Testers are welcome!
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>  drivers/pwm/pwm-pca9685.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 117fccf..6b9ff6c 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -124,7 +124,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			 */
>  			if (duty_ns == pca->duty_ns) {
>  				regmap_update_bits(pca->regmap, PCA9685_MODE1,
> -						   MODE1_RESTART, 0x1);
> +						   MODE1_RESTART,
> +						   MODE1_RESTART);
>  				return 0;
>  			}
>  		} else {
> -- 
> 2.5.5

Good catch!
During testing your change however, I noticed that this whole
conditional for duty_ns == pca->duty_ns (which I added) is bogus:
Restarting the chip means using the same ON and OFF times as before, so
the duty cycle "ratio" stays the same, relative to the period.
Here we are checking for an equal duty cycle in nanoseconds though..

Instead we would have to check if the ratio changed and only if it did
not, set the RESTART bit.

Or we could just remove that conditional. This is only an optimization
for the special case of changing both period_ns and duty_ns at the same
time but with the same ratio as before.

Clemens

Re: [PATCH] pwm: pca9685: Fix misuse of regmap_update_bits

[Thierry Reding]

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

On Mon, Dec 05, 2016 at 05:34:02PM +0100, Clemens Gruber wrote:
> On Tue, Nov 29, 2016 at 06:02:50PM +0100, Florian Vaussard wrote:
> > Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
> > and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
> > (mask & 1), which results in 0 if LSB of mask is 0. Thus the call
> > regmap_update_bits(..., mask, 1) is in reality equivalent to
> > regmap_update_bits(..., mask, 0).
> > 
> > In such a case, the correct use is regmap_update_bits(..., mask, mask).
> > 
> > This driver is performing such a mistake with the MODE1_RESTART mask,
> > which equals (1 << 6). Fix the driver to make it consistent with the
> > API. Please note that this change is untested, as I do not have this
> > piece of hardware. Testers are welcome!
> > 
> > Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 117fccf..6b9ff6c 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -124,7 +124,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			 */
> >  			if (duty_ns == pca->duty_ns) {
> >  				regmap_update_bits(pca->regmap, PCA9685_MODE1,
> > -						   MODE1_RESTART, 0x1);
> > +						   MODE1_RESTART,
> > +						   MODE1_RESTART);
> >  				return 0;
> >  			}
> >  		} else {
> > -- 
> > 2.5.5
> 
> Good catch!
> During testing your change however, I noticed that this whole
> conditional for duty_ns == pca->duty_ns (which I added) is bogus:
> Restarting the chip means using the same ON and OFF times as before, so
> the duty cycle "ratio" stays the same, relative to the period.
> Here we are checking for an equal duty cycle in nanoseconds though..
> 
> Instead we would have to check if the ratio changed and only if it did
> not, set the RESTART bit.
> 
> Or we could just remove that conditional. This is only an optimization
> for the special case of changing both period_ns and duty_ns at the same
> time but with the same ratio as before.

So what's the status on this?

Thierry

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

Re: [PATCH] pwm: pca9685: Fix misuse of regmap_update_bits

[Thierry Reding]

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

On Wed, Jan 18, 2017 at 11:55:10AM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 05:34:02PM +0100, Clemens Gruber wrote:
> > On Tue, Nov 29, 2016 at 06:02:50PM +0100, Florian Vaussard wrote:
> > > Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
> > > and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
> > > (mask & 1), which results in 0 if LSB of mask is 0. Thus the call
> > > regmap_update_bits(..., mask, 1) is in reality equivalent to
> > > regmap_update_bits(..., mask, 0).
> > > 
> > > In such a case, the correct use is regmap_update_bits(..., mask, mask).
> > > 
> > > This driver is performing such a mistake with the MODE1_RESTART mask,
> > > which equals (1 << 6). Fix the driver to make it consistent with the
> > > API. Please note that this change is untested, as I do not have this
> > > piece of hardware. Testers are welcome!
> > > 
> > > Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> > > ---
> > >  drivers/pwm/pwm-pca9685.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 117fccf..6b9ff6c 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -124,7 +124,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  			 */
> > >  			if (duty_ns == pca->duty_ns) {
> > >  				regmap_update_bits(pca->regmap, PCA9685_MODE1,
> > > -						   MODE1_RESTART, 0x1);
> > > +						   MODE1_RESTART,
> > > +						   MODE1_RESTART);
> > >  				return 0;
> > >  			}
> > >  		} else {
> > > -- 
> > > 2.5.5
> > 
> > Good catch!
> > During testing your change however, I noticed that this whole
> > conditional for duty_ns == pca->duty_ns (which I added) is bogus:
> > Restarting the chip means using the same ON and OFF times as before, so
> > the duty cycle "ratio" stays the same, relative to the period.
> > Here we are checking for an equal duty cycle in nanoseconds though..
> > 
> > Instead we would have to check if the ratio changed and only if it did
> > not, set the RESTART bit.
> > 
> > Or we could just remove that conditional. This is only an optimization
> > for the special case of changing both period_ns and duty_ns at the same
> > time but with the same ratio as before.
> 
> So what's the status on this?

Oh wait, that's what these:

	http://patchwork.ozlabs.org/patch/705438/
	http://patchwork.ozlabs.org/patch/705437/

are fixing, right?

Florian, I take it there's no reason to apply this patch anymore given
that Clemens' patch removes this hunk and replaces it with a proper fix
for this particular driver?

Thierry

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

Re: [PATCH] pwm: pca9685: Fix misuse of regmap_update_bits

[Clemens Gruber]

On Wed, Jan 18, 2017 at 12:00:32PM +0100, Thierry Reding wrote:
> On Wed, Jan 18, 2017 at 11:55:10AM +0100, Thierry Reding wrote:
> > On Mon, Dec 05, 2016 at 05:34:02PM +0100, Clemens Gruber wrote:
> > > On Tue, Nov 29, 2016 at 06:02:50PM +0100, Florian Vaussard wrote:
> > > > Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
> > > > and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
> > > > (mask & 1), which results in 0 if LSB of mask is 0. Thus the call
> > > > regmap_update_bits(..., mask, 1) is in reality equivalent to
> > > > regmap_update_bits(..., mask, 0).
> > > > 
> > > > In such a case, the correct use is regmap_update_bits(..., mask, mask).
> > > > 
> > > > This driver is performing such a mistake with the MODE1_RESTART mask,
> > > > which equals (1 << 6). Fix the driver to make it consistent with the
> > > > API. Please note that this change is untested, as I do not have this
> > > > piece of hardware. Testers are welcome!
> > > > 
> > > > Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> > > > ---
> > > >  drivers/pwm/pwm-pca9685.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > index 117fccf..6b9ff6c 100644
> > > > --- a/drivers/pwm/pwm-pca9685.c
> > > > +++ b/drivers/pwm/pwm-pca9685.c
> > > > @@ -124,7 +124,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  			 */
> > > >  			if (duty_ns == pca->duty_ns) {
> > > >  				regmap_update_bits(pca->regmap, PCA9685_MODE1,
> > > > -						   MODE1_RESTART, 0x1);
> > > > +						   MODE1_RESTART,
> > > > +						   MODE1_RESTART);
> > > >  				return 0;
> > > >  			}
> > > >  		} else {
> > > > -- 
> > > > 2.5.5
> > > 
> > > Good catch!
> > > During testing your change however, I noticed that this whole
> > > conditional for duty_ns == pca->duty_ns (which I added) is bogus:
> > > Restarting the chip means using the same ON and OFF times as before, so
> > > the duty cycle "ratio" stays the same, relative to the period.
> > > Here we are checking for an equal duty cycle in nanoseconds though..
> > > 
> > > Instead we would have to check if the ratio changed and only if it did
> > > not, set the RESTART bit.
> > > 
> > > Or we could just remove that conditional. This is only an optimization
> > > for the special case of changing both period_ns and duty_ns at the same
> > > time but with the same ratio as before.
> > 
> > So what's the status on this?
> 
> Oh wait, that's what these:
> 
> 	http://patchwork.ozlabs.org/patch/705438/
> 	http://patchwork.ozlabs.org/patch/705437/
> 
> are fixing, right?

Yes, this is what the first patch of the series is fixing.

The second one corrects the invalid expectation that the period is
always set to 1/200 Hz after boot.

Thanks,
Clemens