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

[PATCH v2 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. I ended up squashing two patches
into one in v2 because the second patch from v1 is already addressed in
patch 1 of v2. I've added a patch in v2 that validates device tree data
for breakinput as suggested by Uwe.

Lee, patch 1 of this small series touches the MFD header for the STM32
timers, but there's no good way to separate the patches, so if you could
provide an Acked-by on that patch so that I can take it through the PWM
tree along with the rest, that'd be great.

Thierry

Thierry Reding (3):
  pwm: stm32: Remove clutter from ternary operator
  pwm: stm32: Pass breakinput instead of its values
  pwm: stm32: Validate breakinput data from DT

 drivers/pwm/pwm-stm32.c          | 36 +++++++++++++++++---------------
 include/linux/mfd/stm32-timers.h | 12 ++++-------
 2 files changed, 23 insertions(+), 25 deletions(-)

-- 
2.23.0

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

[Thierry Reding]

Remove usage of the ternary operator to assign values for register
fields. Instead, parameterize the register and field offset macros
and pass the index to them.

This removes clutter and improves readability.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-stm32.c          | 21 +++++++++------------
 include/linux/mfd/stm32-timers.h | 12 ++++--------
 2 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 9430b4cd383f..a5b323432d8c 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -493,20 +493,17 @@ 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 shift = TIM_BDTR_BKF_SHIFT(index);
+	u32 bke = TIM_BDTR_BKE(index);
+	u32 bkp = TIM_BDTR_BKP(index);
+	u32 bkf = TIM_BDTR_BKF(index);
+	u32 mask = bkf | bkp | bke;
+	u32 bdtr;
 
-	/*
-	 * 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 = (filter & TIM_BDTR_BKF_MASK) << shift | bke;
 
-	bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift;
+	if (level)
+		bdtr |= bkp;
 
 	regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr);
 
diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index 067d14655c28..f8db83aedb2b 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -70,14 +70,11 @@
 #define TIM_CCER_CC4E	BIT(12)	/* Capt/Comp 4  out Ena    */
 #define TIM_CCER_CC4P	BIT(13)	/* Capt/Comp 4  Polarity   */
 #define TIM_CCER_CCXE	(BIT(0) | BIT(4) | BIT(8) | BIT(12))
-#define TIM_BDTR_BKE	BIT(12) /* Break input enable	   */
-#define TIM_BDTR_BKP	BIT(13) /* Break input polarity	   */
+#define TIM_BDTR_BKE(x)	BIT(12 + (x) * 12) /* Break input enable */
+#define TIM_BDTR_BKP(x)	BIT(13 + (x) * 12) /* Break input polarity */
 #define TIM_BDTR_AOE	BIT(14)	/* Automatic Output Enable */
 #define TIM_BDTR_MOE	BIT(15)	/* Main Output Enable      */
-#define TIM_BDTR_BKF	(BIT(16) | BIT(17) | BIT(18) | BIT(19))
-#define TIM_BDTR_BK2F	(BIT(20) | BIT(21) | BIT(22) | BIT(23))
-#define TIM_BDTR_BK2E	BIT(24) /* Break 2 input enable	   */
-#define TIM_BDTR_BK2P	BIT(25) /* Break 2 input polarity  */
+#define TIM_BDTR_BKF(x)	(0xf << (16 + (x) * 4))
 #define TIM_DCR_DBA	GENMASK(4, 0)	/* DMA base addr */
 #define TIM_DCR_DBL	GENMASK(12, 8)	/* DMA burst len */
 
@@ -87,8 +84,7 @@
 #define TIM_CR2_MMS2_SHIFT	20
 #define TIM_SMCR_TS_SHIFT	4
 #define TIM_BDTR_BKF_MASK	0xF
-#define TIM_BDTR_BKF_SHIFT	16
-#define TIM_BDTR_BK2F_SHIFT	20
+#define TIM_BDTR_BKF_SHIFT(x)	(16 + (x) * 4)
 
 enum stm32_timers_dmas {
 	STM32_TIMERS_DMA_CH1,
-- 
2.23.0

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

[Uwe Kleine-König]

On Wed, Oct 16, 2019 at 01:05:59PM +0200, Thierry Reding wrote:
> Remove usage of the ternary operator to assign values for register
> fields. Instead, parameterize the register and field offset macros
> and pass the index to them.
> 
> This removes clutter and improves readability.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/pwm-stm32.c          | 21 +++++++++------------
>  include/linux/mfd/stm32-timers.h | 12 ++++--------
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 9430b4cd383f..a5b323432d8c 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -493,20 +493,17 @@ 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 shift = TIM_BDTR_BKF_SHIFT(index);
> +	u32 bke = TIM_BDTR_BKE(index);
> +	u32 bkp = TIM_BDTR_BKP(index);
> +	u32 bkf = TIM_BDTR_BKF(index);
> +	u32 mask = bkf | bkp | bke;
> +	u32 bdtr;

It's not clear that

	(index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E

is the same as

	TIM_BDTR_BKE(index)

unless we know that index is 0 or 1. A word about that in the commit log
would be great, maybe even ...

>  
> -	/*
> -	 * 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 = (filter & TIM_BDTR_BKF_MASK) << shift | bke;
>  
> -	bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift;
> +	if (level)
> +		bdtr |= bkp;
>  
>  	regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr);
>  
> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> index 067d14655c28..f8db83aedb2b 100644
> --- a/include/linux/mfd/stm32-timers.h
> +++ b/include/linux/mfd/stm32-timers.h
> @@ -70,14 +70,11 @@
>  #define TIM_CCER_CC4E	BIT(12)	/* Capt/Comp 4  out Ena    */
>  #define TIM_CCER_CC4P	BIT(13)	/* Capt/Comp 4  Polarity   */
>  #define TIM_CCER_CCXE	(BIT(0) | BIT(4) | BIT(8) | BIT(12))
> -#define TIM_BDTR_BKE	BIT(12) /* Break input enable	   */
> -#define TIM_BDTR_BKP	BIT(13) /* Break input polarity	   */
> +#define TIM_BDTR_BKE(x)	BIT(12 + (x) * 12) /* Break input enable */
> +#define TIM_BDTR_BKP(x)	BIT(13 + (x) * 12) /* Break input polarity */
>  #define TIM_BDTR_AOE	BIT(14)	/* Automatic Output Enable */
>  #define TIM_BDTR_MOE	BIT(15)	/* Main Output Enable      */
> -#define TIM_BDTR_BKF	(BIT(16) | BIT(17) | BIT(18) | BIT(19))
> -#define TIM_BDTR_BK2F	(BIT(20) | BIT(21) | BIT(22) | BIT(23))
> -#define TIM_BDTR_BK2E	BIT(24) /* Break 2 input enable	   */
> -#define TIM_BDTR_BK2P	BIT(25) /* Break 2 input polarity  */
> +#define TIM_BDTR_BKF(x)	(0xf << (16 + (x) * 4))
>  #define TIM_DCR_DBA	GENMASK(4, 0)	/* DMA base addr */
>  #define TIM_DCR_DBL	GENMASK(12, 8)	/* DMA burst len */
>  
> @@ -87,8 +84,7 @@
>  #define TIM_CR2_MMS2_SHIFT	20
>  #define TIM_SMCR_TS_SHIFT	4
>  #define TIM_BDTR_BKF_MASK	0xF
> -#define TIM_BDTR_BKF_SHIFT	16
> -#define TIM_BDTR_BK2F_SHIFT	20
> +#define TIM_BDTR_BKF_SHIFT(x)	(16 + (x) * 4)

... define the macros as:

	#define TIM_BDTR_BKF_SHIFT(x)        (BUF_ON(index != 0 && index != 1), 16 + (x) * 4)

?

Best regards
Uwe

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

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

[Thierry Reding]

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

On Wed, Oct 16, 2019 at 01:11:31PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 16, 2019 at 01:05:59PM +0200, Thierry Reding wrote:
> > Remove usage of the ternary operator to assign values for register
> > fields. Instead, parameterize the register and field offset macros
> > and pass the index to them.
> > 
> > This removes clutter and improves readability.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/pwm-stm32.c          | 21 +++++++++------------
> >  include/linux/mfd/stm32-timers.h | 12 ++++--------
> >  2 files changed, 13 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index 9430b4cd383f..a5b323432d8c 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -493,20 +493,17 @@ 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 shift = TIM_BDTR_BKF_SHIFT(index);
> > +	u32 bke = TIM_BDTR_BKE(index);
> > +	u32 bkp = TIM_BDTR_BKP(index);
> > +	u32 bkf = TIM_BDTR_BKF(index);
> > +	u32 mask = bkf | bkp | bke;
> > +	u32 bdtr;
> 
> It's not clear that
> 
> 	(index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E
> 
> is the same as
> 
> 	TIM_BDTR_BKE(index)
> 
> unless we know that index is 0 or 1. A word about that in the commit log
> would be great, maybe even ...
> 
> >  
> > -	/*
> > -	 * 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 = (filter & TIM_BDTR_BKF_MASK) << shift | bke;
> >  
> > -	bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift;
> > +	if (level)
> > +		bdtr |= bkp;
> >  
> >  	regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr);
> >  
> > diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> > index 067d14655c28..f8db83aedb2b 100644
> > --- a/include/linux/mfd/stm32-timers.h
> > +++ b/include/linux/mfd/stm32-timers.h
> > @@ -70,14 +70,11 @@
> >  #define TIM_CCER_CC4E	BIT(12)	/* Capt/Comp 4  out Ena    */
> >  #define TIM_CCER_CC4P	BIT(13)	/* Capt/Comp 4  Polarity   */
> >  #define TIM_CCER_CCXE	(BIT(0) | BIT(4) | BIT(8) | BIT(12))
> > -#define TIM_BDTR_BKE	BIT(12) /* Break input enable	   */
> > -#define TIM_BDTR_BKP	BIT(13) /* Break input polarity	   */
> > +#define TIM_BDTR_BKE(x)	BIT(12 + (x) * 12) /* Break input enable */
> > +#define TIM_BDTR_BKP(x)	BIT(13 + (x) * 12) /* Break input polarity */
> >  #define TIM_BDTR_AOE	BIT(14)	/* Automatic Output Enable */
> >  #define TIM_BDTR_MOE	BIT(15)	/* Main Output Enable      */
> > -#define TIM_BDTR_BKF	(BIT(16) | BIT(17) | BIT(18) | BIT(19))
> > -#define TIM_BDTR_BK2F	(BIT(20) | BIT(21) | BIT(22) | BIT(23))
> > -#define TIM_BDTR_BK2E	BIT(24) /* Break 2 input enable	   */
> > -#define TIM_BDTR_BK2P	BIT(25) /* Break 2 input polarity  */
> > +#define TIM_BDTR_BKF(x)	(0xf << (16 + (x) * 4))
> >  #define TIM_DCR_DBA	GENMASK(4, 0)	/* DMA base addr */
> >  #define TIM_DCR_DBL	GENMASK(12, 8)	/* DMA burst len */
> >  
> > @@ -87,8 +84,7 @@
> >  #define TIM_CR2_MMS2_SHIFT	20
> >  #define TIM_SMCR_TS_SHIFT	4
> >  #define TIM_BDTR_BKF_MASK	0xF
> > -#define TIM_BDTR_BKF_SHIFT	16
> > -#define TIM_BDTR_BK2F_SHIFT	20
> > +#define TIM_BDTR_BKF_SHIFT(x)	(16 + (x) * 4)
> 
> ... define the macros as:
> 
> 	#define TIM_BDTR_BKF_SHIFT(x)        (BUF_ON(index != 0 && index != 1), 16 + (x) * 4)
> 
> ?

Given the changes in patch 3/3 that can no longer happen.

Thierry

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

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

[Lee Jones]

On Wed, 16 Oct 2019, Thierry Reding wrote:

> Remove usage of the ternary operator to assign values for register
> fields. Instead, parameterize the register and field offset macros
> and pass the index to them.
> 
> This removes clutter and improves readability.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/pwm-stm32.c          | 21 +++++++++------------

>  include/linux/mfd/stm32-timers.h | 12 ++++--------

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 2/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 | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index a5b323432d8c..db1d675b45fb 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -491,18 +491,18 @@ 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 shift = TIM_BDTR_BKF_SHIFT(index);
-	u32 bke = TIM_BDTR_BKE(index);
-	u32 bkp = TIM_BDTR_BKP(index);
-	u32 bkf = TIM_BDTR_BKF(index);
+	u32 shift = TIM_BDTR_BKF_SHIFT(bi->index);
+	u32 bke = TIM_BDTR_BKE(bi->index);
+	u32 bkp = TIM_BDTR_BKP(bi->index);
+	u32 bkf = TIM_BDTR_BKF(bi->index);
 	u32 mask = bkf | bkp | bke;
 	u32 bdtr;
 
-	bdtr = (filter & TIM_BDTR_BKF_MASK) << shift | bke;
+	bdtr = (bi->filter & TIM_BDTR_BKF_MASK) << shift | bke;
 
-	if (level)
+	if (bi->level)
 		bdtr |= bkp;
 
 	regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr);
@@ -518,10 +518,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 v2 2/3] pwm: stm32: Pass breakinput instead of its values

[Uwe Kleine-König]

On Wed, Oct 16, 2019 at 01:06:00PM +0200, Thierry Reding wrote:
> Instead of passing the individual values of the breakpoint, pass a
> pointer to the breakpoint.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

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

[PATCH v2 3/3] pwm: stm32: Validate breakinput data from DT

[Thierry Reding]

Both index and level can only be either 0 or 1 and the filter value is
limited to values between (and including) 0 and 15. Validate that the
device tree node contains values that are within these ranges.

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

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index db1d675b45fb..7ff48c14fae8 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -530,6 +530,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
 				       struct device_node *np)
 {
 	int nb, ret, array_size;
+	unsigned int i;
 
 	nb = of_property_count_elems_of_size(np, "st,breakinput",
 					     sizeof(struct stm32_breakinput));
@@ -551,6 +552,13 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
 	if (ret)
 		return ret;
 
+	for (i = 0; i < priv->num_breakinputs; i++) {
+		if (priv->breakinputs[i].index > 1 ||
+		    priv->breakinputs[i].level > 1 ||
+		    priv->breakinputs[i].filter > 15)
+			return -EINVAL;
+	}
+
 	return stm32_pwm_apply_breakinputs(priv);
 }
 
-- 
2.23.0

Re: [PATCH v2 3/3] pwm: stm32: Validate breakinput data from DT

[Uwe Kleine-König]

On Wed, Oct 16, 2019 at 01:06:01PM +0200, Thierry Reding wrote:
> Both index and level can only be either 0 or 1 and the filter value is
> limited to values between (and including) 0 and 15. Validate that the
> device tree node contains values that are within these ranges.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/pwm-stm32.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index db1d675b45fb..7ff48c14fae8 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -530,6 +530,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>  				       struct device_node *np)
>  {
>  	int nb, ret, array_size;
> +	unsigned int i;
>  
>  	nb = of_property_count_elems_of_size(np, "st,breakinput",
>  					     sizeof(struct stm32_breakinput));
> @@ -551,6 +552,13 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>  	if (ret)
>  		return ret;
>  
> +	for (i = 0; i < priv->num_breakinputs; i++) {
> +		if (priv->breakinputs[i].index > 1 ||
> +		    priv->breakinputs[i].level > 1 ||
> +		    priv->breakinputs[i].filter > 15)
> +			return -EINVAL;
> +	}
> +

maybe put this patch before patch 1 that relies on index not being
bigger than 1?

Best regards
Uwe

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

Re: [PATCH v2 3/3] pwm: stm32: Validate breakinput data from DT

[Thierry Reding]

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

On Wed, Oct 16, 2019 at 01:15:22PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 16, 2019 at 01:06:01PM +0200, Thierry Reding wrote:
> > Both index and level can only be either 0 or 1 and the filter value is
> > limited to values between (and including) 0 and 15. Validate that the
> > device tree node contains values that are within these ranges.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/pwm-stm32.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index db1d675b45fb..7ff48c14fae8 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -530,6 +530,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
> >  				       struct device_node *np)
> >  {
> >  	int nb, ret, array_size;
> > +	unsigned int i;
> >  
> >  	nb = of_property_count_elems_of_size(np, "st,breakinput",
> >  					     sizeof(struct stm32_breakinput));
> > @@ -551,6 +552,13 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
> >  	if (ret)
> >  		return ret;
> >  
> > +	for (i = 0; i < priv->num_breakinputs; i++) {
> > +		if (priv->breakinputs[i].index > 1 ||
> > +		    priv->breakinputs[i].level > 1 ||
> > +		    priv->breakinputs[i].filter > 15)
> > +			return -EINVAL;
> > +	}
> > +
> 
> maybe put this patch before patch 1 that relies on index not being
> bigger than 1?

Yeah, that's a good idea. Does that resolve the concerns you had on
patch 1?

Thierry

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

Re: [PATCH v2 3/3] pwm: stm32: Validate breakinput data from DT

[Uwe Kleine-König]

On Wed, Oct 16, 2019 at 03:07:16PM +0200, Thierry Reding wrote:
> On Wed, Oct 16, 2019 at 01:15:22PM +0200, Uwe Kleine-König wrote:
> > On Wed, Oct 16, 2019 at 01:06:01PM +0200, Thierry Reding wrote:
> > > Both index and level can only be either 0 or 1 and the filter value is
> > > limited to values between (and including) 0 and 15. Validate that the
> > > device tree node contains values that are within these ranges.
> > > 
> > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > > ---
> > >  drivers/pwm/pwm-stm32.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > index db1d675b45fb..7ff48c14fae8 100644
> > > --- a/drivers/pwm/pwm-stm32.c
> > > +++ b/drivers/pwm/pwm-stm32.c
> > > @@ -530,6 +530,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
> > >  				       struct device_node *np)
> > >  {
> > >  	int nb, ret, array_size;
> > > +	unsigned int i;
> > >  
> > >  	nb = of_property_count_elems_of_size(np, "st,breakinput",
> > >  					     sizeof(struct stm32_breakinput));
> > > @@ -551,6 +552,13 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	for (i = 0; i < priv->num_breakinputs; i++) {
> > > +		if (priv->breakinputs[i].index > 1 ||
> > > +		    priv->breakinputs[i].level > 1 ||
> > > +		    priv->breakinputs[i].filter > 15)
> > > +			return -EINVAL;
> > > +	}
> > > +
> > 
> > maybe put this patch before patch 1 that relies on index not being
> > bigger than 1?
> 
> Yeah, that's a good idea. Does that resolve the concerns you had on
> patch 1?

Yes, assuming you mention that in the commit log.

Best regards
Uwe

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