2024-04-04 07:15:04

by Anand Moon

[permalink] [raw]
Subject: [PATCH v2 1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers

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 <[email protected]>
---
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");
}

- 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);
}

--
2.44.0



2024-04-04 13:01:07

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers

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 <[email protected]>
> ---
> 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

2024-04-04 13:52:37

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers

Hi Greg,

On Thu, 4 Apr 2024 at 18:30, Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > ---
> > 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

2024-04-04 13:54:27

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers

On 04/04/2024 15:52, Anand Moon wrote:
> Hi Greg,
>
> On Thu, 4 Apr 2024 at 18:30, Greg Kroah-Hartman
> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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


2024-04-08 10:03:36

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers

Hi Krzysztof, Greg.

On Thu, 4 Apr 2024 at 19:24, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 04/04/2024 15:52, Anand Moon wrote:
> > Hi Greg,
> >
> > On Thu, 4 Apr 2024 at 18:30, Greg Kroah-Hartman
> > <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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