The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* Re: gpio-mt7621 unroutable IRQs to bank0
       [not found] <CAAMcf8C_A9dJ_v4QRKtb9eGNOpJ7BZNOGsFP4i2WFOZxOVBPnQ@mail.gmail.com>
@ 2026-05-05 12:01 ` Linus Walleij
  2026-05-05 14:21   ` Thomas Gleixner
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2026-05-05 12:01 UTC (permalink / raw)
  To: Vicente Bergas
  Cc: Sergio Paracuellos, Bartosz Golaszewski, Thomas Gleixner,
	Grant Likely, Anna-Maria Behnsen, Linux Kernel Mailing List,
	linux-gpio

Hi Vicente,

On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:

> drivers/gpio/gpio-mt7621.c is registering 96 GPIOs,
> all of them capable of generating IRQs.
> But the `struct irq_chip` can only handle 32 IRQs,
> so, 3 banks are instantiated.
>
> The DTS file specifies the interrupt for one device as
>  interrupt-parent = <&gpio>;
>  interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
> which should be routed to Bank0 GPIO0 but
> it is instead routed to Bank2. This is the bug.
>
> The call trace that leads to the problem is:
>  drivers/i2c/i2c-core-base.c:i2c_device_probe
>  drivers/base/property.c:fwnode_irq_get
>  drivers/of/irq.c:of_irq_get
>  include/linux/irqdomain.h:irq_find_host
>  include/linux/irqdomain.h:irq_find_matching_host
>  include/linux/irqdomain.h:irq_find_matching_fwnode
>  kernel/irq/irqdomain.c:irq_find_matching_fwspec
>
> As a way to prove that this is indeed the problem,
> the following workaround makes it work.
> It just inverts the sorting order of all matches,
> so it picks Bank0 instead of Bank2.

That's a tricksy bug, I can't exactly see where the issue
is.

I think to solve this you might need to allocate an external
irqdomain that deal with the three different gpiochip
instances when translating the irqs.

There is gpiochip_irqchip_add_domain() which is currently
only used in drivers/gpio/gpio-regmap.c, but it should do
the trick I think.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-05 12:01 ` gpio-mt7621 unroutable IRQs to bank0 Linus Walleij
@ 2026-05-05 14:21   ` Thomas Gleixner
  2026-05-05 15:05     ` Sergio Paracuellos
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2026-05-05 14:21 UTC (permalink / raw)
  To: Linus Walleij, Vicente Bergas
  Cc: Sergio Paracuellos, Bartosz Golaszewski, Grant Likely,
	Anna-Maria Behnsen, Linux Kernel Mailing List, linux-gpio

On Tue, May 05 2026 at 14:01, Linus Walleij wrote:
> On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:
>> As a way to prove that this is indeed the problem,
>> the following workaround makes it work.
>> It just inverts the sorting order of all matches,
>> so it picks Bank0 instead of Bank2.
>
> That's a tricksy bug, I can't exactly see where the issue
> is.
>
> I think to solve this you might need to allocate an external
> irqdomain that deal with the three different gpiochip
> instances when translating the irqs.

struct gpio_chip has this:

        /**
         * @of_node_instance_match:
         *
         * Determine if a chip is the right instance. Must be implemented by
         * any driver using more than one gpio_chip per device tree node.
         * Returns true if gc is the instance indicated by i (which is the
         * first cell in the phandles for GPIO lines and gpio-ranges).
         */
        bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);

That driver falls in the category and lacks that callback, no?

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-05 14:21   ` Thomas Gleixner
@ 2026-05-05 15:05     ` Sergio Paracuellos
  2026-05-05 21:36       ` Vicente Bergas
  0 siblings, 1 reply; 11+ messages in thread
From: Sergio Paracuellos @ 2026-05-05 15:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Linus Walleij, Vicente Bergas, Bartosz Golaszewski, Grant Likely,
	Anna-Maria Behnsen, Linux Kernel Mailing List, linux-gpio

Hi,

On Tue, May 5, 2026 at 4:21 PM Thomas Gleixner <tglx@kernel.org> wrote:
>
> On Tue, May 05 2026 at 14:01, Linus Walleij wrote:
> > On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:
> >> As a way to prove that this is indeed the problem,
> >> the following workaround makes it work.
> >> It just inverts the sorting order of all matches,
> >> so it picks Bank0 instead of Bank2.
> >
> > That's a tricksy bug, I can't exactly see where the issue
> > is.
> >
> > I think to solve this you might need to allocate an external
> > irqdomain that deal with the three different gpiochip
> > instances when translating the irqs.
>
> struct gpio_chip has this:
>
>         /**
>          * @of_node_instance_match:
>          *
>          * Determine if a chip is the right instance. Must be implemented by
>          * any driver using more than one gpio_chip per device tree node.
>          * Returns true if gc is the instance indicated by i (which is the
>          * first cell in the phandles for GPIO lines and gpio-ranges).
>          */
>         bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);
>
> That driver falls in the category and lacks that callback, no?
>
> Thanks,
>
>         tglx

The IP core used inside these SoCs has 3 banks of 32 GPIOs each but
there is only one device tree node because the registers of all the
banks are interwoven inside one single IO range. Thus, the driver
internally sets up a gpio controller instance per bank. The driver
lacks of_node_instance_match callback but I am not sure if it needs to
be implemented in this particular case since the driver is using
of_xlate callback for this.

Thanks,
    Sergio Paracuellos

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-05 15:05     ` Sergio Paracuellos
@ 2026-05-05 21:36       ` Vicente Bergas
  2026-05-06 17:43         ` Sergio Paracuellos
  0 siblings, 1 reply; 11+ messages in thread
From: Vicente Bergas @ 2026-05-05 21:36 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Thomas Gleixner, Linus Walleij, Bartosz Golaszewski, Grant Likely,
	Linux Kernel Mailing List, linux-gpio

On Tue, May 5, 2026 at 5:05 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi,
>
> On Tue, May 5, 2026 at 4:21 PM Thomas Gleixner <tglx@kernel.org> wrote:
> >
> > On Tue, May 05 2026 at 14:01, Linus Walleij wrote:
> > > On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > >> As a way to prove that this is indeed the problem,
> > >> the following workaround makes it work.
> > >> It just inverts the sorting order of all matches,
> > >> so it picks Bank0 instead of Bank2.
> > >
> > > That's a tricksy bug, I can't exactly see where the issue
> > > is.
> > >
> > > I think to solve this you might need to allocate an external
> > > irqdomain that deal with the three different gpiochip
> > > instances when translating the irqs.
> >
> > struct gpio_chip has this:
> >
> >         /**
> >          * @of_node_instance_match:
> >          *
> >          * Determine if a chip is the right instance. Must be implemented by
> >          * any driver using more than one gpio_chip per device tree node.
> >          * Returns true if gc is the instance indicated by i (which is the
> >          * first cell in the phandles for GPIO lines and gpio-ranges).
> >          */
> >         bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);
> >
> > That driver falls in the category and lacks that callback, no?
> >
> > Thanks,
> >
> >         tglx
>
> The IP core used inside these SoCs has 3 banks of 32 GPIOs each but
> there is only one device tree node because the registers of all the
> banks are interwoven inside one single IO range. Thus, the driver
> internally sets up a gpio controller instance per bank. The driver
> lacks of_node_instance_match callback but I am not sure if it needs to
> be implemented in this particular case since the driver is using
> of_xlate callback for this.
>
> Thanks,
>     Sergio Paracuellos

Hi all,
just tested with the suggested `of_node_instance_match` by applying
those changes:

```
--- a/drivers/gpio/gpio-mt7621.c
+++ b/drivers/gpio/gpio-mt7621.c
@@ -1,3 +1,5 @@
+#define DEBUG  1
+
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2009-2011 Gabor Juhos <juhosg@openwrt.org>
@@ -206,6 +208,18 @@
     return gpio % MTK_BANK_WIDTH;
 }

