Re: [PATCH v2 1/2] PCI: j721e: Use devm_clk_get_optional_enabled() to get the clock

[Siddharth Vadapalli <s-vadapalli@ti.com>]

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@ti.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@ti.com>
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.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.