[2/3] pwm: stm32: Remove confusing bitmask
diff mbox series

Message ID 20191016073842.1300297-3-thierry.reding@gmail.com
State New
Headers show
Series
  • pwm: stm32: Minor cleanups
Related show

Commit Message

Thierry Reding Oct. 16, 2019, 7:38 a.m. UTC
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(-)

Comments

Uwe Kleine-König Oct. 16, 2019, 8:31 a.m. UTC | #1
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);

Best regards
Uwe
Thierry Reding Oct. 16, 2019, 9:50 a.m. UTC | #2
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
Fabrice Gasnier Oct. 16, 2019, 10:20 a.m. UTC | #3
On 10/16/19 11:50 AM, Thierry Reding wrote:
> 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?

Hi Thierry,

No objection from me, it will probably improve readability.

I'd just suggest an alternative to this: maybe a simple struct array
with two entries can improve readability ? E.g. keep the defines
matching the datasheet, and get rid of the conditional code ?

Dirty proposal:

static const struct stm32_pwm_brk[] = {
	/* {bke, bkp, shift, mask} */
	{ TIM_BDTR_BKE, TIM_BDTR_BKP, ...},
	{ TIM_BDTR_BK2E, TIM_BDTR_BK2P, ...},
}

and use "index" to index it ?

But I'm fine with the macros as well: there's already similar things in
this driver to deal with the channels for instance.

Thanks,
Fabrice
> 
> Thierry
>
Uwe Kleine-König Oct. 16, 2019, 10:52 a.m. UTC | #4
On Wed, Oct 16, 2019 at 12:20:17PM +0200, Fabrice Gasnier wrote:
> On 10/16/19 11:50 AM, Thierry Reding wrote:
> > 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?
> 
> Hi Thierry,
> 
> No objection from me, it will probably improve readability.
> 
> I'd just suggest an alternative to this: maybe a simple struct array
> with two entries can improve readability ? E.g. keep the defines
> matching the datasheet, and get rid of the conditional code ?
> 
> Dirty proposal:
> 
> static const struct stm32_pwm_brk[] = {
> 	/* {bke, bkp, shift, mask} */
> 	{ TIM_BDTR_BKE, TIM_BDTR_BKP, ...},
> 	{ TIM_BDTR_BK2E, TIM_BDTR_BK2P, ...},
> }
> 
> and use "index" to index it ?
> 
> But I'm fine with the macros as well: there's already similar things in
> this driver to deal with the channels for instance.

I didn't test but I wouldn't be surprised if the compiler could better
optimize the solution I suggested. Might be worth a test however.

Best regards
Uwe

Patch
diff mbox series

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;
 	}
 
 	bdtr = bke;
 
-	/*
-	 * The both bits could be set since only one will be wrote
-	 * due to mask value.
-	 */
 	if (level)
-		bdtr |= TIM_BDTR_BKP | TIM_BDTR_BK2P;
+		bdtr |= bkp;
 
 	bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift;