linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 00/13] iio: afe: add temperature rescaling support
@ 2021-07-21  3:06 Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 01/13] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
                   ` (12 more replies)
  0 siblings, 13 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

Add temperature rescaling support to the IIO Analog Front End driver.

This series includes minor bug fixes and adds support for RTD temperature
sensors as well as temperature transducers.

At first I tried to use iio_convert_raw_to_processed() to get more
precision out of processed values but ran into issues when one of my
ADCs didn't provide a scale. I tried to address this in the first few
patches.

When adding offset support to iio-rescale, I also noticed that
iio_read_channel_processed() assumes that the offset is always an
integer which I tried to address in the third patch without breaking
valid implicit truncations.

As Jonathan suggested, I also started a Kunit test suite for the
iio-rescale driver.
Adding these test cases, I found that the IIO_VAL_FRACTIONAL_LOG2
rescaling wasn't as precise as expected so I tried to fix that as well.

Changes since v5:
- add include/linux/iio/afe/rescale.h
- expose functions use to process scale and offset
- add basic iio-rescale kunit test cases
- fix integer overflow case
- improve precision for IIO_VAL_FRACTIONAL_LOG2

Changes since v4:
- only use gcd() when necessary in overflow mitigation
- fix INT_PLUS_{MICRO,NANO} support
- apply Reviewed-by
- fix temperature-transducer bindings

Changes since v3:
- drop unnecessary fallthrough statements
- drop redundant local variables in some calculations
- fix s64 divisions on 32bit platforms by using do_div
- add comment describing iio-rescaler offset calculation
- drop unnecessary MAINTAINERS entry

Changes since v2:
- don't break implicit offset truncations
- make a best effort to get a valid value for fractional types
- drop return value change in iio_convert_raw_to_processed_unlocked()
- don't rely on processed value for offset calculation
- add INT_PLUS_{MICRO,NANO} support in iio-rescale
- revert generic implementation in favor of temperature-sense-rtd and
  temperature-transducer
- add separate section to MAINTAINERS file

Changes since v1:
- rebase on latest iio `testing` branch
- also apply consumer scale on integer channel scale types
- don't break implicit truncation in processed channel offset
  calculation
- drop temperature AFE flavors in favor of a simpler generic
  implementation

Thanks for your time

Liam Beguin (13):
  iio: inkern: apply consumer scale on IIO_VAL_INT cases
  iio: inkern: apply consumer scale when no channel scale is available
  iio: inkern: make a best effort on offset calculation
  iio: afe: rescale: expose scale processing function
  iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  iio: afe: rescale: add offset support
  iio: test: add basic tests for the iio-rescale driver
  iio: afe: rescale: reduce risk of integer overflow
  iio: afe: rescale: fix precision on fractional log scale
  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                 | 242 +++++++++++--
 drivers/iio/inkern.c                          |  40 +-
 drivers/iio/test/Kconfig                      |  10 +
 drivers/iio/test/Makefile                     |   1 +
 drivers/iio/test/iio-test-rescale.c           | 342 ++++++++++++++++++
 include/linux/iio/afe/rescale.h               |  34 ++
 8 files changed, 840 insertions(+), 44 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
 create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
 create mode 100644 drivers/iio/test/iio-test-rescale.c
 create mode 100644 include/linux/iio/afe/rescale.h

Range-diff against v5:
 1:  7b3e374eb7ad <  -:  ------------ iio: afe: rescale: reduce risk of integer overflow
 2:  1d334090e974 <  -:  ------------ iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
 -:  ------------ >  1:  42a7a1047edc iio: inkern: apply consumer scale on IIO_VAL_INT cases
 -:  ------------ >  2:  a1cd89fdad11 iio: inkern: apply consumer scale when no channel scale is available
 -:  ------------ >  3:  ed0721fb6bd1 iio: inkern: make a best effort on offset calculation
 -:  ------------ >  4:  f8fb78bb1112 iio: afe: rescale: expose scale processing function
 -:  ------------ >  5:  16627ca44022 iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
 3:  61873203c140 !  6:  dec2a933fbf3 iio: afe: rescale: add offset support
    @@ Commit message
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
     
      ## drivers/iio/afe/iio-rescale.c ##
    -@@ drivers/iio/afe/iio-rescale.c: struct rescale {
    - 	bool chan_processed;
    - 	s32 numerator;
    - 	s32 denominator;
    -+	s32 offset;
    - };
    +@@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale, int scale_type,
    + 	}
    + }
      
    ++int rescale_process_offset(struct rescale *rescale, int scale_type,
    ++			   int scale, int scale2, int schan_off,
    ++			   int *val, int *val2)
    ++{
    ++	s64 tmp, tmp2;
    ++
    ++	switch (scale_type) {
    ++	case IIO_VAL_FRACTIONAL:
    ++		tmp = (s64)rescale->offset * scale2;
    ++		*val = div_s64(tmp, scale) + schan_off;
    ++		return IIO_VAL_INT;
    ++	case IIO_VAL_INT:
    ++		*val = div_s64(rescale->offset, scale) + schan_off;
    ++		return IIO_VAL_INT;
    ++	case IIO_VAL_FRACTIONAL_LOG2:
    ++		tmp = (s64)rescale->offset * (1 << scale2);
    ++		*val = div_s64(tmp, scale) + schan_off;
    ++		return IIO_VAL_INT;
    ++	case IIO_VAL_INT_PLUS_NANO:
    ++		tmp = (s64)rescale->offset * 1000000000;
    ++		tmp2 = ((s64)scale * 1000000000L) + scale2;
    ++		*val = div64_s64(tmp, tmp2) + schan_off;
    ++		return IIO_VAL_INT;
    ++	case IIO_VAL_INT_PLUS_MICRO:
    ++		tmp = (s64)rescale->offset * 1000000L;
    ++		tmp2 = ((s64)scale * 1000000L) + scale2;
    ++		*val = div64_s64(tmp, tmp2) + schan_off;
    ++		return IIO_VAL_INT;
    ++	default:
    ++		return -EOPNOTSUPP;
    ++	}
    ++}
    ++
      static int rescale_read_raw(struct iio_dev *indio_dev,
    -@@ drivers/iio/afe/iio-rescale.c: 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;
    - 	s64 tmp, tmp2;
    - 	u32 factor;
      	int ret;
    + 
    + 	switch (mask) {
     @@ drivers/iio/afe/iio-rescale.c: static int rescale_read_raw(struct iio_dev *indio_dev,
    - 			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
    - 			return -EOPNOTSUPP;
    + 			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
    @@ drivers/iio/afe/iio-rescale.c: static int rescale_read_raw(struct iio_dev *indio
     +		}
     +
     +		ret = iio_read_channel_scale(rescale->source, &scale, &scale2);
    -+		switch (ret) {
    -+		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 * 1000000000UL;
    -+			tmp2 = ((s64)scale * 1000000000UL) + scale2;
    -+			*val = div_s64(tmp, tmp2) + schan_off;
    -+			return IIO_VAL_INT;
    -+		case IIO_VAL_INT_PLUS_MICRO:
    -+			tmp = (s64)rescale->offset * 1000000UL;
    -+			tmp2 = ((s64)scale * 1000000UL) + scale2;
    -+			*val = div_s64(tmp, tmp2) + schan_off;
    -+			return IIO_VAL_INT;
    -+		default:
    -+			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
    -+			return -EOPNOTSUPP;
    -+		}
    ++		return rescale_process_offset(rescale, ret, scale, scale2,
    ++					      schan_off, val, val2);
      	default:
      		return -EINVAL;
      	}
    @@ drivers/iio/afe/iio-rescale.c: static int rescale_configure_channel(struct devic
      		BIT(IIO_CHAN_INFO_SCALE);
      
     +	if (rescale->offset)
    -+	    chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
    ++		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
     +
      	/*
      	 * Using .read_avail() is fringe to begin with and makes no sense
    @@ drivers/iio/afe/iio-rescale.c: static int rescale_probe(struct platform_device *
      
      	ret = rescale->cfg->props(dev, rescale);
      	if (ret)
    +
    + ## include/linux/iio/afe/rescale.h ##
    +@@ include/linux/iio/afe/rescale.h: 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__ */
 -:  ------------ >  7:  60bd09555300 iio: test: add basic tests for the iio-rescale driver
 -:  ------------ >  8:  f2f4af324d5d iio: afe: rescale: reduce risk of integer overflow
 -:  ------------ >  9:  4a8d77501f32 iio: afe: rescale: fix precision on fractional log scale
 4:  4e6117b9c663 = 10:  2b6e4f47c974 iio: afe: rescale: add RTD temperature sensor support
 5:  bc647d45e293 = 11:  715e1877e30f iio: afe: rescale: add temperature transducers
 6:  570b418eed85 ! 12:  0bc8546f13e5 dt-bindings: iio: afe: add bindings for temperature-sense-rtd
    @@ Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml (new)
     +title: Temperature Sense RTD
     +
     +maintainers:
    -+  - Liam Beguin <lvb@xiphos.com>
    ++  - Liam Beguin <liambeguin@gmail.com>
     +
     +description: |
     +  RTDs (Resistance Temperature Detectors) are a kind of temperature sensors
 7:  3c44ea89754e ! 13:  4bc54afa3e94 dt-bindings: iio: afe: add bindings for temperature transducers
    @@ Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml (new)
     +title: Temperature Transducer
     +
     +maintainers:
    -+  - Liam Beguin <lvb@xiphos.com>
    ++  - Liam Beguin <liambeguin@gmail.com>
     +
     +description: |
     +  A temperature transducer is a device that converts a thermal quantity

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.30.1.489.g328c10930387


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v6 01/13] iio: inkern: apply consumer scale on IIO_VAL_INT cases
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 02/13] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

When a consumer calls iio_read_channel_processed() and the channel has
an integer scale, the scale channel scale is applied and the processed
value is returned as expected.

On the other hand, if the consumer calls iio_convert_raw_to_processed()
the scaling factor requested by the consumer is not applied.

This for example causes the consumer to process mV when expecting uV.
Make sure to always apply the scaling factor requested by the consumer.

Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/inkern.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 391a3380a1d1..b752fe5818e7 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -599,7 +599,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 
 	switch (scale_type) {
 	case IIO_VAL_INT:
-		*processed = raw64 * scale_val;
+		*processed = raw64 * scale_val * scale;
 		break;
 	case IIO_VAL_INT_PLUS_MICRO:
 		if (scale_val2 < 0)
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 02/13] iio: inkern: apply consumer scale when no channel scale is available
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 01/13] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 03/13] iio: inkern: make a best effort on offset calculation Liam Beguin
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

When a consumer calls iio_read_channel_processed() and no channel scale
is available, it's assumed that the scale is one and the raw value is
returned as expected.

On the other hand, if the consumer calls iio_convert_raw_to_processed()
the scaling factor requested by the consumer is not applied.

This for example causes the consumer to process mV when expecting uV.
Make sure to always apply the scaling factor requested by the consumer.

Fixes: adc8ec5ff183 ("iio: inkern: pass through raw values if no scaling")
Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/inkern.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b752fe5818e7..b69027690ed5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -590,10 +590,10 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 					IIO_CHAN_INFO_SCALE);
 	if (scale_type < 0) {
 		/*
-		 * Just pass raw values as processed if no scaling is
-		 * available.
+		 * If no channel scaling is available apply consumer scale to
+		 * raw value and return.
 		 */
-		*processed = raw;
+		*processed = raw * scale;
 		return 0;
 	}
 
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 03/13] iio: inkern: make a best effort on offset calculation
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 01/13] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 02/13] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 04/13] iio: afe: rescale: expose scale processing function Liam Beguin
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

iio_convert_raw_to_processed_unlocked() assumes the offset is an
integer. Make a best effort to get a valid offset value for fractional
cases without breaking implicit truncations.

Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/inkern.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b69027690ed5..5e74d8983874 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -578,13 +578,35 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
 static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 	int raw, int *processed, unsigned int scale)
 {
-	int scale_type, scale_val, scale_val2, offset;
+	int scale_type, scale_val, scale_val2;
+	int offset_type, offset_val, offset_val2;
 	s64 raw64 = raw;
-	int ret;
 
-	ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET);
-	if (ret >= 0)
-		raw64 += offset;
+	offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
+				       IIO_CHAN_INFO_OFFSET);
+	if (offset_type >= 0) {
+		switch (offset_type) {
+		case IIO_VAL_INT:
+			break;
+		case IIO_VAL_INT_PLUS_MICRO:
+		case IIO_VAL_INT_PLUS_NANO:
+			/*
+			 * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO
+			 * implicitely truncate the offset to it's integer form.
+			 */
+			break;
+		case IIO_VAL_FRACTIONAL:
+			offset_val /= offset_val2;
+			break;
+		case IIO_VAL_FRACTIONAL_LOG2:
+			offset_val /= (1 << offset_val2);
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		raw64 += offset_val;
+	}
 
 	scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
 					IIO_CHAN_INFO_SCALE);
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 04/13] iio: afe: rescale: expose scale processing function
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (2 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 03/13] iio: inkern: make a best effort on offset calculation Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c   | 65 ++++++++++++++-------------------
 include/linux/iio/afe/rescale.h | 30 +++++++++++++++
 2 files changed, 58 insertions(+), 37 deletions(-)
 create mode 100644 include/linux/iio/afe/rescale.h

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 774eb3044edd..d0669fd8eac5 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -11,35 +11,46 @@
 #include <linux/gcd.h>
 #include <linux/iio/consumer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/afe/rescale.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 
-struct rescale;
-
-struct rescale_cfg {
-	enum iio_chan_type type;
-	int (*props)(struct device *dev, struct rescale *rescale);
-};
+int rescale_process_scale(struct rescale *rescale, int scale_type,
+			  int *val, int *val2)
+{
+	unsigned long long tmp;
 
-struct rescale {
-	const struct rescale_cfg *cfg;
-	struct iio_channel *source;
-	struct iio_chan_spec chan;
-	struct iio_chan_spec_ext_info *ext_info;
-	bool chan_processed;
-	s32 numerator;
-	s32 denominator;
-};
+	switch (scale_type) {
+	case IIO_VAL_FRACTIONAL:
+		*val *= rescale->numerator;
+		*val2 *= rescale->denominator;
+		return scale_type;
+	case IIO_VAL_INT:
+		*val *= rescale->numerator;
+		if (rescale->denominator == 1)
+			return scale_type;
+		*val2 = rescale->denominator;
+		return IIO_VAL_FRACTIONAL;
+	case IIO_VAL_FRACTIONAL_LOG2:
+		tmp = *val * 1000000000LL;
+		do_div(tmp, rescale->denominator);
+		tmp *= rescale->numerator;
+		do_div(tmp, 1000000000LL);
+		*val = tmp;
+		return scale_type;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
 
 static int rescale_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
 {
 	struct rescale *rescale = iio_priv(indio_dev);
-	unsigned long long tmp;
 	int ret;
 
 	switch (mask) {
@@ -65,27 +76,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 		} else {
 			ret = iio_read_channel_scale(rescale->source, val, val2);
 		}
-		switch (ret) {
-		case IIO_VAL_FRACTIONAL:
-			*val *= rescale->numerator;
-			*val2 *= rescale->denominator;
-			return ret;
-		case IIO_VAL_INT:
-			*val *= rescale->numerator;
-			if (rescale->denominator == 1)
-				return ret;
-			*val2 = rescale->denominator;
-			return IIO_VAL_FRACTIONAL;
-		case IIO_VAL_FRACTIONAL_LOG2:
-			tmp = *val * 1000000000LL;
-			do_div(tmp, rescale->denominator);
-			tmp *= rescale->numerator;
-			do_div(tmp, 1000000000LL);
-			*val = tmp;
-			return ret;
-		default:
-			return -EOPNOTSUPP;
-		}
+		return rescale_process_scale(rescale, ret, val, val2);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
new file mode 100644
index 000000000000..14d4ee1227c6
--- /dev/null
+++ b/include/linux/iio/afe/rescale.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 Liam Beguin <liambeguin@gmail.com>
+ */
+
+#ifndef __IIO_RESCALE_H__
+#define __IIO_RESCALE_H__
+
+#include <linux/iio/iio.h>
+
+struct rescale;
+
+struct rescale_cfg {
+	enum iio_chan_type type;
+	int (*props)(struct device *dev, struct rescale *rescale);
+};
+
+struct rescale {
+	const struct rescale_cfg *cfg;
+	struct iio_channel *source;
+	struct iio_chan_spec chan;
+	struct iio_chan_spec_ext_info *ext_info;
+	bool chan_processed;
+	s32 numerator;
+	s32 denominator;
+};
+
+int rescale_process_scale(struct rescale *rescale, int scale_type,
+			  int *val, int *val2);
+#endif /* __IIO_RESCALE_H__ */
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (3 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 04/13] iio: afe: rescale: expose scale processing function Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-23 21:16   ` Peter Rosin
  2021-07-21  3:06 ` [PATCH v6 06/13] iio: afe: rescale: add offset support Liam Beguin
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index d0669fd8eac5..2b73047365cc 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		do_div(tmp, 1000000000LL);
 		*val = tmp;
 		return scale_type;
+	case IIO_VAL_INT_PLUS_NANO:
+		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
+		tmp = div_s64(tmp, rescale->denominator);
+
+		*val = div_s64(tmp, 1000000000LL);
+		*val2 = tmp - *val * 1000000000LL;
+		return scale_type;
+	case IIO_VAL_INT_PLUS_MICRO:
+		tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
+		tmp = div_s64(tmp, rescale->denominator);
+
+		*val = div_s64(tmp, 1000000);
+		*val2 = tmp - *val * 1000000;
+		return scale_type;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 06/13] iio: afe: rescale: add offset support
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (4 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 07/13] iio: test: add basic tests for the iio-rescale driver Liam Beguin
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c   | 80 +++++++++++++++++++++++++++++++++
 include/linux/iio/afe/rescale.h |  4 ++
 2 files changed, 84 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 2b73047365cc..6f6a711ae3ae 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -60,11 +60,46 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 	}
 }
 
+int rescale_process_offset(struct rescale *rescale, int scale_type,
+			   int scale, int scale2, int schan_off,
+			   int *val, int *val2)
+{
+	s64 tmp, tmp2;
+
+	switch (scale_type) {
+	case IIO_VAL_FRACTIONAL:
+		tmp = (s64)rescale->offset * scale2;
+		*val = div_s64(tmp, scale) + schan_off;
+		return IIO_VAL_INT;
+	case IIO_VAL_INT:
+		*val = div_s64(rescale->offset, scale) + schan_off;
+		return IIO_VAL_INT;
+	case IIO_VAL_FRACTIONAL_LOG2:
+		tmp = (s64)rescale->offset * (1 << scale2);
+		*val = div_s64(tmp, scale) + schan_off;
+		return IIO_VAL_INT;
+	case IIO_VAL_INT_PLUS_NANO:
+		tmp = (s64)rescale->offset * 1000000000;
+		tmp2 = ((s64)scale * 1000000000L) + scale2;
+		*val = div64_s64(tmp, tmp2) + schan_off;
+		return IIO_VAL_INT;
+	case IIO_VAL_INT_PLUS_MICRO:
+		tmp = (s64)rescale->offset * 1000000L;
+		tmp2 = ((s64)scale * 1000000L) + scale2;
+		*val = div64_s64(tmp, tmp2) + schan_off;
+		return IIO_VAL_INT;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int rescale_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
 {
 	struct rescale *rescale = iio_priv(indio_dev);
+	int scale, scale2;
+	int schan_off = 0;
 	int ret;
 
 	switch (mask) {
@@ -91,6 +126,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;
 	}
@@ -167,6 +243,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
@@ -331,6 +410,7 @@ static int rescale_probe(struct platform_device *pdev)
 	rescale->cfg = of_device_get_match_data(dev);
 	rescale->numerator = 1;
 	rescale->denominator = 1;
+	rescale->offset = 0;
 
 	ret = rescale->cfg->props(dev, rescale);
 	if (ret)
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
index 14d4ee1227c6..b152ac487403 100644
--- a/include/linux/iio/afe/rescale.h
+++ b/include/linux/iio/afe/rescale.h
@@ -23,8 +23,12 @@ struct rescale {
 	bool chan_processed;
 	s32 numerator;
 	s32 denominator;
+	s32 offset;
 };
 
 int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2);
+int rescale_process_offset(struct rescale *rescale, int scale_type,
+			   int scale, int scale2, int schan_off,
+			   int *val, int *val2);
 #endif /* __IIO_RESCALE_H__ */
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 07/13] iio: test: add basic tests for the iio-rescale driver
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (5 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 06/13] iio: afe: rescale: add offset support Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-24  8:40   ` kernel test robot
  2021-07-21  3:06 ` [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
---
 drivers/iio/test/Kconfig            |  10 +
 drivers/iio/test/Makefile           |   1 +
 drivers/iio/test/iio-test-rescale.c | 342 ++++++++++++++++++++++++++++
 3 files changed, 353 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..4db74a4d7146 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=y
+	default KUNIT_ALL_TESTS
+	help
+	  If you want to run tests on the iio-rescale code say Y here.
+
+	  This takes advantage of ARCH=um to run tests and should be used by
+	  developers to tests their changes to the rescaling logic.
+
 config IIO_TEST_FORMAT
         bool "Test IIO formatting functions"
         depends on KUNIT=y
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile
index f1099b495301..908963ca0b2f 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -4,4 +4,5 @@
 #
 
 # Keep in alphabetical order
+obj-$(CONFIG_IIO_RESCALE_KUNIT_TEST) += iio-test-rescale.o ../afe/iio-rescale.o
 obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o
diff --git a/drivers/iio/test/iio-test-rescale.c b/drivers/iio/test/iio-test-rescale.c
new file mode 100644
index 000000000000..ac1aee310ccd
--- /dev/null
+++ b/drivers/iio/test/iio-test-rescale.c
@@ -0,0 +1,342 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kunit tests for IIO rescale conversions
+ *
+ * Copyright (c) 2021 Liam Beguin <liambeguin@gmail.com>
+ */
+
+#include <kunit/test.h>
+#include <linux/gcd.h>
+#include <linux/iio/afe/rescale.h>
+#include <linux/iio/iio.h>
+#include <linux/overflow.h>
+
+struct rescale_tc_data {
+	const char *name;
+
+	const s32 numerator;
+	const s32 denominator;
+	const s32 offset;
+
+	const int schan_val;
+	const int schan_val2;
+	const int schan_off;
+	const int schan_scale_type;
+
+	const char *expected;
+	const char *expected_off;
+};
+
+const struct rescale_tc_data scale_cases[] = {
+	/*
+	 * Typical use cases
+	 */
+	{
+		.name = "typical IIO_VAL_INT, positive",
+		.numerator = 1000000,
+		.denominator = 8060,
+		.schan_scale_type = IIO_VAL_INT,
+		.schan_val = 42,
+		.expected = "5210.918114143\n",
+	},
+	{
+		.name = "typical IIO_VAL_INT, negative",
+		.numerator = -1000000,
+		.denominator = 8060,
+		.schan_scale_type = IIO_VAL_INT,
+		.schan_val = 42,
+		.expected = "-5210.918114143\n",
+	},
+	{
+		.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\n",
+	},
+	{
+		.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\n",
+	},
+	{
+		.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\n",
+	},
+	{
+		.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\n",
+	},
+	{
+		.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 = 123456789,
+		.expected = "1256.012008560\n",
+	},
+	{
+		.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 = 123456789,
+		.expected = "-1256.012008560\n",
+	},
+	{
+		.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 = 123456789,
+		.expected = "16557.914267\n",
+	},
+	{
+		.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 = 123456789,
+		.expected = "-16557.914267\n",
+	},
+	/*
+	 * 32-bit overflow conditions
+	 */
+	{
+		.name = "overflow IIO_VAL_FRACTIONAL, positive",
+		.numerator = 2,
+		.denominator = 20,
+		.schan_scale_type = IIO_VAL_FRACTIONAL,
+		.schan_val = 0x7FFFFFFF,
+		.schan_val2 = 1,
+		.expected = "214748364.700000000\n",
+	},
+	{
+		.name = "overflow IIO_VAL_FRACTIONAL, negative",
+		.numerator = -2,
+		.denominator = 20,
+		.schan_scale_type = IIO_VAL_FRACTIONAL,
+		.schan_val = 0x7FFFFFFF,
+		.schan_val2 = 1,
+		.expected = "-214748364.700000000\n",
+	},
+	{
+		.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 = 0x7fffffff,
+		.expected = "1.214748364\n",
+	},
+	{
+		.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 = 0x7fffffff,
+		.expected = "-1.214748364\n",
+	},
+	{
+		.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 = 0x7fffffff,
+		.expected = "215.748364\n",
+	},
+	{
+		.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 = 0x7fffffff,
+		.expected = "-215.748364\n",
+	},
+};
+
+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\n", /* 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\n", /* -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\n", /* 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\n", /* -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\n", /* 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\n", /* -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\n", /* 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\n", /* -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\n", /* 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\n", /* -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);
+
+static void iio_rescale_test_scale(struct kunit *test)
+{
+	struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
+	char *buf = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+	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_scale(&rescale, t->schan_scale_type,
+				&values[0], &values[1]);
+
+	ret = iio_format_value(buf, ret, 2, values);
+
+	KUNIT_EXPECT_EQ(test, (int)strlen(buf), ret);
+	KUNIT_EXPECT_STREQ(test, buf, t->expected);
+}
+
+static void iio_rescale_test_offset(struct kunit *test)
+{
+	struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
+	char *buf = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+	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(buf, ret, 2, values);
+
+	KUNIT_EXPECT_EQ(test, (int)strlen(buf), ret);
+	KUNIT_EXPECT_STREQ(test, buf, 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.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (6 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 07/13] iio: test: add basic tests for the iio-rescale driver Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-23 21:17   ` Peter Rosin
  2021-07-21  3:06 ` [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale Liam Beguin
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

Reduce the risk of integer overflow by doing the scale calculation with
64bit integers and looking for a Greatest Common Divider for both parts
of the fractional value when required.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 6f6a711ae3ae..35fa3b4e53e0 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -21,12 +21,21 @@
 int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
-	unsigned long long tmp;
+	s64 tmp, tmp2;
+	u32 factor;
 
 	switch (scale_type) {
 	case IIO_VAL_FRACTIONAL:
-		*val *= rescale->numerator;
-		*val2 *= rescale->denominator;
+		if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
+		    check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
+			tmp = (s64)*val * rescale->numerator;
+			tmp2 = (s64)*val2 * rescale->denominator;
+			factor = gcd(tmp, tmp2);
+			tmp = div_s64(tmp, factor);
+			tmp2 = div_s64(tmp2, factor);
+		}
+		*val = tmp;
+		*val2 = tmp2;
 		return scale_type;
 	case IIO_VAL_INT:
 		*val *= rescale->numerator;
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (7 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-23 21:20   ` Peter Rosin
  2021-07-21  3:06 ` [PATCH v6 10/13] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected
scale. Update the case so that the rescaler returns a fractional type
and a more precise scale.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 35fa3b4e53e0..47cd4a6d9aca 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		*val2 = rescale->denominator;
 		return IIO_VAL_FRACTIONAL;
 	case IIO_VAL_FRACTIONAL_LOG2:
-		tmp = *val * 1000000000LL;
-		do_div(tmp, rescale->denominator);
-		tmp *= rescale->numerator;
-		do_div(tmp, 1000000000LL);
-		*val = tmp;
-		return scale_type;
+		*val = rescale->numerator * *val;
+		*val2 = rescale->denominator * (1 << *val2);
+		return IIO_VAL_FRACTIONAL;
 	case IIO_VAL_INT_PLUS_NANO:
 		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
 		tmp = div_s64(tmp, rescale->denominator);
-- 
2.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 10/13] iio: afe: rescale: add RTD temperature sensor support
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (8 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 11/13] iio: afe: rescale: add temperature transducers Liam Beguin
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
---
 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 47cd4a6d9aca..6a1ecaad2da4 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -351,10 +351,52 @@ static int rescale_voltage_divider_props(struct device *dev,
 	return 0;
 }
 
+static int rescale_temp_sense_rtd_props(struct device *dev,
+					struct rescale *rescale)
+{
+	u32 factor;
+	u32 alpha;
+	u32 iexc;
+	u32 tmp;
+	int ret;
+	u32 r0;
+
+	ret = device_property_read_u32(dev, "excitation-current-microamp",
+				       &iexc);
+	if (ret) {
+		dev_err(dev, "failed to read excitation-current-microamp: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "alpha-ppm-per-celsius", &alpha);
+	if (ret) {
+		dev_err(dev, "failed to read alpha-ppm-per-celsius: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "r-naught-ohms", &r0);
+	if (ret) {
+		dev_err(dev, "failed to read r-naught-ohms: %d\n", ret);
+		return ret;
+	}
+
+	tmp = r0 * iexc * alpha / 1000000;
+	factor = gcd(tmp, 1000000);
+	rescale->numerator = 1000000 / factor;
+	rescale->denominator = tmp / factor;
+
+	rescale->offset = -1 * ((r0 * iexc) / 1000);
+
+	return 0;
+}
+
 enum rescale_variant {
 	CURRENT_SENSE_AMPLIFIER,
 	CURRENT_SENSE_SHUNT,
 	VOLTAGE_DIVIDER,
+	TEMP_SENSE_RTD,
 };
 
 static const struct rescale_cfg rescale_cfg[] = {
@@ -370,6 +412,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[] = {
@@ -379,6 +425,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.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 11/13] iio: afe: rescale: add temperature transducers
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (9 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 10/13] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 12/13] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 13/13] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/afe/iio-rescale.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 6a1ecaad2da4..3abeaded6169 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -392,11 +392,38 @@ static int rescale_temp_sense_rtd_props(struct device *dev,
 	return 0;
 }
 
+static int rescale_temp_transducer_props(struct device *dev,
+					 struct rescale *rescale)
+{
+	s32 offset = 0;
+	s32 sense = 1;
+	s32 alpha;
+	s64 tmp;
+	int ret;
+
+	device_property_read_u32(dev, "sense-offset-millicelsius", &offset);
+	device_property_read_u32(dev, "sense-resistor-ohms", &sense);
+	ret = device_property_read_u32(dev, "alpha-ppm-per-celsius", &alpha);
+	if (ret) {
+		dev_err(dev, "failed to read alpha-ppm-per-celsius: %d\n", ret);
+		return ret;
+	}
+
+	rescale->numerator = 1000000;
+	rescale->denominator = alpha * sense;
+
+	tmp = (s64)offset * (s64)alpha * (s64)sense;
+	rescale->offset = div_s64(tmp, (s32)1000000);
+
+	return 0;
+}
+
 enum rescale_variant {
 	CURRENT_SENSE_AMPLIFIER,
 	CURRENT_SENSE_SHUNT,
 	VOLTAGE_DIVIDER,
 	TEMP_SENSE_RTD,
+	TEMP_TRANSDUCER,
 };
 
 static const struct rescale_cfg rescale_cfg[] = {
@@ -416,6 +443,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[] = {
@@ -427,6 +458,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.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 12/13] dt-bindings: iio: afe: add bindings for temperature-sense-rtd
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (10 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 11/13] iio: afe: rescale: add temperature transducers Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-21  3:06 ` [PATCH v6 13/13] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
  12 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v6 13/13] dt-bindings: iio: afe: add bindings for temperature transducers
  2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
                   ` (11 preceding siblings ...)
  2021-07-21  3:06 ` [PATCH v6 12/13] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
