[PATCH v13 00/11] iio: afe: add temperature rescaling support

[PATCH v13 00/11] iio: afe: add temperature rescaling support

[Liam Beguin]

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
-- 
2.35.1.4.g5d01301f2b86

[PATCH v13 01/11] iio: afe: rescale: expose scale processing function

[Liam Beguin]

In preparation for the addition of kunit tests, expose the logic
responsible for combining channel scales.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/iio-rescale.c   | 65 ++++++++++++++-------------------
 include/linux/iio/afe/rescale.h | 32 ++++++++++++++++
 2 files changed, 60 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 69710c481376..65832dd09249 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -15,32 +15,43 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 
+#include <linux/iio/afe/rescale.h>
 #include <linux/iio/consumer.h>
 #include <linux/iio/iio.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)
+{
+	s64 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 = (s64)*val * 1000000000LL;
+		tmp = div_s64(tmp, rescale->denominator);
+		tmp *= rescale->numerator;
+		tmp = div_s64(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);
-	s64 tmp;
 	int ret;
 
 	switch (mask) {
@@ -66,27 +77,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 = (s64)*val * 1000000000LL;
-			tmp = div_s64(tmp, rescale->denominator);
-			tmp *= rescale->numerator;
-			tmp = div_s64(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..8a2eb34af327
--- /dev/null
+++ b/include/linux/iio/afe/rescale.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2018 Axentia Technologies AB
+ */
+
+#ifndef __IIO_RESCALE_H__
+#define __IIO_RESCALE_H__
+
+#include <linux/types.h>
+#include <linux/iio/iio.h>
+
+struct device;
+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.35.1.4.g5d01301f2b86

[PATCH v13 02/11] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support

[Liam Beguin]

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>
 
 #include <linux/iio/afe/rescale.h>
 #include <linux/iio/consumer.h>
@@ -23,6 +24,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
 	s64 tmp;
+	s32 rem;
+	u32 mult;
+	u32 neg;
 
 	switch (scale_type) {
 	case IIO_VAL_FRACTIONAL:
@@ -41,6 +45,37 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		tmp *= rescale->numerator;
 		tmp = div_s64(tmp, 1000000000LL);
 		*val = tmp;
+		return scale_type;
+	case IIO_VAL_INT_PLUS_NANO:
+	case IIO_VAL_INT_PLUS_MICRO:
+		mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA;
+
+		/*
+		 * 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;
+
+		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.35.1.4.g5d01301f2b86

Re: [PATCH v13 02/11] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support

[Peter Rosin]

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*

Re: [PATCH v13 02/11] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support

[Liam Beguin]

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

[PATCH v13 03/11] iio: afe: rescale: add offset support

[Liam Beguin]

This is a preparatory change required for the addition of temperature
sensing front ends.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/iio-rescale.c   | 81 +++++++++++++++++++++++++++++++++
 include/linux/iio/afe/rescale.h |  4 ++
 2 files changed, 85 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index f833eb38f8bb..63035b4bce5e 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -3,6 +3,7 @@
  * IIO rescale driver
  *
  * Copyright (C) 2018 Axentia Technologies AB
+ * Copyright (C) 2022 Liam Beguin <liambeguin@gmail.com>
  *
  * Author: Peter Rosin <peda@axentia.se>
  */
@@ -82,11 +83,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 * GIGA;
+		tmp2 = ((s64)scale * GIGA) + scale2;
+		*val = div64_s64(tmp, tmp2) + schan_off;
+		return IIO_VAL_INT;
+	case IIO_VAL_INT_PLUS_MICRO:
+		tmp = (s64)rescale->offset * MEGA;
+		tmp2 = ((s64)scale * MEGA) + 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) {
@@ -113,6 +149,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;
 	}
@@ -189,6 +266,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
@@ -353,6 +433,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 8a2eb34af327..6eecb435488f 100644
--- a/include/linux/iio/afe/rescale.h
+++ b/include/linux/iio/afe/rescale.h
@@ -25,8 +25,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.35.1.4.g5d01301f2b86

[PATCH v13 04/11] iio: afe: rescale: fix accuracy for small fractional scales

[Liam Beguin]

The approximation caused by integer divisions can be costly on smaller
scale values since the decimal part is significant compared to the
integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
cases to maintain accuracy.

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

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 63035b4bce5e..468e6c345bd1 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -25,7 +25,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;
 
@@ -44,9 +44,23 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		tmp = (s64)*val * 1000000000LL;
 		tmp = div_s64(tmp, rescale->denominator);
 		tmp *= rescale->numerator;
-		tmp = div_s64(tmp, 1000000000LL);
+
+		tmp = div_s64_rem(tmp, 1000000000LL, &rem);
 		*val = tmp;
-		return scale_type;
+
+		if (!rem)
+			return scale_type;
+
+		tmp = 1 << *val2;
+
+		rem2 = *val % (int)tmp;
+		*val = *val / (int)tmp;
+
+		*val2 = rem / (int)tmp;
+		if (rem2)
+			*val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
+
+		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:
 		mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA;
-- 
2.35.1.4.g5d01301f2b86

[PATCH v13 05/11] iio: afe: rescale: reduce risk of integer overflow

[Liam Beguin]

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 <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/iio-rescale.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 468e6c345bd1..67273de46843 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -25,21 +25,31 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
 	s64 tmp;
+	int _val, _val2;
 	s32 rem, rem2;
 	u32 mult;
 	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:
+		/*
+		 * When the product of both scales doesn't overflow, avoid
+		 * potential accuracy loss (for in kernel consumers) by
+		 * keeping a fractional representation.
+		 */
+		if (!check_mul_overflow(*val, rescale->numerator, &_val) &&
+		    !check_mul_overflow(*val2, rescale->denominator, &_val2)) {
+			*val = _val;
+			*val2 = _val2;
+			return IIO_VAL_FRACTIONAL;
+		}
+		fallthrough;
 	case IIO_VAL_FRACTIONAL_LOG2:
 		tmp = (s64)*val * 1000000000LL;
 		tmp = div_s64(tmp, rescale->denominator);
@@ -51,7 +61,10 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		if (!rem)
 			return scale_type;
 
-		tmp = 1 << *val2;
+		if (scale_type == IIO_VAL_FRACTIONAL)
+			tmp = *val2;
+		else
+			tmp = ULL(1) << *val2;
 
 		rem2 = *val % (int)tmp;
 		*val = *val / (int)tmp;
-- 
2.35.1.4.g5d01301f2b86

[PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Liam Beguin]

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);
 		*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);
 
 		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;
 	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;
 	rescale->denominator = shunt / factor;
 
 	return 0;
-- 
2.35.1.4.g5d01301f2b86

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Peter Rosin]

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;

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Andy Shevchenko]

On Mon, Jan 31, 2022 at 4:50 PM Peter Rosin <peda@axentia.se> wrote:
> On 2022-01-30 17:10, Liam Beguin wrote:

...

> > -             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.

What do you mean? The idea behind is the use of the macro depending on
the actual power of 10 in use (taking into account the sign of the
exponent part).

> 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.

Does it have a physical meaning?

> 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.

Same question.

> 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.

Huh?!
I believe the answer to the A and B -- yes, which means there are the
correct and incorrect variants 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.

The (main) idea of the macros is to avoid mistyping 0s in the numbers
and miscalculations. Besides that it also provides the same type for
the constants.

> 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.

Sounds like we have a chat here between physicists and computer
science folks :-)

Let's try again, does the value in the tmp variable above have a
_physical_ meaning? (I believe so, because all IIO subsystem is about
physical values)

> 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).

Yes, and using proper macro makes much cleaner the mathematical and
physical point of view to the values.

> 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.

Again, one needs to think carefully about the meaning.
That's the point. If we do not understand the proper values and their
scales, perhaps that is the issue we should solve first?

> 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?

Physical meaning of the values, certainly.

> 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.

...

> >               *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.

Why? (Maybe you are right, maybe not)

...

> SI-defines that are a bit confusing to me.

Yeah, people hate mathematics at scholls, at university, in life...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Peter Rosin]

