usb: ehci-exynos: Use devm_clk_get_enabled() helpers
Subsystems (6)
A message that is not in the archive (sent off-list or not yet received): [email protected]
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.
---
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(-)
drivers/usb/host/ehci-exynos.c +5 -14
@@ -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); }
Signature · 1 line
2.44.0
Greg Kroah-Hartman
maintainerQuoted 29 lines · Anand Moon
The
devm_clk_get_enabled()helpers:
- calldevm_clk_get()
- callclk_prepare_enable()and register what is needed in order to callclk_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_probeconsistently, and use its return value to return the error code.Signed-off-by Anand Moon <[email protected]>---
V2: drop theclk_disable_unpreparein suspend/resume functions
fix theusb_put_hcdreturn before thedevm_clk_get_enabled--- drivers/usb/host/ehci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)drivers/usb/host/ehci-exynos.c +0
-3@@ -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" );
drivers/usb/host/ehci-exynos.c static int exynos_ehci_probe(struct platform_device *pdev) · line 168
Quoted 10 lines
drivers/usb/host/ehci-exynos.c +5
-3+ 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,
Quoted 44 lines · Greg Kroah-Hartman
Quoted 29 lines · Anand Moon
The
devm_clk_get_enabled()helpers:
- calldevm_clk_get()
- callclk_prepare_enable()and register what is needed in order to callclk_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_probeconsistently, and use its return value to return the error code.Signed-off-by Anand Moon <[email protected]>---
V2: drop theclk_disable_unpreparein suspend/resume functions
fix theusb_put_hcdreturn before thedevm_clk_get_enabled--- drivers/usb/host/ehci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)drivers/usb/host/ehci-exynos.c +0
-3@@ -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" );drivers/usb/host/ehci-exynos.c static int exynos_ehci_probe(struct platform_device *pdev) · line 168
Quoted 10 lines
drivers/usb/host/ehci-exynos.c +5
-3+ 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?
Quoted 9 lines
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.
Quoted 1 line
greg k-h
Thanks
-Anand
Quoted 51 lines · Anand Moon
Hi Greg,
Quoted 47 lines · Greg Kroah-Hartman
Quoted 29 lines · Anand Moon
The
devm_clk_get_enabled()helpers:
- calldevm_clk_get()
- callclk_prepare_enable()and register what is needed in order to callclk_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_probeconsistently, and use its return value to return the error code.Signed-off-by Anand Moon <[email protected]>---
V2: drop theclk_disable_unpreparein suspend/resume functions
fix theusb_put_hcdreturn before thedevm_clk_get_enabled--- drivers/usb/host/ehci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)drivers/usb/host/ehci-exynos.c +0
-3@@ -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" );drivers/usb/host/ehci-exynos.c static int exynos_ehci_probe(struct platform_device *pdev) · line 168
Quoted 10 lines
drivers/usb/host/ehci-exynos.c +5
-3+ 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.
Quoted 8 lines
Quoted 6 lines
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.
Quoted 57 lines · Krzysztof Kozlowski
Quoted 55 lines · Anand Moon
Hi Greg,
Quoted 51 lines · Greg Kroah-Hartman
Quoted 29 lines · Anand Moon
The
devm_clk_get_enabled()helpers:
- calldevm_clk_get()
- callclk_prepare_enable()and register what is needed in order to callclk_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_probeconsistently, and use its return value to return the error code.Signed-off-by Anand Moon <[email protected]>---
V2: drop theclk_disable_unpreparein suspend/resume functions
fix theusb_put_hcdreturn before thedevm_clk_get_enabled--- drivers/usb/host/ehci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)drivers/usb/host/ehci-exynos.c +0
-3@@ -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" );drivers/usb/host/ehci-exynos.c static int exynos_ehci_probe(struct platform_device *pdev) · line 168
Quoted 10 lines
drivers/usb/host/ehci-exynos.c +5
-3+ 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 :(
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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
PATCH v2 2/6 usb: ehci-exynos: Switch from CONFIG_PM guards to pm_ptr()
In reply to a message not in the archiveUse 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.
---
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(-)
drivers/usb/host/ehci-exynos.c +3 -8
@@ -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), } };
Signature · 1 line
2.44.0
Re: [PATCH v2 2/6] usb: ehci-exynos: Switch from CONFIG_PM guards to pm_ptr()
Quoted 11 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PMare disabled, without having to use #ifdef guards. IfCONFIG_PMunused, they will simply be discarded by the compiler.Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis disabled.
droppedRUNTIME_PM_OPS
---
Quoted 49 lines
drivers/usb/host/ehci-exynos.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)drivers/usb/host/ehci-exynos.c +3
-8@@ -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), } };Signature · 1 line
2.44.0
PATCH v2 3/6 usb: ohci-exynos: Use devm_clk_get_enabled() helpers
In reply to a message not in the archiveThe 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.
---
v2: new changes in this series.
--- drivers/usb/host/ohci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/usb/host/ohci-exynos.c +5 -14
@@ -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); }
Signature · 1 line
2.44.0
Re: [PATCH v2 3/6] usb: ohci-exynos: Use devm_clk_get_enabled() helpers
Quoted 28 lines · Anand Moon
The
devm_clk_get_enabled()helpers:
- calldevm_clk_get()
- callclk_prepare_enable()and register what is needed in order to callclk_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_probeconsistently, and use its return value to return the error code.Signed-off-by Anand Moon <[email protected]>---
v2: new changes in this series.--- drivers/usb/host/ohci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)drivers/usb/host/ohci-exynos.c +0
-3@@ -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" );
drivers/usb/host/ohci-exynos.c static int exynos_ohci_probe(struct platform_device *pdev) · line 144
Quoted 10 lines
drivers/usb/host/ohci-exynos.c +5
-3+ 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
Re: [PATCH v2 3/6] usb: ohci-exynos: Use devm_clk_get_enabled() helpers
Hi Krzysztof,
Quoted 32 lines · Krzysztof Kozlowski
Quoted 30 lines · Anand Moon
The
devm_clk_get_enabled()helpers:
- calldevm_clk_get()
- callclk_prepare_enable()and register what is needed in order to callclk_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_probeconsistently, and use its return value to return the error code.Signed-off-by Anand Moon <[email protected]>---
v2: new changes in this series.--- drivers/usb/host/ohci-exynos.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)drivers/usb/host/ohci-exynos.c +1
-3@@ -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;
Quoted 12 lines
drivers/usb/host/ohci-exynos.c static int exynos_ohci_probe(struct platform_device *pdev) · line 144
Quoted 8 lines
drivers/usb/host/ohci-exynos.c +4
-3+ 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,
Quoted 5 lines
Best regards,
Krzysztof
Thanks
-Anand
PATCH v2 4/6 usb: ohci-exynos: Switch from CONFIG_PM guards to pm_ptr()
In reply to a message not in the archiveUse 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.
---
v2: new changes in this series.
--- drivers/usb/host/ohci-exynos.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/usb/host/ohci-exynos.c +3 -8
@@ -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), } };
Signature · 1 line
2.44.0
Re: [PATCH v2 4/6] usb: ohci-exynos: Switch from CONFIG_PM guards to pm_ptr()
Quoted 9 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PMare disabled, without having to use #ifdef guards. IfCONFIG_PMunused, they will simply be discarded by the compiler.Signed-off-by Anand Moon <[email protected]>---
v2: new changes in this series.
---
Quoted 49 lines
drivers/usb/host/ohci-exynos.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)drivers/usb/host/ohci-exynos.c +3
-8@@ -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), } };Signature · 1 line
2.44.0
PATCH v2 5/6 usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function
In reply to a message not in the archiveUse devm_regulator_bulk_get_enable() instead of open coded 'devm_regulator_get(), regulator_enable(), regulator_disable().
---
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(-)
drivers/usb/dwc3/dwc3-exynos.c +5 -45
@@ -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) {
Signature · 1 line
2.44.0
Re: [PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function
Quoted 2 lines · Anand Moon
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.
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
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
Re: [PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function
Hi Krzysztof,
Quoted 14 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
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.
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
V2: no changes, did not find any regression in pm suspend/resume.No, that's not equivalent code. No explanation in commit msg.
Quoted 8 lines
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;Quoted 4 lines
Best regards,
Krzysztof
if you feel it's incorrect, I will drop this patch..
Thanks
-Anand
Re: [PATCH v2 5/6] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function
Quoted 25 lines · Anand Moon
Hi Krzysztof,
Quoted 21 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
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.
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
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.
Quoted 5 lines
Quoted 1 line
NAK.
devm_regulator_get_enableanddevm_regulator_bulk_get_enable
both remove the dependency from the driver to handle theregulator_enabledandregulator_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.
Quoted 7 lines
Since these functions set
devm_add_actionto 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
PATCH v2 6/6 usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
In reply to a message not in the archiveUse 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.
---
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(-)
drivers/usb/dwc3/dwc3-exynos.c +3 -9
@@ -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), }, };
Signature · 1 line
2.44.0
Thinh Nguyen
maintainerRe: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
Quoted 9 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PM_SLEEPare disabled, without having to use #ifdef guards. IfCONFIG_PM_SLEEPunused, they will simply be discarded by the compiler.Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis
disabled.
The compiler discards the code, yet we still need __maybe_unused?
BR,
Thinh
Quoted 4 lines
--- drivers/usb/dwc3/dwc3-exynos.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
Hi Thinh,
Quoted 7 lines · Thinh Nguyen
Quoted 5 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PM_SLEEPare disabled, without having to use #ifdef guards. IfCONFIG_PM_SLEEPunused, they will simply be discarded by the compiler.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis
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.
(quoted diff) +0 -0
b/arch/arm/configs/exynos_defconfig
index 7ad48fdda1da..43110e42076e 100644
arch/arm/configs/exynos_defconfig +13 -2
@@ -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
Quoted 3 lines
BR,
Thinh
Thanks
-Anand
Thinh Nguyen
maintainerRe: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
Quoted 25 lines · Anand Moon
Hi Thinh,
Quoted 7 lines · Thinh Nguyen
Quoted 5 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PM_SLEEPare disabled, without having to use #ifdef guards. IfCONFIG_PM_SLEEPunused, they will simply be discarded by the compiler.Quoted 7 lines
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis
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_unusedjust to safeguard the function.I have tried to build with config by disabling
CONFIG_PMandCONFIG_PM_SLEEPbut could get the warning compilation by adding flag W=1 -Werror=unused-function.
<snip>
Quoted 6 lines
But since these
CONFIG_PMandCONFIG_PM_SLEEPcannot 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
Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
Hi Thinh,
Quoted 34 lines · Thinh Nguyen
Quoted 25 lines · Anand Moon
Hi Thinh,
Quoted 7 lines · Thinh Nguyen
Quoted 5 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PM_SLEEPare disabled, without having to use #ifdef guards. IfCONFIG_PM_SLEEPunused, they will simply be discarded by the compiler.Quoted 7 lines
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis
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_unusedjust to safeguard the function.I have tried to build with config by disabling
CONFIG_PMandCONFIG_PM_SLEEPbut could get the warning compilation by adding flag W=1 -Werror=unused-function.<snip>
Quoted 4 lines
But since these
CONFIG_PMandCONFIG_PM_SLEEPcannot be disabled, I am not getting any warning related to these functions.
Quoted 7 lines
Quoted 2 lines
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
Quoted 2 lines
BR,
Thinh
Thanks
-Anand
Thinh Nguyen
maintainerRe: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
Quoted 47 lines · Anand Moon
Hi Thinh,
Quoted 34 lines · Thinh Nguyen
Quoted 25 lines · Anand Moon
Hi Thinh,
Quoted 7 lines · Thinh Nguyen
Quoted 5 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PM_SLEEPare disabled, without having to use #ifdef guards. IfCONFIG_PM_SLEEPunused, they will simply be discarded by the compiler.Quoted 7 lines
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis
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_unusedjust to safeguard the function.I have tried to build with config by disabling
CONFIG_PMandCONFIG_PM_SLEEPbut could get the warning compilation by adding flag W=1 -Werror=unused-function.<snip>
Quoted 4 lines
But since these
CONFIG_PMandCONFIG_PM_SLEEPcannot be disabled, I am not getting any warning related to these functions.Quoted 7 lines
Quoted 2 lines
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
Quoted 9 lines
#define
PTR_IF(cond, ptr)((cond) ? (ptr) : NULL)#define
pm_ptr(_ptr)PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))#definepm_sleep_ptr(_ptr)PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))So if we can disable
CONFIG_PMandCONFIG_PM_SLEEPoptions
the relevant function with the above macro will be set to NULL.
in this case, the compiler will discard the function inSET_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:
Please try it out.
Thanks,
Thinh
Re: [PATCH v2 6/6] usb: dwc3: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
Hi Thinh,
Quoted 59 lines · Thinh Nguyen
Quoted 47 lines · Anand Moon
Hi Thinh,
Quoted 34 lines · Thinh Nguyen
Quoted 25 lines · Anand Moon
Hi Thinh,
Quoted 7 lines · Thinh Nguyen
Quoted 5 lines · Anand Moon
Use the new PM macros for the suspend and resume functions to be automatically dropped by the compiler when
CONFIG_PM_SLEEPare disabled, without having to use #ifdef guards. IfCONFIG_PM_SLEEPunused, they will simply be discarded by the compiler.Quoted 7 lines
Quoted 4 lines
Signed-off-by Anand Moon <[email protected]>---
v2: add__maybe_unusedto suspend/resume functions in caseCONFIG_PMis
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_unusedjust to safeguard the function.I have tried to build with config by disabling
CONFIG_PMandCONFIG_PM_SLEEPbut could get the warning compilation by adding flag W=1 -Werror=unused-function.<snip>
Quoted 4 lines
But since these
CONFIG_PMandCONFIG_PM_SLEEPcannot be disabled, I am not getting any warning related to these functions.Quoted 7 lines
Quoted 2 lines
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
Quoted 9 lines
#define
PTR_IF(cond, ptr)((cond) ? (ptr) : NULL)#define
pm_ptr(_ptr)PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))#definepm_sleep_ptr(_ptr)PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))So if we can disable
CONFIG_PMandCONFIG_PM_SLEEPoptions
the relevant function with the above macro will be set to NULL.
in this case, the compiler will discard the function inSET_SYSTEM_SLEEP_PM_OPS
Quoted 6 lines
There are differences if you use
SET_SYSTEM_SLEEP_PM_OPSvs the new macro. It's noted in this commit: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
Quoted 3 lines
Thanks,
Thinh
Thanks
-Anand