| Message ID | 20240404071350.4242-2-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers | expand |
On Thu, Apr 04, 2024 at 12:43:17PM +0530, Anand Moon wrote: > The devm_clk_get_enabled() helpers: > - call devm_clk_get() > - call clk_prepare_enable() and register what is needed in order to > call clk_disable_unprepare() when needed, as a managed resource. > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > While at it, use dev_err_probe consistently, and use its return value > to return the error code. > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > V2: drop the clk_disable_unprepare in suspend/resume functions > fix the usb_put_hcd return before the devm_clk_get_enabled > --- > drivers/usb/host/ehci-exynos.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > index f644b131cc0b..f00bfd0b13dc 100644 > --- a/drivers/usb/host/ehci-exynos.c > +++ b/drivers/usb/host/ehci-exynos.c > @@ -159,20 +159,15 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > if (err) > - goto fail_clk; > - > - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > + goto fail_io; > > + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > if (IS_ERR(exynos_ehci->clk)) { > - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > - err = PTR_ERR(exynos_ehci->clk); > - goto fail_clk; > + usb_put_hcd(hcd); > + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > + "Failed to get usbhost clock\n"); Why is this logic changed? If you want to call dev_err_probe(), that's great, but do NOT mix it up with a commit that does something totally different. When you say something like "while at it" in a changelog text, that is a HUGE hint that it needs to be a separate commit. Because of that reason alone, I can't take these, you know better :( thanks, greg k-h
Hi Greg, On Thu, 4 Apr 2024 at 18:30, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Apr 04, 2024 at 12:43:17PM +0530, Anand Moon wrote: > > The devm_clk_get_enabled() helpers: > > - call devm_clk_get() > > - call clk_prepare_enable() and register what is needed in order to > > call clk_disable_unprepare() when needed, as a managed resource. > > > > This simplifies the code and avoids the calls to clk_disable_unprepare(). > > > > While at it, use dev_err_probe consistently, and use its return value > > to return the error code. > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > V2: drop the clk_disable_unprepare in suspend/resume functions > > fix the usb_put_hcd return before the devm_clk_get_enabled > > --- > > drivers/usb/host/ehci-exynos.c | 19 +++++-------------- > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > > index f644b131cc0b..f00bfd0b13dc 100644 > > --- a/drivers/usb/host/ehci-exynos.c > > +++ b/drivers/usb/host/ehci-exynos.c > > @@ -159,20 +159,15 @@ static int exynos_ehci_probe(struct platform_device *pdev) > > > > err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > > if (err) > > - goto fail_clk; > > - > > - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > > + goto fail_io; > > > > + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > > if (IS_ERR(exynos_ehci->clk)) { > > - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > > - err = PTR_ERR(exynos_ehci->clk); > > - goto fail_clk; > > + usb_put_hcd(hcd); > > + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > > + "Failed to get usbhost clock\n"); > > Why is this logic changed? > > If you want to call dev_err_probe(), that's great, but do NOT mix it up > with a commit that does something totally different. > > When you say something like "while at it" in a changelog text, that is a > HUGE hint that it needs to be a separate commit. Because of that reason > alone, I can't take these, you know better :( > > thanks, > Ok, I will improve the commit message relevant to the code changes. > greg k-h Thanks -Anand
On 04/04/2024 15:52, Anand Moon wrote: > Hi Greg, > > On Thu, 4 Apr 2024 at 18:30, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> >> On Thu, Apr 04, 2024 at 12:43:17PM +0530, Anand Moon wrote: >>> The devm_clk_get_enabled() helpers: >>> - call devm_clk_get() >>> - call clk_prepare_enable() and register what is needed in order to >>> call clk_disable_unprepare() when needed, as a managed resource. >>> >>> This simplifies the code and avoids the calls to clk_disable_unprepare(). >>> >>> While at it, use dev_err_probe consistently, and use its return value >>> to return the error code. >>> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com> >>> --- >>> V2: drop the clk_disable_unprepare in suspend/resume functions >>> fix the usb_put_hcd return before the devm_clk_get_enabled >>> --- >>> drivers/usb/host/ehci-exynos.c | 19 +++++-------------- >>> 1 file changed, 5 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c >>> index f644b131cc0b..f00bfd0b13dc 100644 >>> --- a/drivers/usb/host/ehci-exynos.c >>> +++ b/drivers/usb/host/ehci-exynos.c >>> @@ -159,20 +159,15 @@ static int exynos_ehci_probe(struct platform_device *pdev) >>> >>> err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); >>> if (err) >>> - goto fail_clk; >>> - >>> - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); >>> + goto fail_io; >>> >>> + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); >>> if (IS_ERR(exynos_ehci->clk)) { >>> - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); >>> - err = PTR_ERR(exynos_ehci->clk); >>> - goto fail_clk; >>> + usb_put_hcd(hcd); >>> + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), >>> + "Failed to get usbhost clock\n"); >> >> Why is this logic changed? >> >> If you want to call dev_err_probe(), that's great, but do NOT mix it up >> with a commit that does something totally different. >> >> When you say something like "while at it" in a changelog text, that is a >> HUGE hint that it needs to be a separate commit. Because of that reason >> alone, I can't take these, you know better :( >> >> thanks, >> > > Ok, I will improve the commit message relevant to the code changes. Please read Greg's message one more time. He did not propose to fix commit msg, right? Best regards, Krzysztof
Hi Krzysztof, Greg. On Thu, 4 Apr 2024 at 19:24, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 04/04/2024 15:52, Anand Moon wrote: > > Hi Greg, > > > > On Thu, 4 Apr 2024 at 18:30, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > >> > >> On Thu, Apr 04, 2024 at 12:43:17PM +0530, Anand Moon wrote: > >>> The devm_clk_get_enabled() helpers: > >>> - call devm_clk_get() > >>> - call clk_prepare_enable() and register what is needed in order to > >>> call clk_disable_unprepare() when needed, as a managed resource. > >>> > >>> This simplifies the code and avoids the calls to clk_disable_unprepare(). > >>> > >>> While at it, use dev_err_probe consistently, and use its return value > >>> to return the error code. > >>> > >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com> > >>> --- > >>> V2: drop the clk_disable_unprepare in suspend/resume functions > >>> fix the usb_put_hcd return before the devm_clk_get_enabled > >>> --- > >>> drivers/usb/host/ehci-exynos.c | 19 +++++-------------- > >>> 1 file changed, 5 insertions(+), 14 deletions(-) > >>> > >>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > >>> index f644b131cc0b..f00bfd0b13dc 100644 > >>> --- a/drivers/usb/host/ehci-exynos.c > >>> +++ b/drivers/usb/host/ehci-exynos.c > >>> @@ -159,20 +159,15 @@ static int exynos_ehci_probe(struct platform_device *pdev) > >>> > >>> err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); > >>> if (err) > >>> - goto fail_clk; > >>> - > >>> - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); > >>> + goto fail_io; > >>> > >>> + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); > >>> if (IS_ERR(exynos_ehci->clk)) { > >>> - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); > >>> - err = PTR_ERR(exynos_ehci->clk); > >>> - goto fail_clk; > >>> + usb_put_hcd(hcd); > >>> + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), > >>> + "Failed to get usbhost clock\n"); > >> > >> Why is this logic changed? > >> > >> If you want to call dev_err_probe(), that's great, but do NOT mix it up > >> with a commit that does something totally different. > >> > >> When you say something like "while at it" in a changelog text, that is a > >> HUGE hint that it needs to be a separate commit. Because of that reason > >> alone, I can't take these, you know better :( > >> > >> thanks, > >> > > > > Ok, I will improve the commit message relevant to the code changes. > > Please read Greg's message one more time. He did not propose to fix > commit msg, right? > Ok I will drop dev_err_probe and keep the error flow as it is. It will not break the feature.. Thanks -Anand
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index f644b131cc0b..f00bfd0b13dc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -159,20 +159,15 @@ static int exynos_ehci_probe(struct platform_device *pdev) err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci); if (err) - goto fail_clk; - - exynos_ehci->clk = devm_clk_get(&pdev->dev, "usbhost"); + goto fail_io; + exynos_ehci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost"); if (IS_ERR(exynos_ehci->clk)) { - dev_err(&pdev->dev, "Failed to get usbhost clock\n"); - err = PTR_ERR(exynos_ehci->clk); - goto fail_clk; + usb_put_hcd(hcd); + return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ehci->clk), + "Failed to get usbhost clock\n"); } - err = clk_prepare_enable(exynos_ehci->clk); - if (err) - goto fail_clk; - hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); if (IS_ERR(hcd->regs)) { err = PTR_ERR(hcd->regs); @@ -223,8 +218,6 @@ static int exynos_ehci_probe(struct platform_device *pdev) exynos_ehci_phy_disable(&pdev->dev); pdev->dev.of_node = exynos_ehci->of_node; fail_io: - clk_disable_unprepare(exynos_ehci->clk); -fail_clk: usb_put_hcd(hcd); return err; } @@ -240,8 +233,6 @@ static void exynos_ehci_remove(struct platform_device *pdev) exynos_ehci_phy_disable(&pdev->dev); - clk_disable_unprepare(exynos_ehci->clk); - usb_put_hcd(hcd); }
The devm_clk_get_enabled() helpers: - call devm_clk_get() - call clk_prepare_enable() and register what is needed in order to call clk_disable_unprepare() when needed, as a managed resource. This simplifies the code and avoids the calls to clk_disable_unprepare(). While at it, use dev_err_probe consistently, and use its return value to return the error code. Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- V2: drop the clk_disable_unprepare in suspend/resume functions fix the usb_put_hcd return before the devm_clk_get_enabled --- drivers/usb/host/ehci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)