Re: [PATCH 3/3] iio: light: vl6180: Add support for Continuous Mode

[Jonathan Cameron]

On Fri,  4 Oct 2024 20:31:48 +0530
Abhash Jha <abhashkumarjha123@xxxxxxxxx> wrote:

> Added support for getting continuous readings from vl6180 using
> triggered buffer approach. The continuous mode can be enabled by
> enabling the buffer.
If you want multiple paragraphs, I'd use a blank line between them.
If not, then tighter wrapping makes sense.
> Also added a trigger and appropriate checks to see that it is used
> with this device.
Normally aim for 75 char wrap point for commit descriptions.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@xxxxxxxxx>
Hi Abhash,

Some comments below.

Thanks,

Jonathan

> ---
>  drivers/iio/light/vl6180.c | 138 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 134 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
> index 4c2b486e2..e724e752e 100644
> --- a/drivers/iio/light/vl6180.c
> +++ b/drivers/iio/light/vl6180.c
> @@ -25,6 +25,10 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #define VL6180_DRV_NAME "vl6180"
>  
> @@ -91,10 +95,16 @@ struct vl6180_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
>  	struct completion completion;
> +	struct iio_trigger *trig;
>  	unsigned int als_gain_milli;
>  	unsigned int als_it_ms;
>  	unsigned int als_meas_rate;
>  	unsigned int range_meas_rate;
> +
> +	struct {
> +		u16 chan;
> +		aligned_u64 timestamp;

aligned_s64 as timestamps are (oddly) always signed.


> +	} scan;
>  };


> +
> +static irqreturn_t vl6180_trigger_handler(int irq, void *priv)
> +{
> +	struct iio_poll_func *pf = priv;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct vl6180_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	for (int i = 0; i < indio_dev->masklength; i++) {
> +		if (test_bit(i, indio_dev->active_scan_mask)) {
> +

Indent broken.  + see below for iio_for_each_active_channel()
which is how you should do this.

> +		ret = vl6180_chan_regs_table[i].word ?
		if (vl6180_chan_regs_table[i].word)
			ret = vl6180...
		else
			ret = v...

Preferred. The ternary is hard to read with such long legs.

> +			vl6180_read_word(data->client, vl6180_chan_regs_table[i].value_reg) :
> +			vl6180_read_byte(data->client, vl6180_chan_regs_table[i].value_reg);
> +		if (ret < 0)
> +			dev_err(&data->client->dev, "failed to read from value regs: %d\n", ret);
> +
> +		data->scan.chan = ret;

Only one bit set?  otherwise this overwrites the same channel each time.

> +		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +						iio_get_time_ns(indio_dev));

This is response to a trigger interrupt - so I'd guess the reading was earlier?
Better to grab a copy of current time nearer that point.

> +		}
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	/* Clear the interrupt flag after data read */
> +	ret = vl6180_write_byte(data->client, VL6180_INTR_CLEAR,
> +		VL6180_CLEAR_ERROR | VL6180_CLEAR_ALS | VL6180_CLEAR_RANGE);
> +	if (ret < 0)
> +		dev_err(&data->client->dev, "failed to clear irq: %d\n", ret);
> +
>  	return IRQ_HANDLED;
>  }
>  
> +static int vl6180_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig)
> +{
> +	struct vl6180_data *data = iio_priv(indio_dev);
> +
> +	return data->trig == trig ? 0 : -EINVAL;
> +}
> +
>  static const struct iio_info vl6180_info = {
>  	.read_raw = vl6180_read_raw,
>  	.write_raw = vl6180_write_raw,
>  	.attrs = &vl6180_attribute_group,
> +	.validate_trigger = vl6180_validate_trigger,

There is a helper for common case of the trigger parent is same as device
(very similar to the one you use below).  That should be enough here
as no other trigger will have that device as parent.

>  };
>  
> -static int vl6180_init(struct vl6180_data *data)
> +static int vl6180_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct vl6180_data *data = iio_priv(indio_dev);
> +
iio_for_each_active_channel()

Note that if you build this on current tree it will give a
compiler error as we enforce not directly accessing the mask_length
field.


> +	for (int i = 0; i < indio_dev->masklength; i++) {
> +		if (test_bit(i, indio_dev->active_scan_mask))
> +			return vl6180_write_byte(data->client,
> +				vl6180_chan_regs_table[i].start_reg,
> +				VL6180_MODE_CONT | VL6180_STARTSTOP);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int vl6180_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct vl6180_data *data = iio_priv(indio_dev);
> +
> +	for (int i = 0; i < indio_dev->masklength; i++) {
iio_for_each_active_channel()
> +		if (test_bit(i, indio_dev->active_scan_mask))
> +			return vl6180_write_byte(data->client,
> +				vl6180_chan_regs_table[i].start_reg,
> +				VL6180_STARTSTOP);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> +	.postenable = &vl6180_buffer_postenable,
> +	.postdisable = &vl6180_buffer_postdisable,
> +};
> +
> +static const struct iio_trigger_ops vl6180_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int vl6180_init(struct vl6180_data *data, struct iio_dev *indio_dev)
>  {
>  	struct i2c_client *client = data->client;
>  	int ret;
> @@ -547,6 +658,12 @@ static int vl6180_init(struct vl6180_data *data)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +						&vl6180_trigger_handler,
> +						&iio_triggered_buffer_setup_ops);

Spacing looks wrong.  Align these last two lines with the & in the first one.

> +	if (ret)
> +		return ret;