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

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

[Anand Moon]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/6] usb: ehci-exynos: Switch from CONFIG_PM guards to pm_ptr()

[Anand Moon]

Use the new PM macros for the suspend and resume functions to be
automatically dropped by the compiler when CONFIG_PM are disabled,
without having to use #ifdef guards. If CONFIG_PM unused,
they will simply be discarded by the compiler.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
disabled.
    dropped RUNTIME_PM_OPS
---
 drivers/usb/host/ehci-exynos.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index f00bfd0b13dc..4cfde1af32be 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -236,8 +236,7 @@ static void exynos_ehci_remove(struct platform_device *pdev)
 	usb_put_hcd(hcd);
 }
 
-#ifdef CONFIG_PM
-static int exynos_ehci_suspend(struct device *dev)
+static int __maybe_unused exynos_ehci_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
@@ -256,7 +255,7 @@ static int exynos_ehci_suspend(struct device *dev)
 	return rc;
 }
 
-static int exynos_ehci_resume(struct device *dev)
+static int __maybe_unused exynos_ehci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
@@ -279,10 +278,6 @@ static int exynos_ehci_resume(struct device *dev)
 	ehci_resume(hcd, false);
 	return 0;
 }
-#else
-#define exynos_ehci_suspend	NULL
-#define exynos_ehci_resume	NULL
-#endif
 
 static const struct dev_pm_ops exynos_ehci_pm_ops = {
 	.suspend	= exynos_ehci_suspend,
@@ -303,7 +298,7 @@ static struct platform_driver exynos_ehci_driver = {
 	.shutdown	= usb_hcd_platform_shutdown,
 	.driver = {
 		.name	= "exynos-ehci",
-		.pm	= &exynos_ehci_pm_ops,
+		.pm	= pm_ptr(&exynos_ehci_pm_ops),
 		.of_match_table = of_match_ptr(exynos_ehci_match),
 	}
 };
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/6] usb: ehci-exynos: Switch from CONFIG_PM guards to pm_ptr()

[Alan Stern]

On Thu, Apr 04, 2024 at 12:43:18PM +0530, Anand Moon wrote:
> Use the new PM macros for the suspend and resume functions to be
> automatically dropped by the compiler when CONFIG_PM are disabled,
> without having to use #ifdef guards. If CONFIG_PM unused,
> they will simply be discarded by the compiler.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
> disabled.
>     dropped RUNTIME_PM_OPS
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/host/ehci-exynos.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> index f00bfd0b13dc..4cfde1af32be 100644
> --- a/drivers/usb/host/ehci-exynos.c
> +++ b/drivers/usb/host/ehci-exynos.c
> @@ -236,8 +236,7 @@ static void exynos_ehci_remove(struct platform_device *pdev)
>  	usb_put_hcd(hcd);
>  }
>  
> -#ifdef CONFIG_PM
> -static int exynos_ehci_suspend(struct device *dev)
> +static int __maybe_unused exynos_ehci_suspend(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
>  	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> @@ -256,7 +255,7 @@ static int exynos_ehci_suspend(struct device *dev)
>  	return rc;
>  }
>  
> -static int exynos_ehci_resume(struct device *dev)
> +static int __maybe_unused exynos_ehci_resume(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
>  	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> @@ -279,10 +278,6 @@ static int exynos_ehci_resume(struct device *dev)
>  	ehci_resume(hcd, false);
>  	return 0;
>  }
> -#else
> -#define exynos_ehci_suspend	NULL
> -#define exynos_ehci_resume	NULL
> -#endif
>  
>  static const struct dev_pm_ops exynos_ehci_pm_ops = {
>  	.suspend	= exynos_ehci_suspend,
> @@ -303,7 +298,7 @@ static struct platform_driver exynos_ehci_driver = {
>  	.shutdown	= usb_hcd_platform_shutdown,
>  	.driver = {
>  		.name	= "exynos-ehci",
> -		.pm	= &exynos_ehci_pm_ops,
> +		.pm	= pm_ptr(&exynos_ehci_pm_ops),
>  		.of_match_table = of_match_ptr(exynos_ehci_match),
>  	}
>  };
> -- 
> 2.44.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/6] usb: ohci-exynos: Use devm_clk_get_enabled() helpers

