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@crapouillou.net>
Cc: patches@opensource.cirrus.com
---
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)
{
int ret;
@@ -742,9 +741,7 @@ static int arizona_runtime_suspend(struct device *dev)
return 0;
}
-#endif
-#ifdef CONFIG_PM_SLEEP
static int arizona_suspend(struct device *dev)
{
struct arizona *arizona = dev_get_drvdata(dev);
@@ -784,17 +781,21 @@ static int arizona_resume(struct device *dev)
return 0;
}
-#endif
+#ifndef CONFIG_PM
+static __maybe_unused
+#endif
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
#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),
},
--
2.35.1On 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@crapouillou.net>
> Cc: patches@opensource.cirrus.com
> ---
> 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?
> {
> 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.
> 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?
>
> #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),
> },
Hi Richard,
Le lun., août 8 2022 at 10:53:54 +0100, Richard Fitzgerald
<rf@opensource.cirrus.com> 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@crapouillou.net>
>> Cc: patches@opensource.cirrus.com
>> ---
>> 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),
>> },
On 08/08/2022 11:06, Paul Cercueil wrote:
> Hi Richard,
>
> Le lun., août 8 2022 at 10:53:54 +0100, Richard Fitzgerald
> <rf@opensource.cirrus.com> 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@crapouillou.net>
>>> Cc: patches@opensource.cirrus.com
>>> ---
>>> 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
Why conditionally export it?
> !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.
The functions will be dropped if they are not referenced. That doesn't
answer why the struct must not be exported.
What is the aim of omitting the struct export?
>
> 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),
>>> },
>
>
Le lun., août 8 2022 at 11:43:31 +0100, Richard Fitzgerald
<rf@opensource.cirrus.com> a écrit :
> On 08/08/2022 11:06, Paul Cercueil wrote:
>> Hi Richard,
>>
>> Le lun., août 8 2022 at 10:53:54 +0100, Richard Fitzgerald
>> <rf@opensource.cirrus.com> 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@crapouillou.net>
>>>> Cc: patches@opensource.cirrus.com
>>>> ---
>>>> 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
>
> Why conditionally export it?
>
>> !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.
>
> The functions will be dropped if they are not referenced. That doesn't
> answer why the struct must not be exported.
>
> What is the aim of omitting the struct export?
The functions are always referenced by the dev_pm_ops structure.
Omitting the struct export means that the struct can now be a "static
__maybe_unused" symbol in the !CONFIG_PM case, and everything related
to PM will be automatically removed by the compiler.
Otherwise, the symbol is exported as usual. The symbol being
conditionally exported is not a problem - the struct is always
referenced (as it should be) using the pm_sleep_ptr() or pm_ptr()
macros.
This is basically what EXPORT_SIMPLE_DEV_PM_OPS() does by the way.
Cheers,
-Paul
>>
>> 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),
>>>> },
>>
>>
On 08/08/2022 12:01, Paul Cercueil wrote:
>
>
> Le lun., août 8 2022 at 11:43:31 +0100, Richard Fitzgerald
> <rf@opensource.cirrus.com> a écrit :
>> On 08/08/2022 11:06, Paul Cercueil wrote:
>>> Hi Richard,
>>>
>>> Le lun., août 8 2022 at 10:53:54 +0100, Richard Fitzgerald
>>> <rf@opensource.cirrus.com> 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@crapouillou.net>
>>>>> Cc: patches@opensource.cirrus.com
>>>>> ---
>>>>> 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
>>
>> Why conditionally export it?
>>
>>> !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.
>>
>> The functions will be dropped if they are not referenced. That doesn't
>> answer why the struct must not be exported.
>>
>> What is the aim of omitting the struct export?
>
> The functions are always referenced by the dev_pm_ops structure.
> Omitting the struct export means that the struct can now be a "static
> __maybe_unused" symbol in the !CONFIG_PM case, and everything related to
> PM will be automatically removed by the compiler.
>
> Otherwise, the symbol is exported as usual. The symbol being
> conditionally exported is not a problem - the struct is always
> referenced (as it should be) using the pm_sleep_ptr() or pm_ptr() macros.
>
> This is basically what EXPORT_SIMPLE_DEV_PM_OPS() does by the way.
>
> Cheers,
> -Paul
>
Ok,
Well ultimately it's up to Lee as the maintainer of the MFD subsystem.
But the open-coded #ifdef around "static __maybe_unused" is ugly, so if
this is going to be a common pattern a new macro would be nice.
>>>
>>> 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),
>>>>> },
>>>
>>>
>
>
On Mon, 08 Aug 2022, Richard Fitzgerald wrote:
> On 08/08/2022 12:01, Paul Cercueil wrote:
> >
> >
> > Le lun., août 8 2022 at 11:43:31 +0100, Richard Fitzgerald
> > <rf@opensource.cirrus.com> a écrit :
> > > On 08/08/2022 11:06, Paul Cercueil wrote:
> > > > Hi Richard,
> > > >
> > > > Le lun., août 8 2022 at 10:53:54 +0100, Richard Fitzgerald
> > > > <rf@opensource.cirrus.com> 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@crapouillou.net>
> > > > > > Cc: patches@opensource.cirrus.com
> > > > > > ---
> > > > > > 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
> > >
> > > Why conditionally export it?
> > >
> > > > !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.
> > >
> > > The functions will be dropped if they are not referenced. That doesn't
> > > answer why the struct must not be exported.
> > >
> > > What is the aim of omitting the struct export?
> >
> > The functions are always referenced by the dev_pm_ops structure.
> > Omitting the struct export means that the struct can now be a "static
> > __maybe_unused" symbol in the !CONFIG_PM case, and everything related to
> > PM will be automatically removed by the compiler.
> >
> > Otherwise, the symbol is exported as usual. The symbol being
> > conditionally exported is not a problem - the struct is always
> > referenced (as it should be) using the pm_sleep_ptr() or pm_ptr()
> > macros.
> >
> > This is basically what EXPORT_SIMPLE_DEV_PM_OPS() does by the way.
> >
> > Cheers,
> > -Paul
> >
> Ok,
> Well ultimately it's up to Lee as the maintainer of the MFD subsystem.
>
> But the open-coded #ifdef around "static __maybe_unused" is ugly, so if
> this is going to be a common pattern a new macro would be nice.
I like to avoid #ifery in C files wherever possible.
--
DEPRECATED: Please use lee@kernel.org
Hi Paul,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.19]
[cannot apply to lee-mfd/for-mfd-next linus/master next-20220805]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Paul-Cercueil/mfd-Remove-ifdef-guards-for-PM-functions/20220807-225947
base: 3d7cb6b04c3f3115719235cc6866b10326de34cd
config: hexagon-randconfig-r013-20220807 (https://download.01.org/0day-ci/archive/20220808/202208080141.UIoxRyzL-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/03342844cafff973ffa39ce471f64a76c4d87b06
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Paul-Cercueil/mfd-Remove-ifdef-guards-for-PM-functions/20220807-225947
git checkout 03342844cafff973ffa39ce471f64a76c4d87b06
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/mfd/arizona-core.c:788:25: error: static declaration of 'arizona_pm_ops' follows non-static declaration
const struct dev_pm_ops arizona_pm_ops = {
^
drivers/mfd/arizona.h:29:32: note: previous declaration is here
extern const struct dev_pm_ops arizona_pm_ops;
^
1 error generated.
vim +/arizona_pm_ops +788 drivers/mfd/arizona-core.c
dc781d0e10fca2 Mark Brown 2013-01-27 784
03342844cafff9 Paul Cercueil 2022-08-07 785 #ifndef CONFIG_PM
03342844cafff9 Paul Cercueil 2022-08-07 786 static __maybe_unused
03342844cafff9 Paul Cercueil 2022-08-07 787 #endif
3cc72986947501 Mark Brown 2012-06-19 @788 const struct dev_pm_ops arizona_pm_ops = {
03342844cafff9 Paul Cercueil 2022-08-07 789 RUNTIME_PM_OPS(arizona_runtime_suspend,
3cc72986947501 Mark Brown 2012-06-19 790 arizona_runtime_resume,
3cc72986947501 Mark Brown 2012-06-19 791 NULL)
03342844cafff9 Paul Cercueil 2022-08-07 792 SYSTEM_SLEEP_PM_OPS(arizona_suspend, arizona_resume)
03342844cafff9 Paul Cercueil 2022-08-07 793 NOIRQ_SYSTEM_SLEEP_PM_OPS(arizona_suspend_noirq,
3612b27cfb4a07 Charles Keepax 2016-08-30 794 arizona_resume_noirq)
3cc72986947501 Mark Brown 2012-06-19 795 };
03342844cafff9 Paul Cercueil 2022-08-07 796 #ifdef CONFIG_PM
3cc72986947501 Mark Brown 2012-06-19 797 EXPORT_SYMBOL_GPL(arizona_pm_ops);
03342844cafff9 Paul Cercueil 2022-08-07 798 #endif
3cc72986947501 Mark Brown 2012-06-19 799
--
0-DAY CI Kernel Test Service
https://01.org/lkp
© 2016 - 2026 Red Hat, Inc.