[PATCH v5 0/3] Replaced IIO_INTENSITY channel with IIO_LIGHT

[PATCH v5 0/3] Replaced IIO_INTENSITY channel with IIO_LIGHT

[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 v5:
- Replaced the IIO_INTENSITY channel with IIO_LIGHT channel
- We calculate the lux value directly using `als_data / (gain * int_time)`
- Provided a scale channel where the scale is 0.6 * WINDOW_FACTOR
- Link to v4: https://lore.kernel.org/linux-iio/20240730065822.5707-1-abhashkumarjha123@gmail.com/T/#m

Changes in v4:
- Added "bitfield.h" include to fix `-Wimplicit-function-declaration`.
- Link to v3: https://lore.kernel.org/linux-iio/20240729115056.355466-1-abhashkumarjha123@gmail.com/

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 | 238 ++++++++++++++++++++++++++++++++++---
 1 file changed, 220 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH v5 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 | 136 ++++++++++++++++++++++++++++++++++---
 1 file changed, 125 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index fff1e8990..ee3d30075 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -23,21 +23,29 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
+#include <linux/bitfield.h>
 
 #include <linux/iio/iio.h>
 
 #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 +68,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 +85,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 +102,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 +114,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 +248,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 v5 1/3] iio: light: ltr390: Add configurable gain and resolution

[Jonathan Cameron]

On Wed, 31 Jul 2024 12:07:03 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> 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>
Look good.  To reduce what we have to cover for future versions, I'll
pick this up now.  No need to include it in v6.

Applied to the togreg branch of iio.git which is pushed out initially
as testing for 0-day to see if it can find anything we missed.

Thanks,

Jonathan

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

[Abhash Jha]

Add new ALS channel and allow reading lux 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 | 100 ++++++++++++++++++++++++++++++++-----
 1 file changed, 88 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index ee3d30075..d3ce43f20 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -63,11 +63,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;
 };
@@ -95,6 +101,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)
@@ -105,16 +130,54 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 	guard(mutex)(&data->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		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;
 		*val = ret;
 		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_SCALE:
-		*val = LTR390_WINDOW_FACTOR;
-		*val2 = LTR390_COUNTS_PER_UVI;
+
+	case IIO_CHAN_INFO_PROCESSED:
+		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;
+
+		/* Converting microseconds to miliseconds */
+		*val = 1000 * ret;
+		*val2 = data->gain * data->int_time_us;
 		return IIO_VAL_FRACTIONAL;
 
+	case IIO_CHAN_INFO_SCALE:
+		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_LIGHT:
+			ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
+			if (ret < 0)
+				return ret;
+
+			/* scale is 0.6 * WINDOW_FACTOR */
+			*val = LTR390_WINDOW_FACTOR * 6;
+			*val2 = 10;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
+
 	case IIO_CHAN_INFO_INT_TIME:
 		*val = data->int_time_us;
 		return IIO_VAL_INT;
@@ -128,11 +191,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_LIGHT,
+		.scan_index = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | 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)
@@ -252,12 +327,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);
@@ -275,8 +352,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 v5 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Jonathan Cameron]

On Wed, 31 Jul 2024 12:07:04 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Add new ALS channel and allow reading lux 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, 

I think I managed to confuse things with earlier reviews.

We want a light channel here, but because the scaling is just linear
we want to provide it as in_illuminance_raw and in_illumiance_scale,
not a processed channel.

Sometimes we have no alternative to a processed channel (typically non
linear maths) but when we can present the scaling via _scale (and
if needed _offset) then we do that.  This avoids a bunch of future
problems:
1 - Processed channels are a pain for buffered capture because they
    often need a lot more bits to store.
2 - Any events on this channel tend to require fiddly reverse lookups
    to get from processed to the raw value that needs to be written
    to the device.

Also note we very rarely provide both processed and raw for a channel.
The only exceptions are corner cases such as having to maintain ABI
that we should have never have introduced in the first place.

A few other comments inline.


Jonathan



> ---
>  drivers/iio/light/ltr390.c | 100 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 88 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index ee3d30075..d3ce43f20 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -63,11 +63,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;
>  };
> @@ -95,6 +101,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)
> @@ -105,16 +130,54 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  	guard(mutex)(&data->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		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;
>  		*val = ret;
>  		return IIO_VAL_INT;
> -	case IIO_CHAN_INFO_SCALE:
> -		*val = LTR390_WINDOW_FACTOR;
> -		*val2 = LTR390_COUNTS_PER_UVI;
> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +		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;
> +
> +		/* Converting microseconds to miliseconds */
> +		*val = 1000 * ret;
> +		*val2 = data->gain * data->int_time_us;
>  		return IIO_VAL_FRACTIONAL;

Don't provide this because userspace should be able to
establish it from RAW and SCALE

>  
> +	case IIO_CHAN_INFO_SCALE:
> +		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_LIGHT:
> +			ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> +			if (ret < 0)
> +				return ret;

Why change the mode to return a fixed scaling?  We don't need the device
to be in the right mode to return the values.


> +
> +			/* scale is 0.6 * WINDOW_FACTOR */
> +			*val = LTR390_WINDOW_FACTOR * 6;
> +			*val2 = 10;

This scale should include the 1000 and data->gain * data->int_time_s
that you are applying in the processed code above.  They are all linear
factors so should be made available to userspace to apply.
			*val = LTR390_WINDOW_FACTOR * 6 * 100;
			*val2 = data->gain * data->int_time_us;
I think. (cancelling the 10).

> +			return IIO_VAL_FRACTIONAL;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
>  	case IIO_CHAN_INFO_INT_TIME:
>  		*val = data->int_time_us;
>  		return IIO_VAL_INT;
> @@ -128,11 +191,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_LIGHT,
> +		.scan_index = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | 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)
> +	},
>  };

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

[Abhash jha]

> Hi Abhash,
>
> I think I managed to confuse things with earlier reviews.
>
> We want a light channel here, but because the scaling is just linear
> we want to provide it as in_illuminance_raw and in_illumiance_scale,
> not a processed channel.
>
> Sometimes we have no alternative to a processed channel (typically non
> linear maths) but when we can present the scaling via _scale (and
> if needed _offset) then we do that.  This avoids a bunch of future
> problems:
> 1 - Processed channels are a pain for buffered capture because they
>     often need a lot more bits to store.
> 2 - Any events on this channel tend to require fiddly reverse lookups
>     to get from processed to the raw value that needs to be written
>     to the device.
>
> Also note we very rarely provide both processed and raw for a channel.
> The only exceptions are corner cases such as having to maintain ABI
> that we should have never have introduced in the first place.
>
> A few other comments inline.
>
>
> Jonathan
>
Hi Jonathan,

I have made the changes, can you look it over and tell me if it looks good.
Link for v7:
https://lore.kernel.org/linux-iio/20240804142321.54586-1-abhashkumarjha123@gmail.com/T/#t

Thanks,
Abhash

[PATCH v5 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 d3ce43f20..84c084bec 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -46,6 +46,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].
@@ -120,6 +122,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)
@@ -160,8 +172,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_LIGHT:
-- 
2.43.0

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

[Jonathan Cameron]

On Wed, 31 Jul 2024 12:07:05 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> 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>
A few additional minor comments inline as you are doing a v6 anyway.

Thanks,

Jonathan

> ---
>  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 d3ce43f20..84c084bec 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -46,6 +46,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].
> @@ -120,6 +122,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;

add const to these two.

> +	int divisor = orig_gain * orig_int_time;
I'd use this orig_gain * orig_int_time inline.

> +	int gain = data->gain;

Use this data->gain inline.

> +
> +	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)
> @@ -160,8 +172,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_LIGHT:

Re: [PATCH v5 0/3] Replaced IIO_INTENSITY channel with IIO_LIGHT

[Jonathan Cameron]

On Wed, 31 Jul 2024 12:07:02 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Hello,
> 
Don't change the cover letter title between versions.
It makes them hard to track.  That title should not reflect changes
form previous version and it should always tell us which driver
the modifications are being made to!

> 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 v5:
> - Replaced the IIO_INTENSITY channel with IIO_LIGHT channel
> - We calculate the lux value directly using `als_data / (gain * int_time)`
> - Provided a scale channel where the scale is 0.6 * WINDOW_FACTOR
> - Link to v4: https://lore.kernel.org/linux-iio/20240730065822.5707-1-abhashkumarjha123@gmail.com/T/#m
> 
> Changes in v4:
> - Added "bitfield.h" include to fix `-Wimplicit-function-declaration`.
> - Link to v3: https://lore.kernel.org/linux-iio/20240729115056.355466-1-abhashkumarjha123@gmail.com/
> 
> 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 | 238 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 220 insertions(+), 18 deletions(-)
> 

Re: [PATCH v5 0/3] Replaced IIO_INTENSITY channel with IIO_LIGHT

[Abhash jha]

On Sat, Aug 3, 2024 at 7:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 31 Jul 2024 12:07:02 +0530
> Abhash Jha <abhashkumarjha123@gmail.com> wrote:
>
> > Hello,
> >
> Don't change the cover letter title between versions.
> It makes them hard to track.  That title should not reflect changes
> form previous version and it should always tell us which driver
> the modifications are being made to!
>
Thanks, I will make sure of this from now on.