[PATCH v1 0/3] PCI: j721e: A couple of cleanups

[PATCH v1 0/3] 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.

Changes
   Add new patch for dev_err_probe return.
   dropped unsesary clk_disable_unprepare as its handle by
   devm_clk_get_optional_enabled.
    
RFC v1: https://lore.kernel.org/all/20251013101727.129260-1-linux.amoon@gmail.com/

Thanks
-Anand

Anand Moon (3):
  PCI: j721e: Propagate dev_err_probe return value
  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 | 39 ++++++++--------------
 1 file changed, 14 insertions(+), 25 deletions(-)


base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
-- 
2.50.1

[PATCH v1 1/3] PCI: j721e: Propagate dev_err_probe return value

[Anand Moon]

Ensure that the return value from dev_err_probe() is consistently assigned
back to ret in all error paths within j721e_pcie_probe(). This ensures
the original error code are propagation for debugging.

Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series
---
 drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 5bc5ab20aa6d..9c7bfa77a66e 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -569,20 +569,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	pm_runtime_enable(dev);
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
-		dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
+		ret = dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
 		goto err_get_sync;
 	}
 
 	ret = j721e_pcie_ctrl_init(pcie);
 	if (ret < 0) {
-		dev_err_probe(dev, ret, "j721e_pcie_ctrl_init failed\n");
+		ret = dev_err_probe(dev, ret, "j721e_pcie_ctrl_init failed\n");
 		goto err_get_sync;
 	}
 
 	ret = devm_request_irq(dev, irq, j721e_pcie_link_irq_handler, 0,
 			       "j721e-pcie-link-down-irq", pcie);
 	if (ret < 0) {
-		dev_err_probe(dev, ret, "failed to request link state IRQ %d\n", irq);
+		ret = dev_err_probe(dev, ret, "failed to request link state IRQ %d\n", irq);
 		goto err_get_sync;
 	}
 
@@ -599,7 +599,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 		ret = cdns_pcie_init_phy(dev, cdns_pcie);
 		if (ret) {
-			dev_err_probe(dev, ret, "Failed to init phy\n");
+			ret = dev_err_probe(dev, ret, "Failed to init phy\n");
 			goto err_get_sync;
 		}
 
@@ -611,7 +611,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 		ret = clk_prepare_enable(clk);
 		if (ret) {
-			dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");
+			ret = dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");
 			goto err_pcie_setup;
 		}
 		pcie->refclk = clk;
@@ -638,7 +638,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	case PCI_MODE_EP:
 		ret = cdns_pcie_init_phy(dev, cdns_pcie);
 		if (ret) {
-			dev_err_probe(dev, ret, "Failed to init phy\n");
+			ret = dev_err_probe(dev, ret, "Failed to init phy\n");
 			goto err_get_sync;
 		}
 
-- 
2.50.1

