Re: [PATCH 2/3] pwm: stm32: Remove confusing bitmask

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, Oct 16, 2019 at 10:31:07AM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 16, 2019 at 09:38:41AM +0200, Thierry Reding wrote:
> > Both BKP bits are set in the BDTR register and the code relies on the
> > mask used during write to make sure only one of them is written. Since
> > this isn't immediately obvious, a comment is needed to explain it. The
> > same can be achieved by making explicit what happens, so add another
> > temporary variable that contains only the one bit that is actually ORed
> > into the register and get rid of the comment.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/pwm-stm32.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index b12fb11b7a55..8f1f3371e1dd 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -493,26 +493,24 @@ static const struct pwm_ops stm32pwm_ops = {
> >  static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
> >  				    int index, int level, int filter)
> >  {
> > -	u32 bke, shift, mask, bdtr;
> > +	u32 bke, bkp, shift, mask, bdtr;
> >  
> >  	if (index == 0) {
> >  		bke = TIM_BDTR_BKE;
> > +		bkp = TIM_BDTR_BKP;
> >  		shift = TIM_BDTR_BKF_SHIFT;
> >  		mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF;
> >  	} else {
> >  		bke = TIM_BDTR_BK2E;
> > +		bkp = TIM_BDTR_BK2P;
> >  		shift = TIM_BDTR_BK2F_SHIFT;
> >  		mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F;
> 
> Assuming in the else branch index is always 1, the following would be
> IMHO nicer:
> 
> #define TIM_BDTR_BKE(i) BIT(12 + 12 * (i))
> #define TIM_BDTR_BKP(i) BIT(13 + 12 * (i))
> #define TIM_BDTR_BKF_SHIFT(i) (16 + 4 * (i))
> 
> ..
> 
> 	bke = TIM_BDTR_BKE(index);
> 	bkp = TIM_BDTR_BKP(index);

I had thought about that, but ultimately decided against it because
the original defines might match exactly what's in the datasheet, so
there's some value to keep the originals.

I suppose one other alternative would be to let the macros be and do the
computations in the driver instead, something like:

	bke = TIM_BDTR_BKE << (index * 12);
	bkp = TIM_BDTR_BKP << (index * 12);
	bkf = TIM_BDTR_BKF << (index *  4);

But yeah, I agree that having the parameters be part of the macros is
even better.

Fabrice, any objection to redefining the macros as above?

Thierry

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