> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Friday, July 8, 2022 4:18 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: devicetree <devicetree@xxxxxxxxxxxxxxx>; open list:GPIO
> SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; linux-input <linux-
> input@xxxxxxxxxxxxxxx>; Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx>; Bartosz Golaszewski
> <brgl@xxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>
> Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi
> key events as 'gpio keys'
>
> [External]
>
> 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).
>
I kind of replied to this in a previous email (to you). It naturally can,
but I would rather prefer to do that in another series... I could have
done it here but it would not be consistent with the rest of the
driver.
> ...
>
> > +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://urldefense.com/v3/__https://www.kernel.org/doc/html/lates
> t/driver-api/gpio/driver.html*infrastructure-helpers-for-gpio-
> irqchips__;Iw!!A3Ni8CS0y2Y!4eMLD5XT2REpOPWl6B9IxYxEgbMfxiW87
> XJu-4KmjeVywSLrobIZqEZpcVJ0dBNbZDGPBpHXvx3xP-HzGS4jIwvu$
>
> ...
>
> > + 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.
>
Well the of_node was set so that I had a proper name in the IRQ domain
IIRC. Will this be handled in the GPIO lib in the future?
The parent assignment was also to make things neater in
/sys/kernel/debug/gpio.
> ...
>
> > + 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.
I see... I will look at Docs. In practice I don't think this matters much
since this handler should never really be called (I think) as we just
call handle_nested_irq().
> ...
>
> > +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?
because I do not trust the HW to always cooperate :). In theory,
we can get some invalid 'gpio' from it.
> > + WARN_ON_ONCE(hwirq == ngpios);
> > +
> > + return -ENOENT;
> > +}
>
> I don't know this code, can you summarize why this additional mapping
> is needed?
>
You have 18 possible pins to use as GPIOs (and hence be IRQ sources). Now,
if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. But
what we get from the device in 'key_val - GPI_PIN_BASE' is, for example,
16 and so we need to get the hwirq which will be 0. It's pretty much the
reverse of what it's being done in the GPIOs callbacks.
> ...
>
> > +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?
We get here if we get a key press (falling edge) or a key release (rising
edge). The events are given by the device and it might be that in some
cases we just want to handle/propagate key presses
(not sure if it makes sense). So we need to match it with the
appropriate irq_type requested. Note that this kind of controlling the IRQ
from SW as there's no way from doing it in the device. That is why we don't
do more than just making sure the IRQ types are valid in irq_set_type.
> > + 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.
>
Hmm, I think I saw it but somehow I though I could not use it (because
of the previous calls to get the irq_type). Hmmm...
- Nuno Sá
