Messages in this thread | | | Date | Mon, 8 Aug 2022 15:47:03 +0100 | From | Lee Jones <> | Subject | Re: [PATCH 20/28] mfd: arizona: Remove #ifdef guards for PM related functions |
| |
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
| |