+static bool mediatek_gpio_node_instance_match(struct gpio_chip *chip,
unsigned int i)
+{
+    struct mtk_gc *rg = to_mediatek_gpio(chip);
+
+    struct mtk_gc *rg0 = rg - rg->bank;
+    struct mtk *mtk = container_of(rg0, struct mtk, gc_map[0]);
+    dev_dbg(mtk->dev, "%u <=> %d\n", i, rg->bank);
+    dump_stack();
+
+    return i == rg->bank;
+}
+
 static const struct irq_chip mt7621_irq_chip = {
     .name        = "mt7621-gpio",
     .irq_mask_ack    = mediatek_gpio_irq_mask,
@@ -253,6 +267,7 @@

     rg->chip.gc.of_gpio_n_cells = 2;
     rg->chip.gc.of_xlate = mediatek_gpio_xlate;
+    rg->chip.gc.of_node_instance_match = mediatek_gpio_node_instance_match;
     rg->chip.gc.label = devm_kasprintf(dev, GFP_KERNEL, "%s-bank%d",
                     dev_name(dev), bank);
     if (!rg->chip.gc.label)
```

It turns out that the `mediatek_gpio_node_instance_match` function is
never called.

Regards,
  Vicente.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-05 21:36       ` Vicente Bergas
@ 2026-05-06 17:43         ` Sergio Paracuellos
  2026-05-06 22:32           ` Vicente Bergas
  0 siblings, 1 reply; 11+ messages in thread
From: Sergio Paracuellos @ 2026-05-06 17:43 UTC (permalink / raw)
  To: Vicente Bergas
  Cc: Thomas Gleixner, Linus Walleij, Bartosz Golaszewski, Grant Likely,
	Linux Kernel Mailing List, linux-gpio

Hi Vicente,

On Tue, May 5, 2026 at 11:36 PM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Tue, May 5, 2026 at 5:05 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, May 5, 2026 at 4:21 PM Thomas Gleixner <tglx@kernel.org> wrote:
> > >
> > > On Tue, May 05 2026 at 14:01, Linus Walleij wrote:
> > > > On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > > >> As a way to prove that this is indeed the problem,
> > > >> the following workaround makes it work.
> > > >> It just inverts the sorting order of all matches,
> > > >> so it picks Bank0 instead of Bank2.
> > > >
> > > > That's a tricksy bug, I can't exactly see where the issue
> > > > is.
> > > >
> > > > I think to solve this you might need to allocate an external
> > > > irqdomain that deal with the three different gpiochip
> > > > instances when translating the irqs.
> > >
> > > struct gpio_chip has this:
> > >
> > >         /**
> > >          * @of_node_instance_match:
> > >          *
> > >          * Determine if a chip is the right instance. Must be implemented by
> > >          * any driver using more than one gpio_chip per device tree node.
> > >          * Returns true if gc is the instance indicated by i (which is the
> > >          * first cell in the phandles for GPIO lines and gpio-ranges).
> > >          */
> > >         bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);
> > >
> > > That driver falls in the category and lacks that callback, no?
> > >
> > > Thanks,
> > >
> > >         tglx
> >
> > The IP core used inside these SoCs has 3 banks of 32 GPIOs each but
> > there is only one device tree node because the registers of all the
> > banks are interwoven inside one single IO range. Thus, the driver
> > internally sets up a gpio controller instance per bank. The driver
> > lacks of_node_instance_match callback but I am not sure if it needs to
> > be implemented in this particular case since the driver is using
> > of_xlate callback for this.
> >
> > Thanks,
> >     Sergio Paracuellos
>
> Hi all,
> just tested with the suggested `of_node_instance_match` by applying
> those changes:
>
> ```
> --- a/drivers/gpio/gpio-mt7621.c
> +++ b/drivers/gpio/gpio-mt7621.c
> @@ -1,3 +1,5 @@
> +#define DEBUG  1
> +
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (C) 2009-2011 Gabor Juhos <juhosg@openwrt.org>
> @@ -206,6 +208,18 @@
>      return gpio % MTK_BANK_WIDTH;
>  }
>
> +static bool mediatek_gpio_node_instance_match(struct gpio_chip *chip,
> unsigned int i)
> +{
> +    struct mtk_gc *rg = to_mediatek_gpio(chip);
> +
> +    struct mtk_gc *rg0 = rg - rg->bank;
> +    struct mtk *mtk = container_of(rg0, struct mtk, gc_map[0]);
> +    dev_dbg(mtk->dev, "%u <=> %d\n", i, rg->bank);
> +    dump_stack();
> +
> +    return i == rg->bank;
> +}
> +
>  static const struct irq_chip mt7621_irq_chip = {
>      .name        = "mt7621-gpio",
>      .irq_mask_ack    = mediatek_gpio_irq_mask,
> @@ -253,6 +267,7 @@
>
>      rg->chip.gc.of_gpio_n_cells = 2;
>      rg->chip.gc.of_xlate = mediatek_gpio_xlate;
> +    rg->chip.gc.of_node_instance_match = mediatek_gpio_node_instance_match;
>      rg->chip.gc.label = devm_kasprintf(dev, GFP_KERNEL, "%s-bank%d",
>                      dev_name(dev), bank);
>      if (!rg->chip.gc.label)
> ```
>
> It turns out that the `mediatek_gpio_node_instance_match` function is
> never called.
>
> Regards,
>   Vicente.

We are using two cells and mediatek_gpio_xlate() callback for this.
IIUC, you would need 3 cells to get this
mediatek_gpio_node_instance_match() called and probably get rid of
mediatek_gpio_xlate() but I have never used of_node_instance_match()
callback so I can be totally wrong.

Best regards,
    Sergio Paracuellos

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-06 17:43         ` Sergio Paracuellos
@ 2026-05-06 22:32           ` Vicente Bergas
  2026-05-07  4:06             ` Sergio Paracuellos
  0 siblings, 1 reply; 11+ messages in thread
From: Vicente Bergas @ 2026-05-06 22:32 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Thomas Gleixner, Linus Walleij, Bartosz Golaszewski, Grant Likely,
	Linux Kernel Mailing List, linux-gpio

On Wed, May 6, 2026 at 7:43 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Vicente,
>
> On Tue, May 5, 2026 at 11:36 PM Vicente Bergas <vicencb@gmail.com> wrote:
> >
> > On Tue, May 5, 2026 at 5:05 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, May 5, 2026 at 4:21 PM Thomas Gleixner <tglx@kernel.org> wrote:
> > > >
> > > > On Tue, May 05 2026 at 14:01, Linus Walleij wrote:
> > > > > On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > > > >> As a way to prove that this is indeed the problem,
> > > > >> the following workaround makes it work.
> > > > >> It just inverts the sorting order of all matches,
> > > > >> so it picks Bank0 instead of Bank2.
> > > > >
> > > > > That's a tricksy bug, I can't exactly see where the issue
> > > > > is.
> > > > >
> > > > > I think to solve this you might need to allocate an external
> > > > > irqdomain that deal with the three different gpiochip
> > > > > instances when translating the irqs.
> > > >
> > > > struct gpio_chip has this:
> > > >
> > > >         /**
> > > >          * @of_node_instance_match:
> > > >          *
> > > >          * Determine if a chip is the right instance. Must be implemented by
> > > >          * any driver using more than one gpio_chip per device tree node.
> > > >          * Returns true if gc is the instance indicated by i (which is the
> > > >          * first cell in the phandles for GPIO lines and gpio-ranges).
> > > >          */
> > > >         bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);
> > > >
> > > > That driver falls in the category and lacks that callback, no?
> > > >
> > > > Thanks,
> > > >
> > > >         tglx
> > >
> > > The IP core used inside these SoCs has 3 banks of 32 GPIOs each but
> > > there is only one device tree node because the registers of all the
> > > banks are interwoven inside one single IO range. Thus, the driver
> > > internally sets up a gpio controller instance per bank. The driver
> > > lacks of_node_instance_match callback but I am not sure if it needs to
> > > be implemented in this particular case since the driver is using
> > > of_xlate callback for this.
> > >
> > > Thanks,
> > >     Sergio Paracuellos
> >
> > Hi all,
> > just tested with the suggested `of_node_instance_match` by applying
> > those changes:
> >
> > ```
> > --- a/drivers/gpio/gpio-mt7621.c
> > +++ b/drivers/gpio/gpio-mt7621.c
> > @@ -1,3 +1,5 @@
> > +#define DEBUG  1
> > +
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> >   * Copyright (C) 2009-2011 Gabor Juhos <juhosg@openwrt.org>
> > @@ -206,6 +208,18 @@
> >      return gpio % MTK_BANK_WIDTH;
> >  }
> >
> > +static bool mediatek_gpio_node_instance_match(struct gpio_chip *chip,
> > unsigned int i)
> > +{
> > +    struct mtk_gc *rg = to_mediatek_gpio(chip);
> > +
> > +    struct mtk_gc *rg0 = rg - rg->bank;
> > +    struct mtk *mtk = container_of(rg0, struct mtk, gc_map[0]);
> > +    dev_dbg(mtk->dev, "%u <=> %d\n", i, rg->bank);
> > +    dump_stack();
> > +
> > +    return i == rg->bank;
> > +}
> > +
> >  static const struct irq_chip mt7621_irq_chip = {
> >      .name        = "mt7621-gpio",
> >      .irq_mask_ack    = mediatek_gpio_irq_mask,
> > @@ -253,6 +267,7 @@
> >
> >      rg->chip.gc.of_gpio_n_cells = 2;
> >      rg->chip.gc.of_xlate = mediatek_gpio_xlate;
> > +    rg->chip.gc.of_node_instance_match = mediatek_gpio_node_instance_match;
> >      rg->chip.gc.label = devm_kasprintf(dev, GFP_KERNEL, "%s-bank%d",
> >                      dev_name(dev), bank);
> >      if (!rg->chip.gc.label)
> > ```
> >
> > It turns out that the `mediatek_gpio_node_instance_match` function is
> > never called.
> >
> > Regards,
> >   Vicente.
>
> We are using two cells and mediatek_gpio_xlate() callback for this.
> IIUC, you would need 3 cells to get this
> mediatek_gpio_node_instance_match() called and probably get rid of
> mediatek_gpio_xlate() but I have never used of_node_instance_match()
> callback so I can be totally wrong.
>
> Best regards,
>     Sergio Paracuellos

I tried removing of_xlate, but then many other things break.
The following patch makes it work, using both of_xlate and
of_node_instance_match.

Now, i am not sure if this is the proper way to fix the issue.
To me it looks ugly that gpios are specified using a single number
from 0 to 95, but associated interrupts need bank + offset.
A single number is more clear, as that way matches the available
documentation of the platform.

Another painpoint of this approach is that existing DT files need to be changed.

If this can be improved: how?
Otherwise, i will update `mediatek,mt7621-gpio.yaml` and send it all
as a pull request.

```
--- mt7628an.dtsi
+++ mt7628an.dtsi
@@ -99,7 +99,7 @@
       interrupt-parent = <&intc>;
       interrupts = <6>;

-      #interrupt-cells = <2>;
+      #interrupt-cells = <3>;
       interrupt-controller;

       gpio-controller;
--- board.dts
+++ board.dts
@@ -191,7 +191,7 @@
     compatible = "focaltech,ft6236";
     reg = <0x38>;
     interrupt-parent = <&gpio>;
-    interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
+    interrupts = <0 0 IRQ_TYPE_EDGE_FALLING>;
     reset-gpios = <&gpio 2 GPIO_ACTIVE_LOW>;
   };
 };
