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

[Guiting Shen]

On Mon, Jul 10, 2023 at 23:00:45PM GMT+8, Thierry Reding wrote:
> On Mon, Jul 10, 2023 at 10:42:14PM +0800, Guiting Shen wrote:
>> The driver would never call clk_eanble() if the pwm channel already
>> enable in bootloader which lead to dump the warning message of "the pwm
>> clk already disabled" when poweroff the pwm channel.
>>
>> Add atmel_pwm_enanle_clk_if_on() in probe function to enable clk if the
>> pwm channel already enabled in bootloader.
> 
> You've got multiple spelling errors in the commit message. Also, PWM is
> an abbreviation and so should be all uppercase (except for the subject
> prefix). I also prefer spelling out terms like "clock" in the commit
> message. This is text that is supposed to be readable. It's not code.

Got it, Thank you. How about this commit message:

The driver would never call clk_enable() if the PWM channel was already
enabled in bootloader which lead to dump the warning message "the pwm
clock already disabled" when turn off the PWM channel.

Add atmel_pwm_enable_clk_if_on() in probe function to enable clk if the
PWM channel was already enabled in bootloader.

>>
>> Signed-off-by: Guiting Shen <aarongt.shen@xxxxxxxxx>
>> ---
>>  drivers/pwm/pwm-atmel.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>> index cdbc23649032..385f12eb604c 100644
>> --- a/drivers/pwm/pwm-atmel.c
>> +++ b/drivers/pwm/pwm-atmel.c
>> @@ -464,6 +464,29 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
>>  
>> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
>> +{
>> +	u32 sr, i;
> 
> Maybe make i an unsigned int since you use it to iterate over npwm,
> which is unsigned int as well.

Ok, I will correct it in v2 patch.

>> +	int err;
>> +
>> +	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)))
> 
> We would usually write this as BIT(i), but I see that the rest of the
> driver uses this notation, so it's fine to keep this as-is.
> 
>> +			continue;
>> +
>> +		err = clk_enable(atmel_pwm->clk);
>> +		if (err) {
>> +			dev_err(atmel_pwm->chip.dev, "enable clock error\n");
> 
> Might be worth to include the error code in the error message to make it
> easier to diagnose where the issue is. Something like:
> 
> 	dev_err(atmel_pwm->chip.dev, "failed to enable clock: %d\n", err);
> 
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int atmel_pwm_probe(struct platform_device *pdev)
>>  {
>>  	struct atmel_pwm_chip *atmel_pwm;
>> @@ -504,6 +527,10 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, atmel_pwm);
>>  
>> +	ret = atmel_pwm_enable_clk_if_on(atmel_pwm);
>> +	if (ret < 0)
>> +		goto unprepare_clk;
> 
> This is not correct. You call this after pwmchip_add(), so you need to
> make sure to also remove the PWM chip on error. Preferably, though, you
> should call this before adding the PWM chip in the first place.

Sorry, I will call this before adding the PWM chip in v2 patch.

>> +
>>  	return ret;
>>  
>>  unprepare_clk:
>> -- 
>> 2.25.1
>>

-- 
Regards,
Guiting Shen