[Anand Moon]

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: new changes in this series.
---
 drivers/usb/host/ohci-exynos.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 20e26a474591..85d04ae0ae40 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -135,20 +135,15 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 
 	err = exynos_ohci_get_phy(&pdev->dev, exynos_ohci);
 	if (err)
-		goto fail_clk;
-
-	exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
+		goto fail_io;
 
+	exynos_ohci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost");
 	if (IS_ERR(exynos_ohci->clk)) {
-		dev_err(&pdev->dev, "Failed to get usbhost clock\n");
-		err = PTR_ERR(exynos_ohci->clk);
-		goto fail_clk;
+		usb_put_hcd(hcd);
+		return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ohci->clk),
+				"Failed to get usbhost clock\n");
 	}
 
-	err = clk_prepare_enable(exynos_ohci->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);
@@ -191,8 +186,6 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 	exynos_ohci_phy_disable(&pdev->dev);
 	pdev->dev.of_node = exynos_ohci->of_node;
 fail_io:
-	clk_disable_unprepare(exynos_ohci->clk);
-fail_clk:
 	usb_put_hcd(hcd);
 	return err;
 }
@@ -208,8 +201,6 @@ static void exynos_ohci_remove(struct platform_device *pdev)
 
 	exynos_ohci_phy_disable(&pdev->dev);
 
-	clk_disable_unprepare(exynos_ohci->clk);
-
 	usb_put_hcd(hcd);
 }
 
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/6] usb: ohci-exynos: Use devm_clk_get_enabled() helpers

[Krzysztof Kozlowski]

On 04/04/2024 09:13, 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: new changes in this series.
> ---
>  drivers/usb/host/ohci-exynos.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 20e26a474591..85d04ae0ae40 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -135,20 +135,15 @@ static int exynos_ohci_probe(struct platform_device *pdev)
>  
>  	err = exynos_ohci_get_phy(&pdev->dev, exynos_ohci);
>  	if (err)
> -		goto fail_clk;
> -
> -	exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
> +		goto fail_io;
>  
> +	exynos_ohci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost");
>  	if (IS_ERR(exynos_ohci->clk)) {
> -		dev_err(&pdev->dev, "Failed to get usbhost clock\n");
> -		err = PTR_ERR(exynos_ohci->clk);
> -		goto fail_clk;
> +		usb_put_hcd(hcd);
> +		return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ohci->clk),
> +				"Failed to get usbhost clock\n");

Why do you introduce entirely parallel exit paths? There is already
single error handling part with labels. Use that.



Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/6] usb: ohci-exynos: Use devm_clk_get_enabled() helpers

[Anand Moon]

Hi Krzysztof,


On Thu, 4 Apr 2024 at 12:50, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/04/2024 09:13, 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: new changes in this series.
> > ---
> >  drivers/usb/host/ohci-exynos.c | 19 +++++--------------
> >  1 file changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> > index 20e26a474591..85d04ae0ae40 100644
> > --- a/drivers/usb/host/ohci-exynos.c
> > +++ b/drivers/usb/host/ohci-exynos.c
> > @@ -135,20 +135,15 @@ static int exynos_ohci_probe(struct platform_device *pdev)
> >
> >       err = exynos_ohci_get_phy(&pdev->dev, exynos_ohci);
> >       if (err)
> > -             goto fail_clk;
> > -
> > -     exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
> > +             goto fail_io;
> >
> > +     exynos_ohci->clk = devm_clk_get_enabled(&pdev->dev, "usbhost");
> >       if (IS_ERR(exynos_ohci->clk)) {
> > -             dev_err(&pdev->dev, "Failed to get usbhost clock\n");
> > -             err = PTR_ERR(exynos_ohci->clk);
> > -             goto fail_clk;
> > +             usb_put_hcd(hcd);
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(exynos_ohci->clk),
> > +                             "Failed to get usbhost clock\n");
>
> Why do you introduce entirely parallel exit paths? There is already
> single error handling part with labels. Use that.
>
OK, I  will use the labels to return over here,
>
>
> Best regards,
> Krzysztof
>
Thanks

-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/6] usb: ohci-exynos: Switch from CONFIG_PM guards to pm_ptr()

[Anand Moon]

Use the new PM macros for the suspend and resume functions to be
automatically dropped by the compiler when CONFIG_PM are disabled,
without having to use #ifdef guards. If CONFIG_PM unused,
they will simply be discarded by the compiler.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v2: new changes in this series.
---
 drivers/usb/host/ohci-exynos.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 85d04ae0ae40..3e647e0b341d 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -212,8 +212,7 @@ static void exynos_ohci_shutdown(struct platform_device *pdev)
 		hcd->driver->shutdown(hcd);
 }
 
-#ifdef CONFIG_PM
-static int exynos_ohci_suspend(struct device *dev)
+static int __maybe_unused exynos_ohci_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
@@ -230,7 +229,7 @@ static int exynos_ohci_suspend(struct device *dev)
 	return 0;
 }
 
-static int exynos_ohci_resume(struct device *dev)
+static int __maybe_unused exynos_ohci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd			= dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci	= to_exynos_ohci(hcd);
@@ -249,10 +248,6 @@ static int exynos_ohci_resume(struct device *dev)
 
 	return 0;
 }
-#else
-#define exynos_ohci_suspend	NULL
-#define exynos_ohci_resume	NULL
-#endif
 
 static const struct ohci_driver_overrides exynos_overrides __initconst = {
 	.extra_priv_size =	sizeof(struct exynos_ohci_hcd),
@@ -277,7 +272,7 @@ static struct platform_driver exynos_ohci_driver = {
 	.shutdown	= exynos_ohci_shutdown,
 	.driver = {
 		.name	= "exynos-ohci",
-		.pm	= &exynos_ohci_pm_ops,
+		.pm	= pm_ptr(&exynos_ohci_pm_ops),
 		.of_match_table	= of_match_ptr(exynos_ohci_match),
 	}
 };
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/6] usb: ohci-exynos: Switch from CONFIG_PM guards to pm_ptr()

[Alan Stern]

On Thu, Apr 04, 2024 at 12:43:20PM +0530, Anand Moon wrote:
> Use the new PM macros for the suspend and resume functions to be
> automatically dropped by the compiler when CONFIG_PM are disabled,
> without having to use #ifdef guards. If CONFIG_PM unused,
> they will simply be discarded by the compiler.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v2: new changes in this series.
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/host/ohci-exynos.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 85d04ae0ae40..3e647e0b341d 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -212,8 +212,7 @@ static void exynos_ohci_shutdown(struct platform_device *pdev)
>  		hcd->driver->shutdown(hcd);
>  }
>  
> -#ifdef CONFIG_PM
> -static int exynos_ohci_suspend(struct device *dev)
> +static int __maybe_unused exynos_ohci_suspend(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
>  	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> @@ -230,7 +229,7 @@ static int exynos_ohci_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int exynos_ohci_resume(struct device *dev)
> +static int __maybe_unused exynos_ohci_resume(struct device *dev)
>  {
>  	struct usb_hcd *hcd			= dev_get_drvdata(dev);
>  	struct exynos_ohci_hcd *exynos_ohci	= to_exynos_ohci(hcd);
> @@ -249,10 +248,6 @@ static int exynos_ohci_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#else
> -#define exynos_ohci_suspend	NULL
> -#define exynos_ohci_resume	NULL
> -#endif
>  
>  static const struct ohci_driver_overrides exynos_overrides __initconst = {
>  	.extra_priv_size =	sizeof(struct exynos_ohci_hcd),
> @@ -277,7 +272,7 @@ static struct platform_driver exynos_ohci_driver = {
>  	.shutdown	= exynos_ohci_shutdown,
>  	.driver = {
>  		.name	= "exynos-ohci",
> -		.pm	= &exynos_ohci_pm_ops,
> +		.pm	= pm_ptr(&exynos_ohci_pm_ops),
>  		.of_match_table	= of_match_ptr(exynos_ohci_match),
>  	}
>  };
> -- 
> 2.44.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function

[Anand Moon]

Use devm_regulator_bulk_get_enable() instead of open coded
'devm_regulator_get(), regulator_enable(), regulator_disable().

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
V2: no changes, did not find any regression in pm suspend/resume.
---
 drivers/usb/dwc3/dwc3-exynos.c | 50 ++++------------------------------
 1 file changed, 5 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 5d365ca51771..2d341f0e22a3 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -32,9 +32,6 @@ struct dwc3_exynos {
 	struct clk		*clks[DWC3_EXYNOS_MAX_CLOCKS];
 	int			num_clks;
 	int			suspend_clk_idx;
-
-	struct regulator	*vdd33;
-	struct regulator	*vdd10;
 };
 
 static int dwc3_exynos_probe(struct platform_device *pdev)
@@ -44,6 +41,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	struct device_node	*node = dev->of_node;
 	const struct dwc3_exynos_driverdata *driver_data;
 	int			i, ret;
+	static const char * const regulators[] = { "vdd33", "vdd10" };
 
 	exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL);
 	if (!exynos)
@@ -78,27 +76,10 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	if (exynos->suspend_clk_idx >= 0)
 		clk_prepare_enable(exynos->clks[exynos->suspend_clk_idx]);
 
-	exynos->vdd33 = devm_regulator_get(dev, "vdd33");
-	if (IS_ERR(exynos->vdd33)) {
-		ret = PTR_ERR(exynos->vdd33);
-		goto vdd33_err;
-	}
-	ret = regulator_enable(exynos->vdd33);
-	if (ret) {
-		dev_err(dev, "Failed to enable VDD33 supply\n");
-		goto vdd33_err;
-	}
-
-	exynos->vdd10 = devm_regulator_get(dev, "vdd10");
-	if (IS_ERR(exynos->vdd10)) {
-		ret = PTR_ERR(exynos->vdd10);
-		goto vdd10_err;
-	}
-	ret = regulator_enable(exynos->vdd10);
-	if (ret) {
-		dev_err(dev, "Failed to enable VDD10 supply\n");
-		goto vdd10_err;
-	}
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
 
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, dev);
@@ -115,10 +96,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	return 0;
 
 populate_err:
-	regulator_disable(exynos->vdd10);
-vdd10_err:
-	regulator_disable(exynos->vdd33);
-vdd33_err:
 	for (i = exynos->num_clks - 1; i >= 0; i--)
 		clk_disable_unprepare(exynos->clks[i]);
 
@@ -140,9 +117,6 @@ static void dwc3_exynos_remove(struct platform_device *pdev)
 
 	if (exynos->suspend_clk_idx >= 0)
 		clk_disable_unprepare(exynos->clks[exynos->suspend_clk_idx]);
-
-	regulator_disable(exynos->vdd33);
-	regulator_disable(exynos->vdd10);
 }
 
 static const struct dwc3_exynos_driverdata exynos5250_drvdata = {
@@ -196,9 +170,6 @@ static int dwc3_exynos_suspend(struct device *dev)
 	for (i = exynos->num_clks - 1; i >= 0; i--)
 		clk_disable_unprepare(exynos->clks[i]);
 
-	regulator_disable(exynos->vdd33);
-	regulator_disable(exynos->vdd10);
-
 	return 0;
 }
 
@@ -207,17 +178,6 @@ static int dwc3_exynos_resume(struct device *dev)
 	struct dwc3_exynos *exynos = dev_get_drvdata(dev);
 	int i, ret;
 
