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

[Fabrice Gasnier <fabrice.gasnier@foss.st.com>]

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