Messages in this thread | | | Date | Tue, 11 Jul 2023 10:30:54 +0800 | Subject | Re: [PATCH] pwm: atmel: enable clk when pwm already enabled in bootloader | From | 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@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
| |