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

Guiting Shen
2023-07-11T02:31:24Z

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@gmail.com>
>> ---
>> 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