| Message ID | 20241004150148.14033-2-abhashkumarjha123@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Interrupt and Continuous mode support for VL6180 | expand |
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
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);
> 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
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
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);
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(-)