[RFC PATCH 0/3] Introduce pm_ptr() / pm_sleep_ptr()

[RFC PATCH 0/3] Introduce pm_ptr() / pm_sleep_ptr()

[Paul Cercueil]

Hi,

I've seen many times things like:

#ifdef CONFIG_PM_SLEEP
static SIMPLE_DEV_PM_OPS(foo_pm_ops, foo_suspend, foo_resume);
#define FOO_PM_OPS (&foo_pm_ops)
#else
#define FOO_PM_OPS NULL
#endif
static struct platform_driver foo_driver = {
		.driver.pm = FOO_PM_OPS,
};

And always wondered why there was no of-match-ptr-like macro to make
things cleaner.

So this RFC adds two macros, pm_ptr() and pm_sleep_ptr(), which resolve
to their argument when CONFIG_PM or CONFIG_PM_SLEEP (respectively) are
enabled, or NULL otherwise.

Patch 3/3 is an example of what it would look like when used in a
driver.

Comments welcome.

Cheers,
-Paul


Paul Cercueil (3):
  PM: introduce pm_ptr() and pm_sleep_ptr()
  PM: Make *_DEV_PM_OPS macros use __maybe_unused
  mmc: jz4740: Use pm_sleep_ptr() macro

 drivers/mmc/host/jz4740_mmc.c | 12 +++---------
 include/linux/pm.h            | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 11 deletions(-)

-- 
2.25.0

[RFC PATCH 1/3] PM: introduce pm_ptr() and pm_sleep_ptr()

[Paul Cercueil]

These macros are analogous to the infamous of_match_ptr(). If CONFIG_PM
or CONFIG_PM_SLEEP are enabled (respectively), these macros will resolve
to their argument, otherwise to NULL.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index e057d1fa2469..1e183d78a1ae 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -374,6 +374,18 @@ const struct dev_pm_ops name = { \
 	SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
 }
 
+#ifdef CONFIG_PM
+#define pm_ptr(_ptr) (_ptr)
+#else
+#define pm_ptr(_ptr) NULL
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+#define pm_sleep_ptr(_ptr) (_ptr)
+#else
+#define pm_sleep_ptr(_ptr) NULL
+#endif
+
 /*
  * PM_EVENT_ messages
  *
-- 
2.25.0

[RFC PATCH 2/3] PM: Make *_DEV_PM_OPS macros use __maybe_unused

[Paul Cercueil]

This way, when the dev_pm_ops instance is not referenced anywhere, it
will simply be dropped by the compiler without a warning.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 1e183d78a1ae..892e4fd198c0 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -351,7 +351,7 @@ struct dev_pm_ops {
  * to RAM and hibernation.
  */
 #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
-const struct dev_pm_ops name = { \
+const struct dev_pm_ops __maybe_unused name = { \
 	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 }
 
@@ -369,7 +369,7 @@ const struct dev_pm_ops name = { \
  * .runtime_resume(), respectively (and analogously for hibernation).
  */
 #define UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
-const struct dev_pm_ops name = { \
+const struct dev_pm_ops __maybe_unused name = { \
 	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 	SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
 }
-- 
2.25.0

[RFC PATCH 3/3] mmc: jz4740: Use pm_sleep_ptr() macro

[Paul Cercueil]

Use the newly introduced pm_sleep_ptr() macro to simplify the code.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mmc/host/jz4740_mmc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index fbae87d1f017..09554f9831de 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -1099,24 +1099,18 @@ static int jz4740_mmc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-
-static int jz4740_mmc_suspend(struct device *dev)
+static int __maybe_unused jz4740_mmc_suspend(struct device *dev)
 {
 	return pinctrl_pm_select_sleep_state(dev);
 }
 
-static int jz4740_mmc_resume(struct device *dev)
+static int __maybe_unused jz4740_mmc_resume(struct device *dev)
 {
 	return pinctrl_select_default_state(dev);
 }
 
 static SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
 	jz4740_mmc_resume);
-#define JZ4740_MMC_PM_OPS (&jz4740_mmc_pm_ops)
-#else
-#define JZ4740_MMC_PM_OPS NULL
-#endif
 
 static struct platform_driver jz4740_mmc_driver = {
 	.probe = jz4740_mmc_probe,
@@ -1124,7 +1118,7 @@ static struct platform_driver jz4740_mmc_driver = {
 	.driver = {
 		.name = "jz4740-mmc",
 		.of_match_table = of_match_ptr(jz4740_mmc_of_match),
-		.pm = JZ4740_MMC_PM_OPS,
+		.pm = pm_sleep_ptr(&jz4740_mmc_pm_ops),
 	},
 };
 
-- 
2.25.0

Re: [RFC PATCH 3/3] mmc: jz4740: Use pm_sleep_ptr() macro

[Ulf Hansson]

On Tue, 11 Feb 2020 at 17:03, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Use the newly introduced pm_sleep_ptr() macro to simplify the code.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/mmc/host/jz4740_mmc.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index fbae87d1f017..09554f9831de 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -1099,24 +1099,18 @@ static int jz4740_mmc_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM_SLEEP
> -
> -static int jz4740_mmc_suspend(struct device *dev)
> +static int __maybe_unused jz4740_mmc_suspend(struct device *dev)
>  {
>         return pinctrl_pm_select_sleep_state(dev);
>  }
>
> -static int jz4740_mmc_resume(struct device *dev)
> +static int __maybe_unused jz4740_mmc_resume(struct device *dev)
>  {
>         return pinctrl_select_default_state(dev);
>  }
>
>  static SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
>         jz4740_mmc_resume);
> -#define JZ4740_MMC_PM_OPS (&jz4740_mmc_pm_ops)
> -#else
> -#define JZ4740_MMC_PM_OPS NULL
> -#endif

All of the above code can be simplified in this way, without having to
convert into using the new pm_sleep_ptr() macro, below.

The only "penalty" would be that, the struct dev_pm_ops
(jz4740_mmc_pm_ops) would then be referenced even when CONFIG_PM* is
unset, thus the compiler would be able to throw it away.

Just wanted to point this out.

>
>  static struct platform_driver jz4740_mmc_driver = {
>         .probe = jz4740_mmc_probe,
> @@ -1124,7 +1118,7 @@ static struct platform_driver jz4740_mmc_driver = {
>         .driver = {
>                 .name = "jz4740-mmc",
>                 .of_match_table = of_match_ptr(jz4740_mmc_of_match),
> -               .pm = JZ4740_MMC_PM_OPS,
> +               .pm = pm_sleep_ptr(&jz4740_mmc_pm_ops),

If the driver would have runtime suspend/resume callbacks, then it
would need the use the pm_ptr() macro instead, I guess.

>         },
>  };
>
> --
> 2.25.0
>

My overall feeling is that this series improves the code/behaviour,
but I am also a bit worried about adding yet another pair of macros
for dealing with CONFIG_PM* callbacks as it could add more confusion.

An option could be to introduce only the pm_ptr() macro, then skip the
optimization that pm_sleep_ptr() gives. This could make it easier to
use, as you wouldn't need to decide between two macros. Just a
thought.

I don't know what Rafael's thinks about this, let's see if he has some
other ideas.

Kind regards
Uffe

Re: [RFC PATCH 3/3] mmc: jz4740: Use pm_sleep_ptr() macro

[Ulf Hansson]

On Thu, 20 Feb 2020 at 14:38, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 11 Feb 2020 at 17:03, Paul Cercueil <paul@crapouillou.net> wrote:
> >
> > Use the newly introduced pm_sleep_ptr() macro to simplify the code.
> >
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > ---
> >  drivers/mmc/host/jz4740_mmc.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> > index fbae87d1f017..09554f9831de 100644
> > --- a/drivers/mmc/host/jz4740_mmc.c
> > +++ b/drivers/mmc/host/jz4740_mmc.c
> > @@ -1099,24 +1099,18 @@ static int jz4740_mmc_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > -#ifdef CONFIG_PM_SLEEP
> > -
> > -static int jz4740_mmc_suspend(struct device *dev)
> > +static int __maybe_unused jz4740_mmc_suspend(struct device *dev)
> >  {
> >         return pinctrl_pm_select_sleep_state(dev);
> >  }
> >
> > -static int jz4740_mmc_resume(struct device *dev)
> > +static int __maybe_unused jz4740_mmc_resume(struct device *dev)
> >  {
> >         return pinctrl_select_default_state(dev);
> >  }
> >
> >  static SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
> >         jz4740_mmc_resume);
> > -#define JZ4740_MMC_PM_OPS (&jz4740_mmc_pm_ops)
> > -#else
> > -#define JZ4740_MMC_PM_OPS NULL
> > -#endif
>
> All of the above code can be simplified in this way, without having to
> convert into using the new pm_sleep_ptr() macro, below.
>
> The only "penalty" would be that, the struct dev_pm_ops
> (jz4740_mmc_pm_ops) would then be referenced even when CONFIG_PM* is
> unset, thus the compiler would be able to throw it away.

/s/would/would not

[...]

Kind regards
Uffe

Re: [RFC PATCH 3/3] mmc: jz4740: Use pm_sleep_ptr() macro

[Paul Cercueil]

Hi Ulf,


Le jeu., févr. 20, 2020 at 14:38, Ulf Hansson <ulf.hansson@linaro.org> 
a écrit :
> On Tue, 11 Feb 2020 at 17:03, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  Use the newly introduced pm_sleep_ptr() macro to simplify the code.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/mmc/host/jz4740_mmc.c | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>> 
>>  diff --git a/drivers/mmc/host/jz4740_mmc.c 
>> b/drivers/mmc/host/jz4740_mmc.c
>>  index fbae87d1f017..09554f9831de 100644
>>  --- a/drivers/mmc/host/jz4740_mmc.c
>>  +++ b/drivers/mmc/host/jz4740_mmc.c
>>  @@ -1099,24 +1099,18 @@ static int jz4740_mmc_remove(struct 
>> platform_device *pdev)
>>          return 0;
>>   }
>> 
>>  -#ifdef CONFIG_PM_SLEEP
>>  -
>>  -static int jz4740_mmc_suspend(struct device *dev)
>>  +static int __maybe_unused jz4740_mmc_suspend(struct device *dev)
>>   {
>>          return pinctrl_pm_select_sleep_state(dev);
>>   }
>> 
>>  -static int jz4740_mmc_resume(struct device *dev)
>>  +static int __maybe_unused jz4740_mmc_resume(struct device *dev)
>>   {
>>          return pinctrl_select_default_state(dev);
>>   }
>> 
>>   static SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
>>          jz4740_mmc_resume);
>>  -#define JZ4740_MMC_PM_OPS (&jz4740_mmc_pm_ops)
>>  -#else
>>  -#define JZ4740_MMC_PM_OPS NULL
>>  -#endif
> 
> All of the above code can be simplified in this way, without having to
> convert into using the new pm_sleep_ptr() macro, below.
> 
> The only "penalty" would be that, the struct dev_pm_ops
> (jz4740_mmc_pm_ops) would then be referenced even when CONFIG_PM* is
> unset, thus the compiler would be able to throw it away.
> 
> Just wanted to point this out.

