[PATCH v3 0/3] Minor cleanups and better error handling

[PATCH v3 0/3] Minor cleanups and better error handling

[Abhash Jha]

Hello,

The first patch in the series adds support for configuring the gain and
resolution(integration time) of the ltr390 sensor by writing to the
respective registers. Then the available values for gain and resolution
that are listed in the datasheet are provided via the `read_avail`
callback. 

The second patch adds a new channel for the ALS feature of the sensor.
The same configuration of gain and resolution has to be provided for this
channel as well. As there are two IIO channels now, we would need to
switch the sensor's mode of operation depending on which sensor is being
accessed. Hence, mode switching is also provided.

Then the third patch adds support for calculating `counts_per_uvi` based
on the current gain and resolution value.

Changes in v3:
- Added cover letter to the patch series.
- Fixed indentation in the patch description.
- Patch specific changes are listed below.

[PATCH v3 1/3]
	- Cleaned up the spurious changes made in v2.
	- ltr390_set_int_time and ltr390_set_gain now return -EINVAL to
    indicate no match.

[PATCH v3 2/3]
	- Used enum ltr390_mode inside the ltr390_data struct.
  - Refactored `ltr390_set_mode` function according to the comments in v2.

[PATCH v3 3/3]
	- Simplified the formula for `counts_per_uvi` calculation.
	- Removed spurious whitespace changes introduced in v2.

- Link to v2: https://lore.kernel.org/linux-iio/20240728151957.310237-1-abhashkumarjha123@gmail.com/

Changes in v2:
- Split the single patch into 3 patches.
- Used FIELD_PREP to perform bit shifting.
- Used enum for mode selection instead of defines.
- Fixed indentation and whitespace issues pointed out in the comments
- Replaced `mutex_lock(&data->lock)` with `guard(mutex)(&data->lock)`
- Provided available values for gain and resolution via `read_avail`
  instead of sysfs attributes.
- Refactored `ltr390_set_gain` and `ltr390_set_int_time`.
- Used early returns instead of single exit points.

- Link to v1: https://lore.kernel.org/linux-iio/20240718104947.7384-1-abhashkumarjha123@gmail.com/

Regards,
Abhash

Abhash Jha (3):
  iio: light: ltr390: Add configurable gain and resolution
  iio: light: ltr390: Add ALS channel and support for gain and
    resolution
  iio: light: ltr390: Calculate 'counts_per_uvi' dynamically

 drivers/iio/light/ltr390.c | 246 +++++++++++++++++++++++++++++++++----
 1 file changed, 225 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] iio: light: ltr390: Add configurable gain and resolution

[Abhash Jha]

Add support for configuring and reading the gain and resolution
(integration time). Also provide the available values for gain and
resoltion respectively via `read_avail` callback.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/ltr390.c | 135 ++++++++++++++++++++++++++++++++++---
 1 file changed, 124 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index fff1e8990..b79cd413f 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -28,16 +28,23 @@
 
 #include <asm/unaligned.h>
 
-#define LTR390_MAIN_CTRL      0x00
-#define LTR390_PART_ID	      0x06
-#define LTR390_UVS_DATA	      0x10
+#define LTR390_MAIN_CTRL		0x00
+#define LTR390_ALS_UVS_MEAS_RATE	0x04
+#define LTR390_ALS_UVS_GAIN		0x05
+#define LTR390_PART_ID			0x06
+#define LTR390_ALS_DATA			0x0D
+#define LTR390_UVS_DATA			0x10
+#define LTR390_INT_CFG			0x19
+
+#define LTR390_PART_NUMBER_ID		0xb
+#define LTR390_ALS_UVS_GAIN_MASK	0x07
+#define LTR390_ALS_UVS_INT_TIME_MASK	0x70
+#define LTR390_ALS_UVS_INT_TIME(x)	FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))
 
 #define LTR390_SW_RESET	      BIT(4)
 #define LTR390_UVS_MODE	      BIT(3)
 #define LTR390_SENSOR_ENABLE  BIT(1)
 
-#define LTR390_PART_NUMBER_ID 0xb
-
 /*
  * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
  * the sensor are equal to 1 UV Index [Datasheet Page#8].
@@ -60,6 +67,8 @@ struct ltr390_data {
 	struct i2c_client *client;
 	/* Protects device from simulataneous reads */
 	struct mutex lock;
