Added support for getting continuous readings from vl6180 using
triggered buffer approach. The continuous mode can be enabled by
enabling the buffer.
Also added a trigger and appropriate checks to see that it is used
with this device.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 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;
+	} scan;
 };
 
 enum { VL6180_ALS, VL6180_RANGE, VL6180_PROX };
@@ -275,6 +285,12 @@ static const struct iio_chan_spec vl6180_channels[] = {
 	{
 		.type = IIO_LIGHT,
 		.address = VL6180_ALS,
+		.scan_index = VL6180_ALS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+		},
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_INT_TIME) |
 			BIT(IIO_CHAN_INFO_SCALE) |
@@ -283,14 +299,27 @@ static const struct iio_chan_spec vl6180_channels[] = {
 	}, {
 		.type = IIO_DISTANCE,
 		.address = VL6180_RANGE,
+		.scan_index = VL6180_RANGE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 8,
+			.storagebits = 8,
+		},
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE) |
 			BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}, {
 		.type = IIO_PROXIMITY,
 		.address = VL6180_PROX,
+		.scan_index = VL6180_PROX,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+		},
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
 /*
@@ -497,17 +526,99 @@ static irqreturn_t vl6180_threaded_irq(int irq, void *priv)
 	struct iio_dev *indio_dev = priv;
 	struct vl6180_data *data = iio_priv(indio_dev);
 
-	complete(&data->completion);
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll_nested(indio_dev->trig);
+	else
+		complete(&data->completion);
+
+	return IRQ_HANDLED;
+}
+
+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)) {
+
+		ret = vl6180_chan_regs_table[i].word ?
+			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;
+		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+						iio_get_time_ns(indio_dev));
+		}
+	}
+
+	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,
 };
 
-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);
+
+	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++) {
+		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);
+	if (ret)
+		return ret;
+
 	/* Default Range inter-measurement time: 50ms
 	 * reg_val = (50 / 10 - 1) = 4
 	 */
@@ -603,7 +720,7 @@ static int vl6180_probe(struct i2c_client *client)
 	indio_dev->name = VL6180_DRV_NAME;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = vl6180_init(data);
+	ret = vl6180_init(data, indio_dev);
 	if (ret < 0)
 		return ret;
 
@@ -618,6 +735,19 @@ static int vl6180_probe(struct i2c_client *client)
 		}
 
 		init_completion(&data->completion);
+
+		data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+						indio_dev->name, iio_device_id(indio_dev));
+		if (!data->trig)
+			return -ENOMEM;
+
+		data->trig->ops = &vl6180_trigger_ops;
+		iio_trigger_set_drvdata(data->trig, indio_dev);
+		ret = devm_iio_trigger_register(&client->dev, data->trig);
+		if (ret)
+			return ret;
+
+		indio_dev->trig = iio_trigger_get(data->trig);
 	}
 
 	return devm_iio_device_register(&client->dev, indio_dev);
-- 
2.43.0

On Fri,  4 Oct 2024 20:31:48 +0530
Abhash Jha <abhashkumarjha123@gmail.com> 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@gmail.com>
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;

> Hi Abhash,
>
> Some comments below.
>
Hi Jonathan,
I will do the fixes and send a v3.

I have a question though:
The device has a 8 x 16-bit HW-buffer.
I want to implement the HW buffer support. Where in this driver should
I read the hardware buffer?
How is that exposed to userspace? Is it even exposed?
There is no buffer-full interrupt, It just has the latest 16 range
measurements and
latest 8 ALS measurements.

There is also a SYSTEM_HISTORY_CTRL register, which configures the HW buffer,
like setting which data to capture (ALS/RANGE) as well as turning the
buffer on/off.
Where should all this configuration be done?
Should it be default or have some sysfs attribute associated with it?

Thanks,
Abhash

On Sat, 5 Oct 2024 22:42:43 +0530
Abhash jha <abhashkumarjha123@gmail.com> wrote:

> > Hi Abhash,
> >
> > Some comments below.
> >  
> Hi Jonathan,
> I will do the fixes and send a v3.
> 
> I have a question though:
> The device has a 8 x 16-bit HW-buffer.
> I want to implement the HW buffer support. Where in this driver should
> I read the hardware buffer?

If it were a fifo there are lots of examples in tree, but those tend
to 'empty' on read. From your description I'm not sure this one does.

> How is that exposed to userspace? Is it even exposed?
> There is no buffer-full interrupt, It just has the latest 16 range
> measurements and
> latest 8 ALS measurements.

Ah. Can we tell if the data is new vs data we have already read?
From a quick glance looks like you can clear it, so maybe we can use that
though we'll have to be careful about races.  Do we have to stop
continuous mode to clear it? Sort of looks like that's the case from
the description of the clear not occuring until a start_stop write.

However we'll have to dead reckon the timing without an interrupt.
That should be fine, as just configure it to max say 3/4 of the
time to fill it.

> 
> There is also a SYSTEM_HISTORY_CTRL register, which configures the HW buffer,
> like setting which data to capture (ALS/RANGE) as well as turning the
> buffer on/off.
> Where should all this configuration be done?
> Should it be default or have some sysfs attribute associated with it?

So if it were a conventional fifo it would  mostly be hidden behind the
software fifo and just act as an optimization of the data capture.
A few things are exposed though as can make a difference to how you configure
the device such as the size of the hardware fifo and the hwfifo threshold
(affects latency of data capture).  However sounds like you don't have
that here.

So challenging feature to support.
> 
> Thanks,
> Abhash