iio: afe: add temperature rescaling support

From: Liam Beguin <[email protected]>

Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types. Add support for these to allow using the iio-rescaler with them.

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

drivers/iio/afe/iio-rescale.c +3 -0

index d0669fd8eac5..8488f1d83527 100644
@@ -22,6 +22,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
 	unsigned long long tmp;
+	s32 rem;
+	u32 mult;
+	u32 neg;
 
 	switch (scale_type) {
 	case IIO_VAL_FRACTIONAL:

@@ -40,6 +43,38 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		tmp *= rescale->numerator;
 		do_div(tmp, 1000000000LL);
 		*val = tmp;

drivers/iio/afe/iio-rescale.c +22 -0

+		 */
+		neg = *val < 0 || *val2 < 0;
+
+		tmp = (s64)abs(*val) * abs(rescale->numerator);
+		*val = div_s64_rem(tmp, abs(rescale->denominator), &rem);
+
+		tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator);
+		tmp = div_s64(tmp, abs(rescale->denominator));
+
+		*val += div_s64_rem(tmp, mult, val2);
+
+		/*
+		 * If only one of the rescaler elements or the schan scale is
+		 * negative, the combined scale is negative.
+		 */
+		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
+			if (*val)
+				*val = -*val;
+			else
+				*val2 = -*val2;
+		}
+
 		return scale_type;
 	default:
 		return -EOPNOTSUPP;

Quoted 33 lines · Liam Beguin

From: Liam Beguin <[email protected]>

Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types. Add support for these to allow using the iio-rescaler with them.

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

drivers/iio/afe/iio-rescale.c +8 -0

index d0669fd8eac5..8488f1d83527 100644
@@ -22,6 +22,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
 	unsigned long long tmp;
+	s32 rem;
+	u32 mult;
+	u32 neg;
 
 	switch (scale_type) {
 	case IIO_VAL_FRACTIONAL:

@@ -40,6 +43,38 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		tmp *= rescale->numerator;
 		do_div(tmp, 1000000000LL);
 		*val = tmp;
+		return scale_type;
+	case IIO_VAL_INT_PLUS_NANO:
+	case IIO_VAL_INT_PLUS_MICRO:
+		if (scale_type == IIO_VAL_INT_PLUS_NANO)
+			mult = 1000000000LL;

drivers/iio/afe/iio-rescale.c int rescale_process_scale(struct rescale *rescale, int scale_type, · line 55

Quoted 5 lines

drivers/iio/afe/iio-rescale.c +5 -0

+		else
+			mult = 1000000LL;
+		/*
+		 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
+		 * *val2 is negative the schan scale is negative

The last line doesn't parse for me. It doesn't end with a period either, so it looks like you moved on before you finished it?

Cheers,
Peter

Quoted 26 lines

drivers/iio/afe/iio-rescale.c +22 -0

+		 */
+		neg = *val < 0 || *val2 < 0;
+
+		tmp = (s64)abs(*val) * abs(rescale->numerator);
+		*val = div_s64_rem(tmp, abs(rescale->denominator), &rem);
+
+		tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator);
+		tmp = div_s64(tmp, abs(rescale->denominator));
+
+		*val += div_s64_rem(tmp, mult, val2);
+
+		/*
+		 * If only one of the rescaler elements or the schan scale is
+		 * negative, the combined scale is negative.
+		 */
+		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
+			if (*val)
+				*val = -*val;
+			else
+				*val2 = -*val2;
+		}
+
 		return scale_type;
 	default:
 		return -EOPNOTSUPP;

From: Liam Beguin <[email protected]>

Reduce the risk of integer overflow by doing the scale calculation on a 64-bit integer. Since the rescaling is only performed on *val, reuse the IIO_VAL_FRACTIONAL_LOG2 case.

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/iio/afe/iio-rescale.c +1 -4

index 809e966f7058..c408c4057c08 100644
@@ -27,16 +27,13 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 	u32 neg;
 
 	switch (scale_type) {
-	case IIO_VAL_FRACTIONAL:
-		*val *= rescale->numerator;
-		*val2 *= rescale->denominator;
-		return scale_type;
 	case IIO_VAL_INT:
 		*val *= rescale->numerator;
 		if (rescale->denominator == 1)
 			return scale_type;
 		*val2 = rescale->denominator;
 		return IIO_VAL_FRACTIONAL;
+	case IIO_VAL_FRACTIONAL:
 	case IIO_VAL_FRACTIONAL_LOG2:
 		tmp = (s64)*val * 1000000000LL;
 		tmp = div_s64(tmp, rescale->denominator);

Quoted 5 lines · Liam Beguin

From: Liam Beguin <[email protected]>

Reduce the risk of integer overflow by doing the scale calculation on a 64-bit integer. Since the rescaling is only performed on *val, reuse the IIO_VAL_FRACTIONAL_LOG2 case.

While this patch certainly helps with overflow problems, it also potentially kills precision in some cases where there currently are no overflow issues.

E.g. this patch transforms 5/32768 scaled by 3/10000 from the exact

15 / 327680000 (0.0000000457763671875)

to the heavily truncated plain old sorry "zero".

Sure, 9/14 improves the situation, but patch 9/14 simply cannot make this example any better than returning 2 significant digits since the value is so small.

Side note, there is also the same type of risk of overflow for IIO_VAL_INT. Why does that case not get the same treatment as IIO_VAL_FRACTIONAL?

But again, I see no elegant solution. The best I can think of is the inelegant solution to provide extra info on the input range, the exact desired scaling method, the desired output type, some mix of all of the above or something else that helps determining the appropriate scaling method w/o looking at the individual number.

Cheers,
Peter

Quoted 28 lines

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/iio/afe/iio-rescale.c +1 -4

index 809e966f7058..c408c4057c08 100644
@@ -27,16 +27,13 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 	u32 neg;
 
 	switch (scale_type) {
-	case IIO_VAL_FRACTIONAL:
-		*val *= rescale->numerator;
-		*val2 *= rescale->denominator;
-		return scale_type;
 	case IIO_VAL_INT:
 		*val *= rescale->numerator;
 		if (rescale->denominator == 1)
 			return scale_type;
 		*val2 = rescale->denominator;
 		return IIO_VAL_FRACTIONAL;
+	case IIO_VAL_FRACTIONAL:
 	case IIO_VAL_FRACTIONAL_LOG2:
 		tmp = (s64)*val * 1000000000LL;
 		tmp = div_s64(tmp, rescale->denominator);

Hi Liam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78]

url:
https://github.com/0day-ci/linux/commits/Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210821-032112
base: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
config: nds32-buildonly-randconfig-r005-20210821 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/e5c2e1505fa3f8cf9fe6d3a21f3a5c585efc6dce
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210821-032112 git checkout e5c2e1505fa3f8cf9fe6d3a21f3a5c585efc6dce

# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross
O=build_dir ARCH=nds32 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

nds32le-linux-ld: drivers/iio/afe/iio-rescale.o: in function `rescale_process_scale':

Quoted 2 lines

Quoted 2 lines

iio-rescale.c:(.text+0x5f4): undefined reference to `__divdi3'
nds32le-linux-ld: iio-rescale.c:(.text+0x5f8): undefined reference to `__divdi3'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Quoted 27 lines · kernel test robot

Hi Liam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78]

url:
https://github.com/0day-ci/linux/commits/Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210821-032112
base: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
config: nds32-buildonly-randconfig-r005-20210821 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/e5c2e1505fa3f8cf9fe6d3a21f3a5c585efc6dce
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210821-032112 git checkout e5c2e1505fa3f8cf9fe6d3a21f3a5c585efc6dce

# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross
O=build_dir ARCH=nds32 SHELL=/bin/bash

Quoted 9 lines

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

nds32le-linux-ld: drivers/iio/afe/iio-rescale.o: in function `rescale_process_scale':

Quoted 2 lines

Quoted 2 lines

iio-rescale.c:(.text+0x5f4): undefined reference to `__divdi3'
nds32le-linux-ld: iio-rescale.c:(.text+0x5f8): undefined reference to `__divdi3'

My mistake, I'll replace the division by a div64_s64().

The if statement is also misaligned here. I'll fix that too.

Thanks,
Liam

Quoted 4 lines

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

From: Liam Beguin <[email protected]>

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 <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/iio/afe/iio-rescale.c +11 -2

index c408c4057c08..7304306c9806 100644
@@ -22,7 +22,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;
 

@@ -38,8 +38,31 @@ 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;
+
+		/*
+		 * For small values, the approximation can be costly,
+		 * change scale type to maintain accuracy.
+		 *
+		 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
+		 */
+		if (scale_type == IIO_VAL_FRACTIONAL)

drivers/iio/afe/iio-rescale.c +4 -0

+
+			 return IIO_VAL_INT_PLUS_NANO;
+		 }
+
 		return scale_type;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Jonathan, do you have any input on that?

Some more examples of problematic properties of this patch:

21837/24041 scaled by 427/24727 is 0.01568544672, you get 0.015685446. Ok. But if you reduce the input number, gcd(21837, 24041) -> 29, you have:
753/829 scaled by 427/24727 which still is 0.01568544672 of course, but in this case you get 0.01568154403. Which is less precise. It is unfortunate that input that should be easier to scale may yield worse results.

760/1373754273 scaled by 427/2727 is 8.662580e-8, and 8.662393e-8 is returned. Which is perhaps not great accuracy, but such is life. However. 761/1373754273 scaled by 427/2727 is 8.673978e-8, which is of course greater, but 8.6e-8 is returned. Which is less than what was returned for the smaller 760/1373754273 value above.

Some of these objections are related to what I talked about in v7, i.e.:

Also, changing the calculation so that you get more precision whenever that is possible feels dangerous. I fear linearity breaks and that bigger input cause smaller output due to rounding if the bigger value has to be rounded down, but that this isn't done carefully enough. I.e. attempting to return an exact fraction and only falling back to the old code when that is not possible is still not safe since the old code isn't careful enough about rounding. I think it is really important that bigger input cause bigger (or equal) output. Otherwise you might trigger instability in feedback loops should a rescaler be involved in a some regulator function.

Sadly, I see no elegant solution to your problem.

One way forward may be to somehow provide information on the expected input range, and then determine the scaling method based on that instead of the individual values. But, as indicated, there's no real elegance in that. It can't be automated...

Quoted 42 lines

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/iio/afe/iio-rescale.c +18 -2

