[PATCH 0/2] Continuous mode support for VL53l0x

[PATCH 0/2] Continuous mode support for VL53l0x

[Abhash Jha]

Hello,

The first patch adds support for checking the sensor ID by reading
MODEL_IDENTIFICATION register and seeing if it returns the value 0xEE

The second patch adds support for continuous mode in the sensor by using
buffers. We enable the sensor's continuous mode in the buffer_postenable
function.
Replaced the irq handler with a threaded irq handler in order to perform
I2C reads for the data. The continuous mode can be disabled by disabling
the buffer.

Regards,
Abhash

Abhash Jha (2):
  iio: proximity: vl53l0x-i2c: Added sensor ID check
  iio: proximity: vl53l0x-i2c: Added continuous mode support

 drivers/iio/proximity/vl53l0x-i2c.c | 136 ++++++++++++++++++++++------
 1 file changed, 110 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH 1/2] iio: proximity: vl53l0x-i2c: Added sensor ID check

[Abhash Jha]

The commit adds a check for the sensor's model ID. We read the model
identification register (0xC0) and expect a value of 0xEE.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/proximity/vl53l0x-i2c.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
index 8d4f3f849..2b3dd18be 100644
--- a/drivers/iio/proximity/vl53l0x-i2c.c
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -39,8 +39,11 @@
 
 #define VL_REG_RESULT_INT_STATUS			0x13
 #define VL_REG_RESULT_RANGE_STATUS			0x14
+#define VL_REG_IDENTIFICATION_MODEL_ID			0xC0
 #define VL_REG_RESULT_RANGE_STATUS_COMPLETE		BIT(0)
 
+#define VL53L0X_MODEL_ID_VAL				0xEE
+
 struct vl53l0x_data {
 	struct i2c_client *client;
 	struct completion completion;
@@ -223,6 +226,7 @@ static int vl53l0x_probe(struct i2c_client *client)
 	struct vl53l0x_data *data;
 	struct iio_dev *indio_dev;
 	int error;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -237,6 +241,11 @@ static int vl53l0x_probe(struct i2c_client *client)
 				     I2C_FUNC_SMBUS_BYTE_DATA))
 		return -EOPNOTSUPP;
 
+	ret = i2c_smbus_read_byte_data(data->client, VL_REG_IDENTIFICATION_MODEL_ID);
+	if (ret != VL53L0X_MODEL_ID_VAL)
+		return dev_err_probe(&client->dev, ret,
+				     "Received invalid model id: 0x%x", ret);
+
 	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
 	if (IS_ERR(data->vdd_supply))
 		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
@@ -265,8 +274,6 @@ static int vl53l0x_probe(struct i2c_client *client)
 
 	/* usage of interrupt is optional */
 	if (client->irq) {
-		int ret;
-
 		init_completion(&data->completion);
 
 		ret = vl53l0x_configure_irq(client, indio_dev);
-- 
2.43.0

Re: [PATCH 1/2] iio: proximity: vl53l0x-i2c: Added sensor ID check

[Jonathan Cameron]

On Sat, 31 Aug 2024 01:46:25 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> The commit adds a check for the sensor's model ID. We read the model
> identification register (0xC0) and expect a value of 0xEE.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
>  drivers/iio/proximity/vl53l0x-i2c.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> index 8d4f3f849..2b3dd18be 100644
> --- a/drivers/iio/proximity/vl53l0x-i2c.c
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -39,8 +39,11 @@
>  
>  #define VL_REG_RESULT_INT_STATUS			0x13
>  #define VL_REG_RESULT_RANGE_STATUS			0x14
> +#define VL_REG_IDENTIFICATION_MODEL_ID			0xC0
>  #define VL_REG_RESULT_RANGE_STATUS_COMPLETE		BIT(0)
>  
> +#define VL53L0X_MODEL_ID_VAL				0xEE
> +
>  struct vl53l0x_data {
>  	struct i2c_client *client;
>  	struct completion completion;
> @@ -223,6 +226,7 @@ static int vl53l0x_probe(struct i2c_client *client)
>  	struct vl53l0x_data *data;
>  	struct iio_dev *indio_dev;
>  	int error;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -237,6 +241,11 @@ static int vl53l0x_probe(struct i2c_client *client)
>  				     I2C_FUNC_SMBUS_BYTE_DATA))
>  		return -EOPNOTSUPP;
>  
> +	ret = i2c_smbus_read_byte_data(data->client, VL_REG_IDENTIFICATION_MODEL_ID);
> +	if (ret != VL53L0X_MODEL_ID_VAL)
> +		return dev_err_probe(&client->dev, ret,
This first ret should be the error return you want.  So -EINVAL or something like that.

> +				     "Received invalid model id: 0x%x", ret);