Yes, what I had in mind with these macros is that in general the 
suspend/resume functions should not be conditionally compiled, as they 
might have errors which would only appear with specific configs, and 
instead should be always compiled but thrown away by the compiler if 
unused.

>> 
>>   static struct platform_driver jz4740_mmc_driver = {
>>          .probe = jz4740_mmc_probe,
>>  @@ -1124,7 +1118,7 @@ static struct platform_driver 
>> jz4740_mmc_driver = {
>>          .driver = {
>>                  .name = "jz4740-mmc",
>>                  .of_match_table = of_match_ptr(jz4740_mmc_of_match),
>>  -               .pm = JZ4740_MMC_PM_OPS,
>>  +               .pm = pm_sleep_ptr(&jz4740_mmc_pm_ops),
> 
> If the driver would have runtime suspend/resume callbacks, then it
> would need the use the pm_ptr() macro instead, I guess.
> 
>>          },
>>   };
>> 
>>  --
>>  2.25.0
>> 
> 
> My overall feeling is that this series improves the code/behaviour,
> but I am also a bit worried about adding yet another pair of macros
> for dealing with CONFIG_PM* callbacks as it could add more confusion.
> 
> An option could be to introduce only the pm_ptr() macro, then skip the
> optimization that pm_sleep_ptr() gives. This could make it easier to
> use, as you wouldn't need to decide between two macros. Just a
> thought.

One macro would be better than none.

Cheers,
-Paul

> I don't know what Rafael's thinks about this, let's see if he has some
> other ideas.
> 
> Kind regards
> Uffe