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/co... --- 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;
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.
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
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 ?
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
stratos-dev@op-lists.linaro.org