[PATCH 0/3] gpio: brcmstb: Bug fixes and wake-up interrupt improvements

[PATCH 0/3] gpio: brcmstb: Bug fixes and wake-up interrupt improvements

[Florian Fainelli]

This patch series corrects the HW interrupt to the bank mapping logic to
be more robust and do not assume any particular order.

The last two patches improve the handling of early wake-up conditions
and makes it more robust so we can use those during "s2idle".

Thank you!

Doug Berger (3):
  gpio: brcmstb: correct hwirq to bank map
  gpio: brcmstb: implement irq_mask_ack
  gpio: brcmstb: allow parent_irq to wake

 drivers/gpio/gpio-brcmstb.c | 121 ++++++++++++++++++++++++------------
 1 file changed, 82 insertions(+), 39 deletions(-)

-- 
2.43.0

[PATCH 1/3] gpio: brcmstb: correct hwirq to bank map

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

The brcmstb_gpio_hwirq_to_bank() function was designed to
accommodate the downward numbering of dynamic GPIOs by
traversing the bank list in the reverse order. However, the
dynamic numbering has changed to increment upward which can
produce an incorrect mapping.

The function is modified to no longer assume an ordering of
the list to accommodate either option.

Fixes: 7b61212f2a07 ("gpiolib: Get rid of ARCH_NR_GPIOS")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/gpio/gpio-brcmstb.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index af9287ff5dc4..2352d099709c 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -301,12 +301,10 @@ static struct brcmstb_gpio_bank *brcmstb_gpio_hwirq_to_bank(
 		struct brcmstb_gpio_priv *priv, irq_hw_number_t hwirq)
 {
 	struct brcmstb_gpio_bank *bank;
-	int i = 0;
 
-	/* banks are in descending order */
-	list_for_each_entry_reverse(bank, &priv->bank_list, node) {
-		i += bank->chip.gc.ngpio;
-		if (hwirq < i)
+	list_for_each_entry(bank, &priv->bank_list, node) {
+		if (hwirq >= bank->chip.gc.offset &&
+		    hwirq < (bank->chip.gc.offset + bank->chip.gc.ngpio))
 			return bank;
 	}
 	return NULL;
-- 
2.43.0

[PATCH 2/3] gpio: brcmstb: implement irq_mask_ack

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

The irq_mask_ack operation is slightly more efficient than doing
irq_mask and irq_ack separately.

More importantly for this driver it bypasses the check of
irqd_irq_masked ensuring a previously masked but still active
interrupt gets remasked if unmasked at the hardware level. This
allows the driver to more efficiently unmask the wake capable
interrupts when quiescing without needing to enable the irqs
individually to clear the irqd_irq_masked state.

Signed-off-by: Doug Berger <opendmb@gmail.com>
[florian: forward ported change after switch to guard()]
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/gpio/gpio-brcmstb.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 2352d099709c..5fb6612c2aa5 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
-// Copyright (C) 2015-2017 Broadcom
+// Copyright (C) 2015-2026 Broadcom
 
 #include <linux/bitops.h>
 #include <linux/gpio/driver.h>
@@ -96,7 +96,7 @@ static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
 }
 
 static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
-		unsigned int hwirq, bool enable)
+		unsigned int hwirq, bool enable, bool ack)
 {
 	struct brcmstb_gpio_priv *priv = bank->parent_priv;
 	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(hwirq, bank));
@@ -110,8 +110,10 @@ static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
 		imask |= mask;
 	else
 		imask &= ~mask;
-	gpio_generic_write_reg(&bank->chip,
-			       priv->reg_base + GIO_MASK(bank->id), imask);
+	if (ack)
+		gpio_generic_write_reg(&bank->chip,
+				       priv->reg_base + GIO_MASK(bank->id),
+				       imask);
 }
 
 static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
@@ -132,7 +134,15 @@ static void brcmstb_gpio_irq_mask(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
 
-	brcmstb_gpio_set_imask(bank, d->hwirq, false);
+	brcmstb_gpio_set_imask(bank, d->hwirq, false, false);
+}
+
+static void brcmstb_gpio_irq_mask_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
+
+	brcmstb_gpio_set_imask(bank, d->hwirq, false, true);
 }
 
 static void brcmstb_gpio_irq_unmask(struct irq_data *d)
@@ -140,7 +150,7 @@ static void brcmstb_gpio_irq_unmask(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
 
-	brcmstb_gpio_set_imask(bank, d->hwirq, true);
+	brcmstb_gpio_set_imask(bank, d->hwirq, true, false);
 }
 
 static void brcmstb_gpio_irq_ack(struct irq_data *d)
@@ -471,6 +481,7 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	priv->irq_chip.name = dev_name(dev);
 	priv->irq_chip.irq_disable = brcmstb_gpio_irq_mask;
 	priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
+	priv->irq_chip.irq_mask_ack = brcmstb_gpio_irq_mask_ack;
 	priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
 	priv->irq_chip.irq_ack = brcmstb_gpio_irq_ack;
 	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
-- 
2.43.0

[PATCH 3/3] gpio: brcmstb: allow parent_irq to wake

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

The classic parent_wake_irq can only occur after the system has
been placed into a hardware managed power management state. This
prevents its use for waking from software managed suspend states
like s2idle.

By allowing the parent_irq to be enabled for wake enabled GPIO
during suspend, these GPIO can now be used to wake from these
states. The 'suspended' boolean is introduced to support wake
event accounting.

Signed-off-by: Doug Berger <opendmb@gmail.com>
[florian: port changes after generic gpio chip conversion]
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/gpio/gpio-brcmstb.c | 90 +++++++++++++++++++++++++------------
 1 file changed, 62 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 5fb6612c2aa5..cecc7cf93796 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -54,6 +54,7 @@ struct brcmstb_gpio_priv {
 	int parent_irq;
 	int num_gpios;
 	int parent_wake_irq;
+	bool suspended;
 };
 
 #define MAX_GPIO_PER_BANK       32