This needs to be message only, not an error return.

If we return an error here we break any future use of fallback device tree compatibles
for future devices running with an old kernel. 

So the most we should do is print a message that observes we don't recognise
the device, then carry on anyway.

We used to get this wrong in IIO and haven't yet fixed all drivers, but
I definitely don't want add such hard failures to more drivers.


> +
>  	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
>  	if (IS_ERR(data->vdd_supply))
>  		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> @@ -265,8 +274,6 @@ static int vl53l0x_probe(struct i2c_client *client)
>  
>  	/* usage of interrupt is optional */
>  	if (client->irq) {
> -		int ret;
> -
>  		init_completion(&data->completion);
>  
>  		ret = vl53l0x_configure_irq(client, indio_dev);

[PATCH 2/2] iio: proximity: vl53l0x-i2c: Added continuous mode support

[Abhash Jha]

The continuous mode of the sensor is enabled in the buffer_postenable.
Replaced the original irq handler with a threaded irq handler to perform
i2c reads during continuous mode.
The continuous mode is disabled by disabling the buffer.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/proximity/vl53l0x-i2c.c | 125 ++++++++++++++++++++++------
 1 file changed, 101 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
index 2b3dd18be..b0c947586 100644
--- a/drivers/iio/proximity/vl53l0x-i2c.c
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -22,6 +22,10 @@
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #define VL_REG_SYSRANGE_START				0x00
 
@@ -49,14 +53,58 @@ struct vl53l0x_data {
 	struct completion completion;
 	struct regulator *vdd_supply;
 	struct gpio_desc *reset_gpio;
+
+	struct {
+		u16 chan;
+		s64 timestamp __aligned(8);
+	} scan;
 };
 
-static irqreturn_t vl53l0x_handle_irq(int irq, void *priv)
+static void vl53l0x_clear_irq(struct vl53l0x_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					VL_REG_SYSTEM_INTERRUPT_CLEAR, 1);
+	if (ret < 0)
+		dev_err(dev, "failed to clear error irq: %d\n", ret);
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					VL_REG_SYSTEM_INTERRUPT_CLEAR, 0);
+	if (ret < 0)
+		dev_err(dev, "failed to clear range irq: %d\n", ret);
+
+	ret = i2c_smbus_read_byte_data(data->client, VL_REG_RESULT_INT_STATUS);
+	if (ret < 0 || ret & 0x07)
+		dev_err(dev, "failed to clear irq: %d\n", ret);
+}
+
+static irqreturn_t vl53l0x_threaded_irq(int irq, void *priv)
 {
 	struct iio_dev *indio_dev = priv;
 	struct vl53l0x_data *data = iio_priv(indio_dev);
+	u8 buffer[12];
+	int ret;
 
-	complete(&data->completion);
+	if (iio_buffer_enabled(indio_dev)) {
+		ret = i2c_smbus_read_i2c_block_data(data->client,
+						VL_REG_RESULT_RANGE_STATUS,
+						sizeof(buffer), buffer);
+		if (ret < 0)
+			return ret;
+		else if (ret != 12)
+			return -EREMOTEIO;
+
+		data->scan.chan = (buffer[10] << 8) + buffer[11];
+		iio_push_to_buffers_with_timestamp(indio_dev,
+						&data->scan,
+						iio_get_time_ns(indio_dev));
+
+		iio_trigger_notify_done(indio_dev->trig);
+		vl53l0x_clear_irq(data);
+	} else
+		complete(&data->completion);
 
 	return IRQ_HANDLED;
 }
@@ -71,8 +119,9 @@ static int vl53l0x_configure_irq(struct i2c_client *client,
 	if (!irq_flags)
 		irq_flags = IRQF_TRIGGER_FALLING;
 
-	ret = devm_request_irq(&client->dev, client->irq, vl53l0x_handle_irq,
-			irq_flags, indio_dev->name, indio_dev);
+	ret = devm_request_threaded_irq(&client->dev, client->irq,
+			NULL, vl53l0x_threaded_irq,
+			irq_flags | IRQF_ONESHOT, indio_dev->name, indio_dev);
 	if (ret) {
 		dev_err(&client->dev, "devm_request_irq error: %d\n", ret);
 		return ret;
@@ -87,26 +136,6 @@ static int vl53l0x_configure_irq(struct i2c_client *client,
 	return ret;
 }
 
-static void vl53l0x_clear_irq(struct vl53l0x_data *data)
-{
-	struct device *dev = &data->client->dev;
-	int ret;
-
-	ret = i2c_smbus_write_byte_data(data->client,
-					VL_REG_SYSTEM_INTERRUPT_CLEAR, 1);
-	if (ret < 0)
-		dev_err(dev, "failed to clear error irq: %d\n", ret);
-
-	ret = i2c_smbus_write_byte_data(data->client,
-					VL_REG_SYSTEM_INTERRUPT_CLEAR, 0);
-	if (ret < 0)
-		dev_err(dev, "failed to clear range irq: %d\n", ret);
-
-	ret = i2c_smbus_read_byte_data(data->client, VL_REG_RESULT_INT_STATUS);
-	if (ret < 0 || ret & 0x07)
-		dev_err(dev, "failed to clear irq: %d\n", ret);
-}
-
 static int vl53l0x_read_proximity(struct vl53l0x_data *data,
 				  const struct iio_chan_spec *chan,
 				  int *val)
@@ -163,7 +192,14 @@ static const struct iio_chan_spec vl53l0x_channels[] = {
 		.type = IIO_DISTANCE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(32),
 };
 
 static int vl53l0x_read_raw(struct iio_dev *indio_dev,
@@ -221,6 +257,40 @@ static int vl53l0x_power_on(struct vl53l0x_data *data)
 	return 0;
 }
 
+static int vl53l0x_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct vl53l0x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, VL_REG_SYSRANGE_START, 0x02);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int vl53l0x_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct vl53l0x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, VL_REG_SYSRANGE_START, 0x01);
+	if (ret < 0)
+		return ret;
+
+	/* Let the ongoing reading finish */
+	reinit_completion(&data->completion);
+	wait_for_completion_timeout(&data->completion, HZ/10);
+	vl53l0x_clear_irq(data);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
+	.postenable = &vl53l0x_buffer_postenable,
+	.postdisable = &vl53l0x_buffer_postdisable,
+};
+
 static int vl53l0x_probe(struct i2c_client *client)
 {
 	struct vl53l0x_data *data;
@@ -279,6 +349,13 @@ static int vl53l0x_probe(struct i2c_client *client)
 		ret = vl53l0x_configure_irq(client, indio_dev);
 		if (ret)
 			return ret;
+
+		ret = devm_iio_triggered_buffer_setup(&client->dev,
+					indio_dev,
+					&iio_pollfunc_store_time,
+					NULL, &iio_triggered_buffer_setup_ops);
+		if (ret)
+			return ret;
 	}
 
 	return devm_iio_device_register(&client->dev, indio_dev);
-- 
2.43.0

Re: [PATCH 2/2] iio: proximity: vl53l0x-i2c: Added continuous mode support

[Jonathan Cameron]

On Sat, 31 Aug 2024 01:46:26 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> The continuous mode of the sensor is enabled in the buffer_postenable.
> Replaced the original irq handler with a threaded irq handler to perform
> i2c reads during continuous mode.
> The continuous mode is disabled by disabling the buffer.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>

Hi Abhash,

You've ended up with an implementation that is somewhere between
a triggered buffer approach (normal one an appropriate here) and that
used when we don't have triggers (typically because there is a fifo in the path).

Please take a look at how other drivers do this.  In particularly if you
are not pushing data to the buffer from the pollfunc (registered for the
triggered buffer) then you are doing it wrong.

