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

[Jonathan Cameron]

On Tue, 10 Sep 2024 10:20:28 +0530
Abhash Jha <abhashkumarjha123@xxxxxxxxx> 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@xxxxxxxxx>
Hi Abhash,

A few comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/light/ltr390.c | 222 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 220 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index c4ff26d68..8a8a118ca 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -24,8 +24,11 @@

>  
> @@ -370,12 +398,186 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
>  	}
>  }
>  
> +static int ltr390_read_threshold(const struct iio_dev *indio_dev,
> +				enum iio_event_direction dir,

Alignment doesn't look quite right.  Is it off one space?
That is, e should be below c.  Hard to tell this in a diff so I
maybe wrong about it being off.


> +				int *val, int *val2)
> +{
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = ltr390_register_read(data, LTR390_THRESH_UP);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_EV_DIR_FALLING:
> +		ret = ltr390_register_read(data, LTR390_THRESH_LOW);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltr390_write_threshold(struct iio_dev *indio_dev,
> +				enum iio_event_direction dir,
> +				int val, int val2)
> +{
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = regmap_bulk_write(data->regmap,
> +					LTR390_THRESH_UP,
Wrap this bit less to get nearer 80 chars.
> +					&val, 3);
> +		return ret;
> +	case IIO_EV_DIR_FALLING:
> +		ret = regmap_bulk_write(data->regmap,
> +					LTR390_THRESH_LOW,
> +					&val, 3);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int ltr390_read_event_config(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir)
> +{
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret, status;
> +
> +	ret = regmap_read(data->regmap, LTR390_INT_CFG, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	return status & LTR390_LS_INT_EN;

Slight preference for FIELD_GET() here which will make this 0 or 1
rather than 0 or 4 (I think?)


> +}
> +


> +static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	int ret, status;
> +
> +	/* Reading the status register to clear the interrupt flag, Datasheet pg: 17*/
> +	ret = regmap_read(data->regmap, LTR390_MAIN_STATUS, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (data->mode) {
> +	case LTR390_SET_ALS_MODE:
> +		iio_push_event(indio_dev,
> +			IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +			IIO_EV_TYPE_THRESH,
Align after (

> +			IIO_EV_DIR_EITHER),
> +			iio_get_time_ns(indio_dev));
> +		break;
> +
> +	case LTR390_SET_UVS_MODE:
> +		iio_push_event(indio_dev,
> +			IIO_UNMOD_EVENT_CODE(IIO_UVINDEX, 0,

Same on alignment as this is hard to read.

> +			IIO_EV_TYPE_THRESH,
> +			IIO_EV_DIR_EITHER),
> +			iio_get_time_ns(indio_dev));
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int ltr390_probe(struct i2c_client *client)
>  {
>  	struct ltr390_data *data;
> @@ -429,6 +631,22 @@ 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) {
> +		int irq_flags = irq_get_trigger_type(client->irq);
> +
> +		if (!irq_flags)
> +			irq_flags = IRQF_TRIGGER_FALLING;
Don't override the firmware description. It should be correct for new drives
as probably no legacy out there. The only time we play this sort of
game is when we have to carry on supporting a wrong configuration that
'works' because the driver was setting more than it should in the past.

> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, ltr390_interrupt_handler,
> +						irq_flags | IRQF_ONESHOT,

Drop irq_flags use here.

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