Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <8934d9ec-e969-4662-b220-9fb1cbeca7b2@gmail.com>
Date:   Mon, 27 Nov 2023 09:48:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] iio: gts-helpers: Round gains and scales
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
References: <ZUDN9n8iXoNwzifQ@dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi>
 <20231126172607.379c9d79@jic23-huawei>
Content-Language: en-US, en-GB
From:   Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20231126172607.379c9d79@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

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

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

Yes. Looks like a brainfart to me. I need to fire-up my tests and revise 
this (and the check you asked about above). It seems to take a while 
from me to wrap my head around this again...

Thanks for pointing this out!

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

> 
>> +	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 :)

Yours,
	-- Matti


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

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