[PATCH] pinctrl: sunxi: dt: number the IRQ banks from the first one that has IRQs

[PATCH] pinctrl: sunxi: dt: number the IRQ banks from the first one that has IRQs

[Pablo Mazzini]

prepare_function_table() assumes the first pin is in bank 0. On the
A523, which has no PA pins, every IRQ bank is numbered one too high:
the last bank writes past the end of irq_array, and interrupts are
looked up at the wrong offsets.

Count the IRQ banks from the first bank that has interrupts.

Fixes: f5e2cd34b12f ("pinctrl: sunxi: allow reading mux values from DT")
Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
index 50a16f3bd131..9c5eb21a86a5 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
@@ -208,7 +208,7 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
 	 * The extra functions will be filled in later.
 	 */
 	irq_bank = 0;
-	last_bank = 0;
+	last_bank = -1;
 	for (i = 0; i < npins; i++) {
 		struct sunxi_desc_pin *pin = &pins[i];
 		int bank = (pin->pin.number - pin_base) / PINS_PER_BANK;
@@ -221,16 +221,14 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
 		func[1].muxval = 1;
 
 		if (irq_mux) {
-			if (bank > last_bank)
+			if (last_bank >= 0 && bank > last_bank)
 				irq_bank++;
 			func[lastfunc].muxval = irq_mux;
 			func[lastfunc].irqbank = irq_bank;
 			func[lastfunc].irqnum = pin->pin.number % PINS_PER_BANK;
 			func[lastfunc].name = "irq";
-		}
-
-		if (bank > last_bank)
 			last_bank = bank;
+		}
 
 		pin->functions = func;
 
-- 
2.53.0

Re: [PATCH] pinctrl: sunxi: dt: number the IRQ banks from the first one that has IRQs

[Andre Przywara]

Hi Pablo,

thanks for sending a patch!

On 9/29/26 22:36, Pablo Mazzini wrote:
> prepare_function_table() assumes the first pin is in bank 0. On the
> A523, which has no PA pins, every IRQ bank is numbered one too high:
> the last bank writes past the end of irq_array, and interrupts are
> looked up at the wrong offsets.

Yes, this is a known issue, and I already provided a quite different fix 
for this[1]: the A523 manual actually lists a PortA GPIO interrupt, even 
though this is pretty likely not functional. But specifying this IRQ in 
the DT fixes the problem as well.
The DT binding does not say how the IRQs are assigned to the GPIO ports, 
so from that point of view my solution is similarly usable as yours - 
though admittedly yours looks a little better and more future proof.

I guess we now need to figure out how we want to handle this in the 
future, and add an explanation to the binding. The A733 has a similar 
problem: PortA does not exist. This time the manual does not explicitly 
specify a GPIO IRQ, but conspicuously leaves a gap before the PortB GPIO 
IRQ, so we can use that as well.

Linus, Chen-Yu, Jernej: what do you think?

Cheers,
Andre

[1] 
https://lore.kernel.org/linux-sunxi/20260327113006.3135663-4-andre.przywara@arm.com/

> 
> Count the IRQ banks from the first bank that has interrupts.
> 
> Fixes: f5e2cd34b12f ("pinctrl: sunxi: allow reading mux values from DT")
> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
> ---
>   drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
> index 50a16f3bd131..9c5eb21a86a5 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
> @@ -208,7 +208,7 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
>   	 * The extra functions will be filled in later.
>   	 */
>   	irq_bank = 0;
> -	last_bank = 0;
> +	last_bank = -1;
>   	for (i = 0; i < npins; i++) {
>   		struct sunxi_desc_pin *pin = &pins[i];
>   		int bank = (pin->pin.number - pin_base) / PINS_PER_BANK;
> @@ -221,16 +221,14 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
>   		func[1].muxval = 1;
>   
>   		if (irq_mux) {
> -			if (bank > last_bank)
> +			if (last_bank >= 0 && bank > last_bank)
>   				irq_bank++;
>   			func[lastfunc].muxval = irq_mux;
>   			func[lastfunc].irqbank = irq_bank;
>   			func[lastfunc].irqnum = pin->pin.number % PINS_PER_BANK;
>   			func[lastfunc].name = "irq";
> -		}
> -
> -		if (bank > last_bank)
>   			last_bank = bank;
> +		}
>   
>   		pin->functions = func;
>   

Re: [PATCH] pinctrl: sunxi: dt: number the IRQ banks from the first one that has IRQs

[Linus Walleij]

On Wed, Sep 30, 2026 at 5:27 PM Andre Przywara <andre.przywara@arm.com> wrote:

> Yes, this is a known issue, and I already provided a quite different fix
> for this[1]: the A523 manual actually lists a PortA GPIO interrupt, even
> though this is pretty likely not functional. But specifying this IRQ in
> the DT fixes the problem as well.
> The DT binding does not say how the IRQs are assigned to the GPIO ports,
> so from that point of view my solution is similarly usable as yours -
> though admittedly yours looks a little better and more future proof.
>
> I guess we now need to figure out how we want to handle this in the
> future, and add an explanation to the binding. The A733 has a similar
> problem: PortA does not exist. This time the manual does not explicitly
> specify a GPIO IRQ, but conspicuously leaves a gap before the PortB GPIO
> IRQ, so we can use that as well.
>
> Linus, Chen-Yu, Jernej: what do you think?

