[PATCH v2 0/2] PCI: j721e: A couple of cleanups

[PATCH v2 0/2] PCI: j721e: A couple of cleanups

[Anand Moon]

Refactor the J721e probe function to use devres helpers for resource
management. This replaces manual clock handling with
devm_clk_get_optional_enabled() and assigns the reset GPIO directly
to the struct members, eliminating unnecessary local variables.

These patches have been compile-tested only, as I do not have access
to the hardware for runtime verification.

v1  : https://lore.kernel.org/all/20251014113234.44418-1-linux.amoon@gmail.com/
RFC : https://lore.kernel.org/all/20251013101727.129260-1-linux.amoon@gmail.com/

Changes
v2  Drop the dev_err_probe return patch.
    Fix small issue address issue by Dan and Markus.
v1:
   Add new patch for dev_err_probe return.
   dropped unsesary clk_disable_unprepare as its handle by
   devm_clk_get_optional_enabled.

Thanks
-Anand

Anand Moon (2):
  PCI: j721e: Use devm_clk_get_optional_enabled() to get the clock
  PCI: j721e: Use inline reset GPIO assignment and drop local variable

 drivers/pci/controller/cadence/pci-j721e.c | 34 +++++++---------------
 1 file changed, 11 insertions(+), 23 deletions(-)


base-commit: 566771afc7a81e343da9939f0bd848d3622e2501
-- 
2.50.1

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

[Anand Moon]

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
@@ -479,7 +479,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	struct cdns_pcie_ep *ep = NULL;
 	struct gpio_desc *gpiod;
 	void __iomem *base;
-	struct clk *clk;
 	u32 num_lanes;
 	u32 mode;
 	int ret;
@@ -603,19 +602,13 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 		}
 
-		clk = devm_clk_get_optional(dev, "pcie_refclk");
-		if (IS_ERR(clk)) {
-			ret = dev_err_probe(dev, PTR_ERR(clk), "failed to get pcie_refclk\n");
+		pcie->refclk = devm_clk_get_optional_enabled(dev, "pcie_refclk");
+		if (IS_ERR(pcie->refclk)) {
+			ret = dev_err_probe(dev, PTR_ERR(pcie->refclk),
+					    "failed to enable pcie_refclk\n");
 			goto err_pcie_setup;
 		}
 
-		ret = clk_prepare_enable(clk);
-		if (ret) {
-			dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");
-			goto err_pcie_setup;
-		}
-		pcie->refclk = clk;
-
 		/*
 		 * Section 2.2 of the PCI Express Card Electromechanical
 		 * Specification (Revision 5.1) mandates that the deassertion
@@ -629,10 +622,8 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 		}
 
 		ret = cdns_pcie_host_setup(rc);
-		if (ret < 0) {
-			clk_disable_unprepare(pcie->refclk);
+		if (ret < 0)
 			goto err_pcie_setup;
-		}
 
 		break;
 	case PCI_MODE_EP:
@@ -679,7 +670,6 @@ static void j721e_pcie_remove(struct platform_device *pdev)
 
 	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
 
-	clk_disable_unprepare(pcie->refclk);
 	cdns_pcie_disable_phy(cdns_pcie);
 	j721e_pcie_disable_link_irq(pcie);
 	pm_runtime_put(dev);
@@ -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);
 	}
 
 	cdns_pcie_disable_phy(pcie->cdns_pcie);
-- 
2.50.1

[PATCH v2 2/2] PCI: j721e: Use inline reset GPIO assignment and drop local variable

[Anand Moon]

The result of devm_gpiod_get_optional() is now assigned directly
assigned to pcie->reset_gpio. This removes a superfluous local gpiod
variable, improving code readability and compactness. The functionality
remains unchanged, but the code is now more streamlined

Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v2: fix the commit message.
v1: Add Rb - Siddharth
---
 drivers/pci/controller/cadence/pci-j721e.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index b678f7d48206..633fe8f93102 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -477,7 +477,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	struct j721e_pcie *pcie;
 	struct cdns_pcie_rc *rc = NULL;
 	struct cdns_pcie_ep *ep = NULL;
-	struct gpio_desc *gpiod;
 	void __iomem *base;
 	u32 num_lanes;
 	u32 mode;
@@ -589,12 +588,12 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 	switch (mode) {
 	case PCI_MODE_RC:
-		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-		if (IS_ERR(gpiod)) {
-			ret = dev_err_probe(dev, PTR_ERR(gpiod), "Failed to get reset GPIO\n");
+		pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+		if (IS_ERR(pcie->reset_gpio)) {
+			ret = dev_err_probe(dev, PTR_ERR(pcie->reset_gpio),
+					    "Failed to get reset GPIO\n");
 			goto err_get_sync;
 		}
-		pcie->reset_gpio = gpiod;
 
 		ret = cdns_pcie_init_phy(dev, cdns_pcie);
 		if (ret) {
@@ -616,9 +615,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 		 * This shall ensure that the power and the reference clock
 		 * are stable.
 		 */
-		if (gpiod) {
+		if (pcie->reset_gpio) {
 			msleep(PCIE_T_PVPERL_MS);
-			gpiod_set_value_cansleep(gpiod, 1);
+			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
 		}
 
 		ret = cdns_pcie_host_setup(rc);
-- 
2.50.1

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

[Siddharth Vadapalli]

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().

>  	}
>  
>  	cdns_pcie_disable_phy(pcie->cdns_pcie);

Regards,
Siddharth.

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

[Anand Moon]

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?
>
> >       }
> >
> >       cdns_pcie_disable_phy(pcie->cdns_pcie);
>
> Regards,
> Siddharth.

Thanks
-Anand

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

[Siddharth Vadapalli]

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.

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

[Anand Moon]

Hi Siddharth,

On Mon, 27 Oct 2025 at 10:42, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>
> 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.
Thanks for clarifying my doubt. That part makes sense.
> Can you clarify why 'clk_disable_unprepare()' should be removed in
> j721e_pcie_suspend_noirq()?
It happened by mistake.
> Regards,
> Siddharth.
Thanks
-Anand