[PATCH v2 0/8] iio: afe: add temperature rescaling support

[PATCH v2 0/8] iio: afe: add temperature rescaling support

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

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

This series includes minor bugfixes and adds support for a generic
temperature front end circuit.

As Peter suggested in v1, if the upstream channel has an offset, the
rescaler will default to using the processed value. This was done to
avoid having to process all supported offset an scale type combinations.

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.

Related to: https://patchwork.kernel.org/project/linux-iio/list/?series=483087

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 (8):
  iio: inkern: apply consumer scale on IIO_VAL_INT cases
  iio: inkern: apply consumer scale when no channel scale is available
  iio: inkern: error out on unsupported offset type
  iio: inkern: return valid type on raw to processed conversion
  iio: afe: rescale: add upstream offset support
  iio: afe: rescale: add offset support
  iio: afe: rescale: add temperature sensor support
  dt-bindings: iio: afe: add binding for temperature-sense-amplifier

 .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 drivers/iio/afe/iio-rescale.c                 | 39 ++++++++++++-
 drivers/iio/inkern.c                          | 46 +++++++++++----
 4 files changed, 131 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml

Range-diff against v1:
 1:  36f038f93537 <  -:  ------------ iio: inkern: always apply scale requested by consumer
 2:  fd3e9a69841a <  -:  ------------ iio: inkern: error out on unsupported offset type
 3:  91c473be7098 <  -:  ------------ iio: afe: rescale: use core to get processed value
 4:  1097973f3bf7 <  -:  ------------ iio: afe: rescale: add offset support
 5:  786badf92421 <  -:  ------------ iio: afe: rescale: add support for temperature sensors
 6:  0cae8abf6a06 <  -:  ------------ dt-bindings: iio: afe: update MAINTAINERS file
 7:  e806c73122f8 <  -:  ------------ dt-bindings: iio: afe: add binding for temperature-sense-rtd
 8:  f156b16ba01a <  -:  ------------ dt-bindings: iio: afe: add binding for temperature-sense-current
 -:  ------------ >  1:  8ebae9e606a2 iio: inkern: apply consumer scale on IIO_VAL_INT cases
 -:  ------------ >  2:  4d6e4d772f94 iio: inkern: apply consumer scale when no channel scale is available
 -:  ------------ >  3:  dd26ddb49658 iio: inkern: error out on unsupported offset type
 -:  ------------ >  4:  56e9e00cab9d iio: inkern: return valid type on raw to processed conversion
 -:  ------------ >  5:  d86fabc43985 iio: afe: rescale: add upstream offset support
 -:  ------------ >  6:  332fdb2d59ae iio: afe: rescale: add offset support
 -:  ------------ >  7:  3eabc81fb9aa iio: afe: rescale: add temperature sensor support
 9:  9bfdfe7d86b7 !  8:  37980da320b2 dt-bindings: iio: afe: add binding for temperature-sense-amplifier
    @@ Commit message
         dt-bindings: iio: afe: add binding for temperature-sense-amplifier
     
         An ADC is often used to measure other quantities indirectly. This
    -    binding describe one cases, the measurement of a temperature through a
    -    voltage sense amplifier such as the LTC2997.
    +    binding describe such a use case, the measurement of a temperature
    +    through an analog front end connected to a voltage channel.
     
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
     
    @@ Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml (new)
     +  - Liam Beguin <lvb@xiphos.com>
     +
     +description: |
    -+  When an io-channel measures the output voltage of a temperature IC such as
    -+  the LTC2997, the interesting measurement is almost always the corresponding
    ++  When an io-channel measures the output voltage of a temperature analog front
    ++  end such as an RTD (resistance thermometer) or a temperature to current
    ++  sensor, the interesting measurement is almost always the corresponding
     +  temperature, not the voltage output. This binding describes such a circuit.
     +
     +properties:
    @@ Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml (new)
     +  '#io-channel-cells':
     +    const: 1
     +
    -+  alpha-micro-volts-per-degree:
    -+    description: |
    -+      Output voltage gain of the temperature IC.
    ++  sense-gain-mult:
    ++    $ref: /schemas/types.yaml#/definitions/uint32
    ++    description: Amplifier gain multiplier. The default is <1>.
     +
    -+  use-kelvin-scale:
    -+    type: boolean
    -+    description: |
    -+      Boolean indicating if alpha uses Kelvin degrees instead of Celsius.
    ++  sense-gain-div:
    ++    $ref: /schemas/types.yaml#/definitions/uint32
    ++    description: Amplifier gain divider. The default is <1>.
    ++
    ++  sense-offset-millicelsius:
    ++    description: Amplifier offset. The default is <0>.
     +
     +additionalProperties: false
     +required:
     +  - compatible
     +  - io-channels
    -+  - alpha-micro-volts-per-degree
     +
     +examples:
     +  - |
    -+    znq_temp: iio-rescale0 {
    ++    pt1000_1: temperature-sensor {
     +        compatible = "temperature-sense-amplifier";
     +        #io-channel-cells = <1>;
     +        io-channels = <&temp_adc 3>;
     +
    -+        use-kelvin-scale;
    -+        alpha-micro-volts-per-degree = <4000>;
    ++        sense-gain-mult = <1000000>;
    ++        sense-gain-div = <3908>;
    ++        sense-offset-millicelsius = <(-255885)>;
     +    };
    -+
     +...
     
      ## MAINTAINERS ##
    @@ MAINTAINERS: L:	linux-iio@vger.kernel.org
      F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
      F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
     +F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
    - F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-current.yaml
    - F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
      F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
    + F:	drivers/iio/afe/iio-rescale.c
    + 

base-commit: 41340965b4f8055f975f73e1e3d23eff8038f013
-- 
2.30.1.489.g328c10930387

[PATCH v2 1/8] iio: inkern: apply consumer scale on IIO_VAL_INT cases

[Liam Beguin]

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

[PATCH v2 2/8] iio: inkern: apply consumer scale when no channel scale is available

[Liam Beguin]

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

[PATCH v2 3/8] iio: inkern: error out on unsupported offset type

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

iio_convert_raw_to_processed_unlocked() assumes the offset is an
integer.
Make that clear to the consumer by returning an error on unsupported
offset types without breaking valid implicit truncations.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/inkern.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b69027690ed5..0b5667f22b1d 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -578,13 +578,37 @@ 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:
+			if (offset_val2 > 1000)
+				return -EINVAL;
+			break;
+		case IIO_VAL_INT_PLUS_NANO:
+			if (offset_val2 > 1000000)
+				return -EINVAL;
+		case IIO_VAL_FRACTIONAL:
+			if (offset_val2 != 1)
+				return -EINVAL;
+			break;
+		case IIO_VAL_FRACTIONAL_LOG2:
+			if (offset_val2)
+				return -EINVAL;
+			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

[PATCH v2 4/8] iio: inkern: return valid type on raw to processed conversion

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

iio_convert_raw_to_processed_unlocked() applies the offset and scale of
a channel on it's raw value.
The processed value returned is always an integer. Return IIO_VAL_INT so
that consumers can use this return value directly.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/inkern.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 0b5667f22b1d..00d234e87234 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -618,7 +618,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 		 * raw value and return.
 		 */
 		*processed = raw * scale;
-		return 0;
+		return IIO_VAL_INT;
 	}
 
 	switch (scale_type) {
@@ -652,7 +652,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 		return -EINVAL;
 	}
 
