Re: [PATCH 1/3] iio: light: vl6180: Add configurable inter-measurement period support

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  4 Oct 2024 20:31:46 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

> Expose the IIO_CHAN_INFO_SAMP_FREQ attribute as a way to configure the
> inter-measurement period for both the IIO_DISTANCE and IIO_LIGHT
> channels. The inter-measurement period must be given in miliseconds.

Hi Abhash,

Sampling frequency must be in Hz and reflect how often the channel
is sampled (not just the inter measurement period.  So this sounds wrong.
It is sometimes complex to compute but we have to stick to the documented
ABI.

Other comments inline.

Thanks

Jonathan

> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>


> @@ -412,11 +430,22 @@ static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
>  	return ret;
>  }
>  
> +static int vl6180_meas_reg_val_from_ms(unsigned int period)
> +{
> +	unsigned int reg_val = 0;
> +
> +	if (period > 10)
> +		reg_val = period < 2550 ? (DIV_ROUND_CLOSEST(period, 10) - 1) : 254;
> +
> +	return reg_val;
> +}
> +
>  static int vl6180_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
>  	struct vl6180_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
> @@ -427,6 +456,24 @@ static int vl6180_write_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  
>  		return vl6180_set_als_gain(data, val, val2);
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
	{

needed to define scope for that guard as you probably intend.

> +		guard(mutex)(&data->lock);
> +		switch (chan->type) {
> +		case IIO_DISTANCE:
> +			data->range_meas_rate = val;
> +			reg_val = vl6180_meas_reg_val_from_ms(val);
> +			return vl6180_write_byte(data->client, VL6180_RANGE_INTER_MEAS_TIME, reg_val);

long lines. I don't mind going over 80 chars when readability is badly hurt, but
in this case it isn't so wrap the parameters.

> +
> +		case IIO_LIGHT:
> +			data->als_meas_rate = val;
> +			reg_val = vl6180_meas_reg_val_from_ms(val);
> +			return vl6180_write_byte(data->client, VL6180_ALS_INTER_MEAS_TIME, reg_val);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -473,6 +520,22 @@ static int vl6180_init(struct vl6180_data *data)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Default Range inter-measurement time: 50ms

As below.

Even though you now it in advance, I'd rather you used the vl6180_meas_reg_val_from_ms()
subject to the whole thing about it needing to be in Hz

> +	 * reg_val = (50 / 10 - 1) = 4
> +	 */
> +	ret = vl6180_write_byte(client, VL6180_RANGE_INTER_MEAS_TIME, 4);
> +	if (ret < 0)
> +		return ret;
> +	data->range_meas_rate = 50;
> +
> +	/* Default ALS inter-measurement time: 10ms
Multiline comment syntax in IIO (and most of the rest of the kernel)
is 
	/*
	 * Default ...

> +	 * reg_val = (10 / 10 - 1) = 0
> +	 */
> +	ret = vl6180_write_byte(client, VL6180_ALS_INTER_MEAS_TIME, 0);
> +	if (ret < 0)
> +		return ret;
> +	data->als_meas_rate = 10;
> +
>  	/* ALS integration time: 100ms */
>  	data->als_it_ms = 100;
>  	ret = vl6180_write_word(client, VL6180_ALS_IT, VL6180_ALS_IT_100);