Also, consider if other triggers could be used as if not you need to
both document why and add the validation callbacks to stop other triggers
being assigned (once you've added one that can be!)

Feel free to ask if you have more questions, but your first reference
should be other drivers (and I hope we don't have any that do it this way).

Jonathan

> ---
>  drivers/iio/proximity/vl53l0x-i2c.c | 125 ++++++++++++++++++++++------
>  1 file changed, 101 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> index 2b3dd18be..b0c947586 100644
> --- a/drivers/iio/proximity/vl53l0x-i2c.c
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -22,6 +22,10 @@
>  #include <linux/module.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #define VL_REG_SYSRANGE_START				0x00
>  
> @@ -49,14 +53,58 @@ struct vl53l0x_data {
>  	struct completion completion;
>  	struct regulator *vdd_supply;
>  	struct gpio_desc *reset_gpio;
> +
> +	struct {
> +		u16 chan;
> +		s64 timestamp __aligned(8);
> +	} scan;
>  };
>  
> -static irqreturn_t vl53l0x_handle_irq(int irq, void *priv)
> +static void vl53l0x_clear_irq(struct vl53l0x_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					VL_REG_SYSTEM_INTERRUPT_CLEAR, 1);
> +	if (ret < 0)
> +		dev_err(dev, "failed to clear error irq: %d\n", ret);
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					VL_REG_SYSTEM_INTERRUPT_CLEAR, 0);
> +	if (ret < 0)
> +		dev_err(dev, "failed to clear range irq: %d\n", ret);

ouch, not a write 1 to clear register?  I can't find docs, but this is really
nasty bit of interface design if you have to toggle the bit.

> +
> +	ret = i2c_smbus_read_byte_data(data->client, VL_REG_RESULT_INT_STATUS);
> +	if (ret < 0 || ret & 0x07)
> +		dev_err(dev, "failed to clear irq: %d\n", ret);

return an error from this, and elect whether to do anything or not
with that at the caller.

> +}
> +
> +static irqreturn_t vl53l0x_threaded_irq(int irq, void *priv)
>  {
>  	struct iio_dev *indio_dev = priv;
>  	struct vl53l0x_data *data = iio_priv(indio_dev);
> +	u8 buffer[12];

See below for more comments, but this is not a triggered buffer
setup. It looks like what we do when draining fifos (where a trigger
concept doesn't apply) but that's not what you have here.  So take
a look at how the split trigger / triggered buffer approach works
in other drivers.

> +	int ret;
>  
> -	complete(&data->completion);
> +	if (iio_buffer_enabled(indio_dev)) {
> +		ret = i2c_smbus_read_i2c_block_data(data->client,
> +						VL_REG_RESULT_RANGE_STATUS,
> +						sizeof(buffer), buffer);
> +		if (ret < 0)
> +			return ret;
> +		else if (ret != 12)
> +			return -EREMOTEIO;
> +
> +		data->scan.chan = (buffer[10] << 8) + buffer[11];

get_unaligned_be16()


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

If you are using a trigger (and they are optional) then this interrupt should
only be call iio_trigger_poll_nested() not do the data reading.


> +
> +		iio_trigger_notify_done(indio_dev->trig);
> +		vl53l0x_clear_irq(data);
> +	} else
> +		complete(&data->completion);
>  
>  	return IRQ_HANDLED;
>  }

>  
> -static void vl53l0x_clear_irq(struct vl53l0x_data *data)
> -{
> -	struct device *dev = &data->client->dev;
> -	int ret;
> -
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					VL_REG_SYSTEM_INTERRUPT_CLEAR, 1);
> -	if (ret < 0)
> -		dev_err(dev, "failed to clear error irq: %d\n", ret);
> -
> -	ret = i2c_smbus_write_byte_data(data->client,
> -					VL_REG_SYSTEM_INTERRUPT_CLEAR, 0);
> -	if (ret < 0)
> -		dev_err(dev, "failed to clear range irq: %d\n", ret);
> -
> -	ret = i2c_smbus_read_byte_data(data->client, VL_REG_RESULT_INT_STATUS);
> -	if (ret < 0 || ret & 0x07)
> -		dev_err(dev, "failed to clear irq: %d\n", ret);
> -}
> -

>  
>  static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> @@ -221,6 +257,40 @@ static int vl53l0x_power_on(struct vl53l0x_data *data)
>  	return 0;
>  }
>  
> +static int vl53l0x_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct vl53l0x_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, VL_REG_SYSRANGE_START, 0x02);

return i2c_smbus_write_byte_data()

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int vl53l0x_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct vl53l0x_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, VL_REG_SYSRANGE_START, 0x01);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Let the ongoing reading finish */
> +	reinit_completion(&data->completion);
> +	wait_for_completion_timeout(&data->completion, HZ/10);
Spaces around /

> +	vl53l0x_clear_irq(data);
> +
> +	return 0;
> +}

