| Message ID | 20240910045030.266946-2-abhashkumarjha123@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Threshold event and Sampling freq support for LTR390 | expand |
On Tue, 10 Sep 2024 10:20:26 +0530 Abhash Jha <abhashkumarjha123@gmail.com> wrote: > Provied configurable sampling frequency(Measurement rate) support. Spell check: Provide > Also exposed the available sampling frequency values using read_avail > callback. > > Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com> Hi Abhash, A few minor comments inline and an (optional) request to cleanup the mask definitions in the existing code. > --- > drivers/iio/light/ltr390.c | 68 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 66 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c > index 7e58b50f3..73ef4a5a0 100644 > --- a/drivers/iio/light/ltr390.c > +++ b/drivers/iio/light/ltr390.c > @@ -39,6 +39,7 @@ > > #define LTR390_PART_NUMBER_ID 0xb > #define LTR390_ALS_UVS_GAIN_MASK 0x07 > +#define LTR390_ALS_UVS_MEAS_RATE_MASK 0x07 These masks should be converted to GENMASK(). If you don't mind doing it a precursor patch to do so would be nice to have. However whether or not you cleanup existing mask definitions, please use GENMASK() for this new one. > #define LTR390_ALS_UVS_INT_TIME_MASK 0x70 > #define LTR390_ALS_UVS_INT_TIME(x) FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x)) > > @@ -87,6 +88,18 @@ static const struct regmap_config ltr390_regmap_config = { > .val_bits = 8, > }; > > +/* Sampling frequency is in mili Hz and mili Seconds */ > +static const int ltr390_samp_freq_table[][2] = { > + [0] = {40000, 25}, I'm trying to slowly get IIO to standardise strongly around [0] = { 4000, 25 }, etc. So space after { and before } > + [1] = {20000, 50}, > + [2] = {10000, 100}, > + [3] = {5000, 200}, > + [4] = {2000, 500}, > + [5] = {1000, 1000}, > + [6] = {500, 2000}, > + [7] = {500, 2000} Add a trailing comma. Sure we probably will never get any more entries but it isn't a terminator entry so convention is put the comma anyway. > +}; > + > static int ltr390_register_read(struct ltr390_data *data, u8 register_address) > { > struct device *dev = &data->client->dev; > @@ -135,6 +148,18 @@ static int ltr390_counts_per_uvi(struct ltr390_data *data) > return DIV_ROUND_CLOSEST(23 * data->gain * data->int_time_us, 10 * orig_gain * orig_int_time); > } > > +static int ltr390_get_samp_freq(struct ltr390_data *data) > +{ > + int ret, value; > + > + ret = regmap_read(data->regmap, LTR390_ALS_UVS_MEAS_RATE, &value); > + if (ret < 0) > + return ret; > + value &= LTR390_ALS_UVS_MEAS_RATE_MASK; FIELD_GET() preferred because then the reader doesn't have to check if this mask includes the LSB. It slightly helps review and compiler will get rid of the shift by nothing anyway. > + > + return ltr390_samp_freq_table[value][0]; > +} > + > static int ltr390_read_raw(struct iio_dev *iio_device, > struct iio_chan_spec const *chan, int *val, > int *val2, long mask) > @@ -191,6 +216,10 @@ static int ltr390_read_raw(struct iio_dev *iio_device, > *val = data->int_time_us; > return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SAMP_FREQ: > + *val = ltr390_get_samp_freq(data); > + return IIO_VAL_INT; > + > default: > return -EINVAL; > } > @@ -199,6 +228,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device, > /* 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 int ltr390_freq_map[] = { 40000, 20000, 10000, 5000, 2000, 1000, 500, 500 }; > > static const struct iio_chan_spec ltr390_channels[] = { > /* UV sensor */ > @@ -206,16 +236,18 @@ static const struct iio_chan_spec ltr390_channels[] = { > .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 = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE) > + | BIT(IIO_CHAN_INFO_SAMP_FREQ) Obviously a long line above, but | should generally be on that previous line. Probably best to reformat it as .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), Note should have always had a trailing comma. Add that whilst here. > }, > /* ALS sensor */ > { > .type = IIO_LIGHT, > .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 = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE) > + | BIT(IIO_CHAN_INFO_SAMP_FREQ) > }, > }; > > @@ -264,6 +296,27 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val) > return -EINVAL; > } > > +static int ltr390_set_samp_freq(struct ltr390_data *data, int val) > +{ > + int ret, idx; > + > + for (idx = 0; idx < ARRAY_SIZE(ltr390_samp_freq_table); idx++) { > + if (ltr390_samp_freq_table[idx][0] != val) > + continue; > + > + guard(mutex)(&data->lock); > + ret = regmap_update_bits(data->regmap, > + LTR390_ALS_UVS_MEAS_RATE, > + LTR390_ALS_UVS_MEAS_RATE_MASK, idx); return regmap_update_bits() is the same thing as what you have here. > + if (ret) > + return ret; > + > + return 0; > + } > + > + return -EINVAL; > +} > +
> Hi Abhash, > > A few minor comments inline and an (optional) request to cleanup > the mask definitions in the existing code. > > > > #define LTR390_PART_NUMBER_ID 0xb > > #define LTR390_ALS_UVS_GAIN_MASK 0x07 > > +#define LTR390_ALS_UVS_MEAS_RATE_MASK 0x07 > These masks should be converted to GENMASK(). > If you don't mind doing it a precursor patch to do so > would be nice to have. > Can I do the mask to GENMASK conversion in an additional cleanup patch at the end? The patch would clean up stuff related to newlines and such. Meanwhile I would use GENMASK for the new ones Thanks, Abhash
On Sat, 14 Sep 2024 22:57:24 +0530 Abhash jha <abhashkumarjha123@gmail.com> wrote: > > Hi Abhash, > > > > A few minor comments inline and an (optional) request to cleanup > > the mask definitions in the existing code. > > > > > > #define LTR390_PART_NUMBER_ID 0xb > > > #define LTR390_ALS_UVS_GAIN_MASK 0x07 > > > +#define LTR390_ALS_UVS_MEAS_RATE_MASK 0x07 > > These masks should be converted to GENMASK(). > > If you don't mind doing it a precursor patch to do so > > would be nice to have. > > > Can I do the mask to GENMASK conversion in an additional cleanup patch > at the end? The patch would clean up stuff related to newlines and such. Doing it at the end is fine, but remember one patch per type of cleanup. So if you have whitespace stuff and genmask, then two patches. > > Meanwhile I would use GENMASK for the new ones Perfect. Thanks, Jonathan > > Thanks, > Abhash
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c index 7e58b50f3..73ef4a5a0 100644 --- a/drivers/iio/light/ltr390.c +++ b/drivers/iio/light/ltr390.c @@ -39,6 +39,7 @@ #define LTR390_PART_NUMBER_ID 0xb #define LTR390_ALS_UVS_GAIN_MASK 0x07 +#define LTR390_ALS_UVS_MEAS_RATE_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)) @@ -87,6 +88,18 @@ static const struct regmap_config ltr390_regmap_config = { .val_bits = 8, }; +/* Sampling frequency is in mili Hz and mili Seconds */ +static const int ltr390_samp_freq_table[][2] = { + [0] = {40000, 25}, + [1] = {20000, 50}, + [2] = {10000, 100}, + [3] = {5000, 200}, + [4] = {2000, 500}, + [5] = {1000, 1000}, + [6] = {500, 2000}, + [7] = {500, 2000} +}; + static int ltr390_register_read(struct ltr390_data *data, u8 register_address) { struct device *dev = &data->client->dev; @@ -135,6 +148,18 @@ static int ltr390_counts_per_uvi(struct ltr390_data *data) return DIV_ROUND_CLOSEST(23 * data->gain * data->int_time_us, 10 * orig_gain * orig_int_time); } +static int ltr390_get_samp_freq(struct ltr390_data *data) +{ + int ret, value; + + ret = regmap_read(data->regmap, LTR390_ALS_UVS_MEAS_RATE, &value); + if (ret < 0) + return ret; + value &= LTR390_ALS_UVS_MEAS_RATE_MASK; + + return ltr390_samp_freq_table[value][0]; +} + static int ltr390_read_raw(struct iio_dev *iio_device, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -191,6 +216,10 @@ static int ltr390_read_raw(struct iio_dev *iio_device, *val = data->int_time_us; return IIO_VAL_INT; + case IIO_CHAN_INFO_SAMP_FREQ: + *val = ltr390_get_samp_freq(data); + return IIO_VAL_INT; + default: return -EINVAL; } @@ -199,6 +228,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device, /* 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 int ltr390_freq_map[] = { 40000, 20000, 10000, 5000, 2000, 1000, 500, 500 }; static const struct iio_chan_spec ltr390_channels[] = { /* UV sensor */ @@ -206,16 +236,18 @@ static const struct iio_chan_spec ltr390_channels[] = { .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 = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ), .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE) + | BIT(IIO_CHAN_INFO_SAMP_FREQ) }, /* ALS sensor */ { .type = IIO_LIGHT, .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 = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SAMP_FREQ), .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE) + | BIT(IIO_CHAN_INFO_SAMP_FREQ) }, }; @@ -264,6 +296,27 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val) return -EINVAL; } +static int ltr390_set_samp_freq(struct ltr390_data *data, int val) +{ + int ret, idx; + + for (idx = 0; idx < ARRAY_SIZE(ltr390_samp_freq_table); idx++) { + if (ltr390_samp_freq_table[idx][0] != val) + continue; + + guard(mutex)(&data->lock); + ret = regmap_update_bits(data->regmap, + LTR390_ALS_UVS_MEAS_RATE, + LTR390_ALS_UVS_MEAS_RATE_MASK, idx); + if (ret) + return ret; + + 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) { @@ -278,6 +331,11 @@ static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec con *type = IIO_VAL_INT; *vals = ltr390_int_time_map_us; return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_SAMP_FREQ: + *length = ARRAY_SIZE(ltr390_freq_map); + *type = IIO_VAL_INT; + *vals = ltr390_freq_map; + return IIO_AVAIL_LIST; default: return -EINVAL; } @@ -301,6 +359,12 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons return ltr390_set_int_time(data, val); + case IIO_CHAN_INFO_SAMP_FREQ: + if (val2 != 0) + return -EINVAL; + + return ltr390_set_samp_freq(data, val); + default: return -EINVAL; }
Provied configurable sampling frequency(Measurement rate) support. Also exposed the available sampling frequency values using read_avail callback. Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com> --- drivers/iio/light/ltr390.c | 68 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-)