[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f08d820b-cef3-bc9d-69ce-fc6d3f65a550@microchip.com>
Date: Wed, 6 Sep 2023 06:15:36 +0000
From: <Hari.PrasathGE@...rochip.com>
To: <christophe.jaillet@...adoo.fr>, <thierry.reding@...il.com>,
<u.kleine-koenig@...gutronix.de>, <Nicolas.Ferre@...rochip.com>,
<alexandre.belloni@...tlin.com>, <claudiu.beznea@...on.dev>
CC: <linux-pwm@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] pwm: atmel: add missing clk_disable_unprepare()
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@...rochip.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,
>
Powered by blists - more mailing lists