index c408c4057c08..7304306c9806 100644
@@ -22,7 +22,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;
 

@@ -38,8 +38,31 @@ 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;
+
+		/*
+		 * For small values, the approximation can be costly,
+		 * change scale type to maintain accuracy.
+		 *
+		 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
+		 */
+		if (scale_type == IIO_VAL_FRACTIONAL)
+			tmp = *val2;
+		else
+			tmp = 1 << *val2;
+
+		 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
+			 *val = div_s64_rem(*val, tmp, &rem2);
+

drivers/iio/afe/iio-rescale.c int rescale_process_scale(struct rescale *rescale, int scale_type, · line 61

Quoted 3 lines

drivers/iio/afe/iio-rescale.c +3 -0

+			 *val2 = div_s64(rem, tmp);
+			 if (rem2)
+				 *val2 += div_s64(rem2 * 1000000000LL, tmp);

rem2 is 32-bit. Might 1000000000LL also be 32-bit on a small machine where 64-bit arithmetic is really expensive? In that case, the above is broken. The safe route is to do these things as in the existing code with a cast to s64. But maybe that's just cargo cult crap?

Cheers,
Peter

Quoted 8 lines

drivers/iio/afe/iio-rescale.c +4 -0

+
+			 return IIO_VAL_INT_PLUS_NANO;
+		 }
+
 		return scale_type;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:

Quoted 11 lines · Peter Rosin

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

Hi Peter,

Thanks for taking time to look at this in detail again. I really appreciate all the feedback you've provided.

Quoted 6 lines

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Quoted 3 lines

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000

The main issue is that the first two examples return 0 which night be worst that loosing a little precision.

56 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {

At the same time, I wonder how "real" these values would be. Having such a small scale would mean having a large raw value. With 16-bits of resolution, that would still give about (1 << 16) * 3.3594e-08 = 0.002201616 mV.

We could try to get more precision out of the first division

	tmp = (s64)*val * 1000000000LL;
	tmp = div_s64(tmp, rescale->denominator);
	tmp *= rescale->numerator;
	tmp = div_s64_rem(tmp, 1000000000LL, &rem);

But then, we'd be more likely to overflow. What would be a good middle ground?

Quoted 4 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

That's a fair point, I didn't know it could be a problem to change scale.

Quoted 10 lines

Jonathan, do you have any input on that?

Some more examples of problematic properties of this patch:

21837/24041 scaled by 427/24727 is 0.01568544672, you get 0.015685446. Ok. But if you reduce the input number, gcd(21837, 24041) -> 29, you have:
753/829 scaled by 427/24727 which still is 0.01568544672 of course, but in this case you get 0.01568154403. Which is less precise. It is unfortunate that input that should be easier to scale may yield worse results.

Expected rel_ppm < 0, but

    rel_ppm == 0

     real=0.015685445
 expected=0.015685446719
# iio_rescale_test_scale: not ok 44 - v8 - 21837/24041 scaled by 427/24727
Expected rel_ppm < 0, but
    rel_ppm == 0

     real=0.015685445

expected=0.015685446719
# iio_rescale_test_scale: not ok 45 - v8 - 753/829 scaled by 427/24727

It seems like both cases are rounded and give the same result. I do get your point though, values that could be simplified might loose more precision because of this change in scale type.

Quoted 6 lines

760/1373754273 scaled by 427/2727 is 8.662580e-8, and 8.662393e-8 is returned. Which is perhaps not great accuracy, but such is life. However. 761/1373754273 scaled by 427/2727 is 8.673978e-8, which is of course greater, but 8.6e-8 is returned. Which is less than what was returned for the smaller 760/1373754273 value above.

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000086626
# iio_rescale_test_scale: not ok 46 - v8 - 760/1373754273 scaled by 427/2727
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

expected=0.000000086740
# iio_rescale_test_scale: not ok 47 - v8 - 761/1373754273 scaled by 427/2727

We fall into the same case as the first two examples where the real value is null.

Considering these null values and the possible issue of not always having the same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO scale?

Quoted 12 lines

Some of these objections are related to what I talked about in v7, i.e.:

Also, changing the calculation so that you get more precision whenever that is possible feels dangerous. I fear linearity breaks and that bigger input cause smaller output due to rounding if the bigger value has to be rounded down, but that this isn't done carefully enough. I.e. attempting to return an exact fraction and only falling back to the old code when that is not possible is still not safe since the old code isn't careful enough about rounding. I think it is really important that bigger input cause bigger (or equal) output. Otherwise you might trigger instability in feedback loops should a rescaler be involved in a some regulator function.

I think I didn't read this closely enought the first time around. I agree that bigger inputs should cause bigger outputs, especially with these rounding errors. My original indention was to have all scales withing a tight margin, that's why I ended up going with ppm for the test cases.

Quoted 7 lines

Sadly, I see no elegant solution to your problem.

One way forward may be to somehow provide information on the expected input range, and then determine the scaling method based on that instead of the individual values. But, as indicated, there's no real elegance in that. It can't be automated...

I guess the issue with that is that unless it's a user parameter, we're always going go have these little islands you mentioned in v7...

Would it be viable to guaranty a MICRO precision instead of NANO, and not have the range parameter?

Quoted 42 lines

Quoted 42 lines

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/iio/afe/iio-rescale.c +18 -2

index c408c4057c08..7304306c9806 100644
@@ -22,7 +22,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;
 

@@ -38,8 +38,31 @@ 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;
+
+		/*
+		 * For small values, the approximation can be costly,
+		 * change scale type to maintain accuracy.
+		 *
+		 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
+		 */
+		if (scale_type == IIO_VAL_FRACTIONAL)
+			tmp = *val2;
+		else
+			tmp = 1 << *val2;
+
+		 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
+			 *val = div_s64_rem(*val, tmp, &rem2);
+

Quoted 8 lines

drivers/iio/afe/iio-rescale.c int rescale_process_scale(struct rescale *rescale, int scale_type, · line 61

Quoted 3 lines

drivers/iio/afe/iio-rescale.c +3 -0

+			 *val2 = div_s64(rem, tmp);
+			 if (rem2)
+				 *val2 += div_s64(rem2 * 1000000000LL, tmp);

rem2 is 32-bit. Might 1000000000LL also be 32-bit on a small machine where 64-bit arithmetic is really expensive? In that case, the above is broken. The safe route is to do these things as in the existing code with a cast to s64. But maybe that's just cargo cult crap?

You're right, this should be

	div_s64((s64)rem2 * 1000000000LL, tmp);

I've been trying th get the kunit tests running on a 32-bit kernel image, but I'm still having issues with that...

Thanks,
Liam

Quoted 13 lines

Cheers,
Peter

Quoted 8 lines

drivers/iio/afe/iio-rescale.c +4 -0

+
+			 return IIO_VAL_INT_PLUS_NANO;
+		 }
+
 		return scale_type;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:

Quoted 24 lines · Liam Beguin

Quoted 11 lines · Peter Rosin

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

Hi Peter,

Thanks for taking time to look at this in detail again. I really appreciate all the feedback you've provided.

Quoted 6 lines

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Quoted 6 lines

Quoted 3 lines

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Excellent!

Quoted 11 lines

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 6 lines

expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000

The main issue is that the first two examples return 0 which night be worst that loosing a little precision.

They shouldn't return zero?

Here's the new code quoted from the test robot (and assuming a 64-bit machine, thus ignoring the 32-bit problem on line 56).

    36		case IIO_VAL_FRACTIONAL:
    37		case IIO_VAL_FRACTIONAL_LOG2:
    38			tmp = (s64)*val * 1000000000LL;
    39			tmp = div_s64(tmp, rescale->denominator);
    40			tmp *= rescale->numerator;
    41	
    42			tmp = div_s64_rem(tmp, 1000000000LL, &rem);
    43			*val = tmp;
    44	
    45			/*
    46			 * For small values, the approximation can be costly,
    47			 * change scale type to maintain accuracy.
    48			 *
    49			 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
    50			 */
    51			if (scale_type == IIO_VAL_FRACTIONAL)
    52				tmp = *val2;
    53			else
    54				tmp = 1 << *val2;
    55	

Quoted 1 line

56 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {

    57				 *val = div_s64_rem(*val, tmp, &rem2);
    58	
    59				 *val2 = div_s64(rem, tmp);
    60				 if (rem2)
    61					 *val2 += div_s64(rem2 * 1000000000LL, tmp);
    62	
    63				 return IIO_VAL_INT_PLUS_NANO;
    64			 }
    65	
    66			return scale_type;

When I go through the above manually, I get:

line

38: tmp = 90000000000    ; 90 * 1000000000
39: tmp = 176817288      ; 90000000000 / 509
40: tmp = 46149312168    ; 176817288 * 261
42: rem = 149312168      ; 46149312168 % 1000000000
    tmp = 46             ; 46149312168 / 1000000000

43: *val = 46
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (149312168 > 10000000 && 46/1373754273 < 100) [yes && yes]

57: rem2 = 46            ; 46 % 1373754273
    *val = 0             ; 46 / 1373754273
59: *val2 = 0            ; 149312168 / 1373754273

60: if 46 [yes]
61: *val2 = 33 ; 0 + 46 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000033]

and

line

38: tmp = 100000000000   ; 100 * 1000000000
39: tmp = 14285714       ; 100000000000 / 7000
40: tmp = 54028570348    ; 176817288 * 3782
42: rem = 28570348       ; 54028570348 % 1000000000
    tmp = 54             ; 54028570348 / 1000000000

43: *val = 54
51: if (<fractional>) [no]
54: tmp = 1073741824 ; 1 << 30
56: if (28570348 > 10000000 && 54/1073741824 < 100) [yes && yes]

57: rem2 = 54            ; 54 % 1073741824
    *val = 0             ; 54 / 1073741824
59: *val2 = 0            ; 28570348 / 1073741824

60: if 46 [yes]
61: *val2 = 50 ; 0 + 54 * 1000000000 / 1073741824
63: return <int-plus-nano> [0.000000050]

Why do you get zero, what am I missing?

Quoted 4 lines

At the same time, I wonder how "real" these values would be. Having such a small scale would mean having a large raw value. With 16-bits of resolution, that would still give about (1 << 16) * 3.3594e-08 = 0.002201616 mV.

If we cap at 16 bits it sounds as if we probably erase some precision provided by 24-bit ADCs. We have drivers for those. I didn't really dig that deep in the driver offerings, but I did find a AD7177 ADC (but no driver) which is 32-bit. If we don't have any 32-bit ADC driver yet, it's only a matter of time, methinks. I have personally worked with 24-bit DACs, and needed every last bit...

Quoted 7 lines

We could try to get more precision out of the first division

	tmp = (s64)*val * 1000000000LL;
	tmp = div_s64(tmp, rescale->denominator);
	tmp *= rescale->numerator;
	tmp = div_s64_rem(tmp, 1000000000LL, &rem);

Quoted 2 lines

But then, we'd be more likely to overflow. What would be a good middle ground?

I don't think we can settle for anything that makes any existing case worse. That's a regression waiting to happen, and what to do then?

Quoted 7 lines

Quoted 4 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

That's a fair point, I didn't know it could be a problem to change scale.

I don't *know* either? But it would not be completely alien to me if the buffered case assumes "raw" numbers, and that there is little room for "meta-data" with each sample.

Quoted 21 lines

Quoted 10 lines

Jonathan, do you have any input on that?

Some more examples of problematic properties of this patch:

21837/24041 scaled by 427/24727 is 0.01568544672, you get 0.015685446. Ok. But if you reduce the input number, gcd(21837, 24041) -> 29, you have:
753/829 scaled by 427/24727 which still is 0.01568544672 of course, but in this case you get 0.01568154403. Which is less precise. It is unfortunate that input that should be easier to scale may yield worse results.

Expected rel_ppm < 0, but

    rel_ppm == 0

     real=0.015685445
 expected=0.015685446719
# iio_rescale_test_scale: not ok 44 - v8 - 21837/24041 scaled by 427/24727
Expected rel_ppm < 0, but
    rel_ppm == 0

     real=0.015685445

Quoted 6 lines

expected=0.015685446719
# iio_rescale_test_scale: not ok 45 - v8 - 753/829 scaled by 427/24727

It seems like both cases are rounded and give the same result. I do get your point though, values that could be simplified might loose more precision because of this change in scale type.

I aimed at this:

line
38: tmp = 21837000000000 ; 21837 * 1000000000

39: tmp = 883123710      ; 21837000000000 / 24727
40: tmp = 377093824170   ; 883123710 * 427
42: rem = 93824170       ; 377093824170 % 1000000000
    tmp = 377            ; 377093824170 / 1000000000

43: *val = 377
51: if (<fractional>) [yes]
52: tmp = 24041
56: if (149312168 > 10000000 && 377/24041 < 100) [yes && yes]

57: rem2 = 377           ; 377 % 24041
    *val = 0             ; 377 / 24041
59: *val2 = 3902         ; 93824170 / 24041

60: if 377 [yes]
61: *val2 = 15685446 ; 3902 + 377 * 1000000000 / 24041
63: return <int-plus-nano> [0.0015685446]

Why does the test output a 5 at the end and not a 6? It's all integer arithmetic so there is no room for rounding issues.

and

line

38: tmp = 753000000000   ; 753 * 1000000000
39: tmp = 30452541       ; 753000000000 / 24727
40: tmp = 13003235007    ; 30452541 * 427
42: rem = 3235007        ; 13003235007 % 1000000000
    tmp = 13             ; 13003235007 / 1000000000

43: *val = 13
51: if (<fractional>) [yes]
52: tmp = 829
56: if (3235007 > 10000000 && 13/829 < 100) [no && yes] 66: return <fractional> [13/829 ~= 0.015681544]

0.015681544 is pretty different from 0.015685446

Again, I don't understand what's going on.

Quoted 17 lines

Quoted 6 lines

760/1373754273 scaled by 427/2727 is 8.662580e-8, and 8.662393e-8 is returned. Which is perhaps not great accuracy, but such is life. However. 761/1373754273 scaled by 427/2727 is 8.673978e-8, which is of course greater, but 8.6e-8 is returned. Which is less than what was returned for the smaller 760/1373754273 value above.

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000086626
# iio_rescale_test_scale: not ok 46 - v8 - 760/1373754273 scaled by 427/2727
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 5 lines

expected=0.000000086740
# iio_rescale_test_scale: not ok 47 - v8 - 761/1373754273 scaled by 427/2727

We fall into the same case as the first two examples where the real value is null.

I aimed at

line

38: tmp = 760000000000   ; 760 * 1000000000
39: tmp = 278694536      ; 760000000000 / 2727
40: tmp = 119002566872   ; 278694536 * 427
42: rem = 2566872        ; 119002566872 % 1000000000
    tmp = 119            ; 119002566872 / 1000000000

43: *val = 119
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (2566872 > 10000000 && 119/1373754273 < 100) [no && yes] 66: return <fractional> [119/1373754273 ~= 0.000000086624]

and

line

38: tmp = 761000000000   ; 761 * 1000000000
39: tmp = 279061239      ; 761000000000 / 2727
40: tmp = 119159149053   ; 279061239 * 427
42: rem = 159149053      ; 119159149053 % 1000000000
    tmp = 119            ; 119159149053 / 1000000000

43: *val = 119
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (159149053 > 10000000 && 119/1373754273 < 100) [yes && yes]

57: rem2 = 119           ; 119 % 1373754273
    *val = 0             ; 119 / 1373754273
59: *val2 = 0            ; 159149053 / 1373754273

60: if 119 [yes]
61: *val2 = 86 ; 0 + 119 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000086]

Quoted 3 lines

Considering these null values and the possible issue of not always having the same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO scale?

No, that absolutely kills the precision for small values that are much better off as-is. The closer you get to zero, the more the conversion to int-plus-nano hurts, relatively speaking.

Quoted 27 lines

Quoted 4 lines

Some of these objections are related to what I talked about in v7, i.e.:

Quoted 9 lines

Also, changing the calculation so that you get more precision whenever that is possible feels dangerous. I fear linearity breaks and that bigger input cause smaller output due to rounding if the bigger value has to be rounded down, but that this isn't done carefully enough. I.e. attempting to return an exact fraction and only falling back to the old code when that is not possible is still not safe since the old code isn't careful enough about rounding. I think it is really important that bigger input cause bigger (or equal) output. Otherwise you might trigger instability in feedback loops should a rescaler be involved in a some regulator function.

I think I didn't read this closely enought the first time around. I agree that bigger inputs should cause bigger outputs, especially with these rounding errors. My original indention was to have all scales withing a tight margin, that's why I ended up going with ppm for the test cases.

Quoted 7 lines

Sadly, I see no elegant solution to your problem.

One way forward may be to somehow provide information on the expected input range, and then determine the scaling method based on that instead of the individual values. But, as indicated, there's no real elegance in that. It can't be automated...

Quoted 5 lines

I guess the issue with that is that unless it's a user parameter, we're always going go have these little islands you mentioned in v7...

Would it be viable to guaranty a MICRO precision instead of NANO, and not have the range parameter?

I don't get what you mean here? Returning int-plus-micro can't be it, since that would be completely pointless and only make it easier to trigger accuracy problems of the conversion. However, I feel that any attempt to shift digits but still having the same general approch will just change the size and position of the islands, and thus not fix the fundamental problematic border between land and water.

Cheers,
Peter

Quoted 74 lines

Quoted 42 lines

Quoted 42 lines

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/iio/afe/iio-rescale.c +18 -2

index c408c4057c08..7304306c9806 100644
@@ -22,7 +22,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;
 

@@ -38,8 +38,31 @@ 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;
+
+		/*
+		 * For small values, the approximation can be costly,
+		 * change scale type to maintain accuracy.
+		 *
+		 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
+		 */
+		if (scale_type == IIO_VAL_FRACTIONAL)
+			tmp = *val2;
+		else
+			tmp = 1 << *val2;
+
+		 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
+			 *val = div_s64_rem(*val, tmp, &rem2);
+

Quoted 8 lines

drivers/iio/afe/iio-rescale.c int rescale_process_scale(struct rescale *rescale, int scale_type, · line 61

Quoted 3 lines

drivers/iio/afe/iio-rescale.c +3 -0

+			 *val2 = div_s64(rem, tmp);
+			 if (rem2)
+				 *val2 += div_s64(rem2 * 1000000000LL, tmp);

rem2 is 32-bit. Might 1000000000LL also be 32-bit on a small machine where 64-bit arithmetic is really expensive? In that case, the above is broken. The safe route is to do these things as in the existing code with a cast to s64. But maybe that's just cargo cult crap?

You're right, this should be

	div_s64((s64)rem2 * 1000000000LL, tmp);

I've been trying th get the kunit tests running on a 32-bit kernel image, but I'm still having issues with that...

Thanks,
Liam

Quoted 13 lines

Cheers,
Peter

Quoted 8 lines

drivers/iio/afe/iio-rescale.c +4 -0

+
+			 return IIO_VAL_INT_PLUS_NANO;
+		 }
+
 		return scale_type;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:

Quoted 120 lines · Peter Rosin

Quoted 24 lines · Liam Beguin

Quoted 11 lines · Peter Rosin

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

Hi Peter,

Thanks for taking time to look at this in detail again. I really appreciate all the feedback you've provided.

Quoted 6 lines

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Quoted 6 lines

Quoted 3 lines

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Excellent!

Quoted 11 lines

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 6 lines

expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000

The main issue is that the first two examples return 0 which night be worst that loosing a little precision.

They shouldn't return zero?

Here's the new code quoted from the test robot (and assuming a 64-bit machine, thus ignoring the 32-bit problem on line 56).

    36		case IIO_VAL_FRACTIONAL:
    37		case IIO_VAL_FRACTIONAL_LOG2:
    38			tmp = (s64)*val * 1000000000LL;
    39			tmp = div_s64(tmp, rescale->denominator);
    40			tmp *= rescale->numerator;
    41	
    42			tmp = div_s64_rem(tmp, 1000000000LL, &rem);
    43			*val = tmp;
    44	
    45			/*
    46			 * For small values, the approximation can be costly,
    47			 * change scale type to maintain accuracy.
    48			 *
    49			 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
    50			 */
    51			if (scale_type == IIO_VAL_FRACTIONAL)
    52				tmp = *val2;
    53			else
    54				tmp = 1 << *val2;
    55	  

Quoted 1 line

56 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {

    57				 *val = div_s64_rem(*val, tmp, &rem2);
    58	
    59				 *val2 = div_s64(rem, tmp);
    60				 if (rem2)
    61					 *val2 += div_s64(rem2 * 1000000000LL, tmp);
    62	
    63				 return IIO_VAL_INT_PLUS_NANO;
    64			 }
    65	
    66			return scale_type;

