[v5] gpio: Use modern PM macros

[PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Jisheng Zhang 1 week, 1 day ago

Use the modern PM macros for the suspend and resume functions to be
automatically dropped by the compiler when CONFIG_PM or
CONFIG_PM_SLEEP are disabled, without having to use #ifdef guards.

This has the advantage of always compiling these functions in,
independently of any Kconfig option. Thanks to that, bugs and other
regressions are subsequently easier to catch.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Acked-by: Doug Berger <opendmb@gmail.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-brcmstb.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index f40c9472588b..af9287ff5dc4 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -533,7 +533,6 @@ static void brcmstb_gpio_shutdown(struct platform_device *pdev)
 	brcmstb_gpio_quiesce(&pdev->dev, false);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static void brcmstb_gpio_bank_restore(struct brcmstb_gpio_priv *priv,
 				      struct brcmstb_gpio_bank *bank)
 {
@@ -572,14 +571,9 @@ static int brcmstb_gpio_resume(struct device *dev)
 	return 0;
 }
 
-#else
-#define brcmstb_gpio_suspend	NULL
-#define brcmstb_gpio_resume	NULL
-#endif /* CONFIG_PM_SLEEP */
-
 static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
-	.suspend_noirq	= brcmstb_gpio_suspend,
-	.resume_noirq = brcmstb_gpio_resume,
+	.suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
+	.resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
 };
 
 static int brcmstb_gpio_probe(struct platform_device *pdev)
@@ -755,7 +749,7 @@ static struct platform_driver brcmstb_gpio_driver = {
 	.driver = {
 		.name = "brcmstb-gpio",
 		.of_match_table = brcmstb_gpio_of_match,
-		.pm = &brcmstb_gpio_pm_ops,
+		.pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),
 	},
 	.probe = brcmstb_gpio_probe,
 	.remove = brcmstb_gpio_remove,
-- 
2.51.0

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Jonas Gorski 1 week ago

Hi,

On Mon, Nov 24, 2025 at 1:39 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> Use the modern PM macros for the suspend and resume functions to be
> automatically dropped by the compiler when CONFIG_PM or
> CONFIG_PM_SLEEP are disabled, without having to use #ifdef guards.
>
> This has the advantage of always compiling these functions in,
> independently of any Kconfig option. Thanks to that, bugs and other
> regressions are subsequently easier to catch.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Acked-by: Doug Berger <opendmb@gmail.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpio-brcmstb.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index f40c9472588b..af9287ff5dc4 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> @@ -533,7 +533,6 @@ static void brcmstb_gpio_shutdown(struct platform_device *pdev)
>         brcmstb_gpio_quiesce(&pdev->dev, false);
>  }
>
> -#ifdef CONFIG_PM_SLEEP
>  static void brcmstb_gpio_bank_restore(struct brcmstb_gpio_priv *priv,
>                                       struct brcmstb_gpio_bank *bank)
>  {
> @@ -572,14 +571,9 @@ static int brcmstb_gpio_resume(struct device *dev)
>         return 0;
>  }
>
> -#else
> -#define brcmstb_gpio_suspend   NULL
> -#define brcmstb_gpio_resume    NULL
> -#endif /* CONFIG_PM_SLEEP */
> -
>  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> -       .suspend_noirq  = brcmstb_gpio_suspend,
> -       .resume_noirq = brcmstb_gpio_resume,
> +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
> +       .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
>  };
>
>  static int brcmstb_gpio_probe(struct platform_device *pdev)
> @@ -755,7 +749,7 @@ static struct platform_driver brcmstb_gpio_driver = {
>         .driver = {
>                 .name = "brcmstb-gpio",
>                 .of_match_table = brcmstb_gpio_of_match,
> -               .pm = &brcmstb_gpio_pm_ops,
> +               .pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),

won't this cause a "brcmstb_gpio_pm_ops is unused" compile warning for
!CONFIG_PM_SLEEP?

You probably need to add a __maybe_unused to brcmstb_gpio_pm_ops
(which incidentally DEFINE_NOIRQ_DEV_PM_OPS() also doesn't set, but
all other *_DEV_PM_OPS() macros do).

Best regards,
Jonas

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Andy Shevchenko 1 week ago

On Mon, Nov 24, 2025 at 2:40 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> On Mon, Nov 24, 2025 at 1:39 AM Jisheng Zhang <jszhang@kernel.org> wrote:

...

> >  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> > -       .suspend_noirq  = brcmstb_gpio_suspend,
> > -       .resume_noirq = brcmstb_gpio_resume,
> > +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
> > +       .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
> >  };

...

> > -               .pm = &brcmstb_gpio_pm_ops,
> > +               .pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),
>
> won't this cause a "brcmstb_gpio_pm_ops is unused" compile warning for
> !CONFIG_PM_SLEEP?
>
> You probably need to add a __maybe_unused to brcmstb_gpio_pm_ops
> (which incidentally DEFINE_NOIRQ_DEV_PM_OPS() also doesn't set, but
> all other *_DEV_PM_OPS() macros do).

Shouldn't it be covered by the same trick as pm_sleep_ptr() does for functions?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Jonas Gorski 1 week ago

On Mon, Nov 24, 2025 at 2:52 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Nov 24, 2025 at 2:40 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > On Mon, Nov 24, 2025 at 1:39 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> ...
>
> > >  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> > > -       .suspend_noirq  = brcmstb_gpio_suspend,
> > > -       .resume_noirq = brcmstb_gpio_resume,
> > > +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
> > > +       .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
> > >  };
>
> ...
>
> > > -               .pm = &brcmstb_gpio_pm_ops,
> > > +               .pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),
> >
> > won't this cause a "brcmstb_gpio_pm_ops is unused" compile warning for
> > !CONFIG_PM_SLEEP?
> >
> > You probably need to add a __maybe_unused to brcmstb_gpio_pm_ops
> > (which incidentally DEFINE_NOIRQ_DEV_PM_OPS() also doesn't set, but
> > all other *_DEV_PM_OPS() macros do).
>
> Shouldn't it be covered by the same trick as pm_sleep_ptr() does for functions?

pm_sleep_ptr() becomes NULL for !CONFIG_PM_SLEEP, so there is no
reference then anymore to brcmstb_gpio_pm_ops. You would need a
wrapper for brcmstb_gpio_pm_ops itself to conditionally define it to
avoid the warning, or add __maybe_unused to it to silence it.

Note how SIMPLE_DEV_PM_OPS() and UNIVERSAL_DEV_PM_OPS() tag the struct
with it (for that reason I assume).

Best regards,
Jonas

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Andy Shevchenko 1 week ago

On Mon, Nov 24, 2025 at 03:20:00PM +0100, Jonas Gorski wrote:
> On Mon, Nov 24, 2025 at 2:52 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Nov 24, 2025 at 2:40 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > > On Mon, Nov 24, 2025 at 1:39 AM Jisheng Zhang <jszhang@kernel.org> wrote:

...

> > > >  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> > > > -       .suspend_noirq  = brcmstb_gpio_suspend,
> > > > -       .resume_noirq = brcmstb_gpio_resume,
> > > > +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
> > > > +       .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
> > > >  };

...

> > > > -               .pm = &brcmstb_gpio_pm_ops,
> > > > +               .pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),
> > >
> > > won't this cause a "brcmstb_gpio_pm_ops is unused" compile warning for
> > > !CONFIG_PM_SLEEP?
> > >
> > > You probably need to add a __maybe_unused to brcmstb_gpio_pm_ops
> > > (which incidentally DEFINE_NOIRQ_DEV_PM_OPS() also doesn't set, but
> > > all other *_DEV_PM_OPS() macros do).

Do they? I mean the modern ones and not that are deprecated.

> > Shouldn't it be covered by the same trick as pm_sleep_ptr() does for functions?
> 
> pm_sleep_ptr() becomes NULL for !CONFIG_PM_SLEEP, so there is no
> reference then anymore to brcmstb_gpio_pm_ops. You would need a
> wrapper for brcmstb_gpio_pm_ops itself to conditionally define it to
> avoid the warning, or add __maybe_unused to it to silence it.

PTR_IF() magic is exactly to make sure compiler will have a visibility while
dropping a dead code. Did I miss anything?

> Note how SIMPLE_DEV_PM_OPS() and UNIVERSAL_DEV_PM_OPS() tag the struct
> with it (for that reason I assume).

Both are deprecated. Not a good orienteer.
None of the new approach uses __maybe_unused. (See DEFINE_*() macros in pm.h.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Jonas Gorski 1 week ago

On Mon, Nov 24, 2025 at 3:49 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Nov 24, 2025 at 03:20:00PM +0100, Jonas Gorski wrote:
> > On Mon, Nov 24, 2025 at 2:52 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Mon, Nov 24, 2025 at 2:40 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > > > On Mon, Nov 24, 2025 at 1:39 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> ...
>
> > > > >  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> > > > > -       .suspend_noirq  = brcmstb_gpio_suspend,
> > > > > -       .resume_noirq = brcmstb_gpio_resume,
> > > > > +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
> > > > > +       .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
> > > > >  };
>
> ...
>
> > > > > -               .pm = &brcmstb_gpio_pm_ops,
> > > > > +               .pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),
> > > >
> > > > won't this cause a "brcmstb_gpio_pm_ops is unused" compile warning for
> > > > !CONFIG_PM_SLEEP?
> > > >
> > > > You probably need to add a __maybe_unused to brcmstb_gpio_pm_ops
> > > > (which incidentally DEFINE_NOIRQ_DEV_PM_OPS() also doesn't set, but
> > > > all other *_DEV_PM_OPS() macros do).
>
> Do they? I mean the modern ones and not that are deprecated.
>
> > > Shouldn't it be covered by the same trick as pm_sleep_ptr() does for functions?
> >
> > pm_sleep_ptr() becomes NULL for !CONFIG_PM_SLEEP, so there is no
> > reference then anymore to brcmstb_gpio_pm_ops. You would need a
> > wrapper for brcmstb_gpio_pm_ops itself to conditionally define it to
> > avoid the warning, or add __maybe_unused to it to silence it.
>
> PTR_IF() magic is exactly to make sure compiler will have a visibility while
> dropping a dead code. Did I miss anything?