-	return 0;
+	return IIO_VAL_INT;
 }
 
 int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
-- 
2.30.1.489.g328c10930387

[PATCH v2 5/8] iio: afe: rescale: add upstream offset support

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Default to using iio_read_channel_processed() if the upstream channel
exposes an offset.

When possible, it's preferable to avoid using
iio_read_channel_processed() to maintain as much precision as possible.

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

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 774eb3044edd..e34148d39b39 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -148,7 +148,10 @@ static int rescale_configure_channel(struct device *dev,
 	chan->ext_info = rescale->ext_info;
 	chan->type = rescale->cfg->type;
 
-	if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
+	if (iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET)) {
+		dev_info(dev, "found upstream offset, using processed value\n");
+		rescale->chan_processed = true;
+	} else if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
 	    iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) {
 		dev_info(dev, "using raw+scale source channel\n");
 	} else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) {
-- 
2.30.1.489.g328c10930387

[PATCH v2 6/8] iio: afe: rescale: add offset support

[Liam Beguin]

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index e34148d39b39..3d445c76dbb2 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,
@@ -86,6 +87,10 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EOPNOTSUPP;
 		}
+	case IIO_CHAN_INFO_OFFSET:
+		*val = rescale->offset;
+
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -165,6 +170,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
@@ -329,6 +337,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

[PATCH v2 7/8] iio: afe: rescale: add temperature sensor support

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Add support for linear temperature sensors. This is meant to work with
different kinds of analog front ends such as RTDs (resistance
thermometers), voltage IC sensors (like the LTC2997), and current IC
sensors (see AD590).

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

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 3d445c76dbb2..9e3c7e2b47cd 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -272,10 +272,29 @@ static int rescale_voltage_divider_props(struct device *dev,
 	return 0;
 }
 
+static int rescale_temp_sense_amplifier_props(struct device *dev,
+					      struct rescale *rescale)
+{
+	s32 gain_mult = 1;
+	s32 gain_div = 1;
+	s32 offset = 0;
+
+	device_property_read_u32(dev, "sense-gain-mult", &gain_mult);
+	device_property_read_u32(dev, "sense-gain-div", &gain_div);
+	device_property_read_u32(dev, "sense-offset-millicelsius", &offset);
+
+	rescale->numerator = gain_mult;
+	rescale->denominator = gain_div;
+	rescale->offset = offset * gain_div / gain_mult;
+
+	return 0;
+}
+
 enum rescale_variant {
 	CURRENT_SENSE_AMPLIFIER,
 	CURRENT_SENSE_SHUNT,
 	VOLTAGE_DIVIDER,
+	TEMP_SENSE_AMPLIFIER,
 };
 
 static const struct rescale_cfg rescale_cfg[] = {
@@ -291,6 +310,10 @@ static const struct rescale_cfg rescale_cfg[] = {
 		.type = IIO_VOLTAGE,
 		.props = rescale_voltage_divider_props,
 	},
+	[TEMP_SENSE_AMPLIFIER] = {
+		.type = IIO_TEMP,
+		.props = rescale_temp_sense_amplifier_props,
+	},
 };
 
 static const struct of_device_id rescale_match[] = {
@@ -300,6 +323,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-amplifier",
+	  .data = &rescale_cfg[TEMP_SENSE_AMPLIFIER], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, rescale_match);
-- 
2.30.1.489.g328c10930387

[PATCH v2 8/8] dt-bindings: iio: afe: add binding for temperature-sense-amplifier

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

An ADC is often used to measure other quantities indirectly. This
binding describe such a use case, the measurement of a temperature
through an analog front end connected to a voltage channel.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml

diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
new file mode 100644
index 000000000000..08f97f052a91
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/afe/temperature-sense-amplifier.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Temperature Sense Amplifier
+
+maintainers:
+  - Liam Beguin <lvb@xiphos.com>
+
+description: |
+  When an io-channel measures the output voltage of a temperature analog front
+  end such as an RTD (resistance thermometer) or a temperature to current
+  sensor, the interesting measurement is almost always the corresponding
+  temperature, not the voltage output. This binding describes such a circuit.
+
+properties:
+  compatible:
+    const: temperature-sense-amplifier
+
+  io-channels:
+    maxItems: 1
+    description: |
+      Channel node of a voltage io-channel.
+
+  '#io-channel-cells':
+    const: 1
+
+  sense-gain-mult:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Amplifier gain multiplier. The default is <1>.
+
+  sense-gain-div:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Amplifier gain divider. The default is <1>.
+
+  sense-offset-millicelsius:
+    description: Amplifier offset. The default is <0>.
+
+additionalProperties: false
+required:
+  - compatible
+  - io-channels
+
+examples:
+  - |
+    pt1000_1: temperature-sensor {
+        compatible = "temperature-sense-amplifier";
+        #io-channel-cells = <1>;
+        io-channels = <&temp_adc 3>;
+
+        sense-gain-mult = <1000000>;
+        sense-gain-div = <3908>;
+        sense-offset-millicelsius = <(-255885)>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e679d422b472..4f7b4ee9f19b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8887,6 +8887,7 @@ L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
 F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
+F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
 F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
 F:	drivers/iio/afe/iio-rescale.c
 
-- 
2.30.1.489.g328c10930387

Re: [PATCH v2 3/8] iio: inkern: error out on unsupported offset type

[Jonathan Cameron]

On Mon,  7 Jun 2021 10:47:13 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> iio_convert_raw_to_processed_unlocked() assumes the offset is an
> integer.
> Make that clear to the consumer by returning an error on unsupported
> offset types without breaking valid implicit truncations.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/iio/inkern.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index b69027690ed5..0b5667f22b1d 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -578,13 +578,37 @@ 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:
> +			if (offset_val2 > 1000)

What's the logic behind this one?  > 1000000
would be an interesting corner case, though I'm not sure we've ever
explicitly disallowed it before.

Why are we at 1000th of that for the check?

> +				return -EINVAL;
> +			break;
> +		case IIO_VAL_INT_PLUS_NANO:
> +			if (offset_val2 > 1000000)

Similar this is a bit odd.

> +				return -EINVAL;
> +		case IIO_VAL_FRACTIONAL:
> +			if (offset_val2 != 1)
> +				return -EINVAL;

We could be more flexible on this, but I don't recall any
channels using this so far.

> +			break;
> +		case IIO_VAL_FRACTIONAL_LOG2:
> +			if (offset_val2)
> +				return -EINVAL;

Same in this case.

> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		raw64 += offset_val;
> +	}
>  
>  	scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
>  					IIO_CHAN_INFO_SCALE);

Re: [PATCH v2 4/8] iio: inkern: return valid type on raw to processed conversion

[Jonathan Cameron]

On Mon,  7 Jun 2021 10:47:14 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> iio_convert_raw_to_processed_unlocked() applies the offset and scale of
> a channel on it's raw value.
> The processed value returned is always an integer. Return IIO_VAL_INT so
> that consumers can use this return value directly.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
This looks likely to cause breakage given that return value will go to
consumers directly via iio_convert_raw_to_processed()

Looks like this will break lmp91000 which checks for error as

if (ret)

> ---
>  drivers/iio/inkern.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 0b5667f22b1d..00d234e87234 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -618,7 +618,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
>  		 * raw value and return.
>  		 */
>  		*processed = raw * scale;
> -		return 0;
> +		return IIO_VAL_INT;
>  	}
>  
>  	switch (scale_type) {
> @@ -652,7 +652,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return IIO_VAL_INT;
>  }
>  
>  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,

Re: [PATCH v2 8/8] dt-bindings: iio: afe: add binding for temperature-sense-amplifier

[Jonathan Cameron]

On Mon,  7 Jun 2021 10:47:18 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> An ADC is often used to measure other quantities indirectly. This
> binding describe such a use case, the measurement of a temperature
> through an analog front end connected to a voltage channel.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> new file mode 100644
> index 000000000000..08f97f052a91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/afe/temperature-sense-amplifier.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Temperature Sense Amplifier
> +
> +maintainers:
> +  - Liam Beguin <lvb@xiphos.com>
> +
> +description: |
> +  When an io-channel measures the output voltage of a temperature analog front
> +  end such as an RTD (resistance thermometer) or a temperature to current
> +  sensor, the interesting measurement is almost always the corresponding
> +  temperature, not the voltage output. This binding describes such a circuit.

Perhaps add something about this only covering the linear cases...

> +
> +properties:
> +  compatible:
> +    const: temperature-sense-amplifier
> +
> +  io-channels:
> +    maxItems: 1
> +    description: |
> +      Channel node of a voltage io-channel.
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  sense-gain-mult:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Amplifier gain multiplier. The default is <1>.
> +
> +  sense-gain-div:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Amplifier gain divider. The default is <1>.
> +
> +  sense-offset-millicelsius:
> +    description: Amplifier offset. The default is <0>.

Whilst it may seem obvious I'd like to see a statement of
how these are used somewhere in here.

   temp_celcius = voltage * gain-mult / gain-div + offset

