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

[Thierry Reding]

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.

> 
> 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.

> +	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.

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

Attachment: signature.asc
Description: PGP signature