Re: [PATCH v2 01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Fri, 2022-07-15 at 20:59 +0200, Andy Shevchenko wrote:
> On Fri, Jul 15, 2022 at 2:50 PM 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.
> 
> ...
> 
> > +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];
> 
> There is a helper to retrieve hwirq from the IRQ chip data.
> 

Will look into that...

> > +       kpad->irq_mask[ADP5588_BANK(real_irq)] &=
> > ~ADP5588_BIT(real_irq);
> > +       gpiochip_disable_irq(gc, d->hwirq);
> > +}
> 
> ...
> 
> > +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];
> 
> Ditto.
> 
> > +       gpiochip_enable_irq(gc, d->hwirq);
> > +       kpad->irq_mask[ADP5588_BANK(real_irq)] |=
> > ADP5588_BIT(real_irq);
> > +}
> 
> ...
> 
> > +static int adp5588_gpiomap_get_hwirq(struct device *dev, const u8
> > *map,
> > +                                    unsigned int gpio, unsigned
> > int ngpios)
> >  {
> 
> > +       unsigned int hwirq;
> > +
> > +       for (hwirq = 0; hwirq < ngpios; hwirq++)
> > +               if (map[hwirq] == gpio)
> > +                       return hwirq;
> 
> I'm sorry if I already asked, but can irq_valid_mask be helpful here?
> 

Yes you did and I did checked it but never replied... So, IIUC, the
'init_valid_mask()' (I think this is the one you are referring too)
receives a bitmap that goes from 0 ... ngpios - 1 where I would set
some bits to 0 if I that line cannot fire interrupts. This is not the
case in here since all lines exported can fire interrupts. This mapping
is something else. Users might not want to have, let's say, pins 10, 15
and 17 (device pins) as part of the keymap matrix so that they are
exported as gpios than can be, optionally, "consumed" for something
else. In this case, the mapping is:

gpio line	device pin

  0		   10
  1		   15
  2		   17

This map was already on the original driver and I don't really intend
to touch it in this series (if ever).

> > +       /* should never happen */
> > +       dev_warn_ratelimited(dev, "could not find the hwirq for
> > gpio(%u)\n", gpio);
> > +
> > +       return -ENOENT;
> > +}
> 
> ...
> 
> > +       int hwirq;
> > +
> > +       hwirq = adp5588_gpiomap_get_hwirq(&client->dev, kpad-
> > >gpiomap,
> > +                                         gpio, kpad->gc.ngpio);
> > +       if (hwirq < 0) {
> > +               dev_err(&client->dev, "Could not get hwirq for
> > key(%u)\n", key_val);
> > +               return;
> > +       }
> 
> Instead of having it signed, can you use INVALID_HWIRQ definition?
> 

Can do that...

> ...
> 
> > +       irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
> > +       if (irq <= 0)
> 
> '<' ? How is it possible?
> 

Well, yes... not really possible. Will change it.


> > +               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);
> 
> 'desc' is quite a confusing name for IRQ chip data! Please rename (we
> have IRQ descriptor and it's not the IRQ chip data).
> 

Yeah, make sense.

- Nuno Sá