Re: [PATCH v2 1/4] iio: light: ltr390: Added configurable sampling frequency support

[Jonathan Cameron]

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;
> +}
> +