Re: [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale

From: Peter Rosin <peda@axentia.se>
To: Liam Beguin <liambeguin@gmail.com>,
	jic23@kernel.org, lars@metafoo.de, pmeerw@pmeerw.net
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale
Date: Wed, 28 Jul 2021 09:58:10 +0200	[thread overview]
Message-ID: <4e477a42-1cae-06f8-2778-fc734359d6f3@axentia.se> (raw)
In-Reply-To: <CD4CHX6R9QRI.2Q76MYJGTXNWK@shaak>

On 2021-07-28 02:26, Liam Beguin wrote:
> On Fri Jul 23, 2021 at 5:20 PM EDT, Peter Rosin wrote:
>> On 2021-07-21 05:06, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@xiphos.com>
>>>
>>> The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected
>>> scale. Update the case so that the rescaler returns a fractional type
>>> and a more precise scale.
>>>
>>> Signed-off-by: Liam Beguin <lvb@xiphos.com>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 9 +++------
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index 35fa3b4e53e0..47cd4a6d9aca 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>  		*val2 = rescale->denominator;
>>>  		return IIO_VAL_FRACTIONAL;
>>>  	case IIO_VAL_FRACTIONAL_LOG2:
>>> -		tmp = *val * 1000000000LL;
>>> -		do_div(tmp, rescale->denominator);
>>> -		tmp *= rescale->numerator;
>>> -		do_div(tmp, 1000000000LL);
>>> -		*val = tmp;
>>> -		return scale_type;
>>> +		*val = rescale->numerator * *val;
>>> +		*val2 = rescale->denominator * (1 << *val2);
>>> +		return IIO_VAL_FRACTIONAL;
>>
>> Hi!
> 
> Hi Peter,
> 
>>
>> I do not think this is an uncontested improvement. You have broken the
>> case
>> where *val2 is "large" before the scale factor is applied.
> 
> I was a little reluctant to add this change as I keep increasing the
> scope of this series, but since I added tests for all cases, I didn't
> want to leave this one out.

> Would you rather I drop this patch and the test cases associated to it?

Why drop the tests? Are they doing any harm? Or are they testing exactly
the problem situation that fail without this patch?

In that case, I guess fix the tests to pass and preferably add tests
for the *val2 is "large" situation (that this patch breaks) so that the
next person trying to improve precision is made aware of the overflow
problem. Does that make sense?

Cheers,
Peter

> Thanks,
> Liam
> 
>>
>> Cheers,
>> Peter
>>
>>>  	case IIO_VAL_INT_PLUS_NANO:
>>>  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>>  		tmp = div_s64(tmp, rescale->denominator);
>>>
> 