No, I just was working with old assumptions, so my bad. I faintly
remember that they used to work that way, but maybe I also
misremember. TIL. So disregard my comment.

> > Note how SIMPLE_DEV_PM_OPS() and UNIVERSAL_DEV_PM_OPS() tag the struct
> > with it (for that reason I assume).
>
> Both are deprecated. Not a good orienteer.
> None of the new approach uses __maybe_unused. (See DEFINE_*() macros in pm.h.)

Maybe that some were using it was confusing me into thinking it is required.

Best regards,
Jonas

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Andy Shevchenko 1 week ago

On Mon, Nov 24, 2025 at 04:05:29PM +0100, Jonas Gorski wrote:
> On Mon, Nov 24, 2025 at 3:49 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Nov 24, 2025 at 03:20:00PM +0100, Jonas Gorski wrote:
> > > On Mon, Nov 24, 2025 at 2:52 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Mon, Nov 24, 2025 at 2:40 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
> > > > > On Mon, Nov 24, 2025 at 1:39 AM Jisheng Zhang <jszhang@kernel.org> wrote:

...

> > > > > >  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> > > > > > -       .suspend_noirq  = brcmstb_gpio_suspend,
> > > > > > -       .resume_noirq = brcmstb_gpio_resume,
> > > > > > +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
> > > > > > +       .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
> > > > > >  };

...

> > > > > > -               .pm = &brcmstb_gpio_pm_ops,
> > > > > > +               .pm = pm_sleep_ptr(&brcmstb_gpio_pm_ops),
> > > > >
> > > > > won't this cause a "brcmstb_gpio_pm_ops is unused" compile warning for
> > > > > !CONFIG_PM_SLEEP?
> > > > >
> > > > > You probably need to add a __maybe_unused to brcmstb_gpio_pm_ops
> > > > > (which incidentally DEFINE_NOIRQ_DEV_PM_OPS() also doesn't set, but
> > > > > all other *_DEV_PM_OPS() macros do).
> >
> > Do they? I mean the modern ones and not that are deprecated.
> >
> > > > Shouldn't it be covered by the same trick as pm_sleep_ptr() does for functions?
> > >
> > > pm_sleep_ptr() becomes NULL for !CONFIG_PM_SLEEP, so there is no
> > > reference then anymore to brcmstb_gpio_pm_ops. You would need a
> > > wrapper for brcmstb_gpio_pm_ops itself to conditionally define it to
> > > avoid the warning, or add __maybe_unused to it to silence it.
> >
> > PTR_IF() magic is exactly to make sure compiler will have a visibility while
> > dropping a dead code. Did I miss anything?
> 
> No, I just was working with old assumptions, so my bad. I faintly
> remember that they used to work that way, but maybe I also
> misremember. TIL. So disregard my comment.

NP. I'm glad everything is clear now about them.

> > > Note how SIMPLE_DEV_PM_OPS() and UNIVERSAL_DEV_PM_OPS() tag the struct
> > > with it (for that reason I assume).
> >
> > Both are deprecated. Not a good orienteer.
> > None of the new approach uses __maybe_unused. (See DEFINE_*() macros in pm.h.)
> 
> Maybe that some were using it was confusing me into thinking it is required.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/14] gpio: brcmstb: Use modern PM macros

Posted by Andy Shevchenko 1 week ago

On Mon, Nov 24, 2025 at 08:20:53AM +0800, Jisheng Zhang wrote:
> Use the modern PM macros for the suspend and resume functions to be
> automatically dropped by the compiler when CONFIG_PM or
> CONFIG_PM_SLEEP are disabled, without having to use #ifdef guards.
> 
> This has the advantage of always compiling these functions in,
> independently of any Kconfig option. Thanks to that, bugs and other
> regressions are subsequently easier to catch.

I think converting to the respective macros for NO_IRQ case makes sense in
the same patch. But I leave this to Bart, I'm not going to give tag here
due to the above.

-- 
With Best Regards,
Andy Shevchenko