[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 at 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 at pengutronix.de>
>>> [ukleinek: split off from a patch that also implemented .get_state()]
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at 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 at 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
>