| Message ID | 20240827023914.2255103-1-bo.wu@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v3] PCI: armada8k: change to use devm_clk_get_enabled() helper | expand |
Hi Wu Bo, On Tue, 27 Aug 2024 at 07:55, Wu Bo <bo.wu@vivo.com> wrote: > > Use devm_clk_get_enabled() instead of devm_clk_get() to make the code > cleaner and avoid calling clk_disable_unprepare() > > Signed-off-by: Wu Bo <bo.wu@vivo.com> > --- > drivers/pci/controller/dwc/pcie-armada8k.c | 36 ++++++++-------------- > 1 file changed, 13 insertions(+), 23 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c > index b5c599ccaacf..e7ef6c2641b8 100644 > --- a/drivers/pci/controller/dwc/pcie-armada8k.c > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c > @@ -284,23 +284,17 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > > pcie->pci = pci; > > - pcie->clk = devm_clk_get(dev, NULL); > + pcie->clk = devm_clk_get_enabled(dev, NULL); > if (IS_ERR(pcie->clk)) > - return PTR_ERR(pcie->clk); > - > - ret = clk_prepare_enable(pcie->clk); > - if (ret) > - return ret; > - > - pcie->clk_reg = devm_clk_get(dev, "reg"); > - if (pcie->clk_reg == ERR_PTR(-EPROBE_DEFER)) { > - ret = -EPROBE_DEFER; > - goto fail; > - } > - if (!IS_ERR(pcie->clk_reg)) { > - ret = clk_prepare_enable(pcie->clk_reg); > - if (ret) > - goto fail_clkreg; > + return dev_err_probe(dev, PTR_ERR(pcie->clk), > + "could not enable clk\n"); > + > + pcie->clk_reg = devm_clk_get_enabled(dev, "reg"); > + if (IS_ERR(pcie->clk_reg)) { > + ret = dev_err_probe(dev, PTR_ERR(pcie->clk_reg), > + "could not enable reg clk\n"); > + if (ret == -EPROBE_DEFER) > + goto out; You can drop this check as dev_err_probe handle this inside It will defer the enabling of clock. > } > > /* Get the dw-pcie unit configuration/control registers base. */ > @@ -308,12 +302,12 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); > if (IS_ERR(pci->dbi_base)) { > ret = PTR_ERR(pci->dbi_base); > - goto fail_clkreg; > + goto out; > } > > ret = armada8k_pcie_setup_phys(pcie); > if (ret) > - goto fail_clkreg; > + goto out; > > platform_set_drvdata(pdev, pcie); > > @@ -325,11 +319,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > > disable_phy: > armada8k_pcie_disable_phys(pcie); > -fail_clkreg: > - clk_disable_unprepare(pcie->clk_reg); > -fail: > - clk_disable_unprepare(pcie->clk); > - > +out: > return ret; > } > Thanks -Anand > -- > 2.25.1 > >
On 2024/8/27 14:44, Anand Moon wrote: > Hi Wu Bo, > > On Tue, 27 Aug 2024 at 07:55, Wu Bo <bo.wu@vivo.com> wrote: >> Use devm_clk_get_enabled() instead of devm_clk_get() to make the code >> cleaner and avoid calling clk_disable_unprepare() >> >> Signed-off-by: Wu Bo <bo.wu@vivo.com> >> --- >> drivers/pci/controller/dwc/pcie-armada8k.c | 36 ++++++++-------------- >> 1 file changed, 13 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c >> index b5c599ccaacf..e7ef6c2641b8 100644 >> --- a/drivers/pci/controller/dwc/pcie-armada8k.c >> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c >> @@ -284,23 +284,17 @@ static int armada8k_pcie_probe(struct platform_device *pdev) >> >> pcie->pci = pci; >> >> - pcie->clk = devm_clk_get(dev, NULL); >> + pcie->clk = devm_clk_get_enabled(dev, NULL); >> if (IS_ERR(pcie->clk)) >> - return PTR_ERR(pcie->clk); >> - >> - ret = clk_prepare_enable(pcie->clk); >> - if (ret) >> - return ret; >> - >> - pcie->clk_reg = devm_clk_get(dev, "reg"); >> - if (pcie->clk_reg == ERR_PTR(-EPROBE_DEFER)) { >> - ret = -EPROBE_DEFER; >> - goto fail; >> - } I don't know much about this device. But from the code here, its previous logic is that the function will only return when the error code is EPROBE_DEFER, and other errors will continue to execute. So I followed the previous logic, is it correct？ >> - if (!IS_ERR(pcie->clk_reg)) { >> - ret = clk_prepare_enable(pcie->clk_reg); >> - if (ret) >> - goto fail_clkreg; >> + return dev_err_probe(dev, PTR_ERR(pcie->clk), >> + "could not enable clk\n"); >> + >> + pcie->clk_reg = devm_clk_get_enabled(dev, "reg"); >> + if (IS_ERR(pcie->clk_reg)) { >> + ret = dev_err_probe(dev, PTR_ERR(pcie->clk_reg), >> + "could not enable reg clk\n"); >> + if (ret == -EPROBE_DEFER) >> + goto out; > You can drop this check as dev_err_probe handle this inside > It will defer the enabling of clock. >> } >> >> /* Get the dw-pcie unit configuration/control registers base. */ >> @@ -308,12 +302,12 @@ static int armada8k_pcie_probe(struct platform_device *pdev) >> pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); >> if (IS_ERR(pci->dbi_base)) { >> ret = PTR_ERR(pci->dbi_base); >> - goto fail_clkreg; >> + goto out; >> } >> >> ret = armada8k_pcie_setup_phys(pcie); >> if (ret) >> - goto fail_clkreg; >> + goto out; >> >> platform_set_drvdata(pdev, pcie); >> >> @@ -325,11 +319,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) >> >> disable_phy: >> armada8k_pcie_disable_phys(pcie); >> -fail_clkreg: >> - clk_disable_unprepare(pcie->clk_reg); >> -fail: >> - clk_disable_unprepare(pcie->clk); >> - >> +out: >> return ret; >> } >> > Thanks > -Anand >> -- >> 2.25.1 >> >>
Hi Wu Bo, On Sat, 31 Aug 2024 at 05:36, Wu Bo <wubo.oduw@gmail.com> wrote: > > On 2024/8/27 14:44, Anand Moon wrote: > > Hi Wu Bo, > > > > On Tue, 27 Aug 2024 at 07:55, Wu Bo <bo.wu@vivo.com> wrote: > >> Use devm_clk_get_enabled() instead of devm_clk_get() to make the code > >> cleaner and avoid calling clk_disable_unprepare() > >> > >> Signed-off-by: Wu Bo <bo.wu@vivo.com> > >> --- > >> drivers/pci/controller/dwc/pcie-armada8k.c | 36 ++++++++-------------- > >> 1 file changed, 13 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c > >> index b5c599ccaacf..e7ef6c2641b8 100644 > >> --- a/drivers/pci/controller/dwc/pcie-armada8k.c > >> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c > >> @@ -284,23 +284,17 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > >> > >> pcie->pci = pci; > >> > >> - pcie->clk = devm_clk_get(dev, NULL); > >> + pcie->clk = devm_clk_get_enabled(dev, NULL); > >> if (IS_ERR(pcie->clk)) > >> - return PTR_ERR(pcie->clk); > >> - > >> - ret = clk_prepare_enable(pcie->clk); > >> - if (ret) > >> - return ret; > >> - > >> - pcie->clk_reg = devm_clk_get(dev, "reg"); > >> - if (pcie->clk_reg == ERR_PTR(-EPROBE_DEFER)) { > >> - ret = -EPROBE_DEFER; > >> - goto fail; > >> - } > > I don't know much about this device. But from the code here, its > previous logic is that the function will only return when the error code > is EPROBE_DEFER, and other errors will continue to execute. > > So I followed the previous logic, is it correct？ We probably get -EPROBE_DEFER since the clk is not getting enabled during probe. and we defer the initialization by returning the error. using dev_err_probe function. . We don't need to recheck for EPROBE_DEFER, again it's handled in dev_err_probe see below. [1] https://elixir.bootlin.com/linux/v6.10.7/source/drivers/base/core.c#L5018 Thanks -Anand > > >> - if (!IS_ERR(pcie->clk_reg)) { > >> - ret = clk_prepare_enable(pcie->clk_reg); > >> - if (ret) > >> - goto fail_clkreg; > >> + return dev_err_probe(dev, PTR_ERR(pcie->clk), > >> + "could not enable clk\n"); > >> + > >> + pcie->clk_reg = devm_clk_get_enabled(dev, "reg"); > >> + if (IS_ERR(pcie->clk_reg)) { > >> + ret = dev_err_probe(dev, PTR_ERR(pcie->clk_reg), > >> + "could not enable reg clk\n"); > >> + if (ret == -EPROBE_DEFER) > >> + goto out; > > You can drop this check as dev_err_probe handle this inside > > It will defer the enabling of clock. > >> } > >> > >> /* Get the dw-pcie unit configuration/control registers base. */ > >> @@ -308,12 +302,12 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > >> pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); > >> if (IS_ERR(pci->dbi_base)) { > >> ret = PTR_ERR(pci->dbi_base); > >> - goto fail_clkreg; > >> + goto out; > >> } > >> > >> ret = armada8k_pcie_setup_phys(pcie); > >> if (ret) > >> - goto fail_clkreg; > >> + goto out; > >> > >> platform_set_drvdata(pdev, pcie); > >> > >> @@ -325,11 +319,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > >> > >> disable_phy: > >> armada8k_pcie_disable_phys(pcie); > >> -fail_clkreg: > >> - clk_disable_unprepare(pcie->clk_reg); > >> -fail: > >> - clk_disable_unprepare(pcie->clk); > >> - > >> +out: > >> return ret; > >> } > >> > > Thanks > > -Anand > >> -- > >> 2.25.1 > >> > >>
diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c index b5c599ccaacf..e7ef6c2641b8 100644 --- a/drivers/pci/controller/dwc/pcie-armada8k.c +++ b/drivers/pci/controller/dwc/pcie-armada8k.c @@ -284,23 +284,17 @@ static int armada8k_pcie_probe(struct platform_device *pdev) pcie->pci = pci; - pcie->clk = devm_clk_get(dev, NULL); + pcie->clk = devm_clk_get_enabled(dev, NULL); if (IS_ERR(pcie->clk)) - return PTR_ERR(pcie->clk); - - ret = clk_prepare_enable(pcie->clk); - if (ret) - return ret; - - pcie->clk_reg = devm_clk_get(dev, "reg"); - if (pcie->clk_reg == ERR_PTR(-EPROBE_DEFER)) { - ret = -EPROBE_DEFER; - goto fail; - } - if (!IS_ERR(pcie->clk_reg)) { - ret = clk_prepare_enable(pcie->clk_reg); - if (ret) - goto fail_clkreg; + return dev_err_probe(dev, PTR_ERR(pcie->clk), + "could not enable clk\n"); + + pcie->clk_reg = devm_clk_get_enabled(dev, "reg"); + if (IS_ERR(pcie->clk_reg)) { + ret = dev_err_probe(dev, PTR_ERR(pcie->clk_reg), + "could not enable reg clk\n"); + if (ret == -EPROBE_DEFER) + goto out; } /* Get the dw-pcie unit configuration/control registers base. */ @@ -308,12 +302,12 @@ static int armada8k_pcie_probe(struct platform_device *pdev) pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); if (IS_ERR(pci->dbi_base)) { ret = PTR_ERR(pci->dbi_base); - goto fail_clkreg; + goto out; } ret = armada8k_pcie_setup_phys(pcie); if (ret) - goto fail_clkreg; + goto out; platform_set_drvdata(pdev, pcie); @@ -325,11 +319,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) disable_phy: armada8k_pcie_disable_phys(pcie); -fail_clkreg: - clk_disable_unprepare(pcie->clk_reg); -fail: - clk_disable_unprepare(pcie->clk); - +out: return ret; }
Use devm_clk_get_enabled() instead of devm_clk_get() to make the code cleaner and avoid calling clk_disable_unprepare() Signed-off-by: Wu Bo <bo.wu@vivo.com> --- drivers/pci/controller/dwc/pcie-armada8k.c | 36 ++++++++-------------- 1 file changed, 13 insertions(+), 23 deletions(-)