Re: [PATCH] iio: gts-helpers: Round gains and scales

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 31 Oct 2023 11:50:46 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The GTS helpers do flooring of scale when calculating available scales.
> This results available-scales to be reported smaller than they should
> when the division in scale computation resulted remainder greater than
> half of the divider. (decimal part of result > 0.5)
> 
> Furthermore, when gains are computed based on scale, the gain resulting
> from the scale computation is also floored. As a consequence the
> floored scales reported by available scales may not match the gains that
> can be set.
> 
> The related discussion can be found from:
> https://lore.kernel.org/all/84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@gmail.com/
> 
> Do rounding when computing scales and gains.
> 
> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Hi Matti,

A few questions inline about the maths.

> 
> ---
> Subjahit, is there any chance you test this patch with your driver? Can
> you drop the:
> 	if (val2 % 10)
> 		val2 += 1;
> from scale setting and do you see written and read scales matching?
> 
> I did run a few Kunit tests on this change - but I'm still a bit jumpy
> on it... Reviewing/testing is highly appreciated!
> 
> Just in case someone is interested in seeing the Kunit tests, they're
> somewhat unpolished & crude and can emit noisy debug prints - but can
> anyways be found from:
> https://github.com/M-Vaittinen/linux/commits/iio-gts-helpers-test-v6.6
> 
> ---
>  drivers/iio/industrialio-gts-helper.c | 58 +++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 7653261d2dc2..7dc144ac10c8 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -18,6 +18,32 @@
>  #include <linux/iio/iio-gts-helper.h>
>  #include <linux/iio/types.h>
>  
> +static int iio_gts_get_gain_32(u64 full, unsigned int scale)
> +{
> +	unsigned int full32 = (unsigned int) full;
> +	unsigned int rem;
> +	int result;
> +
> +	if (full == (u64)full32) {
> +		unsigned int rem;
> +
> +		result = full32 / scale;
> +		rem = full32 - scale * result;
> +		if (rem >= scale / 2)
> +			result++;
> +
> +		return result;
> +	}
> +
> +	rem = do_div(full, scale);

As below, can we just add scale/2 to full in the do_div?

> +	if ((u64)rem >= scale / 2)
> +		result = full + 1;
> +	else
> +		result = full;
> +
> +	return result;
> +}
> +
>  /**
>   * iio_gts_get_gain - Convert scale to total gain
>   *
> @@ -28,30 +54,42 @@
>   *		scale is 64 100 000 000.
>   * @scale:	Linearized scale to compute the gain for.
>   *
> - * Return:	(floored) gain corresponding to the scale. -EINVAL if scale
> + * Return:	(rounded) gain corresponding to the scale. -EINVAL if scale
>   *		is invalid.
>   */
>  static int iio_gts_get_gain(const u64 max, const u64 scale)
>  {
> -	u64 full = max;
> +	u64 full = max, half_div;
> +	unsigned int scale32 = (unsigned int) scale;
>  	int tmp = 1;
>  
> -	if (scale > full || !scale)
> +	if (scale / 2 > full || !scale)

Seems odd. Why are we checking scale / 2 here?

>  		return -EINVAL;
>  
> +	/*
> +	 * The loop-based implementation below will potentially run _long_
> +	 * if we have a small scale and large 'max' - which may be needed when
> +	 * GTS is used for channels returning specific units. Luckily we can
> +	 * avoid the loop when scale is small and fits in 32 bits.
> +	 */
> +	if ((u64)scale32 == scale)
> +		return iio_gts_get_gain_32(full, scale32);
> +
>  	if (U64_MAX - full < scale) {
>  		/* Risk of overflow */
> -		if (full - scale < scale)
> +		if (full - scale / 2 < scale)
>  			return 1;
>  
>  		full -= scale;
>  		tmp++;
>  	}
>  
> -	while (full > scale * (u64)tmp)
> +	half_div = scale >> 2;

Why divide by 4?  Looks like classic issue with using shifts for division
causing confusion.

> +
> +	while (full + half_div >= scale * (u64)tmp)
>  		tmp++;
>  
> -	return tmp;
> +	return tmp - 1;
>  }
>  
>  /**
> @@ -133,6 +171,7 @@ static int iio_gts_linearize(int scale_whole, int scale_nano,
>   * Convert the total gain value to scale. NOTE: This does not separate gain
>   * generated by HW-gain or integration time. It is up to caller to decide what
>   * part of the total gain is due to integration time and what due to HW-gain.
> + * Computed gain is rounded to nearest integer.
>   *
>   * Return: 0 on success. Negative errno on failure.
>   */
> @@ -140,10 +179,13 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
>  				int *scale_int, int *scale_nano)
>  {
>  	u64 tmp;
> +	int rem;
>  
>  	tmp = gts->max_scale;
>  
> -	do_div(tmp, total_gain);
> +	rem = do_div(tmp, total_gain);

can we do usual trick of
do_div(tmp + total_gain/2, total_gain)
to get the same rounding effect?

> +	if (total_gain > 1 && rem >= total_gain / 2)
> +		tmp += 1ULL;
>  
>  	return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
>  }
> @@ -192,7 +234,7 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>  		sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
>  		     NULL);
>  
> -		/* Convert gains to scales */
> +		/* Convert gains to scales. */

Grumble - unrelated change.

>  		for (j = 0; j < gts->num_hwgain; j++) {
>  			ret = iio_gts_total_gain_to_scale(gts, gains[i][j],
>  							  &scales[i][2 * j],
> 
> base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa