[PATCH 0/5] pwm: stm32: Cleanups, get_state() and proper hw take over

[PATCH 0/5] pwm: stm32: Cleanups, get_state() and proper hw take over

[Uwe Kleine-König]

Hello,

some time ago Philipp already sent an implementation of .get_state() for
the stm32 PWM driver. He created this series from the feedback but
didn't come around to send it out. The goal of this series is to allow
to take over a pwm-backlight from the bootloader without flickering on
an stm32mp157 based machine.

The only thing I did here to Philipp's series was to split one of the
patches in two. Philipp had the contents of patches 4 and 5 in a single
patch.

Best regards
Uwe

Philipp Zabel (5):
  pwm: stm32: Replace write_ccrx with regmap_write
  pwm: stm32: Make ch parameter unsigned
  pwm: stm32: Use hweight32 in stm32_pwm_detect_channels
  pwm: stm32: Implement .get_state()
  pwm: stm32: Fix enable count for clk in .probe()

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

base-commit: 4bb36d126cb3147d6bbfd00242a5b846dacad595
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] pwm: stm32: Replace write_ccrx with regmap_write

[Uwe Kleine-König]

From: Philipp Zabel <p.zabel@pengutronix.de>

The TIM_CCR1...4 registers are consecutive, so replace the switch
case with a simple calculation. Since ch is known to be in the 0...3
range (it is set to hwpwm < npwm <= 4), drop the unnecessary error
handling. The return value was not checked anyway. What remains does
not warrant keeping the write_ccrx() function around, so instead call
regmap_write() directly at the singular call site.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 3303a754ea02..5e48584e3bd4 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -52,21 +52,6 @@ static u32 active_channels(struct stm32_pwm *dev)
 	return ccer & TIM_CCER_CCXE;
 }
 
-static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
-{
-	switch (ch) {
-	case 0:
-		return regmap_write(dev->regmap, TIM_CCR1, value);
-	case 1:
-		return regmap_write(dev->regmap, TIM_CCR2, value);
-	case 2:
-		return regmap_write(dev->regmap, TIM_CCR3, value);
-	case 3:
-		return regmap_write(dev->regmap, TIM_CCR4, value);
-	}
-	return -EINVAL;
-}
-
 #define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P)
 #define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E)
 #define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P)
@@ -369,7 +354,7 @@ static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
 	dty = prd * duty_ns;
 	do_div(dty, period_ns);
 
-	write_ccrx(priv, ch, dty);
+	regmap_write(priv->regmap, TIM_CCR1 + 4 * ch, dty);
 
 	/* Configure output mode */
 	shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] pwm: stm32: Replace write_ccrx with regmap_write

[Fabrice Gasnier]

On 10/19/23 22:07, Uwe Kleine-König wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> The TIM_CCR1...4 registers are consecutive, so replace the switch
> case with a simple calculation. Since ch is known to be in the 0...3
> range (it is set to hwpwm < npwm <= 4), drop the unnecessary error
> handling. The return value was not checked anyway. What remains does
> not warrant keeping the write_ccrx() function around, so instead call
> regmap_write() directly at the singular call site.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)

Hi Uwe,

Sorry for the late reply,
You can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks,
Fabrice

> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 3303a754ea02..5e48584e3bd4 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -52,21 +52,6 @@ static u32 active_channels(struct stm32_pwm *dev)
>  	return ccer & TIM_CCER_CCXE;
>  }
>  
> -static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
> -{
> -	switch (ch) {
> -	case 0:
> -		return regmap_write(dev->regmap, TIM_CCR1, value);
> -	case 1:
> -		return regmap_write(dev->regmap, TIM_CCR2, value);
> -	case 2:
> -		return regmap_write(dev->regmap, TIM_CCR3, value);
> -	case 3:
> -		return regmap_write(dev->regmap, TIM_CCR4, value);
> -	}
> -	return -EINVAL;
> -}
> -
>  #define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P)
>  #define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E)
>  #define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P)
> @@ -369,7 +354,7 @@ static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
>  	dty = prd * duty_ns;
>  	do_div(dty, period_ns);
>  
> -	write_ccrx(priv, ch, dty);
> +	regmap_write(priv->regmap, TIM_CCR1 + 4 * ch, dty);
>  
>  	/* Configure output mode */
>  	shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] pwm: stm32: Make ch parameter unsigned

[Uwe Kleine-König]

From: Philipp Zabel <p.zabel@pengutronix.de>

