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
