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

[<Hari.PrasathGE@microchip.com>]

Hello Claudiu,

On 23/08/23 10:29 am, claudiu beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 8/22/23 10:04, Hari Prasath Gujulan Elango wrote:
>> 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.
>>
> 
> Can you add a fixes tag?
> 

yes I will add it.

>> Signed-off-by: Hari Prasath Gujulan Elango <Hari.PrasathGE@microchip.com>
>> ---
>>   drivers/pwm/pwm-atmel-hlcdc.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
>> index 96a709a9d49a..ce46f6c74a14 100644
>> --- a/drivers/pwm/pwm-atmel-hlcdc.c
>> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
>> @@ -108,8 +108,10 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>>                                                 ATMEL_HLCDC_CFG(0),
>>                                                 ATMEL_HLCDC_CLKPWMSEL,
>>                                                 gencfg);
>> -                     if (ret)
>> +                     if (ret) {
>> +                             clk_disable_unprepare(new_clk);
>>                                return ret;
>> +                     }
>>                }
>>
>>                do_div(pwmcval, state->period);
>> @@ -133,20 +135,27 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>>                                         ATMEL_HLCDC_PWMPS_MASK |
>>                                         ATMEL_HLCDC_PWMPOL,
>>                                         pwmcfg);
>> -             if (ret)
>> +             if (ret) {
>> +                     clk_disable_unprepare(new_clk);
>>                        return ret;
>> +             }
>>
>>                ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN,
>>                                   ATMEL_HLCDC_PWM);
>> -             if (ret)
>> +             if (ret) {
>> +                     clk_disable_unprepare(new_clk);
>>                        return ret;
>> +             }
>>
>>                ret = regmap_read_poll_timeout(hlcdc->regmap, ATMEL_HLCDC_SR,
>>                                               status,
>>                                               status & ATMEL_HLCDC_PWM,
>>                                               10, 0);
>> -             if (ret)
>> +             if (ret) {
>> +                     clk_disable_unprepare(new_clk);
> 
> Can you keep a single failure point for all these?
> 
> Also, you have to set chip->cur_clk = NULL otherwise next time your apply
> will get executed the new_clk will not be enabled.
> 

I see that new_clk is assigned to cur_clk in the if (state->enabled) 
block and clk_disable_unprepare() is invoked only in the else block for 
cur_clk and its made NULL. I will cleanup all of this at a single point 
and resend v2.

Thanks,
Hari


> Thank you,
> Claudiu Beznea
> 
>>                        return ret;
>> +             }
>> +
>>        } else {
>>                ret = regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS,
>>                                   ATMEL_HLCDC_PWM);