| Message ID | 20240718104947.7384-1-abhashkumarjha123@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: ltr390: Add configurable gain, resolution and ALS reading | expand |
On Thu, 18 Jul 2024 16:19:45 +0530 Abhash Jha <abhashkumarjha123@gmail.com> wrote: > 1) Add support for configuring the gain and resolution(integration time) > for the sensor. > 2) Add a channel for ALS and provide support for reading the raw and > scale values. > 3) Add automatic mode switching between UVS and ALS based on the > channel type. > 4) Calculate 'counts_per_uvi' based on the current gain and integration > time. Hi Abhash, When a patch lists more than one thing, key thing to think is "maybe this should be multiple patches?" Here at very least separate resolution / gain into one or two patches and the new channel support into another. Probably yet another patch for point 4, Various other comments inline. Jonathan > > Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com> > --- > drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 238 insertions(+), 18 deletions(-) > > diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c > index fff1e8990..56f3c74ae 100644 > --- a/drivers/iio/light/ltr390.c > +++ b/drivers/iio/light/ltr390.c > @@ -25,19 +25,33 @@ > #include <linux/regmap.h> > > #include <linux/iio/iio.h> > - > +#include <linux/iio/sysfs.h> > #include <asm/unaligned.h> > > #define LTR390_MAIN_CTRL 0x00 > #define LTR390_PART_ID 0x06 > #define LTR390_UVS_DATA 0x10 > > +#define LTR390_ALS_DATA 0x0D > +#define LTR390_ALS_UVS_GAIN 0x05 > +#define LTR390_ALS_UVS_MEAS_RATE 0x04 > +#define LTR390_INT_CFG 0x19 If these are register addresses put them in numeric order so it is easy to compare with a datasheet table > + > #define LTR390_SW_RESET BIT(4) > #define LTR390_UVS_MODE BIT(3) > #define LTR390_SENSOR_ENABLE BIT(1) > > #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_MASK_SHIFT 4 Used FIELD_GET() and FIELD_PREP() then you never need a separate SHIFT defintion. > + > +#define LTR390_SET_ALS_MODE 1 > +#define LTR390_SET_UVS_MODE 2 If these are being use to pick options and not writen to hw use an enum. I don't think we care what value they take. > + > +#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]. > @@ -60,6 +74,9 @@ struct ltr390_data { > struct i2c_client *client; > /* Protects device from simulataneous reads */ > struct mutex lock; > + int mode; > + int gain; > + int int_time_us; > }; > > static const struct regmap_config ltr390_regmap_config = { > @@ -87,36 +104,232 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address) > return get_unaligned_le24(recieve_buffer); > } > > + one blank line is neough. > +static int ltr390_set_mode(struct ltr390_data *data, int mode) As suggested above, use an enum for mode. Give than a type name and you can use that here. > +{ > + if (data->mode == mode) > + return 0; > + > + if (mode == LTR390_SET_ALS_MODE) { > + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE); > + data->mode = LTR390_SET_ALS_MODE; > + } else if (mode == LTR390_SET_UVS_MODE) { > + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE); > + data->mode = LTR390_SET_UVS_MODE; Drop this out of the if / else stack and use data->mode = mode; A switch statement may be more appropriate here even if it's a few more lines of code. > + } else { > + return -EINVAL; > + } > + > + 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; > + > + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000); > + int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor); Spaces around * > + > + return uvi; > +} > + > static int ltr390_read_raw(struct iio_dev *iio_device, > struct iio_chan_spec const *chan, int *val, > int *val2, long mask) > { > - int ret; > struct ltr390_data *data = iio_priv(iio_device); > + int ret; Don't move code unless there is a strong reason. Fine to tidy this sort of thing up, but not in a patch doing anything else as it becomes noise. > Almost certainly need locking here as concurrent accesses to sysfs files will result in mode changing whilst the read has not yet happened. > 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); Fix the alignment - looks like mix of spaces and tabs. scripts/checkpatch.pl would have pointed that out. > + 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: > + ret = -EINVAL; return here. Otherwise you overwrite the value below. > + } > + > *val = ret; > - return IIO_VAL_INT; > + ret = IIO_VAL_INT; return here and drop the break. It is much simpler to follow code if it doesn't unnecessarily not return in cases like this as we have to scroll down to see if anything else happens. > + break; > + > case IIO_CHAN_INFO_SCALE: > - *val = LTR390_WINDOW_FACTOR; > - *val2 = LTR390_COUNTS_PER_UVI; > - return IIO_VAL_FRACTIONAL; > + mutex_lock(&data->lock); Add appropriate scope using {} and use guard(mutex)(&data->lock) as then in error paths you can return without unlocking... > + > + switch (chan->type) { > + case IIO_UVINDEX: > + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE); > + if (ret < 0) mutex held. Result is deadlock. Above scoped unlocking avoids that without needing to make sure you unlock in all paths. > + return ret; > + > + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION; > + *val2 = ltr390_counts_per_uvi(data); > + ret = IIO_VAL_FRACTIONAL; return here. > + break; > + > + case IIO_INTENSITY: > + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE); > + if (ret < 0) > + return ret; > + > + *val = LTR390_WINDOW_FACTOR; > + *val2 = data->gain; > + > + ret = IIO_VAL_FRACTIONAL; > + break; return here. > + > + default: > + ret = -EINVAL; return here. > + } > + > + mutex_unlock(&data->lock); With guard() change above, not needed. But close scope here with } > + break; > + > + case IIO_CHAN_INFO_INT_TIME: > + mutex_lock(&data->lock); Given all paths other than invalid ones need the lock, maybe just take it outside of the switch statement - still use guard() though to avoid need to manually unlock. > + *val = data->int_time_us; > + mutex_unlock(&data->lock); > + ret = IIO_VAL_INT; > + break; > + > default: > - return -EINVAL; > + ret = -EINVAL; > } > + > + return ret; This is a bad change as now I need to read to end of function in all code paths. Some code styles insist on single exit points, but the kernel style does not. (not worth a long discussion of why the two common styles came about). Keep those early returns. > } > > -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 IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500"); Please use read_avail() callback and the appropriate mask to provide this. That enables it to be used from in kernel consumers and enforces the ABI without a reviewer having to check what you have aligns. > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18"); Given we don't have a 'gain' control, what is the available applying to? > + > +static struct attribute *ltr390_attributes[] = { > + &iio_const_attr_integration_time_available.dev_attr.attr, > + &iio_const_attr_gain_available.dev_attr.attr, > + NULL, > +}; > + > +static const struct attribute_group ltr390_attribute_group = { > + .attrs = ltr390_attributes, > }; > > -static const struct iio_chan_spec ltr390_channel = { > +static const struct iio_chan_spec ltr390_channels[] = { > + /* UV sensor */ > + { > .type = IIO_UVINDEX, > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) > + .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) Fix style. { .type = ... > + }, > + /* 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) > + }, > +}; > + > +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) { > + mutex_lock(&data->lock); guard here. > + ret = regmap_update_bits(data->regmap, > + LTR390_ALS_UVS_GAIN, > + LTR390_ALS_UVS_GAIN_MASK, idx); > + if (!ret) if (ret) return ret; prefer to keep error paths as the out of line ones as if you review a lot of code, predictability helps review quickly. > + data->gain = ltr390_gain_map[idx]; > + > + mutex_unlock(&data->lock); > + break; > + } > + } > + > + return 0; > +} > + > +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) { flip logic to reduce indent. if (ltr390_int_time_map_us[idx] != val) continue; guard(mutex)... > + mutex_lock(&data->lock); > + ret = regmap_update_bits(data->regmap, > + LTR390_ALS_UVS_MEAS_RATE, > + LTR390_ALS_UVS_INT_TIME_MASK, > + idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT); spaces around << Though FIELD_PREP() probably better solution. > + if (!ret) As in previous funciton. > + data->int_time_us = ltr390_int_time_map_us[idx]; > + > + mutex_unlock(&data->lock); > + break; > + } > + } > + > + return 0; > +} > + > +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); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + if (val2 != 0) > + ret = -EINVAL; > + > + ret = ltr390_set_gain(data, val); > + break; > + > + case IIO_CHAN_INFO_INT_TIME: > + if (val2 != 0) > + ret = -EINVAL; > + > + ret = ltr390_set_int_time(data, val); > + break; > + > + default: > + ret = -EINVAL; > + } > + > + return ret; Use early returns. > +} > + > +static const struct iio_info ltr390_info = { > + .attrs = <r390_attribute_group, > + .read_raw = ltr390_read_raw, > + .write_raw = ltr390_write_raw, > }; > > static int ltr390_probe(struct i2c_client *client) > @@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client) > "regmap initialization failed\n"); > > data->client = client; > + /* default value of int time from pg: 15 of the datasheet */ I'd spell out integration in the comment. > + 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); > @@ -161,8 +381,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"); > > @@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = { > .probe = ltr390_probe, > .id_table = ltr390_id, > }; > + Lack of space is intentional to keep the macro closely coupled to what it applies to. > module_i2c_driver(ltr390_driver); > > MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
On Sat, Jul 20, 2024 at 9:26 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 18 Jul 2024 16:19:45 +0530 > Abhash Jha <abhashkumarjha123@gmail.com> wrote: > > > 1) Add support for configuring the gain and resolution(integration time) > > for the sensor. > > 2) Add a channel for ALS and provide support for reading the raw and > > scale values. > > 3) Add automatic mode switching between UVS and ALS based on the > > channel type. > > 4) Calculate 'counts_per_uvi' based on the current gain and integration > > time. > > Hi Abhash, > > When a patch lists more than one thing, key thing to think is > "maybe this should be multiple patches?" > > Here at very least separate resolution / gain into one or two patches > and the new channel support into another. > Probably yet another patch for point 4, > > Various other comments inline. > > Jonathan > > > > > Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com> > > --- > > drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++--- > > 1 file changed, 238 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c > > index fff1e8990..56f3c74ae 100644 > > --- a/drivers/iio/light/ltr390.c > > +++ b/drivers/iio/light/ltr390.c > > @@ -25,19 +25,33 @@ > > #include <linux/regmap.h> > > > > #include <linux/iio/iio.h> > > - > > +#include <linux/iio/sysfs.h> > > > #include <asm/unaligned.h> > > > > #define LTR390_MAIN_CTRL 0x00 > > #define LTR390_PART_ID 0x06 > > #define LTR390_UVS_DATA 0x10 > > > > +#define LTR390_ALS_DATA 0x0D > > +#define LTR390_ALS_UVS_GAIN 0x05 > > +#define LTR390_ALS_UVS_MEAS_RATE 0x04 > > +#define LTR390_INT_CFG 0x19 > If these are register addresses put them in numeric order so > it is easy to compare with a datasheet table > > > + > > #define LTR390_SW_RESET BIT(4) > > #define LTR390_UVS_MODE BIT(3) > > #define LTR390_SENSOR_ENABLE BIT(1) > > > > #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_MASK_SHIFT 4 > > Used FIELD_GET() and FIELD_PREP() then you never > need a separate SHIFT defintion. > > > + > > +#define LTR390_SET_ALS_MODE 1 > > +#define LTR390_SET_UVS_MODE 2 > > If these are being use to pick options and not writen to hw > use an enum. I don't think we care what value they take. > > > > + > > +#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]. > > @@ -60,6 +74,9 @@ struct ltr390_data { > > struct i2c_client *client; > > /* Protects device from simulataneous reads */ > > struct mutex lock; > > + int mode; > > + int gain; > > + int int_time_us; > > }; > > > > static const struct regmap_config ltr390_regmap_config = { > > @@ -87,36 +104,232 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address) > > return get_unaligned_le24(recieve_buffer); > > } > > > > + > one blank line is neough. > > > +static int ltr390_set_mode(struct ltr390_data *data, int mode) > As suggested above, use an enum for mode. Give than a type name and you > can use that here. > > > +{ > > + if (data->mode == mode) > > + return 0; > > + > > + if (mode == LTR390_SET_ALS_MODE) { > > + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE); > > + data->mode = LTR390_SET_ALS_MODE; > > + } else if (mode == LTR390_SET_UVS_MODE) { > > + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE); > > + data->mode = LTR390_SET_UVS_MODE; > Drop this out of the if / else stack and use > data->mode = mode; > A switch statement may be more appropriate here even if it's a few more lines of > code. > > > + } else { > > + return -EINVAL; > > + } > > + > > + 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; > > + > > + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000); > > + int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor); > > Spaces around * > > > + > > + return uvi; > > +} > > + > > static int ltr390_read_raw(struct iio_dev *iio_device, > > struct iio_chan_spec const *chan, int *val, > > int *val2, long mask) > > { > > - int ret; > > struct ltr390_data *data = iio_priv(iio_device); > > + int ret; > Don't move code unless there is a strong reason. Fine to > tidy this sort of thing up, but not in a patch doing anything else > as it becomes noise. > > > > Almost certainly need locking here as concurrent accesses to sysfs > files will result in mode changing whilst the read has not yet happened. > > > 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); > Fix the alignment - looks like mix of spaces and tabs. > scripts/checkpatch.pl would have pointed that out. > > > + 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: > > + ret = -EINVAL; > return here. Otherwise you overwrite the value below. > > > + } > > + > > *val = ret; > > - return IIO_VAL_INT; > > + ret = IIO_VAL_INT; > return here and drop the break. > It is much simpler to follow code if it doesn't unnecessarily not > return in cases like this as we have to scroll down to see if anything else > happens. > > > + break; > > + > > case IIO_CHAN_INFO_SCALE: > > - *val = LTR390_WINDOW_FACTOR; > > - *val2 = LTR390_COUNTS_PER_UVI; > > - return IIO_VAL_FRACTIONAL; > > + mutex_lock(&data->lock); > Add appropriate scope using {} and use > guard(mutex)(&data->lock) as then in error paths you can > return without unlocking... > > + > > + switch (chan->type) { > > + case IIO_UVINDEX: > > + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE); > > + if (ret < 0) > mutex held. Result is deadlock. Above scoped unlocking avoids that without > needing to make sure you unlock in all paths. > > > > + return ret; > > + > > + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION; > > + *val2 = ltr390_counts_per_uvi(data); > > + ret = IIO_VAL_FRACTIONAL; > return here. > > > + break; > > + > > + case IIO_INTENSITY: > > + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE); > > + if (ret < 0) > > + return ret; > > + > > + *val = LTR390_WINDOW_FACTOR; > > + *val2 = data->gain; > > + > > + ret = IIO_VAL_FRACTIONAL; > > + break; > return here. > > + > > + default: > > + ret = -EINVAL; > return here. > > + } > > + > > + mutex_unlock(&data->lock); > With guard() change above, not needed. > But close scope here with } > > + break; > > + > > + case IIO_CHAN_INFO_INT_TIME: > > + mutex_lock(&data->lock); > Given all paths other than invalid ones need the lock, maybe just take > it outside of the switch statement - still use guard() though to avoid > need to manually unlock. > > > + *val = data->int_time_us; > > + mutex_unlock(&data->lock); > > + ret = IIO_VAL_INT; > > + break; > > + > > default: > > - return -EINVAL; > > + ret = -EINVAL; > > } > > + > > + return ret; > This is a bad change as now I need to read to end of function in all > code paths. Some code styles insist on single exit points, but > the kernel style does not. (not worth a long discussion of why the > two common styles came about). Keep those early returns. > > > > } > > > > -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 IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500"); > Please use read_avail() callback and the appropriate mask to provide this. > That enables it to be used from in kernel consumers and enforces the > ABI without a reviewer having to check what you have aligns. > > > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18"); > Given we don't have a 'gain' control, what is the available applying to? > The gain gets controlled by writing to the iio_info_scale attribute, we write one of the above available values. So that we can scale the raw ALS and UVI values. I could use read_avail() for this too for the IIO_INFO_SCALE channel. Should I do that? Can you elaborate more on your comment? > > + > > +static struct attribute *ltr390_attributes[] = { > > + &iio_const_attr_integration_time_available.dev_attr.attr, > > + &iio_const_attr_gain_available.dev_attr.attr, > > + NULL, > > +}; > > + > > +static const struct attribute_group ltr390_attribute_group = { > > + .attrs = ltr390_attributes, > > }; > > > > -static const struct iio_chan_spec ltr390_channel = { > > +static const struct iio_chan_spec ltr390_channels[] = { > > + /* UV sensor */ > > + { > > .type = IIO_UVINDEX, > > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) > > + .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) > Fix style. > { > .type = ... > > > + }, > > + /* 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) > > + }, > > +}; > > + > > +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) { > > + mutex_lock(&data->lock); > guard here. > > + ret = regmap_update_bits(data->regmap, > > + LTR390_ALS_UVS_GAIN, > > + LTR390_ALS_UVS_GAIN_MASK, idx); > > + if (!ret) > if (ret) > return ret; > prefer to keep error paths as the out of line ones as if you review > a lot of code, predictability helps review quickly. > > > + data->gain = ltr390_gain_map[idx]; > > + > > + mutex_unlock(&data->lock); > > + break; > > + } > > + } > > + > > + return 0; > > +} > > + > > +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) { > flip logic to reduce indent. > if (ltr390_int_time_map_us[idx] != val) > continue; > > guard(mutex)... > > > + mutex_lock(&data->lock); > > + ret = regmap_update_bits(data->regmap, > > + LTR390_ALS_UVS_MEAS_RATE, > > + LTR390_ALS_UVS_INT_TIME_MASK, > > + idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT); > spaces around << > Though FIELD_PREP() probably better solution. > > > + if (!ret) > As in previous funciton. > > + data->int_time_us = ltr390_int_time_map_us[idx]; > > + > > + mutex_unlock(&data->lock); > > + break; > > + } > > + } > > + > > + return 0; > > +} > > + > > +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); > > + int ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_SCALE: > > + if (val2 != 0) > > + ret = -EINVAL; > > + > > + ret = ltr390_set_gain(data, val); > > + break; > > + > > + case IIO_CHAN_INFO_INT_TIME: > > + if (val2 != 0) > > + ret = -EINVAL; > > + > > + ret = ltr390_set_int_time(data, val); > > + break; > > + > > + default: > > + ret = -EINVAL; > > + } > > + > > + return ret; > Use early returns. > > > +} > > + > > +static const struct iio_info ltr390_info = { > > + .attrs = <r390_attribute_group, > > + .read_raw = ltr390_read_raw, > > + .write_raw = ltr390_write_raw, > > }; > > > > static int ltr390_probe(struct i2c_client *client) > > @@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client) > > "regmap initialization failed\n"); > > > > data->client = client; > > + /* default value of int time from pg: 15 of the datasheet */ > I'd spell out integration in the comment. > > > + 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); > > @@ -161,8 +381,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"); > > > > @@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = { > > .probe = ltr390_probe, > > .id_table = ltr390_id, > > }; > > + > Lack of space is intentional to keep the macro closely coupled to what > it applies to. > > > module_i2c_driver(ltr390_driver); > > > > MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>"); > ACK. Will do the necessary changes and send V2 after splitting it into 4 patches (replying again because I missed replying with CC last time)
Please crop replies to only leave the section being discussed. It saves time for everyone reading the thread. > > > -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 IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500"); > > Please use read_avail() callback and the appropriate mask to provide this. > > That enables it to be used from in kernel consumers and enforces the > > ABI without a reviewer having to check what you have aligns. > > > > > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18"); > > Given we don't have a 'gain' control, what is the available applying to? > > > The gain gets controlled by writing to the iio_info_scale attribute, > we write one of the above available values. > So that we can scale the raw ALS and UVI values. I could use > read_avail() for this too for the IIO_INFO_SCALE channel. Should I do > that? Yes, it would be appropriate to provide read_avail for IIO_INFO_SCALE as that is standard ABI that userspace will have way to interpret. > Can you elaborate more on your comment? Basic rule of thumb is think very hard about whether there is an alternative if you are providing attributes directly to an IIO driver. There are a few corners where that is necessary for standard ABI around FIFOs or certain event related attributes + a few special corners for complex hardwware. None of those apply here, so read_avail callback and choosing standard ABI elements to match what you are trying to control / describe is the way to go. That's the stuff that userspace tooling knows how to use. Jonathan
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c index fff1e8990..56f3c74ae 100644 --- a/drivers/iio/light/ltr390.c +++ b/drivers/iio/light/ltr390.c @@ -25,19 +25,33 @@ #include <linux/regmap.h> #include <linux/iio/iio.h> - +#include <linux/iio/sysfs.h> #include <asm/unaligned.h> #define LTR390_MAIN_CTRL 0x00 #define LTR390_PART_ID 0x06 #define LTR390_UVS_DATA 0x10 +#define LTR390_ALS_DATA 0x0D +#define LTR390_ALS_UVS_GAIN 0x05 +#define LTR390_ALS_UVS_MEAS_RATE 0x04 +#define LTR390_INT_CFG 0x19 + #define LTR390_SW_RESET BIT(4) #define LTR390_UVS_MODE BIT(3) #define LTR390_SENSOR_ENABLE BIT(1) #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_MASK_SHIFT 4 + +#define LTR390_SET_ALS_MODE 1 +#define LTR390_SET_UVS_MODE 2 + +#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]. @@ -60,6 +74,9 @@ struct ltr390_data { struct i2c_client *client; /* Protects device from simulataneous reads */ struct mutex lock; + int mode; + int gain; + int int_time_us; }; static const struct regmap_config ltr390_regmap_config = { @@ -87,36 +104,232 @@ 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, int mode) +{ + if (data->mode == mode) + return 0; + + if (mode == LTR390_SET_ALS_MODE) { + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE); + data->mode = LTR390_SET_ALS_MODE; + } else if (mode == LTR390_SET_UVS_MODE) { + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE); + data->mode = LTR390_SET_UVS_MODE; + } else { + return -EINVAL; + } + + 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; + + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000); + int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor); + + return uvi; +} + static int ltr390_read_raw(struct iio_dev *iio_device, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { - int ret; struct ltr390_data *data = iio_priv(iio_device); + int ret; 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: + ret = -EINVAL; + } + *val = ret; - return IIO_VAL_INT; + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_SCALE: - *val = LTR390_WINDOW_FACTOR; - *val2 = LTR390_COUNTS_PER_UVI; - return IIO_VAL_FRACTIONAL; + mutex_lock(&data->lock); + + switch (chan->type) { + case IIO_UVINDEX: + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE); + if (ret < 0) + return ret; + + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION; + *val2 = ltr390_counts_per_uvi(data); + ret = IIO_VAL_FRACTIONAL; + break; + + case IIO_INTENSITY: + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE); + if (ret < 0) + return ret; + + *val = LTR390_WINDOW_FACTOR; + *val2 = data->gain; + + ret = IIO_VAL_FRACTIONAL; + break; + + default: + ret = -EINVAL; + } + + mutex_unlock(&data->lock); + break; + + case IIO_CHAN_INFO_INT_TIME: + mutex_lock(&data->lock); + *val = data->int_time_us; + mutex_unlock(&data->lock); + ret = IIO_VAL_INT; + break; + default: - return -EINVAL; + ret = -EINVAL; } + + return ret; } -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 IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500"); +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18"); + +static struct attribute *ltr390_attributes[] = { + &iio_const_attr_integration_time_available.dev_attr.attr, + &iio_const_attr_gain_available.dev_attr.attr, + NULL, +}; + +static const struct attribute_group ltr390_attribute_group = { + .attrs = ltr390_attributes, }; -static const struct iio_chan_spec ltr390_channel = { +static const struct iio_chan_spec ltr390_channels[] = { + /* UV sensor */ + { .type = IIO_UVINDEX, - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) + .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) + }, + /* 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) + }, +}; + +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) { + mutex_lock(&data->lock); + ret = regmap_update_bits(data->regmap, + LTR390_ALS_UVS_GAIN, + LTR390_ALS_UVS_GAIN_MASK, idx); + if (!ret) + data->gain = ltr390_gain_map[idx]; + + mutex_unlock(&data->lock); + break; + } + } + + return 0; +} + +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) { + mutex_lock(&data->lock); + ret = regmap_update_bits(data->regmap, + LTR390_ALS_UVS_MEAS_RATE, + LTR390_ALS_UVS_INT_TIME_MASK, + idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT); + if (!ret) + data->int_time_us = ltr390_int_time_map_us[idx]; + + mutex_unlock(&data->lock); + break; + } + } + + return 0; +} + +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); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + if (val2 != 0) + ret = -EINVAL; + + ret = ltr390_set_gain(data, val); + break; + + case IIO_CHAN_INFO_INT_TIME: + if (val2 != 0) + ret = -EINVAL; + + ret = ltr390_set_int_time(data, val); + break; + + default: + ret = -EINVAL; + } + + return ret; +} + +static const struct iio_info ltr390_info = { + .attrs = <r390_attribute_group, + .read_raw = ltr390_read_raw, + .write_raw = ltr390_write_raw, }; static int ltr390_probe(struct i2c_client *client) @@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client) "regmap initialization failed\n"); data->client = client; + /* default value of int 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; + /* 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); @@ -161,8 +381,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"); @@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = { .probe = ltr390_probe, .id_table = ltr390_id, }; + module_i2c_driver(ltr390_driver); MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
1) Add support for configuring the gain and resolution(integration time) for the sensor. 2) Add a channel for ALS and provide support for reading the raw and scale values. 3) Add automatic mode switching between UVS and ALS based on the channel type. 4) Calculate 'counts_per_uvi' based on the current gain and integration time. Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com> --- drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++--- 1 file changed, 238 insertions(+), 18 deletions(-)