diff mbox series

[v2,02/12] PCI: brcmstb: Use "clk_out" error path label

Message ID 20240703180300.42959-3-james.quinlan@broadcom.com (mailing list archive)
State Superseded
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: brcnstb: Enable STB 7712 SOC | expand

Commit Message

Jim Quinlan July 3, 2024, 6:02 p.m. UTC
Instead of invoking "clk_disable_unprepare(pcie->clk)" in
a number of error paths, we can just use a "clk_out" label
and goto the label after setting the return value.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Markus Elfring July 4, 2024, 11:40 a.m. UTC | #1
> [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

Can improved adjustments be provided as regular diff data
(without an extra attachment)?


> Instead of invoking "clk_disable_unprepare(pcie->clk)" in
> a number of error paths, we can just use a "clk_out" label
> and goto the label after setting the return value.

* Please improve such a change description with imperative wordings.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n94

* How do you think about to use a summary phrase like
  “Use more common error handling code in brcm_pcie_probe()”?


…
> +++ b/drivers/pci/controller/pcie-brcmstb.c>  	ret = reset_control_reset(pcie->rescal);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		goto clk_out;
> +	}
>
>  	ret = brcm_phy_start(pcie);
…

Does this software update complete the exception handling?

Would you like to add any tags (like “Fixes” and “Cc”) accordingly?


…
> @@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>
>  	return 0;
>
> +clk_out:
> +	clk_disable_unprepare(pcie->clk);
> +	return ret;
>  fail:
…

I suggest to add a blank line before the second label.

Regards,
Markus
Stanimir Varbanov July 4, 2024, 12:53 p.m. UTC | #2
Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> Instead of invoking "clk_disable_unprepare(pcie->clk)" in
> a number of error paths, we can just use a "clk_out" label
> and goto the label after setting the return value.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..c2eb29b886f7 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1620,24 +1620,25 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>  	if (IS_ERR(pcie->rescal)) {
> -		clk_disable_unprepare(pcie->clk);
> -		return PTR_ERR(pcie->rescal);
> +		ret = PTR_ERR(pcie->rescal);
> +		goto clk_out;
>  	}
>  	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
>  	if (IS_ERR(pcie->perst_reset)) {
> -		clk_disable_unprepare(pcie->clk);
> -		return PTR_ERR(pcie->perst_reset);
> +		ret = PTR_ERR(pcie->perst_reset);
> +		goto clk_out;
>  	}
>  

	ret = clk_prepare_enable(pcie->clk);

Have you considered enabling pcie->clk clock here ^^^ and avoid jumping
to clk_out label?

Basically, it'd be easier from error-path POV to get all required
resources and just after that start the initialization sequence of pcie
controller.

~Stan

>  	ret = reset_control_reset(pcie->rescal);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		goto clk_out;
> +	}
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);
> -		clk_disable_unprepare(pcie->clk);
> -		return ret;
> +		goto clk_out;
>  	}
>  
>  	ret = brcm_pcie_setup(pcie);
> @@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +clk_out:
> +	clk_disable_unprepare(pcie->clk);
> +	return ret;
>  fail:
>  	__brcm_pcie_remove(pcie);
>  	return ret;
Jim Quinlan July 5, 2024, 2:48 p.m. UTC | #3
On Thu, Jul 4, 2024 at 7:40 AM Markus Elfring <Markus.Elfring@web.de> wrote:

> > [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
>
> Can improved adjustments be provided as regular diff data
> (without an extra attachment)?
>

I'm not sure what you are referring to... I see no attachment in the copy
of this email I received, and I am sending my patches  with "git
send-email".

I will address your other comments in v3.

Regards,
Jim Quinlan
Broadcom STB/CM

>
>
> > Instead of invoking "clk_disable_unprepare(pcie->clk)" in
> > a number of error paths, we can just use a "clk_out" label
> > and goto the label after setting the return value.
>
> * Please improve such a change description with imperative wordings.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n94
>
> * How do you think about to use a summary phrase like
>   “Use more common error handling code in brcm_pcie_probe()”?
>
>
> …
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> …
> >       ret = reset_control_reset(pcie->rescal);
> > -     if (ret)
> > +     if (ret) {
> >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > +             goto clk_out;
> > +     }
> >
> >       ret = brcm_phy_start(pcie);
> …
>
> Does this software update complete the exception handling?
>
> Would you like to add any tags (like “Fixes” and “Cc”) accordingly?
>
>
> …
> > @@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device
> *pdev)
> >
> >       return 0;
> >
> > +clk_out:
> > +     clk_disable_unprepare(pcie->clk);
> > +     return ret;
> >  fail:
> …
>
> I suggest to add a blank line before the second label.
>
> Regards,
> Markus
>
Markus Elfring July 5, 2024, 3:45 p.m. UTC | #4
>     > [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
>
>     Can improved adjustments be provided as regular diff data
>     (without an extra attachment)?
>
>
> I'm not sure what you are referring to... I see no attachment in the copy of this email I received, …

Do I get inappropriate impressions here from published data representations?
https://lore.kernel.org/linux-kernel/20240703180300.42959-3-james.quinlan@broadcom.com/

Regards,
Markus
Jim Quinlan July 5, 2024, 5:07 p.m. UTC | #5
On Fri, Jul 5, 2024 at 11:45 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >     > [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
> >
> >     Can improved adjustments be provided as regular diff data
> >     (without an extra attachment)?
> >
> >
> > I'm not sure what you are referring to... I see no attachment in the copy of this email I received, …
>
> Do I get inappropriate impressions here from published data representations?
> https://lore.kernel.org/linux-kernel/20240703180300.42959-3-james.quinlan@broadcom.com/

Hi Markus,

Okay, I do see that :-).
It might be something my company is doing to the emails.  At one point
we had to stop sending submissions from our company email because it
would tag each email with
a long legal statement.  I will check with my manager (Florian
Fainaelli) to see if I can do something
about this.

Thanks for the heads-up,
Jim Quinlan
Broadcom STB/CM
>
>
>
> Regards,
> Markus
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c08683febdd4..c2eb29b886f7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1620,24 +1620,25 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 	}
 	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
 	if (IS_ERR(pcie->rescal)) {
-		clk_disable_unprepare(pcie->clk);
-		return PTR_ERR(pcie->rescal);
+		ret = PTR_ERR(pcie->rescal);
+		goto clk_out;
 	}
 	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
 	if (IS_ERR(pcie->perst_reset)) {
-		clk_disable_unprepare(pcie->clk);
-		return PTR_ERR(pcie->perst_reset);
+		ret = PTR_ERR(pcie->perst_reset);
+		goto clk_out;
 	}
 
 	ret = reset_control_reset(pcie->rescal);
-	if (ret)
+	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
+		goto clk_out;
+	}
 
 	ret = brcm_phy_start(pcie);
 	if (ret) {
 		reset_control_rearm(pcie->rescal);
-		clk_disable_unprepare(pcie->clk);
-		return ret;
+		goto clk_out;
 	}
 
 	ret = brcm_pcie_setup(pcie);
@@ -1676,6 +1677,9 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
+clk_out:
+	clk_disable_unprepare(pcie->clk);
+	return ret;
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;