diff mbox series

[v12,09/16] iio: afe: rescale: fix accuracy for small fractional scales

Message ID 20220108205319.2046348-10-liambeguin@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: afe: add temperature rescaling support | expand

Commit Message

Liam Beguin Jan. 8, 2022, 8:53 p.m. UTC 
The approximation caused by integer divisions can be costly on smaller
scale values since the decimal part is significant compared to the
integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
cases to maintain accuracy.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/iio-rescale.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Jan. 9, 2022, 1:02 p.m. UTC | #1 
On Sat, Jan 8, 2022 at 10:53 PM Liam Beguin <liambeguin@gmail.com> wrote:
>
> The approximation caused by integer divisions can be costly on smaller
> scale values since the decimal part is significant compared to the
> integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
> cases to maintain accuracy.

...

> +               tmp = 1 << *val2;

Unfortunately, potential UB is still here. I think you missed a subtle
detail in the implementation of BIT()/BIT_ULL(). Let's put it here:

  #define BIT(nr) (UL(1) << (nr))

where

  #define UL(x) (_UL(x))
  #define _UL(x) (_AC(x, UL))

For non-assembler case

  #define __AC(X,Y) (X##Y)
  #define _AC(X,Y) __AC(X,Y)

Now you may easily see the difference.

...

> +               rem2 = *val % (int)tmp;
> +               *val = *val / (int)tmp;
> +
> +               *val2 = rem / (int)tmp;

Hmm... You divide s64 by 10^9, which means that the maximum value can
be ~10^10 / 2 (because 2^64-1 ~= 10^19), but this _might_ be bigger
than 'int' can hold. Can you confirm that tmp can't be so big?
Peter Rosin Jan. 9, 2022, 8:07 p.m. UTC | #2 
Hi!

On 2022-01-09 14:02, Andy Shevchenko wrote:
> On Sat, Jan 8, 2022 at 10:53 PM Liam Beguin <liambeguin@gmail.com> wrote:
>>
>> The approximation caused by integer divisions can be costly on smaller
>> scale values since the decimal part is significant compared to the
>> integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
>> cases to maintain accuracy.
> 

...

> ...
> 
>> +               rem2 = *val % (int)tmp;
>> +               *val = *val / (int)tmp;
>> +
>> +               *val2 = rem / (int)tmp;
> 
> Hmm... You divide s64 by 10^9, which means that the maximum value can
> be ~10^10 / 2 (because 2^64-1 ~= 10^19), but this _might_ be bigger
> than 'int' can hold. Can you confirm that tmp can't be so big?
> 

If tmp is so big that it does not fit in 32-bits, that is indeed a
problem, and it means that the scale has overflowed. However, the
problem with the scale overflowing very much existed prior to this
series. Doing something about that overflow is not strictly related
to improving the accuracy for small scale factors.

Cheers,
Peter
diff mbox series

Patch

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 68eb3e341939..ed3ef994997f 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -24,7 +24,7 @@  int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
 	s64 tmp;
-	s32 rem;
+	s32 rem, rem2;
 	u32 mult;
 	u32 neg;
 
@@ -43,9 +43,23 @@  int rescale_process_scale(struct rescale *rescale, int scale_type,
 		tmp = (s64)*val * 1000000000LL;
 		tmp = div_s64(tmp, rescale->denominator);
 		tmp *= rescale->numerator;
-		tmp = div_s64(tmp, 1000000000LL);
+
+		tmp = div_s64_rem(tmp, 1000000000LL, &rem);
 		*val = tmp;
-		return scale_type;
+
+		if (!rem)
+			return scale_type;
+
+		tmp = 1 << *val2;
+
+		rem2 = *val % (int)tmp;
+		*val = *val / (int)tmp;
+
+		*val2 = rem / (int)tmp;
+		if (rem2)
+			*val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
+
+		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:
 		mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA;