Re: [PATCH 20/28] mfd: arizona: Remove #ifdef guards for PM related functions

From: Paul Cercueil
Date: Mon Aug 08 2022 - 06:06:47 EST


Hi Richard,

Le lun., août 8 2022 at 10:53:54 +0100, Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx> a écrit :
On 07/08/2022 15:52, Paul Cercueil wrote:
Only export the arizona_pm_ops if CONFIG_PM is set, but leave the
suspend/resume functions (and related code) outside #ifdef guards.

If CONFIG_PM is not set, the arizona_pm_ops will be defined as
"static __maybe_unused", and the structure plus all the callbacks will
be automatically dropped by the compiler.

The advantage is then that these functions are now always compiled
independently of any Kconfig option, and thanks to that bugs and
regressions are easier to catch.

Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
Cc: patches@xxxxxxxxxxxxxxxxxxxxx
---
drivers/mfd/arizona-core.c | 21 +++++++++++----------
drivers/mfd/arizona-i2c.c | 2 +-
drivers/mfd/arizona-spi.c | 2 +-
3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index cbf1dd90b70d..c1acc9521f83 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -480,7 +480,6 @@ static int wm5102_clear_write_sequencer(struct arizona *arizona)
return 0;
}
-#ifdef CONFIG_PM
static int arizona_isolate_dcvdd(struct arizona *arizona)

__maybe_unused?

No need. The symbols are always referenced.

{
int ret;
@@ -742,9 +741,7 @@ static int arizona_runtime_suspend(struct device *dev)

__maybe_unused?

 return 0;
}
-#endif
-#ifdef CONFIG_PM_SLEEP
static int arizona_suspend(struct device *dev)

__maybe_unused?

{
struct arizona *arizona = dev_get_drvdata(dev);
@@ -784,17 +781,21 @@ static int arizona_resume(struct device *dev)

__maybe_unused?

 return 0;
}
-#endif
+#ifndef CONFIG_PM
+static __maybe_unused
+#endif

No need to ifdef a __maybe_unused.

Yes, it is needed, because the symbol is conditionally exported. If !CONFIG_PM, we want the compiler to discard the dev_pm_ops and all the callbacks, hence the "static __maybe_unused". That's the same trick used in _EXPORT_DEV_PM_OPS().

(note that this patch is broken as it does not change the struct name, in the !PM case, which causes conflicts with the .h. I'll fix in v2)

const struct dev_pm_ops arizona_pm_ops = {
- SET_RUNTIME_PM_OPS(arizona_runtime_suspend,
- arizona_runtime_resume,
- NULL)
- SET_SYSTEM_SLEEP_PM_OPS(arizona_suspend, arizona_resume)
- SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(arizona_suspend_noirq,
- arizona_resume_noirq)
+ RUNTIME_PM_OPS(arizona_runtime_suspend,
+ arizona_runtime_resume,
+ NULL)
+ SYSTEM_SLEEP_PM_OPS(arizona_suspend, arizona_resume)
+ NOIRQ_SYSTEM_SLEEP_PM_OPS(arizona_suspend_noirq,
+ arizona_resume_noirq)
};
+#ifdef CONFIG_PM
EXPORT_SYMBOL_GPL(arizona_pm_ops);
+#endif

This ifdeffing is ugly. Why must the structure only be exported if
CONFIG_PM is set?

So that all the PM code is garbage-collected by the compiler if !CONFIG_PM.

Ideally I would use something like EXPORT_SIMPLE_DEV_PM_OPS() which would make the patch much cleaner, but it doesn't support noirq callbacks - and that's why I suggested in the cover letter that maybe a new PM macro can be added if this patch is deemed too messy.

Cheers,
-Paul

 #ifdef CONFIG_OF
static int arizona_of_get_core_pdata(struct arizona *arizona)
diff --git a/drivers/mfd/arizona-i2c.c b/drivers/mfd/arizona-i2c.c
index 6d83e6b9a692..8799d9183bee 100644
--- a/drivers/mfd/arizona-i2c.c
+++ b/drivers/mfd/arizona-i2c.c
@@ -119,7 +119,7 @@ static const struct of_device_id arizona_i2c_of_match[] = {
static struct i2c_driver arizona_i2c_driver = {
.driver = {
.name = "arizona",
- .pm = &arizona_pm_ops,
+ .pm = pm_ptr(&arizona_pm_ops),
.of_match_table = of_match_ptr(arizona_i2c_of_match),
},
.probe = arizona_i2c_probe,
diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c
index 941b0267d09d..da05b966d48c 100644
--- a/drivers/mfd/arizona-spi.c
+++ b/drivers/mfd/arizona-spi.c
@@ -282,7 +282,7 @@ static const struct of_device_id arizona_spi_of_match[] = {
static struct spi_driver arizona_spi_driver = {
.driver = {
.name = "arizona",
- .pm = &arizona_pm_ops,
+ .pm = pm_ptr(&arizona_pm_ops),
.of_match_table = of_match_ptr(arizona_spi_of_match),
.acpi_match_table = ACPI_PTR(arizona_acpi_match),
},