@ 2021-07-21  3:06 ` Liam Beguin
  2021-07-23 22:59   ` Rob Herring
  12 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-21  3:06 UTC (permalink / raw)
  To: liambeguin, peda, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
---
 .../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.30.1.489.g328c10930387


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-21  3:06 ` [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
@ 2021-07-23 21:16   ` Peter Rosin
  2021-07-28  0:21     ` Liam Beguin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Rosin @ 2021-07-23 21:16 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-21 05:06, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> 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 <lvb@xiphos.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index d0669fd8eac5..2b73047365cc 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>  		do_div(tmp, 1000000000LL);
>  		*val = tmp;
>  		return scale_type;
> +	case IIO_VAL_INT_PLUS_NANO:
> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> +		tmp = div_s64(tmp, rescale->denominator);
> +
> +		*val = div_s64(tmp, 1000000000LL);
> +		*val2 = tmp - *val * 1000000000LL;
> +		return scale_type;

Hi!

My objection from v5 still stands. Did you forget or did you simply send the
wrong patch?

Untested suggestion, this time handling negative values and canonicalizing any
overflow from the fraction calculation.

	neg = *val < 0 || *val2 < 0;
	tmp = (s64)abs(*val) * rescale->numerator;
	rem = do_div(tmp, rescale->denominator);
	*val = tmp;
	tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
	do_div(tmp, rescale->denominator);
	*val2 = do_div(tmp, 1000000000LL);
	*val += tmp;
	if (neg) {
		if (*val < 0)
			*val = -*val;
		else
			*val2 = -*val;
	}

> +	case IIO_VAL_INT_PLUS_MICRO:
> +		tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> +		tmp = div_s64(tmp, rescale->denominator);
> +
> +		*val = div_s64(tmp, 1000000);

Why do you not have the LL suffix here?

Cheers,
Peter

> +		*val2 = tmp - *val * 1000000;
> +		return scale_type;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow
  2021-07-21  3:06 ` [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
@ 2021-07-23 21:17   ` Peter Rosin
  2021-07-28  0:07     ` Liam Beguin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Rosin @ 2021-07-23 21:17 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-21 05:06, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> Reduce the risk of integer overflow by doing the scale calculation with
> 64bit integers and looking for a Greatest Common Divider for both parts
> of the fractional value when required.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 6f6a711ae3ae..35fa3b4e53e0 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -21,12 +21,21 @@
>  int rescale_process_scale(struct rescale *rescale, int scale_type,
>  			  int *val, int *val2)
>  {
> -	unsigned long long tmp;
> +	s64 tmp, tmp2;
> +	u32 factor;
>  
>  	switch (scale_type) {
>  	case IIO_VAL_FRACTIONAL:
> -		*val *= rescale->numerator;
> -		*val2 *= rescale->denominator;
> +		if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
> +		    check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
> +			tmp = (s64)*val * rescale->numerator;
> +			tmp2 = (s64)*val2 * rescale->denominator;
> +			factor = gcd(tmp, tmp2);

Hi!

Reiterating that gcd() only works for unsigned operands, so this is broken for
negative values.

Cheers,
Peter

> +			tmp = div_s64(tmp, factor);
> +			tmp2 = div_s64(tmp2, factor);
> +		}
> +		*val = tmp;
> +		*val2 = tmp2;
>  		return scale_type;
>  	case IIO_VAL_INT:
>  		*val *= rescale->numerator;
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale
  2021-07-21  3:06 ` [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale Liam Beguin
@ 2021-07-23 21:20   ` Peter Rosin
  2021-07-28  0:26     ` Liam Beguin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Rosin @ 2021-07-23 21:20 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-21 05:06, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected
> scale. Update the case so that the rescaler returns a fractional type
> and a more precise scale.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 35fa3b4e53e0..47cd4a6d9aca 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>  		*val2 = rescale->denominator;
>  		return IIO_VAL_FRACTIONAL;
>  	case IIO_VAL_FRACTIONAL_LOG2:
> -		tmp = *val * 1000000000LL;
> -		do_div(tmp, rescale->denominator);
> -		tmp *= rescale->numerator;
> -		do_div(tmp, 1000000000LL);
> -		*val = tmp;
> -		return scale_type;
> +		*val = rescale->numerator * *val;
> +		*val2 = rescale->denominator * (1 << *val2);
> +		return IIO_VAL_FRACTIONAL;

Hi!

I do not think this is an uncontested improvement. You have broken the case
where *val2 is "large" before the scale factor is applied.

Cheers,
Peter

>  	case IIO_VAL_INT_PLUS_NANO:
>  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>  		tmp = div_s64(tmp, rescale->denominator);
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 13/13] dt-bindings: iio: afe: add bindings for temperature transducers
  2021-07-21  3:06 ` [PATCH v6 13/13] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
@ 2021-07-23 22:59   ` Rob Herring
  0 siblings, 0 replies; 33+ messages in thread
From: Rob Herring @ 2021-07-23 22:59 UTC (permalink / raw)
  To: Liam Beguin
  Cc: devicetree, pmeerw, linux-kernel, robh+dt, linux-iio, peda, jic23, lars

On Tue, 20 Jul 2021 23:06:13 -0400, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> 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 <lvb@xiphos.com>
> ---
>  .../iio/afe/temperature-transducer.yaml       | 114 ++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 07/13] iio: test: add basic tests for the iio-rescale driver
  2021-07-21  3:06 ` [PATCH v6 07/13] iio: test: add basic tests for the iio-rescale driver Liam Beguin
@ 2021-07-24  8:40   ` kernel test robot
  0 siblings, 0 replies; 33+ messages in thread
From: kernel test robot @ 2021-07-24  8:40 UTC (permalink / raw)
  To: Liam Beguin, peda, jic23, lars, pmeerw
  Cc: clang-built-linux, kbuild-all, linux-kernel, linux-iio,
	devicetree, robh+dt

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

Hi Liam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78]

url:    https://github.com/0day-ci/linux/commits/Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210721-111032
base:   6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
config: arm64-randconfig-r022-20210720 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project c781eb153bfbd1b52b03efe34f56bbeccbb8aba6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6ce4f151647d60473a4d71ed1d3036706632a020
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Liam-Beguin/iio-afe-add-temperature-rescaling-support/20210721-111032
        git checkout 6ce4f151647d60473a4d71ed1d3036706632a020
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: duplicate symbol: rescale_process_scale
   >>> defined at iio-rescale.c
   >>> iio/afe/iio-rescale.o:(rescale_process_scale) in archive drivers/built-in.a
   >>> defined at iio-rescale.c
   >>> iio/afe/iio-rescale.o:(.text+0x0) in archive drivers/built-in.a
--
>> ld.lld: error: duplicate symbol: rescale_process_offset
   >>> defined at iio-rescale.c
   >>> iio/afe/iio-rescale.o:(rescale_process_offset) in archive drivers/built-in.a
   >>> defined at iio-rescale.c
   >>> iio/afe/iio-rescale.o:(.text+0x180) in archive drivers/built-in.a

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45517 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow
  2021-07-23 21:17   ` Peter Rosin
@ 2021-07-28  0:07     ` Liam Beguin
  2021-07-28  7:47       ` Peter Rosin
  0 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-28  0:07 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Fri Jul 23, 2021 at 5:17 PM EDT, Peter Rosin wrote:
> On 2021-07-21 05:06, Liam Beguin wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > Reduce the risk of integer overflow by doing the scale calculation with
> > 64bit integers and looking for a Greatest Common Divider for both parts
> > of the fractional value when required.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/iio/afe/iio-rescale.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 6f6a711ae3ae..35fa3b4e53e0 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -21,12 +21,21 @@
> >  int rescale_process_scale(struct rescale *rescale, int scale_type,
> >  			  int *val, int *val2)
> >  {
> > -	unsigned long long tmp;
> > +	s64 tmp, tmp2;
> > +	u32 factor;
> >  
> >  	switch (scale_type) {
> >  	case IIO_VAL_FRACTIONAL:
> > -		*val *= rescale->numerator;
> > -		*val2 *= rescale->denominator;
> > +		if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
> > +		    check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
> > +			tmp = (s64)*val * rescale->numerator;
> > +			tmp2 = (s64)*val2 * rescale->denominator;
> > +			factor = gcd(tmp, tmp2);

Hi Peter,

>
> Hi!
>
> Reiterating that gcd() only works for unsigned operands, so this is
> broken for
> negative values.

Apologies, I didn't mean to make it seem like I ignored your comments. I
should've added a note. After you pointed out that gcd() only works for
unsigned elements, I added test cases for negative values, and all tests
passed. I'll look into it more.

rescale_voltage_divider_props() seems to also use gcd() with signed
integers.

Thanks,
Liam

>
> Cheers,
> Peter
>
> > +			tmp = div_s64(tmp, factor);
> > +			tmp2 = div_s64(tmp2, factor);
> > +		}
> > +		*val = tmp;
> > +		*val2 = tmp2;
> >  		return scale_type;
> >  	case IIO_VAL_INT:
> >  		*val *= rescale->numerator;
> > 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-23 21:16   ` Peter Rosin
@ 2021-07-28  0:21     ` Liam Beguin
  2021-07-28  7:19       ` Peter Rosin
  0 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-28  0:21 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
> On 2021-07-21 05:06, Liam Beguin wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > 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 <lvb@xiphos.com>
> > ---
> >  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index d0669fd8eac5..2b73047365cc 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >  		do_div(tmp, 1000000000LL);
> >  		*val = tmp;
> >  		return scale_type;
> > +	case IIO_VAL_INT_PLUS_NANO:
> > +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> > +		tmp = div_s64(tmp, rescale->denominator);
> > +
> > +		*val = div_s64(tmp, 1000000000LL);
> > +		*val2 = tmp - *val * 1000000000LL;
> > +		return scale_type;

Hi Peter,

>
> Hi!
>
> My objection from v5 still stands. Did you forget or did you simply send
> the
> wrong patch?

Apologies, again I didn't mean to make it seem like I ignored your comments.
I tried your suggestion, but had issues when *val2 would overflow into
the integer part.
Even though what I has was more prone to integer overflow with the first
multiplication, I thought it was still a valid solution as it passed the
tests.

>
> Untested suggestion, this time handling negative values and
> canonicalizing any
> overflow from the fraction calculation.
>
> neg = *val < 0 || *val2 < 0;
> tmp = (s64)abs(*val) * rescale->numerator;
> rem = do_div(tmp, rescale->denominator);
> *val = tmp;
> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
> do_div(tmp, rescale->denominator);
> *val2 = do_div(tmp, 1000000000LL);
> *val += tmp;
> if (neg) {
> if (*val < 0)
> *val = -*val;
> else
> *val2 = -*val;

I'll look into this suggestion.

> }
>
> > +	case IIO_VAL_INT_PLUS_MICRO:
> > +		tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> > +		tmp = div_s64(tmp, rescale->denominator);
> > +
> > +		*val = div_s64(tmp, 1000000);
>
> Why do you not have the LL suffix here?

Doesnt' LL make it into a 64 bit integer?
I left it out because the second parameter of div_s64() should be s32.

Thanks,
Liam

>
> Cheers,
> Peter
>
> > +		*val2 = tmp - *val * 1000000;
> > +		return scale_type;
> >  	default:
> >  		return -EOPNOTSUPP;
> >  	}
> > 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale
  2021-07-23 21:20   ` Peter Rosin
@ 2021-07-28  0:26     ` Liam Beguin
  2021-07-28  7:58       ` Peter Rosin
  0 siblings, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-28  0:26 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Fri Jul 23, 2021 at 5:20 PM EDT, Peter Rosin wrote:
> On 2021-07-21 05:06, Liam Beguin wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected
> > scale. Update the case so that the rescaler returns a fractional type
> > and a more precise scale.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/iio/afe/iio-rescale.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 35fa3b4e53e0..47cd4a6d9aca 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >  		*val2 = rescale->denominator;
> >  		return IIO_VAL_FRACTIONAL;
> >  	case IIO_VAL_FRACTIONAL_LOG2:
> > -		tmp = *val * 1000000000LL;
> > -		do_div(tmp, rescale->denominator);
> > -		tmp *= rescale->numerator;
> > -		do_div(tmp, 1000000000LL);
> > -		*val = tmp;
> > -		return scale_type;
> > +		*val = rescale->numerator * *val;
> > +		*val2 = rescale->denominator * (1 << *val2);
> > +		return IIO_VAL_FRACTIONAL;
>
> Hi!

Hi Peter,

>
> I do not think this is an uncontested improvement. You have broken the
> case
> where *val2 is "large" before the scale factor is applied.

I was a little reluctant to add this change as I keep increasing the
scope of this series, but since I added tests for all cases, I didn't
want to leave this one out.

Would you rather I drop this patch and the test cases associated to it?

Thanks,
Liam

>
> Cheers,
> Peter
>
> >  	case IIO_VAL_INT_PLUS_NANO:
> >  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >  		tmp = div_s64(tmp, rescale->denominator);
> > 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-28  0:21     ` Liam Beguin
@ 2021-07-28  7:19       ` Peter Rosin
  2021-07-29 15:56         ` Liam Beguin
  2021-07-31 17:47         ` Jonathan Cameron
  0 siblings, 2 replies; 33+ messages in thread
From: Peter Rosin @ 2021-07-28  7:19 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-28 02:21, Liam Beguin wrote:
> On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
>> On 2021-07-21 05:06, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@xiphos.com>
>>>
>>> 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 <lvb@xiphos.com>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index d0669fd8eac5..2b73047365cc 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>  		do_div(tmp, 1000000000LL);
>>>  		*val = tmp;
>>>  		return scale_type;
>>> +	case IIO_VAL_INT_PLUS_NANO:
>>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>> +		tmp = div_s64(tmp, rescale->denominator);
>>> +
>>> +		*val = div_s64(tmp, 1000000000LL);
>>> +		*val2 = tmp - *val * 1000000000LL;
>>> +		return scale_type;
> 
> Hi Peter,
> 
>>
>> Hi!
>>
>> My objection from v5 still stands. Did you forget or did you simply send
>> the
>> wrong patch?
> 
> Apologies, again I didn't mean to make it seem like I ignored your comments.
> I tried your suggestion, but had issues when *val2 would overflow into
> the integer part.

Not saying anything about it not working does indeed make it seem like you
ignored it :-)  Or did I just miss where you said this? Anyway, no problem,
it can be a mess dealing with a string of commits when there are numerous
things to take care of between each iteration. And it's very easy to burn
out and just back away. Please don't do that!

> Even though what I has was more prone to integer overflow with the first
> multiplication, I thought it was still a valid solution as it passed the
> tests.

I did state that you'd need to add overflow handling from the fraction
calculation and handling for negative values, so it was no surprise that
my original sketchy suggestion didn't work as-is.

> 
>>
>> Untested suggestion, this time handling negative values and
>> canonicalizing any
>> overflow from the fraction calculation.
>>
>> neg = *val < 0 || *val2 < 0;
>> tmp = (s64)abs(*val) * rescale->numerator;
>> rem = do_div(tmp, rescale->denominator);
>> *val = tmp;
>> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
>> do_div(tmp, rescale->denominator);
>> *val2 = do_div(tmp, 1000000000LL);
>> *val += tmp;
>> if (neg) {
>> if (*val < 0)
>> *val = -*val;
>> else
>> *val2 = -*val;

This last line should of course be *val2 = -*val2;
Sorry.

> 
> I'll look into this suggestion.

Thanks!

> 
>> }
>>
>>> +	case IIO_VAL_INT_PLUS_MICRO:
>>> +		tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
>>> +		tmp = div_s64(tmp, rescale->denominator);
>>> +
>>> +		*val = div_s64(tmp, 1000000);
>>
>> Why do you not have the LL suffix here?
> 
> Doesnt' LL make it into a 64 bit integer?
> I left it out because the second parameter of div_s64() should be s32.

It just looked really odd with 1000000000LL for all instances, but then
1000000LL only for some. The lack of symmetry bothered me.

To me, it seems as if we either need to support old/small crap with
int being 16-bit, or we don't. If we don't need support for 16-bit,
then we don't need any LL suffix, since 1000000000 fits just fine in
32-bit. If we do need 16-bit support, then we need LL (or something)
all over since neither 1000000 nor 1000000000 fit in 16-bit.

I think the compiler looks at the value of the constant and not the
size of its type when selecting how big values the mul/add/whatever
needs handle. So, adding LL feels like the safe option. Further, I
guesstimate that the runtime cost of adding LL is zero and that the
compile time cost is negligible.

But maybe I'm missing something?

Cheers,
Peter

> 
> Thanks,
> Liam
> 
>>
>> Cheers,
>> Peter
>>
>>> +		*val2 = tmp - *val * 1000000;
>>> +		return scale_type;
>>>  	default:
>>>  		return -EOPNOTSUPP;
>>>  	}
>>>
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow
  2021-07-28  0:07     ` Liam Beguin
@ 2021-07-28  7:47       ` Peter Rosin
  2021-07-29 16:02         ` Liam Beguin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Rosin @ 2021-07-28  7:47 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-28 02:07, Liam Beguin wrote:
> On Fri Jul 23, 2021 at 5:17 PM EDT, Peter Rosin wrote:
>> On 2021-07-21 05:06, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@xiphos.com>
>>>
>>> Reduce the risk of integer overflow by doing the scale calculation with
>>> 64bit integers and looking for a Greatest Common Divider for both parts
>>> of the fractional value when required.
>>>
>>> Signed-off-by: Liam Beguin <lvb@xiphos.com>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index 6f6a711ae3ae..35fa3b4e53e0 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -21,12 +21,21 @@
>>>  int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>  			  int *val, int *val2)
>>>  {
>>> -	unsigned long long tmp;
>>> +	s64 tmp, tmp2;
>>> +	u32 factor;
>>>  
>>>  	switch (scale_type) {
>>>  	case IIO_VAL_FRACTIONAL:
>>> -		*val *= rescale->numerator;
>>> -		*val2 *= rescale->denominator;
>>> +		if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
>>> +		    check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
>>> +			tmp = (s64)*val * rescale->numerator;
>>> +			tmp2 = (s64)*val2 * rescale->denominator;
>>> +			factor = gcd(tmp, tmp2);
> 
> Hi Peter,
> 
>>
>> Hi!
>>
>> Reiterating that gcd() only works for unsigned operands, so this is
>> broken for
>> negative values.
> 
> Apologies, I didn't mean to make it seem like I ignored your comments. I
> should've added a note. After you pointed out that gcd() only works for
> unsigned elements, I added test cases for negative values, and all tests
> passed. I'll look into it more.

Maybe I've misread the code and gcd is in fact working for negative
numbers? However, I imagine it might be arch specific, so testing on
a single arch feels insufficient and deeper analysis is required.

However, looking at lib/math/gcd.c it certainly still looks like
negative values will work very poorly, and there is no macro magic
in include/linux/gcd.h to handle it by wrapping the core C routine.

> rescale_voltage_divider_props() seems to also use gcd() with signed
> integers.

The type of the operands may be s32, but if you look at how those values
are populated, and with what they are populated, I think you will find that
only positive scale factors are sensible for a voltage divider. Using
resistors with so high resistance that s32 is not enough is simply not
supported.

Cheers,
Peter

> Thanks,
> Liam
> 
>>
>> Cheers,
>> Peter
>>
>>> +			tmp = div_s64(tmp, factor);
>>> +			tmp2 = div_s64(tmp2, factor);
>>> +		}
>>> +		*val = tmp;
>>> +		*val2 = tmp2;
>>>  		return scale_type;
>>>  	case IIO_VAL_INT:
>>>  		*val *= rescale->numerator;
>>>
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale
  2021-07-28  0:26     ` Liam Beguin
@ 2021-07-28  7:58       ` Peter Rosin
  2021-07-29 16:19         ` Liam Beguin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Rosin @ 2021-07-28  7:58 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-28 02:26, Liam Beguin wrote:
> On Fri Jul 23, 2021 at 5:20 PM EDT, Peter Rosin wrote:
>> On 2021-07-21 05:06, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@xiphos.com>
>>>
>>> The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected
>>> scale. Update the case so that the rescaler returns a fractional type
>>> and a more precise scale.
>>>
>>> Signed-off-by: Liam Beguin <lvb@xiphos.com>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 9 +++------
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index 35fa3b4e53e0..47cd4a6d9aca 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>  		*val2 = rescale->denominator;
>>>  		return IIO_VAL_FRACTIONAL;
>>>  	case IIO_VAL_FRACTIONAL_LOG2:
>>> -		tmp = *val * 1000000000LL;
>>> -		do_div(tmp, rescale->denominator);
>>> -		tmp *= rescale->numerator;
>>> -		do_div(tmp, 1000000000LL);
>>> -		*val = tmp;
>>> -		return scale_type;
>>> +		*val = rescale->numerator * *val;
>>> +		*val2 = rescale->denominator * (1 << *val2);
>>> +		return IIO_VAL_FRACTIONAL;
>>
>> Hi!
> 
> Hi Peter,
> 
>>
>> I do not think this is an uncontested improvement. You have broken the
>> case
>> where *val2 is "large" before the scale factor is applied.
> 
> I was a little reluctant to add this change as I keep increasing the
> scope of this series, but since I added tests for all cases, I didn't
> want to leave this one out.

> Would you rather I drop this patch and the test cases associated to it?

Why drop the tests? Are they doing any harm? Or are they testing exactly
the problem situation that fail without this patch?

In that case, I guess fix the tests to pass and preferably add tests
for the *val2 is "large" situation (that this patch breaks) so that the
next person trying to improve precision is made aware of the overflow
problem. Does that make sense?

Cheers,
Peter

> Thanks,
> Liam
> 
>>
>> Cheers,
>> Peter
>>
>>>  	case IIO_VAL_INT_PLUS_NANO:
>>>  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>>  		tmp = div_s64(tmp, rescale->denominator);
>>>
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-28  7:19       ` Peter Rosin
@ 2021-07-29 15:56         ` Liam Beguin
  2021-07-30  6:49           ` Peter Rosin
  2021-07-31 17:47         ` Jonathan Cameron
  1 sibling, 1 reply; 33+ messages in thread
From: Liam Beguin @ 2021-07-29 15:56 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Wed Jul 28, 2021 at 3:19 AM EDT, Peter Rosin wrote:
> On 2021-07-28 02:21, Liam Beguin wrote:
> > On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
> >> On 2021-07-21 05:06, Liam Beguin wrote:
> >>> From: Liam Beguin <lvb@xiphos.com>
> >>>
> >>> 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 <lvb@xiphos.com>
> >>> ---
> >>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>> index d0669fd8eac5..2b73047365cc 100644
> >>> --- a/drivers/iio/afe/iio-rescale.c
> >>> +++ b/drivers/iio/afe/iio-rescale.c
> >>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >>>  		do_div(tmp, 1000000000LL);
> >>>  		*val = tmp;
> >>>  		return scale_type;
> >>> +	case IIO_VAL_INT_PLUS_NANO:
> >>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >>> +		tmp = div_s64(tmp, rescale->denominator);
> >>> +
> >>> +		*val = div_s64(tmp, 1000000000LL);
> >>> +		*val2 = tmp - *val * 1000000000LL;
> >>> +		return scale_type;
> > 
> > Hi Peter,
> > 
> >>
> >> Hi!
> >>
> >> My objection from v5 still stands. Did you forget or did you simply send
> >> the
> >> wrong patch?
> > 
> > Apologies, again I didn't mean to make it seem like I ignored your comments.
> > I tried your suggestion, but had issues when *val2 would overflow into
> > the integer part.

Hi Peter,

>
> Not saying anything about it not working does indeed make it seem like
> you
> ignored it :-) Or did I just miss where you said this? Anyway, no
> problem,
> it can be a mess dealing with a string of commits when there are
> numerous
> things to take care of between each iteration. And it's very easy to
> burn
> out and just back away. Please don't do that!

