linux-kernel - Re: [PATCH 2/2] i2c: jz4780: Use devm_clk_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e9a73895-aade-43bc-9a9f-d29b390ee100@vivo.com>
Date: Thu, 22 Aug 2024 20:46:41 +0800
From: Rong Qianfeng <11065417@...o.com>
To: Paul Cercueil <paul@...pouillou.net>,
 Rong Qianfeng <rongqianfeng@...o.com>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Andi Shyti <andi.shyti@...nel.org>, linux-renesas-soc@...r.kernel.org,
 linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-mips@...r.kernel.org
Cc: opensource.kernel@...o.com
Subject: Re: [PATCH 2/2] i2c: jz4780: Use devm_clk_get_enabled() helpers


在 2024/8/22 17:35, Paul Cercueil 写道:
> Hi Rong,
>
> Le jeudi 22 août 2024 à 10:52 +0800, Rong Qianfeng a écrit :
>> The devm_clk_get_enabled() helpers:
>>      - call devm_clk_get()
>>      - call clk_prepare_enable() and register what is needed in order
>> to
>>       call clk_disable_unprepare() when needed, as a managed resource.
>>
>> This simplifies the code and avoids the calls to
>> clk_disable_unprepare().
>>
>> While at it, remove the goto label "err:", and use its return value
>> to
>> return the error code.
>>
>> Signed-off-by: Rong Qianfeng <rongqianfeng@...o.com>
> Acked-by: Paul Cercueil <paul@...pouillou.net>
>
> With a small suggestion below.
>
>> ---
>>   drivers/i2c/busses/i2c-jz4780.c | 21 ++++++---------------
>>   1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-jz4780.c
>> b/drivers/i2c/busses/i2c-jz4780.c
>> index 4aafdfab6305..f5362c5dfb50 100644
>> --- a/drivers/i2c/busses/i2c-jz4780.c
>> +++ b/drivers/i2c/busses/i2c-jz4780.c
>> @@ -792,26 +792,22 @@ static int jz4780_i2c_probe(struct
>> platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, i2c);
>>   
>> -	i2c->clk = devm_clk_get(&pdev->dev, NULL);
>> +	i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>>   	if (IS_ERR(i2c->clk))
>>   		return PTR_ERR(i2c->clk);
>>   
>> -	ret = clk_prepare_enable(i2c->clk);
>> -	if (ret)
>> -		return ret;
>> -
>>   	ret = of_property_read_u32(pdev->dev.of_node, "clock-
>> frequency",
>>   				   &clk_freq);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "clock-frequency not specified
>> in DT\n");
>> -		goto err;
>> +		return ret;
>>   	}
> Since there is no more special handling needed here, you could just:
> if (ret)
>      return dev_err_probe(&pdev->dev, ret, "clock-frequency...\n");
>
> And the same for the other error handling paths that you changed.
>
> I wouldn't request a V2 just for that though.

Thanks for taking the time Paul,highly appreciated.

Cheers,
-Qianfeng
>
> Cheers,
> -Paul
>
>>   
>>   	i2c->speed = clk_freq / 1000;
>>   	if (i2c->speed == 0) {
>>   		ret = -EINVAL;
>>   		dev_err(&pdev->dev, "clock-frequency minimum is
>> 1000\n");
>> -		goto err;
>> +		return ret;
>>   	}
>>   	jz4780_i2c_set_speed(i2c);
>>   
>> @@ -827,29 +823,24 @@ static int jz4780_i2c_probe(struct
>> platform_device *pdev)
>>   
>>   	ret = platform_get_irq(pdev, 0);
>>   	if (ret < 0)
>> -		goto err;
>> +		return ret;
>>   	i2c->irq = ret;
>>   	ret = devm_request_irq(&pdev->dev, i2c->irq, jz4780_i2c_irq,
>> 0,
>>   			       dev_name(&pdev->dev), i2c);
>>   	if (ret)
>> -		goto err;
>> +		return ret;
>>   
>>   	ret = i2c_add_adapter(&i2c->adap);
>>   	if (ret < 0)
>> -		goto err;
>> +		return ret;
>>   
>>   	return 0;
>> -
>> -err:
>> -	clk_disable_unprepare(i2c->clk);
>> -	return ret;
>>   }
>>   
>>   static void jz4780_i2c_remove(struct platform_device *pdev)
>>   {
>>   	struct jz4780_i2c *i2c = platform_get_drvdata(pdev);
>>   
>> -	clk_disable_unprepare(i2c->clk);
>>   	i2c_del_adapter(&i2c->adap);
>>   }
>>