| Message ID | 20211115034334.1713050-1-liambeguin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
On Sun, 14 Nov 2021 22:43:20 -0500 Liam Beguin <liambeguin@gmail.com> wrote: > Hi Jonathan, Peter, > > Apologies for not getting back to you sooner. I got caught up on other > work and wasn't able to dedicate time to this earlier. Hopefully, this > time around, I'll be able to get this to the finish line :-) > > I left out IIO_VAL_INT overflows for now, so that I can focus on getting > the rest of these changes pulled in, but I don't mind adding a patch for > that later on. > > This series focuses on adding temperature rescaling support to the IIO > Analog Front End (AFE) driver. > > The first few patches address minor bugs in IIO inkernel functions, and > prepare the AFE driver for the additional features. > > The main changes to the AFE driver include an initial Kunit test suite, > support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs > and temperature transducer sensors. > > Thanks for your time, > Liam Hi Liam, I'm fine with these. The comment about using the MICRO etc defines can be handled as as trivial follow up patch. Hopefully someone else can figure out the 0-day build issue as I didn't managed to. However, I've long ago lost track of the various precision discussions you and Peter were having so would like Peter's input before taking these. Thanks again for your persistence with this, Jonathan > > Changes since v8: > - reword comment > - fix erroneous 64-bit division > - optimize and use 32-bit divisions when values are know to not overflow > - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed > point > - add test cases > - use nano precision in test cases > - simplify offset calculation in rtd_props() > > 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 > > Liam Beguin (14): > 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 | 271 ++++++- > drivers/iio/inkern.c | 40 +- > drivers/iio/test/Kconfig | 10 + > drivers/iio/test/Makefile | 1 + > drivers/iio/test/iio-test-rescale.c | 705 ++++++++++++++++++ > include/linux/iio/afe/rescale.h | 34 + > 8 files changed, 1232 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 v8: > 1: 42a7a1047edc = 1: ae3cc93baee6 iio: inkern: apply consumer scale on IIO_VAL_INT cases > 2: a1cd89fdad11 = 2: 06f66e7f7403 iio: inkern: apply consumer scale when no channel scale is available > 3: ed0721fb6bd1 = 3: 2dbf6b3bbaeb iio: inkern: make a best effort on offset calculation > 4: f8fb78bb1112 = 4: b083cf307268 iio: afe: rescale: expose scale processing function > 5: 504b7a3f830b ! 5: a0bde29ecc8c iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support > @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale > + else > + mult = 1000000LL; > + /* > -+ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR > -+ * *val2 is negative the schan scale is negative > ++ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if either *val > ++ * OR *val2 is negative the schan scale is negative, i.e. > ++ * *val = 1 and *val2 = -0.5 yields -1.5 not -0.5. > + */ > + neg = *val < 0 || *val2 < 0; > + > 6: c254e9ae813e = 6: c3d0e6248033 iio: afe: rescale: add offset support > 7: ee8814d6abe4 = 7: 2a81fa735103 iio: afe: rescale: use s64 for temporary scale calculations > 8: 62cdcfbc9836 = 8: 8315548d0fce iio: afe: rescale: reduce risk of integer overflow > 9: 88309a5136ee ! 9: 223ed0569cd2 iio: afe: rescale: fix accuracy for small fractional scales > @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale > + > + tmp = div_s64_rem(tmp, 1000000000LL, &rem); > *val = tmp; > +- return scale_type; > ++ > ++ if (!rem) > ++ return scale_type; > + > -+ /* > -+ * 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 = *val % (int)tmp; > ++ *val = *val / (int)tmp; > + > -+ return IIO_VAL_INT_PLUS_NANO; > -+ } > ++ *val2 = rem / (int)tmp; > ++ if (rem2) > ++ *val2 += div_s64((s64)rem2 * 1000000000LL, tmp); > + > - return scale_type; > ++ return IIO_VAL_INT_PLUS_NANO; > case IIO_VAL_INT_PLUS_NANO: > case IIO_VAL_INT_PLUS_MICRO: > + if (scale_type == IIO_VAL_INT_PLUS_NANO) > 10: fb505a9f42f1 ! 10: 90044efdf8be iio: test: add basic tests for the iio-rescale driver > @@ drivers/iio/test/Makefile > # 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 > + CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN) > > ## drivers/iio/test/iio-test-rescale.c (new) ## > @@ > @@ drivers/iio/test/iio-test-rescale.c (new) > + * Use cases with small scales involving divisions > + */ > + { > ++ .name = "small IIO_VAL_FRACTIONAL, 261/509 scaled by 90/1373754273", > ++ .numerator = 261, > ++ .denominator = 509, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > ++ .schan_val = 90, > ++ .schan_val2 = 1373754273, > ++ .expected = "0.000000033594", > ++ }, > ++ { > ++ .name = "small IIO_VAL_FRACTIONAL, 90/1373754273 scaled by 261/509", > ++ .numerator = 90, > ++ .denominator = 1373754273, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > ++ .schan_val = 261, > ++ .schan_val2 = 509, > ++ .expected = "0.000000033594", > ++ }, > ++ { > ++ .name = "small IIO_VAL_FRACTIONAL, 760/1373754273 scaled by 427/2727", > ++ .numerator = 760, > ++ .denominator = 1373754273, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > ++ .schan_val = 427, > ++ .schan_val2 = 2727, > ++ .expected = "0.000000086626", > ++ }, > ++ { > ++ .name = "small IIO_VAL_FRACTIONAL, 761/1373754273 scaled by 427/2727", > ++ .numerator = 761, > ++ .denominator = 1373754273, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > ++ .schan_val = 427, > ++ .schan_val2 = 2727, > ++ .expected = "0.000000086740", > ++ }, > ++ { > ++ .name = "small IIO_VAL_FRACTIONAL, 5/32768 scaled by 3/10000", > ++ .numerator = 5, > ++ .denominator = 32768, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > ++ .schan_val = 3, > ++ .schan_val2 = 10000, > ++ .expected = "0.0000000457763671875", > ++ }, > ++ { > + .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 1", > + .numerator = 6, > + .denominator = 6, > @@ drivers/iio/test/iio-test-rescale.c (new) > + .expected = "-1.3333333333333333", > + }, > + { > ++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 760/32768 scaled by 15/22", > ++ .numerator = 760, > ++ .denominator = 32768, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2, > ++ .schan_val = 15, > ++ .schan_val2 = 22, > ++ .expected = "0.000000082946", > ++ }, > ++ { > ++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 761/32768 scaled by 15/22", > ++ .numerator = 761, > ++ .denominator = 32768, > ++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2, > ++ .schan_val = 15, > ++ .schan_val2 = 22, > ++ .expected = "0.000000083055", > ++ }, > ++ { > + .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 1", > + .numerator = 16, > + .denominator = 3, > @@ drivers/iio/test/iio-test-rescale.c (new) > +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 > ++ * iio_str_to_nano() - Parse a fixed-point string to get an > ++ * IIO_VAL_INT_PLUS_NANO value > + * @str: The string to parse > -+ * @micro: The number as an integer > ++ * @nano: 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) > ++static int iio_str_to_nano(const char *str, s64 *nano) > +{ > -+ int fract_mult = 100000LL; > ++ int fract_mult = 100000000LL; > + int tmp, tmp2; > + int ret = 0; > + > @@ drivers/iio/test/iio-test-rescale.c (new) > + if (tmp < 0) > + tmp2 *= -1; > + > -+ *micro = (s64)tmp * 10 * fract_mult + tmp2; > ++ *nano = (s64)tmp * 10 * fract_mult + tmp2; > + > + return ret; > +} > + > +/** > -+ * iio_test_relative_error_ppm() - Compute relative error (in ppm) between two > -+ * fixed-point strings > ++ * iio_test_relative_error_ppm() - Compute relative error (in parts-per-million) > ++ * 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. > ++ * relative error in parts-per-million. > + */ > +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); > ++ ret = iio_str_to_nano(real_str, &real); > + if (ret < 0) > + return ret; > + > -+ ret = iio_str_to_micro(exp_str, &exp); > ++ ret = iio_str_to_nano(exp_str, &exp); > + if (ret < 0) > + return ret; > + > ++ if (!exp) { > ++ pr_err("Expected value is null, relative error is undefined\n"); > ++ return -EINVAL; > ++ } > ++ > + err = 1000000 * abs(exp - real); > + err = div64_u64(err, abs(exp)); > + return (int)err; > @@ drivers/iio/test/iio-test-rescale.c (new) > + 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, > ++ KUNIT_EXPECT_EQ_MSG(test, rel_ppm, 0, > + "\t real=%s" > + "\texpected=%s\n", > + buff, t->expected); > 11: 050487186e14 = 11: c4ed463e5fb0 iio: afe: rescale: add RTD temperature sensor support > 12: f36a44a5d898 ! 12: ff2f0dc248a7 iio: afe: rescale: add temperature transducers > @@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de > + s32 offset = 0; > + s32 sense = 1; > + s32 alpha; > -+ s64 tmp; > + int ret; > + > + device_property_read_u32(dev, "sense-offset-millicelsius", &offset); > @@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de > + rescale->numerator = 1000000; > + rescale->denominator = alpha * sense; > + > -+ tmp = (s64)offset * (s64)alpha * (s64)sense; > -+ rescale->offset = div_s64(tmp, (s32)1000000); > ++ rescale->offset = div_s64((s64)offset * rescale->denominator, > ++ rescale->numerator); > + > + return 0; > +} > 13: 63be647fd110 = 13: 84bc1f7d1ab5 dt-bindings: iio: afe: add bindings for temperature-sense-rtd > 14: c2f5c19dece3 = 14: 1b76cfb37e23 dt-bindings: iio: afe: add bindings for temperature transducers > > base-commit: 2b6bff0b122785f09cfbdc34b1aa9edceea6e4c1
On Sun, Nov 21, 2021 at 11:25:56AM +0000, Jonathan Cameron wrote: > On Sun, 14 Nov 2021 22:43:20 -0500 > Liam Beguin <liambeguin@gmail.com> wrote: > > > Hi Jonathan, Peter, > > > > Apologies for not getting back to you sooner. I got caught up on other > > work and wasn't able to dedicate time to this earlier. Hopefully, this > > time around, I'll be able to get this to the finish line :-) > > > > I left out IIO_VAL_INT overflows for now, so that I can focus on getting > > the rest of these changes pulled in, but I don't mind adding a patch for > > that later on. > > > > This series focuses on adding temperature rescaling support to the IIO > > Analog Front End (AFE) driver. > > > > The first few patches address minor bugs in IIO inkernel functions, and > > prepare the AFE driver for the additional features. > > > > The main changes to the AFE driver include an initial Kunit test suite, > > support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs > > and temperature transducer sensors. > > > > Thanks for your time, > > Liam > > Hi Liam, Hi Jonathan, > I'm fine with these. The comment about using the MICRO etc defines can > be handled as as trivial follow up patch. Hopefully someone else can > figure out the 0-day build issue as I didn't managed to. I'll prepare the MICRO, NANO changes if we have a v10, otherwise I'll send them as a follow up patch as you suggest. I'll also try to find more time to dig more into that 0-day build issue. > However, I've long ago lost track of the various precision discussions > you and Peter were having so would like Peter's input before taking these. That's my fault, apologies for letting this sit for so long. > Thanks again for your persistence with this, No worries, thanks for your patience :) Cheers, Liam > > Jonathan > > > > > Changes since v8: > > - reword comment > > - fix erroneous 64-bit division > > - optimize and use 32-bit divisions when values are know to not overflow > > - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed > > point > > - add test cases > > - use nano precision in test cases > > - simplify offset calculation in rtd_props() > > > > 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 > > > > Liam Beguin (14): > > 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 | 271 ++++++- > > drivers/iio/inkern.c | 40 +- > > drivers/iio/test/Kconfig | 10 + > > drivers/iio/test/Makefile | 1 + > > drivers/iio/test/iio-test-rescale.c | 705 ++++++++++++++++++ > > include/linux/iio/afe/rescale.h | 34 + > > 8 files changed, 1232 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 v8: > > 1: 42a7a1047edc = 1: ae3cc93baee6 iio: inkern: apply consumer scale on IIO_VAL_INT cases > > 2: a1cd89fdad11 = 2: 06f66e7f7403 iio: inkern: apply consumer scale when no channel scale is available > > 3: ed0721fb6bd1 = 3: 2dbf6b3bbaeb iio: inkern: make a best effort on offset calculation > > 4: f8fb78bb1112 = 4: b083cf307268 iio: afe: rescale: expose scale processing function > > 5: 504b7a3f830b ! 5: a0bde29ecc8c iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support > > @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale > > + else > > + mult = 1000000LL; > > + /* > > -+ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR > > -+ * *val2 is negative the schan scale is negative > > ++ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if either *val > > ++ * OR *val2 is negative the schan scale is negative, i.e. > > ++ * *val = 1 and *val2 = -0.5 yields -1.5 not -0.5. > > + */ > > + neg = *val < 0 || *val2 < 0; > > + > > 6: c254e9ae813e = 6: c3d0e6248033 iio: afe: rescale: add offset support > > 7: ee8814d6abe4 = 7: 2a81fa735103 iio: afe: rescale: use s64 for temporary scale calculations > > 8: 62cdcfbc9836 = 8: 8315548d0fce iio: afe: rescale: reduce risk of integer overflow > > 9: 88309a5136ee ! 9: 223ed0569cd2 iio: afe: rescale: fix accuracy for small fractional scales > > @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale > > + > > + tmp = div_s64_rem(tmp, 1000000000LL, &rem); > > *val = tmp; > > +- return scale_type; > > ++ > > ++ if (!rem) > > ++ return scale_type; > > + > > -+ /* > > -+ * 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 = *val % (int)tmp; > > ++ *val = *val / (int)tmp; > > + > > -+ return IIO_VAL_INT_PLUS_NANO; > > -+ } > > ++ *val2 = rem / (int)tmp; > > ++ if (rem2) > > ++ *val2 += div_s64((s64)rem2 * 1000000000LL, tmp); > > + > > - return scale_type; > > ++ return IIO_VAL_INT_PLUS_NANO; > > case IIO_VAL_INT_PLUS_NANO: > > case IIO_VAL_INT_PLUS_MICRO: > > + if (scale_type == IIO_VAL_INT_PLUS_NANO) > > 10: fb505a9f42f1 ! 10: 90044efdf8be iio: test: add basic tests for the iio-rescale driver > > @@ drivers/iio/test/Makefile > > # 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 > > + CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN) > > > > ## drivers/iio/test/iio-test-rescale.c (new) ## > > @@ > > @@ drivers/iio/test/iio-test-rescale.c (new) > > + * Use cases with small scales involving divisions > > + */ > > + { > > ++ .name = "small IIO_VAL_FRACTIONAL, 261/509 scaled by 90/1373754273", > > ++ .numerator = 261, > > ++ .denominator = 509, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > > ++ .schan_val = 90, > > ++ .schan_val2 = 1373754273, > > ++ .expected = "0.000000033594", > > ++ }, > > ++ { > > ++ .name = "small IIO_VAL_FRACTIONAL, 90/1373754273 scaled by 261/509", > > ++ .numerator = 90, > > ++ .denominator = 1373754273, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > > ++ .schan_val = 261, > > ++ .schan_val2 = 509, > > ++ .expected = "0.000000033594", > > ++ }, > > ++ { > > ++ .name = "small IIO_VAL_FRACTIONAL, 760/1373754273 scaled by 427/2727", > > ++ .numerator = 760, > > ++ .denominator = 1373754273, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > > ++ .schan_val = 427, > > ++ .schan_val2 = 2727, > > ++ .expected = "0.000000086626", > > ++ }, > > ++ { > > ++ .name = "small IIO_VAL_FRACTIONAL, 761/1373754273 scaled by 427/2727", > > ++ .numerator = 761, > > ++ .denominator = 1373754273, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > > ++ .schan_val = 427, > > ++ .schan_val2 = 2727, > > ++ .expected = "0.000000086740", > > ++ }, > > ++ { > > ++ .name = "small IIO_VAL_FRACTIONAL, 5/32768 scaled by 3/10000", > > ++ .numerator = 5, > > ++ .denominator = 32768, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL, > > ++ .schan_val = 3, > > ++ .schan_val2 = 10000, > > ++ .expected = "0.0000000457763671875", > > ++ }, > > ++ { > > + .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 1", > > + .numerator = 6, > > + .denominator = 6, > > @@ drivers/iio/test/iio-test-rescale.c (new) > > + .expected = "-1.3333333333333333", > > + }, > > + { > > ++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 760/32768 scaled by 15/22", > > ++ .numerator = 760, > > ++ .denominator = 32768, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2, > > ++ .schan_val = 15, > > ++ .schan_val2 = 22, > > ++ .expected = "0.000000082946", > > ++ }, > > ++ { > > ++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 761/32768 scaled by 15/22", > > ++ .numerator = 761, > > ++ .denominator = 32768, > > ++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2, > > ++ .schan_val = 15, > > ++ .schan_val2 = 22, > > ++ .expected = "0.000000083055", > > ++ }, > > ++ { > > + .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 1", > > + .numerator = 16, > > + .denominator = 3, > > @@ drivers/iio/test/iio-test-rescale.c (new) > > +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 > > ++ * iio_str_to_nano() - Parse a fixed-point string to get an > > ++ * IIO_VAL_INT_PLUS_NANO value > > + * @str: The string to parse > > -+ * @micro: The number as an integer > > ++ * @nano: 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) > > ++static int iio_str_to_nano(const char *str, s64 *nano) > > +{ > > -+ int fract_mult = 100000LL; > > ++ int fract_mult = 100000000LL; > > + int tmp, tmp2; > > + int ret = 0; > > + > > @@ drivers/iio/test/iio-test-rescale.c (new) > > + if (tmp < 0) > > + tmp2 *= -1; > > + > > -+ *micro = (s64)tmp * 10 * fract_mult + tmp2; > > ++ *nano = (s64)tmp * 10 * fract_mult + tmp2; > > + > > + return ret; > > +} > > + > > +/** > > -+ * iio_test_relative_error_ppm() - Compute relative error (in ppm) between two > > -+ * fixed-point strings > > ++ * iio_test_relative_error_ppm() - Compute relative error (in parts-per-million) > > ++ * 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. > > ++ * relative error in parts-per-million. > > + */ > > +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); > > ++ ret = iio_str_to_nano(real_str, &real); > > + if (ret < 0) > > + return ret; > > + > > -+ ret = iio_str_to_micro(exp_str, &exp); > > ++ ret = iio_str_to_nano(exp_str, &exp); > > + if (ret < 0) > > + return ret; > > + > > ++ if (!exp) { > > ++ pr_err("Expected value is null, relative error is undefined\n"); > > ++ return -EINVAL; > > ++ } > > ++ > > + err = 1000000 * abs(exp - real); > > + err = div64_u64(err, abs(exp)); > > + return (int)err; > > @@ drivers/iio/test/iio-test-rescale.c (new) > > + 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, > > ++ KUNIT_EXPECT_EQ_MSG(test, rel_ppm, 0, > > + "\t real=%s" > > + "\texpected=%s\n", > > + buff, t->expected); > > 11: 050487186e14 = 11: c4ed463e5fb0 iio: afe: rescale: add RTD temperature sensor support > > 12: f36a44a5d898 ! 12: ff2f0dc248a7 iio: afe: rescale: add temperature transducers > > @@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de > > + s32 offset = 0; > > + s32 sense = 1; > > + s32 alpha; > > -+ s64 tmp; > > + int ret; > > + > > + device_property_read_u32(dev, "sense-offset-millicelsius", &offset); > > @@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de > > + rescale->numerator = 1000000; > > + rescale->denominator = alpha * sense; > > + > > -+ tmp = (s64)offset * (s64)alpha * (s64)sense; > > -+ rescale->offset = div_s64(tmp, (s32)1000000); > > ++ rescale->offset = div_s64((s64)offset * rescale->denominator, > > ++ rescale->numerator); > > + > > + return 0; > > +} > > 13: 63be647fd110 = 13: 84bc1f7d1ab5 dt-bindings: iio: afe: add bindings for temperature-sense-rtd > > 14: c2f5c19dece3 = 14: 1b76cfb37e23 dt-bindings: iio: afe: add bindings for temperature transducers > > > > base-commit: 2b6bff0b122785f09cfbdc34b1aa9edceea6e4c1 >
Hi Liam! On 2021-11-15 04:43, Liam Beguin wrote: > Hi Jonathan, Peter, > > Apologies for not getting back to you sooner. I got caught up on other > work and wasn't able to dedicate time to this earlier. Hopefully, this > time around, I'll be able to get this to the finish line :-) > > I left out IIO_VAL_INT overflows for now, so that I can focus on getting > the rest of these changes pulled in, but I don't mind adding a patch for > that later on. > > This series focuses on adding temperature rescaling support to the IIO > Analog Front End (AFE) driver. > > The first few patches address minor bugs in IIO inkernel functions, and > prepare the AFE driver for the additional features. > > The main changes to the AFE driver include an initial Kunit test suite, > support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs > and temperature transducer sensors. > > Thanks for your time, And thanks for yours! > Liam > > Changes since v8: > - reword comment > - fix erroneous 64-bit division > - optimize and use 32-bit divisions when values are know to not overflow > - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed > point This is not what is going on. Patch 9/14 will convert all fractional scales to fixed point. But I would really like if you in the "reduce risk of integer overflow" patch (8/14) would hold true to the above and keep the fractional scale when possible and only fall back to the less precise fractional-log case if any of the multiplications needed for an exact fractional scale causes overflow. The v8 discussion concluded that this was a valid approach, right? I know you also said that the core exposes the scale with nano precision in sysfs anyway, but that is not true for in-kernel consumers. They have an easier time reading the "real" scale value compared to going via the string representation of fixed point returned from iio_format_value. At least the rescaler itself does so, which means that chaining rescalers might suffer needless accuracy degradation. So, please add the overflow fallback thingy right away, it would make me feel much better. > - add test cases > - use nano precision in test cases > - simplify offset calculation in rtd_props() > > 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 > > Liam Beguin (14): > 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 Can you please swap the order of these two patches? (i.e. "reduce risk..." and "fix accuracy...") Basically, I think the accuracy of the IIO_VAL_FRACTIONAL_LOG2 case should be improved before the IIO_VAL_FRACTIONAL case is joined with it. I.e. swap the order of 8/14 and 9/14 (or almost, you need to also move the addition of the scale_type == IIO_VAL_FRACTIONAL condition to the other patch in order for it to make sense). That's all I'm finding. But then again, I don't know what to do about the 0day report on 10/14. It does say that it's a W=1 build, maybe we need not worry about it? Cheers, Peter > 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
On Mon, 22 Nov 2021 01:53:44 +0100 Peter Rosin <peda@axentia.se> wrote: > Hi Liam! > > On 2021-11-15 04:43, Liam Beguin wrote: > > Hi Jonathan, Peter, > > > > Apologies for not getting back to you sooner. I got caught up on other > > work and wasn't able to dedicate time to this earlier. Hopefully, this > > time around, I'll be able to get this to the finish line :-) > > > > I left out IIO_VAL_INT overflows for now, so that I can focus on getting > > the rest of these changes pulled in, but I don't mind adding a patch for > > that later on. > > > > This series focuses on adding temperature rescaling support to the IIO > > Analog Front End (AFE) driver. > > > > The first few patches address minor bugs in IIO inkernel functions, and > > prepare the AFE driver for the additional features. > > > > The main changes to the AFE driver include an initial Kunit test suite, > > support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs > > and temperature transducer sensors. > > > > Thanks for your time, > > And thanks for yours! > > > Liam > > > > Changes since v8: > > - reword comment > > - fix erroneous 64-bit division > > - optimize and use 32-bit divisions when values are know to not overflow > > - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed > > point > > This is not what is going on. Patch 9/14 will convert all fractional > scales to fixed point. But I would really like if you in the "reduce > risk of integer overflow" patch (8/14) would hold true to the above > and keep the fractional scale when possible and only fall back to > the less precise fractional-log case if any of the multiplications > needed for an exact fractional scale causes overflow. > > The v8 discussion concluded that this was a valid approach, right? > > I know you also said that the core exposes the scale with nano > precision in sysfs anyway, but that is not true for in-kernel > consumers. They have an easier time reading the "real" scale value > compared to going via the string representation of fixed point > returned from iio_format_value. At least the rescaler itself does so, > which means that chaining rescalers might suffer needless accuracy > degradation. > > So, please add the overflow fallback thingy right away, it would make > me feel much better. > > > - add test cases > > - use nano precision in test cases > > - simplify offset calculation in rtd_props() > > > > 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 > > > > Liam Beguin (14): > > 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 > > Can you please swap the order of these two patches? (i.e. "reduce > risk..." and "fix accuracy...") > > Basically, I think the accuracy of the IIO_VAL_FRACTIONAL_LOG2 > case should be improved before the IIO_VAL_FRACTIONAL case is > joined with it. I.e. swap the order of 8/14 and 9/14 (or almost, > you need to also move the addition of the > scale_type == IIO_VAL_FRACTIONAL condition to the other patch in > order for it to make sense). > > That's all I'm finding. But then again, I don't know what to do > about the 0day report on 10/14. It does say that it's a W=1 > build, maybe we need not worry about it? W=1 won't affect that undefined symbols error. I'd be cynical and assume it's a random issue, post a v10 perhaps with a note in the cover letter on this. Jonathan > > Cheers, > Peter > > > 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 >
Hi Peter, On Mon, Nov 22, 2021 at 01:53:44AM +0100, Peter Rosin wrote: > Hi Liam! > > On 2021-11-15 04:43, Liam Beguin wrote: > > Hi Jonathan, Peter, > > > > Apologies for not getting back to you sooner. I got caught up on other > > work and wasn't able to dedicate time to this earlier. Hopefully, this > > time around, I'll be able to get this to the finish line :-) > > > > I left out IIO_VAL_INT overflows for now, so that I can focus on getting > > the rest of these changes pulled in, but I don't mind adding a patch for > > that later on. > > > > This series focuses on adding temperature rescaling support to the IIO > > Analog Front End (AFE) driver. > > > > The first few patches address minor bugs in IIO inkernel functions, and > > prepare the AFE driver for the additional features. > > > > The main changes to the AFE driver include an initial Kunit test suite, > > support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs > > and temperature transducer sensors. > > > > Thanks for your time, > > And thanks for yours! > > > Liam > > > > Changes since v8: > > - reword comment > > - fix erroneous 64-bit division > > - optimize and use 32-bit divisions when values are know to not overflow > > - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed > > point > > This is not what is going on. Patch 9/14 will convert all fractional > scales to fixed point. But I would really like if you in the "reduce > risk of integer overflow" patch (8/14) would hold true to the above > and keep the fractional scale when possible and only fall back to > the less precise fractional-log case if any of the multiplications > needed for an exact fractional scale causes overflow. Thanks for looking at these patches again. > The v8 discussion concluded that this was a valid approach, right? Yes, I remember you saying that you'd be more comfortable keeping the IIO_VAL_FRACTIONAL. > I know you also said that the core exposes the scale with nano > precision in sysfs anyway, but that is not true for in-kernel > consumers. They have an easier time reading the "real" scale value > compared to going via the string representation of fixed point > returned from iio_format_value. At least the rescaler itself does so, > which means that chaining rescalers might suffer needless accuracy > degradation. Agreed, that makes total sense. If I'm not mistaken, the first condition in the case, if (!rem), will return IIO_VAL_FRACTIONAL if the division is exact, keeping all the precision. No? > So, please add the overflow fallback thingy right away, it would make > me feel much better. > > > - add test cases > > - use nano precision in test cases > > - simplify offset calculation in rtd_props() > > > > 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 > > > > Liam Beguin (14): > > 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 > > Can you please swap the order of these two patches? (i.e. "reduce > risk..." and "fix accuracy...") > > Basically, I think the accuracy of the IIO_VAL_FRACTIONAL_LOG2 > case should be improved before the IIO_VAL_FRACTIONAL case is > joined with it. I.e. swap the order of 8/14 and 9/14 (or almost, > you need to also move the addition of the > scale_type == IIO_VAL_FRACTIONAL condition to the other patch in > order for it to make sense). Makes sense! I'll swap the order of these commits. > That's all I'm finding. But then again, I don't know what to do > about the 0day report on 10/14. It does say that it's a W=1 > build, maybe we need not worry about it? I didn't have a chance to look into that more, but will now. Cheers, Liam > Cheers, > Peter > > > 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 >
Hi! On 2021-11-27 21:27, Liam Beguin wrote: > Hi Peter, > > On Mon, Nov 22, 2021 at 01:53:44AM +0100, Peter Rosin wrote: >> Hi Liam! >> >> On 2021-11-15 04:43, Liam Beguin wrote: >>> Hi Jonathan, Peter, snip >>> - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed >>> point >> >> This is not what is going on. Patch 9/14 will convert all fractional >> scales to fixed point. But I would really like if you in the "reduce >> risk of integer overflow" patch (8/14) would hold true to the above >> and keep the fractional scale when possible and only fall back to >> the less precise fractional-log case if any of the multiplications >> needed for an exact fractional scale causes overflow. > > Thanks for looking at these patches again. > >> The v8 discussion concluded that this was a valid approach, right? > > Yes, I remember you saying that you'd be more comfortable keeping the > IIO_VAL_FRACTIONAL. > >> I know you also said that the core exposes the scale with nano >> precision in sysfs anyway, but that is not true for in-kernel >> consumers. They have an easier time reading the "real" scale value >> compared to going via the string representation of fixed point >> returned from iio_format_value. At least the rescaler itself does so, >> which means that chaining rescalers might suffer needless accuracy >> degradation. > > Agreed, that makes total sense. > > If I'm not mistaken, the first condition in the case, if (!rem), will > return IIO_VAL_FRACTIONAL if the division is exact, keeping all the > precision. No? Only if the resulting scale fits in nine decimals. That's never the case if you have primes other than 2 and 5 in the denominator (after eliminating gcd of course). Which mean that if you chain one rescaler doing 1/3 and one doing 3/1, you would get a combined scale of 0.999999999 instead of 3/3 if we take the approach of these patches. So, what I'm after is that - for IIO_VAL_FRACTIONAL - not take the multiply-by-1e9 code path /unless/ the existing fractional approach overflows in either numerator or denominator (or both). Side note: The same could be done for IIO_VAL_INT when the numerator overflows (since the denominator cannot overflow), but I guess that can be done later. Cheers, Peter
Hi Peter, On Sun, Nov 28, 2021 at 10:17:50AM +0100, Peter Rosin wrote: > Hi! > > On 2021-11-27 21:27, Liam Beguin wrote: > > Hi Peter, > > > > On Mon, Nov 22, 2021 at 01:53:44AM +0100, Peter Rosin wrote: > >> Hi Liam! > >> > >> On 2021-11-15 04:43, Liam Beguin wrote: > >>> Hi Jonathan, Peter, > > snip > > >>> - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed > >>> point > >> > >> This is not what is going on. Patch 9/14 will convert all fractional > >> scales to fixed point. But I would really like if you in the "reduce > >> risk of integer overflow" patch (8/14) would hold true to the above > >> and keep the fractional scale when possible and only fall back to > >> the less precise fractional-log case if any of the multiplications > >> needed for an exact fractional scale causes overflow. > > > > Thanks for looking at these patches again. > > > >> The v8 discussion concluded that this was a valid approach, right? > > > > Yes, I remember you saying that you'd be more comfortable keeping the > > IIO_VAL_FRACTIONAL. > > > >> I know you also said that the core exposes the scale with nano > >> precision in sysfs anyway, but that is not true for in-kernel > >> consumers. They have an easier time reading the "real" scale value > >> compared to going via the string representation of fixed point > >> returned from iio_format_value. At least the rescaler itself does so, > >> which means that chaining rescalers might suffer needless accuracy > >> degradation. > > > > Agreed, that makes total sense. > > > > If I'm not mistaken, the first condition in the case, if (!rem), will > > return IIO_VAL_FRACTIONAL if the division is exact, keeping all the > > precision. No? > > Only if the resulting scale fits in nine decimals. That's never the > case if you have primes other than 2 and 5 in the denominator (after > eliminating gcd of course). Which mean that if you chain one rescaler > doing 1/3 and one doing 3/1, you would get a combined scale of > 0.999999999 instead of 3/3 if we take the approach of these patches. > > So, what I'm after is that - for IIO_VAL_FRACTIONAL - not take the > multiply-by-1e9 code path /unless/ the existing fractional approach > overflows in either numerator or denominator (or both). Understood, I'll update based on this. > Side note: The same could be done for IIO_VAL_INT when the numerator > overflows (since the denominator cannot overflow), but I guess that > can be done later. Agreed, I don't mind working on this later but I'd like to focus on getting the current changes in first. Thanks, Liam > Cheers, > Peter
Hi Jonathan, Peter, Apologies for not getting back to you sooner. I got caught up on other work and wasn't able to dedicate time to this earlier. Hopefully, this time around, I'll be able to get this to the finish line :-) I left out IIO_VAL_INT overflows for now, so that I can focus on getting the rest of these changes pulled in, but I don't mind adding a patch for that later on. This series focuses on adding temperature rescaling support to the IIO Analog Front End (AFE) driver. The first few patches address minor bugs in IIO inkernel functions, and prepare the AFE driver for the additional features. The main changes to the AFE driver include an initial Kunit test suite, support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs and temperature transducer sensors. Thanks for your time, Liam Changes since v8: - reword comment - fix erroneous 64-bit division - optimize and use 32-bit divisions when values are know to not overflow - keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed point - add test cases - use nano precision in test cases - simplify offset calculation in rtd_props() 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 Liam Beguin (14): 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 | 271 ++++++- drivers/iio/inkern.c | 40 +- drivers/iio/test/Kconfig | 10 + drivers/iio/test/Makefile | 1 + drivers/iio/test/iio-test-rescale.c | 705 ++++++++++++++++++ include/linux/iio/afe/rescale.h | 34 + 8 files changed, 1232 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 v8: 1: 42a7a1047edc = 1: ae3cc93baee6 iio: inkern: apply consumer scale on IIO_VAL_INT cases 2: a1cd89fdad11 = 2: 06f66e7f7403 iio: inkern: apply consumer scale when no channel scale is available 3: ed0721fb6bd1 = 3: 2dbf6b3bbaeb iio: inkern: make a best effort on offset calculation 4: f8fb78bb1112 = 4: b083cf307268 iio: afe: rescale: expose scale processing function 5: 504b7a3f830b ! 5: a0bde29ecc8c iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale + else + mult = 1000000LL; + /* -+ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR -+ * *val2 is negative the schan scale is negative ++ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if either *val ++ * OR *val2 is negative the schan scale is negative, i.e. ++ * *val = 1 and *val2 = -0.5 yields -1.5 not -0.5. + */ + neg = *val < 0 || *val2 < 0; + 6: c254e9ae813e = 6: c3d0e6248033 iio: afe: rescale: add offset support 7: ee8814d6abe4 = 7: 2a81fa735103 iio: afe: rescale: use s64 for temporary scale calculations 8: 62cdcfbc9836 = 8: 8315548d0fce iio: afe: rescale: reduce risk of integer overflow 9: 88309a5136ee ! 9: 223ed0569cd2 iio: afe: rescale: fix accuracy for small fractional scales @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale + + tmp = div_s64_rem(tmp, 1000000000LL, &rem); *val = tmp; +- return scale_type; ++ ++ if (!rem) ++ return scale_type; + -+ /* -+ * 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 = *val % (int)tmp; ++ *val = *val / (int)tmp; + -+ return IIO_VAL_INT_PLUS_NANO; -+ } ++ *val2 = rem / (int)tmp; ++ if (rem2) ++ *val2 += div_s64((s64)rem2 * 1000000000LL, tmp); + - return scale_type; ++ return IIO_VAL_INT_PLUS_NANO; case IIO_VAL_INT_PLUS_NANO: case IIO_VAL_INT_PLUS_MICRO: + if (scale_type == IIO_VAL_INT_PLUS_NANO) 10: fb505a9f42f1 ! 10: 90044efdf8be iio: test: add basic tests for the iio-rescale driver @@ drivers/iio/test/Makefile # 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 + CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN) ## drivers/iio/test/iio-test-rescale.c (new) ## @@ @@ drivers/iio/test/iio-test-rescale.c (new) + * Use cases with small scales involving divisions + */ + { ++ .name = "small IIO_VAL_FRACTIONAL, 261/509 scaled by 90/1373754273", ++ .numerator = 261, ++ .denominator = 509, ++ .schan_scale_type = IIO_VAL_FRACTIONAL, ++ .schan_val = 90, ++ .schan_val2 = 1373754273, ++ .expected = "0.000000033594", ++ }, ++ { ++ .name = "small IIO_VAL_FRACTIONAL, 90/1373754273 scaled by 261/509", ++ .numerator = 90, ++ .denominator = 1373754273, ++ .schan_scale_type = IIO_VAL_FRACTIONAL, ++ .schan_val = 261, ++ .schan_val2 = 509, ++ .expected = "0.000000033594", ++ }, ++ { ++ .name = "small IIO_VAL_FRACTIONAL, 760/1373754273 scaled by 427/2727", ++ .numerator = 760, ++ .denominator = 1373754273, ++ .schan_scale_type = IIO_VAL_FRACTIONAL, ++ .schan_val = 427, ++ .schan_val2 = 2727, ++ .expected = "0.000000086626", ++ }, ++ { ++ .name = "small IIO_VAL_FRACTIONAL, 761/1373754273 scaled by 427/2727", ++ .numerator = 761, ++ .denominator = 1373754273, ++ .schan_scale_type = IIO_VAL_FRACTIONAL, ++ .schan_val = 427, ++ .schan_val2 = 2727, ++ .expected = "0.000000086740", ++ }, ++ { ++ .name = "small IIO_VAL_FRACTIONAL, 5/32768 scaled by 3/10000", ++ .numerator = 5, ++ .denominator = 32768, ++ .schan_scale_type = IIO_VAL_FRACTIONAL, ++ .schan_val = 3, ++ .schan_val2 = 10000, ++ .expected = "0.0000000457763671875", ++ }, ++ { + .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 1", + .numerator = 6, + .denominator = 6, @@ drivers/iio/test/iio-test-rescale.c (new) + .expected = "-1.3333333333333333", + }, + { ++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 760/32768 scaled by 15/22", ++ .numerator = 760, ++ .denominator = 32768, ++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2, ++ .schan_val = 15, ++ .schan_val2 = 22, ++ .expected = "0.000000082946", ++ }, ++ { ++ .name = "small IIO_VAL_FRACTIONAL_LOG2, 761/32768 scaled by 15/22", ++ .numerator = 761, ++ .denominator = 32768, ++ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2, ++ .schan_val = 15, ++ .schan_val2 = 22, ++ .expected = "0.000000083055", ++ }, ++ { + .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 1", + .numerator = 16, + .denominator = 3, @@ drivers/iio/test/iio-test-rescale.c (new) +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 ++ * iio_str_to_nano() - Parse a fixed-point string to get an ++ * IIO_VAL_INT_PLUS_NANO value + * @str: The string to parse -+ * @micro: The number as an integer ++ * @nano: 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) ++static int iio_str_to_nano(const char *str, s64 *nano) +{ -+ int fract_mult = 100000LL; ++ int fract_mult = 100000000LL; + int tmp, tmp2; + int ret = 0; + @@ drivers/iio/test/iio-test-rescale.c (new) + if (tmp < 0) + tmp2 *= -1; + -+ *micro = (s64)tmp * 10 * fract_mult + tmp2; ++ *nano = (s64)tmp * 10 * fract_mult + tmp2; + + return ret; +} + +/** -+ * iio_test_relative_error_ppm() - Compute relative error (in ppm) between two -+ * fixed-point strings ++ * iio_test_relative_error_ppm() - Compute relative error (in parts-per-million) ++ * 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. ++ * relative error in parts-per-million. + */ +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); ++ ret = iio_str_to_nano(real_str, &real); + if (ret < 0) + return ret; + -+ ret = iio_str_to_micro(exp_str, &exp); ++ ret = iio_str_to_nano(exp_str, &exp); + if (ret < 0) + return ret; + ++ if (!exp) { ++ pr_err("Expected value is null, relative error is undefined\n"); ++ return -EINVAL; ++ } ++ + err = 1000000 * abs(exp - real); + err = div64_u64(err, abs(exp)); + return (int)err; @@ drivers/iio/test/iio-test-rescale.c (new) + 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, ++ KUNIT_EXPECT_EQ_MSG(test, rel_ppm, 0, + "\t real=%s" + "\texpected=%s\n", + buff, t->expected); 11: 050487186e14 = 11: c4ed463e5fb0 iio: afe: rescale: add RTD temperature sensor support 12: f36a44a5d898 ! 12: ff2f0dc248a7 iio: afe: rescale: add temperature transducers @@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de + s32 offset = 0; + s32 sense = 1; + s32 alpha; -+ s64 tmp; + int ret; + + device_property_read_u32(dev, "sense-offset-millicelsius", &offset); @@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de + rescale->numerator = 1000000; + rescale->denominator = alpha * sense; + -+ tmp = (s64)offset * (s64)alpha * (s64)sense; -+ rescale->offset = div_s64(tmp, (s32)1000000); ++ rescale->offset = div_s64((s64)offset * rescale->denominator, ++ rescale->numerator); + + return 0; +} 13: 63be647fd110 = 13: 84bc1f7d1ab5 dt-bindings: iio: afe: add bindings for temperature-sense-rtd 14: c2f5c19dece3 = 14: 1b76cfb37e23 dt-bindings: iio: afe: add bindings for temperature transducers base-commit: 2b6bff0b122785f09cfbdc34b1aa9edceea6e4c1