Re: [PATCH v2] pwm: atmel: add missing clk_disable_unprepare()

[Christophe JAILLET]

Le 02/09/2023 à 08:32, Hari Prasath Gujulan Elango a écrit :
> Fix the below smatch warning:
>
> drivers/pwm/pwm-atmel-hlcdc.c:167 atmel_hlcdc_pwm_apply() warn: 'new_clk' from clk_prepare_enable() not released on lines: 112,137,142,149.
>
> 'Fixes: 2b4984bef47a5 ("pwm: atmel-hlcdc: Convert to the atomic PWM API")'

Hi,

There shouldn't be ' before Fixes:, neither at the end.
Commit id should be 12 chars, not 13.
There shouldn't be a blank line between Fixes and Signed-off-by.

I think that the Fixes tag should be 2b4984bef47a ("pwm: add support for 
atmel-hlcdc-pwm device".
The commit you point you have touched this code, be part of what you 
change was already there before that.

> Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@xxxxxxxxxxxxx>

There should be a --- between the signed-of-by and the below changelog, 
so that the changelog will not be merged in the git history.

Also, it is also useful to add the link at lore.kernel.org of previous 
versions.

Here, it would be something like:
v1: 
https://lore.kernel.org/all/20230822070441.22170-1-Hari.PrasathGE@xxxxxxxxxxxxx/

> changelog of v2:
>
>           - moved the clk_disable_unprepare to single point of return.
>           - cur_clk set to NULL before return.
> ---
>   drivers/pwm/pwm-atmel-hlcdc.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> index 96a709a9d49a..4d35b838203f 100644
> --- a/drivers/pwm/pwm-atmel-hlcdc.c
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -44,7 +44,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
>   	struct atmel_hlcdc *hlcdc = chip->hlcdc;
>   	unsigned int status;
> -	int ret;
> +	int ret = 0;

This initialization looks un-needed and un-related to your changes.

>   
>   	if (state->enabled) {
>   		struct clk *new_clk = hlcdc->slow_clk;
> @@ -109,7 +109,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   						 ATMEL_HLCDC_CLKPWMSEL,
>   						 gencfg);
>   			if (ret)
> -				return ret;
> +				goto disable_new_clk;
>   		}
>   
>   		do_div(pwmcval, state->period);
> @@ -134,18 +134,20 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   					 ATMEL_HLCDC_PWMPOL,
>   					 pwmcfg);
>   		if (ret)
> -			return ret;
> +			goto disable_new_clk;
>   
>   		ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN,
>   				   ATMEL_HLCDC_PWM);
>   		if (ret)
> -			return ret;
> +			goto disable_new_clk;
>   
>   		ret = regmap_read_poll_timeout(hlcdc->regmap, ATMEL_HLCDC_SR,
>   					       status,
>   					       status & ATMEL_HLCDC_PWM,
>   					       10, 0);
> -		if (ret)

Removing this test looks wrong.

> +disable_new_clk:
> +			clk_disable_unprepare(new_clk);
> +			chip->cur_clk = NULL;
>   			return ret;

This is a really unusual pattern.
Usually, an error handling path is added at the end of the function, not 
in the middle.

CJ

>   	} else {
>   		ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS,