Re: [PATCH v2] gpio: mt7621: fix interrupt banks mapping on gpio chips

[Sergio Paracuellos]

On Mon, Jun 8, 2026 at 9:22 PM Vicente Bergas <vicencb@xxxxxxxxx> wrote:
>
> On Mon, Jun 8, 2026 at 4:27 PM Sergio Paracuellos
> <sergio.paracuellos@xxxxxxxxx> wrote:
> >
> > On Mon, Jun 8, 2026 at 4:02 PM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
> > >
> > > On Mon, 8 Jun 2026 11:40:57 +0200, Sergio Paracuellos
> > > <sergio.paracuellos@xxxxxxxxx> said:
> > > > Hi,
> > > >
> > > > On Mon, Jun 8, 2026 at 11:05 AM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
> > > >>
> > > >> On Tue, 2 Jun 2026 16:25:13 +0200, Sergio Paracuellos
> > > >> <sergio.paracuellos@xxxxxxxxx> said:
> > > >> > The GPIO controller's registers are organized as sets of eight 32-bit
> > > >> > registers with each set controlling a bank of up to 32 pins. A single
> > > >> > interrupt is shared for all of the banks handled by the controller.
> > > >> > The driver implements this using three gpio chip instances every one
> > > >> > with its own irq chip. Every single pin can generate interrupts having
> > > >> > a total of 96 possible interrupts here. It looks like there is a problem
> > > >> > with interrupts being properly mapped to the gpio bank using this solution.
> > > >> > This problem report is in the following lore's link [0].
> > > >> >
> > > >> > Device tree is using two cells for this, so only the interrupt pin and the
> > > >> > interrupt type are described there. Changing to have three cells to setup
> > > >> > also the bank and implement 'of_node_instance_match()' would also work but
> > > >> > this would be an ABI breakage and also a bit incoherent since gpios itself
> > > >> > are also using two cells and properly mapped in desired bank using through
> > > >> > its pin number on 'of_xlate()'.
> > > >> >
> > > >> > That said, register a linear IRQ domain of the total of 96 interrupts shared
> > > >> > with the three gpio chip instances so the bank and the interrupt is properly
> > > >> > decoded and devices using gpio IRQs properly work.
> > > >> >
> > > >> > [0]: https://lore.kernel.org/linux-gpio/CAAMcf8C_A9dJ_v4QRKtb9eGNOpJ7BZNOGsFP4i2WFOZxOVBPnQ@xxxxxxxxxxxxxx/T/#u
> > > >> >
> > > >> > Fixes: 4ba9c3afda41 ("gpio: mt7621: Add a driver for MT7621")
> > > >> > Co-developed-by: Vicente Bergas <vicencb@xxxxxxxxx>
> > > >> > Signed-off-by: Vicente Bergas <vicencb@xxxxxxxxx>
> > > >> > Tested-by: Vicente Bergas <vicencb@xxxxxxxxx>
> > > >> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > > >> > ---
> > > >>
> > > >> Hi!
> > > >>
> > > >> Can you look at the sashiko review? Especially the bit about tracking the
> > > >> GPIOD_FLAG_IRQ_IS_ENABLED flag looks correct.
> > > >
> > > > I got rid of those two calls (gpiochip_enable_irq() and
> > > > gpiochip_disable_irq()) because the driver "gpio-brcmstb" which is the
> > > > one I based my changes on was not used them at all. We have not found
> > > > anything weird related with that on testing. I do believe that since
> > > > we are using our own callbacks for 'irq_request_resources()' and
> > > > 'irq_release_resources()' we are safe here. Regarding the others I am
> > > > not sure, but the introduction of the remove stuff for the irq domain
> > > > is because there are no devm_* functions for that. Other resources in
> > > > driver are using devm versions so I think the changes are ok as they
> > > > are...
> > > >
> > >
> > > It's about GPIO core: a GPIO that appears as "free" (users can request it) but
> > > was earlier enabled for interrupts cannot be requested in output mode - only
> > > input works. Without this flag set, gpiod_direction_output_nonotify() will allow
> > > you to set direction to output.
> >
> > I see. I need Vicente to re-test without removing
> > gpiochip_enable_irq() and gpiochip_disable_irq() to see if everything
> > is still ok.
> >
> > Vicente, would you mind to test the following change on top of this v2 PATCH?
> >
> > diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
> > index c36aa0abd0c6..a814885ccd5d 100644
> > --- a/drivers/gpio/gpio-mt7621.c
> > +++ b/drivers/gpio/gpio-mt7621.c
> > @@ -144,6 +144,8 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
> >         u32 mask = mt7621_gpio_hwirq_to_offset(d->hwirq, rg);
> >         u32 rise, fall, high, low;
> >
> > +       gpiochip_enable_irq(gc, mask);
> > +
> >         guard(gpio_generic_lock_irqsave)(&rg->chip);
> >
> >         rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> > @@ -174,6 +176,8 @@ mediatek_gpio_irq_mask(struct irq_data *d)
> >                 mtk_gpio_w32(rg, GPIO_REG_HLVL, high & ~BIT(mask));
> >                 mtk_gpio_w32(rg, GPIO_REG_LLVL, low & ~BIT(mask));
> >         }
> > +
> > +       gpiochip_disable_irq(gc, mask);
> >  }
> >
> > Thanks,
> >     Sergio Paracuellos
>
> Hi Sergio, test successful.
> I tested the buttons and touch and it still works.
> I did _not_ test if the irqs are disallowed to be used as gpios in output mode.

Thanks for testing, Vicente. I already sent v3 with these changes included: [0]

Best regards,
    Sergio Paracuellos

[0]: https://lore.kernel.org/linux-gpio/20260609031118.2275735-1-sergio.paracuellos@xxxxxxxxx/T/#u