Input: pm8xxx-vib - fix handling of separate enable register
diff mbox series

Message ID 20191211190026.52662-1-stephan@gerhold.net
State Superseded
Headers show
Series
  • Input: pm8xxx-vib - fix handling of separate enable register
Related show

Commit Message

Stephan Gerhold Dec. 11, 2019, 7 p.m. UTC
Setting the vibrator enable_mask is not implemented correctly:

For regmap_update_bits(map, reg, mask, val) we give in either
regs->enable_mask or 0 (= no-op) as mask and "val" as value.
But "val" actually refers to the vibrator voltage control register,
which has nothing to do with the enable_mask.

So we usually end up doing nothing when we really wanted
to enable the vibrator.

We want to set or clear the enable_mask (to enable/disable the vibrator).
Therefore, change the call to always modify the enable_mask
and set the bits only if we want to enable the vibrator.

Cc: Damien Riegel <damien.riegel@savoirfairelinux.com>
Fixes: d4c7c5c96c92 ("Input: pm8xxx-vib - handle separate enable register")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/input/misc/pm8xxx-vibrator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Dmitry Torokhov Dec. 12, 2019, 7:50 p.m. UTC | #1
On Wed, Dec 11, 2019 at 08:00:26PM +0100, Stephan Gerhold wrote:
> Setting the vibrator enable_mask is not implemented correctly:
> 
> For regmap_update_bits(map, reg, mask, val) we give in either
> regs->enable_mask or 0 (= no-op) as mask and "val" as value.
> But "val" actually refers to the vibrator voltage control register,
> which has nothing to do with the enable_mask.
> 
> So we usually end up doing nothing when we really wanted
> to enable the vibrator.
> 
> We want to set or clear the enable_mask (to enable/disable the vibrator).
> Therefore, change the call to always modify the enable_mask
> and set the bits only if we want to enable the vibrator.
> 
> Cc: Damien Riegel <damien.riegel@savoirfairelinux.com>
> Fixes: d4c7c5c96c92 ("Input: pm8xxx-vib - handle separate enable register")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  drivers/input/misc/pm8xxx-vibrator.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index ecd762f93732..8dc345604a4d 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -90,7 +90,8 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>  
>  	if (regs->enable_mask)
>  		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> -					on ? regs->enable_mask : 0, val);
> +					regs->enable_mask,
> +					on ? regs->enable_mask : 0);

Would it be even clearer to say

		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
					regs->enable_mask, on ? ~0 : 0);

?

Thanks.
Stephan Gerhold Dec. 12, 2019, 9:38 p.m. UTC | #2
On Thu, Dec 12, 2019 at 11:50:31AM -0800, Dmitry Torokhov wrote:
> On Wed, Dec 11, 2019 at 08:00:26PM +0100, Stephan Gerhold wrote:
> > Setting the vibrator enable_mask is not implemented correctly:
> > 
> > For regmap_update_bits(map, reg, mask, val) we give in either
> > regs->enable_mask or 0 (= no-op) as mask and "val" as value.
> > But "val" actually refers to the vibrator voltage control register,
> > which has nothing to do with the enable_mask.
> > 
> > So we usually end up doing nothing when we really wanted
> > to enable the vibrator.
> > 
> > We want to set or clear the enable_mask (to enable/disable the vibrator).
> > Therefore, change the call to always modify the enable_mask
> > and set the bits only if we want to enable the vibrator.
> > 
> > Cc: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > Fixes: d4c7c5c96c92 ("Input: pm8xxx-vib - handle separate enable register")
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  drivers/input/misc/pm8xxx-vibrator.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> > index ecd762f93732..8dc345604a4d 100644
> > --- a/drivers/input/misc/pm8xxx-vibrator.c
> > +++ b/drivers/input/misc/pm8xxx-vibrator.c
> > @@ -90,7 +90,8 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >  
> >  	if (regs->enable_mask)
> >  		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> > -					on ? regs->enable_mask : 0, val);
> > +					regs->enable_mask,
> > +					on ? regs->enable_mask : 0);
> 
> Would it be even clearer to say
> 
> 		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> 					regs->enable_mask, on ? ~0 : 0);
> 
> ?

Functionally it would be equivalent.
I think I considered it when writing the patch, but in my opinion
it does not make the code more readable. We never want to set more than
the bits in the mask, so there is no reason to set them in the value.

But I can change it if you would prefer having ~0. Just let me know!

Thanks,
Stephan

Patch
diff mbox series

diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index ecd762f93732..8dc345604a4d 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -90,7 +90,8 @@  static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
 
 	if (regs->enable_mask)
 		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
-					on ? regs->enable_mask : 0, val);
+					regs->enable_mask,
+					on ? regs->enable_mask : 0);
 
 	return rc;
 }