On 2022-01-31 16:23, Andy Shevchenko wrote:
> On Mon, Jan 31, 2022 at 4:50 PM Peter Rosin <peda@axentia.se> wrote:
>> On 2022-01-30 17:10, Liam Beguin wrote:
> 
> ...
> 
>>> -             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.
> 
> What do you mean? The idea behind is the use of the macro depending on
> the actual power of 10 in use (taking into account the sign of the
> exponent part).
> 
>> 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.
> 
> Does it have a physical meaning?

No, this is just a scaling factor which is moments later
eliminted by a matching inverse operation. It's math purely
about attempting to preserve precision and has nothing to do
with the units of the values that are involved.


>> 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.
> 
> Same question.

No, in the context of B) it's also just math without any physical
quantity in the back seat. The iio core prints fractional-log
*values* (of any and all units) with "nano precision" i.e. nine
decimal digits.

>> 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.
> 
> Huh?!

What I meant originally by "no correct define to use" is that
there are arguments for both GIGA and for NANO and that if both
are not wrong, then none of them is /the/ correct define to use.
Now that you put the focus on physical quantities, there are
other ascpects, see below.

> I believe the answer to the A and B -- yes, which means there are the
> correct and incorrect variants to use.

This doesn't really parse for me. What question is "yes" answering?
What is it that you think is correct and what is incorrect? I.e. what
is your conclusion? Do you think NANO or GIGA fits best? Why?

>> 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.
> 
> The (main) idea of the macros is to avoid mistyping 0s in the numbers
> and miscalculations. Besides that it also provides the same type for
> the constants.

Yes, but I wonder if it is worth the time to work out what the
correct defines to use actually are.

>> 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.
> 
> Sounds like we have a chat here between physicists and computer
> science folks :-)

I don't get what is supposed to be funny. Which would I be and
why? I of course saw the difference in exponents, what I don't
see is why dividing with 10^-9 makes the result NANO while
multiplying with 10^9 (presumably) makes the result GIGA. And
yes, sigh, I do see that you have divisions above and not any
multiplication, but what's confusing me is what happens when I
extend your example with the multiplications I added below. I
simply cannot make up clear rules with only numbers to go by.
I for one need the units to make the call. But we have units
neither here nor in the math the code is implementing, it's
only plain numbers (or numbers with unknown units, or ops where
the unit is of no consequence).

> Let's try again, does the value in the tmp variable above have a
> _physical_ meaning? (I believe so, because all IIO subsystem is about
> physical values)

See above.

>> 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).
> 
> Yes, and using proper macro makes much cleaner the mathematical and
> physical point of view to the values.

Only if you select the correct and relevant define. Otherwise they
make things random and confusing. You can't go hunt for a define
that happens to match the value you need and then argue it's a
good idea to use it based on that. And this borders to that,
because in 3 of 5 cases there are no units involved and the
defines are about unit prefixes. In the remaining two cases (that
you elided) the actual unit prefix involved (from micro-ohms) is
not considered and MEGA is used instead of MICRO.

>> 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.
> 
> Again, one needs to think carefully about the meaning.
> That's the point. If we do not understand the proper values and their
> scales, perhaps that is the issue we should solve first?

Oh, I have a good understanding of how this driver works and where
the numbers are coming from. I have no issues to solve in that
department. And I like to keep it that way, so I want to understand
which of GIGA and NANO is better than the other, and why. For each
instance.

>> 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?
> 
> Physical meaning of the values, certainly.

Not helpful. You seem to be under the impression that all 10^x
numbers are somehow used because there is a unit connected to some
other number. That's simply not always the case.