@@ -231,6 +232,9 @@ static int brcmstb_gpio_priv_set_wake(struct brcmstb_gpio_priv *priv,
 {
 	int ret = 0;
 
+	if (priv->parent_wake_irq == priv->parent_irq)
+		return ret;
+
 	if (enable)
 		ret = enable_irq_wake(priv->parent_wake_irq);
 	else
@@ -281,6 +285,11 @@ static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
 		unsigned int offset;
 
+		if (priv->suspended && bank->wake_active & (u32)status) {
+			priv->suspended = false;
+			pm_wakeup_event(&priv->pdev->dev, 0);
+		}
+
 		for_each_set_bit(offset, &status, 32) {
 			if (offset >= bank->width)
 				dev_warn(&priv->pdev->dev,
@@ -454,18 +463,18 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	}
 
 	if (of_property_read_bool(np, "wakeup-source")) {
+		/*
+		 * Set wakeup capability so we can process boot-time
+		 * "wakeups" (e.g., from S5 cold boot)
+		 */
+		device_set_wakeup_capable(dev, true);
+		device_wakeup_enable(dev);
 		priv->parent_wake_irq = platform_get_irq(pdev, 1);
 		if (priv->parent_wake_irq < 0) {
-			priv->parent_wake_irq = 0;
+			priv->parent_wake_irq = priv->parent_irq;
 			dev_warn(dev,
 				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
 		} else {
-			/*
-			 * Set wakeup capability so we can process boot-time
-			 * "wakeups" (e.g., from S5 cold boot)
-			 */
-			device_set_wakeup_capable(dev, true);
-			device_wakeup_enable(dev);
 			err = devm_request_irq(dev, priv->parent_wake_irq,
 					       brcmstb_gpio_wake_irq_handler,
 					       IRQF_SHARED,
@@ -476,6 +485,7 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 				goto out_free_domain;
 			}
 		}
+		priv->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
 	}
 
 	priv->irq_chip.name = dev_name(dev);
@@ -486,9 +496,6 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	priv->irq_chip.irq_ack = brcmstb_gpio_irq_ack;
 	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
 
-	if (priv->parent_wake_irq)
-		priv->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
-
 	irq_set_chained_handler_and_data(priv->parent_irq,
 					 brcmstb_gpio_irq_handler, priv);
 	irq_set_status_flags(priv->parent_irq, IRQ_DISABLE_UNLAZY);
@@ -511,16 +518,11 @@ static void brcmstb_gpio_bank_save(struct brcmstb_gpio_priv *priv,
 					priv->reg_base + GIO_BANK_OFF(bank->id, i));
 }
 
-static void brcmstb_gpio_quiesce(struct device *dev, bool save)
+static void brcmstb_gpio_quiesce(struct brcmstb_gpio_priv *priv, bool save)
 {
-	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
 	struct brcmstb_gpio_bank *bank;
 	u32 imask;
 
-	/* disable non-wake interrupt */
-	if (priv->parent_irq >= 0)
-		disable_irq(priv->parent_irq);
-
 	list_for_each_entry(bank, &priv->bank_list, node) {
 		if (save)
 			brcmstb_gpio_bank_save(priv, bank);
@@ -538,8 +540,14 @@ static void brcmstb_gpio_quiesce(struct device *dev, bool save)
 
 static void brcmstb_gpio_shutdown(struct platform_device *pdev)
 {
+	struct brcmstb_gpio_priv *priv = dev_get_drvdata(&pdev->dev);
+
+	/* disable interrupts */
+	if (priv->parent_irq > 0)
+		disable_irq(priv->parent_irq);
+
 	/* Enable GPIO for S5 cold boot */
-	brcmstb_gpio_quiesce(&pdev->dev, false);
+	brcmstb_gpio_quiesce(priv, false);
 }
 
 static void brcmstb_gpio_bank_restore(struct brcmstb_gpio_priv *priv,
@@ -555,7 +563,32 @@ static void brcmstb_gpio_bank_restore(struct brcmstb_gpio_priv *priv,
 
 static int brcmstb_gpio_suspend(struct device *dev)
 {
-	brcmstb_gpio_quiesce(dev, true);
+	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
+
+	if (priv->parent_irq > 0)
+		priv->suspended = true;
+
+	return 0;
+}
+
+static int brcmstb_gpio_suspend_noirq(struct device *dev)
+{
+	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
+
+	/* Catch any wakeup sources occurring between suspend and noirq */
+	if (!priv->suspended)
+		return -EBUSY;
+
+	/* disable interrupts while we save the masks */
+	if (priv->parent_irq > 0)
+		disable_irq(priv->parent_irq);
+
+	brcmstb_gpio_quiesce(priv, true);
+
+	/* Now that the masks have been saved re-enable interrupts */
+	if (priv->parent_wake_irq)
+		enable_irq(priv->parent_irq);
+
 	return 0;
 }
 
@@ -563,25 +596,26 @@ static int brcmstb_gpio_resume(struct device *dev)
 {
 	struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
 	struct brcmstb_gpio_bank *bank;
-	bool need_wakeup_event = false;
 
-	list_for_each_entry(bank, &priv->bank_list, node) {
-		need_wakeup_event |= !!__brcmstb_gpio_get_active_irqs(bank);
-		brcmstb_gpio_bank_restore(priv, bank);
-	}
+	/* disable interrupts while we restore the masks */
+	if (priv->parent_wake_irq)
+		disable_irq(priv->parent_irq);
 
-	if (priv->parent_wake_irq && need_wakeup_event)
-		pm_wakeup_event(dev, 0);
+	priv->suspended = false;
+
+	list_for_each_entry(bank, &priv->bank_list, node)
+		brcmstb_gpio_bank_restore(priv, bank);
 
-	/* enable non-wake interrupt */
-	if (priv->parent_irq >= 0)
+	/* re-enable interrupts */
+	if (priv->parent_irq > 0)
 		enable_irq(priv->parent_irq);
 
 	return 0;
 }
 
 static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
-	.suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend),
+	.suspend = pm_sleep_ptr(brcmstb_gpio_suspend),
+	.suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend_noirq),
 	.resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
 };
 
-- 
2.43.0

Re: [PATCH 2/3] gpio: brcmstb: implement irq_mask_ack

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> From: Doug Berger <opendmb@gmail.com>
>
> The irq_mask_ack operation is slightly more efficient than doing
> irq_mask and irq_ack separately.

I would refer to the callbacks as

.irq_mask()
.irq_ack()

et cetera.

> More importantly for this driver it bypasses the check of
> irqd_irq_masked ensuring a previously masked but still active
> interrupt gets remasked if unmasked at the hardware level. This
> allows the driver to more efficiently unmask the wake capable
> interrupts when quiescing without needing to enable the irqs
> individually to clear the irqd_irq_masked state.

...

> -// Copyright (C) 2015-2017 Broadcom
> +// Copyright (C) 2015-2026 Broadcom

Shouldn't it be rather 2015-2017,2026 ? (In one case when I updated a
driver for Intel, I went via Git history to gather the info.)

...

>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
> -               unsigned int hwirq, bool enable)
> +               unsigned int hwirq, bool enable, bool ack)

This type of interface is usually discouraged as it makes code harder
to read and follow. Since there are a lot of duplication, I recommend
to move the ack op to a separate helper.

...

> -       gpio_generic_write_reg(&bank->chip,
> -                              priv->reg_base + GIO_MASK(bank->id), imask);
> +       if (ack)
> +               gpio_generic_write_reg(&bank->chip,
> +                                      priv->reg_base + GIO_MASK(bank->id),
> +                                      imask);

Id est this piece...



> +static void brcmstb_gpio_irq_mask_ack(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
> +
> +       brcmstb_gpio_set_imask(bank, d->hwirq, false, true);

...and call it here explicitly (seems the only place for it, so it can
even be just moved here without an intermediate helper).

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpio: brcmstb: allow parent_irq to wake

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:

> The classic parent_wake_irq can only occur after the system has
> been placed into a hardware managed power management state. This
> prevents its use for waking from software managed suspend states
> like s2idle.
>
> By allowing the parent_irq to be enabled for wake enabled GPIO
> during suspend, these GPIO can now be used to wake from these
> states. The 'suspended' boolean is introduced to support wake
> event accounting.

> Signed-off-by: Doug Berger <opendmb@gmail.com>
> [florian: port changes after generic gpio chip conversion]

Likewise in the previous patch I think this deserves the Co-developed-by tag.

> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

...

> +               if (priv->suspended && bank->wake_active & (u32)status) {

Why casting?

> +                       priv->suspended = false;
> +                       pm_wakeup_event(&priv->pdev->dev, 0);
> +               }

...

>  static void brcmstb_gpio_shutdown(struct platform_device *pdev)
>  {
> +       struct brcmstb_gpio_priv *priv = dev_get_drvdata(&pdev->dev);

> +       /* disable interrupts */

A useless comment.

> +       if (priv->parent_irq > 0)
> +               disable_irq(priv->parent_irq);
> +
>         /* Enable GPIO for S5 cold boot */
> -       brcmstb_gpio_quiesce(&pdev->dev, false);
> +       brcmstb_gpio_quiesce(priv, false);
>  }

...

>  static const struct dev_pm_ops brcmstb_gpio_pm_ops = {

> +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend_noirq),
>         .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),

May we use one of the PM macros for these two assignments?

>  };


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpio: brcmstb: allow parent_irq to wake

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:

> The classic parent_wake_irq can only occur after the system has
> been placed into a hardware managed power management state. This
> prevents its use for waking from software managed suspend states
> like s2idle.
>
> By allowing the parent_irq to be enabled for wake enabled GPIO
> during suspend, these GPIO can now be used to wake from these
> states. The 'suspended' boolean is introduced to support wake
> event accounting.

...

> -static void brcmstb_gpio_quiesce(struct device *dev, bool save)
> +static void brcmstb_gpio_quiesce(struct brcmstb_gpio_priv *priv, bool save)
>  {
> -       struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
>         struct brcmstb_gpio_bank *bank;
>         u32 imask;
>
> -       /* disable non-wake interrupt */
> -       if (priv->parent_irq >= 0)
> -               disable_irq(priv->parent_irq);
> -
>         list_for_each_entry(bank, &priv->bank_list, node) {
>                 if (save)
>                         brcmstb_gpio_bank_save(priv, bank);

>  static void brcmstb_gpio_shutdown(struct platform_device *pdev)

One more thing, how is "save" being used? I can't see it other than a dead code.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpio: brcmstb: allow parent_irq to wake

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 9:44 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:

...

> > -static void brcmstb_gpio_quiesce(struct device *dev, bool save)
> > +static void brcmstb_gpio_quiesce(struct brcmstb_gpio_priv *priv, bool save)
> >  {
> > -       struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
> >         struct brcmstb_gpio_bank *bank;
> >         u32 imask;
> >
> > -       /* disable non-wake interrupt */
> > -       if (priv->parent_irq >= 0)
> > -               disable_irq(priv->parent_irq);
> > -
> >         list_for_each_entry(bank, &priv->bank_list, node) {
> >                 if (save)
> >                         brcmstb_gpio_bank_save(priv, bank);

> One more thing, how is "save" being used? I can't see it other than a dead code.

Ah, it was a preexisted parameter, I was under the impression that it
was just added without use. Sorry for the noise.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] gpio: brcmstb: implement irq_mask_ack

[Florian Fainelli]

On 1/21/2026 11:36 PM, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> From: Doug Berger <opendmb@gmail.com>
>>
>> The irq_mask_ack operation is slightly more efficient than doing
>> irq_mask and irq_ack separately.
> 
> I would refer to the callbacks as
> 
> .irq_mask()
> .irq_ack()
> 
> et cetera.

Ack.

> 
>> More importantly for this driver it bypasses the check of
>> irqd_irq_masked ensuring a previously masked but still active
>> interrupt gets remasked if unmasked at the hardware level. This
>> allows the driver to more efficiently unmask the wake capable
>> interrupts when quiescing without needing to enable the irqs
>> individually to clear the irqd_irq_masked state.
> 
> ...
> 
>> -// Copyright (C) 2015-2017 Broadcom
>> +// Copyright (C) 2015-2026 Broadcom
> 
> Shouldn't it be rather 2015-2017,2026 ? (In one case when I updated a
> driver for Intel, I went via Git history to gather the info.)

Ack.

> 
> ...
> 
>>   static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>> -               unsigned int hwirq, bool enable)
>> +               unsigned int hwirq, bool enable, bool ack)
> 
> This type of interface is usually discouraged as it makes code harder
> to read and follow. Since there are a lot of duplication, I recommend
> to move the ack op to a separate helper.

