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

From: Jonathan Cameron <jic23@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: gts-helpers: Round gains and scales
Date: Mon, 4 Dec 2023 14:30:05 +0000	[thread overview]
Message-ID: <20231204143005.7a564326@jic23-huawei> (raw)
In-Reply-To: <8934d9ec-e969-4662-b220-9fb1cbeca7b2@gmail.com>

On Mon, 27 Nov 2023 09:48:08 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 11/26/23 19:26, Jonathan Cameron wrote:
> > 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.  
> 
> I appreciate the questions :) Thanks!

I found some emails hiding so late replies...
> >   
> >>
> >> ---
> >> 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?  
> 
> The rationale for doing is it in this way is to prevent (theoretical?) 
> overflow when adding scale/2 to full. Maybe this warrants adding a comment?

Hmm. Chances are very low of hitting that.  I'd just go with adding scale/2
before the div.  If you really want to worry about being right at the edge
of available precision, then add a check for that.

> 
> >   
> >> +	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?  
> 
> I am pretty sure I have been thinking of rounding 0.5 to 1.

Not sure I follow - but maybe it'll be clear in v2.

> >   
> >> +
> >> +	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?  
> 
> Only if we don't care about the case where tmp + total_gain/2 overflows.

As above. The cases where that happens are pretty narrow.  I'd not worry about it
or I'd check for that overflow.

> 
> >   
> >> +	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.  
> 
> Yes. I'll drop this.
> 
> >   
> >>   		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  
> 
> All in all, I am still not 100% sure if rounding is the right ambition. 
> Do we cause hidden accuracy issues by doing the rounding under the hood? 
> I feel I need bigger brains :)
Don't we all!

Jonathan

> 
> Yours,
> 	-- Matti
> 
> 