| 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 |
> [-- 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
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;
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 >
> > [-- 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
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 --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, 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(-)