linux-kernel - Re: [PATCH] i3c: master: cdns: Simplify handling clocks in probe()

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] [day] [month] [year] [list]

Message-ID: <aHUcaE/3L1Yr+Wzb@lizhi-Precision-Tower-5810>
Date: Mon, 14 Jul 2025 11:04:08 -0400
From: Frank Li <Frank.li@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Przemysław Gaj <pgaj@...ence.com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i3c: master: cdns: Simplify handling clocks in probe()

On Mon, Jul 14, 2025 at 04:40:53PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2025 16:15, Frank Li wrote:
> > On Sun, Jul 13, 2025 at 05:24:12PM +0200, Krzysztof Kozlowski wrote:
> >> The two clocks, driver is getting, are not being disabled/re-enabled
> >> during runtime of the device.  Eliminate one variable in state struct,
> >> all error paths and a lot of code from probe() and remove() by using
> >> devm_clk_get_enabled().
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> >> ---
> >>  drivers/i3c/master/i3c-master-cdns.c | 51 +++++++---------------------
> >>  1 file changed, 12 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> >> index 449e85d7ba87..cc504b58013a 100644
> >> --- a/drivers/i3c/master/i3c-master-cdns.c
> >> +++ b/drivers/i3c/master/i3c-master-cdns.c
> >> @@ -412,7 +412,6 @@ struct cdns_i3c_master {
> >>  	} xferqueue;
> >>  	void __iomem *regs;
> >>  	struct clk *sysclk;
> >> -	struct clk *pclk;
> >>  	struct cdns_i3c_master_caps caps;
> >>  	unsigned long i3c_scl_lim;
> >>  	const struct cdns_i3c_data *devdata;
> >> @@ -1566,6 +1565,7 @@ MODULE_DEVICE_TABLE(of, cdns_i3c_master_of_ids);
> >>  static int cdns_i3c_master_probe(struct platform_device *pdev)
> >>  {
> >>  	struct cdns_i3c_master *master;
> >> +	struct clk *pclk;
> >>  	int ret, irq;
> >>  	u32 val;
> >>
> >> @@ -1581,11 +1581,11 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
> >>  	if (IS_ERR(master->regs))
> >>  		return PTR_ERR(master->regs);
> >>
> >> -	master->pclk = devm_clk_get(&pdev->dev, "pclk");
> >> -	if (IS_ERR(master->pclk))
> >> -		return PTR_ERR(master->pclk);
> >> +	pclk = devm_clk_get_enabled(&pdev->dev, "pclk");
> >> +	if (IS_ERR(pclk))
> >> +		return PTR_ERR(pclk);
> >>
> >> -	master->sysclk = devm_clk_get(&pdev->dev, "sysclk");
> >> +	master->sysclk = devm_clk_get_enabled(&pdev->dev, "sysclk");
> >
> > Can you use devm_clk_bulk_get_all_enabled() to simpilfy futher?
>
> Instead of asking redundant question check yourself. On a first glance
> it cannot, because it won't be simpler - you still need individual
> clock. But if you find it possible which is not visible on first glance,
> make a proposal instead of just random drive by comments.

It is hard to find that sysclk is used at other place only from this patch
without check source code. I think you are expert. If it is feasible, a
simple reminder should be enough. If not, simple reply the reason/difficult.

In svc i3c driver, we use devm_clk_bulk_get_all_enabled(). And make code
simple because it use runtime pm.

In this case, using two dev_clk_get_enabled() is simplest. I think it should
be similar with svc i3c driver at first glance.

Reviewed-by: Frank Li <Frank.Li@....com>

Frank

>
> Best regards,
> Krzysztof