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

[<Hari.PrasathGE@microchip.com>]

Hello Christophe,

On 02/09/23 9:57 pm, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> 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.
> 

Thank you, I admit that I have messes up this part. Its been quite a 
while sending patches upstream and I seem to have forgotten the basics. 
I will take time to send the v3 paying more attention to these small 
details.

>>
>> Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@microchip.com>
>>
> 
> 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@microchip.com/
> 
>> 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.
> 

Though the kernel API's used below return 0 upon success but just 
thought I will initialize it to 0.

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

Will add it back and include a 'goto'

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

I will move this towards the end as it's done usually.

-Hari

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