Re: [Stratos-dev] [PATCH resend] gpio: aggregator: Add interrupt support

On Fri, Dec 3, 2021 at 3:56 PM Bartosz Golaszewski brgl@bgdev.pl wrote:

On Fri, Dec 3, 2021 at 3:06 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.

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 e9671d1660ef4b40..869dc952cf45218b 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -371,6 +371,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

@@ -411,7 +418,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;
  

@@ -429,6 +437,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

Applied, thanks!

Bart

Eek, missed the explanation.

I think this is fine. For user-space eventual switch to a real irq_chip won't break anything so let's take it.

Bart