When I go through the above manually, I get:

line

38: tmp = 90000000000    ; 90 * 1000000000
39: tmp = 176817288      ; 90000000000 / 509
40: tmp = 46149312168    ; 176817288 * 261
42: rem = 149312168      ; 46149312168 % 1000000000
    tmp = 46             ; 46149312168 / 1000000000

43: *val = 46
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (149312168 > 10000000 && 46/1373754273 < 100) [yes && yes]

57: rem2 = 46            ; 46 % 1373754273
    *val = 0             ; 46 / 1373754273
59: *val2 = 0            ; 149312168 / 1373754273

60: if 46 [yes]
61: *val2 = 33 ; 0 + 46 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000033]

and

line

38: tmp = 100000000000   ; 100 * 1000000000
39: tmp = 14285714       ; 100000000000 / 7000
40: tmp = 54028570348    ; 176817288 * 3782
42: rem = 28570348       ; 54028570348 % 1000000000
    tmp = 54             ; 54028570348 / 1000000000

43: *val = 54
51: if (<fractional>) [no]
54: tmp = 1073741824 ; 1 << 30
56: if (28570348 > 10000000 && 54/1073741824 < 100) [yes && yes]

Quoted 8 lines

57: rem2 = 54            ; 54 % 1073741824
    *val = 0             ; 54 / 1073741824
59: *val2 = 0            ; 28570348 / 1073741824

60: if 46 [yes]
61: *val2 = 50 ; 0 + 54 * 1000000000 / 1073741824
63: return <int-plus-nano> [0.000000050]

Why do you get zero, what am I missing?

So... It turns out, I swapped schan and rescaler values which gives us:

numerator = 90
denominator = 1373754273
schan_val = 261
schan_val2 = 509

line

38: tmp = 261000000000   ; 261 * 1000000000
39: tmp = 189            ; 261000000000 / 1373754273
40: tmp = 17010          ; 189 * 90
42: rem = 17010          ; 17010 % 1000000000
    tmp = 0              ; 17010 / 1000000000

43: *val = 0
51: if (<fractional>) [yes]
52: tmp = 509
56: if (17010 > 10000000 && 0/509 < 100) [no && yes] 66: *val = 0

    *val2 = 509
    return <fractional> [0.000000000]

Swapping back the values, I get the same results as you!

Also, replacing line 56 from the snippet above with

- if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
+ if (abs(rem)) {

Fixes these precision errors. It also prevents us from returning different scales if we swap the two divisions (schan and rescaler parameters).

Quoted 5 lines

Quoted 4 lines

At the same time, I wonder how "real" these values would be. Having such a small scale would mean having a large raw value. With 16-bits of resolution, that would still give about (1 << 16) * 3.3594e-08 = 0.002201616 mV.

Quoted 7 lines

If we cap at 16 bits it sounds as if we probably erase some precision provided by 24-bit ADCs. We have drivers for those. I didn't really dig that deep in the driver offerings, but I did find a AD7177 ADC (but no driver) which is 32-bit. If we don't have any 32-bit ADC driver yet, it's only a matter of time, methinks. I have personally worked with 24-bit DACs, and needed every last bit...

I was only using 16-bits as an example, but I guess you're right, these values do start to make sense when you're looking at 24-bit and 32-bit ADCs.

Quoted 11 lines

Quoted 10 lines

We could try to get more precision out of the first division

	tmp = (s64)*val * 1000000000LL;
	tmp = div_s64(tmp, rescale->denominator);
	tmp *= rescale->numerator;
	tmp = div_s64_rem(tmp, 1000000000LL, &rem);

But then, we'd be more likely to overflow. What would be a good middle ground?

Quoted 3 lines

I don't think we can settle for anything that makes any existing case worse. That's a regression waiting to happen, and what to do then?

Agreed, and looking at this more, there's still ways to improve without having to compromise.
Hopefully adding the test suite will make it easier to catch potential regressions in the future :-)

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Quoted 6 lines

Quoted 2 lines

That's a fair point, I didn't know it could be a problem to change scale.

I don't *know* either? But it would not be completely alien to me if the buffered case assumes "raw" numbers, and that there is little room for "meta-data" with each sample.

Quoted 125 lines

Quoted 22 lines

Quoted 11 lines

Jonathan, do you have any input on that?

Some more examples of problematic properties of this patch:

21837/24041 scaled by 427/24727 is 0.01568544672, you get 0.015685446. Ok. But if you reduce the input number, gcd(21837, 24041) -> 29, you have:
753/829 scaled by 427/24727 which still is 0.01568544672 of course, but in this case you get 0.01568154403. Which is less precise. It is unfortunate that input that should be easier to scale may yield worse results.

Expected rel_ppm < 0, but

    rel_ppm == 0

     real=0.015685445
 expected=0.015685446719
# iio_rescale_test_scale: not ok 44 - v8 - 21837/24041 scaled by 427/24727
Expected rel_ppm < 0, but
    rel_ppm == 0

     real=0.015685445

Quoted 6 lines

expected=0.015685446719
# iio_rescale_test_scale: not ok 45 - v8 - 753/829 scaled by 427/24727

It seems like both cases are rounded and give the same result. I do get your point though, values that could be simplified might loose more precision because of this change in scale type.

I aimed at this:

line
38: tmp = 21837000000000 ; 21837 * 1000000000

39: tmp = 883123710      ; 21837000000000 / 24727
40: tmp = 377093824170   ; 883123710 * 427
42: rem = 93824170       ; 377093824170 % 1000000000
    tmp = 377            ; 377093824170 / 1000000000

43: *val = 377
51: if (<fractional>) [yes]
52: tmp = 24041
56: if (149312168 > 10000000 && 377/24041 < 100) [yes && yes]

57: rem2 = 377           ; 377 % 24041
    *val = 0             ; 377 / 24041
59: *val2 = 3902         ; 93824170 / 24041

60: if 377 [yes]
61: *val2 = 15685446 ; 3902 + 377 * 1000000000 / 24041
63: return <int-plus-nano> [0.0015685446]

Why does the test output a 5 at the end and not a 6? It's all integer arithmetic so there is no room for rounding issues.

and

line

38: tmp = 753000000000   ; 753 * 1000000000
39: tmp = 30452541       ; 753000000000 / 24727
40: tmp = 13003235007    ; 30452541 * 427
42: rem = 3235007        ; 13003235007 % 1000000000
    tmp = 13             ; 13003235007 / 1000000000

43: *val = 13
51: if (<fractional>) [yes]
52: tmp = 829
56: if (3235007 > 10000000 && 13/829 < 100) [no && yes] 66: return <fractional> [13/829 ~= 0.015681544]

0.015681544 is pretty different from 0.015685446

Again, I don't understand what's going on.

Quoted 17 lines

Quoted 6 lines

760/1373754273 scaled by 427/2727 is 8.662580e-8, and 8.662393e-8 is returned. Which is perhaps not great accuracy, but such is life. However. 761/1373754273 scaled by 427/2727 is 8.673978e-8, which is of course greater, but 8.6e-8 is returned. Which is less than what was returned for the smaller 760/1373754273 value above.

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000086626
# iio_rescale_test_scale: not ok 46 - v8 - 760/1373754273 scaled by 427/2727
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 5 lines

expected=0.000000086740
# iio_rescale_test_scale: not ok 47 - v8 - 761/1373754273 scaled by 427/2727

We fall into the same case as the first two examples where the real value is null.

I aimed at

line

38: tmp = 760000000000   ; 760 * 1000000000
39: tmp = 278694536      ; 760000000000 / 2727
40: tmp = 119002566872   ; 278694536 * 427
42: rem = 2566872        ; 119002566872 % 1000000000
    tmp = 119            ; 119002566872 / 1000000000

43: *val = 119
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (2566872 > 10000000 && 119/1373754273 < 100) [no && yes] 66: return <fractional> [119/1373754273 ~= 0.000000086624]

and

line

38: tmp = 761000000000   ; 761 * 1000000000
39: tmp = 279061239      ; 761000000000 / 2727
40: tmp = 119159149053   ; 279061239 * 427
42: rem = 159149053      ; 119159149053 % 1000000000
    tmp = 119            ; 119159149053 / 1000000000

43: *val = 119
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (159149053 > 10000000 && 119/1373754273 < 100) [yes && yes]

57: rem2 = 119           ; 119 % 1373754273
    *val = 0             ; 119 / 1373754273
59: *val2 = 0            ; 159149053 / 1373754273

60: if 119 [yes]
61: *val2 = 86 ; 0 + 119 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000086]

Quoted 7 lines

Quoted 3 lines

Considering these null values and the possible issue of not always having the same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO scale?

No, that absolutely kills the precision for small values that are much better off as-is. The closer you get to zero, the more the conversion to int-plus-nano hurts, relatively speaking.

I'm not sure I understand what you mean. The point of switching to IIO_VAL_INT_PLUS_NANO at the moment is to get more precision on small values. Am I missing something?

Quoted 33 lines

Quoted 27 lines

Quoted 4 lines

Some of these objections are related to what I talked about in v7, i.e.:

Quoted 9 lines

Also, changing the calculation so that you get more precision whenever that is possible feels dangerous. I fear linearity breaks and that bigger input cause smaller output due to rounding if the bigger value has to be rounded down, but that this isn't done carefully enough. I.e. attempting to return an exact fraction and only falling back to the old code when that is not possible is still not safe since the old code isn't careful enough about rounding. I think it is really important that bigger input cause bigger (or equal) output. Otherwise you might trigger instability in feedback loops should a rescaler be involved in a some regulator function.

I think I didn't read this closely enought the first time around. I agree that bigger inputs should cause bigger outputs, especially with these rounding errors. My original indention was to have all scales withing a tight margin, that's why I ended up going with ppm for the test cases.

Quoted 7 lines

Sadly, I see no elegant solution to your problem.

One way forward may be to somehow provide information on the expected input range, and then determine the scaling method based on that instead of the individual values. But, as indicated, there's no real elegance in that. It can't be automated...

Quoted 5 lines

I guess the issue with that is that unless it's a user parameter, we're always going go have these little islands you mentioned in v7...

Would it be viable to guaranty a MICRO precision instead of NANO, and not have the range parameter?

Quoted 6 lines

I don't get what you mean here? Returning int-plus-micro can't be it, since that would be completely pointless and only make it easier to trigger accuracy problems of the conversion. However, I feel that any attempt to shift digits but still having the same general approch will just change the size and position of the islands, and thus not fix the fundamental problematic border between land and water.

My apologies, discard this last comment. I was suggesting to guaranty less precision, but consistent over the full range. I don't believe that's a viable option.

Thanks again for your time,
Liam

Quoted 78 lines

Cheers,
Peter

Quoted 74 lines

Quoted 42 lines

Quoted 42 lines

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/iio/afe/iio-rescale.c +18 -2

index c408c4057c08..7304306c9806 100644
@@ -22,7 +22,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;
 

@@ -38,8 +38,31 @@ 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;
+
+		/*
+		 * For small values, the approximation can be costly,
+		 * change scale type to maintain accuracy.
+		 *
+		 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
+		 */
+		if (scale_type == IIO_VAL_FRACTIONAL)
+			tmp = *val2;
+		else
+			tmp = 1 << *val2;
+
+		 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
+			 *val = div_s64_rem(*val, tmp, &rem2);
+

Quoted 8 lines

drivers/iio/afe/iio-rescale.c int rescale_process_scale(struct rescale *rescale, int scale_type, · line 61

Quoted 3 lines

drivers/iio/afe/iio-rescale.c +3 -0

+			 *val2 = div_s64(rem, tmp);
+			 if (rem2)
+				 *val2 += div_s64(rem2 * 1000000000LL, tmp);  

rem2 is 32-bit. Might 1000000000LL also be 32-bit on a small machine where 64-bit arithmetic is really expensive? In that case, the above is broken. The safe route is to do these things as in the existing code with a cast to s64. But maybe that's just cargo cult crap?

You're right, this should be

	div_s64((s64)rem2 * 1000000000LL, tmp);

I've been trying th get the kunit tests running on a 32-bit kernel image, but I'm still having issues with that...

Thanks,
Liam

Quoted 13 lines

Cheers,
Peter

Quoted 8 lines

drivers/iio/afe/iio-rescale.c +4 -0

+
+			 return IIO_VAL_INT_PLUS_NANO;
+		 }
+
 		return scale_type;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:

Quoted 175 lines · On Sun, 29 Aug 2021 00:41:21 -0400 Liam Beguin <[email protected]> wrote:

Quoted 120 lines · Peter Rosin

Quoted 24 lines · Liam Beguin

Quoted 11 lines · Peter Rosin

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

Hi Peter,

Thanks for taking time to look at this in detail again. I really appreciate all the feedback you've provided.

Quoted 6 lines

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Quoted 6 lines

Quoted 3 lines

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Excellent!

Quoted 11 lines

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 6 lines

expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000

The main issue is that the first two examples return 0 which night be worst that loosing a little precision.

They shouldn't return zero?

Here's the new code quoted from the test robot (and assuming a 64-bit machine, thus ignoring the 32-bit problem on line 56).

    36		case IIO_VAL_FRACTIONAL:
    37		case IIO_VAL_FRACTIONAL_LOG2:
    38			tmp = (s64)*val * 1000000000LL;
    39			tmp = div_s64(tmp, rescale->denominator);
    40			tmp *= rescale->numerator;
    41	
    42			tmp = div_s64_rem(tmp, 1000000000LL, &rem);
    43			*val = tmp;
    44	
    45			/*
    46			 * For small values, the approximation can be costly,
    47			 * change scale type to maintain accuracy.
    48			 *
    49			 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
    50			 */
    51			if (scale_type == IIO_VAL_FRACTIONAL)
    52				tmp = *val2;
    53			else
    54				tmp = 1 << *val2;
    55	  

Quoted 1 line

56 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {

    57				 *val = div_s64_rem(*val, tmp, &rem2);
    58	
    59				 *val2 = div_s64(rem, tmp);
    60				 if (rem2)
    61					 *val2 += div_s64(rem2 * 1000000000LL, tmp);
    62	
    63				 return IIO_VAL_INT_PLUS_NANO;
    64			 }
    65	
    66			return scale_type;

When I go through the above manually, I get:

line

38: tmp = 90000000000    ; 90 * 1000000000
39: tmp = 176817288      ; 90000000000 / 509
40: tmp = 46149312168    ; 176817288 * 261
42: rem = 149312168      ; 46149312168 % 1000000000
    tmp = 46             ; 46149312168 / 1000000000

43: *val = 46
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (149312168 > 10000000 && 46/1373754273 < 100) [yes && yes]

57: rem2 = 46            ; 46 % 1373754273
    *val = 0             ; 46 / 1373754273
59: *val2 = 0            ; 149312168 / 1373754273

60: if 46 [yes]
61: *val2 = 33 ; 0 + 46 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000033]

and

line

38: tmp = 100000000000   ; 100 * 1000000000
39: tmp = 14285714       ; 100000000000 / 7000
40: tmp = 54028570348    ; 176817288 * 3782
42: rem = 28570348       ; 54028570348 % 1000000000
    tmp = 54             ; 54028570348 / 1000000000

43: *val = 54
51: if (<fractional>) [no]
54: tmp = 1073741824 ; 1 << 30
56: if (28570348 > 10000000 && 54/1073741824 < 100) [yes && yes]

Quoted 8 lines

57: rem2 = 54            ; 54 % 1073741824
    *val = 0             ; 54 / 1073741824
59: *val2 = 0            ; 28570348 / 1073741824

60: if 46 [yes]
61: *val2 = 50 ; 0 + 54 * 1000000000 / 1073741824
63: return <int-plus-nano> [0.000000050]

Why do you get zero, what am I missing?

So... It turns out, I swapped schan and rescaler values which gives us:

numerator = 90
denominator = 1373754273
schan_val = 261
schan_val2 = 509

line

38: tmp = 261000000000   ; 261 * 1000000000
39: tmp = 189            ; 261000000000 / 1373754273
40: tmp = 17010          ; 189 * 90
42: rem = 17010          ; 17010 % 1000000000
    tmp = 0              ; 17010 / 1000000000

43: *val = 0
51: if (<fractional>) [yes]
52: tmp = 509
56: if (17010 > 10000000 && 0/509 < 100) [no && yes] 66: *val = 0

    *val2 = 509
    return <fractional> [0.000000000]

Swapping back the values, I get the same results as you!

Also, replacing line 56 from the snippet above with

- if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
+ if (abs(rem)) {

Fixes these precision errors. It also prevents us from returning different scales if we swap the two divisions (schan and rescaler parameters).

Quoted 5 lines

Quoted 4 lines

At the same time, I wonder how "real" these values would be. Having such a small scale would mean having a large raw value. With 16-bits of resolution, that would still give about (1 << 16) * 3.3594e-08 = 0.002201616 mV.

Quoted 7 lines

If we cap at 16 bits it sounds as if we probably erase some precision provided by 24-bit ADCs. We have drivers for those. I didn't really dig that deep in the driver offerings, but I did find a AD7177 ADC (but no driver) which is 32-bit. If we don't have any 32-bit ADC driver yet, it's only a matter of time, methinks. I have personally worked with 24-bit DACs, and needed every last bit...

Quoted 3 lines

I was only using 16-bits as an example, but I guess you're right, these values do start to make sense when you're looking at 24-bit and 32-bit ADCs.

I'd be tempted to be cynical on this. High resolution devices are rare as frankly building a low enough noise board to take advantage is hard. Known users of the AFE infrastructure also rare and so as long as we don't break any 'current' users via loss of accuracy I'm not that bothered if we aren't perfect for 32 bit devices.

I'm guessing we can sometimes sanity check if an overflow will occur at probe? Perhaps do that where possible and print something obvious to the log. Then someone who needs it can figure out the magic maths to do this for those high resolution devices!

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

We could try to get more precision out of the first division

	tmp = (s64)*val * 1000000000LL;
	tmp = div_s64(tmp, rescale->denominator);
	tmp *= rescale->numerator;
	tmp = div_s64_rem(tmp, 1000000000LL, &rem);

But then, we'd be more likely to overflow. What would be a good middle ground?

Quoted 8 lines

Quoted 3 lines

I don't think we can settle for anything that makes any existing case worse. That's a regression waiting to happen, and what to do then?

Agreed, and looking at this more, there's still ways to improve without having to compromise.
Hopefully adding the test suite will make it easier to catch potential regressions in the future :-)

Absolutely. Have that test suite is great :)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

That's a fair point, I didn't know it could be a problem to change scale.

I don't *know* either? But it would not be completely alien to me if the buffered case assumes "raw" numbers, and that there is little room for "meta-data" with each sample.

Spot on. Meta data is a pain so an early design decision in IIO was to not support it in band.

Quoted 177 lines

Quoted 125 lines

Quoted 22 lines

Quoted 11 lines

Jonathan, do you have any input on that?

Some more examples of problematic properties of this patch:

21837/24041 scaled by 427/24727 is 0.01568544672, you get 0.015685446. Ok. But if you reduce the input number, gcd(21837, 24041) -> 29, you have:
753/829 scaled by 427/24727 which still is 0.01568544672 of course, but in this case you get 0.01568154403. Which is less precise. It is unfortunate that input that should be easier to scale may yield worse results.

Expected rel_ppm < 0, but

    rel_ppm == 0

     real=0.015685445
 expected=0.015685446719
# iio_rescale_test_scale: not ok 44 - v8 - 21837/24041 scaled by 427/24727
Expected rel_ppm < 0, but
    rel_ppm == 0

     real=0.015685445

Quoted 6 lines

expected=0.015685446719
# iio_rescale_test_scale: not ok 45 - v8 - 753/829 scaled by 427/24727

It seems like both cases are rounded and give the same result. I do get your point though, values that could be simplified might loose more precision because of this change in scale type.

I aimed at this:

line
38: tmp = 21837000000000 ; 21837 * 1000000000

39: tmp = 883123710      ; 21837000000000 / 24727
40: tmp = 377093824170   ; 883123710 * 427
42: rem = 93824170       ; 377093824170 % 1000000000
    tmp = 377            ; 377093824170 / 1000000000

43: *val = 377
51: if (<fractional>) [yes]
52: tmp = 24041
56: if (149312168 > 10000000 && 377/24041 < 100) [yes && yes]

57: rem2 = 377           ; 377 % 24041
    *val = 0             ; 377 / 24041
59: *val2 = 3902         ; 93824170 / 24041

60: if 377 [yes]
61: *val2 = 15685446 ; 3902 + 377 * 1000000000 / 24041
63: return <int-plus-nano> [0.0015685446]

Why does the test output a 5 at the end and not a 6? It's all integer arithmetic so there is no room for rounding issues.

and

line

38: tmp = 753000000000   ; 753 * 1000000000
39: tmp = 30452541       ; 753000000000 / 24727
40: tmp = 13003235007    ; 30452541 * 427
42: rem = 3235007        ; 13003235007 % 1000000000
    tmp = 13             ; 13003235007 / 1000000000

43: *val = 13
51: if (<fractional>) [yes]
52: tmp = 829
56: if (3235007 > 10000000 && 13/829 < 100) [no && yes] 66: return <fractional> [13/829 ~= 0.015681544]

0.015681544 is pretty different from 0.015685446

Again, I don't understand what's going on.

Quoted 17 lines

Quoted 6 lines

760/1373754273 scaled by 427/2727 is 8.662580e-8, and 8.662393e-8 is returned. Which is perhaps not great accuracy, but such is life. However. 761/1373754273 scaled by 427/2727 is 8.673978e-8, which is of course greater, but 8.6e-8 is returned. Which is less than what was returned for the smaller 760/1373754273 value above.

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000086626
# iio_rescale_test_scale: not ok 46 - v8 - 760/1373754273 scaled by 427/2727
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 5 lines

expected=0.000000086740
# iio_rescale_test_scale: not ok 47 - v8 - 761/1373754273 scaled by 427/2727

We fall into the same case as the first two examples where the real value is null.

I aimed at

line

38: tmp = 760000000000   ; 760 * 1000000000
39: tmp = 278694536      ; 760000000000 / 2727
40: tmp = 119002566872   ; 278694536 * 427
42: rem = 2566872        ; 119002566872 % 1000000000
    tmp = 119            ; 119002566872 / 1000000000

43: *val = 119
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (2566872 > 10000000 && 119/1373754273 < 100) [no && yes] 66: return <fractional> [119/1373754273 ~= 0.000000086624]

and

line

38: tmp = 761000000000   ; 761 * 1000000000
39: tmp = 279061239      ; 761000000000 / 2727
40: tmp = 119159149053   ; 279061239 * 427
42: rem = 159149053      ; 119159149053 % 1000000000
    tmp = 119            ; 119159149053 / 1000000000

43: *val = 119
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (159149053 > 10000000 && 119/1373754273 < 100) [yes && yes]

57: rem2 = 119           ; 119 % 1373754273
    *val = 0             ; 119 / 1373754273
59: *val2 = 0            ; 159149053 / 1373754273

60: if 119 [yes]
61: *val2 = 86 ; 0 + 119 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000086]

Quoted 7 lines

Quoted 3 lines

Considering these null values and the possible issue of not always having the same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO scale?

No, that absolutely kills the precision for small values that are much better off as-is. The closer you get to zero, the more the conversion to int-plus-nano hurts, relatively speaking.

I'm not sure I understand what you mean. The point of switching to IIO_VAL_INT_PLUS_NANO at the moment is to get more precision on small values. Am I missing something?

Quoted 39 lines

Quoted 27 lines

Quoted 4 lines

Some of these objections are related to what I talked about in v7, i.e.:

Quoted 9 lines

Also, changing the calculation so that you get more precision whenever that is possible feels dangerous. I fear linearity breaks and that bigger input cause smaller output due to rounding if the bigger value has to be rounded down, but that this isn't done carefully enough. I.e. attempting to return an exact fraction and only falling back to the old code when that is not possible is still not safe since the old code isn't careful enough about rounding. I think it is really important that bigger input cause bigger (or equal) output. Otherwise you might trigger instability in feedback loops should a rescaler be involved in a some regulator function.

I think I didn't read this closely enought the first time around. I agree that bigger inputs should cause bigger outputs, especially with these rounding errors. My original indention was to have all scales withing a tight margin, that's why I ended up going with ppm for the test cases.

Quoted 7 lines

Sadly, I see no elegant solution to your problem.

One way forward may be to somehow provide information on the expected input range, and then determine the scaling method based on that instead of the individual values. But, as indicated, there's no real elegance in that. It can't be automated...

Quoted 5 lines

I guess the issue with that is that unless it's a user parameter, we're always going go have these little islands you mentioned in v7...

Would it be viable to guaranty a MICRO precision instead of NANO, and not have the range parameter?

I don't get what you mean here? Returning int-plus-micro can't be it, since that would be completely pointless and only make it easier to trigger accuracy problems of the conversion. However, I feel that any attempt to shift digits but still having the same general approch will just change the size and position of the islands, and thus not fix the fundamental problematic border between land and water.

Quoted 3 lines

My apologies, discard this last comment. I was suggesting to guaranty less precision, but consistent over the full range. I don't believe that's a viable option.

Keep up the good work! I'm looking forward to this going in (hopefully shortly!)

Jonathan

Quoted 7 lines

Thanks again for your time,
Liam

Quoted 3 lines

Cheers,
Peter

Quoted 127 lines · Peter Rosin

Quoted 24 lines · Liam Beguin

Quoted 11 lines · Peter Rosin

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

Hi Peter,

Thanks for taking time to look at this in detail again. I really appreciate all the feedback you've provided.

Quoted 6 lines

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Quoted 6 lines

Quoted 3 lines

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Excellent!

Quoted 11 lines

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 6 lines

expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000

The main issue is that the first two examples return 0 which night be worst that loosing a little precision.

They shouldn't return zero?

Here's the new code quoted from the test robot (and assuming a 64-bit machine, thus ignoring the 32-bit problem on line 56).

    36		case IIO_VAL_FRACTIONAL:
    37		case IIO_VAL_FRACTIONAL_LOG2:
    38			tmp = (s64)*val * 1000000000LL;
    39			tmp = div_s64(tmp, rescale->denominator);
    40			tmp *= rescale->numerator;
    41	
    42			tmp = div_s64_rem(tmp, 1000000000LL, &rem);
    43			*val = tmp;
    44	
    45			/*
    46			 * For small values, the approximation can be costly,
    47			 * change scale type to maintain accuracy.
    48			 *
    49			 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
    50			 */
    51			if (scale_type == IIO_VAL_FRACTIONAL)
    52				tmp = *val2;
    53			else
    54				tmp = 1 << *val2;
    55	

Quoted 1 line

56 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {

    57				 *val = div_s64_rem(*val, tmp, &rem2);
    58	
    59				 *val2 = div_s64(rem, tmp);
    60				 if (rem2)
    61					 *val2 += div_s64(rem2 * 1000000000LL, tmp);
    62	
    63				 return IIO_VAL_INT_PLUS_NANO;
    64			 }
    65	
    66			return scale_type;

When I go through the above manually, I get:

line

38: tmp = 90000000000    ; 90 * 1000000000
39: tmp = 176817288      ; 90000000000 / 509
40: tmp = 46149312168    ; 176817288 * 261
42: rem = 149312168      ; 46149312168 % 1000000000
    tmp = 46             ; 46149312168 / 1000000000

43: *val = 46
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (149312168 > 10000000 && 46/1373754273 < 100) [yes && yes]

57: rem2 = 46            ; 46 % 1373754273
    *val = 0             ; 46 / 1373754273
59: *val2 = 0            ; 149312168 / 1373754273

60: if 46 [yes]
61: *val2 = 33 ; 0 + 46 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000033]

and

line

38: tmp = 100000000000   ; 100 * 1000000000
39: tmp = 14285714       ; 100000000000 / 7000
40: tmp = 54028570348    ; 176817288 * 3782
42: rem = 28570348       ; 54028570348 % 1000000000
    tmp = 54             ; 54028570348 / 1000000000

43: *val = 54
51: if (<fractional>) [no]
54: tmp = 1073741824 ; 1 << 30
56: if (28570348 > 10000000 && 54/1073741824 < 100) [yes && yes]

57: rem2 = 54            ; 54 % 1073741824
    *val = 0             ; 54 / 1073741824
59: *val2 = 0            ; 28570348 / 1073741824

60: if 46 [yes]
61: *val2 = 50 ; 0 + 54 * 1000000000 / 1073741824
63: return <int-plus-nano> [0.000000050]

Quoted 1 line

Why do you get zero, what am I missing?

So... It turns out, I swapped schan and rescaler values which gives us:

numerator = 90
denominator = 1373754273
schan_val = 261
schan_val2 = 509

line

38: tmp = 261000000000   ; 261 * 1000000000
39: tmp = 189            ; 261000000000 / 1373754273
40: tmp = 17010          ; 189 * 90
42: rem = 17010          ; 17010 % 1000000000
    tmp = 0              ; 17010 / 1000000000

43: *val = 0
51: if (<fractional>) [yes]
52: tmp = 509
56: if (17010 > 10000000 && 0/509 < 100) [no && yes] 66: *val = 0

    *val2 = 509
    return <fractional> [0.000000000]

Swapping back the values, I get the same results as you!

Also, replacing line 56 from the snippet above with

