[PATCH] gpio: realtek-otto: fix GPIO line IRQ offset

[PATCH] gpio: realtek-otto: fix GPIO line IRQ offset

[Sander Vanheule]

The irqchip uses one domain for all GPIO lines, so the line offset
should be determined w.r.t. the first line of the first port, not the
first line of the triggered port.

Fixes: 0d82fb1127fb ("gpio: Add Realtek Otto GPIO support")
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/gpio/gpio-realtek-otto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
index eeeb39bc171d..bd75401b549d 100644
--- a/drivers/gpio/gpio-realtek-otto.c
+++ b/drivers/gpio/gpio-realtek-otto.c
@@ -205,7 +205,7 @@ static void realtek_gpio_irq_handler(struct irq_desc *desc)
 		status = realtek_gpio_read_isr(ctrl, lines_done / 8);
 		port_pin_count = min(gc->ngpio - lines_done, 8U);
 		for_each_set_bit(offset, &status, port_pin_count)
-			generic_handle_domain_irq(gc->irq.domain, offset);
+			generic_handle_domain_irq(gc->irq.domain, offset + lines_done);
 	}
 
 	chained_irq_exit(irq_chip, desc);
-- 
2.31.1

Re: [PATCH] gpio: realtek-otto: fix GPIO line IRQ offset

[Andy Shevchenko]

On Thu, Oct 28, 2021 at 11:52 AM Sander Vanheule <sander@svanheule.net> wrote:
>
> The irqchip uses one domain for all GPIO lines, so the line offset
> should be determined w.r.t. the first line of the first port, not the
> first line of the triggered port.
>
> Fixes: 0d82fb1127fb ("gpio: Add Realtek Otto GPIO support")

Not sure it fixes anything (it wasn't working from day 1), but in any
case the patch is good.

> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  drivers/gpio/gpio-realtek-otto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
> index eeeb39bc171d..bd75401b549d 100644
> --- a/drivers/gpio/gpio-realtek-otto.c
> +++ b/drivers/gpio/gpio-realtek-otto.c
> @@ -205,7 +205,7 @@ static void realtek_gpio_irq_handler(struct irq_desc *desc)
>                 status = realtek_gpio_read_isr(ctrl, lines_done / 8);
>                 port_pin_count = min(gc->ngpio - lines_done, 8U);
>                 for_each_set_bit(offset, &status, port_pin_count)
> -                       generic_handle_domain_irq(gc->irq.domain, offset);
> +                       generic_handle_domain_irq(gc->irq.domain, offset + lines_done);

Looking into these '/ 8', '8U' sounds to me that it may be a good idea
to switch to for_each_set_clump8(). But it's out of scope here.

>         }
>
>         chained_irq_exit(irq_chip, desc);
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: realtek-otto: fix GPIO line IRQ offset

[Sander Vanheule]

On Thu, 2021-10-28 at 13:23 +0300, Andy Shevchenko wrote:
> On Thu, Oct 28, 2021 at 11:52 AM Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > The irqchip uses one domain for all GPIO lines, so the line offset
> > should be determined w.r.t. the first line of the first port, not the
> > first line of the triggered port.
> > 
> > Fixes: 0d82fb1127fb ("gpio: Add Realtek Otto GPIO support")
> 
> Not sure it fixes anything (it wasn't working from day 1), but in any
> case the patch is good.

The original patch was tested using a GPIO line on port A, so the missing offset was
irrelevant there (as it was zero). After recently acquiring a new device, I was trying to
use a line on port C, which resulted in unhandled interrupts.

> 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> >  drivers/gpio/gpio-realtek-otto.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
> > index eeeb39bc171d..bd75401b549d 100644
> > --- a/drivers/gpio/gpio-realtek-otto.c
> > +++ b/drivers/gpio/gpio-realtek-otto.c
> > @@ -205,7 +205,7 @@ static void realtek_gpio_irq_handler(struct irq_desc *desc)
> >                 status = realtek_gpio_read_isr(ctrl, lines_done / 8);
> >                 port_pin_count = min(gc->ngpio - lines_done, 8U);
> >                 for_each_set_bit(offset, &status, port_pin_count)
> > -                       generic_handle_domain_irq(gc->irq.domain, offset);
> > +                       generic_handle_domain_irq(gc->irq.domain, offset +
> > lines_done);
> 
> Looking into these '/ 8', '8U' sounds to me that it may be a good idea
> to switch to for_each_set_clump8(). But it's out of scope here.

For the current code, I could indeed have used for_each_set_clump8, instead of the u8
read_isr() accessor.

However, I'm also preparing support for the RTL9302 device I have recently acquired. On
RTL930x and RTL931x the port order is reversed, requiring different port order handling.
My plan is to provide different port_read_isr() functions for the different port orders,
that return the requested u8. I'll try to get that patch set out today as well.

Best,
Sander

Re: [PATCH] gpio: realtek-otto: fix GPIO line IRQ offset

[Bartosz Golaszewski]

On Thu, Oct 28, 2021 at 10:52 AM Sander Vanheule <sander@svanheule.net> wrote:
>
> The irqchip uses one domain for all GPIO lines, so the line offset
> should be determined w.r.t. the first line of the first port, not the
> first line of the triggered port.
>
> Fixes: 0d82fb1127fb ("gpio: Add Realtek Otto GPIO support")
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  drivers/gpio/gpio-realtek-otto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
> index eeeb39bc171d..bd75401b549d 100644
> --- a/drivers/gpio/gpio-realtek-otto.c
> +++ b/drivers/gpio/gpio-realtek-otto.c
> @@ -205,7 +205,7 @@ static void realtek_gpio_irq_handler(struct irq_desc *desc)
>                 status = realtek_gpio_read_isr(ctrl, lines_done / 8);
>                 port_pin_count = min(gc->ngpio - lines_done, 8U);
>                 for_each_set_bit(offset, &status, port_pin_count)
> -                       generic_handle_domain_irq(gc->irq.domain, offset);
> +                       generic_handle_domain_irq(gc->irq.domain, offset + lines_done);
>         }
>
>         chained_irq_exit(irq_chip, desc);
> --
> 2.31.1
>

I already sent my last fixes PR to Linus yesterday, so I queued it
with other patches for next. It'll get backported anyway to stable
branches.

Bart