| Message ID | 20220130161101.1067691-1-liambeguin@gmail.com |
|---|---|
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
Hi! I noticed that I have not reviewed this patch. Sorry for my low bandwidth. On 2022-01-30 17:10, Liam Beguin wrote: > Make use of well-defined SI metric prefixes to improve code readability. > > Signed-off-by: Liam Beguin <liambeguin@gmail.com> > --- > drivers/iio/afe/iio-rescale.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 67273de46843..27c6664915ff 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -51,11 +51,11 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > } > fallthrough; > case IIO_VAL_FRACTIONAL_LOG2: > - tmp = (s64)*val * 1000000000LL; > + tmp = (s64)*val * GIGA; > tmp = div_s64(tmp, rescale->denominator); > tmp *= rescale->numerator; > > - tmp = div_s64_rem(tmp, 1000000000LL, &rem); > + tmp = div_s64_rem(tmp, GIGA, &rem); It is NOT easy for me to say which of GIGA/NANO is most fitting. There are a couple of considerations: A) 1000000000 is just a big value (GIGA fits). Something big is needed to not lose too much precision. B) 1000000000 is what the IIO core uses to print fractional-log values with nano precision (NANO fits). This is not really relevant in this context. C) 1000000000 makes the int-plus-nano and fractional-log cases align (NANO fits). This last consideration is introduced with patch 4/11. There is simply no correct define to use. And whichever define is chosen makes the other interpretation less obvious. Which is not desirable, obscures things and make both GIGA and NANO bad options. So, I stepped back to the description provided by Andy in the comments of v11: On 2021-12-22 19:59, Andy Shevchenko wrote: | You should get the proper power after the operation. | Write a formula (mathematically speaking) and check each of them for this. | | 10^-5/10^-9 == 1*10^4 (Used NANO) | 10^-5/10^9 == 1/10^-14 (Used GIGA) | | See the difference? No, I don't really see the difference, that just makes me totally confused. Dividing by 10^-9 or multiplying by 10^9 is as we all know exactly the same, and the kernel cannot deal directly with 10^-9 so both will look the same in code (multiplying by 10^9). So, you must be referring to the "real formula" behind the code. But in that case, if the "real formula" behind the (then equivalent) code had instead been 10^-5*10^9 == 1*10^4 (Used GIGA) 10^-5*10^-9 == 1/10^-14 (Used NANO) the outcome is the opposite. NANO turns GIGA and vice versa. Since you can express the same thing differently in math too, it all crumbles for me. Because of this duality, it will be a matter of taste if GIGA or NANO fits best in any given instance. Sometimes (perhaps commonly) it will be decidedly easy to pick one of them, but in other cases (see above) we will end up with a conflict. What to do then? Or, what am I missing? My taste says NANO in this case, since A) is just some big number and not really about units and B) is as stated not really relevant. Which makes C) win the argument for me. > *val = tmp; > > if (!rem) > @@ -71,7 +71,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > > *val2 = rem / (int)tmp; > if (rem2) > - *val2 += div_s64((s64)rem2 * 1000000000LL, tmp); > + *val2 += div_s64((s64)rem2 * GIGA, tmp); Here, 1000000000 matches the above use. If we go with NANO above, we should go with NANO here as well. > return IIO_VAL_INT_PLUS_NANO; > case IIO_VAL_INT_PLUS_NANO: > @@ -332,8 +332,8 @@ static int rescale_current_sense_amplifier_props(struct device *dev, > * gain_div / (gain_mult * sense), while trying to keep the > * numerator/denominator from overflowing. > */ > - factor = gcd(sense, 1000000); > - rescale->numerator = 1000000 / factor; > + factor = gcd(sense, MEGA); > + rescale->numerator = MEGA / factor; Here, the 1000000 number comes from the unit of the sense resistor (micro-ohms), so I would have preferred MICRO. But who can tell if we -mathematically speaking- have divided the given resistance integer by 10^6 (MEGA) or multiplied it with 10^-6 (MICRO) to account for the unit? Or if we divided the other values with 10^6 (MEGA) (or multiplied by 10^-6, MICRO) to make them fit the unit of the shunt resistance? All of the above is of course equivalent so both MEGA and MICRO are correct. But as stated, MICRO makes to most sense as that is what connects the code to reality and hints at where the value is coming from. For me anyway. > rescale->denominator = sense / factor; > > factor = gcd(rescale->numerator, gain_mult); > @@ -361,8 +361,8 @@ static int rescale_current_sense_shunt_props(struct device *dev, > return ret; > } > > - factor = gcd(shunt, 1000000); > - rescale->numerator = 1000000 / factor; > + factor = gcd(shunt, MEGA); > + rescale->numerator = MEGA / factor; Same here, 1000000 comes from the micro-ohms unit of the shunt resistor, so I would have preferred MICRO. Sorry for the long mail. I blame the duality of these ambiguous SI-defines that are a bit confusing to me. Cheers, Peter > rescale->denominator = shunt / factor; > > return 0;
Hi! On 2022-01-30 17:10, Liam Beguin wrote: > 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 <liambeguin@gmail.com> > Reviewed-by: Peter Rosin <peda@axentia.se> > --- > 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 65832dd09249..f833eb38f8bb 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -14,6 +14,7 @@ > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/property.h> > +#include <linux/units.h> This include should be moved to the first patch that uses stuff from it. Cheers, Peter *snip*
Hi Peter, On Wed, Feb 02, 2022 at 05:58:25PM +0100, Peter Rosin wrote: > Hi! > > On 2022-01-30 17:10, Liam Beguin wrote: > > 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 <liambeguin@gmail.com> > > Reviewed-by: Peter Rosin <peda@axentia.se> > > --- -- snip -- > > + > > + tmp = r0 * iexc * alpha / MEGA; > > + factor = gcd(tmp, MEGA); > > + rescale->numerator = MEGA / factor; > > + rescale->denominator = tmp / factor; > > + > > + rescale->offset = -1 * ((r0 * iexc) / MEGA * MILLI); > > The inner (unneeded) brackets are not helping with clarifying > the precedence. The most "problematic" operation is the last > multiplication inside the outer brackets. Extra brackets are > more useful like this, methinks: > > rescale->offset = -1 * ((r0 * iexc / MEGA) * MILLI); > > But, what is more important is that you in v10 had: > > rescale->offset = -1 * ((r0 * iexc) / 1000); > > What you tricked yourself into writing when you converted to > these prefix defines is not equivalent. You lose precision. > > I.e. dividing by 1000000 and then multiplying by 1000 is not > the same as dividing directly with 1000. And you know this, but > didn't notice perhaps exactly because you got yourself entangled > in prefix macros that blurred the picture? Apologies for this oversight. Your observation is correct, I looked at the prefix changes and failed to catch this mistake. Would you be okay with the following: rescale->offset = -1 * ((r0 * iexc) / KILO); This would keep things consistent with what I said here[1]. [1] https://lore.kernel.org/linux-iio/YfmJ3P1gYaEkVjlY@shaak/ > These macros have wasted quite a bit of review time. I'm not > fully convinced they represent an improvement... Sorry for the wasted cycles here. Cheers, Liam > Cheers, > Peter > > > + > > + return 0; > > +} > > + > > enum rescale_variant { > > CURRENT_SENSE_AMPLIFIER, > > CURRENT_SENSE_SHUNT, > > VOLTAGE_DIVIDER, > > + TEMP_SENSE_RTD, > > }; > > > > static const struct rescale_cfg rescale_cfg[] = { > > @@ -414,6 +456,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[] = { > > @@ -423,6 +469,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);
Hi Peter, On Wed, Feb 02, 2022 at 06:04:25PM +0100, Peter Rosin wrote: > Hi! > > On 2022-01-30 17:10, Liam Beguin wrote: > > 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 <liambeguin@gmail.com> > > Reviewed-by: Peter Rosin <peda@axentia.se> > > --- > > 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 65832dd09249..f833eb38f8bb 100644 > > --- a/drivers/iio/afe/iio-rescale.c > > +++ b/drivers/iio/afe/iio-rescale.c > > @@ -14,6 +14,7 @@ > > #include <linux/of_device.h> > > #include <linux/platform_device.h> > > #include <linux/property.h> > > +#include <linux/units.h> > > This include should be moved to the first patch that uses stuff from > it. Some defines are used a bit further down in mult (copied back from the original message): > > case IIO_VAL_INT_PLUS_NANO: > > case IIO_VAL_INT_PLUS_MICRO: > > mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA; > Cheers, > Peter > > *snip* Cheers, Liam
Jonathan, Peter, Andy, This series focuses on adding temperature rescaling support to the IIO Analog Front End (AFE) driver. The first few patches from previous iterations addressing minor bugs in IIO inkernel functions have been taken in, and are no longer in v13. 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. I'm not quite sure what happened with the left-shift change last time, I had it in my v12 local branch, it seems I got mixed up before sending. Thanks for your time, Liam Changes since v12: - rebase on latest testing branch - fix copyright holder in newly created header file - add myself as a copyright holder of the iio-rescale.c driver at Peter's suggestion - fix undefined behavior on left-shift operation Changes since v11: - update commits with my personal email since all this work was done on my own time - apply Peter's Reviewed-by to my local tree - fix use of units.h - make use of units.h more consistently in iio-rescale.c and in the tests - fix #include ordering - treat 04/16 as a fix. Move it, and add a Fixes: tag - fix undefined behavior on left-shift operation - add comment about fract_mult with iio_str_to_fixpoint() - reword commit message for 14/16, based on Andy's comments Changes since v10: - apply Andy's suggestion for offset calculations - make use of units.h more consistently Changes since v9: - make use of linux/units.h - reorder commits, fix fract_log2 before merging fract - keep fractional representation when not overflowing 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 (11): 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: fix accuracy for small fractional scales iio: afe: rescale: reduce risk of integer overflow iio: afe: rescale: make use of units.h 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 | 292 ++++++- drivers/iio/test/Kconfig | 10 + drivers/iio/test/Makefile | 1 + drivers/iio/test/iio-test-rescale.c | 711 ++++++++++++++++++ include/linux/iio/afe/rescale.h | 36 + 7 files changed, 1226 insertions(+), 39 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 v12: 1: a8ca9300ef2a < -: ------------ iio: inkern: apply consumer scale on IIO_VAL_INT cases 2: efaeceac8d87 < -: ------------ iio: inkern: apply consumer scale when no channel scale is available 3: 8131208a4454 < -: ------------ iio: inkern: make a best effort on offset calculation 4: 06202d8f6481 < -: ------------ iio: afe: rescale: use s64 for temporary scale calculations 5: 87b9d77f0d30 < -: ------------ iio: afe: rescale: reorder includes 6: e9bf09ca9703 ! 1: ee26b0eeac65 iio: afe: rescale: expose scale processing function @@ include/linux/iio/afe/rescale.h (new) @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* -+ * Copyright (C) 2021 Liam Beguin <liambeguin@gmail.com> ++ * Copyright (C) 2018 Axentia Technologies AB + */ + +#ifndef __IIO_RESCALE_H__ 7: 865296d2bc4f = 2: a510097c83f1 iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support 8: aea3159ed169 ! 3: 8f2f2699a9b4 iio: afe: rescale: add offset support @@ Commit message Reviewed-by: Peter Rosin <peda@axentia.se> ## drivers/iio/afe/iio-rescale.c ## +@@ + * IIO rescale driver + * + * Copyright (C) 2018 Axentia Technologies AB ++ * Copyright (C) 2022 Liam Beguin <liambeguin@gmail.com> + * + * Author: Peter Rosin <peda@axentia.se> + */ @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale, int scale_type, } } 9: 7b518cba1cb5 = 4: 2efa970bad26 iio: afe: rescale: fix accuracy for small fractional scales 10: 79844ae7461c ! 5: 201037c0ead8 iio: afe: rescale: reduce risk of integer overflow @@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale + if (scale_type == IIO_VAL_FRACTIONAL) + tmp = *val2; + else -+ tmp = 1 << *val2; ++ tmp = ULL(1) << *val2; rem2 = *val % (int)tmp; *val = *val / (int)tmp; 11: 19f28d029522 = 6: 0e3bf50d9eb2 iio: afe: rescale: make use of units.h 12: 18b743ae2f8b = 7: 72813d9788e4 iio: test: add basic tests for the iio-rescale driver 13: 240a3f1424fc = 8: 8ee4c16355af iio: afe: rescale: add RTD temperature sensor support 14: d7dc1e1f8f9c = 9: 36a9bb066369 iio: afe: rescale: add temperature transducers 15: c0a94061491a = 10: 581962b44cf3 dt-bindings: iio: afe: add bindings for temperature-sense-rtd 16: b29eed6b4e17 = 11: d09d377b05ac dt-bindings: iio: afe: add bindings for temperature transducers base-commit: cd717ac6f69db4953ca701c6220c7cb58e17f35a