Good point, knowing the order and what set in those parameters can be 
confusing.

> 
> ...
> 
>> -       gpio_generic_write_reg(&bank->chip,
>> -                              priv->reg_base + GIO_MASK(bank->id), imask);
>> +       if (ack)
>> +               gpio_generic_write_reg(&bank->chip,
>> +                                      priv->reg_base + GIO_MASK(bank->id),
>> +                                      imask);
> 
> Id est this piece...
> 
> 
> 
>> +static void brcmstb_gpio_irq_mask_ack(struct irq_data *d)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +       struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
>> +
>> +       brcmstb_gpio_set_imask(bank, d->hwirq, false, true);
> 
> ...and call it here explicitly (seems the only place for it, so it can
> even be just moved here without an intermediate helper).

Will do, thanks!
-- 
Florian

Re: [PATCH 3/3] gpio: brcmstb: allow parent_irq to wake

[Florian Fainelli]

On 1/21/2026 11:42 PM, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
> 
>> The classic parent_wake_irq can only occur after the system has
>> been placed into a hardware managed power management state. This
>> prevents its use for waking from software managed suspend states
>> like s2idle.
>>
>> By allowing the parent_irq to be enabled for wake enabled GPIO
>> during suspend, these GPIO can now be used to wake from these
>> states. The 'suspended' boolean is introduced to support wake
>> event accounting.
> 
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>> [florian: port changes after generic gpio chip conversion]
> 
> Likewise in the previous patch I think this deserves the Co-developed-by tag.

OK.

> 
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> ...
> 
>> +               if (priv->suspended && bank->wake_active & (u32)status) {
> 
> Why casting?

status is an unsigned long, which is what for_each_set_bit() expects, so 
it is intended here to ensure the top bits are not participating in the 
comparison, I think this is just being extra explicit with intent here.

> 
>> +                       priv->suspended = false;
>> +                       pm_wakeup_event(&priv->pdev->dev, 0);
>> +               }
> 
> ...
> 
>>   static void brcmstb_gpio_shutdown(struct platform_device *pdev)
>>   {
>> +       struct brcmstb_gpio_priv *priv = dev_get_drvdata(&pdev->dev);
> 
>> +       /* disable interrupts */
> 
> A useless comment.

Indeed.

> 
>> +       if (priv->parent_irq > 0)
>> +               disable_irq(priv->parent_irq);
>> +
>>          /* Enable GPIO for S5 cold boot */
>> -       brcmstb_gpio_quiesce(&pdev->dev, false);
>> +       brcmstb_gpio_quiesce(priv, false);
>>   }
> 
> ...
> 
>>   static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
> 
>> +       .suspend_noirq = pm_sleep_ptr(brcmstb_gpio_suspend_noirq),
>>          .resume_noirq = pm_sleep_ptr(brcmstb_gpio_resume),
> 
> May we use one of the PM macros for these two assignments?

Sure, can do that!
-- 
Florian

Re: [PATCH 2/3] gpio: brcmstb: implement irq_mask_ack

[Florian Fainelli]

