Re: [PATCH v3 3/4] iio: light: ltr390: Interrupts and threshold event support

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 14 Sep 2024 23:42:45 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Added support for threshold events for both the ALS and UVI channels.
> The events are reported when the threshold interrupt is triggered. Both
> rising and falling threshold types are supported.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
I made a few more tweaks whilst picking this up.
Please take a look at the result.

Jonathan

> ---
>  drivers/iio/light/ltr390.c | 212 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 212 insertions(+)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index e152009b5..57bf48595 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -25,8 +25,11 @@
>  #include <linux/regmap.h>
>  #include <linux/bitfield.h>
>  #include <linux/device.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
I moved these as well + cheeky move of the bitfield.h in here
as that was beginning to annoy me.

>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/events.h>

> +
>  static int ltr390_probe(struct i2c_client *client)
>  {
>  	struct ltr390_data *data;
> @@ -428,6 +627,19 @@ static int ltr390_probe(struct i2c_client *client)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable the sensor\n");
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
we have dev, so I used it.
> +						NULL, ltr390_interrupt_handler,
> +						IRQF_TRIGGER_FALLING |
Dropped this specifying of direction. Direction should be set by firmware not the driver.
We have it done in some older drivers and can't fix that now, but for new
code we should never force it here.

> +						IRQF_ONESHOT,
> +						"ltr390_thresh_event",
> +						indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "request irq (%d) failed\n", client->irq);
return dev_err_probe(dev, ret, ...

in keeping with other local code and as it is generally nicer.
> +			return ret;
> +		}
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>