Re: [PATCH 3/5] iio: accel: mma9551: Add support to get irqs directly from fwnode

[Alexandru Ardelean <aardelean@deviqon.com>]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> The driver previous supported using GPIO requests to retrieve
> multiple interrupt lines.  As existing firmware may be using
> this method, we need to continue to support it.  However, that doesn't
> stop us also supporting just getting irqs directly.
>
> The handling of irqflags has to take into account the fact that using
> a GPIO method to identify the interrupt does not convey direction of
> the trigger that fwnode_irq_get() will. So we need to set the
> IRQF_TRIGGER_RISING in that path but not otherwise, where it will
> cause an issue if we reprobe the driver after removal.
>

I'm not too experienced here [with this GPIO/IRQ API] to be able to
review this confidently.

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9551.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 1b4a8b27f14a..a0bb4ccdbec7 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -406,30 +406,37 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev)
>         int i, ret;
>         struct mma9551_data *data = iio_priv(indio_dev);
>         struct device *dev = &data->client->dev;
> +       unsigned long irqflags = IRQF_ONESHOT;
>
>         for (i = 0; i < MMA9551_GPIO_COUNT; i++) {
> -               gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> -               if (IS_ERR(gpio)) {
> -                       dev_err(dev, "acpi gpio get index failed\n");
> -                       return PTR_ERR(gpio);
> -               }
> -
> -               ret = gpiod_to_irq(gpio);
> -               if (ret < 0)
> +               /* GPIO provided for backwards compatibility reasons */
> +               ret = fwnode_irq_get(dev_fwnode(dev), i);
> +               if (ret == -EPROBE_DEFER)
>                         return ret;
>
> +               /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
> +               if (ret == 0 || ret == -EINVAL) {
> +                       gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> +                       if (IS_ERR(gpio)) {
> +                               dev_err(dev, "gpio get index failed\n");
> +                               return PTR_ERR(gpio);
> +                       }
> +
> +                       ret = gpiod_to_irq(gpio);
> +                       if (ret < 0)
> +                               return ret;
> +                       /* GPIO interrupt does npt have a specified direction */
> +                       irqflags |= IRQF_TRIGGER_RISING;
> +               }
>                 data->irqs[i] = ret;
>                 ret = devm_request_threaded_irq(dev, data->irqs[i],
> -                               NULL, mma9551_event_handler,
> -                               IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> -                               MMA9551_IRQ_NAME, indio_dev);
> +                                               NULL, mma9551_event_handler,
> +                                               irqflags,
> +                                               MMA9551_IRQ_NAME, indio_dev);
>                 if (ret < 0) {
>                         dev_err(dev, "request irq %d failed\n", data->irqs[i]);
>                         return ret;
>                 }
> -
> -               dev_dbg(dev, "gpio resource, no:%d irq:%d\n",
> -                       desc_to_gpio(gpio), data->irqs[i]);
>         }
>
>         return 0;
> --
> 2.31.1
>