On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá [off-list ref] wrote:

This change replaces the support for GPIs as key event generators.
Instead of reporting the events directly, we add a gpio based irqchip
so that these events can be consumed by keys defined in the gpio-keys
driver (as it's goal is indeed for keys on GPIOs capable of generating
interrupts). With this, the gpio-adp5588 driver can also be dropped.

The basic idea is that all the pins that are not being used as part of
the keymap matrix can be possibly requested as GPIOs by gpio-keys
(it's also fine to use these pins as plain interrupts though that's not
really the point).

Since the gpiochip now also has irqchip capabilities, we should only
remove it after we free the device interrupt (otherwise we could, in
theory, be handling GPIs interrupts while the gpiochip is concurrently
removed). Thus the call 'adp5588_gpio_add()' is moved and since the
setup phase also needs to come before making the gpios visible, we also
need to move 'adp5588_setup()'.

While at it, always select GPIOLIB so that we don't need to use #ifdef
guards.

...

        u8 dat_out[3];
        u8 dir[3];

+       u8 int_en[3];
+       u8 irq_mask[3];

Wondering why these can't be converted to natural bitmaps. (See
gpio-pca953x as an example).

...

+static void adp5588_irq_mask(struct irq_data *d)
+{
+       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+       unsigned long real_irq = kpad->gpiomap[d->hwirq];
+
+       kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq);
+}
+
+static void adp5588_irq_unmask(struct irq_data *d)
+{
+       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+       unsigned long real_irq = kpad->gpiomap[d->hwirq];
+
+       kpad->irq_mask[ADP5588_BANK(real_irq)] |= ADP5588_BIT(real_irq);
+}

Please follow the suitable example from here:
https://www.kernel.org/doc/html/latest/driver-api/gpio/driver.html#infrastructure-helpers-for-gpio-irqchips

...

+       kpad->gc.parent = &kpad->client->dev;

+       kpad->gc.of_node = kpad->client->dev.of_node;

We are going to remove of_node from GPIO. Moreover the parent device
and its node is a duplication, just drop the latter and GPIO library
will take care of it.

...

+       irq_chip->name = "adp5588";
+       irq_chip->irq_mask = adp5588_irq_mask;
+       irq_chip->irq_unmask = adp5588_irq_unmask;
+       irq_chip->irq_bus_lock = adp5588_irq_bus_lock;
+       irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock;
+       irq_chip->irq_set_type = adp5588_irq_set_type;
+       irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
+       girq = &kpad->gc.irq;
+       girq->chip = irq_chip;

+       girq->handler = handle_simple_irq;

By default it should be handle_bad_irq() and locked in the ->irq_set_type().

+       girq->threaded = true;

See documentation above.

...

+static int adp5588_gpiomap_get_hwirq(const u8 *map, unsigned int gpio,
+                                    unsigned int ngpios)
 {
-       return 0;
+       unsigned int hwirq;
+
+       for (hwirq = 0; hwirq < ngpios; hwirq++)
+               if (map[hwirq] == gpio)
+                       return hwirq;

+       /* should never happen */

Then why it's here?

+       WARN_ON_ONCE(hwirq == ngpios);
+
+       return -ENOENT;
+}

I don't know this code, can you summarize why this additional mapping is needed?

...

+static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad, int key_val,
+                                   int key_press)
+{
+       unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type, hwirq;
+       struct i2c_client *client = kpad->client;
+       struct irq_data *desc;
+
+       hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio);
+       if (hwirq < 0) {
+               dev_err(&client->dev, "Could not get hwirq for key(%u)\n", key_val);
+               return;
+       }
+
+       irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
+       if (irq <= 0)
+               return;
+
+       desc = irq_get_irq_data(irq);
+       if (!desc) {
+               dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
+               return;
+       }
+
+       irq_type = irqd_get_trigger_type(desc);
+
+       /*
+        * Default is active low which means key_press is asserted on
+        * the falling edge.
+        */
+       if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
+           (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))

This is dup from ->irq_set_type(). Or how this can be not like this?

+               handle_nested_irq(irq);

There is new helpers I believe for getting domain and handle an IRQ.
Grep for the recent (one-two last cycles) changes in the GPIO drivers.

 }

-- 
With Best Regards,
Andy Shevchenko