- if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
+ if (abs(rem)) {

Fixes these precision errors. It also prevents us from returning different scales if we swap the two divisions (schan and rescaler parameters).

Quoted 7 lines

Quoted 3 lines

Considering these null values and the possible issue of not always having the same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO scale?

No, that absolutely kills the precision for small values that are much better off as-is. The closer you get to zero, the more the conversion to int-plus-nano hurts, relatively speaking.

I'm not sure I understand what you mean. The point of switching to IIO_VAL_INT_PLUS_NANO at the moment is to get more precision on small values. Am I missing something?

Quoted 128 lines · Liam Beguin

Quoted 127 lines · Peter Rosin

Quoted 24 lines · Liam Beguin

Quoted 11 lines · Peter Rosin

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

Hi Peter,

Thanks for taking time to look at this in detail again. I really appreciate all the feedback you've provided.

Quoted 6 lines

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Quoted 6 lines

Quoted 3 lines

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Excellent!

Quoted 11 lines

Expected rel_ppm < 0, but

    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000

Quoted 6 lines

expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000

The main issue is that the first two examples return 0 which night be worst that loosing a little precision.

They shouldn't return zero?

Here's the new code quoted from the test robot (and assuming a 64-bit machine, thus ignoring the 32-bit problem on line 56).

    36		case IIO_VAL_FRACTIONAL:
    37		case IIO_VAL_FRACTIONAL_LOG2:
    38			tmp = (s64)*val * 1000000000LL;
    39			tmp = div_s64(tmp, rescale->denominator);
    40			tmp *= rescale->numerator;
    41	
    42			tmp = div_s64_rem(tmp, 1000000000LL, &rem);
    43			*val = tmp;
    44	
    45			/*
    46			 * For small values, the approximation can be costly,
    47			 * change scale type to maintain accuracy.
    48			 *
    49			 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
    50			 */
    51			if (scale_type == IIO_VAL_FRACTIONAL)
    52				tmp = *val2;
    53			else
    54				tmp = 1 << *val2;
    55	

Quoted 1 line

56 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {

    57				 *val = div_s64_rem(*val, tmp, &rem2);
    58	
    59				 *val2 = div_s64(rem, tmp);
    60				 if (rem2)
    61					 *val2 += div_s64(rem2 * 1000000000LL, tmp);
    62	
    63				 return IIO_VAL_INT_PLUS_NANO;
    64			 }
    65	
    66			return scale_type;

When I go through the above manually, I get:

line

38: tmp = 90000000000    ; 90 * 1000000000
39: tmp = 176817288      ; 90000000000 / 509
40: tmp = 46149312168    ; 176817288 * 261
42: rem = 149312168      ; 46149312168 % 1000000000
    tmp = 46             ; 46149312168 / 1000000000

43: *val = 46
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (149312168 > 10000000 && 46/1373754273 < 100) [yes && yes]

57: rem2 = 46            ; 46 % 1373754273
    *val = 0             ; 46 / 1373754273
59: *val2 = 0            ; 149312168 / 1373754273

60: if 46 [yes]
61: *val2 = 33 ; 0 + 46 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000033]

and

line

38: tmp = 100000000000   ; 100 * 1000000000
39: tmp = 14285714       ; 100000000000 / 7000
40: tmp = 54028570348    ; 176817288 * 3782
42: rem = 28570348       ; 54028570348 % 1000000000
    tmp = 54             ; 54028570348 / 1000000000

43: *val = 54
51: if (<fractional>) [no]
54: tmp = 1073741824 ; 1 << 30
56: if (28570348 > 10000000 && 54/1073741824 < 100) [yes && yes]

57: rem2 = 54            ; 54 % 1073741824
    *val = 0             ; 54 / 1073741824
59: *val2 = 0            ; 28570348 / 1073741824

60: if 46 [yes]
61: *val2 = 50 ; 0 + 54 * 1000000000 / 1073741824
63: return <int-plus-nano> [0.000000050]

Quoted 3 lines

Quoted 1 line

Why do you get zero, what am I missing?

So... It turns out, I swapped schan and rescaler values which gives us:

Ahh, good. The explanation is simple!

Quoted 27 lines

numerator = 90
denominator = 1373754273
schan_val = 261
schan_val2 = 509

line

38: tmp = 261000000000   ; 261 * 1000000000
39: tmp = 189            ; 261000000000 / 1373754273
40: tmp = 17010          ; 189 * 90
42: rem = 17010          ; 17010 % 1000000000
    tmp = 0              ; 17010 / 1000000000

43: *val = 0
51: if (<fractional>) [yes]
52: tmp = 509
56: if (17010 > 10000000 && 0/509 < 100) [no && yes] 66: *val = 0

    *val2 = 509
    return <fractional> [0.000000000]

Swapping back the values, I get the same results as you!

Also, replacing line 56 from the snippet above with

- if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
+ if (abs(rem)) {

Quoted 3 lines

Fixes these precision errors. It also prevents us from returning different scales if we swap the two divisions (schan and rescaler parameters).

No, it doesn't fix the precision problems. Not really, it only reduces them. See below.

*snip*

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

Considering these null values and the possible issue of not always having the same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO scale?

No, that absolutely kills the precision for small values that are much better off as-is. The closer you get to zero, the more the conversion to int-plus-nano hurts, relatively speaking.

I'm not sure I understand what you mean. The point of switching to IIO_VAL_INT_PLUS_NANO at the moment is to get more precision on small values. Am I missing something?

Yes, apparently :-) We always sacrifice accuracy by going to IIO_VAL_INT_PLUS_NANO. More is lost with smaller numbers, relatively. That is an inherent property of fix-point style representations such as IIO_VAL_INT_PLUS_NANO. Unless we get lucky and just happen to be able to represent the desired number exactly of course, but that tends to be both non-interesting and the exception.

Let's go back to the example from my response to the 8/14 patch, i.e. 5/32768 scaled by 3/10000. With the current code this yields

15 / 327680000 (0.0000000457763671875)

Note, the above is /exact/. With IIO_VAL_INT_PLUS_NANO we instead get the truncated 0.000000045

The relative error introduced by the IIO_VAL_INT_PLUS_NANO conversion is almost 1.7% in this case. Sure, rounding instead of truncating would reduce that to 0.5%, but that's not really a significant improvement if you compare to /no/ error. Besides, there are many smaller numbers with even bigger relative conversion "noise".

And remember, this function is used to rescale the scale of the raw values. We are going to multiply the scale and the raw values at some point. If we have rounding errors in the scale, they will multiply with the raw values. It wouldn't look too good if something that should be able to reach 3V with a lot of accuracy (ca 26 bits) instead caps out at 2.94912V (or hits 3.014656V) because of accuracy issues with the scaling (1.7% too low or 0.5% too high).

It's impossible to do better than exact, which is what we have now for IIO_VAL_FRACTIONAL and IIO_VAL_INT (for IIO_VAL_FRACTIONAL_LOG2, not so much...). At least as long as there's no overflow.

Cheers,
Peter

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Jonathan, do you have any input on that?

Some more examples of problematic properties of this patch:

21837/24041 scaled by 427/24727 is 0.01568544672, you get 0.015685446. Ok. But if you reduce the input number, gcd(21837, 24041) -> 29, you have:
753/829 scaled by 427/24727 which still is 0.01568544672 of course, but in this case you get 0.01568154403. Which is less precise. It is unfortunate that input that should be easier to scale may yield worse results.

760/1373754273 scaled by 427/2727 is 8.662580e-8, and 8.662393e-8 is returned. Which is perhaps not great accuracy, but such is life. However. 761/1373754273 scaled by 427/2727 is 8.673978e-8, which is of course greater, but 8.6e-8 is returned. Which is less than what was returned for the smaller 760/1373754273 value above.

Some of these objections are related to what I talked about in v7, i.e.:

Also, changing the calculation so that you get more precision whenever that is possible feels dangerous. I fear linearity breaks and that bigger input cause smaller output due to rounding if the bigger value has to be rounded down, but that this isn't done carefully enough. I.e. attempting to return an exact fraction and only falling back to the old code when that is not possible is still not safe since the old code isn't careful enough about rounding. I think it is really important that bigger input cause bigger (or equal) output. Otherwise you might trigger instability in feedback loops should a rescaler be involved in a some regulator function.

Sadly, I see no elegant solution to your problem.

One way forward may be to somehow provide information on the expected input range, and then determine the scaling method based on that instead of the individual values. But, as indicated, there's no real elegance in that. It can't be automated...

Quoted 34 lines

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/iio/afe/iio-rescale.c +11 -2

index c408c4057c08..7304306c9806 100644
@@ -22,7 +22,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;
 

@@ -38,8 +38,31 @@ 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;
+
+		/*
+		 * For small values, the approximation can be costly,
+		 * change scale type to maintain accuracy.
+		 *
+		 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
+		 */
+		if (scale_type == IIO_VAL_FRACTIONAL)

drivers/iio/afe/iio-rescale.c int rescale_process_scale(struct rescale *rescale, int scale_type, · line 61

Quoted 10 lines

drivers/iio/afe/iio-rescale.c +10 -0

+			tmp = *val2;
+		else
+			tmp = 1 << *val2;
+
+		 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
+			 *val = div_s64_rem(*val, tmp, &rem2);
+
+			 *val2 = div_s64(rem, tmp);
+			 if (rem2)
+				 *val2 += div_s64(rem2 * 1000000000LL, tmp);  

rem2 is 32-bit. Might 1000000000LL also be 32-bit on a small machine where 64-bit arithmetic is really expensive? In that case, the above is broken. The safe route is to do these things as in the existing code with a cast to s64. But maybe that's just cargo cult crap?

Cheers,
Peter

Quoted 8 lines

drivers/iio/afe/iio-rescale.c +4 -0

+
+			 return IIO_VAL_INT_PLUS_NANO;
+		 }
+
 		return scale_type;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:

Quoted 20 lines · On Mon, 23 Aug 2021 00:18:55 +0200 Peter Rosin <[email protected]> wrote:

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

Quoted 6 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Jonathan, do you have any input on that?

I'm a bit lost. As things currently stand IIO buffered data flows all use _RAW. It's either up to userspace or the inkernel user to query scale and use that to compute the appropriate _processed values. There have been various discussions over the years on how to add metadata but it's tricky without adding significant complexity and for vast majority of usecases not necessary. Given the rescaler copes with _raw and _processed inputs, we would only support buffered flows if using the _raw ones.

If nothing changes in configuration of the rescaler, the scale should be static for a given device. What format that 'scale' takes is something that userspace code or inkernel users should cope fine with given they need to do that anyway for different devices.

Quoted 38 lines · Jonathan Cameron

Quoted 20 lines · On Mon, 23 Aug 2021 00:18:55 +0200 Peter Rosin <[email protected]> wrote:

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

Quoted 6 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Jonathan, do you have any input on that?

I'm a bit lost. As things currently stand IIO buffered data flows all use _RAW. It's either up to userspace or the inkernel user to query scale and use that to compute the appropriate _processed values. There have been various discussions over the years on how to add metadata but it's tricky without adding significant complexity and for vast majority of usecases not necessary. Given the rescaler copes with _raw and _processed inputs, we would only support buffered flows if using the _raw ones.

Quoted 4 lines

If nothing changes in configuration of the rescaler, the scale should be static for a given device. What format that 'scale' takes is something that userspace code or inkernel users should cope fine with given they need to do that anyway for different devices.

Ok, if 'scale' (and 'offset') of the source channel is to be considered static, then it is much safer to ignore the "island problem" and rescale each value independently on a case-by-case basis. We should add an explicit comment somewhere that we make this assumption.

Sorry for wasting time and effort by not realizing by myself (and earlier).

Maybe something like this?

/*
* The rescaler assumes that the 'scale' and 'offset' properties of
* the source channel are static. If they are not, there exist some
* corner cases where rounding/truncating might cause confusing
* mathematical properties (non-linearity).
*/

I then propose that we rescale IIO_VAL_FRACTIONAL as before if that works, thus preserving any previous exact rescaling, but if there is an overflow while doing that, then we fall back to new code that rescales to a IIO_VAL_INT_PLUS_NANO value. Trying the gcd-thing as it ended up in v7 still seems expensive to me, but maybe I overestimate the cost of gcd? Anyway, my guts vote for completely skipping gcd and that we aim directly for IIO_VAL_INT_PLUS_NANO in case of overflow while doing the old thing.

Having said that, if 'scale' and 'offset' indeed are static, then the gcd cost can be mitigated by caching the result. Exact rescaling is always nice...

If IIO_VAL_INT overflows while rescaling, we are SOL whichever way we turn, so ignore doing anything about that.

Liam, was it IIO_VAL_FRACTIONAL or IIO_VAL_FRACTIONAL_LOG2 that was your real use case? Anyway, your 100 ppm threshold is probably as good a compromise as any for this case. However, that threshold does nothing for the case where the conversion to IIO_VAL_INT_PLUS_NANO throws away way more precision than the existing operations. It would probably be good to somehow stay with the old method for the really tiny values, if there is a viable test/threshold for when to do what.

Cheers,
Peter

Quoted 38 lines · Jonathan Cameron

Quoted 20 lines · On Mon, 23 Aug 2021 00:18:55 +0200 Peter Rosin <[email protected]> wrote:

[I started to write an answer to your plans in the v7 thread, but didn't have time to finish before v8 appeared...]

Quoted 6 lines · Liam Beguin

From: Liam Beguin <[email protected]>

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.

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8, but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more digits). So, in this case you lose precision with the new code.

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

Quoted 6 lines

I'm also wondering if it is wise to not always return the same scale type? What happens if we want to extend this driver to scale a buffered channel? Honest question! I don't know, but I fear that this patch may make that step more difficult to take??

Jonathan, do you have any input on that?

I'm a bit lost. As things currently stand IIO buffered data flows all use _RAW. It's either up to userspace or the inkernel user to query scale and use that to compute the appropriate _processed values. There have been various discussions over the years on how to add metadata but it's tricky without adding significant complexity and for vast majority of usecases not necessary. Given the rescaler copes with _raw and _processed inputs, we would only support buffered flows if using the _raw ones.

Quoted 4 lines

If nothing changes in configuration of the rescaler, the scale should be static for a given device. What format that 'scale' takes is something that userspace code or inkernel users should cope fine with given they need to do that anyway for different devices.

Ok, if 'scale' (and 'offset') of the source channel is to be considered static, then it is much safer to ignore the "island problem" and rescale each value independently on a case-by-case basis. We should add an explicit comment somewhere that we make this assumption.

Sorry for wasting time and effort by not realizing by myself (and earlier).

Maybe something like this?

/*
* The rescaler assumes that the 'scale' and 'offset' properties of
* the source channel are static. If they are not, there exist some
* corner cases where rounding/truncating might cause confusing
* mathematical properties (non-linearity).
*/

I then propose that we rescale IIO_VAL_FRACTIONAL as before if that works, thus preserving any previous exact rescaling, but if there is an overflow while doing that, then we fall back to new code that rescales to a IIO_VAL_INT_PLUS_NANO value. Trying the gcd-thing as it ended up in v7 still seems expensive to me, but maybe I overestimate the cost of gcd? Anyway, my guts vote for completely skipping gcd and that we aim directly for IIO_VAL_INT_PLUS_NANO in case of overflow while doing the old thing.

Having said that, if 'scale' and 'offset' indeed are static, then the gcd cost can be mitigated by caching the result. Exact rescaling is always nice...

If IIO_VAL_INT overflows while rescaling, we are SOL whichever way we turn, so ignore doing anything about that.

Liam, was it IIO_VAL_FRACTIONAL or IIO_VAL_FRACTIONAL_LOG2 that was your real use case? Anyway, your 100 ppm threshold is probably as good a compromise as any for this case. However, that threshold does nothing for the case where the conversion to IIO_VAL_INT_PLUS_NANO throws away way more precision than the existing operations. It would probably be good to somehow stay with the old method for the really tiny values, if there is a viable test/threshold for when to do what.

Cheers,
Peter

Quoted 7 lines · Peter Rosin

Having said that, if 'scale' and 'offset' indeed are static, then the gcd cost can be mitigated by caching the result. Exact rescaling is always nice...

If IIO_VAL_INT overflows while rescaling, we are SOL whichever way we turn, so ignore doing anything about that.

I was thinking of using check_mul_overflow() to do something about the overflow, but I'm happy to leave it out for now.

From: Liam Beguin <[email protected]>

A temperature transducer is a device that converts a thermal quantity into any other physical quantity. This patch add support for temperature to voltage (like the LTC2997) and temperature to current (like the AD590) linear transducers.
In both cases these are assumed to be connected to a voltage ADC.

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

drivers/iio/afe/iio-rescale.c +12 -0

index 8cdcb6ffb563..12de44058bea 100644
@@ -427,11 +427,38 @@ static int rescale_temp_sense_rtd_props(struct device *dev,
 	return 0;
 }
 
+static int rescale_temp_transducer_props(struct device *dev,
+					 struct rescale *rescale)
+{
+	s32 offset = 0;
+	s32 sense = 1;
+	s32 alpha;
+	s64 tmp;
+	int ret;
+
+	device_property_read_u32(dev, "sense-offset-millicelsius", &offset);
+	device_property_read_u32(dev, "sense-resistor-ohms", &sense);
+	ret = device_property_read_u32(dev, "alpha-ppm-per-celsius", &alpha);

drivers/iio/afe/iio-rescale.c +11 -0

+
+	return 0;
+}
+
 enum rescale_variant {
 	CURRENT_SENSE_AMPLIFIER,
 	CURRENT_SENSE_SHUNT,
 	VOLTAGE_DIVIDER,
 	TEMP_SENSE_RTD,
+	TEMP_TRANSDUCER,
 };
 
 static const struct rescale_cfg rescale_cfg[] = {

@@ -451,6 +478,10 @@ static const struct rescale_cfg rescale_cfg[] = {
 		.type = IIO_TEMP,
 		.props = rescale_temp_sense_rtd_props,
 	},
+	[TEMP_TRANSDUCER] = {
+		.type = IIO_TEMP,
+		.props = rescale_temp_transducer_props,
+	},
 };
 
 static const struct of_device_id rescale_match[] = {

@@ -462,6 +493,8 @@ static const struct of_device_id rescale_match[] = {
 	  .data = &rescale_cfg[VOLTAGE_DIVIDER], },
 	{ .compatible = "temperature-sense-rtd",
 	  .data = &rescale_cfg[TEMP_SENSE_RTD], },
+	{ .compatible = "temperature-transducer",
+	  .data = &rescale_cfg[TEMP_TRANSDUCER], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, rescale_match);

Quoted 37 lines · Liam Beguin

From: Liam Beguin <[email protected]>

A temperature transducer is a device that converts a thermal quantity into any other physical quantity. This patch add support for temperature to voltage (like the LTC2997) and temperature to current (like the AD590) linear transducers.
In both cases these are assumed to be connected to a voltage ADC.

Signed-off-by Liam Beguin <[email protected]>

---
 drivers/iio/afe/iio-rescale.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

drivers/iio/afe/iio-rescale.c +16 -0

index 8cdcb6ffb563..12de44058bea 100644
@@ -427,11 +427,38 @@ static int rescale_temp_sense_rtd_props(struct device *dev,
 	return 0;
 }
 
+static int rescale_temp_transducer_props(struct device *dev,
+					 struct rescale *rescale)
+{
+	s32 offset = 0;
+	s32 sense = 1;
+	s32 alpha;
+	s64 tmp;
+	int ret;
+
+	device_property_read_u32(dev, "sense-offset-millicelsius", &offset);
+	device_property_read_u32(dev, "sense-resistor-ohms", &sense);
+	ret = device_property_read_u32(dev, "alpha-ppm-per-celsius", &alpha);
+	if (ret) {
+		dev_err(dev, "failed to read alpha-ppm-per-celsius: %d\n", ret);
+		return ret;
+	}

drivers/iio/afe/iio-rescale.c static int rescale_temp_sense_rtd_props(struct device *dev, · line 451

Quoted 6 lines

drivers/iio/afe/iio-rescale.c +6 -0

+
+	rescale->numerator = 1000000;
+	rescale->denominator = alpha * sense;
+
+	tmp = (s64)offset * (s64)alpha * (s64)sense;
+	rescale->offset = div_s64(tmp, (s32)1000000);

Error: Too many casts :-)

I think it would make sense to lose tmp, and just spell it out in one statement?

	rescale->offset = div_s64((s64)offset * rescale->denominator,
				  rescale->numerator);

Because you are prepping the offset so that it will survive a later rescaler multiplication, and all the "random" multiplications and divisions don't make that very clear.

Cheers,
Peter

Quoted 34 lines

drivers/iio/afe/iio-rescale.c +11 -0

+
+	return 0;
+}
+
 enum rescale_variant {
 	CURRENT_SENSE_AMPLIFIER,
 	CURRENT_SENSE_SHUNT,
 	VOLTAGE_DIVIDER,
 	TEMP_SENSE_RTD,
+	TEMP_TRANSDUCER,
 };
 
 static const struct rescale_cfg rescale_cfg[] = {

@@ -451,6 +478,10 @@ static const struct rescale_cfg rescale_cfg[] = {
 		.type = IIO_TEMP,
 		.props = rescale_temp_sense_rtd_props,
 	},
+	[TEMP_TRANSDUCER] = {
+		.type = IIO_TEMP,
+		.props = rescale_temp_transducer_props,
+	},
 };
 
 static const struct of_device_id rescale_match[] = {

@@ -462,6 +493,8 @@ static const struct of_device_id rescale_match[] = {
 	  .data = &rescale_cfg[VOLTAGE_DIVIDER], },
 	{ .compatible = "temperature-sense-rtd",
 	  .data = &rescale_cfg[TEMP_SENSE_RTD], },
+	{ .compatible = "temperature-transducer",
+	  .data = &rescale_cfg[TEMP_TRANSDUCER], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, rescale_match);