* [PATCH v3 00/10] iio: afe: add temperature rescaling support
@ 2021-07-01 1:00 Liam Beguin
2021-07-01 1:00 ` [PATCH v3 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
` (9 more replies)
0 siblings, 10 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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.
Related to: https://patchwork.kernel.org/project/linux-iio/list/?series=483087
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 | 111 ++++++++++++
MAINTAINERS | 8 +
drivers/iio/afe/iio-rescale.c | 163 +++++++++++++++++-
drivers/iio/inkern.c | 44 ++++-
5 files changed, 415 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 v2:
1: eb81d6b5ed88 = 1: 42a7a1047edc iio: inkern: apply consumer scale on IIO_VAL_INT cases
2: d132d86c0dd2 = 2: a1cd89fdad11 iio: inkern: apply consumer scale when no channel scale is available
3: 7a659ccc5d7b ! 3: e74ff6b2f663 iio: inkern: error out on unsupported offset type
@@ Metadata
Author: Liam Beguin <liambeguin@gmail.com>
## Commit message ##
- iio: inkern: error out on unsupported offset type
+ iio: inkern: make a best effort on offset calculation
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.
+ 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 ##
@@ drivers/iio/inkern.c: EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
+ int offset_type, offset_val, offset_val2;
s64 raw64 = raw;
- int ret;
++ int tmp;
- ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET);
- if (ret >= 0)
@@ drivers/iio/inkern.c: EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
+ case IIO_VAL_INT:
+ break;
+ case IIO_VAL_INT_PLUS_MICRO:
-+ if (offset_val2 > 1000)
-+ return -EINVAL;
-+ break;
++ fallthrough;
+ case IIO_VAL_INT_PLUS_NANO:
-+ if (offset_val2 > 1000000)
-+ return -EINVAL;
++ /*
++ * 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:
-+ if (offset_val2 != 1)
-+ return -EINVAL;
++ tmp = offset_val / offset_val2;
++ offset_val = tmp;
+ break;
+ case IIO_VAL_FRACTIONAL_LOG2:
-+ if (offset_val2)
-+ return -EINVAL;
++ tmp = offset_val / (1 << offset_val2);
++ offset_val = tmp;
+ break;
+ default:
+ return -EINVAL;
4: 272d29f21eac < -: ------------ iio: inkern: return valid type on raw to processed conversion
5: ab38421fbae2 < -: ------------ iio: afe: rescale: add upstream offset support
-: ------------ > 4: a5696ca3c14f iio: afe: rescale: reduce risk of integer overflow
-: ------------ > 5: 2b435a2f58e8 iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
6: 59e93788dd28 ! 6: 577020b8326b iio: afe: rescale: add offset support
@@ drivers/iio/afe/iio-rescale.c: struct rescale {
static int rescale_read_raw(struct iio_dev *indio_dev,
@@ drivers/iio/afe/iio-rescale.c: static int rescale_read_raw(struct iio_dev *indio_dev,
- default:
+ 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;
+@@ drivers/iio/afe/iio-rescale.c: static int rescale_read_raw(struct iio_dev *indio_dev,
+ dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
return -EOPNOTSUPP;
}
+ case IIO_CHAN_INFO_OFFSET:
-+ *val = rescale->offset;
++ /*
++ * Processed channels are scaled 1-to-1 and source offset is
++ * already taken into account.
++ *
++ * In other cases, the final offset value is defined by:
++ * 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 < 0)
++ return ret;
++ else if (ret != IIO_VAL_INT)
++ return -EOPNOTSUPP;
++ }
+
-+ return IIO_VAL_INT;
++ 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;
++ factor = gcd(tmp, tmp2);
++ tmp /= factor;
++ tmp2 /= factor;
++ *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;
++ factor = gcd(tmp, tmp2);
++ tmp /= factor;
++ tmp2 /= factor;
++ *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;
}
7: b413cb4f190b < -: ------------ iio: afe: rescale: add temperature sensor support
8: 3f6564f5a346 < -: ------------ dt-bindings: iio: afe: add binding for temperature-sense-amplifier
-: ------------ > 7: 0add5863ff00 iio: afe: rescale: add RTD temperature sensor support
-: ------------ > 8: 0306e16020d4 iio: afe: rescale: add temperature transducers
-: ------------ > 9: 6906c5a21861 dt-bindings: iio: afe: add bindings for temperature-sense-rtd
-: ------------ > 10: ac8d4eef179b dt-bindings: iio: afe: add bindings for temperature transducers
base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
--
2.30.1.489.g328c10930387
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 02/10] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
` (8 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 UTC (permalink / raw)
To: liambeguin, peda, jic23, lars, pmeerw
Cc: linux-kernel, linux-iio, devicetree, robh+dt
From: Liam Beguin <lvb@xiphos.com>
When a consumer calls iio_read_channel_processed() and the channel has
an integer scale, the scale channel scale is applied and the processed
value is returned as expected.
On the other hand, if the consumer calls iio_convert_raw_to_processed()
the scaling factor requested by the consumer is not applied.
This for example causes the consumer to process mV when expecting uV.
Make sure to always apply the scaling factor requested by the consumer.
Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
drivers/iio/inkern.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 391a3380a1d1..b752fe5818e7 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -599,7 +599,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
switch (scale_type) {
case IIO_VAL_INT:
- *processed = raw64 * scale_val;
+ *processed = raw64 * scale_val * scale;
break;
case IIO_VAL_INT_PLUS_MICRO:
if (scale_val2 < 0)
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 02/10] iio: inkern: apply consumer scale when no channel scale is available
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
2021-07-01 1:00 ` [PATCH v3 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
` (7 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 UTC (permalink / raw)
To: liambeguin, peda, jic23, lars, pmeerw
Cc: linux-kernel, linux-iio, devicetree, robh+dt
From: Liam Beguin <lvb@xiphos.com>
When a consumer calls iio_read_channel_processed() and no channel scale
is available, it's assumed that the scale is one and the raw value is
returned as expected.
On the other hand, if the consumer calls iio_convert_raw_to_processed()
the scaling factor requested by the consumer is not applied.
This for example causes the consumer to process mV when expecting uV.
Make sure to always apply the scaling factor requested by the consumer.
Fixes: adc8ec5ff183 ("iio: inkern: pass through raw values if no scaling")
Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
drivers/iio/inkern.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b752fe5818e7..b69027690ed5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -590,10 +590,10 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
IIO_CHAN_INFO_SCALE);
if (scale_type < 0) {
/*
- * Just pass raw values as processed if no scaling is
- * available.
+ * If no channel scaling is available apply consumer scale to
+ * raw value and return.
*/
- *processed = raw;
+ *processed = raw * scale;
return 0;
}
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
2021-07-01 1:00 ` [PATCH v3 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-07-01 1:00 ` [PATCH v3 02/10] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-04 16:26 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
` (6 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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 | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b69027690ed5..e1712c1099c5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -578,13 +578,39 @@ 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;
+ int tmp;
- 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:
+ fallthrough;
+ 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:
+ tmp = offset_val / offset_val2;
+ offset_val = tmp;
+ break;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ tmp = offset_val / (1 << offset_val2);
+ offset_val = tmp;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ raw64 += offset_val;
+ }
scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
IIO_CHAN_INFO_SCALE);
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (2 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-04 16:36 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
` (5 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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.
Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
drivers/iio/afe/iio-rescale.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 774eb3044edd..98bcb5d418d6 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,11 @@ 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;
+ factor = gcd(tmp, tmp2);
+ *val = tmp / factor;
+ *val2 = tmp2 / factor;
return ret;
case IIO_VAL_INT:
*val *= rescale->numerator;
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (3 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-04 16:37 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 06/10] iio: afe: rescale: add offset support Liam Beguin
` (4 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 UTC (permalink / raw)
To: liambeguin, peda, jic23, lars, pmeerw
Cc: linux-kernel, linux-iio, devicetree, robh+dt
From: Liam Beguin <lvb@xiphos.com>
Add IIO_VAL_INT_PLUS_{NANO,MICRO} scaling support.
Scale the integer part and the decimal parts individually and keep the
original scaling type.
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 98bcb5d418d6..8f79c582519c 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -87,7 +87,16 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
do_div(tmp, 1000000000LL);
*val = tmp;
return ret;
+ case IIO_VAL_INT_PLUS_NANO:
+ fallthrough;
+ 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);
+ return ret;
default:
+ dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
return -EOPNOTSUPP;
}
default:
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 06/10] iio: afe: rescale: add offset support
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (4 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-04 16:53 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 07/10] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
` (3 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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 | 63 +++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 8f79c582519c..c8750286c308 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;
@@ -99,6 +102,62 @@ 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, the final offset value is defined by:
+ * 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 < 0)
+ return ret;
+ else if (ret != IIO_VAL_INT)
+ return -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;
+ factor = gcd(tmp, tmp2);
+ tmp /= factor;
+ tmp2 /= factor;
+ *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;
+ factor = gcd(tmp, tmp2);
+ tmp /= factor;
+ tmp2 /= factor;
+ *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;
}
@@ -175,6 +234,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
@@ -339,6 +401,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 related [flat|nested] 21+ messages in thread
* [PATCH v3 07/10] iio: afe: rescale: add RTD temperature sensor support
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (5 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 06/10] iio: afe: rescale: add offset support Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 08/10] iio: afe: rescale: add temperature transducers Liam Beguin
` (2 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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 c8750286c308..5d7fe8695b68 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -336,10 +336,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[] = {
@@ -355,6 +397,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[] = {
@@ -364,6 +410,8 @@ static const struct of_device_id rescale_match[] = {
.data = &rescale_cfg[CURRENT_SENSE_SHUNT], },
{ .compatible = "voltage-divider",
.data = &rescale_cfg[VOLTAGE_DIVIDER], },
+ { .compatible = "temperature-sense-rtd",
+ .data = &rescale_cfg[TEMP_SENSE_RTD], },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, rescale_match);
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 08/10] iio: afe: rescale: add temperature transducers
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (6 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 07/10] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-07-01 1:00 ` [PATCH v3 10/10] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
9 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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 5d7fe8695b68..e820e9f37a7d 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -377,11 +377,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[] = {
@@ -401,6 +428,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[] = {
@@ -412,6 +443,8 @@ static const struct of_device_id rescale_match[] = {
.data = &rescale_cfg[VOLTAGE_DIVIDER], },
{ .compatible = "temperature-sense-rtd",
.data = &rescale_cfg[TEMP_SENSE_RTD], },
+ { .compatible = "temperature-transducer",
+ .data = &rescale_cfg[TEMP_TRANSDUCER], },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, rescale_match);
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (7 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 08/10] iio: afe: rescale: add temperature transducers Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
2021-07-04 17:02 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 10/10] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin
9 siblings, 1 reply; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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>
---
.../iio/afe/temperature-sense-rtd.yaml | 101 ++++++++++++++++++
MAINTAINERS | 7 ++
2 files changed, 108 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..e23e74e63ec5
--- /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: 1
+
+ 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 = <1>;
+ io-channels = <&temp_adc1 0>;
+
+ excitation-current-microamp = <1000>; /* i = U/R = 5 / 5000 */
+ alpha-ppm-per-celsius = <3908>;
+ r-naught-ohms = <1000>;
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 9bf553e53f0f..ed80e671a16a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8897,6 +8897,13 @@ F: Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
F: drivers/iio/afe/iio-rescale.c
+IIO UNIT CONVERTER (TEMPERATURE)
+M: Liam Beguin <liambeguin@gmail.com>
+R: Peter Rosin <peda@axentia.se>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
+
IKANOS/ADI EAGLE ADSL USB DRIVER
M: Matthieu Castet <castet.matthieu@free.fr>
M: Stanislaw Gruszka <stf_xl@wp.pl>
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 10/10] dt-bindings: iio: afe: add bindings for temperature transducers
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
` (8 preceding siblings ...)
2021-07-01 1:00 ` [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
@ 2021-07-01 1:00 ` Liam Beguin
9 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-01 1:00 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 | 111 ++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 112 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..d16c0ade2271
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
@@ -0,0 +1,111 @@
+# 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: 1
+
+ sense-offset-millicelsius:
+ description: |
+ Temperature offset. The default is <0>.
+ This offset is commonly used to convert from Kelvins to degrees Celsius.
+ In that case, sense-offset-millicelsius would be set to <(-273150)>.
+
+ sense-resistor-ohms:
+ description: |
+ The sense resistor. Defaults to <1>.
+ Set sense-resistor-ohms to <1> when using a temperature to voltage
+ transducer.
+
+ 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 = <1>;
+ 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 = <1>;
+ io-channels = <&temp_adc 2>;
+
+ sense-offset-millicelsius = <(-273150)>; /* Kelvin to degrees Celsius */
+ alpha-ppm-per-celsius = <4000>; /* 4 mV/K */
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index ed80e671a16a..e9e11c3ea4e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8903,6 +8903,7 @@ R: Peter Rosin <peda@axentia.se>
L: linux-iio@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
+F: Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
IKANOS/ADI EAGLE ADSL USB DRIVER
M: Matthieu Castet <castet.matthieu@free.fr>
--
2.30.1.489.g328c10930387
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation
2021-07-01 1:00 ` [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
@ 2021-07-04 16:26 ` Jonathan Cameron
2021-07-04 18:03 ` Liam Beguin
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2021-07-04 16:26 UTC (permalink / raw)
To: Liam Beguin
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Wed, 30 Jun 2021 21:00:27 -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 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>
Looks good, but a few really minor comments / questions inline.
Thanks,
Jonathan
> ---
> drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index b69027690ed5..e1712c1099c5 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -578,13 +578,39 @@ 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;
> + int tmp;
>
> - 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:
> + fallthrough;
I'm fairly sure you don't need to mark fallthroughs in the case where
there is nothing in the case statement at all. That case is assumed
to be deliberate by the various static checkers. I am seeing a few
examples as you have it here in kernel, but it certainly isn't particularly common
so I'm assuming those where the result of people falsely thinking it was necessary
or the outcomes of code changes in the surrounding code.
> + 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:
> + tmp = offset_val / offset_val2;
> + offset_val = tmp;
What benefit do we get from the local variable?
offset_val /= offset_val2; would be alternative.
> + break;
> + case IIO_VAL_FRACTIONAL_LOG2:
> + tmp = offset_val / (1 << offset_val2);
> + offset_val = tmp;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + raw64 += offset_val;
> + }
>
> scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> IIO_CHAN_INFO_SCALE);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow
2021-07-01 1:00 ` [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
@ 2021-07-04 16:36 ` Jonathan Cameron
2021-07-05 4:23 ` Liam Beguin
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2021-07-04 16:36 UTC (permalink / raw)
To: Liam Beguin
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Wed, 30 Jun 2021 21:00:28 -0400
Liam Beguin <liambeguin@gmail.com> 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.
>
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
> drivers/iio/afe/iio-rescale.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 774eb3044edd..98bcb5d418d6 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,11 @@ 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;
> + factor = gcd(tmp, tmp2);
Hmm. I wonder if there are cases where this doesn't work and we end up
truncating because the gcd is say 1. If all of val, val2, rescale->numerator,
rescale->denominator are primes and the rescale values are moderately large
then that might happen. We probably need a fallback position. Perhaps
check tmp / factor and temp2/factor will fit in an int. If not, shift them until
they do even if we have to dump some precision to do so.
This stuff is getting fiddly enough we might want to figure out some self tests
that exercise the various cases.
> + *val = tmp / factor;
> + *val2 = tmp2 / factor;
This is doing 64 bit numbers divided by 32 bit ones. Doesn't that require
use of do_div() etc on 32 bit platforms?
> return ret;
> case IIO_VAL_INT:
> *val *= rescale->numerator;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
2021-07-01 1:00 ` [PATCH v3 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
@ 2021-07-04 16:37 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2021-07-04 16:37 UTC (permalink / raw)
To: Liam Beguin
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Wed, 30 Jun 2021 21:00:29 -0400
Liam Beguin <liambeguin@gmail.com> wrote:
> From: Liam Beguin <lvb@xiphos.com>
>
> Add IIO_VAL_INT_PLUS_{NANO,MICRO} scaling support.
> Scale the integer part and the decimal parts individually and keep the
> original scaling type.
>
> 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 98bcb5d418d6..8f79c582519c 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -87,7 +87,16 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> do_div(tmp, 1000000000LL);
> *val = tmp;
> return ret;
> + case IIO_VAL_INT_PLUS_NANO:
> + fallthrough;
As earlier, I'm fairly sure you don't need this (but I could be wrong,
so if I am wrong point me at a reference).
> + 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);
> + return ret;
> default:
> + dev_err(&indio_dev->dev, "unsupported type %d\n", ret);
> return -EOPNOTSUPP;
> }
> default:
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 06/10] iio: afe: rescale: add offset support
2021-07-01 1:00 ` [PATCH v3 06/10] iio: afe: rescale: add offset support Liam Beguin
@ 2021-07-04 16:53 ` Jonathan Cameron
2021-07-05 4:27 ` Liam Beguin
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2021-07-04 16:53 UTC (permalink / raw)
To: Liam Beguin
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Wed, 30 Jun 2021 21:00:30 -0400
Liam Beguin <liambeguin@gmail.com> wrote:
> 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>
Hi Liam.
A few remaining things inline.
Looking good in general.
Jonathan
> ---
> drivers/iio/afe/iio-rescale.c | 63 +++++++++++++++++++++++++++++++++++
> 1 file changed, 63 insertions(+)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 8f79c582519c..c8750286c308 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;
> @@ -99,6 +102,62 @@ 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, the final offset value is defined by:
> + * offset = schan_offset + rescaler_offset / schan_scale
Maths is right, but perhaps useful to express how this is derived as I had
to scribble it down to check.
Want to express real world measurement as:
scale * (raw + offset)
Given we applying scale and offset recursively we have.
rescaler_scale * (schan_scale * (raw + schan_offset) + rescaler_offset)
which can be rearrange into the correct form at.
rescaler_scale *schan_scale* (raw + (schan_offset + rescaler_offset/schan_scale))
Thus the scale and offset we expose to userspace should be.
scale = rescaler_scale * schan_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 < 0)
> + return ret;
if you've returned in the first branch of the if, no need to worry about the
else for checking the second condition.
if (ret != IIO_VAL_INT)
return...
> + else if (ret != IIO_VAL_INT)
> + return -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;
> + factor = gcd(tmp, tmp2);
> + tmp /= factor;
> + tmp2 /= factor;
What is the benefit to doing gcd division before div_s64?
> + *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;
> + factor = gcd(tmp, tmp2);
> + tmp /= factor;
> + tmp2 /= factor;
> + *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;
> }
> @@ -175,6 +234,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
> @@ -339,6 +401,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)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd
2021-07-01 1:00 ` [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
@ 2021-07-04 17:02 ` Jonathan Cameron
2021-07-05 4:31 ` Liam Beguin
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2021-07-04 17:02 UTC (permalink / raw)
To: Liam Beguin
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Wed, 30 Jun 2021 21:00:33 -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 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>
> ---
> .../iio/afe/temperature-sense-rtd.yaml | 101 ++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 108 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..e23e74e63ec5
> --- /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: 1
Only 1 channel, so this should be 0. For consumers of this driver
they only need to identify the device, not the device + channel.
https://github.com/devicetree-org/dt-schema/blob/master/schemas/iio/iio.yaml
> +
> + 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 = <1>;
> + io-channels = <&temp_adc1 0>;
> +
> + excitation-current-microamp = <1000>; /* i = U/R = 5 / 5000 */
> + alpha-ppm-per-celsius = <3908>;
> + r-naught-ohms = <1000>;
> + };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9bf553e53f0f..ed80e671a16a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8897,6 +8897,13 @@ F: Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
> F: drivers/iio/afe/iio-rescale.c
>
> +IIO UNIT CONVERTER (TEMPERATURE)
> +M: Liam Beguin <liambeguin@gmail.com>
> +R: Peter Rosin <peda@axentia.se>
> +L: linux-iio@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
I'm not sure we'd normally bother with a MAINTAINERS entry when it is just the binding doc
(as rest is in the driver). The binding doc itself has it's own local maintainers entry
which is the more useful one.
> +
> IKANOS/ADI EAGLE ADSL USB DRIVER
> M: Matthieu Castet <castet.matthieu@free.fr>
> M: Stanislaw Gruszka <stf_xl@wp.pl>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation
2021-07-04 16:26 ` Jonathan Cameron
@ 2021-07-04 18:03 ` Liam Beguin
0 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-04 18:03 UTC (permalink / raw)
To: Jonathan Cameron
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Sun Jul 4, 2021 at 12:26 PM EDT, Jonathan Cameron wrote:
> On Wed, 30 Jun 2021 21:00:27 -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 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>
Hi Jonathan,
Thanks for taking the time to review this again.
> Looks good, but a few really minor comments / questions inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++-----
> > 1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index b69027690ed5..e1712c1099c5 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -578,13 +578,39 @@ 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;
> > + int tmp;
> >
> > - 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:
> > + fallthrough;
>
> I'm fairly sure you don't need to mark fallthroughs in the case where
> there is nothing in the case statement at all. That case is assumed
> to be deliberate by the various static checkers. I am seeing a few
> examples as you have it here in kernel, but it certainly isn't
> particularly common
> so I'm assuming those where the result of people falsely thinking it was
> necessary
> or the outcomes of code changes in the surrounding code.
>
I thought it was always required with `-Wimplicit-fallthrough`.
Building without it gives no warnings, and after looking into it a
little, I found a bugzilla thread[1] that confirms what you're saying.
Thanks for pointing that out.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7652
> > + 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:
> > + tmp = offset_val / offset_val2;
> > + offset_val = tmp;
>
> What benefit do we get from the local variable?
> offset_val /= offset_val2; would be alternative.
>
Apologies for that, will fix!
Thanks,
Liam
> > + break;
> > + case IIO_VAL_FRACTIONAL_LOG2:
> > + tmp = offset_val / (1 << offset_val2);
> > + offset_val = tmp;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + raw64 += offset_val;
> > + }
> >
> > scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
> > IIO_CHAN_INFO_SCALE);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow
2021-07-04 16:36 ` Jonathan Cameron
@ 2021-07-05 4:23 ` Liam Beguin
2021-07-05 8:29 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: Liam Beguin @ 2021-07-05 4:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Sun Jul 4, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> On Wed, 30 Jun 2021 21:00:28 -0400
> Liam Beguin <liambeguin@gmail.com> 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.
> >
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> > drivers/iio/afe/iio-rescale.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 774eb3044edd..98bcb5d418d6 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,11 @@ 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;
> > + factor = gcd(tmp, tmp2);
>
> Hmm. I wonder if there are cases where this doesn't work and we end up
> truncating because the gcd is say 1. If all of val, val2,
> rescale->numerator,
> rescale->denominator are primes and the rescale values are moderately
> large
> then that might happen. We probably need a fallback position. Perhaps
> check tmp / factor and temp2/factor will fit in an int. If not, shift
> them until
> they do even if we have to dump some precision to do so.
>
I see what you mean. If we want to do that I guess it would also apply
to other areas of the driver.
> This stuff is getting fiddly enough we might want to figure out some
> self tests
> that exercise the various cases.
>
I never implemented kernel self tests before, I guess it should follow
the example of drivers/iio/test/iio-test-format.c?
Would you be okay to add this in a follow up series?
> > + *val = tmp / factor;
> > + *val2 = tmp2 / factor;
>
> This is doing 64 bit numbers divided by 32 bit ones. Doesn't that
> require
> use of do_div() etc on 32 bit platforms?
>
Apologies for that mistake, will fix.
> > return ret;
> > case IIO_VAL_INT:
> > *val *= rescale->numerator;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 06/10] iio: afe: rescale: add offset support
2021-07-04 16:53 ` Jonathan Cameron
@ 2021-07-05 4:27 ` Liam Beguin
0 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-05 4:27 UTC (permalink / raw)
To: Jonathan Cameron
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Sun Jul 4, 2021 at 12:53 PM EDT, Jonathan Cameron wrote:
> On Wed, 30 Jun 2021 21:00:30 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > 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>
> Hi Liam.
>
> A few remaining things inline.
>
> Looking good in general.
>
> Jonathan
>
> > ---
> > drivers/iio/afe/iio-rescale.c | 63 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 8f79c582519c..c8750286c308 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;
> > @@ -99,6 +102,62 @@ 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, the final offset value is defined by:
> > + * offset = schan_offset + rescaler_offset / schan_scale
>
> Maths is right, but perhaps useful to express how this is derived as I
> had
> to scribble it down to check.
>
> Want to express real world measurement as:
> scale * (raw + offset)
> Given we applying scale and offset recursively we have.
> rescaler_scale * (schan_scale * (raw + schan_offset) + rescaler_offset)
> which can be rearrange into the correct form at.
> rescaler_scale *schan_scale* (raw + (schan_offset +
> rescaler_offset/schan_scale))
> Thus the scale and offset we expose to userspace should be.
> scale = rescaler_scale * schan_scale
> offset = schan_offset + rescaler_offset/schan_scale;
>
Understood, I'll update the comment with more details.
>
> > + */
> > + 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 < 0)
> > + return ret;
> if you've returned in the first branch of the if, no need to worry about
> the
> else for checking the second condition.
>
> if (ret != IIO_VAL_INT)
> return...
Right, I'll combine both statements.
>
>
> > + else if (ret != IIO_VAL_INT)
> > + return -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;
> > + factor = gcd(tmp, tmp2);
> > + tmp /= factor;
> > + tmp2 /= factor;
>
> What is the benefit to doing gcd division before div_s64?
>
You're right the GCD division is unnecessary here, will drop.
> > + *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;
> > + factor = gcd(tmp, tmp2);
> > + tmp /= factor;
> > + tmp2 /= factor;
> > + *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;
> > }
> > @@ -175,6 +234,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
> > @@ -339,6 +401,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)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd
2021-07-04 17:02 ` Jonathan Cameron
@ 2021-07-05 4:31 ` Liam Beguin
0 siblings, 0 replies; 21+ messages in thread
From: Liam Beguin @ 2021-07-05 4:31 UTC (permalink / raw)
To: Jonathan Cameron
Cc: peda, lars, pmeerw, linux-kernel, linux-iio, devicetree, robh+dt
On Sun Jul 4, 2021 at 1:02 PM EDT, Jonathan Cameron wrote:
> On Wed, 30 Jun 2021 21:00:33 -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 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>
> > ---
> > .../iio/afe/temperature-sense-rtd.yaml | 101 ++++++++++++++++++
> > MAINTAINERS | 7 ++
> > 2 files changed, 108 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..e23e74e63ec5
> > --- /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: 1
>
> Only 1 channel, so this should be 0. For consumers of this driver
> they only need to identify the device, not the device + channel.
>
> https://github.com/devicetree-org/dt-schema/blob/master/schemas/iio/iio.yaml
>
Thanks for pointing that out. will fix.
> > +
> > + 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 = <1>;
> > + io-channels = <&temp_adc1 0>;
> > +
> > + excitation-current-microamp = <1000>; /* i = U/R = 5 / 5000 */
> > + alpha-ppm-per-celsius = <3908>;
> > + r-naught-ohms = <1000>;
> > + };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9bf553e53f0f..ed80e671a16a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8897,6 +8897,13 @@ F: Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> > F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
> > F: drivers/iio/afe/iio-rescale.c
> >
> > +IIO UNIT CONVERTER (TEMPERATURE)
> > +M: Liam Beguin <liambeguin@gmail.com>
> > +R: Peter Rosin <peda@axentia.se>
> > +L: linux-iio@vger.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
>
> I'm not sure we'd normally bother with a MAINTAINERS entry when it is
> just the binding doc
> (as rest is in the driver). The binding doc itself has it's own local
> maintainers entry
> which is the more useful one.
>
You're right, it makes sense to drop the changes to MAINTAINERS as the
information is already contained it the bindings.
I added this after briefly talking about it with Peter.
Thanks again for reviewing these changes,
Liam
> > +
> > IKANOS/ADI EAGLE ADSL USB DRIVER
> > M: Matthieu Castet <castet.matthieu@free.fr>
> > M: Stanislaw Gruszka <stf_xl@wp.pl>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow
2021-07-05 4:23 ` Liam Beguin
@ 2021-07-05 8:29 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2021-07-05 8:29 UTC (permalink / raw)
To: Liam Beguin
Cc: Jonathan Cameron, peda, lars, pmeerw, linux-kernel, linux-iio,
devicetree, robh+dt
On Mon, 05 Jul 2021 00:23:59 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:
> On Sun Jul 4, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> > On Wed, 30 Jun 2021 21:00:28 -0400
> > Liam Beguin <liambeguin@gmail.com> 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.
> > >
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > ---
> > > drivers/iio/afe/iio-rescale.c | 10 +++++++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > > index 774eb3044edd..98bcb5d418d6 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,11 @@ 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;
> > > + factor = gcd(tmp, tmp2);
> >
> > Hmm. I wonder if there are cases where this doesn't work and we end up
> > truncating because the gcd is say 1. If all of val, val2,
> > rescale->numerator,
> > rescale->denominator are primes and the rescale values are moderately
> > large
> > then that might happen. We probably need a fallback position. Perhaps
> > check tmp / factor and temp2/factor will fit in an int. If not, shift
> > them until
> > they do even if we have to dump some precision to do so.
> >
>
> I see what you mean. If we want to do that I guess it would also apply
> to other areas of the driver.
Certainly possible. It's a bit obscure so may not have occurred to anyone
on previous reviews :(
>
> > This stuff is getting fiddly enough we might want to figure out some
> > self tests
> > that exercise the various cases.
> >
>
> I never implemented kernel self tests before, I guess it should follow
> the example of drivers/iio/test/iio-test-format.c?
>
> Would you be okay to add this in a follow up series?
Yes, that's fine.
>
> > > + *val = tmp / factor;
> > > + *val2 = tmp2 / factor;
> >
> > This is doing 64 bit numbers divided by 32 bit ones. Doesn't that
> > require
> > use of do_div() etc on 32 bit platforms?
> >
>
> Apologies for that mistake, will fix.
>
> > > return ret;
> > > case IIO_VAL_INT:
> > > *val *= rescale->numerator;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-07-05 8:30 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-01 1:00 [PATCH v3 00/10] iio: afe: add temperature rescaling support Liam Beguin
2021-07-01 1:00 ` [PATCH v3 01/10] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-07-01 1:00 ` [PATCH v3 02/10] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
2021-07-01 1:00 ` [PATCH v3 03/10] iio: inkern: make a best effort on offset calculation Liam Beguin
2021-07-04 16:26 ` Jonathan Cameron
2021-07-04 18:03 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 04/10] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
2021-07-04 16:36 ` Jonathan Cameron
2021-07-05 4:23 ` Liam Beguin
2021-07-05 8:29 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
2021-07-04 16:37 ` Jonathan Cameron
2021-07-01 1:00 ` [PATCH v3 06/10] iio: afe: rescale: add offset support Liam Beguin
2021-07-04 16:53 ` Jonathan Cameron
2021-07-05 4:27 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 07/10] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
2021-07-01 1:00 ` [PATCH v3 08/10] iio: afe: rescale: add temperature transducers Liam Beguin
2021-07-01 1:00 ` [PATCH v3 09/10] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-07-04 17:02 ` Jonathan Cameron
2021-07-05 4:31 ` Liam Beguin
2021-07-01 1:00 ` [PATCH v3 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).