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 New, archived
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;
Markus Elfring July 5, 2024, 3:45 p.m. UTC | #3
>     > [-- 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 | #4
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;