Mainly because those familiar with the IIO usage of offset
would expect
(voltage + offset) * gain-mult/gain-div 

which doesn't make sense for this device but might leave
people confused!

> +
> +additionalProperties: false
> +required:
> +  - compatible
> +  - io-channels
> +
> +examples:
> +  - |
> +    pt1000_1: temperature-sensor {
> +        compatible = "temperature-sense-amplifier";
> +        #io-channel-cells = <1>;
> +        io-channels = <&temp_adc 3>;
> +
> +        sense-gain-mult = <1000000>;
> +        sense-gain-div = <3908>;
> +        sense-offset-millicelsius = <(-255885)>;
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e679d422b472..4f7b4ee9f19b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8887,6 +8887,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
>  F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> +F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
>  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
>  F:	drivers/iio/afe/iio-rescale.c
>  

Re: [PATCH v2 3/8] iio: inkern: error out on unsupported offset type

[Liam Beguin]

Hi Jonathan,

On Wed Jun 9, 2021 at 4:28 PM EDT, Jonathan Cameron wrote:
> On Mon, 7 Jun 2021 10:47:13 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > iio_convert_raw_to_processed_unlocked() assumes the offset is an
> > integer.
> > Make that clear to the consumer by returning an error on unsupported
> > offset types without breaking valid implicit truncations.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/iio/inkern.c | 34 +++++++++++++++++++++++++++++-----
> >  1 file changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index b69027690ed5..0b5667f22b1d 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -578,13 +578,37 @@ 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:
> > +			if (offset_val2 > 1000)
>
> What's the logic behind this one? > 1000000
> would be an interesting corner case, though I'm not sure we've ever
> explicitly disallowed it before.
>
> Why are we at 1000th of that for the check?
>

For these the idea was to go with one milli of precision.
I don't know if that's a good criteria but I wanted to start with
something. Do you have any suggestions?

> > +				return -EINVAL;
> > +			break;
> > +		case IIO_VAL_INT_PLUS_NANO:
> > +			if (offset_val2 > 1000000)
>
> Similar this is a bit odd.
>
> > +				return -EINVAL;
> > +		case IIO_VAL_FRACTIONAL:
> > +			if (offset_val2 != 1)
> > +				return -EINVAL;
>
> We could be more flexible on this, but I don't recall any
> channels using this so far.
>
> > +			break;
> > +		case IIO_VAL_FRACTIONAL_LOG2:
> > +			if (offset_val2)
> > +				return -EINVAL;
>
> Same in this case.
>

For these two cases, I went with what Peter suggested in the previous
version, to not break on valid implicit truncations.

What would be a good precision criteria for all offset types?

> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +		raw64 += offset_val;
> > +	}
> >  
> >  	scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> >  					IIO_CHAN_INFO_SCALE);

Thanks for looking at this,
Liam

Re: [PATCH v2 4/8] iio: inkern: return valid type on raw to processed conversion

[Liam Beguin]

On Wed Jun 9, 2021 at 4:32 PM EDT, Jonathan Cameron wrote:
> On Mon, 7 Jun 2021 10:47:14 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > iio_convert_raw_to_processed_unlocked() applies the offset and scale of
> > a channel on it's raw value.
> > The processed value returned is always an integer. Return IIO_VAL_INT so
> > that consumers can use this return value directly.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> This looks likely to cause breakage given that return value will go to
> consumers directly via iio_convert_raw_to_processed()
>
> Looks like this will break lmp91000 which checks for error as
>
> if (ret)
>

IIO_VAL_INT seems like a better return value here since the consumer
gets an integer. I can look at existing consumers and patch those too.
Or would you rather I drop this patch?

> > ---
> >  drivers/iio/inkern.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index 0b5667f22b1d..00d234e87234 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -618,7 +618,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
> >  		 * raw value and return.
> >  		 */
> >  		*processed = raw * scale;
> > -		return 0;
> > +		return IIO_VAL_INT;
> >  	}
> >  
> >  	switch (scale_type) {
> > @@ -652,7 +652,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
> >  		return -EINVAL;
> >  	}
> >  
> > -	return 0;
> > +	return IIO_VAL_INT;
> >  }
> >  
> >  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,

Re: [PATCH v2 8/8] dt-bindings: iio: afe: add binding for temperature-sense-amplifier

[Liam Beguin]

On Wed Jun 9, 2021 at 4:52 PM EDT, Jonathan Cameron wrote:
> On Mon, 7 Jun 2021 10:47:18 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > An ADC is often used to measure other quantities indirectly. This
> > binding describe such a use case, the measurement of a temperature
> > through an analog front end connected to a voltage channel.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > new file mode 100644
> > index 000000000000..08f97f052a91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/afe/temperature-sense-amplifier.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Temperature Sense Amplifier
> > +
> > +maintainers:
> > +  - Liam Beguin <lvb@xiphos.com>
> > +
> > +description: |
> > +  When an io-channel measures the output voltage of a temperature analog front
> > +  end such as an RTD (resistance thermometer) or a temperature to current
> > +  sensor, the interesting measurement is almost always the corresponding
> > +  temperature, not the voltage output. This binding describes such a circuit.
>
> Perhaps add something about this only covering the linear cases...
>

Okay, will do.

> > +
> > +properties:
> > +  compatible:
> > +    const: temperature-sense-amplifier
> > +
> > +  io-channels:
> > +    maxItems: 1
> > +    description: |
> > +      Channel node of a voltage io-channel.
> > +
> > +  '#io-channel-cells':
> > +    const: 1
> > +
> > +  sense-gain-mult:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Amplifier gain multiplier. The default is <1>.
> > +
> > +  sense-gain-div:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Amplifier gain divider. The default is <1>.
> > +
> > +  sense-offset-millicelsius:
> > +    description: Amplifier offset. The default is <0>.
>
> Whilst it may seem obvious I'd like to see a statement of
> how these are used somewhere in here.
>
> temp_celcius = voltage * gain-mult / gain-div + offset
>
> Mainly because those familiar with the IIO usage of offset
> would expect
> (voltage + offset) * gain-mult/gain-div
>
> which doesn't make sense for this device but might leave
> people confused!
>

You're right, I'll add an explicit statement of how these parameters are
used.

Thanks,
Liam

> > +
> > +additionalProperties: false
> > +required:
> > +  - compatible
> > +  - io-channels
> > +
> > +examples:
> > +  - |
> > +    pt1000_1: temperature-sensor {
> > +        compatible = "temperature-sense-amplifier";
> > +        #io-channel-cells = <1>;
> > +        io-channels = <&temp_adc 3>;
> > +
> > +        sense-gain-mult = <1000000>;
> > +        sense-gain-div = <3908>;
> > +        sense-offset-millicelsius = <(-255885)>;
> > +    };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e679d422b472..4f7b4ee9f19b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8887,6 +8887,7 @@ L:	linux-iio@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
> >  F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> > +F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> >  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
> >  F:	drivers/iio/afe/iio-rescale.c
> >  

Re: [PATCH v2 3/8] iio: inkern: error out on unsupported offset type

[Jonathan Cameron]

On Wed, 09 Jun 2021 17:40:47 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> Hi Jonathan,
> 
> On Wed Jun 9, 2021 at 4:28 PM EDT, Jonathan Cameron wrote:
> > On Mon, 7 Jun 2021 10:47:13 -0400
> > Liam Beguin <liambeguin@gmail.com> wrote:
> >  
> > > From: Liam Beguin <lvb@xiphos.com>
> > > 
> > > iio_convert_raw_to_processed_unlocked() assumes the offset is an
> > > integer.
> > > Make that clear to the consumer by returning an error on unsupported
> > > offset types without breaking valid implicit truncations.
> > > 
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > ---
> > >  drivers/iio/inkern.c | 34 +++++++++++++++++++++++++++++-----
> > >  1 file changed, 29 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index b69027690ed5..0b5667f22b1d 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -578,13 +578,37 @@ 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:
> > > +			if (offset_val2 > 1000)  
> >
> > What's the logic behind this one? > 1000000
> > would be an interesting corner case, though I'm not sure we've ever
> > explicitly disallowed it before.
> >
> > Why are we at 1000th of that for the check?
> >  
> 
> For these the idea was to go with one milli of precision.
> I don't know if that's a good criteria but I wanted to start with
> something. Do you have any suggestions?
> 
> > > +				return -EINVAL;
> > > +			break;
> > > +		case IIO_VAL_INT_PLUS_NANO:
> > > +			if (offset_val2 > 1000000)  
> >
> > Similar this is a bit odd.
> >  
> > > +				return -EINVAL;
> > > +		case IIO_VAL_FRACTIONAL:
> > > +			if (offset_val2 != 1)
> > > +				return -EINVAL;  
> >
> > We could be more flexible on this, but I don't recall any
> > channels using this so far.
> >  
> > > +			break;
> > > +		case IIO_VAL_FRACTIONAL_LOG2:
> > > +			if (offset_val2)
> > > +				return -EINVAL;  
> >
> > Same in this case.
> >  
> 
> For these two cases, I went with what Peter suggested in the previous
> version, to not break on valid implicit truncations.
> 
> What would be a good precision criteria for all offset types?

@Peter, what were your thoughts on this.

I was thinking we'd just not worry about how much truncation was happening
and just silently eat it.

J
> 
> > > +			break;
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		raw64 += offset_val;
> > > +	}
> > >  
> > >  	scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> > >  					IIO_CHAN_INFO_SCALE);  
> 
> Thanks for looking at this,
> Liam

Re: [PATCH v2 4/8] iio: inkern: return valid type on raw to processed conversion

[Jonathan Cameron]

On Wed, 09 Jun 2021 17:46:58 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> On Wed Jun 9, 2021 at 4:32 PM EDT, Jonathan Cameron wrote:
> > On Mon, 7 Jun 2021 10:47:14 -0400
> > Liam Beguin <liambeguin@gmail.com> wrote:
> >  
> > > From: Liam Beguin <lvb@xiphos.com>
> > > 
> > > iio_convert_raw_to_processed_unlocked() applies the offset and scale of
> > > a channel on it's raw value.
> > > The processed value returned is always an integer. Return IIO_VAL_INT so
> > > that consumers can use this return value directly.
> > > 
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>  
> > This looks likely to cause breakage given that return value will go to
> > consumers directly via iio_convert_raw_to_processed()
> >
> > Looks like this will break lmp91000 which checks for error as
> >
> > if (ret)
> >  
> 
> IIO_VAL_INT seems like a better return value here since the consumer
> gets an integer. I can look at existing consumers and patch those too.
> Or would you rather I drop this patch?
If we were looking at actually allowing this to return other types,
then I'd agree with updating callers appropriately.

For now we aren't doing that, so the only question is success or fail.
So I'd drop this one.

Most consumers don't care about IIO types.

Jonathan

> 
> > > ---
> > >  drivers/iio/inkern.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index 0b5667f22b1d..00d234e87234 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -618,7 +618,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
> > >  		 * raw value and return.
> > >  		 */
> > >  		*processed = raw * scale;
> > > -		return 0;
> > > +		return IIO_VAL_INT;
> > >  	}
> > >  
> > >  	switch (scale_type) {
> > > @@ -652,7 +652,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	return 0;
> > > +	return IIO_VAL_INT;
> > >  }
> > >  
> > >  int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,  
> 

Re: [PATCH v2 3/8] iio: inkern: error out on unsupported offset type

[Peter Rosin]

Hi!

On 2021-06-10 11:06, Jonathan Cameron wrote:
> On Wed, 09 Jun 2021 17:40:47 -0400
> "Liam Beguin" <liambeguin@gmail.com> wrote:
> 
>> Hi Jonathan,
>>
>> On Wed Jun 9, 2021 at 4:28 PM EDT, Jonathan Cameron wrote:
>>> On Mon, 7 Jun 2021 10:47:13 -0400
>>> Liam Beguin <liambeguin@gmail.com> wrote:
>>>  
>>>> From: Liam Beguin <lvb@xiphos.com>
>>>>
>>>> iio_convert_raw_to_processed_unlocked() assumes the offset is an
>>>> integer.
>>>> Make that clear to the consumer by returning an error on unsupported
>>>> offset types without breaking valid implicit truncations.
>>>>
>>>> Signed-off-by: Liam Beguin <lvb@xiphos.com>
>>>> ---
>>>>  drivers/iio/inkern.c | 34 +++++++++++++++++++++++++++++-----
>>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>>> index b69027690ed5..0b5667f22b1d 100644
>>>> --- a/drivers/iio/inkern.c
>>>> +++ b/drivers/iio/inkern.c
>>>> @@ -578,13 +578,37 @@ 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:
>>>> +			if (offset_val2 > 1000)  
>>>
>>> What's the logic behind this one? > 1000000
>>> would be an interesting corner case, though I'm not sure we've ever
>>> explicitly disallowed it before.
>>>
>>> Why are we at 1000th of that for the check?
>>>  
>>
>> For these the idea was to go with one milli of precision.
>> I don't know if that's a good criteria but I wanted to start with
>> something. Do you have any suggestions?
>>
>>>> +				return -EINVAL;
>>>> +			break;
>>>> +		case IIO_VAL_INT_PLUS_NANO:
>>>> +			if (offset_val2 > 1000000)  
>>>
>>> Similar this is a bit odd.
>>>  
>>>> +				return -EINVAL;
>>>> +		case IIO_VAL_FRACTIONAL:
>>>> +			if (offset_val2 != 1)
>>>> +				return -EINVAL;  
>>>
>>> We could be more flexible on this, but I don't recall any
>>> channels using this so far.
>>>  
>>>> +			break;
>>>> +		case IIO_VAL_FRACTIONAL_LOG2:
>>>> +			if (offset_val2)
>>>> +				return -EINVAL;  
>>>
>>> Same in this case.
>>>  
>>
>> For these two cases, I went with what Peter suggested in the previous
>> version, to not break on valid implicit truncations.
>>
>> What would be a good precision criteria for all offset types?
> 
> @Peter, what were your thoughts on this.
> 
> I was thinking we'd just not worry about how much truncation was happening
> and just silently eat it.

For the "integer-plus" formats, that was my thinking too. Previously that
was exactly what was happeneing, and v1 randomly broke any user that relied
on 42.424242 being truncated to 42. This is still the case with this v2, as
v2 is allowing only a very slim set of truncations. I meant that this new
code needs to allow all truncations, just as before. Anything short of that
must have a much better description of why it is safe to all of a sudden
disallow truncation. I.e. such a change needs to come with traces of an
audit of how this function is used, and why changing the semantics will not
cause regressions.

For IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2, it seems correct to
error out if the denominator isn't 1, because relying on using an offset of
e.g. 187 for a IIO_VAL_FRACTIONAL of 187/169 is not at all healthy.

Both erroring out and doing a best effort calculation for these fractional
cases with denominator != 1 would be ok by me, because they are plain and
simple severly broken at the moment.

Cheers,
Peter

> J
>>
>>>> +			break;
>>>> +		default:
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		raw64 += offset_val;
>>>> +	}
>>>>  
>>>>  	scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
>>>>  					IIO_CHAN_INFO_SCALE);  
>>
>> Thanks for looking at this,
>> Liam
> 

Re: [PATCH v2 7/8] iio: afe: rescale: add temperature sensor support

[Peter Rosin]

Hi!

On 2021-06-07 16:47, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> Add support for linear temperature sensors. This is meant to work with
> different kinds of analog front ends such as RTDs (resistance
> thermometers), voltage IC sensors (like the LTC2997), and current IC
> sensors (see AD590).
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 3d445c76dbb2..9e3c7e2b47cd 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -272,10 +272,29 @@ static int rescale_voltage_divider_props(struct device *dev,
>  	return 0;
>  }
>  
> +static int rescale_temp_sense_amplifier_props(struct device *dev,
> +					      struct rescale *rescale)
> +{
> +	s32 gain_mult = 1;
> +	s32 gain_div = 1;
> +	s32 offset = 0;
> +
> +	device_property_read_u32(dev, "sense-gain-mult", &gain_mult);
> +	device_property_read_u32(dev, "sense-gain-div", &gain_div);
> +	device_property_read_u32(dev, "sense-offset-millicelsius", &offset);
> +
> +	rescale->numerator = gain_mult;
> +	rescale->denominator = gain_div;
> +	rescale->offset = offset * gain_div / gain_mult;

This should be done with 64-bit math, no? After all, An offset of
approximately 300000 is not unexpected, and that's quite big to
scale with 32-bit math.

Cheers,
Peter

> +
> +	return 0;
> +}
> +
>  enum rescale_variant {
>  	CURRENT_SENSE_AMPLIFIER,
>  	CURRENT_SENSE_SHUNT,
>  	VOLTAGE_DIVIDER,
> +	TEMP_SENSE_AMPLIFIER,
>  };
>  
>  static const struct rescale_cfg rescale_cfg[] = {
> @@ -291,6 +310,10 @@ static const struct rescale_cfg rescale_cfg[] = {
>  		.type = IIO_VOLTAGE,
>  		.props = rescale_voltage_divider_props,
>  	},
> +	[TEMP_SENSE_AMPLIFIER] = {
> +		.type = IIO_TEMP,
> +		.props = rescale_temp_sense_amplifier_props,
> +	},
>  };
>  
>  static const struct of_device_id rescale_match[] = {
> @@ -300,6 +323,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-amplifier",
> +	  .data = &rescale_cfg[TEMP_SENSE_AMPLIFIER], },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, rescale_match);
> 

Re: [PATCH v2 8/8] dt-bindings: iio: afe: add binding for temperature-sense-amplifier

[Peter Rosin]

Hi!

Should "amplifier" really be part of the name for this binding when it's now
just a generic voltage-to-temperature rescale binding? Or, perhaps a better
description is THE generic voltage-to-temperature rescale binding?

But that's not a strong opinion, I know next to nothing about these things
and it might be that an amplifier is involved in the vast majority of cases?
Maybe it's enough to be more explicit in the describing text that the binding
is intended for analog front ends lacking an amplifier as well? I just find
it a bit confusing since there are actual HW that calls itself "temperature
sence amplifiers" that I think this binding targets but then isn't
exemplified anywhere.

Also, it disturbs my sense of symmetry if volt->temp gets a generic
binding like this, when volt->current and volt->volt have bindings for
specific front ends. Again, it's not a strong opinion, I'm just pointing it
out. For the record, I started out with a generic volt->current binding
similar to this volt->temp binding, but got push-back so that we now have
two specific volt->current bindings. Again, I'm just pointing this out, and
I'm perfectly aware that "rules" and opinions change over time.

On 2021-06-07 16:47, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> An ADC is often used to measure other quantities indirectly. This
> binding describe such a use case, the measurement of a temperature
> through an analog front end connected to a voltage channel.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> new file mode 100644
> index 000000000000..08f97f052a91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/afe/temperature-sense-amplifier.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Temperature Sense Amplifier
> +
> +maintainers:
> +  - Liam Beguin <lvb@xiphos.com>

Here, you claim maintainership...

> +
> +description: |
> +  When an io-channel measures the output voltage of a temperature analog front
> +  end such as an RTD (resistance thermometer) or a temperature to current
> +  sensor, the interesting measurement is almost always the corresponding
> +  temperature, not the voltage output. This binding describes such a circuit.

Why would you convert from a voltage if you have a "temperature to current
sensor"? Such a sensor should give you a current. Yeah yeah, I get it, you
bake some resistance into the "gain" and you are done. But I think these
things should be explicitly mentioned with examples. I think it would be a
lot less terse if you spell out a couple of common ways to connect one of
these linear temperature sensors and how that then maps to the gain that the
consumer of the binding needs to use.

It would also be a good thing to mention sensors by name, so that someone
grepping for them finds this binding. It's a djungle out there.

> +
> +properties:
> +  compatible:
> +    const: temperature-sense-amplifier
> +
> +  io-channels:
> +    maxItems: 1
> +    description: |
> +      Channel node of a voltage io-channel.
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  sense-gain-mult:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Amplifier gain multiplier. The default is <1>.
> +
> +  sense-gain-div:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Amplifier gain divider. The default is <1>.
> +
> +  sense-offset-millicelsius:
> +    description: Amplifier offset. The default is <0>.
> +
> +additionalProperties: false
> +required:
> +  - compatible
> +  - io-channels
> +
> +examples:
> +  - |
> +    pt1000_1: temperature-sensor {
> +        compatible = "temperature-sense-amplifier";
> +        #io-channel-cells = <1>;
> +        io-channels = <&temp_adc 3>;
> +
> +        sense-gain-mult = <1000000>;
> +        sense-gain-div = <3908>;
> +        sense-offset-millicelsius = <(-255885)>;
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e679d422b472..4f7b4ee9f19b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8887,6 +8887,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
>  F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> +F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml

...and here, you give maintenance to me. I didn't want all afe bindings so I
didn't put an asterisk there for a reason :-)
This binding is backed by the iio-rescale driver, so it's not totally alien
for me to maintain it, but I'd be more happy if you listed yourself as I think
you intended to?

Cheers,
Peter

>  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
>  F:	drivers/iio/afe/iio-rescale.c
>  
> 

Re: [PATCH v2 8/8] dt-bindings: iio: afe: add binding for temperature-sense-amplifier

[Liam Beguin]

Hi Peter,

On Fri Jun 11, 2021 at 3:37 AM EDT, Peter Rosin wrote:
> Hi!
>
> Should "amplifier" really be part of the name for this binding when it's
> now
> just a generic voltage-to-temperature rescale binding? Or, perhaps a
> better
> description is THE generic voltage-to-temperature rescale binding?

I went with temperature-sense-amplifier because it resembled what you
had for the current sense binding, and because of the generic scaling
factor in v2.

>
> But that's not a strong opinion, I know next to nothing about these
> things
> and it might be that an amplifier is involved in the vast majority of
> cases?
> Maybe it's enough to be more explicit in the describing text that the
> binding
> is intended for analog front ends lacking an amplifier as well? I just
> find
> it a bit confusing since there are actual HW that calls itself
> "temperature
> sence amplifiers" that I think this binding targets but then isn't
> exemplified anywhere.
>
> Also, it disturbs my sense of symmetry if volt->temp gets a generic
> binding like this, when volt->current and volt->volt have bindings for
> specific front ends. Again, it's not a strong opinion, I'm just pointing
> it
> out. For the record, I started out with a generic volt->current binding
> similar to this volt->temp binding, but got push-back so that we now
> have
> two specific volt->current bindings. Again, I'm just pointing this out,
> and
> I'm perfectly aware that "rules" and opinions change over time.

I agree with you that it can be confusing given that some temperature
sensors call themselves "temperature sense amplifiers".

In v1, temperature-sense-amplifier was used for that kind of device.

I liked having one binding per front end type like we had in v1 because
the devicetree ends up listing hardware parameters which I find is more
explicit.

I went with a generic one for v2 because I thought it would make it
applicable to other use-cases and simplify implementation.

I don't have strong feelings about keeping v2 over v1 bindings, and
given what we talked about v1 might be better here.

Do you have a preference?

>
> On 2021-06-07 16:47, Liam Beguin wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > An ADC is often used to measure other quantities indirectly. This
> > binding describe such a use case, the measurement of a temperature
> > through an analog front end connected to a voltage channel.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > new file mode 100644
> > index 000000000000..08f97f052a91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/afe/temperature-sense-amplifier.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Temperature Sense Amplifier
> > +
> > +maintainers:
> > +  - Liam Beguin <lvb@xiphos.com>
>
> Here, you claim maintainership...
>
> > +
> > +description: |
> > +  When an io-channel measures the output voltage of a temperature analog front
> > +  end such as an RTD (resistance thermometer) or a temperature to current
> > +  sensor, the interesting measurement is almost always the corresponding
> > +  temperature, not the voltage output. This binding describes such a circuit.
>
> Why would you convert from a voltage if you have a "temperature to
> current
> sensor"? Such a sensor should give you a current. Yeah yeah, I get it,
> you
> bake some resistance into the "gain" and you are done. But I think these
> things should be explicitly mentioned with examples. I think it would be
> a
> lot less terse if you spell out a couple of common ways to connect one
> of
> these linear temperature sensors and how that then maps to the gain that
> the
> consumer of the binding needs to use.
>
> It would also be a good thing to mention sensors by name, so that
> someone
> grepping for them finds this binding. It's a djungle out there.
>

You're right adding sensors names here would be very useful.

I'll rework the description with your suggestions and add specific
examples with maybe schematic drawings like in voltage-divider.yaml.

> > +
> > +properties:
> > +  compatible:
> > +    const: temperature-sense-amplifier
> > +
> > +  io-channels:
> > +    maxItems: 1
> > +    description: |
> > +      Channel node of a voltage io-channel.
> > +
> > +  '#io-channel-cells':
> > +    const: 1
> > +
> > +  sense-gain-mult:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Amplifier gain multiplier. The default is <1>.
> > +
> > +  sense-gain-div:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Amplifier gain divider. The default is <1>.
> > +
> > +  sense-offset-millicelsius:
> > +    description: Amplifier offset. The default is <0>.
> > +
> > +additionalProperties: false
> > +required:
> > +  - compatible
> > +  - io-channels
> > +
> > +examples:
> > +  - |
> > +    pt1000_1: temperature-sensor {
> > +        compatible = "temperature-sense-amplifier";
> > +        #io-channel-cells = <1>;
> > +        io-channels = <&temp_adc 3>;
> > +
> > +        sense-gain-mult = <1000000>;
> > +        sense-gain-div = <3908>;
> > +        sense-offset-millicelsius = <(-255885)>;
> > +    };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e679d422b472..4f7b4ee9f19b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8887,6 +8887,7 @@ L:	linux-iio@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
> >  F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> > +F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
>
> ...and here, you give maintenance to me. I didn't want all afe bindings
> so I
> didn't put an asterisk there for a reason :-)

Sorry about that :-)

I don't really know what the convention is here. I put myself as a
maintainer on the yaml file since I created it.

For the MAINTAINERS patch, would something like this be better?

IIO UNIT CONVERTER (TEMPERATURE)
M:	Liam Beguin <liambeguin@gmail.com>
R:	Peter Rosin <peda@axentia.se>
F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml

Should I also add myself as a reviewer to the iio-rescaler driver for
the temperature related changes?

Also, I noticed an issue with the offset. When we're not using a
processed channel, the upstream channel scale has to be applied to the
offset which I forgot to do. I'm working on this for v3.

Thanks for your time,
Liam

> This binding is backed by the iio-rescale driver, so it's not totally
> alien
> for me to maintain it, but I'd be more happy if you listed yourself as I
> think
> you intended to?
>
> Cheers,
> Peter
>
> >  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
> >  F:	drivers/iio/afe/iio-rescale.c
> >  
> >