Re: [PATCH] pwm: iqs620a: Replace one remaining instance of regmap_update_bits()

[Jeff LaBundy <jeff@labundy.com>]

Hi Uwe,

Thank you for taking a look!

On Tue, Dec 27, 2022 at 10:02:53AM +0100, Uwe Kleine-König wrote:
> Hello Jeff,
> 
> On Mon, Dec 26, 2022 at 11:02:41PM -0600, Jeff LaBundy wrote:
> > The call to regmap_update_bits() which was responsible for clearing
> > the PWM output enable register bit was recently dropped in favor of
> > a call to regmap_clear_bits(), thereby simplifying the code.
> > 
> > Similarly, the call to regmap_update_bits() which sets the same bit
> > can be simplified with a call to regmap_set_bits().
> > 
> > Fixes: 2c85895bf3d2 ("pwm: iqs620a: Use regmap_clear_bits and regmap_set_bits where applicable")
> 
> I wouldn't call this a fix. This trailer triggers the stable guys to
> backport the commit for stable. I wouldn't support that.

My naive assumption was this commit would get ignored by stable for the
foreseeable future, since the original commit itself is not yet in any
stable trees. Rather, the intent was to complete the good intentions of
a recent commit and provide some context.

That being said, there is no functional change or bug to be addressed,
so the argument to not call this a fix is valid as well. I will update
the commit message.

> 
> > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > ---
> >  drivers/pwm/pwm-iqs620a.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> > index 4987ca940b64..8362b4870c66 100644
> > --- a/drivers/pwm/pwm-iqs620a.c
> > +++ b/drivers/pwm/pwm-iqs620a.c
> > @@ -55,8 +55,8 @@ static int iqs620_pwm_init(struct iqs620_pwm_private *iqs620_pwm,
> >  	if (ret)
> >  		return ret;
> >  
> > -	return regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> > -				  IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
> 
> This is strange, because val has more bits set than mask
> (IQS620_PWR_SETTINGS_PWM_OUT == 0x80).

Right, the intent was to make it blatantly obvious that all masked bits
are to be set, prior to being aware that regmap_set_bits() was recently
introduced at the time. Both are functionally equivalent to each other
as well as regmap_update_bits(regmap, reg, mask, ~0), the last of which
still exists in a handful of places. Granted I'm responsible for a few
of them ;)

> 
> > +	return regmap_set_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
> > +			       IQS620_PWR_SETTINGS_PWM_OUT);
> >  }
> 
> The change looks fine however.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Kind regards,
Jeff LaBundy