| Message ID | 20240628205430.24775-3-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
On Fri, Jun 28, 2024 at 04:54:21PM -0400, Jim Quinlan wrote: > Instead of invoking "clk_disable_unprepare(pcie->clk)" in > a number of error paths. It's nice if the commit log is readable by itself, without reading the subject line as the first line of the commit log. Same way a title is not directly part of an article/essay/book itself.
> [-- Attachment #1: Type: text/plain, Size: 1600 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. * Can a wording approach (like the following) be a better change description? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n45 Add a jump target so that a bit of exception handling can be better reused at the end of this function implementation. * 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
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;
Instead of invoking "clk_disable_unprepare(pcie->clk)" in a number of error paths. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/pci/controller/pcie-brcmstb.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)