Skip to contentSkip to conversation

usb: ehci-exynos: Use devm_clk_get_enabled() helpers

PATCH Superseded Anand Moon 5 participants 23 messages Apr 4, 2024 – Apr 12, 2024 1 message not in the archive

Subsystems (6)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Anand Moon

author
#1 lore raw

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
index f644b131cc0b..f00bfd0b13dc 100644
@@ -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
#2 lore raw
Quoted 29 lines · 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.

---
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 +0 -3
index f644b131cc0b..f00bfd0b13dc 100644
@@ -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

author
#3 lore raw

Hi Greg,

Quoted 44 lines · Greg Kroah-Hartman
Quoted 29 lines · 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.

---
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 +0 -3
index f644b131cc0b..f00bfd0b13dc 100644
@@ -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

#4 lore raw
Quoted 51 lines · Anand Moon

Hi Greg,

Quoted 47 lines · Greg Kroah-Hartman
Quoted 29 lines · 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.

---
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 +0 -3
index f644b131cc0b..f00bfd0b13dc 100644
@@ -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

author
#5 lore raw

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:
- 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 +0 -3
index f644b131cc0b..f00bfd0b13dc 100644
@@ -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

author
#6 lore raw

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

In reply to a message not in the archive

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.

---
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
index f00bfd0b13dc..4cfde1af32be 100644
@@ -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
maintainer Acked-by on v2 2/6
#7 lore raw

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_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
---

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
index f00bfd0b13dc..4cfde1af32be 100644
@@ -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
author
#8 lore raw

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

In reply to a message not in the archive

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: 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
index 20e26a474591..85d04ae0ae40 100644
@@ -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
#9 lore raw

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:
- 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 +0 -3
index 20e26a474591..85d04ae0ae40 100644
@@ -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

author
#10 lore raw

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:
- 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 +1 -3
index 20e26a474591..85d04ae0ae40 100644
@@ -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

author
#11 lore raw

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

In reply to a message not in the archive

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.

---
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
index 85d04ae0ae40..3e647e0b341d 100644
@@ -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
maintainer Acked-by on v2 4/6
#12 lore raw

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_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.
---

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
index 85d04ae0ae40..3e647e0b341d 100644
@@ -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
author
#13 lore raw

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

In reply to a message not in the archive

Use 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
index 5d365ca51771..2d341f0e22a3 100644
@@ -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
#14 lore raw

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

---
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

author
#15 lore raw

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

---
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

#16 lore raw

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

---
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_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.

Quoted 7 lines

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

author
#17 lore raw

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 archive

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.

---
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
index 2d341f0e22a3..90259ad1d0d9 100644
@@ -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
maintainer
#18 lore raw

Re: [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_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.

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(-)
author
#19 lore raw

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_SLEEP are disabled, without having to use #ifdef guards. If CONFIG_PM_SLEEP unused, they will simply be discarded by the compiler.

Quoted 7 lines
Quoted 4 lines

---
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.

(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

maintainer
#20 lore raw

Re: [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_SLEEP are disabled, without having to use #ifdef guards. If CONFIG_PM_SLEEP unused, they will simply be discarded by the compiler.

Quoted 7 lines
Quoted 4 lines

---
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>

Quoted 6 lines

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

author
#21 lore raw

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_SLEEP are disabled, without having to use #ifdef guards. If CONFIG_PM_SLEEP unused, they will simply be discarded by the compiler.

Quoted 7 lines
Quoted 4 lines

---
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>

Quoted 4 lines

But since these CONFIG_PM and CONFIG_PM_SLEEP cannot 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

maintainer
#22 lore raw

Re: [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_SLEEP are disabled, without having to use #ifdef guards. If CONFIG_PM_SLEEP unused, they will simply be discarded by the compiler.

Quoted 7 lines
Quoted 4 lines

---
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>

Quoted 4 lines

But since these CONFIG_PM and CONFIG_PM_SLEEP cannot 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)) #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

author
#23 lore raw

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_SLEEP are disabled, without having to use #ifdef guards. If CONFIG_PM_SLEEP unused, they will simply be discarded by the compiler.

Quoted 7 lines
Quoted 4 lines

---
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>

Quoted 4 lines

But since these CONFIG_PM and CONFIG_PM_SLEEP cannot 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)) #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 6 lines

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

Quoted 3 lines

Thanks,
Thinh

Thanks
-Anand