[PATCH v3 0/2] Continuous mode support for VL53l0x

[PATCH v3 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.
Added a trigger to the device for the continuous mode. Also validating that
the device uses the internal trigger provided by us.

Changes in v3:
- Added "asm/unaligned.h" include to fix `-Wimplicit-function-declaration`.
- The above error was pointed during testing by kernel-test-robot
- Link to v2: https://lore.kernel.org/linux-iio/20240902122557.129013-1-abhashkumarjha123@gmail.com/T/#t

Changes in v2:
- Added a trigger for the device
- Added a poll function for the triggered buffer setup
- Performed the data reading and pushing to buffers in the poll function
- Minor code refactoring

- Link to v1: https://lore.kernel.org/linux-iio/20240830201627.298264-1-abhashkumarjha123@gmail.com/T/#t


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 | 177 +++++++++++++++++++++++-----
 1 file changed, 150 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH v3 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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
index 8d4f3f849..31d6aeb95 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,13 @@ 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 < 0)
+		return -EINVAL;
+
+	if (ret != VL53L0X_MODEL_ID_VAL)
+		dev_info(&client->dev, "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 +276,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 v3 1/2] iio: proximity: vl53l0x-i2c: Added sensor ID check

[Jonathan Cameron]

On Tue,  3 Sep 2024 09:26:35 +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>
Hi Abhash,

Small comment on the message below

> ---
>  drivers/iio/proximity/vl53l0x-i2c.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> index 8d4f3f849..31d6aeb95 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,13 @@ 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 < 0)
> +		return -EINVAL;
> +
> +	if (ret != VL53L0X_MODEL_ID_VAL)
> +		dev_info(&client->dev, "Received invalid model id: 0x%x", ret);
Unknown model id:

It's probably valid, we just don't know what it means!

> +
>  	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 +276,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 v3 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.
Added a trigger for this device to be used for continuous mode.

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

diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
index 31d6aeb95..f91a9495a 100644
--- a/drivers/iio/proximity/vl53l0x-i2c.c
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -22,6 +22,12 @@
 #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>
+
+#include <asm/unaligned.h>
 
 #define VL_REG_SYSRANGE_START				0x00
 
@@ -49,14 +55,75 @@ struct vl53l0x_data {
 	struct completion completion;
 	struct regulator *vdd_supply;
 	struct gpio_desc *reset_gpio;
+	struct iio_trigger *trig;
+
+	struct {
+		u16 chan;
+		s64 timestamp __aligned(8);
+	} scan;
 };
 
-static irqreturn_t vl53l0x_handle_irq(int irq, void *priv)
+static int 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);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static irqreturn_t vl53l0x_trigger_handler(int irq, void *priv)
+{
+	struct iio_poll_func *pf = priv;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct vl53l0x_data *data = iio_priv(indio_dev);
+	u8 buffer[12];
+	int ret;
+
+	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 = get_unaligned_be16(&buffer[10]);
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+					iio_get_time_ns(indio_dev));
+
+	iio_trigger_notify_done(indio_dev->trig);
+	ret = vl53l0x_clear_irq(data);
+	if (ret < 0)
+		return ret;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t vl53l0x_threaded_irq(int irq, void *priv)
 {
 	struct iio_dev *indio_dev = priv;
 	struct vl53l0x_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;
 }
@@ -71,8 +138,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 +155,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)
@@ -128,7 +176,9 @@ static int vl53l0x_read_proximity(struct vl53l0x_data *data,
 		if (time_left == 0)
 			return -ETIMEDOUT;
 
-		vl53l0x_clear_irq(data);
+		ret = vl53l0x_clear_irq(data);
+		if (ret < 0)
+			return ret;
 	} else {
 		do {
 			ret = i2c_smbus_read_byte_data(client,
@@ -163,7 +213,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 +278,41 @@ 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);
+
+	return i2c_smbus_write_byte_data(data->client, VL_REG_SYSRANGE_START, 0x02);
+}
+
+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);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
+	.postenable = &vl53l0x_buffer_postenable,
+	.postdisable = &vl53l0x_buffer_postdisable,
+};
+
+static const struct iio_trigger_ops vl53l0x_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+};
+
 static int vl53l0x_probe(struct i2c_client *client)
 {
 	struct vl53l0x_data *data;
@@ -278,9 +370,31 @@ static int vl53l0x_probe(struct i2c_client *client)
 	if (client->irq) {
 		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 = &vl53l0x_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);
+
 		ret = vl53l0x_configure_irq(client, indio_dev);
 		if (ret)
 			return ret;
+
+		ret = devm_iio_triggered_buffer_setup(&client->dev,
+					indio_dev,
+					NULL,
+					&vl53l0x_trigger_handler,
+					&iio_triggered_buffer_setup_ops);
+		if (ret)
+			return ret;
 	}
 
 	return devm_iio_device_register(&client->dev, indio_dev);
-- 
2.43.0

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

[Jonathan Cameron]

On Tue,  3 Sep 2024 09:26:36 +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.
> Added a trigger for this device to be used for continuous mode.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>

Just to check, did you try this with other triggers?
Currently you have a check that the trigger is only used with the device
but not the validate_trigger that says the device may only use this
trigger.

A few minor comments inline. This is looking good in general.

Jonathan

> ---
>  drivers/iio/proximity/vl53l0x-i2c.c | 164 +++++++++++++++++++++++-----
>  1 file changed, 139 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> index 31d6aeb95..f91a9495a 100644
> --- a/drivers/iio/proximity/vl53l0x-i2c.c
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -22,6 +22,12 @@
>  #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>
> +
> +#include <asm/unaligned.h>
>  
>  #define VL_REG_SYSRANGE_START				0x00
>  
> @@ -49,14 +55,75 @@ struct vl53l0x_data {
>  	struct completion completion;
>  	struct regulator *vdd_supply;
>  	struct gpio_desc *reset_gpio;
> +	struct iio_trigger *trig;
> +
> +	struct {
> +		u16 chan;
> +		s64 timestamp __aligned(8);
> +	} scan;
>  };
>  
> -static irqreturn_t vl53l0x_handle_irq(int irq, void *priv)
> +static int 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);
It's unusual to carry on after a failed write. Add a comment on why that
makes sense here.
> +
> +	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);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t vl53l0x_trigger_handler(int irq, void *priv)
> +{
> +	struct iio_poll_func *pf = priv;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct vl53l0x_data *data = iio_priv(indio_dev);
> +	u8 buffer[12];
> +	int ret;
> +
> +	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 = get_unaligned_be16(&buffer[10]);
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +					iio_get_time_ns(indio_dev));
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	ret = vl53l0x_clear_irq(data);
> +	if (ret < 0)
> +		return ret;

you can't return normal error values from an interrupt handler.
Print a message and return IRQ_HANDLED is probably the right thing to do.

> +
> +	return IRQ_HANDLED;
> +}



>  static int vl53l0x_read_proximity(struct vl53l0x_data *data,
>  				  const struct iio_chan_spec *chan,
>  				  int *val)
> @@ -128,7 +176,9 @@ static int vl53l0x_read_proximity(struct vl53l0x_data *data,
>  		if (time_left == 0)
>  			return -ETIMEDOUT;
>  
> -		vl53l0x_clear_irq(data);
> +		ret = vl53l0x_clear_irq(data);
> +		if (ret < 0)
> +			return ret;
>  	} else {
>  		do {
>  			ret = i2c_smbus_read_byte_data(client,
> @@ -163,7 +213,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),
32
That's a big channel index to set.  Technically you can do that as they
are monotonic only, but more normal to have it right after the previous channel.

>  };
>  
>  static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> @@ -221,6 +278,41 @@ 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);
> +
> +	return i2c_smbus_write_byte_data(data->client, VL_REG_SYSRANGE_START, 0x02);
> +}
> +
> +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);

Can we name that 0x01 and the 0x02 above with names that give
us a hint what they are doing?

> +	if (ret < 0)
> +		return ret;
> +
> +	/* Let the ongoing reading finish */
> +	reinit_completion(&data->completion);
> +	wait_for_completion_timeout(&data->completion, HZ/10);

Space around the / to comply with kernel style.

> +	vl53l0x_clear_irq(data);
> +	if (ret < 0)

ret not set by anyone.

If it's form vl53l0x_clear_irq() and that can't do positive returns then
return vl53l0x_clear_irq(data);
should work.

> +		return ret;
> +
> +	return 0;
> +}
> +static const struct iio_trigger_ops vl53l0x_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
> +};