[PATCH] gpio: aggregator: Add interrupt support

[PATCH] gpio: aggregator: Add interrupt support

[Geert Uytterhoeven]

Currently the GPIO Aggregator does not support interrupts.  This means
that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
and userspace applications using line events do not work.

Add interrupt support by providing a gpio_chip.to_irq() callback, which
just calls into the parent GPIO controller.

Note that this does not implement full interrupt controller (irq_chip)
support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
still does not work.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
I would prefer to avoid implementing irq_chip support, until there is a
real use case for this.

This has been tested with gpio-keys and gpiomon on the Koelsch
development board:

  - gpio-keys, using a DT overlay[1]:

	$ overlay add r8a7791-koelsch-keyboard-controlled-led
	$ echo gpio-aggregator > /sys/devices/platform/frobnicator/driver_override
	$ echo frobnicator > /sys/bus/platform/drivers/gpio-aggregator/bind

	$ gpioinfo frobnicator
	gpiochip12 - 3 lines:
		line   0:      "light"      "light"  output  active-high [used]
		line   1:         "on"         "On"   input   active-low [used]
		line   2:        "off"        "Off"   input   active-low [used]

	$ echo 255 > /sys/class/leds/light/brightness
	$ echo 0 > /sys/class/leds/light/brightness

	$ evtest /dev/input/event0

  - gpiomon, using the GPIO sysfs API:

	$ echo keyboard > /sys/bus/platform/drivers/gpio-keys/unbind
	$ echo e6055800.gpio 2,6 > /sys/bus/platform/drivers/gpio-aggregator/new_device
	$ gpiomon gpiochip12 0 1

[1] "ARM: dts: koelsch: Add overlay for keyboard-controlled LED"
    https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays&id=c78d817869e63a3485bb4ab98aeea6ce368a396e
---
 drivers/gpio/gpio-aggregator.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 34e35b64dcdc0581..2ff867d2a3630d3b 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -374,6 +374,13 @@ static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
 	return gpiod_set_config(fwd->descs[offset], config);
 }
 
+static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+	return gpiod_to_irq(fwd->descs[offset]);
+}
+
 /**
  * gpiochip_fwd_create() - Create a new GPIO forwarder
  * @dev: Parent device pointer
@@ -414,7 +421,8 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	for (i = 0; i < ngpios; i++) {
 		struct gpio_chip *parent = gpiod_to_chip(descs[i]);
 
-		dev_dbg(dev, "%u => gpio-%d\n", i, desc_to_gpio(descs[i]));
+		dev_dbg(dev, "%u => gpio %d irq %d\n", i,
+			desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
 
 		if (gpiod_cansleep(descs[i]))
 			chip->can_sleep = true;
@@ -432,6 +440,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	chip->get_multiple = gpio_fwd_get_multiple_locked;
 	chip->set = gpio_fwd_set;
 	chip->set_multiple = gpio_fwd_set_multiple_locked;
+	chip->to_irq = gpio_fwd_to_irq;
 	chip->base = -1;
 	chip->ngpio = ngpios;
 	fwd->descs = descs;
-- 
2.25.1

Re: [PATCH] gpio: aggregator: Add interrupt support

[Andy Shevchenko]

On Mon, Oct 4, 2021 at 3:45 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Currently the GPIO Aggregator does not support interrupts.  This means
> that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
> and userspace applications using line events do not work.
>
> Add interrupt support by providing a gpio_chip.to_irq() callback, which
> just calls into the parent GPIO controller.
>
> Note that this does not implement full interrupt controller (irq_chip)
> support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
> still does not work.

...

> @@ -414,7 +421,8 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>         for (i = 0; i < ngpios; i++) {
>                 struct gpio_chip *parent = gpiod_to_chip(descs[i]);
>
> -               dev_dbg(dev, "%u => gpio-%d\n", i, desc_to_gpio(descs[i]));
> +               dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> +                       desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));

This is an unconditional call that will allocate the IRQ descriptor
even if we don't use it. Correct?
If so, I don't like this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: aggregator: Add interrupt support

[Geert Uytterhoeven]

Hi Andy,

On Mon, Oct 4, 2021 at 3:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Oct 4, 2021 at 3:45 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Currently the GPIO Aggregator does not support interrupts.  This means
> > that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
> > and userspace applications using line events do not work.
> >
> > Add interrupt support by providing a gpio_chip.to_irq() callback, which
> > just calls into the parent GPIO controller.
> >
> > Note that this does not implement full interrupt controller (irq_chip)
> > support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
> > still does not work.
>
> ...
>
> > @@ -414,7 +421,8 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> >         for (i = 0; i < ngpios; i++) {
> >                 struct gpio_chip *parent = gpiod_to_chip(descs[i]);
> >
> > -               dev_dbg(dev, "%u => gpio-%d\n", i, desc_to_gpio(descs[i]));
> > +               dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> > +                       desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
>
> This is an unconditional call that will allocate the IRQ descriptor

If DEBUG and CONFIG_DYNAMIC_DEBUG* are not enabled, it's a no-op
(protected by if (0) { ... }).
If CONFIG_DYNAMIC_DEBUG is enabled, the operation is a no-op if not
enabled dynamically (if (dynamic_checl) { ... }).
If DEBUG (CONFIG_DEBUG_GPIO) is enabled, the output is wanted.

(yes, I've just checked the preprocessor and assembler output ;-).

> even if we don't use it. Correct?

It calls .to_irq() of the parent GPIO controller, which is usually
just doing some offset addition.  But that's driver-dependent.

> If so, I don't like this.

No worries, desc_to_gpio() and gpiod_to_irq() are only evaluated when
the debug output is wanted.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: aggregator: Add interrupt support

[Viresh Kumar]

On 04-10-21, 14:44, Geert Uytterhoeven wrote:
> Currently the GPIO Aggregator does not support interrupts.  This means
> that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
> and userspace applications using line events do not work.
> 
> Add interrupt support by providing a gpio_chip.to_irq() callback, which
> just calls into the parent GPIO controller.
> 
> Note that this does not implement full interrupt controller (irq_chip)
> support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
> still does not work.

Hi Geert,

Thanks for looking into this. I am not sure of the difference it makes
with and without full irq-chip, but lemme explain the use case that we
are concerned about with virtio.

Eventually the interrupt should be visible to userspace, with
something like libgpiod. Which can then send the information over
virtio to the guest.

Will the interrupts be visible in userspace with your patch ?

-- 
viresh

Re: [PATCH] gpio: aggregator: Add interrupt support

[Geert Uytterhoeven]

Hi Viresh,

On Tue, Oct 5, 2021 at 7:50 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 04-10-21, 14:44, Geert Uytterhoeven wrote:
> > Currently the GPIO Aggregator does not support interrupts.  This means
> > that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
> > and userspace applications using line events do not work.
> >
> > Add interrupt support by providing a gpio_chip.to_irq() callback, which
> > just calls into the parent GPIO controller.
> >
> > Note that this does not implement full interrupt controller (irq_chip)
> > support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
> > still does not work.
>
> Thanks for looking into this. I am not sure of the difference it makes
> with and without full irq-chip, but lemme explain the use case that we
> are concerned about with virtio.
>
> Eventually the interrupt should be visible to userspace, with
> something like libgpiod. Which can then send the information over
> virtio to the guest.

Exactly, that was what I had in mind, too.

> Will the interrupts be visible in userspace with your patch ?

Yes they are.
Before, gpiomon (test app from libgpiod) didn't work, now it does.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: aggregator: Add interrupt support

[kamel.bouhara]

Le 2021-10-04 14:44, Geert Uytterhoeven a écrit :

Hello,

What is the status for this patch, is there any remaining
changes to be made ?

Kind regards,
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

> Currently the GPIO Aggregator does not support interrupts.  This means
> that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
> and userspace applications using line events do not work.
> 
> Add interrupt support by providing a gpio_chip.to_irq() callback, which
> just calls into the parent GPIO controller.
> 
> Note that this does not implement full interrupt controller (irq_chip)
> support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
> still does not work.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> I would prefer to avoid implementing irq_chip support, until there is a
> real use case for this.
> 
> This has been tested with gpio-keys and gpiomon on the Koelsch
> development board:
> 
>   - gpio-keys, using a DT overlay[1]:
> 
> 	$ overlay add r8a7791-koelsch-keyboard-controlled-led
> 	$ echo gpio-aggregator > 
> /sys/devices/platform/frobnicator/driver_override
> 	$ echo frobnicator > /sys/bus/platform/drivers/gpio-aggregator/bind
> 
> 	$ gpioinfo frobnicator
> 	gpiochip12 - 3 lines:
> 		line   0:      "light"      "light"  output  active-high [used]
> 		line   1:         "on"         "On"   input   active-low [used]
> 		line   2:        "off"        "Off"   input   active-low [used]
> 
> 	$ echo 255 > /sys/class/leds/light/brightness
> 	$ echo 0 > /sys/class/leds/light/brightness
> 
> 	$ evtest /dev/input/event0
> 
>   - gpiomon, using the GPIO sysfs API:
> 
> 	$ echo keyboard > /sys/bus/platform/drivers/gpio-keys/unbind
> 	$ echo e6055800.gpio 2,6 > 
> /sys/bus/platform/drivers/gpio-aggregator/new_device
> 	$ gpiomon gpiochip12 0 1
> 
> [1] "ARM: dts: koelsch: Add overlay for keyboard-controlled LED"
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays&id=c78d817869e63a3485bb4ab98aeea6ce368a396e
> ---
>  drivers/gpio/gpio-aggregator.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-aggregator.c 
> b/drivers/gpio/gpio-aggregator.c
> index 34e35b64dcdc0581..2ff867d2a3630d3b 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -374,6 +374,13 @@ static int gpio_fwd_set_config(struct gpio_chip
> *chip, unsigned int offset,
>  	return gpiod_set_config(fwd->descs[offset], config);
>  }
> 
> +static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int 
> offset)
> +{
> +	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> +	return gpiod_to_irq(fwd->descs[offset]);
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -414,7 +421,8 @@ static struct gpiochip_fwd
> *gpiochip_fwd_create(struct device *dev,
>  	for (i = 0; i < ngpios; i++) {
>  		struct gpio_chip *parent = gpiod_to_chip(descs[i]);
> 
> -		dev_dbg(dev, "%u => gpio-%d\n", i, desc_to_gpio(descs[i]));
> +		dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> +			desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
> 
>  		if (gpiod_cansleep(descs[i]))
>  			chip->can_sleep = true;
> @@ -432,6 +440,7 @@ static struct gpiochip_fwd
> *gpiochip_fwd_create(struct device *dev,
>  	chip->get_multiple = gpio_fwd_get_multiple_locked;
>  	chip->set = gpio_fwd_set;
>  	chip->set_multiple = gpio_fwd_set_multiple_locked;
> +	chip->to_irq = gpio_fwd_to_irq;
>  	chip->base = -1;
>  	chip->ngpio = ngpios;
>  	fwd->descs = descs;

Re: [PATCH] gpio: aggregator: Add interrupt support

[Geert Uytterhoeven]

Hi Kamel,

On Thu, Apr 13, 2023 at 9:48 AM <kamel.bouhara@bootlin.com> wrote:
> Le 2021-10-04 14:44, Geert Uytterhoeven a écrit :
> What is the status for this patch, is there any remaining
> changes to be made ?

You mean commit a00128dfc8fc0cc8 ("gpio: aggregator: Add interrupt
support") in v5.17?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: aggregator: Add interrupt support

[Kamel Bouhara]

Le Thu, Apr 13, 2023 at 10:54:34AM +0200, Geert Uytterhoeven a écrit :
> Hi Kamel,
>

Hello Geert,

> On Thu, Apr 13, 2023 at 9:48 AM <kamel.bouhara@bootlin.com> wrote:
> > Le 2021-10-04 14:44, Geert Uytterhoeven a écrit :
> > What is the status for this patch, is there any remaining
> > changes to be made ?
>
> You mean commit a00128dfc8fc0cc8 ("gpio: aggregator: Add interrupt
> support") in v5.17?
>

Sorry, I was checking on a v5.15 :) and based on the thread, I tough it
was still not applied !

Thanks.

Cheers,

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com