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>
---
drivers/gpio/gpio-pxa.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index fa22f3faa163..288c72f9c015 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -66,13 +66,10 @@ struct pxa_gpio_bank {
unsigned long irq_mask;
unsigned long irq_edge_rise;
unsigned long irq_edge_fall;
-
-#ifdef CONFIG_PM
unsigned long saved_gplr;
unsigned long saved_gpdr;
unsigned long saved_grer;
unsigned long saved_gfer;
-#endif
};
struct pxa_gpio_chip {
@@ -746,7 +743,6 @@ static int __init pxa_gpio_dt_init(void)
}
device_initcall(pxa_gpio_dt_init);
-#ifdef CONFIG_PM
static int pxa_gpio_suspend(void)
{
struct pxa_gpio_chip *pchip = pxa_gpio_chip;
@@ -787,14 +783,10 @@ static void pxa_gpio_resume(void)
writel_relaxed(c->saved_gpdr, c->regbase + GPDR_OFFSET);
}
}
-#else
-#define pxa_gpio_suspend NULL
-#define pxa_gpio_resume NULL
-#endif
static struct syscore_ops pxa_gpio_syscore_ops = {
- .suspend = pxa_gpio_suspend,
- .resume = pxa_gpio_resume,
+ .suspend = pm_ptr(pxa_gpio_suspend),
+ .resume = pm_ptr(pxa_gpio_resume),
};
static int __init pxa_gpio_sysinit(void)
--
2.51.0On Tue, Nov 18, 2025 at 2:50 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. ... > unsigned long irq_mask; > unsigned long irq_edge_rise; > unsigned long irq_edge_fall; > - Stray blank line removal. > -#ifdef CONFIG_PM > unsigned long saved_gplr; > unsigned long saved_gpdr; > unsigned long saved_grer; > unsigned long saved_gfer; > -#endif Otherwise, LGTM. -- With Best Regards, Andy Shevchenko
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Tue, Nov 18, 2025 at 2:50 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. ...zip... > >> -#ifdef CONFIG_PM >> unsigned long saved_gplr; >> unsigned long saved_gpdr; >> unsigned long saved_grer; >> unsigned long saved_gfer; >> -#endif Actually this is not equivalent to what was there before. With Jisheng's patch, with CONFIG_PM disabled, he adds 16 bytes to the structure. You might thing today, 16 bytes is nothing. True, but on a 64MB RAM devices, it's something. That might not be a reason to reject the patch, but it's not only a "modernisation patch". Cheers. -- Robert
On Tue, Nov 18, 2025 at 11:03:41PM +0100, Robert Jarzmik wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > > On Tue, Nov 18, 2025 at 2:50 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. > ...zip... > > > > > -#ifdef CONFIG_PM > > > unsigned long saved_gplr; > > > unsigned long saved_gpdr; > > > unsigned long saved_grer; > > > unsigned long saved_gfer; > > > -#endif > > Actually this is not equivalent to what was there before. > > With Jisheng's patch, with CONFIG_PM disabled, he adds 16 bytes to the > structure. You might thing today, 16 bytes is nothing. True, but on a > 64MB RAM devices, it's something. hmm, each controller adds 16bytes, then even on 100 controller platforms 1600bytes. 1600 Bytes/64MB ~= 0.238%. it's trival. And is there such platform? From another side, recently UP support is removed from the core sched, that removing adds more .text and .data overhead, so if the users really care about this kind of 16bytes, it means he(she) can't afford even the 16Bytes overhead, then I bet he(she) the always SMP in core sched, so why not stick with the old kernel? What do you think? > > That might not be a reason to reject the patch, but it's not only a > "modernisation patch". > > Cheers. > > -- > Robert
Jisheng Zhang <jszhang@kernel.org> writes: > On Tue, Nov 18, 2025 at 11:03:41PM +0100, Robert Jarzmik wrote: > > hmm, each controller adds 16bytes, then even on 100 controller > platforms > 1600bytes. 1600 Bytes/64MB ~= 0.238%. it's trival. And is there > such platform? Yes, actually most of them have around 64MB, at least the pxa25x and pxa27x. The pxa3xx might have more (thing 128MB, maybe 256MB). There are very old platforms, we're in 2003/2004 there ... > From another side, recently UP support is removed from the core > sched, > that removing adds more .text and .data overhead, so if the > users really > care about this kind of 16bytes, it means he(she) can't afford > even the > 16Bytes overhead, then I bet he(she) the always SMP in core > sched, so > why not stick with the old kernel? What do you think? I think I would go with Andy's proposal, decouple the changes : - keep your changes in the PM callbaks - remove your change (put back the ifdef) in the data structure Cheers. -- Robert
On Thu, Nov 20, 2025 at 09:48:30PM +0100, Robert Jarzmik wrote: > Jisheng Zhang <jszhang@kernel.org> writes: > > On Tue, Nov 18, 2025 at 11:03:41PM +0100, Robert Jarzmik wrote: > > > > hmm, each controller adds 16bytes, then even on 100 controller platforms > > 1600bytes. 1600 Bytes/64MB ~= 0.238%. it's trival. And is there such > > platform? > Yes, actually most of them have around 64MB, at least the pxa25x and pxa27x. > The pxa3xx might have more (thing 128MB, maybe 256MB). > There are very old platforms, we're in 2003/2004 there ... > > > From another side, recently UP support is removed from the core sched, > > that removing adds more .text and .data overhead, so if the users really > > care about this kind of 16bytes, it means he(she) can't afford even the > > 16Bytes overhead, then I bet he(she) the always SMP in core sched, so > > why not stick with the old kernel? What do you think? > I think I would go with Andy's proposal, decouple the changes : > - keep your changes in the PM callbaks > - remove your change (put back the ifdef) in the data structure It can't be done like this, unfortunately. Either we need to waste a pointer and kmalloc() overheads at runtime, or keep these bytes for !PM cases. Alternatively we can drop this change and simply add a comment explaining the memory requirements and why we don't want to always waste those bytes. Ideally would be good to have some kind of struct_group() macro that is dependent on IS_ENABLED() case. It may help in many cases like this then. -- With Best Regards, Andy Shevchenko
On Wed, Nov 19, 2025 at 12:04 AM Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > On Tue, Nov 18, 2025 at 2:50 AM Jisheng Zhang > > <jszhang@kernel.org> wrote: ... > >> -#ifdef CONFIG_PM > >> unsigned long saved_gplr; > >> unsigned long saved_gpdr; > >> unsigned long saved_grer; > >> unsigned long saved_gfer; > >> -#endif > > Actually this is not equivalent to what was there before. > > With Jisheng's patch, with CONFIG_PM disabled, he adds 16 bytes to > the > structure. You might thing today, 16 bytes is nothing. True, but > on a > 64MB RAM devices, it's something. > > That might not be a reason to reject the patch, but it's not only > a > "modernisation patch". Actually a good point! On the same grounds I semi-nacked the gpio-dwapb change. -- With Best Regards, Andy Shevchenko
On Wed, Nov 19, 2025 at 09:56:12AM +0200, Andy Shevchenko wrote: > On Wed, Nov 19, 2025 at 12:04 AM Robert Jarzmik <robert.jarzmik@free.fr> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > > On Tue, Nov 18, 2025 at 2:50 AM Jisheng Zhang > > > <jszhang@kernel.org> wrote: > > ... > > > >> -#ifdef CONFIG_PM > > >> unsigned long saved_gplr; > > >> unsigned long saved_gpdr; > > >> unsigned long saved_grer; > > >> unsigned long saved_gfer; > > >> -#endif > > > > Actually this is not equivalent to what was there before. > > > > With Jisheng's patch, with CONFIG_PM disabled, he adds 16 bytes to > > the > > structure. You might thing today, 16 bytes is nothing. True, but > > on a > > 64MB RAM devices, it's something. > > > > That might not be a reason to reject the patch, but it's not only > > a > > "modernisation patch". > > Actually a good point! On the same grounds I semi-nacked the gpio-dwapb change. Hi Andy and Robert, I have explained the reason of embedding within the cleanup/modernize patch. Could you plz check? Thanks
On Wed, Nov 19, 2025 at 3:05 PM Jisheng Zhang <jszhang@kernel.org> wrote: > On Wed, Nov 19, 2025 at 09:56:12AM +0200, Andy Shevchenko wrote: ... > I have explained the reason of embedding within the cleanup/modernize > patch. Could you plz check? I already proposed a compromise. I like the new PM macros and less ifdeffery in the code, but let's decouple that one from the refactoring of the data types. You may send them separately with the cover letter describing the reasoning. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.