PCI: j721e: A couple of cleanups

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.

v3 : https://lore.kernel.org/all/[email protected]/
v2 : https://lore.kernel.org/all/[email protected]/
v1 : https://lore.kernel.org/all/[email protected]/
RFC : https://lore.kernel.org/all/[email protected]/

Changes
v4 : Improve the commit message.

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 | 33 ++++++++--------------
 1 file changed, 11 insertions(+), 22 deletions(-)

base-commit: fd57572253bc356330dbe5b233c2e1d8426c66fd

2.50.1

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.

---
v4: Add Rb Siddharth

---
 drivers/pci/controller/cadence/pci-j721e.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

index 5bc5ab20aa6d..a88b2e52fd78 100644
@@ -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);

2.50.1

Assign the result of devm_gpiod_get_optional() directly to pcie->reset_gpio. Thus removes a superfluous local variable, which simplifies control flow and improves code clarity without affecting functional behavior.

---
v4: Improve the commit message

---
 drivers/pci/controller/cadence/pci-j721e.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

index a88b2e52fd78..ecd1b0312400 100644
@@ -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

…> Thus removes a superfluous local variable, which simplifies control flow

remove? Simplify?

improve?

Regards,
Markus

Hi All,

These changes are v4 revision. This got messed up in the format output folder Sorry for the inconvenience.

Thanks
-Anand

…> Changes

Would an other version number be more appropriate for the subject prefixes of this patch series?

…> v1:
…> dropped unsesary clk_disable_unprepare as its handle by
…

unnecessary?

Regards,
Markus

Hi Markus,

Yes, if there are some more review comments on these patches, Or they get lost in emails.

Thanks
-Anand

Applied, thanks!

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

      commit: 59ba8c0981e73cb2bb70074be4ef46b4188424ea
[2/2] PCI: j721e: Use inline reset GPIO assignment and drop local variable
      commit: 8895c0fc0671f38746ee1c02b728b126f7dbbf97

Best regards,

Manivannan Sadhasivam <[email protected]>

Moved to pci/controller/j721e so they're separate from the keystone changes.