>  static int vl53l0x_probe(struct i2c_client *client)
>  {
>  	struct vl53l0x_data *data;
> @@ -279,6 +349,13 @@ static int vl53l0x_probe(struct i2c_client *client)
>  		ret = vl53l0x_configure_irq(client, indio_dev);
>  		if (ret)
>  			return ret;
> +
> +		ret = devm_iio_triggered_buffer_setup(&client->dev,
> +					indio_dev,
> +					&iio_pollfunc_store_time,

This is odd.  You don't seem to have a function to be called to actually store
the data.  Note you also need to consider if other triggers might be used.

Ultimately this doesn't look like a triggered buffer to me.
It is fine for some devices to drive the buffer directly but in that
case register the kfifo buffer. 

I'm not sure what reason we have to do that here though as this is a very
conventional one interrupt per 'scan' of data device.

So you should be registering a trigger, and a buffer then letting the
trigger drive the buffer.



> +					NULL, &iio_triggered_buffer_setup_ops);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return devm_iio_device_register(&client->dev, indio_dev);

Re: [PATCH 2/2] iio: proximity: vl53l0x-i2c: Added continuous mode support

[Abhash jha]

> Also, consider if other triggers could be used as if not you need to
> both document why and add the validation callbacks to stop other triggers
> being assigned (once you've added one that can be!)
>
> Feel free to ask if you have more questions, but your first reference
> should be other drivers (and I hope we don't have any that do it this way).
>
I used this driver as a reference
https://github.com/torvalds/linux/blob/master/drivers/iio/adc/vf610_adc.c#L556

>
> ouch, not a write 1 to clear register?  I can't find docs, but this is really
> nasty bit of interface design if you have to toggle the bit.
>
Actually ST has not provided a register map or any application note
for the sensor.
So there's no way to cross reference. Hence I kept the original code.
But I will try to write 1 to clear register with my sensor.

> > +             ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +                                     indio_dev,
> > +                                     &iio_pollfunc_store_time,
>
> This is odd.  You don't seem to have a function to be called to actually store
> the data.  Note you also need to consider if other triggers might be used.
>
> I'm not sure what reason we have to do that here though as this is a very
> conventional one interrupt per 'scan' of data device.
>
> So you should be registering a trigger, and a buffer then letting the
> trigger drive the buffer.

Why do I need to register a trigger? Would it be fine to let the irq
fill the buffer
with data as  it continuously reads it in the poll function?

So according to my understanding,
I need to move all the data reading and pushing to the poll function
and not do it in the irq handler.
Then register that poll function here during iio_triggered_buffer_setup.
Is there something that I am missing?

Regards,
Abhash

Re: [PATCH 2/2] iio: proximity: vl53l0x-i2c: Added continuous mode support

[Jonathan Cameron]

On Sat, 31 Aug 2024 22:12:27 +0530
Abhash jha <abhashkumarjha123@gmail.com> wrote:

> > Also, consider if other triggers could be used as if not you need to
> > both document why and add the validation callbacks to stop other triggers
> > being assigned (once you've added one that can be!)
> >
> > Feel free to ask if you have more questions, but your first reference
> > should be other drivers (and I hope we don't have any that do it this way).
> >  
> I used this driver as a reference
> https://github.com/torvalds/linux/blob/master/drivers/iio/adc/vf610_adc.c#L556

ouch.  Whilst that's an ancient driver, its unfortunately handling the buffer
completely wrongly :(  It's old enough that I suspect there will be no interest
in improving that one.

> 
> >
> > ouch, not a write 1 to clear register?  I can't find docs, but this is really
> > nasty bit of interface design if you have to toggle the bit.
> >  
> Actually ST has not provided a register map or any application note
> for the sensor.
> So there's no way to cross reference. Hence I kept the original code.
> But I will try to write 1 to clear register with my sensor.
> 
> > > +             ret = devm_iio_triggered_buffer_setup(&client->dev,
> > > +                                     indio_dev,
> > > +                                     &iio_pollfunc_store_time,  
> >
> > This is odd.  You don't seem to have a function to be called to actually store
> > the data.  Note you also need to consider if other triggers might be used.
> >
> > I'm not sure what reason we have to do that here though as this is a very
> > conventional one interrupt per 'scan' of data device.
> >
> > So you should be registering a trigger, and a buffer then letting the
> > trigger drive the buffer.  
> 
> Why do I need to register a trigger? Would it be fine to let the irq
> fill the buffer
> with data as  it continuously reads it in the poll function?

That is a possibility but it's not nearly as flexible as providing
a trigger so we generally only do that if there is a fifo in the way
so no 'per scan' signal is available.

The model of IIO is that you may associate different triggers
with a device and each trigger can drive capture from multiple devices
allowing reasonable data synchronisation.  There are lots of
devices that don't have a sequencer / 'continuous' mode and we
want the interface to looks the same for those as it does for those
that do have such support.

> 
> So according to my understanding,
> I need to move all the data reading and pushing to the poll function
> and not do it in the irq handler.
> Then register that poll function here during iio_triggered_buffer_setup.
> Is there something that I am missing?

You have it right.

The only other thing is to work out if it is possible to use other
triggers with the device (I can't see why you wouldn't be able to
use this trigger to drive other device).  

Sometimes using other triggers requires a slightly more complex
sequence so you can check if using a devices own trigger or not
and modify what goes on if it is a different trigger.

If multiple trigger support is complex, it is fine to start out
with providing a trigger validation function iio_info->validate_trigger()
and there is an iio_validate_own_trigger() that you can use to
avoid having to code anything. That relies on the iio_trigger
and the iio_device having the same parent (which they should
do here).

Jonathan

> 
> Regards,
> Abhash