It was my mistake. Thanks for the encouragement :-)

>
> > Even though what I has was more prone to integer overflow with the first
> > multiplication, I thought it was still a valid solution as it passed the
> > tests.
>
> I did state that you'd need to add overflow handling from the fraction
> calculation and handling for negative values, so it was no surprise that
> my original sketchy suggestion didn't work as-is.
>
> > 
> >>
> >> Untested suggestion, this time handling negative values and
> >> canonicalizing any
> >> overflow from the fraction calculation.
> >>
> >> neg = *val < 0 || *val2 < 0;
> >> tmp = (s64)abs(*val) * rescale->numerator;
> >> rem = do_div(tmp, rescale->denominator);
> >> *val = tmp;
> >> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
> >> do_div(tmp, rescale->denominator);
> >> *val2 = do_div(tmp, 1000000000LL);
> >> *val += tmp;
> >> if (neg) {
> >> if (*val < 0)
> >> *val = -*val;
> >> else
> >> *val2 = -*val;
>
> This last line should of course be *val2 = -*val2;
> Sorry.
>
> > 
> > I'll look into this suggestion.
>
> Thanks!
>

Starting from what you suggested, here's what I came up with.
I also added a few test cases to cover corner cases.

	if (scale_type == IIO_VAL_INT_PLUS_NANO)
		mult = 1000000000LL;
	else
		mult = 1000000LL;
	/*
	 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
	 * *val2 is negative the schan scale is negative
	 */
	neg = *val < 0 || *val2 < 0;

	tmp = (s64)abs(*val) * (s32)abs(rescale->numerator);
	*val = div_s64_rem(tmp, (s32)abs(rescale->denominator), &rem);

	tmp = (s64)rem * mult +
		(s64)abs(*val2) * (s32)abs(rescale->numerator);
	tmp = div_s64(tmp, (s32)abs(rescale->denominator));

	*val += div_s64_rem(tmp, mult, val2);

	/*
	 * If the schan scale or only one of the rescaler elements is
	 * negative, the combined scale is negative.
	 */
	if (neg || ((rescale->numerator < 0) ^ (rescale->denominator < 0)))
		*val = -*val;

	return scale_type;
> > 
> >> }
> >>
> >>> +	case IIO_VAL_INT_PLUS_MICRO:
> >>> +		tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> >>> +		tmp = div_s64(tmp, rescale->denominator);
> >>> +
> >>> +		*val = div_s64(tmp, 1000000);
> >>
> >> Why do you not have the LL suffix here?
> > 
> > Doesnt' LL make it into a 64 bit integer?
> > I left it out because the second parameter of div_s64() should be s32.
>
> It just looked really odd with 1000000000LL for all instances, but then
> 1000000LL only for some. The lack of symmetry bothered me.
>
> To me, it seems as if we either need to support old/small crap with
> int being 16-bit, or we don't. If we don't need support for 16-bit,
> then we don't need any LL suffix, since 1000000000 fits just fine in
> 32-bit. If we do need 16-bit support, then we need LL (or something)
> all over since neither 1000000 nor 1000000000 fit in 16-bit.
>
> I think the compiler looks at the value of the constant and not the
> size of its type when selecting how big values the mul/add/whatever
> needs handle. So, adding LL feels like the safe option. Further, I
> guesstimate that the runtime cost of adding LL is zero and that the
> compile time cost is negligible.

Thanks for the explanation, I thought it might matter but I agree that
the asymmetry looks odd. I'll fix it.

Thanks,
Liam

>
> But maybe I'm missing something?
>
> Cheers,
> Peter
>
> > 
> > Thanks,
> > Liam
> > 
> >>
> >> Cheers,
> >> Peter
> >>
> >>> +		*val2 = tmp - *val * 1000000;
> >>> +		return scale_type;
> >>>  	default:
> >>>  		return -EOPNOTSUPP;
> >>>  	}
> >>>
> > 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow
  2021-07-28  7:47       ` Peter Rosin
@ 2021-07-29 16:02         ` Liam Beguin
  0 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-29 16:02 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Wed Jul 28, 2021 at 3:47 AM EDT, Peter Rosin wrote:
> On 2021-07-28 02:07, Liam Beguin wrote:
> > On Fri Jul 23, 2021 at 5:17 PM EDT, Peter Rosin wrote:
> >> On 2021-07-21 05:06, Liam Beguin wrote:
> >>> From: Liam Beguin <lvb@xiphos.com>
> >>>
> >>> Reduce the risk of integer overflow by doing the scale calculation with
> >>> 64bit integers and looking for a Greatest Common Divider for both parts
> >>> of the fractional value when required.
> >>>
> >>> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> >>> ---
> >>>  drivers/iio/afe/iio-rescale.c | 15 ++++++++++++---
> >>>  1 file changed, 12 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>> index 6f6a711ae3ae..35fa3b4e53e0 100644
> >>> --- a/drivers/iio/afe/iio-rescale.c
> >>> +++ b/drivers/iio/afe/iio-rescale.c
> >>> @@ -21,12 +21,21 @@
> >>>  int rescale_process_scale(struct rescale *rescale, int scale_type,
> >>>  			  int *val, int *val2)
> >>>  {
> >>> -	unsigned long long tmp;
> >>> +	s64 tmp, tmp2;
> >>> +	u32 factor;
> >>>  
> >>>  	switch (scale_type) {
> >>>  	case IIO_VAL_FRACTIONAL:
> >>> -		*val *= rescale->numerator;
> >>> -		*val2 *= rescale->denominator;
> >>> +		if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
> >>> +		    check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
> >>> +			tmp = (s64)*val * rescale->numerator;
> >>> +			tmp2 = (s64)*val2 * rescale->denominator;
> >>> +			factor = gcd(tmp, tmp2);
> > 
> > Hi Peter,
> > 
> >>
> >> Hi!
> >>
> >> Reiterating that gcd() only works for unsigned operands, so this is
> >> broken for
> >> negative values.
> > 
> > Apologies, I didn't mean to make it seem like I ignored your comments. I
> > should've added a note. After you pointed out that gcd() only works for
> > unsigned elements, I added test cases for negative values, and all tests
> > passed. I'll look into it more.
>
> Maybe I've misread the code and gcd is in fact working for negative
> numbers? However, I imagine it might be arch specific, so testing on
> a single arch feels insufficient and deeper analysis is required.
>
> However, looking at lib/math/gcd.c it certainly still looks like
> negative values will work very poorly, and there is no macro magic
> in include/linux/gcd.h to handle it by wrapping the core C routine.

I agree that looking at lib/math/gcd.c odd things might happen with
negative values. I'll use the the absolute values to calculate the GCD
as it shouldn't affect the value of factor.

>
> > rescale_voltage_divider_props() seems to also use gcd() with signed
> > integers.
>
> The type of the operands may be s32, but if you look at how those values
> are populated, and with what they are populated, I think you will find
> that
> only positive scale factors are sensible for a voltage divider. Using
> resistors with so high resistance that s32 is not enough is simply not
> supported.

That makes sense!

Thanks,
Liam

>
> Cheers,
> Peter
>
> > Thanks,
> > Liam
> > 
> >>
> >> Cheers,
> >> Peter
> >>
> >>> +			tmp = div_s64(tmp, factor);
> >>> +			tmp2 = div_s64(tmp2, factor);
> >>> +		}
> >>> +		*val = tmp;
> >>> +		*val2 = tmp2;
> >>>  		return scale_type;
> >>>  	case IIO_VAL_INT:
> >>>  		*val *= rescale->numerator;
> >>>
> > 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale
  2021-07-28  7:58       ` Peter Rosin
@ 2021-07-29 16:19         ` Liam Beguin
  0 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-29 16:19 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Wed Jul 28, 2021 at 3:58 AM EDT, Peter Rosin wrote:
> On 2021-07-28 02:26, Liam Beguin wrote:
> > On Fri Jul 23, 2021 at 5:20 PM EDT, Peter Rosin wrote:
> >> On 2021-07-21 05:06, Liam Beguin wrote:
> >>> From: Liam Beguin <lvb@xiphos.com>
> >>>
> >>> The IIO_VAL_FRACTIONAL_LOG2 scale type doesn't return the expected
> >>> scale. Update the case so that the rescaler returns a fractional type
> >>> and a more precise scale.
> >>>
> >>> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> >>> ---
> >>>  drivers/iio/afe/iio-rescale.c | 9 +++------
> >>>  1 file changed, 3 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>> index 35fa3b4e53e0..47cd4a6d9aca 100644
> >>> --- a/drivers/iio/afe/iio-rescale.c
> >>> +++ b/drivers/iio/afe/iio-rescale.c
> >>> @@ -44,12 +44,9 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >>>  		*val2 = rescale->denominator;
> >>>  		return IIO_VAL_FRACTIONAL;
> >>>  	case IIO_VAL_FRACTIONAL_LOG2:
> >>> -		tmp = *val * 1000000000LL;
> >>> -		do_div(tmp, rescale->denominator);
> >>> -		tmp *= rescale->numerator;
> >>> -		do_div(tmp, 1000000000LL);
> >>> -		*val = tmp;
> >>> -		return scale_type;
> >>> +		*val = rescale->numerator * *val;
> >>> +		*val2 = rescale->denominator * (1 << *val2);
> >>> +		return IIO_VAL_FRACTIONAL;
> >>
> >> Hi!
> > 
> > Hi Peter,
> > 
> >>
> >> I do not think this is an uncontested improvement. You have broken the
> >> case
> >> where *val2 is "large" before the scale factor is applied.
> > 
> > I was a little reluctant to add this change as I keep increasing the
> > scope of this series, but since I added tests for all cases, I didn't
> > want to leave this one out.
>
> > Would you rather I drop this patch and the test cases associated to it?
>
> Why drop the tests? Are they doing any harm? Or are they testing exactly
> the problem situation that fail without this patch?

They are testing this problem and fail without the patch.

>
> In that case, I guess fix the tests to pass and preferably add tests
> for the *val2 is "large" situation (that this patch breaks) so that the
> next person trying to improve precision is made aware of the overflow
> problem. Does that make sense?

To handle large values of *val2, I could use the same logic as in
IIO_VAL_FRACTIONAL with check_mul_overflow() and gcd().

would that be okay?

Thanks,
Liam

>
> Cheers,
> Peter
>
> > Thanks,
> > Liam
> > 
> >>
> >> Cheers,
> >> Peter
> >>
> >>>  	case IIO_VAL_INT_PLUS_NANO:
> >>>  		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >>>  		tmp = div_s64(tmp, rescale->denominator);
> >>>
> > 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-29 15:56         ` Liam Beguin
@ 2021-07-30  6:49           ` Peter Rosin
  2021-07-30  7:01             ` Peter Rosin
  2021-07-30 19:57             ` Liam Beguin
  0 siblings, 2 replies; 33+ messages in thread
From: Peter Rosin @ 2021-07-30  6:49 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-29 17:56, Liam Beguin wrote:
> On Wed Jul 28, 2021 at 3:19 AM EDT, Peter Rosin wrote:
>> On 2021-07-28 02:21, Liam Beguin wrote:
>>> On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
>>>> On 2021-07-21 05:06, Liam Beguin wrote:
>>>>> From: Liam Beguin <lvb@xiphos.com>
>>>>>
>>>>> 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 <lvb@xiphos.com>
>>>>> ---
>>>>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>>>> index d0669fd8eac5..2b73047365cc 100644
>>>>> --- a/drivers/iio/afe/iio-rescale.c
>>>>> +++ b/drivers/iio/afe/iio-rescale.c
>>>>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>>>  		do_div(tmp, 1000000000LL);
>>>>>  		*val = tmp;
>>>>>  		return scale_type;
>>>>> +	case IIO_VAL_INT_PLUS_NANO:
>>>>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>>>> +		tmp = div_s64(tmp, rescale->denominator);
>>>>> +
>>>>> +		*val = div_s64(tmp, 1000000000LL);
>>>>> +		*val2 = tmp - *val * 1000000000LL;
>>>>> +		return scale_type;
>>>
>>> Hi Peter,
>>>
>>>>
>>>> Hi!
>>>>
>>>> My objection from v5 still stands. Did you forget or did you simply send
>>>> the
>>>> wrong patch?
>>>
>>> Apologies, again I didn't mean to make it seem like I ignored your comments.
>>> I tried your suggestion, but had issues when *val2 would overflow into
>>> the integer part.
> 
> Hi Peter,
> 
>>
>> Not saying anything about it not working does indeed make it seem like
>> you
>> ignored it :-) Or did I just miss where you said this? Anyway, no
>> problem,
>> it can be a mess dealing with a string of commits when there are
>> numerous
>> things to take care of between each iteration. And it's very easy to
>> burn
>> out and just back away. Please don't do that!
> 
> It was my mistake. Thanks for the encouragement :-)
> 
>>
>>> Even though what I has was more prone to integer overflow with the first
>>> multiplication, I thought it was still a valid solution as it passed the
>>> tests.
>>
>> I did state that you'd need to add overflow handling from the fraction
>> calculation and handling for negative values, so it was no surprise that
>> my original sketchy suggestion didn't work as-is.
>>
>>>
>>>>
>>>> Untested suggestion, this time handling negative values and
>>>> canonicalizing any
>>>> overflow from the fraction calculation.
>>>>
>>>> neg = *val < 0 || *val2 < 0;
>>>> tmp = (s64)abs(*val) * rescale->numerator;
>>>> rem = do_div(tmp, rescale->denominator);
>>>> *val = tmp;
>>>> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
>>>> do_div(tmp, rescale->denominator);
>>>> *val2 = do_div(tmp, 1000000000LL);
>>>> *val += tmp;
>>>> if (neg) {
>>>> if (*val < 0)
>>>> *val = -*val;
>>>> else
>>>> *val2 = -*val;
>>
>> This last line should of course be *val2 = -*val2;
>> Sorry.
>>
>>>
>>> I'll look into this suggestion.
>>
>> Thanks!
>>
> 
> Starting from what you suggested, here's what I came up with.
> I also added a few test cases to cover corner cases.
> 
> 	if (scale_type == IIO_VAL_INT_PLUS_NANO)
> 		mult = 1000000000LL;
> 	else
> 		mult = 1000000LL;
> 	/*
> 	 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
> 	 * *val2 is negative the schan scale is negative
> 	 */
> 	neg = *val < 0 || *val2 < 0;
> 
> 	tmp = (s64)abs(*val) * (s32)abs(rescale->numerator);

Small nit, but I think abs() returns a signed type compatible
with the argument type. I.e. (s32)abs(rescale->...) where both
numerator and denominator are already s32 could just as well
be written without the cast as plain old abs(rescale->...)


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

Unconditionally negating *val doesn't negate the combined value when
*val is zero and *val2 isn't. My test "if (*val < 0)" above attempting
to take care of this case is clearly not right. It should of course be
"if (*val > 0)" since *val is not yet negated. Duh!

In fact, I think a few tests scaling to/from the [-1,1] interval
would be benefitial for this exact reason.

Cheers,
Peter

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-30  6:49           ` Peter Rosin
@ 2021-07-30  7:01             ` Peter Rosin
  2021-07-30 20:01               ` Liam Beguin
  2021-07-30 19:57             ` Liam Beguin
  1 sibling, 1 reply; 33+ messages in thread
From: Peter Rosin @ 2021-07-30  7:01 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-30 08:49, Peter Rosin wrote:
> On 2021-07-29 17:56, Liam Beguin wrote:
>> On Wed Jul 28, 2021 at 3:19 AM EDT, Peter Rosin wrote:
>>> On 2021-07-28 02:21, Liam Beguin wrote:
>>>> On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
>>>>> On 2021-07-21 05:06, Liam Beguin wrote:
>>>>>> From: Liam Beguin <lvb@xiphos.com>
>>>>>>
>>>>>> 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 <lvb@xiphos.com>
>>>>>> ---
>>>>>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>>>>> index d0669fd8eac5..2b73047365cc 100644
>>>>>> --- a/drivers/iio/afe/iio-rescale.c
>>>>>> +++ b/drivers/iio/afe/iio-rescale.c
>>>>>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>>>>>>  		do_div(tmp, 1000000000LL);
>>>>>>  		*val = tmp;
>>>>>>  		return scale_type;
>>>>>> +	case IIO_VAL_INT_PLUS_NANO:
>>>>>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>>>>> +		tmp = div_s64(tmp, rescale->denominator);
>>>>>> +
>>>>>> +		*val = div_s64(tmp, 1000000000LL);
>>>>>> +		*val2 = tmp - *val * 1000000000LL;
>>>>>> +		return scale_type;
>>>>
>>>> Hi Peter,
>>>>
>>>>>
>>>>> Hi!
>>>>>
>>>>> My objection from v5 still stands. Did you forget or did you simply send
>>>>> the
>>>>> wrong patch?
>>>>
>>>> Apologies, again I didn't mean to make it seem like I ignored your comments.
>>>> I tried your suggestion, but had issues when *val2 would overflow into
>>>> the integer part.
>>
>> Hi Peter,
>>
>>>
>>> Not saying anything about it not working does indeed make it seem like
>>> you
>>> ignored it :-) Or did I just miss where you said this? Anyway, no
>>> problem,
>>> it can be a mess dealing with a string of commits when there are
>>> numerous
>>> things to take care of between each iteration. And it's very easy to
>>> burn
>>> out and just back away. Please don't do that!
>>
>> It was my mistake. Thanks for the encouragement :-)
>>
>>>
>>>> Even though what I has was more prone to integer overflow with the first
>>>> multiplication, I thought it was still a valid solution as it passed the
>>>> tests.
>>>
>>> I did state that you'd need to add overflow handling from the fraction
>>> calculation and handling for negative values, so it was no surprise that
>>> my original sketchy suggestion didn't work as-is.
>>>
>>>>
>>>>>
>>>>> Untested suggestion, this time handling negative values and
>>>>> canonicalizing any
>>>>> overflow from the fraction calculation.
>>>>>
>>>>> neg = *val < 0 || *val2 < 0;
>>>>> tmp = (s64)abs(*val) * rescale->numerator;
>>>>> rem = do_div(tmp, rescale->denominator);
>>>>> *val = tmp;
>>>>> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
>>>>> do_div(tmp, rescale->denominator);
>>>>> *val2 = do_div(tmp, 1000000000LL);
>>>>> *val += tmp;
>>>>> if (neg) {
>>>>> if (*val < 0)
>>>>> *val = -*val;
>>>>> else
>>>>> *val2 = -*val;
>>>
>>> This last line should of course be *val2 = -*val2;
>>> Sorry.
>>>
>>>>
>>>> I'll look into this suggestion.
>>>
>>> Thanks!
>>>
>>
>> Starting from what you suggested, here's what I came up with.
>> I also added a few test cases to cover corner cases.
>>
>> 	if (scale_type == IIO_VAL_INT_PLUS_NANO)
>> 		mult = 1000000000LL;
>> 	else
>> 		mult = 1000000LL;
>> 	/*
>> 	 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
>> 	 * *val2 is negative the schan scale is negative
>> 	 */
>> 	neg = *val < 0 || *val2 < 0;
>>
>> 	tmp = (s64)abs(*val) * (s32)abs(rescale->numerator);
> 
> Small nit, but I think abs() returns a signed type compatible
> with the argument type. I.e. (s32)abs(rescale->...) where both
> numerator and denominator are already s32 could just as well
> be written without the cast as plain old abs(rescale->...)
> 
> 
>> 	*val = div_s64_rem(tmp, (s32)abs(rescale->denominator), &rem);
>>
>> 	tmp = (s64)rem * mult +
>> 		(s64)abs(*val2) * (s32)abs(rescale->numerator);
>> 	tmp = div_s64(tmp, (s32)abs(rescale->denominator));
>>
>> 	*val += div_s64_rem(tmp, mult, val2);
>>
>> 	/*
>> 	 * If the schan scale or only one of the rescaler elements is
>> 	 * negative, the combined scale is negative.
>> 	 */
>> 	if (neg || ((rescale->numerator < 0) ^ (rescale->denominator < 0)))

Hang on, that's not right. If the value and only one of the rescaler
elements is negative, the result is positive. || is not the correct
logical operation.

>> 		*val = -*val;
> 
> Unconditionally negating *val doesn't negate the combined value when
> *val is zero and *val2 isn't. My test "if (*val < 0)" above attempting
> to take care of this case is clearly not right. It should of course be
> "if (*val > 0)" since *val is not yet negated. Duh!
> 
> In fact, I think a few tests scaling to/from the [-1,1] interval
> would be benefitial for this exact reason.

So, with both these issues taken care of:

 	if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
		if (*val > 0)
			*val = -*val;
		else
			*val2 = -*val2;
	}

