On Tue, 10 Sep 2024 10:20:26 +0530
Abhash Jha <abhashkumarjha123@xxxxxxxxx> 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@xxxxxxxxx>
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;
> +}
> +