--- a/drivers/gpio/gpio-mt7621.c
+++ b/drivers/gpio/gpio-mt7621.c
@@ -206,6 +206,11 @@
   return gpio % MTK_BANK_WIDTH;
 }

+static bool mediatek_gpio_node_instance_match(struct gpio_chip *chip,
unsigned int i)
+{
+  return i == to_mediatek_gpio(chip)->bank;
+}
+
 static const struct irq_chip mt7621_irq_chip = {
   .name    = "mt7621-gpio",
   .irq_mask_ack  = mediatek_gpio_irq_mask,
@@ -253,6 +258,7 @@

   rg->chip.gc.of_gpio_n_cells = 2;
   rg->chip.gc.of_xlate = mediatek_gpio_xlate;
+  rg->chip.gc.of_node_instance_match = mediatek_gpio_node_instance_match;
   rg->chip.gc.label = devm_kasprintf(dev, GFP_KERNEL, "%s-bank%d",
           dev_name(dev), bank);
   if (!rg->chip.gc.label)
```

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-06 22:32           ` Vicente Bergas
@ 2026-05-07  4:06             ` Sergio Paracuellos
  2026-05-07  8:54               ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Sergio Paracuellos @ 2026-05-07  4:06 UTC (permalink / raw)
  To: Vicente Bergas
  Cc: Thomas Gleixner, Linus Walleij, Bartosz Golaszewski, Grant Likely,
	Linux Kernel Mailing List, linux-gpio

On Thu, May 7, 2026 at 12:32 AM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Wed, May 6, 2026 at 7:43 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Vicente,
> >
> > On Tue, May 5, 2026 at 11:36 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > >
> > > On Tue, May 5, 2026 at 5:05 PM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, May 5, 2026 at 4:21 PM Thomas Gleixner <tglx@kernel.org> wrote:
> > > > >
> > > > > On Tue, May 05 2026 at 14:01, Linus Walleij wrote:
> > > > > > On Sat, May 2, 2026 at 11:52 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > > > > >> As a way to prove that this is indeed the problem,
> > > > > >> the following workaround makes it work.
> > > > > >> It just inverts the sorting order of all matches,
> > > > > >> so it picks Bank0 instead of Bank2.
> > > > > >
> > > > > > That's a tricksy bug, I can't exactly see where the issue
> > > > > > is.
> > > > > >
> > > > > > I think to solve this you might need to allocate an external
> > > > > > irqdomain that deal with the three different gpiochip
> > > > > > instances when translating the irqs.
> > > > >
> > > > > struct gpio_chip has this:
> > > > >
> > > > >         /**
> > > > >          * @of_node_instance_match:
> > > > >          *
> > > > >          * Determine if a chip is the right instance. Must be implemented by
> > > > >          * any driver using more than one gpio_chip per device tree node.
> > > > >          * Returns true if gc is the instance indicated by i (which is the
> > > > >          * first cell in the phandles for GPIO lines and gpio-ranges).
> > > > >          */
> > > > >         bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);
> > > > >
> > > > > That driver falls in the category and lacks that callback, no?
> > > > >
> > > > > Thanks,
> > > > >
> > > > >         tglx
> > > >
> > > > The IP core used inside these SoCs has 3 banks of 32 GPIOs each but
> > > > there is only one device tree node because the registers of all the
> > > > banks are interwoven inside one single IO range. Thus, the driver
> > > > internally sets up a gpio controller instance per bank. The driver
> > > > lacks of_node_instance_match callback but I am not sure if it needs to
> > > > be implemented in this particular case since the driver is using
> > > > of_xlate callback for this.
> > > >
> > > > Thanks,
> > > >     Sergio Paracuellos
> > >
> > > Hi all,
> > > just tested with the suggested `of_node_instance_match` by applying
> > > those changes:
> > >
> > > ```
> > > --- a/drivers/gpio/gpio-mt7621.c
> > > +++ b/drivers/gpio/gpio-mt7621.c
> > > @@ -1,3 +1,5 @@
> > > +#define DEBUG  1
> > > +
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  /*
> > >   * Copyright (C) 2009-2011 Gabor Juhos <juhosg@openwrt.org>
> > > @@ -206,6 +208,18 @@
> > >      return gpio % MTK_BANK_WIDTH;
> > >  }
> > >
> > > +static bool mediatek_gpio_node_instance_match(struct gpio_chip *chip,
> > > unsigned int i)
> > > +{
> > > +    struct mtk_gc *rg = to_mediatek_gpio(chip);
> > > +
> > > +    struct mtk_gc *rg0 = rg - rg->bank;
> > > +    struct mtk *mtk = container_of(rg0, struct mtk, gc_map[0]);
> > > +    dev_dbg(mtk->dev, "%u <=> %d\n", i, rg->bank);
> > > +    dump_stack();
> > > +
> > > +    return i == rg->bank;
> > > +}
> > > +
> > >  static const struct irq_chip mt7621_irq_chip = {
> > >      .name        = "mt7621-gpio",
> > >      .irq_mask_ack    = mediatek_gpio_irq_mask,
> > > @@ -253,6 +267,7 @@
> > >
> > >      rg->chip.gc.of_gpio_n_cells = 2;
> > >      rg->chip.gc.of_xlate = mediatek_gpio_xlate;
> > > +    rg->chip.gc.of_node_instance_match = mediatek_gpio_node_instance_match;
> > >      rg->chip.gc.label = devm_kasprintf(dev, GFP_KERNEL, "%s-bank%d",
> > >                      dev_name(dev), bank);
> > >      if (!rg->chip.gc.label)
> > > ```
> > >
> > > It turns out that the `mediatek_gpio_node_instance_match` function is
> > > never called.
> > >
> > > Regards,
> > >   Vicente.
> >
> > We are using two cells and mediatek_gpio_xlate() callback for this.
> > IIUC, you would need 3 cells to get this
> > mediatek_gpio_node_instance_match() called and probably get rid of
> > mediatek_gpio_xlate() but I have never used of_node_instance_match()
> > callback so I can be totally wrong.
> >
> > Best regards,
> >     Sergio Paracuellos
>
> I tried removing of_xlate, but then many other things break.
> The following patch makes it work, using both of_xlate and
> of_node_instance_match.
>
> Now, i am not sure if this is the proper way to fix the issue.
> To me it looks ugly that gpios are specified using a single number
> from 0 to 95, but associated interrupts need bank + offset.
> A single number is more clear, as that way matches the available
> documentation of the platform.
>
> Another painpoint of this approach is that existing DT files need to be changed.
>
> If this can be improved: how?
> Otherwise, i will update `mediatek,mt7621-gpio.yaml` and send it all
> as a pull request.

Linus, Bartosz, any advice regarding this?

>
> ```
> --- mt7628an.dtsi
> +++ mt7628an.dtsi
> @@ -99,7 +99,7 @@
>        interrupt-parent = <&intc>;
>        interrupts = <6>;
>
> -      #interrupt-cells = <2>;
> +      #interrupt-cells = <3>;
>        interrupt-controller;
>
>        gpio-controller;
> --- board.dts
> +++ board.dts
> @@ -191,7 +191,7 @@
>      compatible = "focaltech,ft6236";
>      reg = <0x38>;
>      interrupt-parent = <&gpio>;
> -    interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
> +    interrupts = <0 0 IRQ_TYPE_EDGE_FALLING>;
>      reset-gpios = <&gpio 2 GPIO_ACTIVE_LOW>;
>    };
>  };
> --- a/drivers/gpio/gpio-mt7621.c
> +++ b/drivers/gpio/gpio-mt7621.c
> @@ -206,6 +206,11 @@
>    return gpio % MTK_BANK_WIDTH;
>  }
>
> +static bool mediatek_gpio_node_instance_match(struct gpio_chip *chip,
> unsigned int i)
> +{
> +  return i == to_mediatek_gpio(chip)->bank;
> +}
> +
>  static const struct irq_chip mt7621_irq_chip = {
>    .name    = "mt7621-gpio",
>    .irq_mask_ack  = mediatek_gpio_irq_mask,
> @@ -253,6 +258,7 @@
>
>    rg->chip.gc.of_gpio_n_cells = 2;
>    rg->chip.gc.of_xlate = mediatek_gpio_xlate;
> +  rg->chip.gc.of_node_instance_match = mediatek_gpio_node_instance_match;
>    rg->chip.gc.label = devm_kasprintf(dev, GFP_KERNEL, "%s-bank%d",
>            dev_name(dev), bank);
>    if (!rg->chip.gc.label)
> ```

Vicente, I know that you are using openwrt dts but take into account
that there are already in tree device trees that must be updated as
well if you end up updating the mt7621-gpio yaml doc:
- https://elixir.bootlin.com/linux/v7.0.1/source/arch/mips/boot/dts/ralink/mt7621.dtsi#L180
- https://elixir.bootlin.com/linux/v7.0.1/source/arch/mips/boot/dts/ralink/mt7628a.dtsi#L173

Thanks,
    Sergio Paracuellos

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-07  4:06             ` Sergio Paracuellos
@ 2026-05-07  8:54               ` Linus Walleij
  2026-05-07 10:52                 ` Sergio Paracuellos
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2026-05-07  8:54 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Vicente Bergas, Thomas Gleixner, Bartosz Golaszewski,
	Grant Likely, Linux Kernel Mailing List, linux-gpio

On Thu, May 7, 2026 at 6:06 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:

> Linus, Bartosz, any advice regarding this?

I sent the advice to try to use an external irqdomain already?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-07  8:54               ` Linus Walleij
@ 2026-05-07 10:52                 ` Sergio Paracuellos
  2026-05-12  0:17                   ` Vicente Bergas
  0 siblings, 1 reply; 11+ messages in thread
From: Sergio Paracuellos @ 2026-05-07 10:52 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Vicente Bergas, Thomas Gleixner, Bartosz Golaszewski,
	Grant Likely, Linux Kernel Mailing List, linux-gpio

On Thu, May 7, 2026 at 10:54 AM Linus Walleij <linusw@kernel.org> wrote:
>
> On Thu, May 7, 2026 at 6:06 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>
> > Linus, Bartosz, any advice regarding this?
>
> I sent the advice to try to use an external irqdomain already?

True. Sorry, I missed that somehow.

Best regards,
   Sergio Paracuellos

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-07 10:52                 ` Sergio Paracuellos
@ 2026-05-12  0:17                   ` Vicente Bergas
  2026-05-14 12:00                     ` Sergio Paracuellos
  0 siblings, 1 reply; 11+ messages in thread
From: Vicente Bergas @ 2026-05-12  0:17 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Linus Walleij, Thomas Gleixner, Bartosz Golaszewski, Grant Likely,
	Linux Kernel Mailing List, linux-gpio

On Thu, May 7, 2026 at 12:52 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Thu, May 7, 2026 at 10:54 AM Linus Walleij <linusw@kernel.org> wrote:
> >
> > On Thu, May 7, 2026 at 6:06 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> >
> > > Linus, Bartosz, any advice regarding this?
> >
> > I sent the advice to try to use an external irqdomain already?
>
> True. Sorry, I missed that somehow.
>
> Best regards,
>    Sergio Paracuellos

Please, can somebody write some general guidelines on how to use an
external irqdomain?
I'm lost here, i don't know where to start.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: gpio-mt7621 unroutable IRQs to bank0
  2026-05-12  0:17                   ` Vicente Bergas
@ 2026-05-14 12:00                     ` Sergio Paracuellos
  0 siblings, 0 replies; 11+ messages in thread
From: Sergio Paracuellos @ 2026-05-14 12:00 UTC (permalink / raw)
  To: Vicente Bergas
  Cc: Linus Walleij, Thomas Gleixner, Bartosz Golaszewski, Grant Likely,
	Linux Kernel Mailing List, linux-gpio

On Tue, May 12, 2026 at 2:17 AM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Thu, May 7, 2026 at 12:52 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > On Thu, May 7, 2026 at 10:54 AM Linus Walleij <linusw@kernel.org> wrote:
> > >
> > > On Thu, May 7, 2026 at 6:06 AM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > > Linus, Bartosz, any advice regarding this?
> > >
> > > I sent the advice to try to use an external irqdomain already?
> >
> > True. Sorry, I missed that somehow.
> >
> > Best regards,
> >    Sergio Paracuellos
>
> Please, can somebody write some general guidelines on how to use an
> external irqdomain?
> I'm lost here, i don't know where to start.

IIUC we need to register an external irq_domain for the 96 pins that
the three banks (32 gpios each) can reference. Current driver has
three gpio_chip instances but we will have one external domain now so
I don't know but we would need probably a specific xlate function so
when the device tree asks for a gpio on whatever bank the driver maps
the correct offset in the external domain. The other tricky part would
be the irq handler itself that now has to iterate the three banks to
trigger the proper virtual IRQ.

Linus, is my understanding correct?

Thanks,
    Sergio Paracuellos

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-05-14 12:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CAAMcf8C_A9dJ_v4QRKtb9eGNOpJ7BZNOGsFP4i2WFOZxOVBPnQ@mail.gmail.com>
2026-05-05 12:01 ` gpio-mt7621 unroutable IRQs to bank0 Linus Walleij
2026-05-05 14:21   ` Thomas Gleixner
2026-05-05 15:05     ` Sergio Paracuellos
2026-05-05 21:36       ` Vicente Bergas
2026-05-06 17:43         ` Sergio Paracuellos
2026-05-06 22:32           ` Vicente Bergas
2026-05-07  4:06             ` Sergio Paracuellos
2026-05-07  8:54               ` Linus Walleij
2026-05-07 10:52                 ` Sergio Paracuellos
2026-05-12  0:17                   ` Vicente Bergas
2026-05-14 12:00                     ` Sergio Paracuellos

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox