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

From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
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: Tue, 5 Dec 2023 09:10:59 +0200	[thread overview]
Message-ID: <c6088472-e68f-42a5-8c8b-1fd065e9f524@gmail.com> (raw)
In-Reply-To: <20231204143005.7a564326@jic23-huawei>

On 12/4/23 16:30, Jonathan Cameron wrote:
> 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...

Better late than never :)

To tell the truth, delays have been Ok. I think Subhajit has not needed 
this urgently and the darkness of the winter in Finland has hindered my 
energy and activity to very low levels.

>>>> ---
>>>> 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.

I think the v2 will ditch this function.
>>>> +	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.

Basically, when scale is greater than max, the division yields values 
smaller than 1. So, when we do rounding, everything equal to or greater 
than 0.5 and smaller than 1 should be rounded upwards. Eg, purely from 
computational perspective, when the "full" is half of the scale, 
division returns 0.5. Thus the check.

But I think your question is very much a valid one. By design the driver 
gives the max value - and I think that scale exceeding this maximum can 
indeed be considered to be invalid. Not that I feel 100% certain on that 
Today :)

> 
>>>    
>>>> +
>>>> +	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.

part of me says you're right while part of me screams that
1) a _division_ causing overflow is against all that is well and good.
2) if we can cope with the overflow, then we should cope with it.

I am very much undecided what is the best approach here. I'll see how 
much clarity there is in the v2 code, what comments can do and then I'll 
throw it for you to judge :)

>>
>> 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!

Well, luckily the software development can be seen as an iterative 
process :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~