-	ret = regulator_enable(exynos->vdd33);
-	if (ret) {
-		dev_err(dev, "Failed to enable VDD33 supply\n");
-		return ret;
-	}
-	ret = regulator_enable(exynos->vdd10);
-	if (ret) {
-		dev_err(dev, "Failed to enable VDD10 supply\n");
-		return ret;
-	}
-
 	for (i = 0; i < exynos->num_clks; i++) {
 		ret = clk_prepare_enable(exynos->clks[i]);
 		if (ret) {
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function

[Krzysztof Kozlowski]

On 04/04/2024 09:13, Anand Moon wrote:
> Use devm_regulator_bulk_get_enable() instead of open coded
> 'devm_regulator_get(), regulator_enable(), regulator_disable().

I fail to see how did you replace open-coded suspend/resume paths.

> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> V2: no changes, did not find any regression in pm suspend/resume.

No, that's not equivalent code. No explanation in commit msg.

You already got comments on this and nothing improved. You just entirely
ignored received comments. That's not how it works.

I don't think you understand the code and Linux driver model. This patch
repeats several previous attempts with similar issues: no logic behind a
change.

NAK.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function

[Anand Moon]

Hi Krzysztof,

On Thu, 4 Apr 2024 at 12:53, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/04/2024 09:13, Anand Moon wrote:
> > Use devm_regulator_bulk_get_enable() instead of open coded
> > 'devm_regulator_get(), regulator_enable(), regulator_disable().
>
> I fail to see how did you replace open-coded suspend/resume paths.
>
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > V2: no changes, did not find any regression in pm suspend/resume.
>
> No, that's not equivalent code. No explanation in commit msg.
>
> You already got comments on this and nothing improved. You just entirely
> ignored received comments. That's not how it works.
>
> I don't think you understand the code and Linux driver model. This patch
> repeats several previous attempts with similar issues: no logic behind a
> change.
>
> NAK.

devm_regulator_get_enable and devm_regulator_bulk_get_enable
both remove the dependency from the driver to handle the regulator_enabled
and regulator_disabled. ie this removes the regulator from the driver structure.

Since these functions set devm_add_action to disable the regulator when the
resource is not used.

     ret = devm_add_action(dev, devm_regulator_bulk_disable, devres);
     if (!ret)
               return 0;
>
> Best regards,
> Krzysztof
>

if you feel it's incorrect, I will drop this patch..

Thanks
-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function

[Krzysztof Kozlowski]

On 04/04/2024 09:38, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Thu, 4 Apr 2024 at 12:53, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 04/04/2024 09:13, Anand Moon wrote:
>>> Use devm_regulator_bulk_get_enable() instead of open coded
>>> 'devm_regulator_get(), regulator_enable(), regulator_disable().
>>
>> I fail to see how did you replace open-coded suspend/resume paths.
>>
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> V2: no changes, did not find any regression in pm suspend/resume.
>>
>> No, that's not equivalent code. No explanation in commit msg.
>>
>> You already got comments on this and nothing improved. You just entirely
>> ignored received comments. That's not how it works.
>>
>> I don't think you understand the code and Linux driver model. This patch
>> repeats several previous attempts with similar issues: no logic behind a
>> change.
>>
>> NAK.
> 
> devm_regulator_get_enable and devm_regulator_bulk_get_enable
> both remove the dependency from the driver to handle the regulator_enabled
> and regulator_disabled. ie this removes the regulator from the driver structure.

Not true. Please do not paste some generic knowledge and assume reviewer
knows it. Instead provide proof.

> 
> Since these functions set devm_add_action to disable the regulator when the
> resource is not used.
> 
>      ret = devm_add_action(dev, devm_regulator_bulk_disable, devres);
>      if (!ret)
>                return 0;

Listen, you already got comments on this at v1. Address previous
comments instead of repeating something unrelated. We should not have
the same discussion twice.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

Use the new PM macros for the suspend and resume functions to be
automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
they will simply be discarded by the compiler.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
   disabled.
---
 drivers/usb/dwc3/dwc3-exynos.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 2d341f0e22a3..90259ad1d0d9 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -161,8 +161,7 @@ static const struct of_device_id exynos_dwc3_match[] = {
 };
 MODULE_DEVICE_TABLE(of, exynos_dwc3_match);
 
-#ifdef CONFIG_PM_SLEEP
-static int dwc3_exynos_suspend(struct device *dev)
+static int __maybe_unused dwc3_exynos_suspend(struct device *dev)
 {
 	struct dwc3_exynos *exynos = dev_get_drvdata(dev);
 	int i;
@@ -173,7 +172,7 @@ static int dwc3_exynos_suspend(struct device *dev)
 	return 0;
 }
 
-static int dwc3_exynos_resume(struct device *dev)
+static int __maybe_unused dwc3_exynos_resume(struct device *dev)
 {
 	struct dwc3_exynos *exynos = dev_get_drvdata(dev);
 	int i, ret;
@@ -194,18 +193,13 @@ static const struct dev_pm_ops dwc3_exynos_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_exynos_suspend, dwc3_exynos_resume)
 };
 
-#define DEV_PM_OPS	(&dwc3_exynos_dev_pm_ops)
-#else
-#define DEV_PM_OPS	NULL
-#endif /* CONFIG_PM_SLEEP */
-
 static struct platform_driver dwc3_exynos_driver = {
 	.probe		= dwc3_exynos_probe,
 	.remove_new	= dwc3_exynos_remove,
 	.driver		= {
 		.name	= "exynos-dwc3",
 		.of_match_table = exynos_dwc3_match,
-		.pm	= DEV_PM_OPS,
+		.pm	= pm_sleep_ptr(&dwc3_exynos_dev_pm_ops),
 	},
 };
 
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Thinh Nguyen]

On Thu, Apr 04, 2024, Anand Moon wrote:
> Use the new PM macros for the suspend and resume functions to be
> automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
> without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
> they will simply be discarded by the compiler.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
>    disabled.

The compiler discards the code, yet we still need __maybe_unused?

BR,
Thinh

> ---
>  drivers/usb/dwc3/dwc3-exynos.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

Hi Thinh,

On Tue, 9 Apr 2024 at 07:24, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Thu, Apr 04, 2024, Anand Moon wrote:
> > Use the new PM macros for the suspend and resume functions to be
> > automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
> > without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
> > they will simply be discarded by the compiler.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
> >    disabled.
>
> The compiler discards the code, yet we still need __maybe_unused?
>
Earlier version had not added this since but I removed the
guard.CONFIG_PM_SLEEP.
added __maybe_unused just to safeguard the function.

I have tried to build with config by disabling CONFIG_PM and CONFIG_PM_SLEEP
but could get the warning compilation by adding flag W=1
-Werror=unused-function.

diff --git a/arch/arm/configs/exynos_defconfig
b/arch/arm/configs/exynos_defconfig
index 7ad48fdda1da..43110e42076e 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -29,8 +29,19 @@ CONFIG_ARM_EXYNOS_CPUIDLE=y
 CONFIG_VFP=y
 CONFIG_NEON=y
 CONFIG_KERNEL_MODE_NEON=y
-CONFIG_PM_DEBUG=y
-CONFIG_PM_ADVANCED_DEBUG=y
+CONFIG_PM_SLEEP=n
+CONFIG_PM_SLEEP_SMP=n
+# CONFIG_PM_AUTOSLEEP is not set
+# CONFIG_PM_USERSPACE_AUTOSLEEP is not set
+# CONFIG_PM_WAKELOCKS is not set
+CONFIG_PM=n
+CONFIG_PM_DEBUG=n
+CONFIG_PM_ADVANCED_DEBUG=n
+# CONFIG_PM_TEST_SUSPEND is not set
+# CONFIG_PM_SLEEP_DEBUG=n
+#
+CONFIG_PM_DEBUG=n
+CONFIG_PM_ADVANCED_DEBUG=n
 CONFIG_ENERGY_MODEL=y
 CONFIG_KALLSYMS_ALL=y
 CONFIG_MODULES=y

But since these CONFIG_PM and CONFIG_PM_SLEEP cannot be disabled,
I am not getting any warning related to these functions.

Do you want me to remove __maybe_unused ?

alarm@archl-xu4b:~/linux-exynos-5.y-devel$ ./buildkernelexynos-arch.sh
arch/arm/configs/exynos_defconfig:43:warning: override: reassigning to
symbol PM_DEBUG
arch/arm/configs/exynos_defconfig:44:warning: override: reassigning to
symbol PM_ADVANCED_DEBUG
#
# configuration written to .config
#
  SYNC    include/config/auto.conf
  CALL    scripts/checksyscalls.sh
  CC      arch/arm/mach-exynos/pm.o

