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

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

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

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

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