linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 00/10] iio: afe: add temperature rescaling support
@ 2021-07-15  3:12 Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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 two
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 was suggested by Jonathan [1], I started implementing Kunit tests for
some of these cases[2]. It's pretty far from being ready but it still
helped test things faster this time around!

I'll send another series with the tests once I've cleaned it up and
figured out how to avoid copying part of the driver...

[1] https://patchwork.kernel.org/project/linux-iio/patch/20210701010034.303088-5-liambeguin@gmail.com/#24290449
[2] https://git.sr.ht/~liambeguin/Linux/commit/iio-rescale-test/v1

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 (10):
  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: reduce risk of integer overflow
  iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  iio: afe: rescale: add offset support
  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                 | 183 +++++++++++++++++-
 drivers/iio/inkern.c                          |  40 +++-
 4 files changed, 426 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
 create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml

Range-diff against v4:
 -:  ------------ >  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
 1:  e23e6cb26b92 !  4:  7b3e374eb7ad iio: afe: rescale: reduce risk of integer overflow
    @@ Commit message
     
         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.
    +    of the fractional value when required.
     
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
     
    @@ drivers/iio/afe/iio-rescale.c: static int rescale_read_raw(struct iio_dev *indio
     -			*val2 *= rescale->denominator;
     +			tmp = (s64)*val * rescale->numerator;
     +			tmp2 = (s64)*val2 * rescale->denominator;
    -+			factor = gcd(tmp, tmp2);
    -+			do_div(tmp, factor);
    ++			if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
    ++			check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
    ++				factor = gcd(tmp, tmp2);
    ++				do_div(tmp, factor);
    ++				do_div(tmp2, factor);
    ++			}
     +			*val = tmp;
    -+			do_div(tmp2, factor);
     +			*val2 = tmp2;
      			return ret;
      		case IIO_VAL_INT:
 2:  28203b672942 !  5:  1d334090e974 iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
    @@ Metadata
      ## Commit message ##
         iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
     
    -    Add IIO_VAL_INT_PLUS_{NANO,MICRO} scaling support.
    -    Scale the integer part and the decimal parts individually and keep the
    -    original scaling type.
    +    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: static int rescale_read_raw(struct iio_dev *indio
      			*val = tmp;
      			return ret;
     +		case IIO_VAL_INT_PLUS_NANO:
    ++			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
    ++			do_div(tmp, rescale->denominator);
    ++
    ++			*val = div_s64(tmp, 1000000000LL);
    ++			*val2 = tmp - *val * 1000000000LL;
    ++			return ret;
     +		case IIO_VAL_INT_PLUS_MICRO:
    -+			tmp = (s64)*val * rescale->numerator;
    -+			*val = div_s64(tmp, rescale->denominator);
    -+			tmp = (s64)*val2 * rescale->numerator;
    -+			*val2 = div_s64(tmp, rescale->denominator);
    ++			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
    ++			do_div(tmp, rescale->denominator);
    ++
    ++			*val = div_s64(tmp, 1000000LL);
    ++			*val2 = tmp - *val * 1000000LL;
     +			return ret;
      		default:
     +			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
 3:  a6c944ae0f99 =  6:  61873203c140 iio: afe: rescale: add offset support
 4:  cc5eb96512d5 =  7:  4e6117b9c663 iio: afe: rescale: add RTD temperature sensor support
 5:  d8aa257aad35 =  8:  bc647d45e293 iio: afe: rescale: add temperature transducers
 6:  f038d6a08ea2 !  9:  570b418eed85 dt-bindings: iio: afe: add bindings for temperature-sense-rtd
    @@ Commit message
         voltage across an RTD resistor such as a PT1000.
     
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
    +    Reviewed-by: Rob Herring <robh@kernel.org>
     
      ## Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml (new) ##
     @@
 7:  1db42cb25254 ! 10:  3c44ea89754e dt-bindings: iio: afe: add bindings for temperature transducers
    @@ Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml (new)
     +
     +  sense-offset-millicelsius:
     +    description: |
    -+      Temperature offset. The default is <0>.
    ++      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. Defaults to <1>.
    -+      Set sense-resistor-ohms to <1> when using a temperature to voltage
    -+      transducer.
    ++      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: |
    @@ Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml (new)
     +      datasheet.
     +
     +additionalProperties: false
    ++
     +required:
     +  - compatible
     +  - io-channels

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.30.1.489.g328c10930387


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

* [PATCH v5 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 02/10] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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	[flat|nested] 20+ messages in thread

* [PATCH v5 02/10] iio: inkern: apply consumer scale when no channel scale is available
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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	[flat|nested] 20+ messages in thread

* [PATCH v5 03/10] iio: inkern: make a best effort on offset calculation
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 02/10] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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	[flat|nested] 20+ messages in thread

