[PATCH 0/3] Interrupt and Continuous mode support for VL6180

[PATCH 0/3] Interrupt and Continuous mode support for VL6180

[Abhash Jha]

Hello,

The first patch adds support for configuring the Sampling frequency
(inter-measurement period) of the sensor. The values must be provided
in miliseconds. The default value for the inter-measurement period for
ALS is 10ms and for Range is 50ms.

The second patch adds support for interrupt based single shot reading.
We registered an irq_handler that fires everytime the data is ready.
And then we read the appropriate value in the `vl6180_measure` routine.

The third patch adds support for continuous mode in the sensor by using
buffers. We enable the sensor's continuous mode in the buffer_postenable
function depending on the `active_scan_mask`.
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.

Thanks,
Abhash Jha


Abhash Jha (3):
  iio: light: vl6180: Add configurable inter-measurement period support
  iio: light: vl6180: Added Interrupt support for single shot access
  iio: light: vl6180: Add support for Continuous Mode

 drivers/iio/light/vl6180.c | 267 ++++++++++++++++++++++++++++++++++---
 1 file changed, 250 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support

[Abhash Jha]

Expose the IIO_CHAN_INFO_SAMP_FREQ attribute as a way to configure the
inter-measurement period for both the IIO_DISTANCE and IIO_LIGHT
channels. The inter-measurement period must be given in miliseconds.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/vl6180.c | 67 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
index a1b2b3c0b..52a837644 100644
--- a/drivers/iio/light/vl6180.c
+++ b/drivers/iio/light/vl6180.c
@@ -38,7 +38,9 @@
 #define VL6180_OUT_OF_RESET 0x016
 #define VL6180_HOLD 0x017
 #define VL6180_RANGE_START 0x018
+#define VL6180_RANGE_INTER_MEAS_TIME 0x01b
 #define VL6180_ALS_START 0x038
+#define VL6180_ALS_INTER_MEAS_TIME 0x03e
 #define VL6180_ALS_GAIN 0x03f
 #define VL6180_ALS_IT 0x040
 
@@ -86,6 +88,8 @@ struct vl6180_data {
 	struct mutex lock;
 	unsigned int als_gain_milli;
 	unsigned int als_it_ms;
+	unsigned int als_meas_rate;
+	unsigned int range_meas_rate;
 };
 
 enum { VL6180_ALS, VL6180_RANGE, VL6180_PROX };
@@ -261,12 +265,14 @@ static const struct iio_chan_spec vl6180_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_INT_TIME) |
 			BIT(IIO_CHAN_INFO_SCALE) |
-			BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+			BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+			BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}, {
 		.type = IIO_DISTANCE,
 		.address = VL6180_RANGE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_SCALE),
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}, {
 		.type = IIO_PROXIMITY,
 		.address = VL6180_PROX,
@@ -333,6 +339,18 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_FRACTIONAL;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_DISTANCE:
+			*val = data->range_meas_rate;
+			return IIO_VAL_INT;
+		case IIO_LIGHT:
+			*val = data->als_meas_rate;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+
 	default:
 		return -EINVAL;
 	}
@@ -412,11 +430,22 @@ static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
 	return ret;
 }
 
+static int vl6180_meas_reg_val_from_ms(unsigned int period)
+{
+	unsigned int reg_val = 0;
+
+	if (period > 10)
+		reg_val = period < 2550 ? (DIV_ROUND_CLOSEST(period, 10) - 1) : 254;
+
+	return reg_val;
+}
+
 static int vl6180_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
 	struct vl6180_data *data = iio_priv(indio_dev);
+	unsigned int reg_val;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
@@ -427,6 +456,24 @@ static int vl6180_write_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		return vl6180_set_als_gain(data, val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		guard(mutex)(&data->lock);
+		switch (chan->type) {
+		case IIO_DISTANCE:
+			data->range_meas_rate = val;
+			reg_val = vl6180_meas_reg_val_from_ms(val);
+			return vl6180_write_byte(data->client, VL6180_RANGE_INTER_MEAS_TIME, reg_val);
+
+		case IIO_LIGHT:
+			data->als_meas_rate = val;
+			reg_val = vl6180_meas_reg_val_from_ms(val);
+			return vl6180_write_byte(data->client, VL6180_ALS_INTER_MEAS_TIME, reg_val);
+
+		default:
+			return -EINVAL;
+		}
+
 	default:
 		return -EINVAL;
 	}
@@ -473,6 +520,22 @@ static int vl6180_init(struct vl6180_data *data)
 	if (ret < 0)
 		return ret;
 
+	/* Default Range inter-measurement time: 50ms
+	 * reg_val = (50 / 10 - 1) = 4
+	 */
+	ret = vl6180_write_byte(client, VL6180_RANGE_INTER_MEAS_TIME, 4);
+	if (ret < 0)
+		return ret;
+	data->range_meas_rate = 50;
+
+	/* Default ALS inter-measurement time: 10ms
+	 * reg_val = (10 / 10 - 1) = 0
+	 */
+	ret = vl6180_write_byte(client, VL6180_ALS_INTER_MEAS_TIME, 0);
+	if (ret < 0)
+		return ret;
+	data->als_meas_rate = 10;
+
 	/* ALS integration time: 100ms */
 	data->als_it_ms = 100;
 	ret = vl6180_write_word(client, VL6180_ALS_IT, VL6180_ALS_IT_100);
-- 
2.43.0

Re: [PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support

[kernel test robot]

Hi Abhash,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abhash-Jha/iio-light-vl6180-Add-configurable-inter-measurement-period-support/20241004-230433
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241004150148.14033-2-abhashkumarjha123%40gmail.com
patch subject: [PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support
config: x86_64-buildonly-randconfig-001-20241005 (https://download.01.org/0day-ci/archive/20241005/202410052012.iy9nXdU8-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241005/202410052012.iy9nXdU8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410052012.iy9nXdU8-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/iio/light/vl6180.c:461:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
     461 |                 guard(mutex)(&data->lock);
         |                 ^
   include/linux/cleanup.h:167:2: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |         ^
   include/linux/cleanup.h:122:2: note: expanded from macro 'CLASS'
     122 |         class_##_name##_t var __cleanup(class_##_name##_destructor) =   \
         |         ^
   <scratch space>:101:1: note: expanded from here
     101 | class_mutex_t
         | ^
>> drivers/iio/light/vl6180.c:477:2: error: cannot jump from switch statement to this case label
     477 |         default:
         |         ^
   drivers/iio/light/vl6180.c:461:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
     461 |                 guard(mutex)(&data->lock);
         |                 ^
   include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
     167 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:99:1: note: expanded from here
      99 | __UNIQUE_ID_guard385
         | ^
   1 warning and 1 error generated.


vim +477 drivers/iio/light/vl6180.c

006f437eee8f94 Abhash Jha            2024-10-04  442  
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  443  static int vl6180_write_raw(struct iio_dev *indio_dev,
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  444  			     struct iio_chan_spec const *chan,
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  445  			     int val, int val2, long mask)
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  446  {
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  447  	struct vl6180_data *data = iio_priv(indio_dev);
006f437eee8f94 Abhash Jha            2024-10-04  448  	unsigned int reg_val;
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  449  
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  450  	switch (mask) {
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  451  	case IIO_CHAN_INFO_INT_TIME:
1e2ed3d0d27d80 Stefan Brüns          2017-09-24  452  		return vl6180_set_it(data, val, val2);
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  453  
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  454  	case IIO_CHAN_INFO_HARDWAREGAIN:
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  455  		if (chan->type != IIO_LIGHT)
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  456  			return -EINVAL;
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  457  
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  458  		return vl6180_set_als_gain(data, val, val2);
006f437eee8f94 Abhash Jha            2024-10-04  459  
006f437eee8f94 Abhash Jha            2024-10-04  460  	case IIO_CHAN_INFO_SAMP_FREQ:
006f437eee8f94 Abhash Jha            2024-10-04 @461  		guard(mutex)(&data->lock);
006f437eee8f94 Abhash Jha            2024-10-04  462  		switch (chan->type) {
006f437eee8f94 Abhash Jha            2024-10-04  463  		case IIO_DISTANCE:
006f437eee8f94 Abhash Jha            2024-10-04  464  			data->range_meas_rate = val;
006f437eee8f94 Abhash Jha            2024-10-04  465  			reg_val = vl6180_meas_reg_val_from_ms(val);
006f437eee8f94 Abhash Jha            2024-10-04  466  			return vl6180_write_byte(data->client, VL6180_RANGE_INTER_MEAS_TIME, reg_val);
006f437eee8f94 Abhash Jha            2024-10-04  467  
006f437eee8f94 Abhash Jha            2024-10-04  468  		case IIO_LIGHT:
006f437eee8f94 Abhash Jha            2024-10-04  469  			data->als_meas_rate = val;
006f437eee8f94 Abhash Jha            2024-10-04  470  			reg_val = vl6180_meas_reg_val_from_ms(val);
006f437eee8f94 Abhash Jha            2024-10-04  471  			return vl6180_write_byte(data->client, VL6180_ALS_INTER_MEAS_TIME, reg_val);
006f437eee8f94 Abhash Jha            2024-10-04  472  
006f437eee8f94 Abhash Jha            2024-10-04  473  		default:
006f437eee8f94 Abhash Jha            2024-10-04  474  			return -EINVAL;
006f437eee8f94 Abhash Jha            2024-10-04  475  		}
006f437eee8f94 Abhash Jha            2024-10-04  476  
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19 @477  	default:
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  478  		return -EINVAL;
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  479  	}
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  480  }
5e7f47e495ad36 Manivannan Sadhasivam 2017-03-19  481  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support

[Jonathan Cameron]

On Fri,  4 Oct 2024 20:31:46 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Expose the IIO_CHAN_INFO_SAMP_FREQ attribute as a way to configure the
> inter-measurement period for both the IIO_DISTANCE and IIO_LIGHT
> channels. The inter-measurement period must be given in miliseconds.

Hi Abhash,

Sampling frequency must be in Hz and reflect how often the channel
is sampled (not just the inter measurement period.  So this sounds wrong.
It is sometimes complex to compute but we have to stick to the documented
ABI.

Other comments inline.

Thanks

Jonathan

> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>


> @@ -412,11 +430,22 @@ static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
>  	return ret;
>  }
>  
> +static int vl6180_meas_reg_val_from_ms(unsigned int period)
> +{
> +	unsigned int reg_val = 0;
> +
> +	if (period > 10)
> +		reg_val = period < 2550 ? (DIV_ROUND_CLOSEST(period, 10) - 1) : 254;
> +
> +	return reg_val;
> +}
> +
>  static int vl6180_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
>  	struct vl6180_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
> @@ -427,6 +456,24 @@ static int vl6180_write_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  
>  		return vl6180_set_als_gain(data, val, val2);
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
	{

needed to define scope for that guard as you probably intend.

> +		guard(mutex)(&data->lock);
> +		switch (chan->type) {
> +		case IIO_DISTANCE:
> +			data->range_meas_rate = val;
> +			reg_val = vl6180_meas_reg_val_from_ms(val);
> +			return vl6180_write_byte(data->client, VL6180_RANGE_INTER_MEAS_TIME, reg_val);

long lines. I don't mind going over 80 chars when readability is badly hurt, but
in this case it isn't so wrap the parameters.

> +
> +		case IIO_LIGHT:
> +			data->als_meas_rate = val;
> +			reg_val = vl6180_meas_reg_val_from_ms(val);
> +			return vl6180_write_byte(data->client, VL6180_ALS_INTER_MEAS_TIME, reg_val);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -473,6 +520,22 @@ static int vl6180_init(struct vl6180_data *data)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Default Range inter-measurement time: 50ms

As below.

Even though you now it in advance, I'd rather you used the vl6180_meas_reg_val_from_ms()
subject to the whole thing about it needing to be in Hz

> +	 * reg_val = (50 / 10 - 1) = 4
> +	 */
> +	ret = vl6180_write_byte(client, VL6180_RANGE_INTER_MEAS_TIME, 4);
> +	if (ret < 0)
> +		return ret;
> +	data->range_meas_rate = 50;
> +
> +	/* Default ALS inter-measurement time: 10ms
Multiline comment syntax in IIO (and most of the rest of the kernel)
is 
	/*
	 * Default ...

> +	 * reg_val = (10 / 10 - 1) = 0
> +	 */
> +	ret = vl6180_write_byte(client, VL6180_ALS_INTER_MEAS_TIME, 0);
> +	if (ret < 0)
> +		return ret;
> +	data->als_meas_rate = 10;
> +
>  	/* ALS integration time: 100ms */
>  	data->als_it_ms = 100;
>  	ret = vl6180_write_word(client, VL6180_ALS_IT, VL6180_ALS_IT_100);

Re: [PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support

[Abhash jha]

> Hi Abhash,
>
> Sampling frequency must be in Hz and reflect how often the channel
> is sampled (not just the inter measurement period.  So this sounds wrong.
> It is sometimes complex to compute but we have to stick to the documented
> ABI.
Got it. I thought of skipping out the complex computation in the
driver and assumed
the user would give me pre-computed ms values.

Just one thing, Is it better to just use IIO_CHAN_INFO_SAMP_FREQ for
"inter-measurement period"
and get the input in HZ (converting HZ to ms in driver)
Or
Define a custom sysfs attribute like `inter_measurement_period` to get
ms values? for this driver.

Thanks,
Abhash

Re: [PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support

[Jonathan Cameron]

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

> > Hi Abhash,
> >
> > Sampling frequency must be in Hz and reflect how often the channel
> > is sampled (not just the inter measurement period.  So this sounds wrong.
> > It is sometimes complex to compute but we have to stick to the documented
> > ABI.  
> Got it. I thought of skipping out the complex computation in the
> driver and assumed
> the user would give me pre-computed ms values.
> 
> Just one thing, Is it better to just use IIO_CHAN_INFO_SAMP_FREQ for
> "inter-measurement period"
> and get the input in HZ (converting HZ to ms in driver)
This one.   
> Or
> Define a custom sysfs attribute like `inter_measurement_period` to get
> ms values? for this driver.
Always best to avoid custom attributes where at all possible because
standard userspace has no way to know how to use them.

Jonathan

> 
> Thanks,
> Abhash

[PATCH 2/3] iio: light: vl6180: Added Interrupt support for single shot access

[Abhash Jha]

Configured the GPIO1 pin to provide output interrupts. And then the
interrupts are serviced in the `vl6180_measure` function when the
irq_handler signals that the reading is complete.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/vl6180.c | 64 +++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
index 52a837644..4c2b486e2 100644
--- a/drivers/iio/light/vl6180.c
+++ b/drivers/iio/light/vl6180.c
@@ -33,6 +33,7 @@
 #define VL6180_MODEL_ID_VAL 0xb4
 
 /* Configuration registers */
+#define VL6180_MODE_GPIO1 0x011
 #define VL6180_INTR_CONFIG 0x014
 #define VL6180_INTR_CLEAR 0x015
 #define VL6180_OUT_OF_RESET 0x016
@@ -70,6 +71,9 @@
 /* bits of the HOLD register */
 #define VL6180_HOLD_ON BIT(0)
 
+/* bit for enabling GPIO1 Interrupt Output */
+#define VL6180_GPIO1_INTR_OUT BIT(4)
+
 /* default value for the ALS_IT register */
 #define VL6180_ALS_IT_100 0x63 /* 100 ms */
 
@@ -86,6 +90,7 @@
 struct vl6180_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	struct completion completion;
 	unsigned int als_gain_milli;
 	unsigned int als_it_ms;
 	unsigned int als_meas_rate;
@@ -211,6 +216,7 @@ static int vl6180_write_word(struct i2c_client *client, u16 cmd, u16 val)
 static int vl6180_measure(struct vl6180_data *data, int addr)
 {
 	struct i2c_client *client = data->client;
+	unsigned long time_left;
 	int tries = 20, ret;
 	u16 value;
 
@@ -221,19 +227,26 @@ static int vl6180_measure(struct vl6180_data *data, int addr)
 	if (ret < 0)
 		goto fail;
 
-	while (tries--) {
-		ret = vl6180_read_byte(client, VL6180_INTR_STATUS);
-		if (ret < 0)
-			goto fail;
-
-		if (ret & vl6180_chan_regs_table[addr].drdy_mask)
-			break;
-		msleep(20);
-	}
+	if (client->irq) {
+		reinit_completion(&data->completion);
+		time_left = wait_for_completion_timeout(&data->completion, HZ/10);
+		if (time_left == 0)
+			return -ETIMEDOUT;
+	} else {
+		while (tries--) {
+			ret = vl6180_read_byte(client, VL6180_INTR_STATUS);
+			if (ret < 0)
+				goto fail;
+
+			if (ret & vl6180_chan_regs_table[addr].drdy_mask)
+				break;
+			msleep(20);
+		}
 
-	if (tries < 0) {
-		ret = -EIO;
-		goto fail;
+		if (tries < 0) {
+			ret = -EIO;
+			goto fail;
+		}
 	}
 
 	/* Read result value from appropriate registers */
@@ -479,6 +492,15 @@ static int vl6180_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+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);
+	return IRQ_HANDLED;
+}
+
 static const struct iio_info vl6180_info = {
 	.read_raw = vl6180_read_raw,
 	.write_raw = vl6180_write_raw,
@@ -514,6 +536,11 @@ static int vl6180_init(struct vl6180_data *data)
 	if (ret != 0x01)
 		dev_info(&client->dev, "device is not fresh out of reset\n");
 
+	ret = vl6180_write_byte(client, VL6180_MODE_GPIO1,
+				VL6180_GPIO1_INTR_OUT);
+	if (ret < 0)
+		return ret;
+
 	/* Enable ALS and Range ready interrupts */
 	ret = vl6180_write_byte(client, VL6180_INTR_CONFIG,
 				VL6180_ALS_READY | VL6180_RANGE_READY);
@@ -580,6 +607,19 @@ static int vl6180_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, vl6180_threaded_irq,
+						IRQF_ONESHOT,
+						indio_dev->name, indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "devm_request_irq error: %d\n", ret);
+			return ret;
+		}
+
+		init_completion(&data->completion);
+	}
+
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
-- 
2.43.0

Re: [PATCH 2/3] iio: light: vl6180: Added Interrupt support for single shot access

[Jonathan Cameron]

On Fri,  4 Oct 2024 20:31:47 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Configured the GPIO1 pin to provide output interrupts. And then the
> interrupts are serviced in the `vl6180_measure` function when the
> irq_handler signals that the reading is complete.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>

A few comments inline.

Thanks,

Jonathan

> ---

> @@ -211,6 +216,7 @@ static int vl6180_write_word(struct i2c_client *client, u16 cmd, u16 val)
>  static int vl6180_measure(struct vl6180_data *data, int addr)
>  {
>  	struct i2c_client *client = data->client;
> +	unsigned long time_left;
>  	int tries = 20, ret;
>  	u16 value;
>  
> @@ -221,19 +227,26 @@ static int vl6180_measure(struct vl6180_data *data, int addr)
>  	if (ret < 0)
>  		goto fail;
>  
> -	while (tries--) {
> -		ret = vl6180_read_byte(client, VL6180_INTR_STATUS);
> -		if (ret < 0)
> -			goto fail;
> -
> -		if (ret & vl6180_chan_regs_table[addr].drdy_mask)
> -			break;
> -		msleep(20);
> -	}
> +	if (client->irq) {
> +		reinit_completion(&data->completion);

That's late so there is a race condition. You might be delayed just before this
and finish the measurement before the reint_completion() in which case you'll
clear the complete() that happens in the interrupt handler before
then waiting on it.  This reinit needs to be before whatever can potentially trigger
that interrupt.

> +		time_left = wait_for_completion_timeout(&data->completion, HZ/10);
HZ / 10
> +		if (time_left == 0)
> +			return -ETIMEDOUT;
> +	} else {
> +		while (tries--) {
> +			ret = vl6180_read_byte(client, VL6180_INTR_STATUS);
> +			if (ret < 0)
> +				goto fail;
> +
> +			if (ret & vl6180_chan_regs_table[addr].drdy_mask)
> +				break;
> +			msleep(20);
> +		}
>  
> -	if (tries < 0) {
> -		ret = -EIO;
> -		goto fail;
> +		if (tries < 0) {
> +			ret = -EIO;
> +			goto fail;
> +		}
>  	}
>  
>  	/* Read result value from appropriate registers */
> @@ -479,6 +492,15 @@ static int vl6180_write_raw(struct iio_dev *indio_dev,
>  	}
>  }

...

>  static const struct iio_info vl6180_info = {
>  	.read_raw = vl6180_read_raw,
>  	.write_raw = vl6180_write_raw,
> @@ -514,6 +536,11 @@ static int vl6180_init(struct vl6180_data *data)
>  	if (ret != 0x01)
>  		dev_info(&client->dev, "device is not fresh out of reset\n");
>  
> +	ret = vl6180_write_byte(client, VL6180_MODE_GPIO1,
> +				VL6180_GPIO1_INTR_OUT);

I would only do this if the interrupt is wired. It's probably harmless otherwise
but seems a bit odd if that pin isn't connected to anything.

> +	if (ret < 0)
> +		return ret;
> +
>  	/* Enable ALS and Range ready interrupts */
>  	ret = vl6180_write_byte(client, VL6180_INTR_CONFIG,
>  				VL6180_ALS_READY | VL6180_RANGE_READY);
> @@ -580,6 +607,19 @@ static int vl6180_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, vl6180_threaded_irq,
> +						IRQF_ONESHOT,
> +						indio_dev->name, indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "devm_request_irq error: %d\n", ret);
> +			return ret;

return dev_err_probe()

> +		}
> +
> +		init_completion(&data->completion);
> +	}
> +
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  

Re: [PATCH 2/3] iio: light: vl6180: Added Interrupt support for single shot access

[Abhash jha]

> > +     if (client->irq) {
> > +             reinit_completion(&data->completion);
>
> That's late so there is a race condition. You might be delayed just before this
> and finish the measurement before the reint_completion() in which case you'll
> clear the complete() that happens in the interrupt handler before
> then waiting on it.
Yes this makes sense.

> This reinit needs to be before whatever can potentially trigger
> that interrupt.
Can you explain this part, because where can i reinit it, The measurement
starts when we write the START_STOP bit to SYSRANGE_START. So should
it be before that.
I'm kind of confused with this.

Thank you,
Abhash

Re: [PATCH 2/3] iio: light: vl6180: Added Interrupt support for single shot access

[Jonathan Cameron]

On Sat, 5 Oct 2024 23:05:25 +0530
Abhash jha <abhashkumarjha123@gmail.com> wrote:

> > > +     if (client->irq) {
> > > +             reinit_completion(&data->completion);  
> >
> > That's late so there is a race condition. You might be delayed just before this
> > and finish the measurement before the reint_completion() in which case you'll
> > clear the complete() that happens in the interrupt handler before
> > then waiting on it.  
> Yes this makes sense.
> 
> > This reinit needs to be before whatever can potentially trigger
> > that interrupt.  
> Can you explain this part, because where can i reinit it, The measurement
> starts when we write the START_STOP bit to SYSRANGE_START. So should
> it be before that.
Immediately above that write looks right to me. So before writing START_STOP.
That way the completion will be ready for a complete whenever
one turns up.

Jonathan

> I'm kind of confused with this.
> 
> Thank you,
> Abhash

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

[Abhash Jha]

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

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@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;

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

[Abhash jha]

> 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

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

[Jonathan Cameron]

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