> BR,
> Thinh
>

Thanks
-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Thinh Nguyen]

On Wed, Apr 10, 2024, Anand Moon wrote:
> Hi Thinh,
> 
> On Tue, 9 Apr 2024 at 07:24, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Thu, Apr 04, 2024, Anand Moon wrote:
> > > Use the new PM macros for the suspend and resume functions to be
> > > automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
> > > without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
> > > they will simply be discarded by the compiler.
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > > v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
> > >    disabled.
> >
> > The compiler discards the code, yet we still need __maybe_unused?
> >
> Earlier version had not added this since but I removed the
> guard.CONFIG_PM_SLEEP.
> added __maybe_unused just to safeguard the function.
> 
> I have tried to build with config by disabling CONFIG_PM and CONFIG_PM_SLEEP
> but could get the warning compilation by adding flag W=1
> -Werror=unused-function.
> 

<snip>

> 
> But since these CONFIG_PM and CONFIG_PM_SLEEP cannot be disabled,
> I am not getting any warning related to these functions.
> 
> Do you want me to remove __maybe_unused ?
> 

The warning was there as expected. You should to use it along with
DEFINE_SIMPLE_DEV_PM_OPS(). Let me know if you still see the same
warning.

BR,
Thinh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

Hi Thinh,

On Thu, 11 Apr 2024 at 04:56, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Wed, Apr 10, 2024, Anand Moon wrote:
> > Hi Thinh,
> >
> > On Tue, 9 Apr 2024 at 07:24, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > >
> > > On Thu, Apr 04, 2024, Anand Moon wrote:
> > > > Use the new PM macros for the suspend and resume functions to be
> > > > automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
> > > > without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
> > > > they will simply be discarded by the compiler.
> > > >
> > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > ---
> > > > v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
> > > >    disabled.
> > >
> > > The compiler discards the code, yet we still need __maybe_unused?
> > >
> > Earlier version had not added this since but I removed the
> > guard.CONFIG_PM_SLEEP.
> > added __maybe_unused just to safeguard the function.
> >
> > I have tried to build with config by disabling CONFIG_PM and CONFIG_PM_SLEEP
> > but could get the warning compilation by adding flag W=1
> > -Werror=unused-function.
> >
>
> <snip>
>
> >
> > But since these CONFIG_PM and CONFIG_PM_SLEEP cannot be disabled,
> > I am not getting any warning related to these functions.
> >
> > Do you want me to remove __maybe_unused ?
> >
>
> The warning was there as expected. You should to use it along with
> DEFINE_SIMPLE_DEV_PM_OPS(). Let me know if you still see the same
> warning.
>

But the warnings are related to the following macros

#define PTR_IF(cond, ptr) ((cond) ? (ptr) : NULL)

#define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))
#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))

So if we can disable CONFIG_PM and CONFIG_PM_SLEEP options
the relevant function with the above macro will be set to NULL.
in this case, the compiler will discard the function in SET_SYSTEM_SLEEP_PM_OPS

> BR,
> Thinh

Thanks
-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Thinh Nguyen]