(bitwise ^ is safe since all operands come from logical operations, i.e.
they are either zero or one and nothing else)

Cheers,
Peter


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-30  6:49           ` Peter Rosin
  2021-07-30  7:01             ` Peter Rosin
@ 2021-07-30 19:57             ` Liam Beguin
  1 sibling, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-30 19:57 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Fri Jul 30, 2021 at 2:49 AM EDT, Peter Rosin wrote:
> On 2021-07-29 17:56, Liam Beguin wrote:
> > On Wed Jul 28, 2021 at 3:19 AM EDT, Peter Rosin wrote:
> >> On 2021-07-28 02:21, Liam Beguin wrote:
> >>> On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
> >>>> On 2021-07-21 05:06, Liam Beguin wrote:
> >>>>> From: Liam Beguin <lvb@xiphos.com>
> >>>>>
> >>>>> 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 <lvb@xiphos.com>
> >>>>> ---
> >>>>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
> >>>>>  1 file changed, 14 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>>>> index d0669fd8eac5..2b73047365cc 100644
> >>>>> --- a/drivers/iio/afe/iio-rescale.c
> >>>>> +++ b/drivers/iio/afe/iio-rescale.c
> >>>>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >>>>>  		do_div(tmp, 1000000000LL);
> >>>>>  		*val = tmp;
> >>>>>  		return scale_type;
> >>>>> +	case IIO_VAL_INT_PLUS_NANO:
> >>>>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >>>>> +		tmp = div_s64(tmp, rescale->denominator);
> >>>>> +
> >>>>> +		*val = div_s64(tmp, 1000000000LL);
> >>>>> +		*val2 = tmp - *val * 1000000000LL;
> >>>>> +		return scale_type;
> >>>
> >>> Hi Peter,
> >>>
> >>>>
> >>>> Hi!
> >>>>
> >>>> My objection from v5 still stands. Did you forget or did you simply send
> >>>> the
> >>>> wrong patch?
> >>>
> >>> Apologies, again I didn't mean to make it seem like I ignored your comments.
> >>> I tried your suggestion, but had issues when *val2 would overflow into
> >>> the integer part.
> > 
> > Hi Peter,
> > 
> >>
> >> Not saying anything about it not working does indeed make it seem like
> >> you
> >> ignored it :-) Or did I just miss where you said this? Anyway, no
> >> problem,
> >> it can be a mess dealing with a string of commits when there are
> >> numerous
> >> things to take care of between each iteration. And it's very easy to
> >> burn
> >> out and just back away. Please don't do that!
> > 
> > It was my mistake. Thanks for the encouragement :-)
> > 
> >>
> >>> Even though what I has was more prone to integer overflow with the first
> >>> multiplication, I thought it was still a valid solution as it passed the
> >>> tests.
> >>
> >> I did state that you'd need to add overflow handling from the fraction
> >> calculation and handling for negative values, so it was no surprise that
> >> my original sketchy suggestion didn't work as-is.
> >>
> >>>
> >>>>
> >>>> Untested suggestion, this time handling negative values and
> >>>> canonicalizing any
> >>>> overflow from the fraction calculation.
> >>>>
> >>>> neg = *val < 0 || *val2 < 0;
> >>>> tmp = (s64)abs(*val) * rescale->numerator;
> >>>> rem = do_div(tmp, rescale->denominator);
> >>>> *val = tmp;
> >>>> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
> >>>> do_div(tmp, rescale->denominator);
> >>>> *val2 = do_div(tmp, 1000000000LL);
> >>>> *val += tmp;
> >>>> if (neg) {
> >>>> if (*val < 0)
> >>>> *val = -*val;
> >>>> else
> >>>> *val2 = -*val;
> >>
> >> This last line should of course be *val2 = -*val2;
> >> Sorry.
> >>
> >>>
> >>> I'll look into this suggestion.
> >>
> >> Thanks!
> >>
> > 
> > Starting from what you suggested, here's what I came up with.
> > I also added a few test cases to cover corner cases.
> > 
> > 	if (scale_type == IIO_VAL_INT_PLUS_NANO)
> > 		mult = 1000000000LL;
> > 	else
> > 		mult = 1000000LL;
> > 	/*
> > 	 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
> > 	 * *val2 is negative the schan scale is negative
> > 	 */
> > 	neg = *val < 0 || *val2 < 0;
> > 
> > 	tmp = (s64)abs(*val) * (s32)abs(rescale->numerator);
>
> Small nit, but I think abs() returns a signed type compatible
> with the argument type. I.e. (s32)abs(rescale->...) where both
> numerator and denominator are already s32 could just as well
> be written without the cast as plain old abs(rescale->...)

Understood, I'll get rid of the redundant typecasts

>
>
> > 	*val = div_s64_rem(tmp, (s32)abs(rescale->denominator), &rem);
> > 
> > 	tmp = (s64)rem * mult +
> > 		(s64)abs(*val2) * (s32)abs(rescale->numerator);
> > 	tmp = div_s64(tmp, (s32)abs(rescale->denominator));
> > 
> > 	*val += div_s64_rem(tmp, mult, val2);
> > 
> > 	/*
> > 	 * If the schan scale or only one of the rescaler elements is
> > 	 * negative, the combined scale is negative.
> > 	 */
> > 	if (neg || ((rescale->numerator < 0) ^ (rescale->denominator < 0)))
> > 		*val = -*val;
>
> Unconditionally negating *val doesn't negate the combined value when
> *val is zero and *val2 isn't. My test "if (*val < 0)" above attempting
> to take care of this case is clearly not right. It should of course be
> "if (*val > 0)" since *val is not yet negated. Duh!

Oh I see, thanks for pointing that out. Since at that point *val can't
be negative because of all the abs() calls, we could also just check
that *val is not zero.

>
> In fact, I think a few tests scaling to/from the [-1,1] interval
> would be benefitial for this exact reason.

Sounds good, I'll add a few more cases for this.
Thanks,
Liam

>
> Cheers,
> Peter


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-30  7:01             ` Peter Rosin
@ 2021-07-30 20:01               ` Liam Beguin
  0 siblings, 0 replies; 33+ messages in thread
From: Liam Beguin @ 2021-07-30 20:01 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Fri Jul 30, 2021 at 3:01 AM EDT, Peter Rosin wrote:
> On 2021-07-30 08:49, Peter Rosin wrote:
> > On 2021-07-29 17:56, Liam Beguin wrote:
> >> On Wed Jul 28, 2021 at 3:19 AM EDT, Peter Rosin wrote:
> >>> On 2021-07-28 02:21, Liam Beguin wrote:
> >>>> On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:
> >>>>> On 2021-07-21 05:06, Liam Beguin wrote:
> >>>>>> From: Liam Beguin <lvb@xiphos.com>
> >>>>>>
> >>>>>> 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 <lvb@xiphos.com>
> >>>>>> ---
> >>>>>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
> >>>>>>  1 file changed, 14 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>>>>> index d0669fd8eac5..2b73047365cc 100644
> >>>>>> --- a/drivers/iio/afe/iio-rescale.c
> >>>>>> +++ b/drivers/iio/afe/iio-rescale.c
> >>>>>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >>>>>>  		do_div(tmp, 1000000000LL);
> >>>>>>  		*val = tmp;
> >>>>>>  		return scale_type;
> >>>>>> +	case IIO_VAL_INT_PLUS_NANO:
> >>>>>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >>>>>> +		tmp = div_s64(tmp, rescale->denominator);
> >>>>>> +
> >>>>>> +		*val = div_s64(tmp, 1000000000LL);
> >>>>>> +		*val2 = tmp - *val * 1000000000LL;
> >>>>>> +		return scale_type;
> >>>>
> >>>> Hi Peter,
> >>>>
> >>>>>
> >>>>> Hi!
> >>>>>
> >>>>> My objection from v5 still stands. Did you forget or did you simply send
> >>>>> the
> >>>>> wrong patch?
> >>>>
> >>>> Apologies, again I didn't mean to make it seem like I ignored your comments.
> >>>> I tried your suggestion, but had issues when *val2 would overflow into
> >>>> the integer part.
> >>
> >> Hi Peter,
> >>
> >>>
> >>> Not saying anything about it not working does indeed make it seem like
> >>> you
> >>> ignored it :-) Or did I just miss where you said this? Anyway, no
> >>> problem,
> >>> it can be a mess dealing with a string of commits when there are
> >>> numerous
> >>> things to take care of between each iteration. And it's very easy to
> >>> burn
> >>> out and just back away. Please don't do that!
> >>
> >> It was my mistake. Thanks for the encouragement :-)
> >>
> >>>
> >>>> Even though what I has was more prone to integer overflow with the first
> >>>> multiplication, I thought it was still a valid solution as it passed the
> >>>> tests.
> >>>
> >>> I did state that you'd need to add overflow handling from the fraction
> >>> calculation and handling for negative values, so it was no surprise that
> >>> my original sketchy suggestion didn't work as-is.
> >>>
> >>>>
> >>>>>
> >>>>> Untested suggestion, this time handling negative values and
> >>>>> canonicalizing any
> >>>>> overflow from the fraction calculation.
> >>>>>
> >>>>> neg = *val < 0 || *val2 < 0;
> >>>>> tmp = (s64)abs(*val) * rescale->numerator;
> >>>>> rem = do_div(tmp, rescale->denominator);
> >>>>> *val = tmp;
> >>>>> tmp = rem * 1000000000LL + (s64)abs(*val2) * rescale->numerator;
> >>>>> do_div(tmp, rescale->denominator);
> >>>>> *val2 = do_div(tmp, 1000000000LL);
> >>>>> *val += tmp;
> >>>>> if (neg) {
> >>>>> if (*val < 0)
> >>>>> *val = -*val;
> >>>>> else
> >>>>> *val2 = -*val;
> >>>
> >>> This last line should of course be *val2 = -*val2;
> >>> Sorry.
> >>>
> >>>>
> >>>> I'll look into this suggestion.
> >>>
> >>> Thanks!
> >>>
> >>
> >> Starting from what you suggested, here's what I came up with.
> >> I also added a few test cases to cover corner cases.
> >>
> >> 	if (scale_type == IIO_VAL_INT_PLUS_NANO)
> >> 		mult = 1000000000LL;
> >> 	else
> >> 		mult = 1000000LL;
> >> 	/*
> >> 	 * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if *val OR
> >> 	 * *val2 is negative the schan scale is negative
> >> 	 */
> >> 	neg = *val < 0 || *val2 < 0;
> >>
> >> 	tmp = (s64)abs(*val) * (s32)abs(rescale->numerator);
> > 
> > Small nit, but I think abs() returns a signed type compatible
> > with the argument type. I.e. (s32)abs(rescale->...) where both
> > numerator and denominator are already s32 could just as well
> > be written without the cast as plain old abs(rescale->...)
> > 
> > 
> >> 	*val = div_s64_rem(tmp, (s32)abs(rescale->denominator), &rem);
> >>
> >> 	tmp = (s64)rem * mult +
> >> 		(s64)abs(*val2) * (s32)abs(rescale->numerator);
> >> 	tmp = div_s64(tmp, (s32)abs(rescale->denominator));
> >>
> >> 	*val += div_s64_rem(tmp, mult, val2);
> >>
> >> 	/*
> >> 	 * If the schan scale or only one of the rescaler elements is
> >> 	 * negative, the combined scale is negative.
> >> 	 */
> >> 	if (neg || ((rescale->numerator < 0) ^ (rescale->denominator < 0)))
>
> Hang on, that's not right. If the value and only one of the rescaler
> elements is negative, the result is positive. || is not the correct
> logical operation.
>
> >> 		*val = -*val;
> > 
> > Unconditionally negating *val doesn't negate the combined value when
> > *val is zero and *val2 isn't. My test "if (*val < 0)" above attempting
> > to take care of this case is clearly not right. It should of course be
> > "if (*val > 0)" since *val is not yet negated. Duh!
> > 
> > In fact, I think a few tests scaling to/from the [-1,1] interval
> > would be benefitial for this exact reason.
>
> So, with both these issues taken care of:
>
> if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
> if (*val > 0)
> *val = -*val;
> else
> *val2 = -*val2;
> }
>
> (bitwise ^ is safe since all operands come from logical operations, i.e.
> they are either zero or one and nothing else)

You're right, this should've been a ^ from the start.

Thanks,
Liam

>
> Cheers,
> Peter


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-28  7:19       ` Peter Rosin
  2021-07-29 15:56         ` Liam Beguin
@ 2021-07-31 17:47         ` Jonathan Cameron
  1 sibling, 0 replies; 33+ messages in thread
From: Jonathan Cameron @ 2021-07-31 17:47 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Liam Beguin, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt

On Wed, 28 Jul 2021 09:19:58 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2021-07-28 02:21, Liam Beguin wrote:
> > On Fri Jul 23, 2021 at 5:16 PM EDT, Peter Rosin wrote:  
> >> On 2021-07-21 05:06, Liam Beguin wrote:  
> >>> From: Liam Beguin <lvb@xiphos.com>
> >>>
> >>> 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 <lvb@xiphos.com>
> >>> ---
> >>>  drivers/iio/afe/iio-rescale.c | 14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>> index d0669fd8eac5..2b73047365cc 100644
> >>> --- a/drivers/iio/afe/iio-rescale.c
> >>> +++ b/drivers/iio/afe/iio-rescale.c
> >>> @@ -41,6 +41,20 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >>>  		do_div(tmp, 1000000000LL);
> >>>  		*val = tmp;
> >>>  		return scale_type;
> >>> +	case IIO_VAL_INT_PLUS_NANO:
> >>> +		tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >>> +		tmp = div_s64(tmp, rescale->denominator);
> >>> +
> >>> +		*val = div_s64(tmp, 1000000000LL);
> >>> +		*val2 = tmp - *val * 1000000000LL;
> >>> +		return scale_type;  
> > 
> > Hi Peter,
> >   
> >>
> >> Hi!
> >>
> >> My objection from v5 still stands. Did you forget or did you simply send
> >> the
> >> wrong patch?  
> > 
> > Apologies, again I didn't mean to make it seem like I ignored your comments.
> > I tried your suggestion, but had issues when *val2 would overflow into
> > the integer part.  
> 
> Not saying anything about it not working does indeed make it seem like you
> ignored it :-)  Or did I just miss where you said this? Anyway, no problem,
> it can be a mess dealing with a string of commits when there are numerous
> things to take care of between each iteration. And it's very easy to burn
> out and just back away. Please don't do that!

