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.
The dwapb_context structure is always embedded into struct
dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes
data overhead for !CONFIG_PM_SLEP.
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
drivers/gpio/gpio-dwapb.c | 31 +++++++------------------------
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 43b667b41f5d..d831e6c61566 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -78,7 +78,6 @@ struct dwapb_platform_data {
unsigned int nports;
};
-#ifdef CONFIG_PM_SLEEP
/* Store GPIO context across system-wide suspend/resume transitions */
struct dwapb_context {
u32 data;
@@ -91,7 +90,6 @@ struct dwapb_context {
u32 int_deb;
u32 wake_en;
};
-#endif
struct dwapb_gpio_port_irqchip {
unsigned int nr_irqs;
@@ -102,9 +100,7 @@ struct dwapb_gpio_port {
struct gpio_chip gc;
struct dwapb_gpio_port_irqchip *pirq;
struct dwapb_gpio *gpio;
-#ifdef CONFIG_PM_SLEEP
- struct dwapb_context *ctx;
-#endif
+ struct dwapb_context ctx;
unsigned int idx;
};
#define to_dwapb_gpio(_gc) \
@@ -357,12 +353,11 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int dwapb_irq_set_wake(struct irq_data *d, unsigned int enable)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
- struct dwapb_context *ctx = gpio->ports[0].ctx;
+ struct dwapb_context *ctx = &gpio->ports[0].ctx;
irq_hw_number_t bit = irqd_to_hwirq(d);
if (enable)
@@ -372,9 +367,6 @@ static int dwapb_irq_set_wake(struct irq_data *d, unsigned int enable)
return 0;
}
-#else
-#define dwapb_irq_set_wake NULL
-#endif
static const struct irq_chip dwapb_irq_chip = {
.name = DWAPB_DRIVER_NAME,
@@ -384,7 +376,7 @@ static const struct irq_chip dwapb_irq_chip = {
.irq_set_type = dwapb_irq_set_type,
.irq_enable = dwapb_irq_enable,
.irq_disable = dwapb_irq_disable,
- .irq_set_wake = dwapb_irq_set_wake,
+ .irq_set_wake = pm_sleep_ptr(dwapb_irq_set_wake),
.flags = IRQCHIP_IMMUTABLE,
GPIOCHIP_IRQ_RESOURCE_HELPERS,
};
@@ -509,12 +501,6 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->gpio = gpio;
port->idx = pp->idx;
-#ifdef CONFIG_PM_SLEEP
- port->ctx = devm_kzalloc(gpio->dev, sizeof(*port->ctx), GFP_KERNEL);
- if (!port->ctx)
- return -ENOMEM;
-#endif
-
dat = gpio->regs + GPIO_EXT_PORTA + pp->idx * GPIO_EXT_PORT_STRIDE;
set = gpio->regs + GPIO_SWPORTA_DR + pp->idx * GPIO_SWPORT_DR_STRIDE;
dirout = gpio->regs + GPIO_SWPORTA_DDR + pp->idx * GPIO_SWPORT_DDR_STRIDE;
@@ -746,7 +732,6 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int dwapb_gpio_suspend(struct device *dev)
{
struct dwapb_gpio *gpio = dev_get_drvdata(dev);
@@ -758,7 +743,7 @@ static int dwapb_gpio_suspend(struct device *dev)
for (i = 0; i < gpio->nr_ports; i++) {
unsigned int offset;
unsigned int idx = gpio->ports[i].idx;
- struct dwapb_context *ctx = gpio->ports[i].ctx;
+ struct dwapb_context *ctx = &gpio->ports[i].ctx;
offset = GPIO_SWPORTA_DDR + idx * GPIO_SWPORT_DDR_STRIDE;
ctx->dir = dwapb_read(gpio, offset);
@@ -805,7 +790,7 @@ static int dwapb_gpio_resume(struct device *dev)
for (i = 0; i < gpio->nr_ports; i++) {
unsigned int offset;
unsigned int idx = gpio->ports[i].idx;
- struct dwapb_context *ctx = gpio->ports[i].ctx;
+ struct dwapb_context *ctx = &gpio->ports[i].ctx;
offset = GPIO_SWPORTA_DR + idx * GPIO_SWPORT_DR_STRIDE;
dwapb_write(gpio, offset, ctx->data);
@@ -832,15 +817,13 @@ static int dwapb_gpio_resume(struct device *dev)
return 0;
}
-#endif
-static SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend,
- dwapb_gpio_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend, dwapb_gpio_resume);
static struct platform_driver dwapb_gpio_driver = {
.driver = {
.name = DWAPB_DRIVER_NAME,
- .pm = &dwapb_gpio_pm_ops,
+ .pm = pm_sleep_ptr(&dwapb_gpio_pm_ops),
.of_match_table = dwapb_of_match,
.acpi_match_table = dwapb_acpi_match,
},
--
2.50.1On Wed, Aug 20, 2025 at 6:58 PM 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.
>
> The dwapb_context structure is always embedded into struct
> dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes
> data overhead for !CONFIG_PM_SLEP.
I don't think it's a good approach to add a lot of data for peanuts in
case of PM_SLEEP=n.
Can you just drop that part from the patch and we can discuss it separately?
...
> -#ifdef CONFIG_PM_SLEEP
> static int dwapb_irq_set_wake(struct irq_data *d, unsigned int enable)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
> - struct dwapb_context *ctx = gpio->ports[0].ctx;
> + struct dwapb_context *ctx = &gpio->ports[0].ctx;
> irq_hw_number_t bit = irqd_to_hwirq(d);
>
> if (enable)
> @@ -372,9 +367,6 @@ static int dwapb_irq_set_wake(struct irq_data *d, unsigned int enable)
>
> return 0;
> }
> -#else
> -#define dwapb_irq_set_wake NULL
> -#endif
>
> static const struct irq_chip dwapb_irq_chip = {
> .name = DWAPB_DRIVER_NAME,
> @@ -384,7 +376,7 @@ static const struct irq_chip dwapb_irq_chip = {
> .irq_set_type = dwapb_irq_set_type,
> .irq_enable = dwapb_irq_enable,
> .irq_disable = dwapb_irq_disable,
> - .irq_set_wake = dwapb_irq_set_wake,
> + .irq_set_wake = pm_sleep_ptr(dwapb_irq_set_wake),
> .flags = IRQCHIP_IMMUTABLE,
> GPIOCHIP_IRQ_RESOURCE_HELPERS,
> };
This is an interesting piece. I haven't seen much similar in other
GPIO drivers, I would suggest to split it to a separate patch. Also, I
would always have a callback assigned.
...
> -static SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend,
> - dwapb_gpio_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend, dwapb_gpio_resume);
I think Bart wants the 80 limit to be enforced. Can you just make the
split rather logical?
static DEFINE_SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops,
dwapb_gpio_suspend, dwapb_gpio_resume);
--
With Best Regards,
Andy Shevchenko
On Wed, 20 Aug 2025 19:54:44 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > The dwapb_context structure is always embedded into struct > > dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes > > data overhead for !CONFIG_PM_SLEP. > > I don't think it's a good approach to add a lot of data for peanuts in > case of PM_SLEEP=n. It wastes 36 bytes in case of PM=n. The driver currently allocates the struct with kzalloc and stores a pointer to it in case of PM=y. So this probably has an overhead in the same order of magnitude (pointer + malloc overhead/alignment/fragmentation) in case of PM=y now. -- Michael Büsch https://bues.ch/
On Wed, Aug 20, 2025 at 8:11 PM Michael Büsch <mb@bues.ch> wrote: > > On Wed, 20 Aug 2025 19:54:44 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > The dwapb_context structure is always embedded into struct > > > dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes > > > data overhead for !CONFIG_PM_SLEP. > > > > I don't think it's a good approach to add a lot of data for peanuts in > > case of PM_SLEEP=n. > > It wastes 36 bytes in case of PM=n. ...per port. > The driver currently allocates the struct with kzalloc and stores a pointer to it > in case of PM=y. > So this probably has an overhead in the same order of magnitude (pointer + > malloc overhead/alignment/fragmentation) in case of PM=y now. ...per driver. So, I can't say it's equal, but I leave this to maintainers to decide, -- With Best Regards, Andy Shevchenko
On Wed, Aug 20, 2025 at 10:04:39PM +0300, Andy Shevchenko wrote: > On Wed, Aug 20, 2025 at 8:11 PM Michael Büsch <mb@bues.ch> wrote: > > > > On Wed, 20 Aug 2025 19:54:44 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > The dwapb_context structure is always embedded into struct > > > > dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes > > > > data overhead for !CONFIG_PM_SLEP. > > > > > > I don't think it's a good approach to add a lot of data for peanuts in > > > case of PM_SLEEP=n. > > > > It wastes 36 bytes in case of PM=n. > > ...per port. > > > The driver currently allocates the struct with kzalloc and stores a pointer to it > > in case of PM=y. > > So this probably has an overhead in the same order of magnitude (pointer + > > malloc overhead/alignment/fragmentation) in case of PM=y now. > > ...per driver. Before the patch, struct dwapb_context *ctx is also per port. > > So, I can't say it's equal, but I leave this to maintainers to decide, What in my mind now: this is linux rather than RTOS. After greping the the arm/arm64/riscv dts dir, the max port number is 6, the berlin2q soc families, so this means current we have wasted 216 bytes memory which is trivial compared to the system memory.
On Thu, Aug 21, 2025 at 8:02 PM Jisheng Zhang <jszhang@kernel.org> wrote: > On Wed, Aug 20, 2025 at 10:04:39PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 20, 2025 at 8:11 PM Michael Büsch <mb@bues.ch> wrote: > > > On Wed, 20 Aug 2025 19:54:44 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > The dwapb_context structure is always embedded into struct > > > > > dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes > > > > > data overhead for !CONFIG_PM_SLEP. > > > > > > > > I don't think it's a good approach to add a lot of data for peanuts in > > > > case of PM_SLEEP=n. > > > > > > It wastes 36 bytes in case of PM=n. > > > > ...per port. > > > > > The driver currently allocates the struct with kzalloc and stores a pointer to it > > > in case of PM=y. > > > So this probably has an overhead in the same order of magnitude (pointer + > > > malloc overhead/alignment/fragmentation) in case of PM=y now. > > > > ...per driver. > > Before the patch, struct dwapb_context *ctx is also per port. OK. So the comparison is 4 or 8 bytes per port vs. 36 bytes per port, correct? > > So, I can't say it's equal, but I leave this to maintainers to decide, > > What in my mind now: this is linux rather than RTOS. After greping the > the arm/arm64/riscv dts dir, the max port number is 6, the berlin2q > soc families, so this means current we have wasted 216 bytes memory which > is trivial compared to the system memory. Maybe, but this should be clarified in the commit message. And again, I have no strong objection on this part, but it needs to be described accurately at bare minimum. -- With Best Regards, Andy Shevchenko
On Thu, Aug 21, 2025 at 10:32:01PM +0300, Andy Shevchenko wrote: > On Thu, Aug 21, 2025 at 8:02 PM Jisheng Zhang <jszhang@kernel.org> wrote: > > On Wed, Aug 20, 2025 at 10:04:39PM +0300, Andy Shevchenko wrote: > > > On Wed, Aug 20, 2025 at 8:11 PM Michael Büsch <mb@bues.ch> wrote: > > > > On Wed, 20 Aug 2025 19:54:44 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > The dwapb_context structure is always embedded into struct > > > > > > dwapb_gpio_port to simplify code. Sure this brings a tiny 36 bytes > > > > > > data overhead for !CONFIG_PM_SLEP. > > > > > > > > > > I don't think it's a good approach to add a lot of data for peanuts in > > > > > case of PM_SLEEP=n. > > > > > > > > It wastes 36 bytes in case of PM=n. > > > > > > ...per port. > > > > > > > The driver currently allocates the struct with kzalloc and stores a pointer to it > > > > in case of PM=y. > > > > So this probably has an overhead in the same order of magnitude (pointer + > > > > malloc overhead/alignment/fragmentation) in case of PM=y now. > > > > > > ...per driver. > > > > Before the patch, struct dwapb_context *ctx is also per port. > > OK. So the comparison is 4 or 8 bytes per port vs. 36 bytes per port, correct? yep, I think so > > > > So, I can't say it's equal, but I leave this to maintainers to decide, > > > > What in my mind now: this is linux rather than RTOS. After greping the > > the arm/arm64/riscv dts dir, the max port number is 6, the berlin2q > > soc families, so this means current we have wasted 216 bytes memory which > > is trivial compared to the system memory. > > Maybe, but this should be clarified in the commit message. And again, > I have no strong objection on this part, but it needs to be described > accurately at bare minimum. Good idea, will do in v2. > > -- > With Best Regards, > Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.