[PATCH 0/3] pwm: stm32: Minor cleanups

[PATCH 0/3] pwm: stm32: Minor cleanups

[Thierry Reding]

Hi,

Looking at Fabrice's STM32 patches I noticed that we're now passing the
breakinput values (u32) into a function via int parameters. The easiest
way to fix this inconsistency is by just passing a pointer to the break
input structure. There's some preparatory work here that makes the code
slightly more readable, in my opinion, but it's really marginal, so I'm
not terribly thrilled by this series in retrospect.

If nobody else thinks this is a big improvement I'll just scrap it.

Thierry

Thierry Reding (3):
  pwm: stm32: Remove clutter from ternary operator
  pwm: stm32: Remove confusing bitmask
  pwm: stm32: Pass breakinput instead of its values

 drivers/pwm/pwm-stm32.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

-- 
2.23.0

[PATCH 1/3] pwm: stm32: Remove clutter from ternary operator

[Thierry Reding]

Remove usage of the ternary operator to assign values for register
fields. This removes clutter and improves readability.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-stm32.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 9430b4cd383f..b12fb11b7a55 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -493,11 +493,19 @@ 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 = (index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E;
-	int shift = (index == 0) ? TIM_BDTR_BKF_SHIFT : TIM_BDTR_BK2F_SHIFT;
-	u32 mask = (index == 0) ? TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF
-				: TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F;
-	u32 bdtr = bke;
+	u32 bke, shift, mask, bdtr;
+
+	if (index == 0) {
+		bke = TIM_BDTR_BKE;
+		shift = TIM_BDTR_BKF_SHIFT;
+		mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF;
+	} else {
+		bke = TIM_BDTR_BK2E;
+		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
-- 
2.23.0

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

[Thierry Reding]

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;
 	}
 
 	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;
 
-- 
2.23.0

[PATCH 3/3] pwm: stm32: Pass breakinput instead of its values

[Thierry Reding]

Instead of passing the individual values of the breakpoint, pass a
pointer to the breakpoint.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-stm32.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 8f1f3371e1dd..9e500928c37d 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -491,11 +491,11 @@ static const struct pwm_ops stm32pwm_ops = {
 };
 
 static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
-				    int index, int level, int filter)
+				    const struct stm32_breakinput *bi)
 {
 	u32 bke, bkp, shift, mask, bdtr;
 
-	if (index == 0) {
+	if (bi->index == 0) {
 		bke = TIM_BDTR_BKE;
 		bkp = TIM_BDTR_BKP;
 		shift = TIM_BDTR_BKF_SHIFT;
@@ -509,10 +509,10 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
 
 	bdtr = bke;
 
-	if (level)
+	if (bi->level)
 		bdtr |= bkp;
 
-	bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift;
+	bdtr |= (bi->filter & TIM_BDTR_BKF_MASK) << shift;
 
 	regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr);
 
@@ -527,10 +527,7 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
 	int ret;
 
 	for (i = 0; i < priv->num_breakinputs; i++) {
-		ret = stm32_pwm_set_breakinput(priv,
-					       priv->breakinputs[i].index,
-					       priv->breakinputs[i].level,
-					       priv->breakinputs[i].filter);
+		ret = stm32_pwm_set_breakinput(priv, &priv->breakinputs[i]);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.23.0

Re: [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator

[Uwe Kleine-König]

On Wed, Oct 16, 2019 at 09:38:40AM +0200, Thierry Reding wrote:
> Remove usage of the ternary operator to assign values for register
> fields. This removes clutter and improves readability.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/pwm-stm32.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 9430b4cd383f..b12fb11b7a55 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -493,11 +493,19 @@ 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 = (index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E;
> -	int shift = (index == 0) ? TIM_BDTR_BKF_SHIFT : TIM_BDTR_BK2F_SHIFT;
> -	u32 mask = (index == 0) ? TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF
> -				: TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F;
> -	u32 bdtr = bke;
> +	u32 bke, shift, mask, bdtr;
> +
> +	if (index == 0) {
> +		bke = TIM_BDTR_BKE;
> +		shift = TIM_BDTR_BKF_SHIFT;
> +		mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF;
> +	} else {
> +		bke = TIM_BDTR_BK2E;
> +		shift = TIM_BDTR_BK2F_SHIFT;
> +		mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F;
> +	}
> +
> +	bdtr = bke;

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Is index always in {0, 1}? Maybe a comment or a check about that would
be helpful. (-> separate patch I think).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 0/3] pwm: stm32: Minor cleanups

[Uwe Kleine-König]

On Wed, Oct 16, 2019 at 09:38:39AM +0200, Thierry Reding wrote:
> Hi,
> 
> Looking at Fabrice's STM32 patches I noticed that we're now passing the
> breakinput values (u32) into a function via int parameters. The easiest
> way to fix this inconsistency is by just passing a pointer to the break
> input structure. There's some preparatory work here that makes the code
> slightly more readable, in my opinion, but it's really marginal, so I'm
> not terribly thrilled by this series in retrospect.
> 
> If nobody else thinks this is a big improvement I'll just scrap it.

I like it. Together with my suggestion to add parameters to the register
offsets I think it's worth the effort.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator

[Thierry Reding]

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

On Wed, Oct 16, 2019 at 10:26:12AM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 16, 2019 at 09:38:40AM +0200, Thierry Reding wrote:
> > Remove usage of the ternary operator to assign values for register
> > fields. This removes clutter and improves readability.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/pwm-stm32.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index 9430b4cd383f..b12fb11b7a55 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -493,11 +493,19 @@ 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 = (index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E;
> > -	int shift = (index == 0) ? TIM_BDTR_BKF_SHIFT : TIM_BDTR_BK2F_SHIFT;
> > -	u32 mask = (index == 0) ? TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF
> > -				: TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F;
> > -	u32 bdtr = bke;
> > +	u32 bke, shift, mask, bdtr;
> > +
> > +	if (index == 0) {
> > +		bke = TIM_BDTR_BKE;
> > +		shift = TIM_BDTR_BKF_SHIFT;
> > +		mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF;
> > +	} else {
> > +		bke = TIM_BDTR_BK2E;
> > +		shift = TIM_BDTR_BK2F_SHIFT;
> > +		mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F;
> > +	}
> > +
> > +	bdtr = bke;
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Is index always in {0, 1}? Maybe a comment or a check about that would
> be helpful. (-> separate patch I think).

The bindings say that index can only be 0 or 1. I guess strictly it
might actually depend on the number of break inputs, but given these
register definitions, there's only ever two.

But yeah, it might be a good idea to sanitize the values upon probe.
I'll add another patch to do that.

Thierry

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

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

[Thierry Reding]

[-- 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 --]

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

[Fabrice Gasnier]

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
> 

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |