diff mbox series

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

Message ID 20240729115056.355466-3-abhashkumarjha123@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series Minor cleanups and better error handling | expand

Commit Message

Abhash Jha July 29, 2024, 11:50 a.m. UTC 
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(-)

Comments

Jonathan Cameron July 29, 2024, 7:53 p.m. UTC | #1 
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
Abhash Jha July 30, 2024, 7:17 a.m. UTC | #2 
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
Jonathan Cameron July 30, 2024, 6:21 p.m. UTC | #3 
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
Abhash Jha Aug. 3, 2024, 5:17 a.m. UTC | #4 
> 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
diff mbox series

Patch

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");