The channel parameter is only ever set to pwm->hwpwm.
Make it unsigned int as well.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 5e48584e3bd4..009f9c1a5eca 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -308,7 +308,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
-static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
+static int stm32_pwm_config(struct stm32_pwm *priv, unsigned int ch,
 			    int duty_ns, int period_ns)
 {
 	unsigned long long prd, div, dty;
@@ -371,7 +371,7 @@ static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
 	return 0;
 }
 
-static int stm32_pwm_set_polarity(struct stm32_pwm *priv, int ch,
+static int stm32_pwm_set_polarity(struct stm32_pwm *priv, unsigned int ch,
 				  enum pwm_polarity polarity)
 {
 	u32 mask;
@@ -386,7 +386,7 @@ static int stm32_pwm_set_polarity(struct stm32_pwm *priv, int ch,
 	return 0;
 }
 
-static int stm32_pwm_enable(struct stm32_pwm *priv, int ch)
+static int stm32_pwm_enable(struct stm32_pwm *priv, unsigned int ch)
 {
 	u32 mask;
 	int ret;
@@ -411,7 +411,7 @@ static int stm32_pwm_enable(struct stm32_pwm *priv, int ch)
 	return 0;
 }
 
-static void stm32_pwm_disable(struct stm32_pwm *priv, int ch)
+static void stm32_pwm_disable(struct stm32_pwm *priv, unsigned int ch)
 {
 	u32 mask;
 
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] pwm: stm32: Make ch parameter unsigned

[Fabrice Gasnier]

On 10/19/23 22:07, Uwe Kleine-König wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> The channel parameter is only ever set to pwm->hwpwm.
> Make it unsigned int as well.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Hi Uwe,

You can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks,
Fabrice

> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 5e48584e3bd4..009f9c1a5eca 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -308,7 +308,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> -static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
> +static int stm32_pwm_config(struct stm32_pwm *priv, unsigned int ch,
>  			    int duty_ns, int period_ns)
>  {
>  	unsigned long long prd, div, dty;
> @@ -371,7 +371,7 @@ static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
>  	return 0;
>  }
>  
> -static int stm32_pwm_set_polarity(struct stm32_pwm *priv, int ch,
> +static int stm32_pwm_set_polarity(struct stm32_pwm *priv, unsigned int ch,
>  				  enum pwm_polarity polarity)
>  {
>  	u32 mask;
> @@ -386,7 +386,7 @@ static int stm32_pwm_set_polarity(struct stm32_pwm *priv, int ch,
>  	return 0;
>  }
>  
> -static int stm32_pwm_enable(struct stm32_pwm *priv, int ch)
> +static int stm32_pwm_enable(struct stm32_pwm *priv, unsigned int ch)
>  {
>  	u32 mask;
>  	int ret;
> @@ -411,7 +411,7 @@ static int stm32_pwm_enable(struct stm32_pwm *priv, int ch)
>  	return 0;
>  }
>  
> -static void stm32_pwm_disable(struct stm32_pwm *priv, int ch)
> +static void stm32_pwm_disable(struct stm32_pwm *priv, unsigned int ch)
>  {
>  	u32 mask;
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] pwm: stm32: Use hweight32 in stm32_pwm_detect_channels

[Uwe Kleine-König]

From: Philipp Zabel <p.zabel@pengutronix.de>

Use hweight32() to count the CCxE bits in stm32_pwm_detect_channels().
Since the return value is assigned to chip.npwm, change it to unsigned
int as well.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 009f9c1a5eca..cc6cae07c02c 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -563,10 +563,9 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
 	priv->have_complementary_output = (ccer != 0);
 }
 
-static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
+static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv)
 {
 	u32 ccer;
-	int npwm = 0;
 
 	/*
 	 * If channels enable bits don't exist writing 1 will have no
@@ -576,19 +575,7 @@ static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
 	regmap_read(priv->regmap, TIM_CCER, &ccer);
 	regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
 
-	if (ccer & TIM_CCER_CC1E)
-		npwm++;
-
-	if (ccer & TIM_CCER_CC2E)
-		npwm++;
-
-	if (ccer & TIM_CCER_CC3E)
-		npwm++;
-
-	if (ccer & TIM_CCER_CC4E)
-		npwm++;
-
-	return npwm;
+	return hweight32(ccer & TIM_CCER_CCXE);
 }
 
 static int stm32_pwm_probe(struct platform_device *pdev)
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] pwm: stm32: Use hweight32 in stm32_pwm_detect_channels

[Fabrice Gasnier]

On 10/19/23 22:07, Uwe Kleine-König wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Use hweight32() to count the CCxE bits in stm32_pwm_detect_channels().
> Since the return value is assigned to chip.npwm, change it to unsigned
> int as well.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)

Hi Uwe,

You can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks,
Fabrice

> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 009f9c1a5eca..cc6cae07c02c 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -563,10 +563,9 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>  	priv->have_complementary_output = (ccer != 0);
>  }
>  
> -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> +static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv)
>  {
>  	u32 ccer;
> -	int npwm = 0;
>  
>  	/*
>  	 * If channels enable bits don't exist writing 1 will have no
> @@ -576,19 +575,7 @@ static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
>  	regmap_read(priv->regmap, TIM_CCER, &ccer);
>  	regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
>  
> -	if (ccer & TIM_CCER_CC1E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC2E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC3E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC4E)
> -		npwm++;
> -
> -	return npwm;
> +	return hweight32(ccer & TIM_CCER_CCXE);
>  }
>  
>  static int stm32_pwm_probe(struct platform_device *pdev)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] pwm: stm32: Implement .get_state()

[Uwe Kleine-König]

From: Philipp Zabel <p.zabel@pengutronix.de>

Implement the &pwm_ops->get_state callback so drivers can inherit PWM
state set by the bootloader.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
[ukl: split off from a patch that also fixes clk enable count in .probe()]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index cc6cae07c02c..68239567a564 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -471,8 +471,50 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
+static int stm32_pwm_get_state(struct pwm_chip *chip,
+			       struct pwm_device *pwm, struct pwm_state *state)
+{
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	int ch = pwm->hwpwm;
+	unsigned long rate;
+	u32 ccer, psc, arr, ccr;
+	u64 dty, prd;
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
+	if (ret)
+		goto out;
+
+	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
+	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	ret = regmap_read(priv->regmap, TIM_PSC, &psc);
+	if (ret)
+		goto out;
+	ret = regmap_read(priv->regmap, TIM_ARR, &arr);
+	if (ret)
+		goto out;
+	ret = regmap_read(priv->regmap, TIM_CCR1 + 4 * ch, &ccr);
+	if (ret)
+		goto out;
+
+	rate = clk_get_rate(priv->clk);
+
+	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
+	state->period = DIV_ROUND_UP_ULL(prd, rate);
+	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
+	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
+
+out:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
 static const struct pwm_ops stm32pwm_ops = {
 	.apply = stm32_pwm_apply_locked,
+	.get_state = stm32_pwm_get_state,
 	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
 };
 
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] pwm: stm32: Implement .get_state()

[Fabrice Gasnier]

On 10/19/23 22:07, Uwe Kleine-König wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Implement the &pwm_ops->get_state callback so drivers can inherit PWM
> state set by the bootloader.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> [ukl: split off from a patch that also fixes clk enable count in .probe()]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32.c | 42 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 

Hi Uwe,

You can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks,
Fabrice

> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index cc6cae07c02c..68239567a564 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -471,8 +471,50 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> +static int stm32_pwm_get_state(struct pwm_chip *chip,
> +			       struct pwm_device *pwm, struct pwm_state *state)
> +{
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	int ch = pwm->hwpwm;
> +	unsigned long rate;
> +	u32 ccer, psc, arr, ccr;
> +	u64 dty, prd;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> +	if (ret)
> +		goto out;
> +
> +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
> +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	ret = regmap_read(priv->regmap, TIM_PSC, &psc);
> +	if (ret)
> +		goto out;
> +	ret = regmap_read(priv->regmap, TIM_ARR, &arr);
> +	if (ret)
> +		goto out;
> +	ret = regmap_read(priv->regmap, TIM_CCR1 + 4 * ch, &ccr);
> +	if (ret)
> +		goto out;
> +
> +	rate = clk_get_rate(priv->clk);
> +
> +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
>  static const struct pwm_ops stm32pwm_ops = {
>  	.apply = stm32_pwm_apply_locked,
> +	.get_state = stm32_pwm_get_state,
>  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
>  };
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] pwm: stm32: Fix enable count for clk in .probe()

[Uwe Kleine-König]

From: Philipp Zabel <p.zabel@pengutronix.de>

Make the driver take over hardware state without disabling in .probe()
and enable the clock for each enabled channel.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
[ukleinek: split off from a patch that also implemented .get_state()]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 68239567a564..97a2c3c09b69 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -605,17 +605,21 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
 	priv->have_complementary_output = (ccer != 0);
 }
 
-static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv)
+static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv,
+					      unsigned int *num_enabled)
 {
-	u32 ccer;
+	u32 ccer, ccer_backup;
 
 	/*
 	 * If channels enable bits don't exist writing 1 will have no
 	 * effect so we can detect and count them.
 	 */
+	regmap_read(priv->regmap, TIM_CCER, &ccer_backup);
 	regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
 	regmap_read(priv->regmap, TIM_CCER, &ccer);
-	regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
+	regmap_write(priv->regmap, TIM_CCER, ccer_backup);
+
+	*num_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);
 
 	return hweight32(ccer & TIM_CCER_CCXE);
 }
@@ -626,6 +630,8 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
 	struct stm32_pwm *priv;
+	unsigned int num_enabled;
+	unsigned int i;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -648,7 +654,11 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 
 	priv->chip.dev = dev;
 	priv->chip.ops = &stm32pwm_ops;
-	priv->chip.npwm = stm32_pwm_detect_channels(priv);
+	priv->chip.npwm = stm32_pwm_detect_channels(priv, &num_enabled);
+
+	/* Initialize clock refcount to number of enabled PWM channels. */
+	for (i = 0; i < num_enabled; i++)
+		clk_enable(priv->clk);
 
 	ret = devm_pwmchip_add(dev, &priv->chip);
 	if (ret < 0)
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] pwm: stm32: Fix enable count for clk in .probe()

