Add temperature rescaling support to the IIO Analog Front End driver.
This series includes minor bug fixes and adds support for RTD temperature
sensors as well as temperature transducers.
At first I tried to use iio_convert_raw_to_processed() to get more
precision out of processed values but ran into issues when one of my
ADCs didn't provide a scale. This was addressed in the first few
patches.
When adding offset support to iio-rescale, I also noticed that
iio_read_channel_processed() assumes that the offset is always an
integer which I tried to address in the third patch without breaking
valid implicit truncations.
As Jonathan suggested, I also started a Kunit test suite for the
iio-rescale driver. Adding the test cases, I ran into accuracy and
overflow issues, these was addressed in 07-09/14.
Changes since v7:
- drop gcd() logic in rescale_process_scale()
- use div_s64() instead of do_div() for signed 64-bit divisions
- combine IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2 scale cases
- switch to INT_PLUS_NANO when accuracy is lost with FRACTIONAL scales
- rework test logic to allow for small relative error
- rename test variables to align error output messages
Changes since v6:
- rework IIO_VAL_INT_PLUS_{NANO,MICRO} based on Peter's suggestion
- combine IIO_VAL_INT_PLUS_{NANO,MICRO} cases
- add test cases for negative IIO_VAL_INT_PLUS_{NANO,MICRO} corner cases
- force use of positive integers with gcd()
- reduce risk of integer overflow in IIO_VAL_FRACTIONAL_LOG2
- fix duplicate symbol build error
- apply Reviewed-by
Changes since v5:
- add include/linux/iio/afe/rescale.h
- expose functions use to process scale and offset
- add basic iio-rescale kunit test cases
- fix integer overflow case
- improve precision for IIO_VAL_FRACTIONAL_LOG2
Changes since v4:
- only use gcd() when necessary in overflow mitigation
- fix INT_PLUS_{MICRO,NANO} support
- apply Reviewed-by
- fix temperature-transducer bindings
Changes since v3:
- drop unnecessary fallthrough statements
- drop redundant local variables in some calculations
- fix s64 divisions on 32bit platforms by using do_div
- add comment describing iio-rescaler offset calculation
- drop unnecessary MAINTAINERS entry
Changes since v2:
- don't break implicit offset truncations
- make a best effort to get a valid value for fractional types
- drop return value change in iio_convert_raw_to_processed_unlocked()
- don't rely on processed value for offset calculation
- add INT_PLUS_{MICRO,NANO} support in iio-rescale
- revert generic implementation in favor of temperature-sense-rtd and
temperature-transducer
- add separate section to MAINTAINERS file
Changes since v1:
- rebase on latest iio `testing` branch
- also apply consumer scale on integer channel scale types
- don't break implicit truncation in processed channel offset
calculation
- drop temperature AFE flavors in favor of a simpler generic
implementation
Thanks for your time
Liam Beguin (13):
iio: inkern: apply consumer scale on IIO_VAL_INT cases
iio: inkern: apply consumer scale when no channel scale is available
iio: inkern: make a best effort on offset calculation
iio: afe: rescale: expose scale processing function
iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
iio: afe: rescale: add offset support
iio: afe: rescale: use s64 for temporary scale calculations
iio: afe: rescale: reduce risk of integer overflow
iio: afe: rescale: fix accuracy for small fractional scales
iio: test: add basic tests for the iio-rescale driver
iio: afe: rescale: add RTD temperature sensor support
iio: afe: rescale: add temperature transducers
dt-bindings: iio: afe: add bindings for temperature-sense-rtd
dt-bindings: iio: afe: add bindings for temperature transducers
.../iio/afe/temperature-sense-rtd.yaml | 101 +++
.../iio/afe/temperature-transducer.yaml | 114 ++++
drivers/iio/afe/iio-rescale.c | 277 +++++++-
drivers/iio/inkern.c | 40 +-
drivers/iio/test/Kconfig | 10 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-rescale.c | 637 ++++++++++++++++++
include/linux/iio/afe/rescale.h | 34 +
8 files changed, 1170 insertions(+), 44 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
create mode 100644 drivers/iio/test/iio-test-rescale.c
create mode 100644 include/linux/iio/afe/rescale.h
Range-diff against v7:
-: ------------ > 1: 42a7a1047edc iio: inkern: apply consumer scale on IIO_VAL_INT cases
-: ------------ > 2: a1cd89fdad11 iio: inkern: apply consumer scale when no channel scale is available
-: ------------ > 3: ed0721fb6bd1 iio: inkern: make a best effort on offset calculation
-: ------------ > 4: f8fb78bb1112 iio: afe: rescale: expose scale processing function
1: 014363f5f703 ! 5: 504b7a3f830b iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
@@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale
int *val, int *val2)
{
unsigned long long tmp;
++ s32 rem;
+ u32 mult;
-+ u32 rem;
+ u32 neg;
switch (scale_type) {
2: e4e3bcd752dd = 6: c254e9ae813e iio: afe: rescale: add offset support
3: 23efde61c576 < -: ------------ iio: afe: rescale: reduce risk of integer overflow
4: e16f69d61f09 < -: ------------ iio: afe: rescale: fix precision on fractional log scale
-: ------------ > 7: ee8814d6abe4 iio: afe: rescale: use s64 for temporary scale calculations
-: ------------ > 8: 62cdcfbc9836 iio: afe: rescale: reduce risk of integer overflow
-: ------------ > 9: 88309a5136ee iio: afe: rescale: fix accuracy for small fractional scales
5: 88ad312c5a1b ! 10: fb505a9f42f1 iio: test: add basic tests for the iio-rescale driver
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 42,
-+ .expected = "5210.918114143\n",
++ .expected = "5210.918114143",
+ },
+ {
+ .name = "typical IIO_VAL_INT, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 42,
-+ .expected = "-5210.918114143\n",
++ .expected = "-5210.918114143",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 42,
+ .schan_val2 = 20,
-+ .expected = "260.545905707\n",
++ .expected = "260.545905707",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 42,
+ .schan_val2 = 20,
-+ .expected = "-260.545905707\n",
++ .expected = "-260.545905707",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
-+ .expected = "0.049528301\n",
++ .expected = "0.049528301",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
-+ .expected = "-0.049528301\n",
++ .expected = "-0.049528301",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456,
-+ .expected = "1240.710106203\n",
++ .expected = "1240.710106203",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456,
-+ .expected = "-1240.710106203\n",
++ .expected = "-1240.710106203",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 1234,
-+ .expected = "1240.847890\n",
++ .expected = "1240.84789",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 1234,
-+ .expected = "-1240.847890\n",
++ .expected = "-1240.84789",
++ },
++ /*
++ * Use cases with small scales involving divisions
++ */
++ {
++ .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 1",
++ .numerator = 6,
++ .denominator = 6,
++ .schan_scale_type = IIO_VAL_FRACTIONAL,
++ .schan_val = 1,
++ .schan_val2 = 3,
++ .expected = "0.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL, -1 < scale < 0",
++ .numerator = -6,
++ .denominator = 6,
++ .schan_scale_type = IIO_VAL_FRACTIONAL,
++ .schan_val = 1,
++ .schan_val2 = 3,
++ .expected = "-0.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 2",
++ .numerator = 8,
++ .denominator = 2,
++ .schan_scale_type = IIO_VAL_FRACTIONAL,
++ .schan_val = 1,
++ .schan_val2 = 3,
++ .expected = "1.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL, -2 < scale < 0",
++ .numerator = -8,
++ .denominator = 2,
++ .schan_scale_type = IIO_VAL_FRACTIONAL,
++ .schan_val = 1,
++ .schan_val2 = 3,
++ .expected = "-1.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 1",
++ .numerator = 16,
++ .denominator = 3,
++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
++ .schan_val = 1,
++ .schan_val2 = 4,
++ .expected = "0.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL_LOG2, -1 < scale < 0",
++ .numerator = -16,
++ .denominator = 3,
++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
++ .schan_val = 1,
++ .schan_val2 = 4,
++ .expected = "-0.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 2",
++ .numerator = 8,
++ .denominator = 3,
++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
++ .schan_val = 1,
++ .schan_val2 = 1,
++ .expected = "1.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_FRACTIONAL_LOG2, -2 < scale < 0",
++ .numerator = -8,
++ .denominator = 3,
++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
++ .schan_val = 1,
++ .schan_val2 = 1,
++ .expected = "-1.3333333333333333",
++ },
++ {
++ .name = "small IIO_VAL_INT_PLUS_MICRO, positive",
++ .numerator = 1,
++ .denominator = 2,
++ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
++ .schan_val = 5,
++ .schan_val2 = 1234,
++ .expected = "2.500617",
++ },
++ {
++ .name = "small IIO_VAL_INT_PLUS_MICRO, negative",
++ .numerator = -1,
++ .denominator = 2,
++ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
++ .schan_val = 5,
++ .schan_val2 = 1234,
++ .expected = "-2.500617",
+ },
+ /*
+ * INT_PLUS_{MICRO,NANO} positive/negative corner cases
+ */
+ {
-+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative schan",
++ .name = "negative IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
-+ .expected = "-1240.710106203\n",
++ .expected = "-1240.710106203",
+ },
+ {
-+ .name = "typical IIO_VAL_INT_PLUS_NANO, both negative",
++ .name = "negative IIO_VAL_INT_PLUS_NANO, both negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
-+ .expected = "1240.710106203\n",
++ .expected = "1240.710106203",
+ },
+ {
-+ .name = "typical IIO_VAL_INT_PLUS_NANO, 3 negative",
++ .name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative",
+ .numerator = -1000000,
+ .denominator = -8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
-+ .expected = "-1240.710106203\n",
++ .expected = "-1240.710106203",
+ },
+ {
-+ .name = "typical IIO_VAL_INT_PLUS_NANO, 4 negative",
++ .name = "negative IIO_VAL_INT_PLUS_NANO, 4 negative",
+ .numerator = -1000000,
+ .denominator = -8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = -123456,
-+ .expected = "-1240.710106203\n",
++ .expected = "-1240.710106203",
+ },
+ {
-+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative, *val = 0",
++ .name = "negative IIO_VAL_INT_PLUS_NANO, negative, *val = 0",
+ .numerator = 1,
+ .denominator = -10,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 0,
+ .schan_val2 = 123456789,
-+ .expected = "-0.012345678\n",
++ .expected = "-0.012345678",
+ },
+ /*
+ * INT_PLUS_{MICRO,NANO} decimal part overflow
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
-+ .expected = "1256.012008560\n",
++ .expected = "1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
-+ .expected = "-1256.012008560\n",
++ .expected = "-1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, negative schan",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456789,
-+ .expected = "-1256.012008560\n",
++ .expected = "-1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
-+ .expected = "16557.914267\n",
++ .expected = "16557.914267",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
-+ .expected = "-16557.914267\n",
++ .expected = "-16557.914267",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, negative schan",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = -10,
+ .schan_val2 = 123456789,
-+ .expected = "-16557.914267\n",
++ .expected = "-16557.914267",
+ },
+ /*
+ * 32-bit overflow conditions
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
-+ .schan_val = 0x7FFFFFFF,
++ .schan_val = S32_MAX,
+ .schan_val2 = 1,
-+ .expected = "214748364.700000000\n",
++ .expected = "214748364.7",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
-+ .schan_val = 0x7FFFFFFF,
++ .schan_val = S32_MAX,
+ .schan_val2 = 1,
-+ .expected = "-214748364.700000000\n",
++ .expected = "-214748364.7",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL_LOG2, positive",
-+ .numerator = 0x7FFFFFFF,
-+ .denominator = 53,
++ .numerator = S32_MAX,
++ .denominator = 4096,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
-+ .expected = "2532409.961084905\n",
++ .expected = "32767.99998474121",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL_LOG2, negative",
-+ .numerator = 0x7FFFFFFF,
-+ .denominator = 53,
++ .numerator = S32_MAX,
++ .denominator = 4096,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = -4096,
+ .schan_val2 = 16,
-+ .expected = "-2532409.961084905\n",
++ .expected = "-32767.99998474121",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
-+ .schan_val2 = 0x7fffffff,
-+ .expected = "1.214748364\n",
++ .schan_val2 = S32_MAX,
++ .expected = "1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
-+ .schan_val2 = 0x7fffffff,
-+ .expected = "-1.214748364\n",
++ .schan_val2 = S32_MAX,
++ .expected = "-1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, negative schan",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
-+ .schan_val2 = 0x7fffffff,
-+ .expected = "-1.214748364\n",
++ .schan_val2 = S32_MAX,
++ .expected = "-1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
-+ .schan_val2 = 0x7fffffff,
-+ .expected = "215.748364\n",
++ .schan_val2 = S32_MAX,
++ .expected = "215.748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
-+ .schan_val2 = 0x7fffffff,
-+ .expected = "-215.748364\n",
++ .schan_val2 = S32_MAX,
++ .expected = "-215.748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, negative schan",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = -10,
-+ .schan_val2 = 0x7fffffff,
-+ .expected = "-215.748364\n",
++ .schan_val2 = S32_MAX,
++ .expected = "-215.748364",
+ },
+};
+
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 123,
+ .schan_val2 = 0,
+ .schan_off = 14,
-+ .expected_off = "24\n", /* 23.872 */
++ .expected_off = "24", /* 23.872 */
+ },
+ {
+ .name = "typical IIO_VAL_INT, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 12,
+ .schan_val2 = 0,
+ .schan_off = 14,
-+ .expected_off = "-88\n", /* -88.83333333333333 */
++ .expected_off = "-88", /* -88.83333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 12,
+ .schan_val2 = 34,
+ .schan_off = 14,
-+ .expected_off = "3510\n", /* 3510.333333333333 */
++ .expected_off = "3510", /* 3510.333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 12,
+ .schan_val2 = 34,
+ .schan_off = 14,
-+ .expected_off = "-3482\n", /* -3482.333333333333 */
++ .expected_off = "-3482", /* -3482.333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 12,
+ .schan_val2 = 16,
+ .schan_off = 14,
-+ .expected_off = "6739299\n", /* 6739299.333333333 */
++ .expected_off = "6739299", /* 6739299.333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 12,
+ .schan_val2 = 16,
+ .schan_off = 14,
-+ .expected_off = "-6739271\n", /* -6739271.333333333 */
++ .expected_off = "-6739271", /* -6739271.333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
-+ .expected_off = "135\n", /* 135.8951219647469 */
++ .expected_off = "135", /* 135.8951219647469 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
-+ .expected_off = "-107\n", /* -107.89512196474689 */
++ .expected_off = "-107", /* -107.89512196474689 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, positive",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
-+ .expected_off = "23\n", /* 23.246438560723952 */
++ .expected_off = "23", /* 23.246438560723952 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, negative",
@@ drivers/iio/test/iio-test-rescale.c (new)
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
-+ .expected_off = "-78\n", /* -78.50185091745313 */
++ .expected_off = "-78", /* -78.50185091745313 */
+ },
+};
+
@@ drivers/iio/test/iio-test-rescale.c (new)
+KUNIT_ARRAY_PARAM(iio_rescale_scale, scale_cases, case_to_desc);
+KUNIT_ARRAY_PARAM(iio_rescale_offset, offset_cases, case_to_desc);
+
++/**
++ * iio_str_to_micro() - Parse a fixed-point string to get an
++ * IIO_VAL_INT_PLUS_MICRO value
++ * @str: The string to parse
++ * @micro: The number as an integer
++ *
++ * Returns 0 on success, or a negative error code if the string cound not be
++ * parsed.
++ */
++static int iio_str_to_micro(const char *str, s64 *micro)
++{
++ int fract_mult = 100000LL;
++ int tmp, tmp2;
++ int ret = 0;
++
++ ret = iio_str_to_fixpoint(str, fract_mult, &tmp, &tmp2);
++ if (ret < 0)
++ return ret;
++
++ if (tmp < 0)
++ tmp2 *= -1;
++
++ *micro = (s64)tmp * 10 * fract_mult + tmp2;
++
++ return ret;
++}
++
++/**
++ * iio_test_relative_error_ppm() - Compute relative error (in ppm) between two
++ * fixed-point strings
++ * @real_str: The real value as a string
++ * @exp_str: The expected value as a string
++ *
++ * Returns a negative error code if the strings cound not be parsed, or the
++ * relative error in ppm.
++ */
++static int iio_test_relative_error_ppm(const char *real_str, const char *exp_str)
++{
++ s64 real, exp, err;
++ int ret;
++
++ ret = iio_str_to_micro(real_str, &real);
++ if (ret < 0)
++ return ret;
++
++ ret = iio_str_to_micro(exp_str, &exp);
++ if (ret < 0)
++ return ret;
++
++ err = 1000000 * abs(exp - real);
++ err = div64_u64(err, abs(exp));
++ return (int)err;
++}
++
+static void iio_rescale_test_scale(struct kunit *test)
+{
+ struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
-+ char *buf = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
++ char *buff = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+ struct rescale rescale;
+ int values[2];
++ int rel_ppm;
+ int ret;
+
+ rescale.numerator = t->numerator;
@@ drivers/iio/test/iio-test-rescale.c (new)
+ values[1] = t->schan_val2;
+
+ ret = rescale_process_scale(&rescale, t->schan_scale_type,
-+ &values[0], &values[1]);
++ &values[0], &values[1]);
++
++ ret = iio_format_value(buff, ret, 2, values);
++ KUNIT_EXPECT_EQ(test, (int)strlen(buff), ret);
+
-+ ret = iio_format_value(buf, ret, 2, values);
++ rel_ppm = iio_test_relative_error_ppm(buff, t->expected);
++ KUNIT_EXPECT_GE_MSG(test, rel_ppm, 0, "failed to compute ppm\n");
+
-+ KUNIT_EXPECT_EQ(test, (int)strlen(buf), ret);
-+ KUNIT_EXPECT_STREQ(test, buf, t->expected);
++ KUNIT_EXPECT_LT_MSG(test, rel_ppm, 500,
++ "\t real=%s"
++ "\texpected=%s\n",
++ buff, t->expected);
+}
+
+static void iio_rescale_test_offset(struct kunit *test)
+{
+ struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
-+ char *buf = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
++ char *buff_off = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+ struct rescale rescale;
+ int values[2];
+ int ret;
@@ drivers/iio/test/iio-test-rescale.c (new)
+ t->schan_val, t->schan_val2, t->schan_off,
+ &values[0], &values[1]);
+
-+ ret = iio_format_value(buf, ret, 2, values);
++ ret = iio_format_value(buff_off, ret, 2, values);
++ KUNIT_EXPECT_EQ(test, (int)strlen(buff_off), ret);
+
-+ KUNIT_EXPECT_EQ(test, (int)strlen(buf), ret);
-+ KUNIT_EXPECT_STREQ(test, buf, t->expected_off);
++ KUNIT_EXPECT_STREQ(test, strim(buff_off), t->expected_off);
+}
+
+static struct kunit_case iio_rescale_test_cases[] = {
6: 2c4a31e62c31 = 11: 050487186e14 iio: afe: rescale: add RTD temperature sensor support
7: d1258a38d194 = 12: f36a44a5d898 iio: afe: rescale: add temperature transducers
8: 01fa34bf5362 = 13: 63be647fd110 dt-bindings: iio: afe: add bindings for temperature-sense-rtd
9: 107e61e09eec = 14: c2f5c19dece3 dt-bindings: iio: afe: add bindings for temperature transducers
base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
When a consumer calls iio_read_channel_processed() and no channel scale
is available, it's assumed that the scale is one and the raw value is
returned as expected.
On the other hand, if the consumer calls iio_convert_raw_to_processed()
the scaling factor requested by the consumer is not applied.
This for example causes the consumer to process mV when expecting uV.
Make sure to always apply the scaling factor requested by the consumer.
Fixes: adc8ec5ff183 ("iio: inkern: pass through raw values if no scaling")
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/inkern.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b752fe5818e7..b69027690ed5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -590,10 +590,10 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
IIO_CHAN_INFO_SCALE);
if (scale_type < 0) {
/*
- * Just pass raw values as processed if no scaling is
- * available.
+ * If no channel scaling is available apply consumer scale to
+ * raw value and return.
*/
- *processed = raw;
+ *processed = raw * scale;
return 0;
}
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
iio_convert_raw_to_processed_unlocked() assumes the offset is an
integer. Make a best effort to get a valid offset value for fractional
cases without breaking implicit truncations.
Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/inkern.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b69027690ed5..5e74d8983874 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -578,13 +578,35 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
int raw, int *processed, unsigned int scale)
{
- int scale_type, scale_val, scale_val2, offset;
+ int scale_type, scale_val, scale_val2;
+ int offset_type, offset_val, offset_val2;
s64 raw64 = raw;
- int ret;
- ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET);
- if (ret >= 0)
- raw64 += offset;
+ offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
+ IIO_CHAN_INFO_OFFSET);
+ if (offset_type >= 0) {
+ switch (offset_type) {
+ case IIO_VAL_INT:
+ break;
+ case IIO_VAL_INT_PLUS_MICRO:
+ case IIO_VAL_INT_PLUS_NANO:
+ /*
+ * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO
+ * implicitely truncate the offset to it's integer form.
+ */
+ break;
+ case IIO_VAL_FRACTIONAL:
+ offset_val /= offset_val2;
+ break;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ offset_val /= (1 << offset_val2);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ raw64 += offset_val;
+ }
scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
IIO_CHAN_INFO_SCALE);
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
In preparation for the addition of kunit tests, expose the logic
responsible for combining channel scales.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 65 ++++++++++++++-------------------
include/linux/iio/afe/rescale.h | 30 +++++++++++++++
2 files changed, 58 insertions(+), 37 deletions(-)
create mode 100644 include/linux/iio/afe/rescale.h
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 774eb3044edd..d0669fd8eac5 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -11,35 +11,46 @@
#include <linux/gcd.h>
#include <linux/iio/consumer.h>
#include <linux/iio/iio.h>
+#include <linux/iio/afe/rescale.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/property.h>
-struct rescale;
-
-struct rescale_cfg {
- enum iio_chan_type type;
- int (*props)(struct device *dev, struct rescale *rescale);
-};
+int rescale_process_scale(struct rescale *rescale, int scale_type,
+ int *val, int *val2)
+{
+ unsigned long long tmp;
-struct rescale {
- const struct rescale_cfg *cfg;
- struct iio_channel *source;
- struct iio_chan_spec chan;
- struct iio_chan_spec_ext_info *ext_info;
- bool chan_processed;
- s32 numerator;
- s32 denominator;
-};
+ 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_LOG2:
+ tmp = *val * 1000000000LL;
+ do_div(tmp, rescale->denominator);
+ tmp *= rescale->numerator;
+ do_div(tmp, 1000000000LL);
+ *val = tmp;
+ return scale_type;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
static int rescale_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
struct rescale *rescale = iio_priv(indio_dev);
- unsigned long long tmp;
int ret;
switch (mask) {
@@ -65,27 +76,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
} else {
ret = iio_read_channel_scale(rescale->source, val, val2);
}
- switch (ret) {
- case IIO_VAL_FRACTIONAL:
- *val *= rescale->numerator;
- *val2 *= rescale->denominator;
- return ret;
- case IIO_VAL_INT:
- *val *= rescale->numerator;
- if (rescale->denominator == 1)
- return ret;
- *val2 = rescale->denominator;
- return IIO_VAL_FRACTIONAL;
- case IIO_VAL_FRACTIONAL_LOG2:
- tmp = *val * 1000000000LL;
- do_div(tmp, rescale->denominator);
- tmp *= rescale->numerator;
- do_div(tmp, 1000000000LL);
- *val = tmp;
- return ret;
- default:
- return -EOPNOTSUPP;
- }
+ return rescale_process_scale(rescale, ret, val, val2);
default:
return -EINVAL;
}
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
new file mode 100644
index 000000000000..14d4ee1227c6
--- /dev/null
+++ b/include/linux/iio/afe/rescale.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 Liam Beguin <[email protected]>
+ */
+
+#ifndef __IIO_RESCALE_H__
+#define __IIO_RESCALE_H__
+
+#include <linux/iio/iio.h>
+
+struct rescale;
+
+struct rescale_cfg {
+ enum iio_chan_type type;
+ int (*props)(struct device *dev, struct rescale *rescale);
+};
+
+struct rescale {
+ const struct rescale_cfg *cfg;
+ struct iio_channel *source;
+ struct iio_chan_spec chan;
+ struct iio_chan_spec_ext_info *ext_info;
+ bool chan_processed;
+ s32 numerator;
+ s32 denominator;
+};
+
+int rescale_process_scale(struct rescale *rescale, int scale_type,
+ int *val, int *val2);
+#endif /* __IIO_RESCALE_H__ */
--
2.32.0.452.g940fe202adcb
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(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index d0669fd8eac5..8488f1d83527 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -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;
+ else
+ mult = 1000000LL;
+ /*
+ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
+ * *val2 is negative the schan scale is negative
+ */
+ 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;
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
All four scaling coefficients can take signed values.
Make tmp a signed 64-bit integer and switch to div_s64() to preserve
signs during 64-bit divisions.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 48a8096f68ca..809e966f7058 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -21,7 +21,7 @@
int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2)
{
- unsigned long long tmp;
+ s64 tmp;
s32 rem;
u32 mult;
u32 neg;
@@ -38,10 +38,10 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
*val2 = rescale->denominator;
return IIO_VAL_FRACTIONAL;
case IIO_VAL_FRACTIONAL_LOG2:
- tmp = *val * 1000000000LL;
- do_div(tmp, rescale->denominator);
+ tmp = (s64)*val * 1000000000LL;
+ tmp = div_s64(tmp, rescale->denominator);
tmp *= rescale->numerator;
- do_div(tmp, 1000000000LL);
+ tmp = div_s64(tmp, 1000000000LL);
*val = tmp;
return scale_type;
case IIO_VAL_INT_PLUS_NANO:
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
This is a preparatory change required for the addition of temperature
sensing front ends.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 80 +++++++++++++++++++++++++++++++++
include/linux/iio/afe/rescale.h | 4 ++
2 files changed, 84 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 8488f1d83527..48a8096f68ca 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -81,11 +81,46 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
}
}
+int rescale_process_offset(struct rescale *rescale, int scale_type,
+ int scale, int scale2, int schan_off,
+ int *val, int *val2)
+{
+ s64 tmp, tmp2;
+
+ switch (scale_type) {
+ case IIO_VAL_FRACTIONAL:
+ tmp = (s64)rescale->offset * scale2;
+ *val = div_s64(tmp, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT:
+ *val = div_s64(rescale->offset, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ tmp = (s64)rescale->offset * (1 << scale2);
+ *val = div_s64(tmp, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT_PLUS_NANO:
+ tmp = (s64)rescale->offset * 1000000000;
+ tmp2 = ((s64)scale * 1000000000L) + scale2;
+ *val = div64_s64(tmp, tmp2) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT_PLUS_MICRO:
+ tmp = (s64)rescale->offset * 1000000L;
+ tmp2 = ((s64)scale * 1000000L) + scale2;
+ *val = div64_s64(tmp, tmp2) + schan_off;
+ return IIO_VAL_INT;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static int rescale_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
struct rescale *rescale = iio_priv(indio_dev);
+ int scale, scale2;
+ int schan_off = 0;
int ret;
switch (mask) {
@@ -112,6 +147,47 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
ret = iio_read_channel_scale(rescale->source, val, val2);
}
return rescale_process_scale(rescale, ret, val, val2);
+ case IIO_CHAN_INFO_OFFSET:
+ /*
+ * Processed channels are scaled 1-to-1 and source offset is
+ * already taken into account.
+ *
+ * In other cases, real world measurement are expressed as:
+ *
+ * schan_scale * (raw + schan_offset)
+ *
+ * Given that the rescaler parameters are applied recursively:
+ *
+ * rescaler_scale * (schan_scale * (raw + schan_offset) +
+ * rescaler_offset)
+ *
+ * Or,
+ *
+ * (rescaler_scale * schan_scale) * (raw +
+ * (schan_offset + rescaler_offset / schan_scale)
+ *
+ * Thus, reusing the original expression the parameters exposed
+ * to userspace are:
+ *
+ * scale = schan_scale * rescaler_scale
+ * offset = schan_offset + rescaler_offset / schan_scale
+ */
+ if (rescale->chan_processed) {
+ *val = rescale->offset;
+ return IIO_VAL_INT;
+ }
+
+ if (iio_channel_has_info(rescale->source->channel,
+ IIO_CHAN_INFO_OFFSET)) {
+ ret = iio_read_channel_offset(rescale->source,
+ &schan_off, NULL);
+ if (ret != IIO_VAL_INT)
+ return ret < 0 ? ret : -EOPNOTSUPP;
+ }
+
+ ret = iio_read_channel_scale(rescale->source, &scale, &scale2);
+ return rescale_process_offset(rescale, ret, scale, scale2,
+ schan_off, val, val2);
default:
return -EINVAL;
}
@@ -188,6 +264,9 @@ static int rescale_configure_channel(struct device *dev,
chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE);
+ if (rescale->offset)
+ chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
+
/*
* Using .read_avail() is fringe to begin with and makes no sense
* whatsoever for processed channels, so we make sure that this cannot
@@ -352,6 +431,7 @@ static int rescale_probe(struct platform_device *pdev)
rescale->cfg = of_device_get_match_data(dev);
rescale->numerator = 1;
rescale->denominator = 1;
+ rescale->offset = 0;
ret = rescale->cfg->props(dev, rescale);
if (ret)
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
index 14d4ee1227c6..b152ac487403 100644
--- a/include/linux/iio/afe/rescale.h
+++ b/include/linux/iio/afe/rescale.h
@@ -23,8 +23,12 @@ struct rescale {
bool chan_processed;
s32 numerator;
s32 denominator;
+ s32 offset;
};
int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2);
+int rescale_process_offset(struct rescale *rescale, int scale_type,
+ int scale, int scale2, int schan_off,
+ int *val, int *val2);
#endif /* __IIO_RESCALE_H__ */
--
2.32.0.452.g940fe202adcb
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(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index c408c4057c08..7304306c9806 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -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);
+
+ *val2 = div_s64(rem, tmp);
+ if (rem2)
+ *val2 += div_s64(rem2 * 1000000000LL, tmp);
+
+ return IIO_VAL_INT_PLUS_NANO;
+ }
+
return scale_type;
case IIO_VAL_INT_PLUS_NANO:
case IIO_VAL_INT_PLUS_MICRO:
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
An RTD (Resistance Temperature Detector) is a kind of temperature
sensor used to get a linear voltage to temperature reading within a
give range (usually 0 to 100 degrees Celsius). Common types of RTDs
include PT100, PT500, and PT1000.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 48 +++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 7304306c9806..8cdcb6ffb563 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -386,10 +386,52 @@ static int rescale_voltage_divider_props(struct device *dev,
return 0;
}
+static int rescale_temp_sense_rtd_props(struct device *dev,
+ struct rescale *rescale)
+{
+ u32 factor;
+ u32 alpha;
+ u32 iexc;
+ u32 tmp;
+ int ret;
+ u32 r0;
+
+ ret = device_property_read_u32(dev, "excitation-current-microamp",
+ &iexc);
+ if (ret) {
+ dev_err(dev, "failed to read excitation-current-microamp: %d\n",
+ ret);
+ return ret;
+ }
+
+ 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;
+ }
+
+ ret = device_property_read_u32(dev, "r-naught-ohms", &r0);
+ if (ret) {
+ dev_err(dev, "failed to read r-naught-ohms: %d\n", ret);
+ return ret;
+ }
+
+ tmp = r0 * iexc * alpha / 1000000;
+ factor = gcd(tmp, 1000000);
+ rescale->numerator = 1000000 / factor;
+ rescale->denominator = tmp / factor;
+
+ rescale->offset = -1 * ((r0 * iexc) / 1000);
+
+ return 0;
+}
+
enum rescale_variant {
CURRENT_SENSE_AMPLIFIER,
CURRENT_SENSE_SHUNT,
VOLTAGE_DIVIDER,
+ TEMP_SENSE_RTD,
};
static const struct rescale_cfg rescale_cfg[] = {
@@ -405,6 +447,10 @@ static const struct rescale_cfg rescale_cfg[] = {
.type = IIO_VOLTAGE,
.props = rescale_voltage_divider_props,
},
+ [TEMP_SENSE_RTD] = {
+ .type = IIO_TEMP,
+ .props = rescale_temp_sense_rtd_props,
+ },
};
static const struct of_device_id rescale_match[] = {
@@ -414,6 +460,8 @@ static const struct of_device_id rescale_match[] = {
.data = &rescale_cfg[CURRENT_SENSE_SHUNT], },
{ .compatible = "voltage-divider",
.data = &rescale_cfg[VOLTAGE_DIVIDER], },
+ { .compatible = "temperature-sense-rtd",
+ .data = &rescale_cfg[TEMP_SENSE_RTD], },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, rescale_match);
--
2.32.0.452.g940fe202adcb
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(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 8cdcb6ffb563..12de44058bea 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -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;
+ }
+
+ rescale->numerator = 1000000;
+ rescale->denominator = alpha * sense;
+
+ tmp = (s64)offset * (s64)alpha * (s64)sense;
+ rescale->offset = div_s64(tmp, (s32)1000000);
+
+ 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);
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
An ADC is often used to measure other quantities indirectly.
This binding describe one case, the measurement of a temperature
through a temperature transducer (either voltage or current).
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../iio/afe/temperature-transducer.yaml | 114 ++++++++++++++++++
1 file changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
new file mode 100644
index 000000000000..cfbf5350db27
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/afe/temperature-transducer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Temperature Transducer
+
+maintainers:
+ - Liam Beguin <[email protected]>
+
+description: |
+ A temperature transducer is a device that converts a thermal quantity
+ into any other physical quantity. This binding applies to 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.
+
+ When an io-channel measures the output voltage of a temperature analog front
+ end such as a temperature transducer, the interesting measurement is almost
+ always the corresponding temperature, not the voltage output. This binding
+ describes such a circuit.
+
+ The general transfer function here is (using SI units)
+ V(T) = Rsense * Isense(T)
+ T = (Isense(T) / alpha) + offset
+ T = 1 / (Rsense * alpha) * (V + offset * Rsense * alpha)
+
+ When using a temperature to voltage transducer, Rsense is set to 1.
+
+ The following circuits show a temperature to current and a temperature to
+ voltage transducer that can be used with this binding.
+
+ VCC
+ -----
+ |
+ +---+---+
+ | AD590 | VCC
+ +---+---+ -----
+ | |
+ V proportional to T +----+----+
+ | D+ --+ |
+ +---- Vout | LTC2997 +--- Vout
+ | D- --+ |
+ +---+----+ +---------+
+ | Rsense | |
+ +---+----+ -----
+ | GND
+ -----
+ GND
+
+properties:
+ compatible:
+ const: temperature-transducer
+
+ io-channels:
+ maxItems: 1
+ description: |
+ Channel node of a voltage io-channel.
+
+ '#io-channel-cells':
+ const: 0
+
+ sense-offset-millicelsius:
+ description: |
+ Temperature offset.
+ This offset is commonly used to convert from Kelvins to degrees Celsius.
+ In that case, sense-offset-millicelsius would be set to <(-273150)>.
+ default: 0
+
+ sense-resistor-ohms:
+ description: |
+ The sense resistor.
+ By default sense-resistor-ohms cancels out the resistor making the
+ circuit behave like a temperature transducer.
+ default: 1
+
+ alpha-ppm-per-celsius:
+ description: |
+ Sometimes referred to as output gain, slope, or temperature coefficient.
+
+ alpha is expressed in parts per million which can be micro-amps per
+ degrees Celsius or micro-volts per degrees Celsius. The is the main
+ characteristic of a temperature transducer and should be stated in the
+ datasheet.
+
+additionalProperties: false
+
+required:
+ - compatible
+ - io-channels
+ - alpha-ppm-per-celsius
+
+examples:
+ - |
+ ad950: temperature-sensor-0 {
+ compatible = "temperature-transducer";
+ #io-channel-cells = <0>;
+ io-channels = <&temp_adc 3>;
+
+ sense-offset-millicelsius = <(-273150)>; /* Kelvin to degrees Celsius */
+ sense-resistor-ohms = <8060>;
+ alpha-ppm-per-celsius = <1>; /* 1 uA/K */
+ };
+ - |
+ znq_tmp: temperature-sensor-1 {
+ compatible = "temperature-transducer";
+ #io-channel-cells = <0>;
+ io-channels = <&temp_adc 2>;
+
+ sense-offset-millicelsius = <(-273150)>; /* Kelvin to degrees Celsius */
+ alpha-ppm-per-celsius = <4000>; /* 4 mV/K */
+ };
+...
--
2.32.0.452.g940fe202adcb
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(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 809e966f7058..c408c4057c08 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -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);
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
The iio-rescale driver supports various combinations of scale types and
offsets. These can often result in large integer multiplications. Make
sure these calculations are done right by adding a set of kunit test
cases that build on top of iio-test-format.
To run these tests, add the following to .kunitconfig
$ cat .kunitconfig
CONFIG_IIO=y
CONFIG_IIO_RESCALE_KUNIT_TEST=y
CONFIG_KUNIT=y
Then run:
$ ./tools/testing/kunit/kunit.py run --kunitconfig .kunitconfig
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/test/Kconfig | 10 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-rescale.c | 637 ++++++++++++++++++++++++++++
3 files changed, 648 insertions(+)
create mode 100644 drivers/iio/test/iio-test-rescale.c
diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
index 679a7794af20..872ed4ed140a 100644
--- a/drivers/iio/test/Kconfig
+++ b/drivers/iio/test/Kconfig
@@ -4,6 +4,16 @@
#
# Keep in alphabetical order
+config IIO_RESCALE_KUNIT_TEST
+ bool "Test IIO rescale conversion functions"
+ depends on KUNIT && !IIO_RESCALE
+ default KUNIT_ALL_TESTS
+ help
+ If you want to run tests on the iio-rescale code say Y here.
+
+ This takes advantage of ARCH=um to run tests and should be used by
+ developers to tests their changes to the rescaling logic.
+
config IIO_TEST_FORMAT
bool "Test IIO formatting functions"
depends on KUNIT=y
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile
index f1099b495301..908963ca0b2f 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -4,4 +4,5 @@
#
# Keep in alphabetical order
+obj-$(CONFIG_IIO_RESCALE_KUNIT_TEST) += iio-test-rescale.o ../afe/iio-rescale.o
obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o
diff --git a/drivers/iio/test/iio-test-rescale.c b/drivers/iio/test/iio-test-rescale.c
new file mode 100644
index 000000000000..68ab5a40e397
--- /dev/null
+++ b/drivers/iio/test/iio-test-rescale.c
@@ -0,0 +1,637 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kunit tests for IIO rescale conversions
+ *
+ * Copyright (c) 2021 Liam Beguin <[email protected]>
+ */
+
+#include <kunit/test.h>
+#include <linux/gcd.h>
+#include <linux/iio/afe/rescale.h>
+#include <linux/iio/iio.h>
+#include <linux/overflow.h>
+
+struct rescale_tc_data {
+ const char *name;
+
+ const s32 numerator;
+ const s32 denominator;
+ const s32 offset;
+
+ const int schan_val;
+ const int schan_val2;
+ const int schan_off;
+ const int schan_scale_type;
+
+ const char *expected;
+ const char *expected_off;
+};
+
+const struct rescale_tc_data scale_cases[] = {
+ /*
+ * Typical use cases
+ */
+ {
+ .name = "typical IIO_VAL_INT, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 42,
+ .expected = "5210.918114143",
+ },
+ {
+ .name = "typical IIO_VAL_INT, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 42,
+ .expected = "-5210.918114143",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 42,
+ .schan_val2 = 20,
+ .expected = "260.545905707",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 42,
+ .schan_val2 = 20,
+ .expected = "-260.545905707",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, positive",
+ .numerator = 42,
+ .denominator = 53,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
+ .expected = "0.049528301",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, negative",
+ .numerator = -42,
+ .denominator = 53,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
+ .expected = "-0.049528301",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456,
+ .expected = "1240.710106203",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 1234,
+ .expected = "1240.84789",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 1234,
+ .expected = "-1240.84789",
+ },
+ /*
+ * Use cases with small scales involving divisions
+ */
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 1",
+ .numerator = 6,
+ .denominator = 6,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, -1 < scale < 0",
+ .numerator = -6,
+ .denominator = 6,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "-0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 2",
+ .numerator = 8,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, -2 < scale < 0",
+ .numerator = -8,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "-1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 1",
+ .numerator = 16,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 4,
+ .expected = "0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, -1 < scale < 0",
+ .numerator = -16,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 4,
+ .expected = "-0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 2",
+ .numerator = 8,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 1,
+ .expected = "1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, -2 < scale < 0",
+ .numerator = -8,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 1,
+ .expected = "-1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 1,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 5,
+ .schan_val2 = 1234,
+ .expected = "2.500617",
+ },
+ {
+ .name = "small IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -1,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 5,
+ .schan_val2 = 1234,
+ .expected = "-2.500617",
+ },
+ /*
+ * INT_PLUS_{MICRO,NANO} positive/negative corner cases
+ */
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, both negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
+ .expected = "1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative",
+ .numerator = -1000000,
+ .denominator = -8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, 4 negative",
+ .numerator = -1000000,
+ .denominator = -8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = -123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, negative, *val = 0",
+ .numerator = 1,
+ .denominator = -10,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 0,
+ .schan_val2 = 123456789,
+ .expected = "-0.012345678",
+ },
+ /*
+ * INT_PLUS_{MICRO,NANO} decimal part overflow
+ */
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "-1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456789,
+ .expected = "-1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "16557.914267",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "-16557.914267",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = -10,
+ .schan_val2 = 123456789,
+ .expected = "-16557.914267",
+ },
+ /*
+ * 32-bit overflow conditions
+ */
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL, positive",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = S32_MAX,
+ .schan_val2 = 1,
+ .expected = "214748364.7",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = S32_MAX,
+ .schan_val2 = 1,
+ .expected = "-214748364.7",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL_LOG2, positive",
+ .numerator = S32_MAX,
+ .denominator = 4096,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
+ .expected = "32767.99998474121",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL_LOG2, negative",
+ .numerator = S32_MAX,
+ .denominator = 4096,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = -4096,
+ .schan_val2 = 16,
+ .expected = "-32767.99998474121",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, positive",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "-1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = S32_MAX,
+ .expected = "-1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "215.748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "-215.748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, negative schan",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = -10,
+ .schan_val2 = S32_MAX,
+ .expected = "-215.748364",
+ },
+};
+
+const struct rescale_tc_data offset_cases[] = {
+ /*
+ * Typical use cases
+ */
+ {
+ .name = "typical IIO_VAL_INT, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 123,
+ .schan_val2 = 0,
+ .schan_off = 14,
+ .expected_off = "24", /* 23.872 */
+ },
+ {
+ .name = "typical IIO_VAL_INT, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 12,
+ .schan_val2 = 0,
+ .schan_off = 14,
+ .expected_off = "-88", /* -88.83333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 12,
+ .schan_val2 = 34,
+ .schan_off = 14,
+ .expected_off = "3510", /* 3510.333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 12,
+ .schan_val2 = 34,
+ .schan_off = 14,
+ .expected_off = "-3482", /* -3482.333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 12,
+ .schan_val2 = 16,
+ .schan_off = 14,
+ .expected_off = "6739299", /* 6739299.333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 12,
+ .schan_val2 = 16,
+ .schan_off = 14,
+ .expected_off = "-6739271", /* -6739271.333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "135", /* 135.8951219647469 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "-107", /* -107.89512196474689 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "23", /* 23.246438560723952 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, negative",
+ .offset = -12345,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "-78", /* -78.50185091745313 */
+ },
+};
+
+static void case_to_desc(const struct rescale_tc_data *t, char *desc)
+{
+ strcpy(desc, t->name);
+}
+
+KUNIT_ARRAY_PARAM(iio_rescale_scale, scale_cases, case_to_desc);
+KUNIT_ARRAY_PARAM(iio_rescale_offset, offset_cases, case_to_desc);
+
+/**
+ * iio_str_to_micro() - Parse a fixed-point string to get an
+ * IIO_VAL_INT_PLUS_MICRO value
+ * @str: The string to parse
+ * @micro: The number as an integer
+ *
+ * Returns 0 on success, or a negative error code if the string cound not be
+ * parsed.
+ */
+static int iio_str_to_micro(const char *str, s64 *micro)
+{
+ int fract_mult = 100000LL;
+ int tmp, tmp2;
+ int ret = 0;
+
+ ret = iio_str_to_fixpoint(str, fract_mult, &tmp, &tmp2);
+ if (ret < 0)
+ return ret;
+
+ if (tmp < 0)
+ tmp2 *= -1;
+
+ *micro = (s64)tmp * 10 * fract_mult + tmp2;
+
+ return ret;
+}
+
+/**
+ * iio_test_relative_error_ppm() - Compute relative error (in ppm) between two
+ * fixed-point strings
+ * @real_str: The real value as a string
+ * @exp_str: The expected value as a string
+ *
+ * Returns a negative error code if the strings cound not be parsed, or the
+ * relative error in ppm.
+ */
+static int iio_test_relative_error_ppm(const char *real_str, const char *exp_str)
+{
+ s64 real, exp, err;
+ int ret;
+
+ ret = iio_str_to_micro(real_str, &real);
+ if (ret < 0)
+ return ret;
+
+ ret = iio_str_to_micro(exp_str, &exp);
+ if (ret < 0)
+ return ret;
+
+ err = 1000000 * abs(exp - real);
+ err = div64_u64(err, abs(exp));
+ return (int)err;
+}
+
+static void iio_rescale_test_scale(struct kunit *test)
+{
+ struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
+ char *buff = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+ struct rescale rescale;
+ int values[2];
+ int rel_ppm;
+ int ret;
+
+ rescale.numerator = t->numerator;
+ rescale.denominator = t->denominator;
+ rescale.offset = t->offset;
+ values[0] = t->schan_val;
+ values[1] = t->schan_val2;
+
+ ret = rescale_process_scale(&rescale, t->schan_scale_type,
+ &values[0], &values[1]);
+
+ ret = iio_format_value(buff, ret, 2, values);
+ KUNIT_EXPECT_EQ(test, (int)strlen(buff), ret);
+
+ rel_ppm = iio_test_relative_error_ppm(buff, t->expected);
+ KUNIT_EXPECT_GE_MSG(test, rel_ppm, 0, "failed to compute ppm\n");
+
+ KUNIT_EXPECT_LT_MSG(test, rel_ppm, 500,
+ "\t real=%s"
+ "\texpected=%s\n",
+ buff, t->expected);
+}
+
+static void iio_rescale_test_offset(struct kunit *test)
+{
+ struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
+ char *buff_off = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+ struct rescale rescale;
+ int values[2];
+ int ret;
+
+ rescale.numerator = t->numerator;
+ rescale.denominator = t->denominator;
+ rescale.offset = t->offset;
+ values[0] = t->schan_val;
+ values[1] = t->schan_val2;
+
+ ret = rescale_process_offset(&rescale, t->schan_scale_type,
+ t->schan_val, t->schan_val2, t->schan_off,
+ &values[0], &values[1]);
+
+ ret = iio_format_value(buff_off, ret, 2, values);
+ KUNIT_EXPECT_EQ(test, (int)strlen(buff_off), ret);
+
+ KUNIT_EXPECT_STREQ(test, strim(buff_off), t->expected_off);
+}
+
+static struct kunit_case iio_rescale_test_cases[] = {
+ KUNIT_CASE_PARAM(iio_rescale_test_scale, iio_rescale_scale_gen_params),
+ KUNIT_CASE_PARAM(iio_rescale_test_offset, iio_rescale_offset_gen_params),
+ {}
+};
+
+static struct kunit_suite iio_rescale_test_suite = {
+ .name = "iio-rescale",
+ .test_cases = iio_rescale_test_cases,
+};
+kunit_test_suite(iio_rescale_test_suite);
--
2.32.0.452.g940fe202adcb
From: Liam Beguin <[email protected]>
An ADC is often used to measure other quantities indirectly. This
binding describe one case, the measurement of a temperature through the
voltage across an RTD resistor such as a PT1000.
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../iio/afe/temperature-sense-rtd.yaml | 101 ++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
new file mode 100644
index 000000000000..336ce96371db
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/afe/temperature-sense-rtd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Temperature Sense RTD
+
+maintainers:
+ - Liam Beguin <[email protected]>
+
+description: |
+ RTDs (Resistance Temperature Detectors) are a kind of temperature sensors
+ used to get a linear voltage to temperature reading within a give range
+ (usually 0 to 100 degrees Celsius).
+
+ When an io-channel measures the output voltage across an RTD such as a
+ PT1000, the interesting measurement is almost always the corresponding
+ temperature, not the voltage output. This binding describes such a circuit.
+
+ The general transfer function here is (using SI units)
+
+ V = R(T) * iexc
+ R(T) = r0 * (1 + alpha * T)
+ T = 1 / (alpha * r0 * iexc) * (V - r0 * iexc)
+
+ The following circuit matches what's in the examples section.
+
+ 5V0
+ -----
+ |
+ +---+----+
+ | R 5k |
+ +---+----+
+ |
+ V 1mA
+ |
+ +---- Vout
+ |
+ +---+----+
+ | PT1000 |
+ +---+----+
+ |
+ -----
+ GND
+
+properties:
+ compatible:
+ const: temperature-sense-rtd
+
+ io-channels:
+ maxItems: 1
+ description: |
+ Channel node of a voltage io-channel.
+
+ '#io-channel-cells':
+ const: 0
+
+ excitation-current-microamp:
+ description: The current fed through the RTD sensor.
+
+ alpha-ppm-per-celsius:
+ description: |
+ alpha can also be expressed in micro-ohms per ohm Celsius. It's a linear
+ approximation of the resistance versus temperature relationship
+ between 0 and 100 degrees Celsius.
+
+ alpha = (R_100 - R_0) / (100 * R_0)
+
+ Where, R_100 is the resistance of the sensor at 100 degrees Celsius, and
+ R_0 (or r-naught-ohms) is the resistance of the sensor at 0 degrees
+ Celsius.
+
+ Pure platinum has an alpha of 3925. Industry standards such as IEC60751
+ and ASTM E-1137 specify an alpha of 3850.
+
+ r-naught-ohms:
+ description: |
+ Resistance of the sensor at 0 degrees Celsius.
+ Common values are 100 for PT100, 500 for PT500, and 1000 for PT1000
+
+additionalProperties: false
+required:
+ - compatible
+ - io-channels
+ - excitation-current-microamp
+ - alpha-ppm-per-celsius
+ - r-naught-ohms
+
+examples:
+ - |
+ pt1000_1: temperature-sensor0 {
+ compatible = "temperature-sense-rtd";
+ #io-channel-cells = <0>;
+ io-channels = <&temp_adc1 0>;
+
+ excitation-current-microamp = <1000>; /* i = U/R = 5 / 5000 */
+ alpha-ppm-per-celsius = <3908>;
+ r-naught-ohms = <1000>;
+ };
+...
--
2.32.0.452.g940fe202adcb
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':
>> 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/[email protected]
On Fri Aug 20, 2021 at 7:37 PM EDT, kernel test robot wrote:
> 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':
> >> 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().
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -53,7 +53,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
else
tmp = 1 << *val2;
- if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
+ if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
*val = div_s64_rem(*val, tmp, &rem2);
*val2 = div_s64(rem, tmp);
The if statement is also misaligned here. I'll fix that too.
Thanks,
Liam
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
Hi Liam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78]
url: https://github.com/0day-ci/linux/commits/Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210821-032112
base: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
config: openrisc-randconfig-m031-20210821 (attached as .config)
compiler: or1k-linux-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
smatch warnings:
drivers/iio/afe/iio-rescale.c:56 rescale_process_scale() warn: inconsistent indenting
vim +56 drivers/iio/afe/iio-rescale.c
20
21 int rescale_process_scale(struct rescale *rescale, int scale_type,
22 int *val, int *val2)
23 {
24 s64 tmp;
25 s32 rem, rem2;
26 u32 mult;
27 u32 neg;
28
29 switch (scale_type) {
30 case IIO_VAL_INT:
31 *val *= rescale->numerator;
32 if (rescale->denominator == 1)
33 return scale_type;
34 *val2 = rescale->denominator;
35 return IIO_VAL_FRACTIONAL;
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
> 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;
67 case IIO_VAL_INT_PLUS_NANO:
68 case IIO_VAL_INT_PLUS_MICRO:
69 if (scale_type == IIO_VAL_INT_PLUS_NANO)
70 mult = 1000000000LL;
71 else
72 mult = 1000000LL;
73 /*
74 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
75 * *val2 is negative the schan scale is negative
76 */
77 neg = *val < 0 || *val2 < 0;
78
79 tmp = (s64)abs(*val) * abs(rescale->numerator);
80 *val = div_s64_rem(tmp, abs(rescale->denominator), &rem);
81
82 tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator);
83 tmp = div_s64(tmp, abs(rescale->denominator));
84
85 *val += div_s64_rem(tmp, mult, val2);
86
87 /*
88 * If only one of the rescaler elements or the schan scale is
89 * negative, the combined scale is negative.
90 */
91 if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
92 if (*val)
93 *val = -*val;
94 else
95 *val2 = -*val2;
96 }
97
98 return scale_type;
99 default:
100 return -EOPNOTSUPP;
101 }
102 }
103
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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: i386-randconfig-r026-20210821 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# 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
make W=1 O=build_dir ARCH=i386 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 >>):
ld: drivers/iio/afe/iio-rescale.o: in function `rescale_process_scale':
>> drivers/iio/afe/iio-rescale.c:56: undefined reference to `__divdi3'
vim +56 drivers/iio/afe/iio-rescale.c
20
21 int rescale_process_scale(struct rescale *rescale, int scale_type,
22 int *val, int *val2)
23 {
24 s64 tmp;
25 s32 rem, rem2;
26 u32 mult;
27 u32 neg;
28
29 switch (scale_type) {
30 case IIO_VAL_INT:
31 *val *= rescale->numerator;
32 if (rescale->denominator == 1)
33 return scale_type;
34 *val2 = rescale->denominator;
35 return IIO_VAL_FRACTIONAL;
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
> 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;
67 case IIO_VAL_INT_PLUS_NANO:
68 case IIO_VAL_INT_PLUS_MICRO:
69 if (scale_type == IIO_VAL_INT_PLUS_NANO)
70 mult = 1000000000LL;
71 else
72 mult = 1000000LL;
73 /*
74 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
75 * *val2 is negative the schan scale is negative
76 */
77 neg = *val < 0 || *val2 < 0;
78
79 tmp = (s64)abs(*val) * abs(rescale->numerator);
80 *val = div_s64_rem(tmp, abs(rescale->denominator), &rem);
81
82 tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator);
83 tmp = div_s64(tmp, abs(rescale->denominator));
84
85 *val += div_s64_rem(tmp, mult, val2);
86
87 /*
88 * If only one of the rescaler elements or the schan scale is
89 * negative, the combined scale is negative.
90 */
91 if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
92 if (*val)
93 *val = -*val;
94 else
95 *val2 = -*val2;
96 }
97
98 return scale_type;
99 default:
100 return -EOPNOTSUPP;
101 }
102 }
103
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
[I started to write an answer to your plans in the v7 thread, but didn't
have time to finish before v8 appeared...]
On 2021-08-20 21:17, Liam Beguin wrote:
> 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...
> Signed-off-by: Liam Beguin <[email protected]>
> ---
> drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index c408c4057c08..7304306c9806 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -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);
> +
> + *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
> +
> + return IIO_VAL_INT_PLUS_NANO;
> + }
> +
> return scale_type;
> case IIO_VAL_INT_PLUS_NANO:
> case IIO_VAL_INT_PLUS_MICRO:
>
On 2021-08-21 03:33, Liam Beguin wrote:
> On Fri Aug 20, 2021 at 7:37 PM EDT, kernel test robot wrote:
>> 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':
>>>> 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().
>
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -53,7 +53,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> else
> tmp = 1 << *val2;
>
> - if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
> + if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
> *val = div_s64_rem(*val, tmp, &rem2);
>
> *val2 = div_s64(rem, tmp);
>
At this point in the code, you know that tmp is a 32-bit value. *val
and rem are also 32-bit. It feels like a waste to not exploit that fact
and spend cycles doing lots of pointless 64-bit math on weakish 32-bit
machines.
Cheers,
Peter
>
> The if statement is also misaligned here. I'll fix that too.
>
> Thanks,
> Liam
>
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/[email protected]
>
On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
> [I started to write an answer to your plans in the v7 thread, but didn't
> have time to finish before v8 appeared...]
>
> On 2021-08-20 21:17, Liam Beguin wrote:
> > 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.
> 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 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.
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?
> 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.
>
> 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.
>
> 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?
>
> 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.
>
> 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?
>
> > Signed-off-by: Liam Beguin <[email protected]>
> > ---
> > drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index c408c4057c08..7304306c9806 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -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);
> > +
> > + *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
>
> Cheers,
> Peter
>
> > +
> > + return IIO_VAL_INT_PLUS_NANO;
> > + }
> > +
> > return scale_type;
> > case IIO_VAL_INT_PLUS_NANO:
> > case IIO_VAL_INT_PLUS_MICRO:
> >
>
On 2021-08-20 21:17, Liam Beguin wrote:
> 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(+)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index d0669fd8eac5..8488f1d83527 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -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;
> + 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
> + */
> + 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;
>
On 2021-08-20 21:17, Liam Beguin wrote:
> 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(+)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 8cdcb6ffb563..12de44058bea 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -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;
> + }
> +
> + 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
> +
> + 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);
>
On 2021-08-20 21:17, Liam Beguin wrote:
> 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
> Signed-off-by: Liam Beguin <[email protected]>
> ---
> drivers/iio/afe/iio-rescale.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 809e966f7058..c408c4057c08 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -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);
>
On 2021-08-24 22:28, Liam Beguin wrote:
> On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
>> [I started to write an answer to your plans in the v7 thread, but didn't
>> have time to finish before v8 appeared...]
>>
>> On 2021-08-20 21:17, Liam Beguin wrote:
>>> 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.
>
>> 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 see what you mean here.
> I added test cases with these values to see exactly what we get.
Excellent!
>
> 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.
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
> 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?
> 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...
> 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?
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?
>> 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.
>>
>> 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.
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.
>>
>> 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.
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]
> 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.
>>
>> 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.
>
>>
>> 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?
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
>>
>>> Signed-off-by: Liam Beguin <[email protected]>
>>> ---
>>> drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
>>> 1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index c408c4057c08..7304306c9806 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -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);
>>> +
>>> + *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
>
>>
>> Cheers,
>> Peter
>>
>>> +
>>> + return IIO_VAL_INT_PLUS_NANO;
>>> + }
>>> +
>>> return scale_type;
>>> case IIO_VAL_INT_PLUS_NANO:
>>> case IIO_VAL_INT_PLUS_MICRO:
>>>
>>
On Thu, Aug 26, 2021 at 10:56:11AM +0200, Peter Rosin wrote:
> On 2021-08-20 21:17, Liam Beguin wrote:
> > 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(+)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 8cdcb6ffb563..12de44058bea 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -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;
> > + }
> > +
> > + 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 :-)
Oof! You're right, that doesn't look too good...
I guess I haven't looked at this part of the series for a few
revisions, my bad.
>
> 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.
>
Yeah, I agree! The way I had it spelled out wasn't much more readable.
I'll use your suggestion instead.
Thanks,
Liam
> Cheers,
> Peter
>
> > +
> > + 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);
> >
On Thu, Aug 26, 2021 at 10:11:18AM +0200, Peter Rosin wrote:
> On 2021-08-20 21:17, Liam Beguin wrote:
> > 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(+)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index d0669fd8eac5..8488f1d83527 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -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;
> > + 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?
I meant to warn that IIO_VAL_INT_PLUS_{MICRO,NANO} are a little odd, and
that if either one *val or *val2 is negative, the result will be
negative.
i.e. *val = 1 and *val2 = -0.5 gives -1.5 and not 0.5.
I'll give it another try and will add the period.
Thanks,
Liam
>
> Cheers,
> Peter
>
> > + */
> > + 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;
> >
On Thu, Aug 26, 2021 at 11:13:38AM +0200, Peter Rosin wrote:
> On 2021-08-20 21:17, Liam Beguin wrote:
> > 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.
The 100 ppm check introduced in 09/14 is really objective and might not
be the best choice. Changing it to
- if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
+ if (abs(rem)) {
Helps with the precision issues you brought up here, and in 09/14.
I was originally trying to keep the original scale as much as possible,
I'll continue the rest of the discussion on the 09/14 thread we already
have.
>
> 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?
>
Being totally honest, I noticed we have the same issue with IIO_VAL_INT,
but since I didn't run into the issue on my setup I left it out to focus
on getting the rest cleaned up.
I guess it couldn't hurt to fix that too while we're at it.
I'll work on it!
> 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.
I don't really like having to add a range parameter.
If changing the scale type dynamically isn't an issue, I think we can
get away with not adding a parameter.
If it is an issue, we might have to look into it...
Thanks,
Liam
>
> Cheers,
> Peter
>
> > Signed-off-by: Liam Beguin <[email protected]>
> > ---
> > drivers/iio/afe/iio-rescale.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 809e966f7058..c408c4057c08 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -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);
> >
>
On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:
> On 2021-08-24 22:28, Liam Beguin wrote:
> > On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
> >> [I started to write an answer to your plans in the v7 thread, but didn't
> >> have time to finish before v8 appeared...]
> >>
> >> On 2021-08-20 21:17, Liam Beguin wrote:
> >>> 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.
> >
> >> 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 see what you mean here.
> > I added test cases with these values to see exactly what we get.
>
> Excellent!
>
> >
> > 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.
>
> 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
> > 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?
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).
>
> > 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...
>
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.
> > 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?
>
> 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 :-)
> >> 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.
>
> >>
> >> 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.
>
> 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.
>
> >>
> >> 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.
>
> 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]
>
> > 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?
>
> >>
> >> 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.
> >
> >>
> >> 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?
>
> 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
>
> Cheers,
> Peter
>
> >>
> >>> Signed-off-by: Liam Beguin <[email protected]>
> >>> ---
> >>> drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
> >>> 1 file changed, 25 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>> index c408c4057c08..7304306c9806 100644
> >>> --- a/drivers/iio/afe/iio-rescale.c
> >>> +++ b/drivers/iio/afe/iio-rescale.c
> >>> @@ -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);
> >>> +
> >>> + *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
> >
> >>
> >> Cheers,
> >> Peter
> >>
> >>> +
> >>> + return IIO_VAL_INT_PLUS_NANO;
> >>> + }
> >>> +
> >>> return scale_type;
> >>> case IIO_VAL_INT_PLUS_NANO:
> >>> case IIO_VAL_INT_PLUS_MICRO:
> >>>
> >>
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...]
>
> On 2021-08-20 21:17, Liam Beguin wrote:
> > 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?
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.
Jonathan
>
> 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...
>
> > Signed-off-by: Liam Beguin <[email protected]>
> > ---
> > drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index c408c4057c08..7304306c9806 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -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);
> > +
> > + *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
>
> > +
> > + return IIO_VAL_INT_PLUS_NANO;
> > + }
> > +
> > return scale_type;
> > case IIO_VAL_INT_PLUS_NANO:
> > case IIO_VAL_INT_PLUS_MICRO:
> >
On Sun, 29 Aug 2021 00:41:21 -0400
Liam Beguin <[email protected]> wrote:
> On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:
> > On 2021-08-24 22:28, Liam Beguin wrote:
> > > On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
> > >> [I started to write an answer to your plans in the v7 thread, but didn't
> > >> have time to finish before v8 appeared...]
> > >>
> > >> On 2021-08-20 21:17, Liam Beguin wrote:
> > >>> 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.
> > >
> > >> 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 see what you mean here.
> > > I added test cases with these values to see exactly what we get.
> >
> > Excellent!
> >
> > >
> > > 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.
> >
> > 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
> > > 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?
>
> 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).
>
> >
> > > 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...
> >
>
> 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!
>
> > > 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?
> >
> > 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 :)
>
> > >> 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.
Spot on. Meta data is a pain so an early design decision in IIO was to
not support it in band.
> >
> > >>
> > >> 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.
> >
> > 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.
> >
> > >>
> > >> 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.
> >
> > 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]
> >
> > > 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?
>
> >
> > >>
> > >> 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.
> > >
> > >>
> > >> 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?
> >
> > 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.
Keep up the good work! I'm looking forward to this going in (hopefully
shortly!)
Jonathan
>
> Thanks again for your time,
> Liam
>
> >
> > Cheers,
> > Peter
> >
> > >>
> > >>> Signed-off-by: Liam Beguin <[email protected]>
> > >>> ---
> > >>> drivers/iio/afe/iio-rescale.c | 27 +++++++++++++++++++++++++--
> > >>> 1 file changed, 25 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > >>> index c408c4057c08..7304306c9806 100644
> > >>> --- a/drivers/iio/afe/iio-rescale.c
> > >>> +++ b/drivers/iio/afe/iio-rescale.c
> > >>> @@ -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);
> > >>> +
> > >>> + *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
> > >
> > >>
> > >> Cheers,
> > >> Peter
> > >>
> > >>> +
> > >>> + return IIO_VAL_INT_PLUS_NANO;
> > >>> + }
> > >>> +
> > >>> return scale_type;
> > >>> case IIO_VAL_INT_PLUS_NANO:
> > >>> case IIO_VAL_INT_PLUS_MICRO:
> > >>>
> > >>
On 2021-08-29 06:41, Liam Beguin wrote:
> On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:
>> On 2021-08-24 22:28, Liam Beguin wrote:
>>> On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
>>>> [I started to write an answer to your plans in the v7 thread, but didn't
>>>> have time to finish before v8 appeared...]
>>>>
>>>> On 2021-08-20 21:17, Liam Beguin wrote:
>>>>> 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.
>>>
>>>> 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 see what you mean here.
>>> I added test cases with these values to see exactly what we get.
>>
>> Excellent!
>>
>>>
>>> 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.
>>
>> 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
>> > 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?
>
> So... It turns out, I swapped schan and rescaler values which gives us:
Ahh, good. The explanation is simple!
>
> 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).
No, it doesn't fix the precision problems. Not really, it only reduces
them. See below.
*snip*
>>> 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
On 2021-08-30 13:22, Jonathan Cameron wrote:
> 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...]
>>
>> On 2021-08-20 21:17, Liam Beguin wrote:
>>> 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?
>
> 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.
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
On Mon, 30 Aug 2021 16:30:54 +0200
Peter Rosin <[email protected]> wrote:
> On 2021-08-30 13:22, Jonathan Cameron wrote:
> > 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...]
> >>
> >> On 2021-08-20 21:17, Liam Beguin wrote:
> >>> 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?
> >
> > 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.
>
> 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).
> */
Looks good to me.
There are some really obscure corner cases in which it is theoretically possible
to have scale change autonomously. Reality is they mostly don't happen in reality
and are on strange sensors you couldn't stick an AFE on anyway ;)
IIRC hid-sensors in theory allow this, but as far as I know, no device actually does
it... I'm not sure the driver takes any notice if they do!
Jonathan
>
> 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
On Mon, Aug 30, 2021 at 04:30:54PM +0200, Peter Rosin wrote:
> On 2021-08-30 13:22, Jonathan Cameron wrote:
> > 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...]
> >>
> >> On 2021-08-20 21:17, Liam Beguin wrote:
> >>> 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?
> >
> > 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.
>
> 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).
No worries, I was also under the impression that the source channel
scale (and offset) could change.
>
> 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).
> */
>
If this is more of a general assumption, is there a place in the
Documentation/ that we could put this comment?
If not, I'll add it here.
> 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.
I agree with you, I'd much rather drop gcd() from rescale_process_scale()
altogether.
I was planning on keeping IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2
combined, but getting rid of the 100ppm condition in favor of a simple
if (rem).
>
> 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.
>
> 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.
>
My original issue was with IIO_VAL_FRACTIONAL.
I'll look into what we can do for small IIO_VAL_FRACTIONAL_LOG2 values,
and will add more tests for that too.
Thanks,
Liam
> Cheers,
> Peter
On 2021-09-02 04:27, Liam Beguin wrote:
> On Mon, Aug 30, 2021 at 04:30:54PM +0200, Peter Rosin wrote:
>>
>> 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.
My mistake, you are right. A sufficiently large denominator can of course
be used to dodge overflow in the numerator by sacrificing some accuracy
even if our maximum scale is still limited to 32 bits.
I apparently didn't have my brains about when I wrote the above...
Cheers,
Peter
On Mon, Aug 30, 2021 at 03:03:52PM +0200, Peter Rosin wrote:
> On 2021-08-29 06:41, Liam Beguin wrote:
> > On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:
> >> On 2021-08-24 22:28, Liam Beguin wrote:
> >>> On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
> >>>> [I started to write an answer to your plans in the v7 thread, but didn't
> >>>> have time to finish before v8 appeared...]
> >>>>
> >>>> On 2021-08-20 21:17, Liam Beguin wrote:
> >>>>> 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.
> >>>
> >>>> 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 see what you mean here.
> >>> I added test cases with these values to see exactly what we get.
> >>
> >> Excellent!
> >>
> >>>
> >>> 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.
> >>
> >> 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
> >> > 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?
> >
> > So... It turns out, I swapped schan and rescaler values which gives us:
>
> Ahh, good. The explanation is simple!
>
> >
> > 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).
>
> No, it doesn't fix the precision problems. Not really, it only reduces
> them. See below.
>
> *snip*
>
> >>> 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?
Hi Peter,
Apologies for the late reply.
> 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.
I think I see where our misunderstanding comes from :-)
I understand that mathematically, IIO_VAL_FRACTIONAL is more accurate
than IIO_VAL_INT_PLUS_NANO for rational numbers given that it provides
an exact value to the IIO consumer.
My point is that the IIO core will represent IIO_VAL_FRACTIONAL scales
as fixed point when using iio_format_value().
Also, my current setup, uses drivers/hwmon/iio_hwmon.c which is worst
since it relies on iio_read_channel_processed() to get an integer scale.
(I wonder if it would make sense at some point to update iio_hwmon.c to
use iio_format_value() instead).
> 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).
I understand your point, but a device that has an IIO_VAL_FRACTIONAL
scale with *val=15 and *val2=327680000 will also show a scale of
0.000000045 in the sysfs interface.
Since other parts of the core already behave like this, I'm inclined to
say that this is a more general "issue", and that this kind of precision
loss would only affect consumers making direct use of the scaling
values. With all this, I wonder how careful we really have to be with
these extra digits.
> 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.
Right, but like I said above, depending on which path you take, that
value might not be exact in the end.
Thanks,
Liam
>
> Cheers,
> Peter
On Mon, Aug 30, 2021 at 12:27:24PM +0100, Jonathan Cameron wrote:
> On Sun, 29 Aug 2021 00:41:21 -0400
> Liam Beguin <[email protected]> wrote:
>
> > On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:
> > > On 2021-08-24 22:28, Liam Beguin wrote:
> > > > On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
> > > >> [I started to write an answer to your plans in the v7 thread, but didn't
> > > >> have time to finish before v8 appeared...]
> > > >>
> > > >> On 2021-08-20 21:17, Liam Beguin wrote:
> > > >>> 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.
> > > >
> > > >> 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 see what you mean here.
> > > > I added test cases with these values to see exactly what we get.
> > >
> > > Excellent!
> > >
> > > >
> > > > 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.
> > >
> > > 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
> > > > 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?
> >
> > 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).
> >
> > >
> > > > 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...
> > >
> >
> > 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.
Hi Jonathan,
> 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!
Good point, I'll see if I can add a check that could help for this.
> > > > 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?
> > >
> > > 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 :)
>
> >
> > > >> 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.
>
> Spot on. Meta data is a pain so an early design decision in IIO was to
> not support it in band.
>
> > >
> > > >>
> > > >> 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.
> > >
> > > 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.
> > >
> > > >>
> > > >> 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.
> > >
> > > 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]
> > >
> > > > 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?
> >
> > >
> > > >>
> > > >> 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.
> > > >
> > > >>
> > > >> 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?
> > >
> > > 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.
>
> Keep up the good work! I'm looking forward to this going in (hopefully
> shortly!)
Thanks again for the encouragement, it's been really nice working with
all of you!
Liam
> Jonathan
>
> >
> > Thanks again for your time,
> > Liam
> >
> > >
> > > Cheers,
> > > Peter
*snip*