Can't we do both patches? Maybe there is something I don't get with this
code...

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sunxi: dt: number the IRQ banks from the first one that has IRQs

[Andre Przywara]

Hi Linus,

On 10/1/26 14:06, Linus Walleij wrote:
> On Wed, Sep 30, 2026 at 5:27 PM Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> Yes, this is a known issue, and I already provided a quite different fix
>> for this[1]: the A523 manual actually lists a PortA GPIO interrupt, even
>> though this is pretty likely not functional. But specifying this IRQ in
>> the DT fixes the problem as well.
>> The DT binding does not say how the IRQs are assigned to the GPIO ports,
>> so from that point of view my solution is similarly usable as yours -
>> though admittedly yours looks a little better and more future proof.
>>
>> I guess we now need to figure out how we want to handle this in the
>> future, and add an explanation to the binding. The A733 has a similar
>> problem: PortA does not exist. This time the manual does not explicitly
>> specify a GPIO IRQ, but conspicuously leaves a gap before the PortB GPIO
>> IRQ, so we can use that as well.
>>
>> Linus, Chen-Yu, Jernej: what do you think?
> 
> Can't we do both patches? Maybe there is something I don't get with this
> code...

Well, if we change the DT and the code, they fight each other, and would 
be off by one again. So we need to decide for one solution, and since 
this affects the DT, this has compatibility implications. At the moment 
the code assumes that GPIO IRQ counting starts with PortA, regardless of 
whether this implemented or not. That does not sound very future proof: 
while A523 specifies an IRQ for PortA (even though it's not 
implemented!), I am afraid future SoCs might not.
So having Pablo's patch would make a lot of sense, but would break with 
the just changed DT for the A523. That former DT change was fine, since 
it never worked before, and the new v7.2 DT should work on older kernels 
even.
So we could revert the DT patch, and take Pablo's code patch, with a 
Fixes: tag, but that would create some broken combinations meanwhile.

And in any case we should add a word to the binding document, to pin 
down what's expected here.

Cheers,
Andre.

Re: [PATCH] pinctrl: sunxi: dt: number the IRQ banks from the first one that has IRQs

[Linus Walleij]

On Tue, Sep 29, 2026 at 10:37 PM Pablo Mazzini <pmazzini@gmail.com> wrote:

> prepare_function_table() assumes the first pin is in bank 0. On the
> A523, which has no PA pins, every IRQ bank is numbered one too high:
> the last bank writes past the end of irq_array,

Why don't you add a patch to protect against that?

Something like this (completely untested, not even compiled)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 31cd142ce0f7..c0c90d9e3215 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -1632,9 +1632,10 @@ int sunxi_pinctrl_init_with_flags(struct
platform_device *pdev,
        else
                pctl->pow_mod_sel_offset = PIO_POW_MOD_SEL_REG;

+       pctl->irqs = IRQ_PER_BANK * pctl->desc->irq_banks;
        pctl->irq_array = devm_kcalloc(&pdev->dev,
-                                      IRQ_PER_BANK * pctl->desc->irq_banks,
-                                      sizeof(*pctl->irq_array),
+                                      pctl->irqs,
+                                      sizeof(unsigned int),
                                       GFP_KERNEL);
        if (!pctl->irq_array)
                return -ENOMEM;
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 8bd00c6ff628..9e0ed9c0c1ee 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -176,7 +176,8 @@ struct sunxi_pinctrl {
        struct sunxi_pinctrl_group      *groups;
        unsigned                        ngroups;
        int                             *irq;
-       unsigned                        *irq_array;
+       unsigned                        irq_array[] __counted_by(irqs);
+       unsigned                        irqs;
        raw_spinlock_t                  lock;
        /*
         * Output latch shadow, one word per bank.  Seeded lockless at


> and interrupts are
> looked up at the wrong offsets.
>
> Count the IRQ banks from the first bank that has interrupts.
(...)
>         irq_bank = 0;
> -       last_bank = 0;
> +       last_bank = -1;
>         for (i = 0; i < npins; i++) {
>                 struct sunxi_desc_pin *pin = &pins[i];
>                 int bank = (pin->pin.number - pin_base) / PINS_PER_BANK;
> @@ -221,16 +221,14 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
>                 func[1].muxval = 1;
>
>                 if (irq_mux) {
> -                       if (bank > last_bank)
> +                       if (last_bank >= 0 && bank > last_bank)
>                                 irq_bank++;
>                         func[lastfunc].muxval = irq_mux;
>                         func[lastfunc].irqbank = irq_bank;
>                         func[lastfunc].irqnum = pin->pin.number % PINS_PER_BANK;
>                         func[lastfunc].name = "irq";
> -               }
> -
> -               if (bank > last_bank)
>                         last_bank = bank;
> +               }

This looks reasonable.

Yours,
Linus Walleij