Re: [PATCH v4 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support

[Peter Rosin]

On 2021-07-09 21:30, Liam Beguin wrote:
> On Fri Jul 9, 2021 at 12:29 PM EDT, Peter Rosin wrote:
>>
>>
>> On 2021-07-06 18:09, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@xxxxxxxxxx>
>>>
>>> Add IIO_VAL_INT_PLUS_{NANO,MICRO} scaling support.
>>> Scale the integer part and the decimal parts individually and keep the
>>> original scaling type.
>>>
>>> Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index ba3bdcc69b16..1d0e24145d87 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -89,7 +89,15 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>>>  			do_div(tmp, 1000000000LL);
>>>  			*val = tmp;
>>>  			return ret;
>>> +		case IIO_VAL_INT_PLUS_NANO:
>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>> +			tmp = (s64)*val * rescale->numerator;
>>> +			*val = div_s64(tmp, rescale->denominator);
>>> +			tmp = (s64)*val2 * rescale->numerator;
>>> +			*val2 = div_s64(tmp, rescale->denominator);
>>
> 
> Hi Peter,
> 
>> Hi!
>>
>> You are losing precision, and you are not mormalising after the
>> calculation.
> 
> Can you elaborate a little on what you mean here?
> 
> Do you mean that I should make sure that *val2, the PLUS_{NANO,MICRO}
> part, doesn't contain an integer part? And if so transfer that part back
> to *val?

Yes. On 32-bit, you will easily wrap, especially for PLUS_NANO. You'd
only need a scale factor of 10 or so and a fractional part above .5 to
hit the roof (10 * 500000000 > 2^32).

But I also mean that you are losing precision when you are scaling
the integer part and the fractional part separately. That deserves
at least a comment, but ideally it should be handled correctly.

>> I think it's better to not even attempt this given that the results can
>> be
>> really poor.
> 
> Unfortunatelly, I'm kinda stuck with this as some of my ADC use these
> types.

Ok. Crap. :-)

Cheers,
Peter