>> 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.
> 
> ...
> 
>>>               *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.
> 
> Why? (Maybe you are right, maybe not)

Right, that's arguably a mistake. Here, 1000000000 is simply the
number that's needed to adjust the 9 decimal digits for the
int-plus-nano return value, whatever unit that int-plus-nano
value has. So, there is only a match with the above from the
prespective of C).

But regardless, it would feel extremely odd to use one of the
GIGA/NANO defines above, and the other here.

> ...
> 
>> SI-defines that are a bit confusing to me.
> 
> Yeah, people hate mathematics at scholls, at university, in life...

You seem to imply something. What, exactly? Care to spell it out?

Cheers,
Peter

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Andy Shevchenko]

On Tue, Feb 1, 2022 at 3:59 AM Peter Rosin <peda@axentia.se> wrote:
> On 2022-01-31 16:23, Andy Shevchenko wrote:
> > On Mon, Jan 31, 2022 at 4:50 PM Peter Rosin <peda@axentia.se> wrote:
> >> On 2022-01-30 17:10, Liam Beguin wrote:
> >
> > ...
> >
> >>> -             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.
> >
> > What do you mean? The idea behind is the use of the macro depending on
> > the actual power of 10 in use (taking into account the sign of the
> > exponent part).
> >
> >> 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.
> >
> > Does it have a physical meaning?
>
> No, this is just a scaling factor which is moments later
> eliminted by a matching inverse operation. It's math purely
> about attempting to preserve precision and has nothing to do
> with the units of the values that are involved.

I see your point now, shame on me.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Liam Beguin]

Hi Peter,

On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote:
> 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:

I agree with you that the choice behind GIGA/NANO can be a bit
confusing.

In my opinion, these defines makes the code easier to read if you
consider them as multipliers with no physical meaning, basically a
pretty name for a power of 10.

By this logic, we wouldn't ever use FEMTO to DECI.

Cheers,
Liam

> 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;

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Jonathan Cameron]

On Tue, 1 Feb 2022 14:28:28 -0500
Liam Beguin <liambeguin@gmail.com> wrote:

> Hi Peter,
> 
> On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote:
> > 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:  
> 
> I agree with you that the choice behind GIGA/NANO can be a bit
> confusing.
> 
> In my opinion, these defines makes the code easier to read if you
> consider them as multipliers with no physical meaning, basically a
> pretty name for a power of 10.
> 
> By this logic, we wouldn't ever use FEMTO to DECI.

Not sure if it would help but maybe it's worth a local define
of something like

#define MULT9 1000000000LL
to loose the association with any particular SI basis and
just indicate it's a bit number being used to retain precision
in some maths?  Would need a comment to stop people sending
patches to replace it with GIGA though ;)

My ultimate preference here is for whatever works for Peter and
Liam as the people who are mostly likely to have to deal
with any changes to this driver in the future.

Jonathan


> 
> Cheers,
> Liam
> 
> > 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;  

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Andy Shevchenko]

On Sat, Feb 5, 2022 at 7:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 1 Feb 2022 14:28:28 -0500
> Liam Beguin <liambeguin@gmail.com> wrote:

...

> Not sure if it would help but maybe it's worth a local define
> of something like
>
> #define MULT9 1000000000LL
> to loose the association with any particular SI basis and
> just indicate it's a bit number being used to retain precision
> in some maths?  Would need a comment to stop people sending
> patches to replace it with GIGA though ;)
>
> My ultimate preference here is for whatever works for Peter and
> Liam as the people who are mostly likely to have to deal
> with any changes to this driver in the future.

SI multipliers are for values with the physical meaning. While Peter
found them confusing, it's a pretty much win in my opinion when we
talk about data from nature. For the pure mathematical scale it may be
confusing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Liam Beguin]