[Fabrice Gasnier]

On 10/19/23 22:07, Uwe Kleine-König wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Make the driver take over hardware state without disabling in .probe()
> and enable the clock for each enabled channel.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> [ukleinek: split off from a patch that also implemented .get_state()]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)


Hi Uwe,

I'd only have a suggestion on the commit title:
pwm: stm32: Fix enable count for clk in .probe()
            ^
On first sight, the "Fix" may suggest that this fixes something older in
the tree. This would suggest a Fixes tag which isn't the case in this
series, as this is a split of the .get_state() patch.
Is it possible to rephrase ?
something like: set clk enable count when probing channels ?

Apart from that, you can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

---
I've additional questions, unrelated to this patch. I had a look at
pwm-stm32-lp.c, the clock enabling is done directly in the .get_state()
routine. I think this should be moved to the .probe() routine as done
here. There's likely a risk, that clk enable count gets increased
artificially, at least since commit cfc4c189bc70 "pwm: Read initial
hardware state at request time".
Shall I send a patch for pwm-stm32-lp.c, similar as this patch ? Or has
it been spotted already ?

Best Regards,
Thanks,
Fabrice

> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 68239567a564..97a2c3c09b69 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -605,17 +605,21 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>  	priv->have_complementary_output = (ccer != 0);
>  }
>  
> -static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> +static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv,
> +					      unsigned int *num_enabled)
>  {
> -	u32 ccer;
> +	u32 ccer, ccer_backup;
>  
>  	/*
>  	 * If channels enable bits don't exist writing 1 will have no
>  	 * effect so we can detect and count them.
>  	 */
> +	regmap_read(priv->regmap, TIM_CCER, &ccer_backup);
>  	regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
>  	regmap_read(priv->regmap, TIM_CCER, &ccer);
> -	regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
> +	regmap_write(priv->regmap, TIM_CCER, ccer_backup);
> +
> +	*num_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);
>  
>  	return hweight32(ccer & TIM_CCER_CCXE);
>  }
> @@ -626,6 +630,8 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
>  	struct stm32_pwm *priv;
> +	unsigned int num_enabled;
> +	unsigned int i;
>  	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -648,7 +654,11 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>  
>  	priv->chip.dev = dev;
>  	priv->chip.ops = &stm32pwm_ops;
> -	priv->chip.npwm = stm32_pwm_detect_channels(priv);
> +	priv->chip.npwm = stm32_pwm_detect_channels(priv, &num_enabled);
> +
> +	/* Initialize clock refcount to number of enabled PWM channels. */
> +	for (i = 0; i < num_enabled; i++)
> +		clk_enable(priv->clk);
>  
>  	ret = devm_pwmchip_add(dev, &priv->chip);
>  	if (ret < 0)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] pwm: stm32: Fix enable count for clk in .probe()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2140 bytes --]