* [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (2 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15 10:23   ` Peter Rosin
  2021-07-15  3:12 ` [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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 774eb3044edd..4c3cfd4d5181 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	struct rescale *rescale = iio_priv(indio_dev);
-	unsigned long long tmp;
+	s64 tmp, tmp2;
+	u32 factor;
 	int ret;
 
 	switch (mask) {
@@ -67,8 +68,16 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 		}
 		switch (ret) {
 		case IIO_VAL_FRACTIONAL:
-			*val *= rescale->numerator;
-			*val2 *= rescale->denominator;
+			tmp = (s64)*val * rescale->numerator;
+			tmp2 = (s64)*val2 * rescale->denominator;
+			if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
+			check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {
+				factor = gcd(tmp, tmp2);
+				do_div(tmp, factor);
+				do_div(tmp2, factor);
+			}
+			*val = tmp;
+			*val2 = tmp2;
 			return ret;
 		case IIO_VAL_INT:
 			*val *= rescale->numerator;
-- 
2.30.1.489.g328c10930387


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

* [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (3 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  9:48   ` Peter Rosin
  2021-07-15  3:12 ` [PATCH v5 06/10] iio: afe: rescale: add offset support Liam Beguin
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 4c3cfd4d5181..a2b220b5ba86 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 			do_div(tmp, 1000000000LL);
 			*val = tmp;
 			return ret;
+		case IIO_VAL_INT_PLUS_NANO:
+			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
+			do_div(tmp, rescale->denominator);
+
+			*val = div_s64(tmp, 1000000000LL);
+			*val2 = tmp - *val * 1000000000LL;
+			return ret;
+		case IIO_VAL_INT_PLUS_MICRO:
+			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
+			do_div(tmp, rescale->denominator);
+
+			*val = div_s64(tmp, 1000000LL);
+			*val2 = tmp - *val * 1000000LL;
+			return ret;
 		default:
+			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
 			return -EOPNOTSUPP;
 		}
 	default:
-- 
2.30.1.489.g328c10930387


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

* [PATCH v5 06/10] iio: afe: rescale: add offset support
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (4 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 07/10] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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 | 72 +++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index a2b220b5ba86..4d3b44884a89 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -32,6 +32,7 @@ struct rescale {
 	bool chan_processed;
 	s32 numerator;
 	s32 denominator;
+	s32 offset;
 };
 
 static int rescale_read_raw(struct iio_dev *indio_dev,
@@ -39,6 +40,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 			    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;
@@ -110,6 +113,71 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
 			return -EOPNOTSUPP;
 		}
+	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);
+		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;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -186,6 +254,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
@@ -350,6 +421,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)
-- 
2.30.1.489.g328c10930387


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

* [PATCH v5 07/10] iio: afe: rescale: add RTD temperature sensor support
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (5 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 06/10] iio: afe: rescale: add offset support Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 08/10] iio: afe: rescale: add temperature transducers Liam Beguin
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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 4d3b44884a89..055f6b7c9869 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -356,10 +356,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[] = {
@@ -375,6 +417,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[] = {
@@ -384,6 +430,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	[flat|nested] 20+ messages in thread

* [PATCH v5 08/10] iio: afe: rescale: add temperature transducers
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (6 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 07/10] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 10/10] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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 055f6b7c9869..4b3b5e9bbe40 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -397,11 +397,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[] = {
@@ -421,6 +448,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[] = {
@@ -432,6 +463,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	[flat|nested] 20+ messages in thread

* [PATCH v5 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (7 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 08/10] iio: afe: rescale: add temperature transducers Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  2021-07-15  3:12 ` [PATCH v5 10/10] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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..11a3d4bd3065
--- /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 <lvb@xiphos.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	[flat|nested] 20+ messages in thread

* [PATCH v5 10/10] dt-bindings: iio: afe: add bindings for temperature transducers
  2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
                   ` (8 preceding siblings ...)
  2021-07-15  3:12 ` [PATCH v5 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
@ 2021-07-15  3:12 ` Liam Beguin
  9 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-15  3:12 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..41411514aee0
--- /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 <lvb@xiphos.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	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-15  3:12 ` [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
@ 2021-07-15  9:48   ` Peter Rosin
  2021-07-16 19:18     ` Liam Beguin
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Rosin @ 2021-07-15  9:48 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt


On 2021-07-15 05:12, 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 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 4c3cfd4d5181..a2b220b5ba86 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  			do_div(tmp, 1000000000LL);
>  			*val = tmp;
>  			return ret;
> +		case IIO_VAL_INT_PLUS_NANO:
> +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> +			do_div(tmp, rescale->denominator);
> +
> +			*val = div_s64(tmp, 1000000000LL);
> +			*val2 = tmp - *val * 1000000000LL;
> +			return ret;

This is too simplistic and prone to overflow. We need something like this
(untested)

	tmp = (s64)*val * rescale->numerator;
	rem = do_div(tmp, rescale->denominator);
	*val = tmp;
	tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
	do_div(tmp, rescale->denominator);
	*val2 = tmp;

Still not very safe with numerator and denominator both "large", but much
better. And then we need normalizing the fraction part after the above, of
course.

And, of course, I'm not sure what *val == -1 and *val2 == 500000000 really
means. Is that -1.5 or -0.5? The above may very well need adjusting for
negative values...

Cheers,
Peter

> +		case IIO_VAL_INT_PLUS_MICRO:
> +			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> +			do_div(tmp, rescale->denominator);
> +
> +			*val = div_s64(tmp, 1000000LL);
> +			*val2 = tmp - *val * 1000000LL;
> +			return ret;
>  		default:
> +			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
>  			return -EOPNOTSUPP;
>  		}
>  	default:
> 

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

* Re: [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow
  2021-07-15  3:12 ` [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
@ 2021-07-15 10:23   ` Peter Rosin
  2021-07-16 16:46     ` Liam Beguin
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Rosin @ 2021-07-15 10:23 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On 2021-07-15 05:12, 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 774eb3044edd..4c3cfd4d5181 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct rescale *rescale = iio_priv(indio_dev);
> -	unsigned long long tmp;
> +	s64 tmp, tmp2;
> +	u32 factor;
>  	int ret;
>  
>  	switch (mask) {
> @@ -67,8 +68,16 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  		}
>  		switch (ret) {
>  		case IIO_VAL_FRACTIONAL:
> -			*val *= rescale->numerator;
> -			*val2 *= rescale->denominator;
> +			tmp = (s64)*val * rescale->numerator;
> +			tmp2 = (s64)*val2 * rescale->denominator;
> +			if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
> +			check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {

The white space should be like this, methinks.

			if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
			    check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2))
			{

> +				factor = gcd(tmp, tmp2);

And I just realized, gcd() works on unsigned values which is a bit safer for the
scale factor. But here, for the actual values, more care is needed.

> +				do_div(tmp, factor);
> +				do_div(tmp2, factor);
> +			}
> +			*val = tmp;
> +			*val2 = tmp2;

And beside the above points, the whole mechanism seems broken. The returned value
in the third argument to check_mul_overflow isn't useful if there is an overflow.
Yet, the code continues to use tmp and tmp2 in case of overflow. And why do you
first multiply tmp and tmp2 without checks, only to then do the same mul again
but with checks? Or have I completely misunderstood how check_mul_overflow
works?

Cheers,
Peter

>  			return ret;
>  		case IIO_VAL_INT:
>  			*val *= rescale->numerator;
> 

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

* Re: [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow
  2021-07-15 10:23   ` Peter Rosin
@ 2021-07-16 16:46     ` Liam Beguin
  0 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-16 16:46 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Thu Jul 15, 2021 at 6:23 AM EDT, Peter Rosin wrote:
> On 2021-07-15 05:12, 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 774eb3044edd..4c3cfd4d5181 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> >  			    int *val, int *val2, long mask)
> >  {
> >  	struct rescale *rescale = iio_priv(indio_dev);
> > -	unsigned long long tmp;
> > +	s64 tmp, tmp2;
> > +	u32 factor;
> >  	int ret;
> >  
> >  	switch (mask) {
> > @@ -67,8 +68,16 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> >  		}
> >  		switch (ret) {
> >  		case IIO_VAL_FRACTIONAL:
> > -			*val *= rescale->numerator;
> > -			*val2 *= rescale->denominator;
> > +			tmp = (s64)*val * rescale->numerator;
> > +			tmp2 = (s64)*val2 * rescale->denominator;
> > +			if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
> > +			check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) {

Hi Peter,

>
> The white space should be like this, methinks.
>
> if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) ||
> check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2))
> {
>

Sorry about that... Like I said in the cover letter, I'm working on
getting kunit tests running for the iio-rescale. At the moment it still
requires copying part of the code over and sure enough I forgot to copy
some of it back. My apologies for the noise...

This is what I meant to send:

case IIO_VAL_FRACTIONAL:
	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);
		do_div(tmp, factor);
		do_div(tmp2, factor);
	}

	*val = tmp;
	*val2 = tmp2;
	return ret;

I'll also move the opening bracket on a new line if you prefer.

> > +				factor = gcd(tmp, tmp2);
>
> And I just realized, gcd() works on unsigned values which is a bit safer
> for the
> scale factor. But here, for the actual values, more care is needed.
>

I added negative test cases to take this into account. I'll update and
resend. I'm going to find a way to get the test cases ready for the next
revision.

> > +				do_div(tmp, factor);
> > +				do_div(tmp2, factor);
> > +			}
> > +			*val = tmp;
> > +			*val2 = tmp2;
>
> And beside the above points, the whole mechanism seems broken. The
> returned value
> in the third argument to check_mul_overflow isn't useful if there is an
> overflow.
> Yet, the code continues to use tmp and tmp2 in case of overflow. And why
> do you
> first multiply tmp and tmp2 without checks, only to then do the same mul
> again
> but with checks? Or have I completely misunderstood how
> check_mul_overflow
> works?
>

Again, my apologies for this. It's not what I meant to send.
Hopefully the snippet above makes more sense.

Thanks for your time,
Liam

> Cheers,
> Peter
>
> >  			return ret;
> >  		case IIO_VAL_INT:
> >  			*val *= rescale->numerator;
> > 


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

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-15  9:48   ` Peter Rosin
@ 2021-07-16 19:18     ` Liam Beguin
  2021-07-17  8:11       ` Peter Rosin
  2021-07-17 16:55       ` Jonathan Cameron
  0 siblings, 2 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-16 19:18 UTC (permalink / raw)
  To: Peter Rosin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt

On Thu Jul 15, 2021 at 5:48 AM EDT, Peter Rosin wrote:
>
> On 2021-07-15 05:12, 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 | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 4c3cfd4d5181..a2b220b5ba86 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> >  			do_div(tmp, 1000000000LL);
> >  			*val = tmp;
> >  			return ret;
> > +		case IIO_VAL_INT_PLUS_NANO:
> > +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> > +			do_div(tmp, rescale->denominator);
> > +
> > +			*val = div_s64(tmp, 1000000000LL);
> > +			*val2 = tmp - *val * 1000000000LL;
> > +			return ret;
>
> This is too simplistic and prone to overflow. We need something like
> this
> (untested)
>
> tmp = (s64)*val * rescale->numerator;
> rem = do_div(tmp, rescale->denominator);
> *val = tmp;
> tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
> do_div(tmp, rescale->denominator);
> *val2 = tmp;
>
> Still not very safe with numerator and denominator both "large", but
> much
> better. And then we need normalizing the fraction part after the above,
> of
> course.
>

Understood, I'll test that.

> And, of course, I'm not sure what *val == -1 and *val2 == 500000000
> really
> means. Is that -1.5 or -0.5? The above may very well need adjusting for
> negative values...
>

I would've assumed the correct answer is -1 + 500000000e-9 = -0.5
but adding a test case to iio-test-format.c seems to return -1.5...

I believe that's a bug but we can work around if for now by moving the
integer part of *val2 to *val.

Liam

> Cheers,
> Peter
>
> > +		case IIO_VAL_INT_PLUS_MICRO:
> > +			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> > +			do_div(tmp, rescale->denominator);
> > +
> > +			*val = div_s64(tmp, 1000000LL);
> > +			*val2 = tmp - *val * 1000000LL;
> > +			return ret;
> >  		default:
> > +			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
> >  			return -EOPNOTSUPP;
> >  		}
> >  	default:
> > 


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

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-16 19:18     ` Liam Beguin
@ 2021-07-17  8:11       ` Peter Rosin
  2021-07-17 16:55       ` Jonathan Cameron
  1 sibling, 0 replies; 20+ messages in thread
From: Peter Rosin @ 2021-07-17  8:11 UTC (permalink / raw)
  To: Liam Beguin, jic23, lars, pmeerw
  Cc: linux-kernel, linux-iio, devicetree, robh+dt



On 2021-07-16 21:18, Liam Beguin wrote:
> On Thu Jul 15, 2021 at 5:48 AM EDT, Peter Rosin wrote:
>>
>> On 2021-07-15 05:12, 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 | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index 4c3cfd4d5181..a2b220b5ba86 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>>>  			do_div(tmp, 1000000000LL);
>>>  			*val = tmp;
>>>  			return ret;
>>> +		case IIO_VAL_INT_PLUS_NANO:
>>> +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>> +			do_div(tmp, rescale->denominator);
>>> +
>>> +			*val = div_s64(tmp, 1000000000LL);
>>> +			*val2 = tmp - *val * 1000000000LL;
>>> +			return ret;
>>
>> This is too simplistic and prone to overflow. We need something like
>> this
>> (untested)
>>
>> tmp = (s64)*val * rescale->numerator;
>> rem = do_div(tmp, rescale->denominator);
>> *val = tmp;
>> tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
>> do_div(tmp, rescale->denominator);
>> *val2 = tmp;
>>
>> Still not very safe with numerator and denominator both "large", but
>> much
>> better. And then we need normalizing the fraction part after the above,
>> of
>> course.
>>
> 
> Understood, I'll test that.

I made a thinko. The remainder should not be re-multiplied with the
numerator...

	tmp = (s64)*val * rescale->numerator;
	rem = do_div(tmp, rescale->denominator);
	*val = tmp;
	tmp = (s64)rem * 1000000000LL + (s64)*val2 * rescale->numerator;
	do_div(tmp, rescale->denominator);
	*val2 = tmp;

And that actually reduces the risk of overflow too, which is nice!

Cheers,
Peter

>> And, of course, I'm not sure what *val == -1 and *val2 == 500000000
>> really
>> means. Is that -1.5 or -0.5? The above may very well need adjusting for
>> negative values...
>>
> 
> I would've assumed the correct answer is -1 + 500000000e-9 = -0.5
> but adding a test case to iio-test-format.c seems to return -1.5...
> 
> I believe that's a bug but we can work around if for now by moving the
> integer part of *val2 to *val.
> 
> Liam
> 
>> Cheers,
>> Peter
>>
>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>> +			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
>>> +			do_div(tmp, rescale->denominator);
>>> +
>>> +			*val = div_s64(tmp, 1000000LL);
>>> +			*val2 = tmp - *val * 1000000LL;
>>> +			return ret;
>>>  		default:
>>> +			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
>>>  			return -EOPNOTSUPP;
>>>  		}
>>>  	default:
>>>
> 

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

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-16 19:18     ` Liam Beguin
  2021-07-17  8:11       ` Peter Rosin
@ 2021-07-17 16:55       ` Jonathan Cameron
  2021-07-18 23:44         ` Liam Beguin
  1 sibling, 1 reply; 20+ messages in thread
From: Jonathan Cameron @ 2021-07-17 16:55 UTC (permalink / raw)
  To: Liam Beguin
  Cc: Peter Rosin, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt

On Fri, 16 Jul 2021 15:18:33 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> On Thu Jul 15, 2021 at 5:48 AM EDT, Peter Rosin wrote:
> >
> > On 2021-07-15 05:12, 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 | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > > index 4c3cfd4d5181..a2b220b5ba86 100644
> > > --- a/drivers/iio/afe/iio-rescale.c
> > > +++ b/drivers/iio/afe/iio-rescale.c
> > > @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> > >  			do_div(tmp, 1000000000LL);
> > >  			*val = tmp;
> > >  			return ret;
> > > +		case IIO_VAL_INT_PLUS_NANO:
> > > +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> > > +			do_div(tmp, rescale->denominator);
> > > +
> > > +			*val = div_s64(tmp, 1000000000LL);
> > > +			*val2 = tmp - *val * 1000000000LL;
> > > +			return ret;  
> >
> > This is too simplistic and prone to overflow. We need something like
> > this
> > (untested)
> >
> > tmp = (s64)*val * rescale->numerator;
> > rem = do_div(tmp, rescale->denominator);
> > *val = tmp;
> > tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
> > do_div(tmp, rescale->denominator);
> > *val2 = tmp;
> >
> > Still not very safe with numerator and denominator both "large", but
> > much
> > better. And then we need normalizing the fraction part after the above,
> > of
> > course.
> >  
> 
> Understood, I'll test that.
> 
> > And, of course, I'm not sure what *val == -1 and *val2 == 500000000
> > really
> > means. Is that -1.5 or -0.5? The above may very well need adjusting for
> > negative values...
> >  
> 
> I would've assumed the correct answer is -1 + 500000000e-9 = -0.5
> but adding a test case to iio-test-format.c seems to return -1.5...

No. -1.5 is as intended, though the IIO_VAL_PLUS_MICRO is rather confusing
naming :( We should perhaps add more documentation for that.  Signs were
always a bit of a pain with this two integer scheme for fixed point.

The intent is to have moderately readable look up tables with the problem that
we don't have a signed 0 available.  Meh, maybe this decision a long time
back wasn't a the right one, but it may be a pain to change now as too many
drivers to check!

1, 0000000  == 1
0, 5000000  == 0.5
0, 0000000  == 0
0, -5000000 == -0.5
-1, 5000000 == -1.5


> 
> I believe that's a bug but we can work around if for now by moving the
> integer part of *val2 to *val.

Yup.  Fiddly corner cases..

Jonathan

> 
> Liam
> 
> > Cheers,
> > Peter
> >  
> > > +		case IIO_VAL_INT_PLUS_MICRO:
> > > +			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> > > +			do_div(tmp, rescale->denominator);
> > > +
> > > +			*val = div_s64(tmp, 1000000LL);
> > > +			*val2 = tmp - *val * 1000000LL;
> > > +			return ret;
> > >  		default:
> > > +			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
> > >  			return -EOPNOTSUPP;
> > >  		}
> > >  	default:
> > >   
> 


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

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-17 16:55       ` Jonathan Cameron
@ 2021-07-18 23:44         ` Liam Beguin
  2021-07-19  8:31           ` Peter Rosin
  0 siblings, 1 reply; 20+ messages in thread
From: Liam Beguin @ 2021-07-18 23:44 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Peter Rosin, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt

On Sat Jul 17, 2021 at 12:55 PM EDT, Jonathan Cameron wrote:
> On Fri, 16 Jul 2021 15:18:33 -0400
> "Liam Beguin" <liambeguin@gmail.com> wrote:
>
> > On Thu Jul 15, 2021 at 5:48 AM EDT, Peter Rosin wrote:
> > >
> > > On 2021-07-15 05:12, 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 | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > > > index 4c3cfd4d5181..a2b220b5ba86 100644
> > > > --- a/drivers/iio/afe/iio-rescale.c
> > > > +++ b/drivers/iio/afe/iio-rescale.c
> > > > @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> > > >  			do_div(tmp, 1000000000LL);
> > > >  			*val = tmp;
> > > >  			return ret;
> > > > +		case IIO_VAL_INT_PLUS_NANO:
> > > > +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> > > > +			do_div(tmp, rescale->denominator);
> > > > +
> > > > +			*val = div_s64(tmp, 1000000000LL);
> > > > +			*val2 = tmp - *val * 1000000000LL;
> > > > +			return ret;  
> > >
> > > This is too simplistic and prone to overflow. We need something like
> > > this
> > > (untested)
> > >
> > > tmp = (s64)*val * rescale->numerator;
> > > rem = do_div(tmp, rescale->denominator);
> > > *val = tmp;
> > > tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
> > > do_div(tmp, rescale->denominator);
> > > *val2 = tmp;
> > >
> > > Still not very safe with numerator and denominator both "large", but
> > > much
> > > better. And then we need normalizing the fraction part after the above,
> > > of
> > > course.
> > >  
> > 
> > Understood, I'll test that.
> > 
> > > And, of course, I'm not sure what *val == -1 and *val2 == 500000000
> > > really
> > > means. Is that -1.5 or -0.5? The above may very well need adjusting for
> > > negative values...
> > >  
> > 
> > I would've assumed the correct answer is -1 + 500000000e-9 = -0.5
> > but adding a test case to iio-test-format.c seems to return -1.5...
>

Hi Jonathan,

> No. -1.5 is as intended, though the IIO_VAL_PLUS_MICRO is rather
> confusing
> naming :( We should perhaps add more documentation for that. Signs were
> always a bit of a pain with this two integer scheme for fixed point.
>
> The intent is to have moderately readable look up tables with the
> problem that
> we don't have a signed 0 available. Meh, maybe this decision a long time
> back wasn't a the right one, but it may be a pain to change now as too
> many
> drivers to check!
>
> 1, 0000000 == 1
> 0, 5000000 == 0.5
> 0, 0000000 == 0
> 0, -5000000 == -0.5
> -1, 5000000 == -1.5
>

Understood, thanks for clearing that out.

Liam

>
> > 
> > I believe that's a bug but we can work around if for now by moving the
> > integer part of *val2 to *val.
>
> Yup. Fiddly corner cases..
>
> Jonathan
>
> > 
> > Liam
> > 
> > > Cheers,
> > > Peter
> > >  
> > > > +		case IIO_VAL_INT_PLUS_MICRO:
> > > > +			tmp = ((s64)*val * 1000000LL + *val2) * rescale->numerator;
> > > > +			do_div(tmp, rescale->denominator);
> > > > +
> > > > +			*val = div_s64(tmp, 1000000LL);
> > > > +			*val2 = tmp - *val * 1000000LL;
> > > > +			return ret;
> > > >  		default:
> > > > +			dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
> > > >  			return -EOPNOTSUPP;
> > > >  		}
> > > >  	default:
> > > >   
> > 


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

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-18 23:44         ` Liam Beguin
@ 2021-07-19  8:31           ` Peter Rosin
  2021-07-19 15:15             ` Liam Beguin
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Rosin @ 2021-07-19  8:31 UTC (permalink / raw)
  To: Liam Beguin, Jonathan Cameron
  Cc: lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt



On 2021-07-19 01:44, Liam Beguin wrote:
> On Sat Jul 17, 2021 at 12:55 PM EDT, Jonathan Cameron wrote:
>> On Fri, 16 Jul 2021 15:18:33 -0400
>> "Liam Beguin" <liambeguin@gmail.com> wrote:
>>
>>> On Thu Jul 15, 2021 at 5:48 AM EDT, Peter Rosin wrote:
>>>>
>>>> On 2021-07-15 05:12, 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 | 15 +++++++++++++++
>>>>>  1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>>>> index 4c3cfd4d5181..a2b220b5ba86 100644
>>>>> --- a/drivers/iio/afe/iio-rescale.c
>>>>> +++ b/drivers/iio/afe/iio-rescale.c
>>>>> @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>>>>>  			do_div(tmp, 1000000000LL);
>>>>>  			*val = tmp;
>>>>>  			return ret;
>>>>> +		case IIO_VAL_INT_PLUS_NANO:
>>>>> +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
>>>>> +			do_div(tmp, rescale->denominator);
>>>>> +
>>>>> +			*val = div_s64(tmp, 1000000000LL);
>>>>> +			*val2 = tmp - *val * 1000000000LL;
>>>>> +			return ret;  
>>>>
>>>> This is too simplistic and prone to overflow. We need something like
>>>> this
>>>> (untested)
>>>>
>>>> tmp = (s64)*val * rescale->numerator;
>>>> rem = do_div(tmp, rescale->denominator);
>>>> *val = tmp;
>>>> tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
>>>> do_div(tmp, rescale->denominator);
>>>> *val2 = tmp;
>>>>
>>>> Still not very safe with numerator and denominator both "large", but
>>>> much
>>>> better. And then we need normalizing the fraction part after the above,
>>>> of
>>>> course.
>>>>  
>>>
>>> Understood, I'll test that.
>>>
>>>> And, of course, I'm not sure what *val == -1 and *val2 == 500000000
>>>> really
>>>> means. Is that -1.5 or -0.5? The above may very well need adjusting for
>>>> negative values...
>>>>  
>>>
>>> I would've assumed the correct answer is -1 + 500000000e-9 = -0.5
>>> but adding a test case to iio-test-format.c seems to return -1.5...
>>
> 
> Hi Jonathan,
> 
>> No. -1.5 is as intended, though the IIO_VAL_PLUS_MICRO is rather
>> confusing
>> naming :( We should perhaps add more documentation for that. Signs were
>> always a bit of a pain with this two integer scheme for fixed point.
>>
>> The intent is to have moderately readable look up tables with the
>> problem that
>> we don't have a signed 0 available. Meh, maybe this decision a long time
>> back wasn't a the right one, but it may be a pain to change now as too
>> many
>> drivers to check!
>>
>> 1, 0000000 == 1
>> 0, 5000000 == 0.5
>> 0, 0000000 == 0
>> 0, -5000000 == -0.5
>> -1, 5000000 == -1.5
>>
> 
> Understood, thanks for clearing that out.

I just realized that do_div assumes unsigned operands...

:-(

Cheers,
Peter

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

* Re: [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
  2021-07-19  8:31           ` Peter Rosin
@ 2021-07-19 15:15             ` Liam Beguin
  0 siblings, 0 replies; 20+ messages in thread
From: Liam Beguin @ 2021-07-19 15:15 UTC (permalink / raw)
  To: Peter Rosin, Jonathan Cameron
  Cc: lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt

On Mon Jul 19, 2021 at 4:31 AM EDT, Peter Rosin wrote:
>
>
> On 2021-07-19 01:44, Liam Beguin wrote:
> > On Sat Jul 17, 2021 at 12:55 PM EDT, Jonathan Cameron wrote:
> >> On Fri, 16 Jul 2021 15:18:33 -0400
> >> "Liam Beguin" <liambeguin@gmail.com> wrote:
> >>
> >>> On Thu Jul 15, 2021 at 5:48 AM EDT, Peter Rosin wrote:
> >>>>
> >>>> On 2021-07-15 05:12, 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 | 15 +++++++++++++++
> >>>>>  1 file changed, 15 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >>>>> index 4c3cfd4d5181..a2b220b5ba86 100644
> >>>>> --- a/drivers/iio/afe/iio-rescale.c
> >>>>> +++ b/drivers/iio/afe/iio-rescale.c
> >>>>> @@ -92,7 +92,22 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> >>>>>  			do_div(tmp, 1000000000LL);
> >>>>>  			*val = tmp;
> >>>>>  			return ret;
> >>>>> +		case IIO_VAL_INT_PLUS_NANO:
> >>>>> +			tmp = ((s64)*val * 1000000000LL + *val2) * rescale->numerator;
> >>>>> +			do_div(tmp, rescale->denominator);
> >>>>> +
> >>>>> +			*val = div_s64(tmp, 1000000000LL);
> >>>>> +			*val2 = tmp - *val * 1000000000LL;
> >>>>> +			return ret;  
> >>>>
> >>>> This is too simplistic and prone to overflow. We need something like
> >>>> this
> >>>> (untested)
> >>>>
> >>>> tmp = (s64)*val * rescale->numerator;
> >>>> rem = do_div(tmp, rescale->denominator);
> >>>> *val = tmp;
> >>>> tmp = ((s64)rem * 1000000000LL + (s64)*val2) * rescale->numerator;
> >>>> do_div(tmp, rescale->denominator);
> >>>> *val2 = tmp;
> >>>>
> >>>> Still not very safe with numerator and denominator both "large", but
> >>>> much
> >>>> better. And then we need normalizing the fraction part after the above,
> >>>> of
> >>>> course.
> >>>>  
> >>>
> >>> Understood, I'll test that.
> >>>
> >>>> And, of course, I'm not sure what *val == -1 and *val2 == 500000000
> >>>> really
> >>>> means. Is that -1.5 or -0.5? The above may very well need adjusting for
> >>>> negative values...
> >>>>  
> >>>
> >>> I would've assumed the correct answer is -1 + 500000000e-9 = -0.5
> >>> but adding a test case to iio-test-format.c seems to return -1.5...
> >>
> > 
> > Hi Jonathan,
> > 
> >> No. -1.5 is as intended, though the IIO_VAL_PLUS_MICRO is rather
> >> confusing
> >> naming :( We should perhaps add more documentation for that. Signs were
> >> always a bit of a pain with this two integer scheme for fixed point.
> >>
> >> The intent is to have moderately readable look up tables with the
> >> problem that
> >> we don't have a signed 0 available. Meh, maybe this decision a long time
> >> back wasn't a the right one, but it may be a pain to change now as too
> >> many
> >> drivers to check!
> >>
> >> 1, 0000000 == 1
> >> 0, 5000000 == 0.5
> >> 0, 0000000 == 0
> >> 0, -5000000 == -0.5
> >> -1, 5000000 == -1.5
> >>
> > 
> > Understood, thanks for clearing that out.

Hi Peter,

>
> I just realized that do_div assumes unsigned operands...
>
> :-(

I noticed the same thing after adding the kunit tests.
I added patches for that.

For IIO_VAL_PLUS_{MICRO,NANO} specifically, I have something working but
I like your approach better so I'll work on it a little more.

Thanks,
Liam

>
> Cheers,
> Peter


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

end of thread, other threads:[~2021-07-19 16:07 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15  3:12 [PATCH v5 00/10] iio: afe: add temperature rescaling support Liam Beguin
2021-07-15  3:12 ` [PATCH v5 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-07-15  3:12 ` [PATCH v5 02/10] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
2021-07-15  3:12 ` [PATCH v5 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
2021-07-15  3:12 ` [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
2021-07-15 10:23   ` Peter Rosin
2021-07-16 16:46     ` Liam Beguin
2021-07-15  3:12 ` [PATCH v5 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
2021-07-15  9:48   ` Peter Rosin
2021-07-16 19:18     ` Liam Beguin
2021-07-17  8:11       ` Peter Rosin
2021-07-17 16:55       ` Jonathan Cameron
2021-07-18 23:44         ` Liam Beguin
2021-07-19  8:31           ` Peter Rosin
2021-07-19 15:15             ` Liam Beguin
2021-07-15  3:12 ` [PATCH v5 06/10] iio: afe: rescale: add offset support Liam Beguin
2021-07-15  3:12 ` [PATCH v5 07/10] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
2021-07-15  3:12 ` [PATCH v5 08/10] iio: afe: rescale: add temperature transducers Liam Beguin
2021-07-15  3:12 ` [PATCH v5 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-07-15  3:12 ` [PATCH v5 10/10] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin

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