linux-kernel - Re: [PATCH v2 1/2] PCI: j721e: Use devm_clk_get_optional

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: <bef3d7015012c4004d21cd829892ca942496a6dd.camel@ti.com>
Date: Mon, 27 Oct 2025 10:42:17 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Anand Moon <linux.amoon@...il.com>
CC: Vignesh Raghavendra <vigneshr@...com>,
        Lorenzo Pieralisi
	<lpieralisi@...nel.org>,
        Krzysztof Wilczyński
	<kwilczynski@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        "Rob
 Herring" <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        "open
 list:PCI DRIVER FOR TI DRA7XX/J721E" <linux-omap@...r.kernel.org>,
        "open
 list:PCI DRIVER FOR TI DRA7XX/J721E" <linux-pci@...r.kernel.org>,
        "moderated
 list:PCI DRIVER FOR TI DRA7XX/J721E" <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>,
        Markus Elfring
	<Markus.Elfring@....de>,
        Dan Carpenter <dan.carpenter@...aro.org>,
        Siddharth
 Vadapalli <s-vadapalli@...com>
Subject: Re: [PATCH v2 1/2] PCI: j721e: Use devm_clk_get_optional_enabled()
 to get the clock

On Sat, 2025-10-25 at 14:07 +0530, Anand Moon wrote:
> Hi Siddharth,
> 
> Thanks for your review comments,
> 
> On Sat, 25 Oct 2025 at 13:20, Siddharth Vadapalli <s-vadapalli@...com> wrote:
> > 
> > On Sat, 2025-10-25 at 13:13 +0530, Anand Moon wrote:
> > > Use devm_clk_get_optional_enabled() helper instead of calling
> > > devm_clk_get_optional() and then clk_prepare_enable(). It simplifies
> > > the clk_prepare_enable() and clk_disable_unprepare() with proper error
> > > handling and makes the code more compact.
> > > The result of devm_clk_get_optional_enabled() is now assigned directly
> > > to pcie->refclk. This removes a superfluous local clk variable,
> > > improving code readability and compactness. The functionality
> > > remains unchanged, but the code is now more streamlined.
> > > 
> > > Cc: Siddharth Vadapalli <s-vadapalli@...com>
> > > Signed-off-by: Anand Moon <linux.amoon@...il.com>
> > > ---
> > > v2: Rephase the commit message and use proper error pointer
> > >     PTR_ERR(pcie->refclk) to return error.
> > > v1: Drop explicit clk_disable_unprepare as it handled by
> > >     devm_clk_get_optional_enabled, Since devm_clk_get_optional_enabled
> > >     internally manages clk_prepare_enable and clk_disable_unprepare
> > >     as part of its lifecycle, the explicit call to clk_disable_unprepare
> > >     is redundant and can be safely removed.
> > > ---
> > >  drivers/pci/controller/cadence/pci-j721e.c | 21 +++++----------------
> > >  1 file changed, 5 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > > index 5bc5ab20aa6d..b678f7d48206 100644
> > > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > 
> > [TRIMMED]
> > 
> > > @@ -692,7 +682,6 @@ static int j721e_pcie_suspend_noirq(struct device *dev)
> > > 
> > >       if (pcie->mode == PCI_MODE_RC) {
> > >               gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> > > -             clk_disable_unprepare(pcie->refclk);
> > 
> > j721e_pcie_resume_noirq() contains clk_enable_prepare().
> Ok I will drop the clk_prepare_enable and clk_disable_unprepare in
> this function?

The clock needs to be disabled on Suspend and enabled on Resume.

The reasoning behind replacing:
devm_clk_get_optional()  + clk_prepare_enable()
with:
devm_clk_get_optional_enabled()
is clear to me, but the removal of the 'clk_disable_unprepare()' on the
Suspend path isn't.

Removing 'clk_disable_unprepare()' in the driver's remove path makes sense
because the
devm() API will automatically disable and unprepare the clock when the
device is "unbound".
However, to the best of my understanding, the device is not "unbound"
during Suspend.
Can you clarify why 'clk_disable_unprepare()' should be removed in
j721e_pcie_suspend_noirq()?

Regards,
Siddharth.