Re: [PATCH v3] pwm: atmel: Enable clk when pwm already enabled in bootloader

[Guiting Shen]

On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
>> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
>> +{
>> +	unsigned int i;
>> +	int err;
>> +	u32 sr;
>> +
>> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
>> +	if (!sr)
>> +		return 0;
>> +
>> +	for (i = 0; i < atmel_pwm->chip.npwm; i++) {
>> +		if (!(sr & (1 << i)))
>> +			continue;
>> +
>> +		err = clk_enable(atmel_pwm->clk);
>> +		if (err) {
>> +			dev_err(atmel_pwm->chip.dev,
>> +				"failed to enable clock: %pe\n", ERR_PTR(err));
> 
> Here you leak possibly a few enables. While it's not likely that the
> (say) third enable goes wrong, it's also not that hard to handle?!

The driver used the enable_count member of struct clk_core to count the
PWM channels(4 channels). It will enable hardware clock only when one of
the PWM channels becomed on from all PWM channels off which maybe return
error. And in second/third/fourth times to clk_enable(), it just
increased the enable_count of struct clk_core which would never return
error.

It maybe confused at first time to view the code.
Do it need to add something like that: ?

for (i = 0; i < atmel_pwm->chip.npwm; i++) {
	if (!(sr & (1 << i)))
		continue;

	err = clk_enable(atmel_pwm->clk);
	if (err) {
		dev_err(atmel_pwm->chip.dev,
			"failed to enable clock: %pe\n", ERR_PTR(err));

		for (i = 0; i < cnt; i++)
			clk_disable(atmel_pwm->clk);
		return err;
	}
	cnt++;
}

-- 
Regards,
Guiting Shen