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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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.

> +       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?

> +       /* 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?

...

> +       irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
> +       if (irq <= 0)

'<' ? How is it possible?

> +               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).

-- 
With Best Regards,
Andy Shevchenko