Just to add here, I'm really appreciating the two of you figuring this out
between you and looking forward to getting the resulting improvements (particularly
the tests!) in place.

Thanks,

Jonathan

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2021-07-31 17:45 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21  3:06 [PATCH v6 00/13] iio: afe: add temperature rescaling support Liam Beguin
2021-07-21  3:06 ` [PATCH v6 01/13] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-07-21  3:06 ` [PATCH v6 02/13] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
2021-07-21  3:06 ` [PATCH v6 03/13] iio: inkern: make a best effort on offset calculation Liam Beguin
2021-07-21  3:06 ` [PATCH v6 04/13] iio: afe: rescale: expose scale processing function Liam Beguin
2021-07-21  3:06 ` [PATCH v6 05/13] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
2021-07-23 21:16   ` Peter Rosin
2021-07-28  0:21     ` Liam Beguin
2021-07-28  7:19       ` Peter Rosin
2021-07-29 15:56         ` Liam Beguin
2021-07-30  6:49           ` Peter Rosin
2021-07-30  7:01             ` Peter Rosin
2021-07-30 20:01               ` Liam Beguin
2021-07-30 19:57             ` Liam Beguin
2021-07-31 17:47         ` Jonathan Cameron
2021-07-21  3:06 ` [PATCH v6 06/13] iio: afe: rescale: add offset support Liam Beguin
2021-07-21  3:06 ` [PATCH v6 07/13] iio: test: add basic tests for the iio-rescale driver Liam Beguin
2021-07-24  8:40   ` kernel test robot
2021-07-21  3:06 ` [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
2021-07-23 21:17   ` Peter Rosin
2021-07-28  0:07     ` Liam Beguin
2021-07-28  7:47       ` Peter Rosin
2021-07-29 16:02         ` Liam Beguin
2021-07-21  3:06 ` [PATCH v6 09/13] iio: afe: rescale: fix precision on fractional log scale Liam Beguin
2021-07-23 21:20   ` Peter Rosin
2021-07-28  0:26     ` Liam Beguin
2021-07-28  7:58       ` Peter Rosin
2021-07-29 16:19         ` Liam Beguin
2021-07-21  3:06 ` [PATCH v6 10/13] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
2021-07-21  3:06 ` [PATCH v6 11/13] iio: afe: rescale: add temperature transducers Liam Beguin
2021-07-21  3:06 ` [PATCH v6 12/13] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-07-21  3:06 ` [PATCH v6 13/13] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
2021-07-23 22:59   ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).