| Message ID | 20191211190026.52662-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: pm8xxx-vib - fix handling of separate enable register | expand |
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.
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
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; }
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(-)