On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@xxxxxxxxxx> 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