On 1/22/2026 11:17 AM, Florian Fainelli wrote:
> 
> 
> On 1/21/2026 11:36 PM, Andy Shevchenko wrote:
>> On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
>> <florian.fainelli@broadcom.com> wrote:
>>>
>>> From: Doug Berger <opendmb@gmail.com>
>>>
>>> The irq_mask_ack operation is slightly more efficient than doing
>>> irq_mask and irq_ack separately.
>>
>> I would refer to the callbacks as
>>
>> .irq_mask()
>> .irq_ack()
>>
>> et cetera.
> 
> Ack.
> 
>>
>>> More importantly for this driver it bypasses the check of
>>> irqd_irq_masked ensuring a previously masked but still active
>>> interrupt gets remasked if unmasked at the hardware level. This
>>> allows the driver to more efficiently unmask the wake capable
>>> interrupts when quiescing without needing to enable the irqs
>>> individually to clear the irqd_irq_masked state.
>>
>> ...
>>
>>> -// Copyright (C) 2015-2017 Broadcom
>>> +// Copyright (C) 2015-2026 Broadcom
>>
>> Shouldn't it be rather 2015-2017,2026 ? (In one case when I updated a
>> driver for Intel, I went via Git history to gather the info.)
> 
> Ack.
> 
>>
>> ...
>>
>>>   static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>>> -               unsigned int hwirq, bool enable)
>>> +               unsigned int hwirq, bool enable, bool ack)
>>
>> This type of interface is usually discouraged as it makes code harder
>> to read and follow. Since there are a lot of duplication, I recommend
>> to move the ack op to a separate helper.
> 
> Good point, knowing the order and what set in those parameters can be 
> confusing.
> 
>>
>> ...
>>
>>> -       gpio_generic_write_reg(&bank->chip,
>>> -                              priv->reg_base + GIO_MASK(bank->id), 
>>> imask);
>>> +       if (ack)
>>> +               gpio_generic_write_reg(&bank->chip,
>>> +                                      priv->reg_base + 
>>> GIO_MASK(bank->id),
>>> +                                      imask);
>>
>> Id est this piece...
>>
>>
>>
>>> +static void brcmstb_gpio_irq_mask_ack(struct irq_data *d)
>>> +{
>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>> +       struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
>>> +
>>> +       brcmstb_gpio_set_imask(bank, d->hwirq, false, true);
>>
>> ...and call it here explicitly (seems the only place for it, so it can
>> even be just moved here without an intermediate helper).

Actually we need it to be part of brcmsftb_gpio_set_imask() because this 
is where the guard(gpio_generic_lock_irqsave) resides. I can't really 
see a better alternative, short of create two implementations: of 
brcmstb_gpio_set_imask() and brcmstb_gpio_set_imask_ack() which does not 
feel any better than the proposed patch.
--
Florian

Re: [PATCH 2/3] gpio: brcmstb: implement irq_mask_ack

[Florian Fainelli]

On 1/22/26 11:26, Florian Fainelli wrote:
> 
> 
> On 1/22/2026 11:17 AM, Florian Fainelli wrote:
>>
>>
>> On 1/21/2026 11:36 PM, Andy Shevchenko wrote:
>>> On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
>>> <florian.fainelli@broadcom.com> wrote:
>>>>
>>>> From: Doug Berger <opendmb@gmail.com>
>>>>
>>>> The irq_mask_ack operation is slightly more efficient than doing
>>>> irq_mask and irq_ack separately.
>>>
>>> I would refer to the callbacks as
>>>
>>> .irq_mask()
>>> .irq_ack()
>>>
>>> et cetera.
>>
>> Ack.
>>
>>>
>>>> More importantly for this driver it bypasses the check of
>>>> irqd_irq_masked ensuring a previously masked but still active
>>>> interrupt gets remasked if unmasked at the hardware level. This
>>>> allows the driver to more efficiently unmask the wake capable
>>>> interrupts when quiescing without needing to enable the irqs
>>>> individually to clear the irqd_irq_masked state.
>>>
>>> ...
>>>
>>>> -// Copyright (C) 2015-2017 Broadcom
>>>> +// Copyright (C) 2015-2026 Broadcom
>>>
>>> Shouldn't it be rather 2015-2017,2026 ? (In one case when I updated a
>>> driver for Intel, I went via Git history to gather the info.)
>>
>> Ack.
>>
>>>
>>> ...
>>>
>>>>   static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>>>> -               unsigned int hwirq, bool enable)
>>>> +               unsigned int hwirq, bool enable, bool ack)
>>>
>>> This type of interface is usually discouraged as it makes code harder
>>> to read and follow. Since there are a lot of duplication, I recommend
>>> to move the ack op to a separate helper.
>>
>> Good point, knowing the order and what set in those parameters can be 
>> confusing.
>>
>>>
>>> ...
>>>
>>>> -       gpio_generic_write_reg(&bank->chip,
>>>> -                              priv->reg_base + GIO_MASK(bank->id), 
>>>> imask);
>>>> +       if (ack)
>>>> +               gpio_generic_write_reg(&bank->chip,
>>>> +                                      priv->reg_base + 
>>>> GIO_MASK(bank->id),
>>>> +                                      imask);
>>>
>>> Id est this piece...
>>>
>>>
>>>
>>>> +static void brcmstb_gpio_irq_mask_ack(struct irq_data *d)
>>>> +{
>>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>>> +       struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
>>>> +
>>>> +       brcmstb_gpio_set_imask(bank, d->hwirq, false, true);
>>>
>>> ...and call it here explicitly (seems the only place for it, so it can
>>> even be just moved here without an intermediate helper).
> 
> Actually we need it to be part of brcmsftb_gpio_set_imask() because this 
> is where the guard(gpio_generic_lock_irqsave) resides. I can't really 
> see a better alternative, short of create two implementations: of 
> brcmstb_gpio_set_imask() and brcmstb_gpio_set_imask_ack() which does not 
> feel any better than the proposed patch.

Or creating yet another set of intermediary helpers that are not 
including the locking, and then using them in places where the locking 
needs to be used, yes, that's probably the cleanest, let me do that.
-- 
Florian

Re: [PATCH 3/3] gpio: brcmstb: allow parent_irq to wake

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 9:24 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
> On 1/21/2026 11:42 PM, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 3:06 AM Florian Fainelli
> > <florian.fainelli@broadcom.com> wrote:

...

> >> +               if (priv->suspended && bank->wake_active & (u32)status) {
> >
> > Why casting?
>
> status is an unsigned long, which is what for_each_set_bit() expects, so
> it is intended here to ensure the top bits are not participating in the
> comparison, I think this is just being extra explicit with intent here.

Isn't that guaranteed by the C standard?

> >> +                       priv->suspended = false;
> >> +                       pm_wakeup_event(&priv->pdev->dev, 0);
> >> +               }


-- 
With Best Regards,
Andy Shevchenko