Re: [PATCH 1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

> Ensure that the return value from dev_err_probe() is consistently assigned
> back to ret in all error paths within j721e_pcie_probe(). This ensures
> the original error code are propagation for debugging.

I find the change description improvable.


I propose to take another source code transformation approach better into account.
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075

Example:
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636

	ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n");
	if (ret)
		goto err_get_sync;


How do you think about to achieve such a source code variant also with the help of
the semantic patch language (Coccinelle software)?

Regards,
Markus

Re: [PATCH 1/3] PCI: j721e: Propagate dev_err_probe return value

[Anand Moon]

Hi Markus,

Thanks for the review comments.
On Sat, 18 Oct 2025 at 14:26, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Ensure that the return value from dev_err_probe() is consistently assigned
> > back to ret in all error paths within j721e_pcie_probe(). This ensures
> > the original error code are propagation for debugging.
>
> I find the change description improvable.
>
Ok, I will update this once I get some more feedback on the changes.

>
> I propose to take another source code transformation approach better into account.
> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
>
> Example:
> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636
>
>         ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n");
>         if (ret)
>                 goto err_get_sync;
>
No, the correct code ensures that dev_err_probe() is only called when
an actual error
has occurred, providing a clear and accurate log entry. For deferred
probe (-EPROBE_DEFER),
it will correctly log at a debug level, as intended for that scenario.
For other errors,
it will provide a standard error message.
>
> How do you think about to achieve such a source code variant also with the help of
> the semantic patch language (Coccinelle software)?
I do not have any idea about this.
>
> Regards,
> Markus
Thanks
-Anand

Re: [PATCH 1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

>> I propose to take another source code transformation approach better into account.
>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
>>
>> Example:
>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636
>>
>>         ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n");
>>         if (ret)
>>                 goto err_get_sync;
>>
> No, the correct code ensures that dev_err_probe() is only called when
> an actual error
> has occurred, providing a clear and accurate log entry. …

Where do you see undesirable technical differences?

Regards,
Markus

Re: [PATCH 1/3] PCI: j721e: Propagate dev_err_probe return value

[Anand Moon]

Hi Markus, Vignesh,

On Sat, 18 Oct 2025 at 16:12, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> I propose to take another source code transformation approach better into account.
> >> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
> >>
> >> Example:
> >> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636
> >>
> >>         ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n");
> >>         if (ret)
> >>                 goto err_get_sync;
> >>
> > No, the correct code ensures that dev_err_probe() is only called when
> > an actual error
> > has occurred, providing a clear and accurate log entry. …
>
> Where do you see undesirable technical differences?

The primary issue I wanted to confirm was the function execution order.
since cdns_pcie_init_phy within dev_err_probe function

If other developers agree with the approach, I will modify this in a
separate patch

As Dan Carpenter pointed out - " Wait, no, this doesn't make sense.
It's just assigning ret to itself."

This patch seems irrelevant to me as the return value gets propagated
to the error path.
Sorry for the noise. Let's drop these changes.

Since I don't have this hardware for testing, I will verify it on
another available device.
>
> Regards,
> Markus

Thanks
-Anand

Re: [1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

>>>> I propose to take another source code transformation approach better into account.
>>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
>>>>
>>>> Example:
>>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636
>>>>
>>>>         ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n");
>>>>         if (ret)
>>>>                 goto err_get_sync;
>>>>
>>> No, the correct code ensures that dev_err_probe() is only called when
>>> an actual error
>>> has occurred, providing a clear and accurate log entry. …
>>
>> Where do you see undesirable technical differences?
> 
> The primary issue I wanted to confirm was the function execution order.

The desired control flow can be clarified in some ways.


> since cdns_pcie_init_phy within dev_err_probe function

One function should be executed before its return value will be directly passed
to a subsequent function call, shouldn't it?


> If other developers agree with the approach, I will modify this in a
> separate patch

There might be special coding style preferences involved (for a while).

Regards,
Markus

Re: [PATCH 1/3] PCI: j721e: Propagate dev_err_probe return value

[Christophe JAILLET]

Le 19/10/2025 à 12:15, Anand Moon a écrit :
> Hi Markus, Vignesh,
> 
> On Sat, 18 Oct 2025 at 16:12, Markus Elfring <Markus.Elfring@web.de> wrote:
>>
>>>> I propose to take another source code transformation approach better into account.
>>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/core.c#L5031-L5075
>>>>
>>>> Example:
>>>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/pci/controller/cadence/pci-j721e.c#L444-L636
>>>>
>>>>          ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to init phy\n");
>>>>          if (ret)
>>>>                  goto err_get_sync;
>>>>
>>> No, the correct code ensures that dev_err_probe() is only called when
>>> an actual error
>>> has occurred, providing a clear and accurate log entry. …
>>
>> Where do you see undesirable technical differences?
> 
> The primary issue I wanted to confirm was the function execution order.
> since cdns_pcie_init_phy within dev_err_probe function
> 
> If other developers agree with the approach, I will modify this in a
> separate patch

This other approach is just broken.

Using:
	ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to 
init phy\n");

1) is hard to read and understand.

2) would log an error message even if 0 is returned. This is just wrong.

2 good reasons not to do such things.


You should ignore people that are already ignored by most people on 
these lists.

CJ

> 
> As Dan Carpenter pointed out - " Wait, no, this doesn't make sense.
> It's just assigning ret to itself."

Yes, Dan is right.

> 
> This patch seems irrelevant to me as the return value gets propagated
> to the error path.
> Sorry for the noise. Let's drop these changes.
> 
> Since I don't have this hardware for testing, I will verify it on
> another available device.
>>
>> Regards,
>> Markus
> 
> Thanks
> -Anand
> 
> 

Re: [1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

> > As Dan Carpenter pointed out - " Wait, no, this doesn't make sense.
> > It's just assigning ret to itself."
> 
> Yes, Dan is right.

Would you become more interested in an other source code transformation?
https://lore.kernel.org/cocci/fe93e337-7d8e-4b82-b187-b5a67b627544@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2025-10/msg00020.html

Regards,
Markus

Re: [1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

…
> Using:
> 	ret = dev_err_probe(dev, cdns_pcie_init_phy(dev, cdns_pcie), "Failed to 
> init phy\n");
> 
> 1) is hard to read and understand.
…

I would like to inform you that it can be determined with the help of another
small SmPL script that variations of such assignment expressions are used in
108 source files of the software “Linux next-20251017”.
How will development imaginations evolve further then?

Regards,
Markus

Re: [1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

>> How do you think about to achieve such a source code variant also with the help of
>> the semantic patch language (Coccinelle software)?
> I do not have any idea about this.

Can another source code search pattern (like the following) trigger further
development considerations?

@display@
expression dev, e, x;
@@
 if (e)
 {
 ... when != e = x
*   dev_err_probe(dev, e, ...);
 ...
 }


Regards,
Markus

Re: [1/3] PCI: j721e: Propagate dev_err_probe return value

[Markus Elfring]

>> How do you think about to achieve such a source code variant also with the help of
>> the semantic patch language (Coccinelle software)?
> I do not have any idea about this.

Your software understanding evolved into another direction.

Can the following source code search pattern represent discussed development concerns
in a succinct way?

@display@
expression dev, e;
@@
*e = dev_err_probe(dev, e, ...)


The evaluation of such a tiny SmPL script can point out that 14 source files
of the software “Linux next-20251017” contain corresponding update candidates.
Will any adjustments become helpful also at these places?

Regards,
Markus

Re: [PATCH v1 1/3] PCI: j721e: Propagate dev_err_probe return value

[Dan Carpenter]

On Tue, Oct 14, 2025 at 05:02:27PM +0530, Anand Moon wrote:
> Ensure that the return value from dev_err_probe() is consistently assigned
> back to ret in all error paths within j721e_pcie_probe(). This ensures
> the original error code are propagation for debugging.
> 
> Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v1: new patch in this series
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 5bc5ab20aa6d..9c7bfa77a66e 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -569,20 +569,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  	pm_runtime_enable(dev);
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret < 0) {
> -		dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> +		ret = dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
>  		goto err_get_sync;

Wait, no, this doesn't make sense.  It's just assigning ret to itself.
That's just confusing and pointless.

regards,
dan carpenter

Re: [PATCH v1 1/3] PCI: j721e: Propagate dev_err_probe return value

[Anand Moon]

Hi Dan

On Sat, 18 Oct 2025 at 15:18, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Tue, Oct 14, 2025 at 05:02:27PM +0530, Anand Moon wrote:
> > Ensure that the return value from dev_err_probe() is consistently assigned
> > back to ret in all error paths within j721e_pcie_probe(). This ensures
> > the original error code are propagation for debugging.
> >
> > Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v1: new patch in this series
> > ---
> >  drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index 5bc5ab20aa6d..9c7bfa77a66e 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > @@ -569,20 +569,20 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >       pm_runtime_enable(dev);
> >       ret = pm_runtime_get_sync(dev);
> >       if (ret < 0) {
> > -             dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> > +             ret = dev_err_probe(dev, ret, "pm_runtime_get_sync failed\n");
> >               goto err_get_sync;
>
> Wait, no, this doesn't make sense.  It's just assigning ret to itself.
> That's just confusing and pointless.
>
Ok, Thanks for clarifying.
> regards,
> dan carpenter
>
Thanks
-Anand

[PATCH v1 2/3] 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 error handling and makes the code more compact. This changes removes
the unnecessary clk variable and assigns the result of the
devm_clk_get_optional_enabled() call directly to pcie->refclk.
This makes the code more concise and readable without changing the
behavior.

Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: Drop explicit clk_disable_unprepare — 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 | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 9c7bfa77a66e..ed8e182f0772 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,18 +602,11 @@ 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");
-			goto err_pcie_setup;
-		}
-
-		ret = clk_prepare_enable(clk);
-		if (ret) {
+		pcie->refclk = devm_clk_get_optional_enabled(dev, "pcie_refclk");
+		if (IS_ERR(pcie->refclk)) {
 			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
@@ -630,7 +622,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 		ret = cdns_pcie_host_setup(rc);
 		if (ret < 0) {
-			clk_disable_unprepare(pcie->refclk);
 			goto err_pcie_setup;
 		}
 
@@ -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

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

[Markus Elfring]

> Use devm_clk_get_optional_enabled() helper instead of calling
> devm_clk_get_optional() and then clk_prepare_enable(). It simplifies
> the error handling and makes the code more compact. …

Can this information be sufficient so that the subsequent two sentences
may be omitted from such a change description?

Regards,
Markus

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

[Dan Carpenter]

On Tue, Oct 14, 2025 at 05:02:28PM +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 error handling and makes the code more compact. This changes removes
> the unnecessary clk variable and assigns the result of the
> devm_clk_get_optional_enabled() call directly to pcie->refclk.
> This makes the code more concise and readable without changing the
> behavior.
> 
> Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v1: Drop explicit clk_disable_unprepare — 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 | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 9c7bfa77a66e..ed8e182f0772 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,18 +602,11 @@ 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");
> -			goto err_pcie_setup;
> -		}
> -
> -		ret = clk_prepare_enable(clk);
> -		if (ret) {
> +		pcie->refclk = devm_clk_get_optional_enabled(dev, "pcie_refclk");
> +		if (IS_ERR(pcie->refclk)) {
>  			ret = dev_err_probe(dev, ret, "failed to enable pcie_refclk\n");

This isn't correct.  It's assigning ret to itself when it should be
assigning PTR_ERR(pcie->refclk) to ret.

regards,
dan carpenter

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

[Dan Carpenter]

On Tue, Oct 14, 2025 at 05:02:28PM +0530, Anand Moon wrote:
> @@ -630,7 +622,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  
>  		ret = cdns_pcie_host_setup(rc);
>  		if (ret < 0) {
> -			clk_disable_unprepare(pcie->refclk);
>  			goto err_pcie_setup;
>  		}

This will introduce a checkpatch warning with checkpatch.pl -f because
you need to delete the curly braces.

regards,
dan carpenter

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

[Anand Moon]

Change removes the unnecessary local gpiod variable and assigns the result
of the devm_gpiod_get_optional() call directly to pcie->reset_gpio.
This makes the code more concise and readable without changing the
behavior.

Cc: Siddharth Vadapalli <s-vadapalli@ti.com>
Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
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 ed8e182f0772..bd8fda0baba8 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) {
@@ -615,9 +614,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 3/3] PCI: j721e: Use inline reset GPIO assignment and drop local variable

[Markus Elfring]

> Change removes the unnecessary local gpiod variable and assigns the result
> of the devm_gpiod_get_optional() call directly to pcie->reset_gpio.
> This makes the code more concise and readable without changing the
> behavior.

Will another imperative wording approach become more helpful for an improved
change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.18-rc1#n94

Regards,
Markus