On Tue, Nov 14, 2023 at 02:35:19PM +0100, Fabrice Gasnier wrote:
> On 10/19/23 22:07, Uwe Kleine-König wrote:
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > Make the driver take over hardware state without disabling in .probe()
> > and enable the clock for each enabled channel.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > [ukleinek: split off from a patch that also implemented .get_state()]
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-stm32.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> 
> Hi Uwe,
> 
> I'd only have a suggestion on the commit title:
> pwm: stm32: Fix enable count for clk in .probe()
>             ^
> On first sight, the "Fix" may suggest that this fixes something older in
> the tree. This would suggest a Fixes tag which isn't the case in this
> series, as this is a split of the .get_state() patch.
> Is it possible to rephrase ?

IMHO it is a fix, the hw state wasn't properly taken over before.
If you want a Fixes line, that's:

Fixes: 7edf7369205b ("pwm: Add driver for STM32 plaftorm")

> something like: set clk enable count when probing channels ?
> 
> Apart from that, you can add my:
> Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> ---
> I've additional questions, unrelated to this patch. I had a look at
> pwm-stm32-lp.c, the clock enabling is done directly in the .get_state()
> routine. I think this should be moved to the .probe() routine as done
> here. There's likely a risk, that clk enable count gets increased
> artificially, at least since commit cfc4c189bc70 "pwm: Read initial
> hardware state at request time".
> Shall I send a patch for pwm-stm32-lp.c, similar as this patch ? Or has
> it been spotted already ?