+	int gain;
+	int int_time_us;
 };
 
 static const struct regmap_config ltr390_regmap_config = {
@@ -75,8 +84,6 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
 	int ret;
 	u8 recieve_buffer[3];
 
-	guard(mutex)(&data->lock);
-
 	ret = regmap_bulk_read(data->regmap, register_address, recieve_buffer,
 			       sizeof(recieve_buffer));
 	if (ret) {
@@ -94,6 +101,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 	int ret;
 	struct ltr390_data *data = iio_priv(iio_device);
 
+	guard(mutex)(&data->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = ltr390_register_read(data, LTR390_UVS_DATA);
@@ -105,18 +113,118 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 		*val = LTR390_WINDOW_FACTOR;
 		*val2 = LTR390_COUNTS_PER_UVI;
 		return IIO_VAL_FRACTIONAL;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = data->int_time_us;
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
 }
 
-static const struct iio_info ltr390_info = {
-	.read_raw = ltr390_read_raw,
-};
+/* integration time in us */
+static const int ltr390_int_time_map_us[] = { 400000, 200000, 100000, 50000, 25000, 12500 };
+static const int ltr390_gain_map[] = { 1, 3, 6, 9, 18 };
 
 static const struct iio_chan_spec ltr390_channel = {
 	.type = IIO_UVINDEX,
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+};
+
+static int ltr390_set_gain(struct ltr390_data *data, int val)
+{
+	int ret, idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
+		if (ltr390_gain_map[idx] != val)
+			continue;
+
+		guard(mutex)(&data->lock);
+		ret = regmap_update_bits(data->regmap,
+					LTR390_ALS_UVS_GAIN,
+					LTR390_ALS_UVS_GAIN_MASK, idx);
+		if (ret)
+			return ret;
+
+		data->gain = ltr390_gain_map[idx];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int ltr390_set_int_time(struct ltr390_data *data, int val)
+{
+	int ret, idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
+		if (ltr390_int_time_map_us[idx] != val)
+			continue;
+
+		guard(mutex)(&data->lock);
+		ret = regmap_update_bits(data->regmap,
+					LTR390_ALS_UVS_MEAS_RATE,
+					LTR390_ALS_UVS_INT_TIME_MASK,
+					LTR390_ALS_UVS_INT_TIME(idx));
+		if (ret)
+			return ret;
+
+		data->int_time_us = ltr390_int_time_map_us[idx];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+				const int **vals, int *type, int *length, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*length = ARRAY_SIZE(ltr390_gain_map);
+		*type = IIO_VAL_INT;
+		*vals = ltr390_gain_map;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(ltr390_int_time_map_us);
+		*type = IIO_VAL_INT;
+		*vals = ltr390_int_time_map_us;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val2 != 0)
+			return -EINVAL;
+
+		return ltr390_set_gain(data, val);
+
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val2 != 0)
+			return -EINVAL;
+
+		return ltr390_set_int_time(data, val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ltr390_info = {
+	.read_raw = ltr390_read_raw,
+	.write_raw = ltr390_write_raw,
+	.read_avail = ltr390_read_avail,
 };
 
 static int ltr390_probe(struct i2c_client *client)
@@ -139,6 +247,11 @@ static int ltr390_probe(struct i2c_client *client)
 				     "regmap initialization failed\n");
 
 	data->client = client;
+	/* default value of integration time from pg: 15 of the datasheet */
+	data->int_time_us = 100000;
+	/* default value of gain from pg: 16 of the datasheet */
+	data->gain = 3;
+
 	mutex_init(&data->lock);
 
 	indio_dev->info = &ltr390_info;
-- 
2.43.0

Re: [PATCH v3 1/3] iio: light: ltr390: Add configurable gain and resolution

[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.11-rc1 next-20240729]
[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-ltr390-Add-configurable-gain-and-resolution/20240729-222433
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20240729115056.355466-2-abhashkumarjha123%40gmail.com
patch subject: [PATCH v3 1/3] iio: light: ltr390: Add configurable gain and resolution
config: i386-buildonly-randconfig-003-20240730 (https://download.01.org/0day-ci/archive/20240730/202407300717.9WTaBkEv-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240730/202407300717.9WTaBkEv-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/202407300717.9WTaBkEv-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/light/ltr390.c:171:6: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     171 |                                         LTR390_ALS_UVS_INT_TIME(idx));
         |                                         ^
   drivers/iio/light/ltr390.c:42:36: note: expanded from macro 'LTR390_ALS_UVS_INT_TIME'
      42 | #define LTR390_ALS_UVS_INT_TIME(x)      FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))
         |                                         ^
   1 error generated.


vim +/FIELD_PREP +171 drivers/iio/light/ltr390.c

   158	
   159	static int ltr390_set_int_time(struct ltr390_data *data, int val)
   160	{
   161		int ret, idx;
   162	
   163		for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
   164			if (ltr390_int_time_map_us[idx] != val)
   165				continue;
   166	
   167			guard(mutex)(&data->lock);
   168			ret = regmap_update_bits(data->regmap,
   169						LTR390_ALS_UVS_MEAS_RATE,
   170						LTR390_ALS_UVS_INT_TIME_MASK,
 > 171						LTR390_ALS_UVS_INT_TIME(idx));
   172			if (ret)
   173				return ret;
   174	
   175			data->int_time_us = ltr390_int_time_map_us[idx];
   176			return 0;
   177		}
   178	
   179		return -EINVAL;
   180	}
   181	

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

Re: [PATCH v3 1/3] iio: light: ltr390: Add configurable gain and resolution

[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.11-rc1 next-20240729]
[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-ltr390-Add-configurable-gain-and-resolution/20240729-222433
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20240729115056.355466-2-abhashkumarjha123%40gmail.com
patch subject: [PATCH v3 1/3] iio: light: ltr390: Add configurable gain and resolution
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240730/202407301035.KehnJ97o-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240730/202407301035.KehnJ97o-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/202407301035.KehnJ97o-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/iio/light/ltr390.c: In function 'ltr390_set_int_time':
>> drivers/iio/light/ltr390.c:42:41: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
      42 | #define LTR390_ALS_UVS_INT_TIME(x)      FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))
         |                                         ^~~~~~~~~~
   drivers/iio/light/ltr390.c:171:41: note: in expansion of macro 'LTR390_ALS_UVS_INT_TIME'
     171 |                                         LTR390_ALS_UVS_INT_TIME(idx));
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~


vim +/FIELD_PREP +42 drivers/iio/light/ltr390.c

    38	
    39	#define LTR390_PART_NUMBER_ID		0xb
    40	#define LTR390_ALS_UVS_GAIN_MASK	0x07
    41	#define LTR390_ALS_UVS_INT_TIME_MASK	0x70
  > 42	#define LTR390_ALS_UVS_INT_TIME(x)	FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))
    43	

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

[PATCH v3 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Abhash Jha]

Add new ALS channel and allow reading raw and scale values.
Also provide gain and resolution configuration for ALS channel.
Add automatic mode switching between the UVS and ALS channel
based on which channel is being accessed.
The default mode in which the sensor start is ALS mode.

Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
---
 drivers/iio/light/ltr390.c | 109 ++++++++++++++++++++++++++++++++-----
 1 file changed, 94 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index b79cd413f..79f5a516a 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -62,11 +62,17 @@
  */
 #define LTR390_WINDOW_FACTOR 1
 
+enum ltr390_mode {
+	LTR390_SET_ALS_MODE,
+	LTR390_SET_UVS_MODE,
+};
+
 struct ltr390_data {
 	struct regmap *regmap;
 	struct i2c_client *client;
 	/* Protects device from simulataneous reads */
 	struct mutex lock;
+	enum ltr390_mode mode;
 	int gain;
 	int int_time_us;
 };
@@ -94,6 +100,25 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
 	return get_unaligned_le24(recieve_buffer);
 }
 
+static int ltr390_set_mode(struct ltr390_data *data, enum ltr390_mode mode)
+{
+	if (data->mode == mode)
+		return 0;
+
+	switch (mode) {
+	case LTR390_SET_ALS_MODE:
+		regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
+		break;
+
+	case LTR390_SET_UVS_MODE:
+		regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
+		break;
+	}
+
+	data->mode = mode;
+	return 0;
+}
+
 static int ltr390_read_raw(struct iio_dev *iio_device,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
@@ -104,15 +129,56 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 	guard(mutex)(&data->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = ltr390_register_read(data, LTR390_UVS_DATA);
-		if (ret < 0)
-			return ret;
+		switch (chan->type) {
+		case IIO_UVINDEX:
+			ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+			if (ret < 0)
+				return ret;
+
+			ret = ltr390_register_read(data, LTR390_UVS_DATA);
+			if (ret < 0)
+				return ret;
+			break;
+
+		case IIO_INTENSITY:
+			ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+			if (ret < 0)
+				return ret;
+
+			ret = ltr390_register_read(data, LTR390_ALS_DATA);
+			if (ret < 0)
+				return ret;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+
 		*val = ret;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		*val = LTR390_WINDOW_FACTOR;
-		*val2 = LTR390_COUNTS_PER_UVI;
-		return IIO_VAL_FRACTIONAL;
+		switch (chan->type) {
+		case IIO_UVINDEX:
+			ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
+			if (ret < 0)
+				return ret;
+
+			*val = LTR390_WINDOW_FACTOR;
+			*val2 = LTR390_COUNTS_PER_UVI;
+			return IIO_VAL_FRACTIONAL;
+
+		case IIO_INTENSITY:
+			ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+			if (ret < 0)
+				return ret;
+
+			*val = LTR390_WINDOW_FACTOR;
+			*val2 = data->gain;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
 
 	case IIO_CHAN_INFO_INT_TIME:
 		*val = data->int_time_us;
@@ -127,11 +193,23 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 static const int ltr390_int_time_map_us[] = { 400000, 200000, 100000, 50000, 25000, 12500 };
 static const int ltr390_gain_map[] = { 1, 3, 6, 9, 18 };
 
-static const struct iio_chan_spec ltr390_channel = {
-	.type = IIO_UVINDEX,
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
-	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
-	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+static const struct iio_chan_spec ltr390_channels[] = {
+	/* UV sensor */
+	{
+		.type = IIO_UVINDEX,
+		.scan_index = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+	},
+	/* ALS sensor */
+	{
+		.type = IIO_INTENSITY,
+		.scan_index = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
+	},
 };
 
 static int ltr390_set_gain(struct ltr390_data *data, int val)
@@ -251,12 +329,14 @@ static int ltr390_probe(struct i2c_client *client)
 	data->int_time_us = 100000;
 	/* default value of gain from pg: 16 of the datasheet */
 	data->gain = 3;
+	/* default mode for ltr390 is ALS mode */
+	data->mode = LTR390_SET_ALS_MODE;
 
 	mutex_init(&data->lock);
 
 	indio_dev->info = &ltr390_info;
-	indio_dev->channels = &ltr390_channel;
-	indio_dev->num_channels = 1;
+	indio_dev->channels = ltr390_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
 	indio_dev->name = "ltr390";
 
 	ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
@@ -274,8 +354,7 @@ static int ltr390_probe(struct i2c_client *client)
 	/* Wait for the registers to reset before proceeding */
 	usleep_range(1000, 2000);
 
-	ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
-			      LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
+	ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to enable the sensor\n");
 
-- 
2.43.0

Re: [PATCH v3 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Jonathan Cameron]

On Mon, 29 Jul 2024 17:20:54 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Add new ALS channel and allow reading raw and scale values.
> Also provide gain and resolution configuration for ALS channel.
> Add automatic mode switching between the UVS and ALS channel
> based on which channel is being accessed.
> The default mode in which the sensor start is ALS mode.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
Hi Abhash,

Patch looks good but one quick question.
Why not present an IIO_LIGHT channel?  Needs to be converted
to be illuminance (with scale / offset applied) rather than IIO_INTENSITY
which we use when the frequency response is different from the requirements
to measure Lux (and the units get very vague!)

Looks like what you have here is close, but not quite the right scale
factor as not including integration time and the mysterious 0.6 on the datasheet.

If we can provide a signal scaled to illuminance that tends to be a lot
more useful for things like screen brightness control because it should
be close at least to other light sensors.

Jonathan

Re: [PATCH v3 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Abhash jha]

On Tue, Jul 30, 2024 at 1:23 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 29 Jul 2024 17:20:54 +0530
> Abhash Jha <abhashkumarjha123@gmail.com> wrote:
>
> > Add new ALS channel and allow reading raw and scale values.
> > Also provide gain and resolution configuration for ALS channel.
> > Add automatic mode switching between the UVS and ALS channel
> > based on which channel is being accessed.
> > The default mode in which the sensor start is ALS mode.
> >
> > Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> Hi Abhash,
>
> Patch looks good but one quick question.
> Why not present an IIO_LIGHT channel?  Needs to be converted
> to be illuminance (with scale / offset applied) rather than IIO_INTENSITY
> which we use when the frequency response is different from the requirements
> to measure Lux (and the units get very vague!)
>
> Looks like what you have here is close, but not quite the right scale
> factor as not including integration time and the mysterious 0.6 on the datasheet.

Yes, I just noticed it now. I will provide the integration time and
0.6 as part of the
scale calculation in the next version.

>
> If we can provide a signal scaled to illuminance that tends to be a lot
> more useful for things like screen brightness control because it should
> be close at least to other light sensors.
>
Hi Jonathan,
It did not occur to me that the IIO_LIGHT channel could be used
directly and hence I
went with the IIO_INTENSITY approach.
Yes we could provide the IIO_LIGHT channel and perform lux calculation
in the driver.
Would that mean forgoing the IIO_INTENSITY channel? Or do we keep both?

Abhash

> Jonathan

Re: [PATCH v3 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Jonathan Cameron]

On Tue, 30 Jul 2024 12:47:10 +0530
Abhash jha <abhashkumarjha123@gmail.com> wrote:

> On Tue, Jul 30, 2024 at 1:23 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 29 Jul 2024 17:20:54 +0530
> > Abhash Jha <abhashkumarjha123@gmail.com> wrote:
> >  
> > > Add new ALS channel and allow reading raw and scale values.
> > > Also provide gain and resolution configuration for ALS channel.
> > > Add automatic mode switching between the UVS and ALS channel
> > > based on which channel is being accessed.
> > > The default mode in which the sensor start is ALS mode.
> > >
> > > Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>  
> > Hi Abhash,
> >
> > Patch looks good but one quick question.
> > Why not present an IIO_LIGHT channel?  Needs to be converted
> > to be illuminance (with scale / offset applied) rather than IIO_INTENSITY
> > which we use when the frequency response is different from the requirements
> > to measure Lux (and the units get very vague!)
> >
> > Looks like what you have here is close, but not quite the right scale
> > factor as not including integration time and the mysterious 0.6 on the datasheet.  
> 
> Yes, I just noticed it now. I will provide the integration time and
> 0.6 as part of the
> scale calculation in the next version.
> 
> >
> > If we can provide a signal scaled to illuminance that tends to be a lot
> > more useful for things like screen brightness control because it should
> > be close at least to other light sensors.
> >  
> Hi Jonathan,
> It did not occur to me that the IIO_LIGHT channel could be used
> directly and hence I
> went with the IIO_INTENSITY approach.
> Yes we could provide the IIO_LIGHT channel and perform lux calculation
> in the driver.
> Would that mean forgoing the IIO_INTENSITY channel? Or do we keep both?

Expose the scaling as _scale for the light channel and don't expose
intensity (as it will be the _raw value anyway).

It's rare to see a linear function for intensity to lux transform but
I think there are one or two others like this.  Mostly the transform
is nonlinear and involves multiple intensity channels which is why
we normally have those and IIO_LIGHT.

Thanks,

Jonathan

> 
> Abhash
> 
> > Jonathan  

Re: [PATCH v3 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Abhash jha]

> Expose the scaling as _scale for the light channel and don't expose
> intensity (as it will be the _raw value anyway).
>
> It's rare to see a linear function for intensity to lux transform but
> I think there are one or two others like this.  Mostly the transform
> is nonlinear and involves multiple intensity channels which is why
> we normally have those and IIO_LIGHT.
> Thanks,
>
> Jonathan

Thank you for your comments, I have made the changes and sent a patch v5.
Link of v5 : https://lore.kernel.org/linux-iio/20240731063706.25412-1-abhashkumarjha123@gmail.com/T/#t

Regards,
Abhash

[PATCH v3 3/3] iio: light: ltr390: Calculate 'counts_per_uvi' dynamically

[Abhash Jha]

counts_per_uvi depends on the current value of gain and resolution.
Hence, we cannot use the hardcoded value 96.
The `counts_per_uvi` function gives the count based on the current gain
and resolution (integration time).

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

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 79f5a516a..dacbe9f62 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -45,6 +45,8 @@
 #define LTR390_UVS_MODE	      BIT(3)
 #define LTR390_SENSOR_ENABLE  BIT(1)
 
+#define LTR390_FRACTIONAL_PRECISION 100
+
 /*
  * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
  * the sensor are equal to 1 UV Index [Datasheet Page#8].
@@ -119,6 +121,16 @@ static int ltr390_set_mode(struct ltr390_data *data, enum ltr390_mode mode)
 	return 0;
 }
 
+static int ltr390_counts_per_uvi(struct ltr390_data *data)
+{
+	int orig_gain = 18;
+	int orig_int_time = 400;
+	int divisor = orig_gain * orig_int_time;
+	int gain = data->gain;
+
+	return DIV_ROUND_CLOSEST(23 * gain * data->int_time_us, 10 * divisor);
+}
+
 static int ltr390_read_raw(struct iio_dev *iio_device,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
@@ -163,8 +175,8 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 			if (ret < 0)
 				return ret;
 
-			*val = LTR390_WINDOW_FACTOR;
-			*val2 = LTR390_COUNTS_PER_UVI;
+			*val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
+			*val2 = ltr390_counts_per_uvi(data);
 			return IIO_VAL_FRACTIONAL;
 
 		case IIO_INTENSITY:
-- 
2.43.0