On Thu, Apr 11, 2024, Anand Moon wrote:
> Hi Thinh,
> 
> On Thu, 11 Apr 2024 at 04:56, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Wed, Apr 10, 2024, Anand Moon wrote:
> > > Hi Thinh,
> > >
> > > On Tue, 9 Apr 2024 at 07:24, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > > >
> > > > On Thu, Apr 04, 2024, Anand Moon wrote:
> > > > > Use the new PM macros for the suspend and resume functions to be
> > > > > automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
> > > > > without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
> > > > > they will simply be discarded by the compiler.
> > > > >
> > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > > ---
> > > > > v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
> > > > >    disabled.
> > > >
> > > > The compiler discards the code, yet we still need __maybe_unused?
> > > >
> > > Earlier version had not added this since but I removed the
> > > guard.CONFIG_PM_SLEEP.
> > > added __maybe_unused just to safeguard the function.
> > >
> > > I have tried to build with config by disabling CONFIG_PM and CONFIG_PM_SLEEP
> > > but could get the warning compilation by adding flag W=1
> > > -Werror=unused-function.
> > >
> >
> > <snip>
> >
> > >
> > > But since these CONFIG_PM and CONFIG_PM_SLEEP cannot be disabled,
> > > I am not getting any warning related to these functions.
> > >
> > > Do you want me to remove __maybe_unused ?
> > >
> >
> > The warning was there as expected. You should to use it along with
> > DEFINE_SIMPLE_DEV_PM_OPS(). Let me know if you still see the same
> > warning.
> >
> 
> But the warnings are related to the following macros
> 
> #define PTR_IF(cond, ptr) ((cond) ? (ptr) : NULL)
> 
> #define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))
> #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
> 
> So if we can disable CONFIG_PM and CONFIG_PM_SLEEP options
> the relevant function with the above macro will be set to NULL.
> in this case, the compiler will discard the function in SET_SYSTEM_SLEEP_PM_OPS
> 

There are differences if you use SET_SYSTEM_SLEEP_PM_OPS vs the new
macro. It's noted in this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1a3c7bb088266fa2db017be299f91f1c1894c857

Please try it out.

Thanks,
Thinh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

Hi Thinh,

On Fri, 12 Apr 2024 at 03:52, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Thu, Apr 11, 2024, Anand Moon wrote:
> > Hi Thinh,
> >
> > On Thu, 11 Apr 2024 at 04:56, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > >
> > > On Wed, Apr 10, 2024, Anand Moon wrote:
> > > > Hi Thinh,
> > > >
> > > > On Tue, 9 Apr 2024 at 07:24, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > > > >
> > > > > On Thu, Apr 04, 2024, Anand Moon wrote:
> > > > > > Use the new PM macros for the suspend and resume functions to be
> > > > > > automatically dropped by the compiler when CONFIG_PM_SLEEP are disabled,
> > > > > > without having to use #ifdef guards. If CONFIG_PM_SLEEP unused,
> > > > > > they will simply be discarded by the compiler.
> > > > > >
> > > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > > > ---
> > > > > > v2: add __maybe_unused to suspend/resume functions in case CONFIG_PM is
> > > > > >    disabled.
> > > > >
> > > > > The compiler discards the code, yet we still need __maybe_unused?
> > > > >
> > > > Earlier version had not added this since but I removed the
> > > > guard.CONFIG_PM_SLEEP.
> > > > added __maybe_unused just to safeguard the function.
> > > >
> > > > I have tried to build with config by disabling CONFIG_PM and CONFIG_PM_SLEEP
> > > > but could get the warning compilation by adding flag W=1
> > > > -Werror=unused-function.
> > > >
> > >
> > > <snip>
> > >
> > > >
> > > > But since these CONFIG_PM and CONFIG_PM_SLEEP cannot be disabled,
> > > > I am not getting any warning related to these functions.
> > > >
> > > > Do you want me to remove __maybe_unused ?
> > > >
> > >
> > > The warning was there as expected. You should to use it along with
> > > DEFINE_SIMPLE_DEV_PM_OPS(). Let me know if you still see the same
> > > warning.
> > >
> >
> > But the warnings are related to the following macros
> >
> > #define PTR_IF(cond, ptr) ((cond) ? (ptr) : NULL)
> >
> > #define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))
> > #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
> >
> > So if we can disable CONFIG_PM and CONFIG_PM_SLEEP options
> > the relevant function with the above macro will be set to NULL.
> > in this case, the compiler will discard the function in SET_SYSTEM_SLEEP_PM_OPS
> >
>
> There are differences if you use SET_SYSTEM_SLEEP_PM_OPS vs the new
> macro. It's noted in this commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1a3c7bb088266fa2db017be299f91f1c1894c857
>
> Please try it out.

Thank you very much for this input

I will drop the __maybe_unused from my patch series.
and switch to use DEFINE_SIMPLE_DEV_PM_OPS macro
for suspend resume functions

>
> Thanks,
> Thinh

Thanks
-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel