[PATCH] Input: pm8xxx-vib - fix handling of separate enable register

[PATCH] Input: pm8xxx-vib - fix handling of separate enable register

[Stephan Gerhold]

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);
 
 	return rc;
 }
-- 
2.24.0

Re: [PATCH] Input: pm8xxx-vib - fix handling of separate enable register

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] Input: pm8xxx-vib - fix handling of separate enable register

[Stephan Gerhold]

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