On Sat, Feb 05, 2022 at 05:54:04PM +0000, Jonathan Cameron wrote:
> On Tue, 1 Feb 2022 14:28:28 -0500
> Liam Beguin <liambeguin@gmail.com> wrote:
> 
> > Hi Peter,
> > 
> > On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote:
> > > 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:  
> > 
> > I agree with you that the choice behind GIGA/NANO can be a bit
> > confusing.
> > 
> > In my opinion, these defines makes the code easier to read if you
> > consider them as multipliers with no physical meaning, basically a
> > pretty name for a power of 10.
> > 
> > By this logic, we wouldn't ever use FEMTO to DECI.
> 
> Not sure if it would help but maybe it's worth a local define
> of something like
> 
> #define MULT9 1000000000LL
> to loose the association with any particular SI basis and
> just indicate it's a bit number being used to retain precision
> in some maths?  Would need a comment to stop people sending
> patches to replace it with GIGA though ;)
> 
> My ultimate preference here is for whatever works for Peter and
> Liam as the people who are mostly likely to have to deal
> with any changes to this driver in the future.

Hi Jonathan,

My preference here is to keep GIGA, if it makes everyone more
comfortable, I can add a comment explaing the intention of the
multiplication?

Cheers,
Liam

> Jonathan
> 
> 
> > 
> > Cheers,
> > Liam
> > 
> > > 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;  
> 

Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h

[Jonathan Cameron]

On Mon, 7 Feb 2022 09:05:11 -0500
Liam Beguin <liambeguin@gmail.com> wrote:

> On Sat, Feb 05, 2022 at 05:54:04PM +0000, Jonathan Cameron wrote:
> > On Tue, 1 Feb 2022 14:28:28 -0500
> > Liam Beguin <liambeguin@gmail.com> wrote:
> >   
> > > Hi Peter,
> > > 
> > > On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote:  
> > > > 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:    
> > > 
> > > I agree with you that the choice behind GIGA/NANO can be a bit
> > > confusing.
> > > 
> > > In my opinion, these defines makes the code easier to read if you
> > > consider them as multipliers with no physical meaning, basically a
> > > pretty name for a power of 10.
> > > 
> > > By this logic, we wouldn't ever use FEMTO to DECI.  
> > 
> > Not sure if it would help but maybe it's worth a local define
> > of something like
> > 
> > #define MULT9 1000000000LL
> > to loose the association with any particular SI basis and
> > just indicate it's a bit number being used to retain precision
> > in some maths?  Would need a comment to stop people sending
> > patches to replace it with GIGA though ;)
> > 
> > My ultimate preference here is for whatever works for Peter and
> > Liam as the people who are mostly likely to have to deal
> > with any changes to this driver in the future.  
> 
> Hi Jonathan,
> 
> My preference here is to keep GIGA, if it makes everyone more
> comfortable, I can add a comment explaing the intention of the
> multiplication?
> 
Works for me.

J

> Cheers,
> Liam
> 
> > Jonathan
> > 
> >   
> > > 
> > > Cheers,
> > > Liam
> > >   
> > > > 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;    
> >   

[PATCH v13 07/11] iio: test: add basic tests for the iio-rescale driver

[Liam Beguin]

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 <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/test/Kconfig            |  10 +
 drivers/iio/test/Makefile           |   1 +
 drivers/iio/test/iio-test-rescale.c | 711 ++++++++++++++++++++++++++++
 3 files changed, 722 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 467519a2027e..f15ae0a6394f 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -4,5 +4,6 @@
 #
 
 # 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)