I'm not aware of someone working on stm32-lp, so feel free to prepare a
patch!

Best regards and thanks for your review,
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] pwm: stm32: Fix enable count for clk in .probe()

[Fabrice Gasnier]

On 11/14/23 22:20, Uwe Kleine-König wrote:
> On Tue, Nov 14, 2023 at 02:35:19PM +0100, Fabrice Gasnier wrote:
>> On 10/19/23 22:07, Uwe Kleine-König wrote:
>>> From: Philipp Zabel <p.zabel@pengutronix.de>
>>>
>>> Make the driver take over hardware state without disabling in .probe()
>>> and enable the clock for each enabled channel.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> [ukleinek: split off from a patch that also implemented .get_state()]
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>>  drivers/pwm/pwm-stm32.c | 18 ++++++++++++++----
>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>>
>> Hi Uwe,
>>
>> I'd only have a suggestion on the commit title:
>> pwm: stm32: Fix enable count for clk in .probe()
>>             ^
>> On first sight, the "Fix" may suggest that this fixes something older in
>> the tree. This would suggest a Fixes tag which isn't the case in this
>> series, as this is a split of the .get_state() patch.
>> Is it possible to rephrase ?
> 
> IMHO it is a fix, the hw state wasn't properly taken over before.

Hi Uwe,

Indeed, the HW state wasn't taken. Instead the driver used to be
forcibly stop each channel: regmap_clear_bits(priv->regmap, TIM_CCER,
TIM_CCER_CCXE);
So the clk enable count (of 0) reflects this. That's kind of consistent
state.

> If you want a Fixes line, that's:
> 
> Fixes: 7edf7369205b ("pwm: Add driver for STM32 plaftorm")

Well, the original driver didn't implement the .get_state.
BUT, more thinking about this, I think it lacks to read the global
enable status of the timer, e.g. TIM_CR1_CEN.

Original driver stops each channel (clear CCXE), but after probing, the
global TIM_CR1_CEN bit may remains 1 (from bootloader), without a
running clock.

If we're talking about fixing the original driver (probably unrelevant
without a working .get_state), then I think this part is missing.

(with or without a Fixes tag) Could you add a check on global counter
enable bit (TIM_CR1_CEN) both in the .get_state(), and in the
stm32_pwm_detect_channels(), that counts the num_enabled channels ?

In case the TIM_CR1_CEN isn't set (but some of the TIM_CCER_CCXE are),
the driver will report enabled channels. But physically, the pwm output
will be inactive.
That's more a robustness case... depending on what's been done possibly
by bootloader. What to you think ?

> 
>> something like: set clk enable count when probing channels ?
>>
>> Apart from that, you can add my:
>> Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>
>> ---
>> I've additional questions, unrelated to this patch. I had a look at
>> pwm-stm32-lp.c, the clock enabling is done directly in the .get_state()
>> routine. I think this should be moved to the .probe() routine as done
>> here. There's likely a risk, that clk enable count gets increased
>> artificially, at least since commit cfc4c189bc70 "pwm: Read initial
>> hardware state at request time".
>> Shall I send a patch for pwm-stm32-lp.c, similar as this patch ? Or has
>> it been spotted already ?
> 
> I'm not aware of someone working on stm32-lp, so feel free to prepare a
> patch!

Ok thanks! Will look into it.
Best Regards,
Fabrice

> 
> Best regards and thanks for your review,
> Uwe
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] pwm: stm32: Fix enable count for clk in .probe()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1273 bytes --]

Hello Fabrice,

On Wed, Nov 15, 2023 at 10:02:20AM +0100, Fabrice Gasnier wrote:
> (with or without a Fixes tag) Could you add a check on global counter
> enable bit (TIM_CR1_CEN) both in the .get_state(), and in the
> stm32_pwm_detect_channels(), that counts the num_enabled channels ?

I'd address that separately, mostly because the patches forwarded here
are not mine.

> In case the TIM_CR1_CEN isn't set (but some of the TIM_CCER_CCXE are),
> the driver will report enabled channels. But physically, the pwm output
> will be inactive.

Huuu, that means if channel #0 is running and I start a capture on
channel #1 which results in unconditionally calling
regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); (in
stm32_pwm_raw_capture()) channel #0 stops to toggle!

> That's more a robustness case... depending on what's been done possibly
> by bootloader. What to you think ?

I would assume that a bootloader that sets the CCXE bits also enables
TIM_CR1_CEN and so in practise Philipp's patch is fine. But I agree that
doing things properly would be better.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] pwm: stm32: Fix enable count for clk in .probe()

[Fabrice Gasnier]

On 11/15/23 22:01, Uwe Kleine-König wrote:
> Hello Fabrice,
> 
> On Wed, Nov 15, 2023 at 10:02:20AM +0100, Fabrice Gasnier wrote:
>> (with or without a Fixes tag) Could you add a check on global counter
>> enable bit (TIM_CR1_CEN) both in the .get_state(), and in the
>> stm32_pwm_detect_channels(), that counts the num_enabled channels ?
> 
> I'd address that separately, mostly because the patches forwarded here
> are not mine.

Ok, let's go this way.

> 
>> In case the TIM_CR1_CEN isn't set (but some of the TIM_CCER_CCXE are),
>> the driver will report enabled channels. But physically, the pwm output
>> will be inactive.
> 
> Huuu, that means if channel #0 is running and I start a capture on
> channel #1 which results in unconditionally calling
> regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); (in
> stm32_pwm_raw_capture()) channel #0 stops to toggle!

Basically no, please find my answers sent separately on "[PATCH] pwm:
stm32: Mark capture support as broken"

> 
>> That's more a robustness case... depending on what's been done possibly
>> by bootloader. What to you think ?
> 
> I would assume that a bootloader that sets the CCXE bits also enables
> TIM_CR1_CEN and so in practise Philipp's patch is fine. But I agree that
> doing things properly would be better.

ok, thanks,
Best Regards,
Fabrice
> 
> Best regards
> Uwe
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] pwm: stm32: Cleanups, get_state() and proper hw take over

[Thierry Reding]

On Thu, 19 Oct 2023 22:06:59 +0200, Uwe Kleine-König wrote:
> some time ago Philipp already sent an implementation of .get_state() for
> the stm32 PWM driver. He created this series from the feedback but
> didn't come around to send it out. The goal of this series is to allow
> to take over a pwm-backlight from the bootloader without flickering on
> an stm32mp157 based machine.
> 
> The only thing I did here to Philipp's series was to split one of the
> patches in two. Philipp had the contents of patches 4 and 5 in a single
> patch.
> 
> [...]

Applied, thanks!

[1/5] pwm: stm32: Replace write_ccrx with regmap_write
      commit: f98ef6bec483b921ae4341c7719c2fe26d5dd6ee
[2/5] pwm: stm32: Make ch parameter unsigned
      commit: 4cf03120c3a96e89ef2ec28f752e47d11be6b036
[3/5] pwm: stm32: Use hweight32 in stm32_pwm_detect_channels
      commit: 406a2ae953352d88f24ca1d564c898f4d896faf0
[4/5] pwm: stm32: Implement .get_state()
      commit: 6912b67c655f502df2d3c80002562fac87801ada
[5/5] pwm: stm32: Fix enable count for clk in .probe()
      commit: c0031cba442cc13bef2385e53c29a98308a20961

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel