Re: [PATCH v2 3/3] iio: light: ltr390: Calculate 'counts_per_uvi' dynamically

[Jonathan Cameron]

On Sun, 28 Jul 2024 20:49:56 +0530
Abhash Jha <abhashkumarjha123@xxxxxxxxx> wrote:

>     counts_per_uvi depends on the current value of gain and
>     resolution. Hence we cannot use the hardcoded value of 96.
>     The `counts_per_uvi` function gives the count based on the
>     current gain and resolution (integration time).
Fix the indent of this description and rewrap to take advantage of up
to 75 chars.

Some unrelated changes in this patch make it look like a lot more than
the relatively small 'real' change.  Make sure those are gone
for v3.  If you feel like adding a patch 4 that makes the white
space cleanup, then that is fine as well.

> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@xxxxxxxxx>
> ---
>  drivers/iio/light/ltr390.c | 41 ++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index d1a259141..aceb97e3d 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -45,6 +45,8 @@
>  #define LTR390_UVS_MODE	      BIT(3)
>  #define LTR390_SENSOR_ENABLE  BIT(1)
>  
> +#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].
> @@ -122,6 +124,19 @@ static int ltr390_set_mode(struct ltr390_data *data, int mode)
>  	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 DIV_ROUND_CLOSEST()

I assume maths is done like this to avoid overflow? e.g. why not

	return DIV_ROUND_CLOSEST(23 * gain * data->int_time_us, 10 * divisor)

which I think is the same 'maths' as long as 23 * gain * data->int_time_us < 2^31 - 1

If you are avoiding overflow, then add a comment on that so it doesn't get
accidentally broken by a future simplification.



> +
> +	return uvi;
> +}
> +
>  static int ltr390_read_raw(struct iio_dev *iio_device,
>  			   struct iio_chan_spec const *chan, int *val,
>  			   int *val2, long mask)
> @@ -167,8 +182,8 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  			if (ret < 0)
>  				return ret;
>  
> -			*val = LTR390_WINDOW_FACTOR;
> -			*val2 = LTR390_COUNTS_PER_UVI;
> +			*val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
> +			*val2 = ltr390_counts_per_uvi(data);
>  			return IIO_VAL_FRACTIONAL;
>  
>  		case IIO_INTENSITY:
> @@ -202,17 +217,21 @@ 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_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_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +							BIT(IIO_CHAN_INFO_SCALE)
>  	},
>  	/* ALS sensor */
>  	{
>  		.type = IIO_INTENSITY,
>  		.scan_index = 1,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.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_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +							BIT(IIO_CHAN_INFO_SCALE)
All unrelated chagnes.

>  	},
>  };
>  
> @@ -261,8 +280,9 @@ static int ltr390_set_int_time(struct ltr390_data *data, int val)
>  	return 0;
>  }
>  
> -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)
> +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)
Unrelated change.

>  {
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -280,8 +300,9 @@ static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec con
>  	}
>  }
>  
> -static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> -				int val, int val2, long mask)
> +static int ltr390_write_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int val, int val2, long mask)
Unrelted change. Get rid of this for v3.

>  {
>  	struct ltr390_data *data = iio_priv(indio_dev);
>