diff --git a/drivers/iio/test/iio-test-rescale.c b/drivers/iio/test/iio-test-rescale.c
new file mode 100644
index 000000000000..a3bbe2db76d3
--- /dev/null
+++ b/drivers/iio/test/iio-test-rescale.c
@@ -0,0 +1,711 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kunit tests for IIO rescale conversions
+ *
+ * Copyright (c) 2021 Liam Beguin <liambeguin@gmail.com>
+ */
+
+#include <linux/gcd.h>
+#include <linux/overflow.h>
+#include <linux/units.h>
+
+#include <linux/iio/afe/rescale.h>
+#include <linux/iio/iio.h>
+
+#include <kunit/test.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, 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,
+		.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, 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,
+		.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_nano() - Parse a fixed-point string to get an
+ *                      IIO_VAL_INT_PLUS_NANO value
+ * @str: The string to parse
+ * @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_nano(const char *str, s64 *nano)
+{
+	int tmp, tmp2;
+	int ret = 0;
+
+	/*
+	 * iio_str_to_fixpoint() uses 10^8 here instead of 10^9 as fract_mult is
+	 * the multiplier for the first decimal place.
+	 */
+	ret = iio_str_to_fixpoint(str, 100000000, &tmp, &tmp2);
+	if (ret < 0)
+		return ret;
+
+	if (tmp < 0)
+		tmp2 *= -1;
+
+	*nano = (s64)tmp * GIGA + tmp2;
+
+	return ret;
+}
+
+/**
+ * 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 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_nano(real_str, &real);
+	if (ret < 0)
+		return ret;
+
+	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 = MEGA * abs(exp - real);
+
+	return (int)div64_u64(err, abs(exp));
+}
+
+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_EQ_MSG(test, rel_ppm, 0,
+			    "\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.35.1.4.g5d01301f2b86

[PATCH v13 08/11] iio: afe: rescale: add RTD temperature sensor support

[Liam Beguin]

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>
---
 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 27c6664915ff..ae71a545c7e0 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -395,10 +395,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 / MEGA;
+	factor = gcd(tmp, MEGA);
+	rescale->numerator = MEGA / factor;
+	rescale->denominator = tmp / factor;
+
+	rescale->offset = -1 * ((r0 * iexc) / MEGA * MILLI);
+
+	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);
-- 
2.35.1.4.g5d01301f2b86

Re: [PATCH v13 08/11] iio: afe: rescale: add RTD temperature sensor support

[Peter Rosin]

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>
> ---
>  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 27c6664915ff..ae71a545c7e0 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -395,10 +395,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 / 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?

These macros have wasted quite a bit of review time. I'm not
fully convinced they represent an improvement...

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);

Re: [PATCH v13 08/11] iio: afe: rescale: add RTD temperature sensor support

[Liam Beguin]

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);

[PATCH v13 09/11] iio: afe: rescale: add temperature transducers

[Liam Beguin]

A temperature transducer is a device that converts a thermal quantity
into any other physical quantity. This patch adds 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 <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/iio-rescale.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index ae71a545c7e0..706d2d224777 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -436,11 +436,37 @@ 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;
+	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 = MEGA;
+	rescale->denominator = alpha * sense;
+
+	rescale->offset = div_s64((s64)offset * rescale->denominator,
+				  rescale->numerator);
+
+	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[] = {
@@ -460,6 +486,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[] = {
@@ -471,6 +501,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.35.1.4.g5d01301f2b86

[PATCH v13 10/11] dt-bindings: iio: afe: add bindings for temperature-sense-rtd

[Liam Beguin]

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 <liambeguin@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 .../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 <liambeguin@gmail.com>
+
+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.35.1.4.g5d01301f2b86

[PATCH v13 11/11] dt-bindings: iio: afe: add bindings for temperature transducers

[Liam Beguin]

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 <liambeguin@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 .../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 <liambeguin@gmail.com>
+
+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.35.1.4.g5d01301f2b86

Re: [PATCH v13 00/11] iio: afe: add temperature rescaling support

[Andy Shevchenko]

On Sun, Jan 30, 2022 at 6:11 PM Liam Beguin <liambeguin@gmail.com> wrote:
>
> 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.

LGTM (with the potential room to clean up in the future, but let's
digest this first)
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> 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
> --
> 2.35.1.4.g5d01301f2b86
>


-- 
With Best Regards,
Andy Shevchenko