| Message ID | 20240731063706.25412-3-abhashkumarjha123@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Replaced IIO_INTENSITY channel with IIO_LIGHT | expand |
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) > + }, > };
> 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
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 = <r390_info; - indio_dev->channels = <r390_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");
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(-)