Jonathan, Peter, Andy,
I left out IIO_VAL_INT overflows for now, so that I can focus on getting
the rest of these changes pulled in, but I don't mind adding a patch for
that later on.
This series focuses on adding temperature rescaling support to the IIO
Analog Front End (AFE) driver.
The first few patches address minor bugs in IIO inkernel functions, and
prepare the AFE driver for the additional features.
The main changes to the AFE driver include an initial Kunit test suite,
support for IIO_VAL_INT_PLUS_{NANO,MICRO} scales, and support for RTDs
and temperature transducer sensors.
I applied Peter's Reviewed-by only on the unchanged commits, I wasn't
sure it would be okay to do so on the whole series with the few
changes in v11.
Thanks for your time,
Liam
Changes since v10:
- apply Andy's suggestion for offset calculations
- make use of units.h more consistently
Changes since v9:
- make use of linux/units.h
- reorder commits, fix fract_log2 before merging fract
- keep fractional representation when not overflowing
Changes since v8:
- reword comment
- fix erroneous 64-bit division
- optimize and use 32-bit divisions when values are know to not overflow
- keep IIO_VAL_FRACTIONAL scale when possible, if not default to fixed
point
- add test cases
- use nano precision in test cases
- simplify offset calculation in rtd_props()
Changes since v7:
- drop gcd() logic in rescale_process_scale()
- use div_s64() instead of do_div() for signed 64-bit divisions
- combine IIO_VAL_FRACTIONAL and IIO_VAL_FRACTIONAL_LOG2 scale cases
- switch to INT_PLUS_NANO when accuracy is lost with FRACTIONAL scales
- rework test logic to allow for small relative error
- rename test variables to align error output messages
Changes since v6:
- rework IIO_VAL_INT_PLUS_{NANO,MICRO} based on Peter's suggestion
- combine IIO_VAL_INT_PLUS_{NANO,MICRO} cases
- add test cases for negative IIO_VAL_INT_PLUS_{NANO,MICRO} corner cases
- force use of positive integers with gcd()
- reduce risk of integer overflow in IIO_VAL_FRACTIONAL_LOG2
- fix duplicate symbol build error
- apply Reviewed-by
Changes since v5:
- add include/linux/iio/afe/rescale.h
- expose functions use to process scale and offset
- add basic iio-rescale kunit test cases
- fix integer overflow case
- improve precision for IIO_VAL_FRACTIONAL_LOG2
Changes since v4:
- only use gcd() when necessary in overflow mitigation
- fix INT_PLUS_{MICRO,NANO} support
- apply Reviewed-by
- fix temperature-transducer bindings
Changes since v3:
- drop unnecessary fallthrough statements
- drop redundant local variables in some calculations
- fix s64 divisions on 32bit platforms by using do_div
- add comment describing iio-rescaler offset calculation
- drop unnecessary MAINTAINERS entry
Changes since v2:
- don't break implicit offset truncations
- make a best effort to get a valid value for fractional types
- drop return value change in iio_convert_raw_to_processed_unlocked()
- don't rely on processed value for offset calculation
- add INT_PLUS_{MICRO,NANO} support in iio-rescale
- revert generic implementation in favor of temperature-sense-rtd and
temperature-transducer
- add separate section to MAINTAINERS file
Changes since v1:
- rebase on latest iio `testing` branch
- also apply consumer scale on integer channel scale types
- don't break implicit truncation in processed channel offset
calculation
- drop temperature AFE flavors in favor of a simpler generic
implementation
Liam Beguin (15):
iio: inkern: apply consumer scale on IIO_VAL_INT cases
iio: inkern: apply consumer scale when no channel scale is available
iio: inkern: make a best effort on offset calculation
iio: afe: rescale: expose scale processing function
iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
iio: afe: rescale: add offset support
iio: afe: rescale: use s64 for temporary scale calculations
iio: afe: rescale: fix accuracy for small fractional scales
iio: afe: rescale: reduce risk of integer overflow
iio: afe: rescale: make use of units.h
iio: test: add basic tests for the iio-rescale driver
iio: afe: rescale: add RTD temperature sensor support
iio: afe: rescale: add temperature transducers
dt-bindings: iio: afe: add bindings for temperature-sense-rtd
dt-bindings: iio: afe: add bindings for temperature transducers
.../iio/afe/temperature-sense-rtd.yaml | 101 +++
.../iio/afe/temperature-transducer.yaml | 114 +++
drivers/iio/afe/iio-rescale.c | 291 +++++++-
drivers/iio/inkern.c | 40 +-
drivers/iio/test/Kconfig | 10 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-rescale.c | 705 ++++++++++++++++++
include/linux/iio/afe/rescale.h | 34 +
8 files changed, 1248 insertions(+), 48 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
create mode 100644 drivers/iio/test/iio-test-rescale.c
create mode 100644 include/linux/iio/afe/rescale.h
Range-diff against v10:
-: ------------ > 1: ae3cc93baee6 iio: inkern: apply consumer scale on IIO_VAL_INT cases
-: ------------ > 2: 06f66e7f7403 iio: inkern: apply consumer scale when no channel scale is available
1: 2dbf6b3bbaeb ! 3: 1717b82460c0 iio: inkern: make a best effort on offset calculation
@@ drivers/iio/inkern.c: EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
+ offset_val /= offset_val2;
+ break;
+ case IIO_VAL_FRACTIONAL_LOG2:
-+ offset_val /= (1 << offset_val2);
++ offset_val >>= offset_val2;
+ break;
+ default:
+ return -EINVAL;
2: b083cf307268 = 4: 6fc26588f651 iio: afe: rescale: expose scale processing function
3: f46b59690e46 = 5: 8e63c4036157 iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support
4: 80701b87cdf4 ! 6: eea57faec241 iio: afe: rescale: add offset support
@@ drivers/iio/afe/iio-rescale.c: int rescale_process_scale(struct rescale *rescale
+ *val = div_s64(tmp, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT_PLUS_NANO:
-+ tmp = (s64)rescale->offset * 1000000000;
-+ tmp2 = ((s64)scale * 1000000000L) + scale2;
++ tmp = (s64)rescale->offset * NANO;
++ tmp2 = ((s64)scale * NANO) + scale2;
+ *val = div64_s64(tmp, tmp2) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT_PLUS_MICRO:
-+ tmp = (s64)rescale->offset * 1000000L;
-+ tmp2 = ((s64)scale * 1000000L) + scale2;
++ tmp = (s64)rescale->offset * MICRO;
++ tmp2 = ((s64)scale * MICRO) + scale2;
+ *val = div64_s64(tmp, tmp2) + schan_off;
+ return IIO_VAL_INT;
+ default:
5: a3d8fb812678 = 7: 6bc5dd8c92ac iio: afe: rescale: use s64 for temporary scale calculations
6: b83947d96676 = 8: 7d426d67a7fd iio: afe: rescale: fix accuracy for small fractional scales
7: c42c8121bcdf = 9: dbea6ae8fec2 iio: afe: rescale: reduce risk of integer overflow
-: ------------ > 10: 9ab1138449d3 iio: afe: rescale: make use of units.h
8: 6c87d491a275 = 11: 3006151cd193 iio: test: add basic tests for the iio-rescale driver
9: 3f3d1939b17c ! 12: d4229e8d7f24 iio: afe: rescale: add RTD temperature sensor support
@@ drivers/iio/afe/iio-rescale.c: static int rescale_voltage_divider_props(struct d
+ return ret;
+ }
+
-+ tmp = r0 * iexc * alpha / 1000000;
-+ factor = gcd(tmp, 1000000);
-+ rescale->numerator = 1000000 / factor;
++ tmp = r0 * iexc * alpha / MICRO;
++ factor = gcd(tmp, MICRO);
++ rescale->numerator = MICRO / factor;
+ rescale->denominator = tmp / factor;
+
-+ rescale->offset = -1 * ((r0 * iexc) / 1000);
++ rescale->offset = -1 * ((r0 * iexc) / MICRO * MILLI);
+
+ return 0;
+}
10: 0c83d15f3b92 ! 13: b93e303c5e60 iio: afe: rescale: add temperature transducers
@@ drivers/iio/afe/iio-rescale.c: static int rescale_temp_sense_rtd_props(struct de
+ return ret;
+ }
+
-+ rescale->numerator = 1000000;
++ rescale->numerator = MICRO;
+ rescale->denominator = alpha * sense;
+
+ rescale->offset = div_s64((s64)offset * rescale->denominator,
11: 55af6c9391f8 = 14: 7bdd57435c5c dt-bindings: iio: afe: add bindings for temperature-sense-rtd
12: 8f9cd46c702e = 15: 604ef3e42c07 dt-bindings: iio: afe: add bindings for temperature transducers
base-commit: 2b6bff0b122785f09cfbdc34b1aa9edceea6e4c1
--
2.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
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 0222885b334c..021e1397ffc5 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -616,7 +616,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.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
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 021e1397ffc5..dbe13fad3cbb 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -607,10 +607,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.34.0
From: Liam Beguin <[email protected]>
In preparation for the addition of kunit tests, expose the logic
responsible for combining channel scales.
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 65 ++++++++++++++-------------------
include/linux/iio/afe/rescale.h | 30 +++++++++++++++
2 files changed, 58 insertions(+), 37 deletions(-)
create mode 100644 include/linux/iio/afe/rescale.h
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 774eb3044edd..d0669fd8eac5 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -11,35 +11,46 @@
#include <linux/gcd.h>
#include <linux/iio/consumer.h>
#include <linux/iio/iio.h>
+#include <linux/iio/afe/rescale.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/property.h>
-struct rescale;
-
-struct rescale_cfg {
- enum iio_chan_type type;
- int (*props)(struct device *dev, struct rescale *rescale);
-};
+int rescale_process_scale(struct rescale *rescale, int scale_type,
+ int *val, int *val2)
+{
+ unsigned long long tmp;
-struct rescale {
- const struct rescale_cfg *cfg;
- struct iio_channel *source;
- struct iio_chan_spec chan;
- struct iio_chan_spec_ext_info *ext_info;
- bool chan_processed;
- s32 numerator;
- s32 denominator;
-};
+ switch (scale_type) {
+ case IIO_VAL_FRACTIONAL:
+ *val *= rescale->numerator;
+ *val2 *= rescale->denominator;
+ return scale_type;
+ case IIO_VAL_INT:
+ *val *= rescale->numerator;
+ if (rescale->denominator == 1)
+ return scale_type;
+ *val2 = rescale->denominator;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ tmp = *val * 1000000000LL;
+ do_div(tmp, rescale->denominator);
+ tmp *= rescale->numerator;
+ do_div(tmp, 1000000000LL);
+ *val = tmp;
+ return scale_type;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
static int rescale_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
struct rescale *rescale = iio_priv(indio_dev);
- unsigned long long tmp;
int ret;
switch (mask) {
@@ -65,27 +76,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
} else {
ret = iio_read_channel_scale(rescale->source, val, val2);
}
- switch (ret) {
- case IIO_VAL_FRACTIONAL:
- *val *= rescale->numerator;
- *val2 *= rescale->denominator;
- return ret;
- case IIO_VAL_INT:
- *val *= rescale->numerator;
- if (rescale->denominator == 1)
- return ret;
- *val2 = rescale->denominator;
- return IIO_VAL_FRACTIONAL;
- case IIO_VAL_FRACTIONAL_LOG2:
- tmp = *val * 1000000000LL;
- do_div(tmp, rescale->denominator);
- tmp *= rescale->numerator;
- do_div(tmp, 1000000000LL);
- *val = tmp;
- return ret;
- default:
- return -EOPNOTSUPP;
- }
+ return rescale_process_scale(rescale, ret, val, val2);
default:
return -EINVAL;
}
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
new file mode 100644
index 000000000000..14d4ee1227c6
--- /dev/null
+++ b/include/linux/iio/afe/rescale.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 Liam Beguin <[email protected]>
+ */
+
+#ifndef __IIO_RESCALE_H__
+#define __IIO_RESCALE_H__
+
+#include <linux/iio/iio.h>
+
+struct rescale;
+
+struct rescale_cfg {
+ enum iio_chan_type type;
+ int (*props)(struct device *dev, struct rescale *rescale);
+};
+
+struct rescale {
+ const struct rescale_cfg *cfg;
+ struct iio_channel *source;
+ struct iio_chan_spec chan;
+ struct iio_chan_spec_ext_info *ext_info;
+ bool chan_processed;
+ s32 numerator;
+ s32 denominator;
+};
+
+int rescale_process_scale(struct rescale *rescale, int scale_type,
+ int *val, int *val2);
+#endif /* __IIO_RESCALE_H__ */
--
2.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
---
drivers/iio/inkern.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index dbe13fad3cbb..df74765d33dc 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -595,13 +595,35 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
int raw, int *processed, unsigned int scale)
{
- int scale_type, scale_val, scale_val2, offset;
+ int scale_type, scale_val, scale_val2;
+ int offset_type, offset_val, offset_val2;
s64 raw64 = raw;
- int ret;
- ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET);
- if (ret >= 0)
- raw64 += offset;
+ offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
+ IIO_CHAN_INFO_OFFSET);
+ if (offset_type >= 0) {
+ switch (offset_type) {
+ case IIO_VAL_INT:
+ break;
+ case IIO_VAL_INT_PLUS_MICRO:
+ case IIO_VAL_INT_PLUS_NANO:
+ /*
+ * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO
+ * implicitely truncate the offset to it's integer form.
+ */
+ break;
+ case IIO_VAL_FRACTIONAL:
+ offset_val /= offset_val2;
+ break;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ offset_val >>= offset_val2;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ raw64 += offset_val;
+ }
scale_type = iio_channel_read(chan, &scale_val, &scale_val2,
IIO_CHAN_INFO_SCALE);
--
2.34.0
From: Liam Beguin <[email protected]>
This is a preparatory change required for the addition of temperature
sensing front ends.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 80 +++++++++++++++++++++++++++++++++
include/linux/iio/afe/rescale.h | 4 ++
2 files changed, 84 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 190a83e08008..6a2d4ae80652 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -81,11 +81,46 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
}
}
+int rescale_process_offset(struct rescale *rescale, int scale_type,
+ int scale, int scale2, int schan_off,
+ int *val, int *val2)
+{
+ s64 tmp, tmp2;
+
+ switch (scale_type) {
+ case IIO_VAL_FRACTIONAL:
+ tmp = (s64)rescale->offset * scale2;
+ *val = div_s64(tmp, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT:
+ *val = div_s64(rescale->offset, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_FRACTIONAL_LOG2:
+ tmp = (s64)rescale->offset * (1 << scale2);
+ *val = div_s64(tmp, scale) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT_PLUS_NANO:
+ tmp = (s64)rescale->offset * NANO;
+ tmp2 = ((s64)scale * NANO) + scale2;
+ *val = div64_s64(tmp, tmp2) + schan_off;
+ return IIO_VAL_INT;
+ case IIO_VAL_INT_PLUS_MICRO:
+ tmp = (s64)rescale->offset * MICRO;
+ tmp2 = ((s64)scale * MICRO) + scale2;
+ *val = div64_s64(tmp, tmp2) + schan_off;
+ return IIO_VAL_INT;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static int rescale_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
struct rescale *rescale = iio_priv(indio_dev);
+ int scale, scale2;
+ int schan_off = 0;
int ret;
switch (mask) {
@@ -112,6 +147,47 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
ret = iio_read_channel_scale(rescale->source, val, val2);
}
return rescale_process_scale(rescale, ret, val, val2);
+ case IIO_CHAN_INFO_OFFSET:
+ /*
+ * Processed channels are scaled 1-to-1 and source offset is
+ * already taken into account.
+ *
+ * In other cases, real world measurement are expressed as:
+ *
+ * schan_scale * (raw + schan_offset)
+ *
+ * Given that the rescaler parameters are applied recursively:
+ *
+ * rescaler_scale * (schan_scale * (raw + schan_offset) +
+ * rescaler_offset)
+ *
+ * Or,
+ *
+ * (rescaler_scale * schan_scale) * (raw +
+ * (schan_offset + rescaler_offset / schan_scale)
+ *
+ * Thus, reusing the original expression the parameters exposed
+ * to userspace are:
+ *
+ * scale = schan_scale * rescaler_scale
+ * offset = schan_offset + rescaler_offset / schan_scale
+ */
+ if (rescale->chan_processed) {
+ *val = rescale->offset;
+ return IIO_VAL_INT;
+ }
+
+ if (iio_channel_has_info(rescale->source->channel,
+ IIO_CHAN_INFO_OFFSET)) {
+ ret = iio_read_channel_offset(rescale->source,
+ &schan_off, NULL);
+ if (ret != IIO_VAL_INT)
+ return ret < 0 ? ret : -EOPNOTSUPP;
+ }
+
+ ret = iio_read_channel_scale(rescale->source, &scale, &scale2);
+ return rescale_process_offset(rescale, ret, scale, scale2,
+ schan_off, val, val2);
default:
return -EINVAL;
}
@@ -188,6 +264,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
@@ -352,6 +431,7 @@ static int rescale_probe(struct platform_device *pdev)
rescale->cfg = of_device_get_match_data(dev);
rescale->numerator = 1;
rescale->denominator = 1;
+ rescale->offset = 0;
ret = rescale->cfg->props(dev, rescale);
if (ret)
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
index 14d4ee1227c6..b152ac487403 100644
--- a/include/linux/iio/afe/rescale.h
+++ b/include/linux/iio/afe/rescale.h
@@ -23,8 +23,12 @@ struct rescale {
bool chan_processed;
s32 numerator;
s32 denominator;
+ s32 offset;
};
int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2);
+int rescale_process_offset(struct rescale *rescale, int scale_type,
+ int scale, int scale2, int schan_off,
+ int *val, int *val2);
#endif /* __IIO_RESCALE_H__ */
--
2.34.0
From: Liam Beguin <[email protected]>
The approximation caused by integer divisions can be costly on smaller
scale values since the decimal part is significant compared to the
integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
cases to maintain accuracy.
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index a7297b4ba17e..ca8fd69bfe46 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -23,7 +23,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2)
{
s64 tmp;
- s32 rem;
+ s32 rem, rem2;
u32 mult;
u32 neg;
@@ -42,9 +42,23 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
tmp = (s64)*val * 1000000000LL;
tmp = div_s64(tmp, rescale->denominator);
tmp *= rescale->numerator;
- tmp = div_s64(tmp, 1000000000LL);
+
+ tmp = div_s64_rem(tmp, 1000000000LL, &rem);
*val = tmp;
- return scale_type;
+
+ if (!rem)
+ return scale_type;
+
+ tmp = 1 << *val2;
+
+ rem2 = *val % (int)tmp;
+ *val = *val / (int)tmp;
+
+ *val2 = rem / (int)tmp;
+ if (rem2)
+ *val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
+
+ return IIO_VAL_INT_PLUS_NANO;
case IIO_VAL_INT_PLUS_NANO:
case IIO_VAL_INT_PLUS_MICRO:
mult = scale_type == IIO_VAL_INT_PLUS_NANO ? NANO : MICRO;
--
2.34.0
From: Liam Beguin <[email protected]>
Reduce the risk of integer overflow by doing the scale calculation on
a 64-bit integer. Since the rescaling is only performed on *val, reuse
the IIO_VAL_FRACTIONAL_LOG2 case.
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index ca8fd69bfe46..0000a58bab9d 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -23,21 +23,31 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2)
{
s64 tmp;
+ int _val, _val2;
s32 rem, rem2;
u32 mult;
u32 neg;
switch (scale_type) {
- case IIO_VAL_FRACTIONAL:
- *val *= rescale->numerator;
- *val2 *= rescale->denominator;
- return scale_type;
case IIO_VAL_INT:
*val *= rescale->numerator;
if (rescale->denominator == 1)
return scale_type;
*val2 = rescale->denominator;
return IIO_VAL_FRACTIONAL;
+ case IIO_VAL_FRACTIONAL:
+ /*
+ * When the product of both scales doesn't overflow, avoid
+ * potential accuracy loss (for in kernel consumers) by
+ * keeping a fractional representation.
+ */
+ if (!check_mul_overflow(*val, rescale->numerator, &_val) &&
+ !check_mul_overflow(*val2, rescale->denominator, &_val2)) {
+ *val = _val;
+ *val2 = _val2;
+ return IIO_VAL_FRACTIONAL;
+ }
+ fallthrough;
case IIO_VAL_FRACTIONAL_LOG2:
tmp = (s64)*val * 1000000000LL;
tmp = div_s64(tmp, rescale->denominator);
@@ -49,7 +59,10 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
if (!rem)
return scale_type;
- tmp = 1 << *val2;
+ if (scale_type == IIO_VAL_FRACTIONAL)
+ tmp = *val2;
+ else
+ tmp = 1 << *val2;
rem2 = *val % (int)tmp;
*val = *val / (int)tmp;
--
2.34.0
From: Liam Beguin <[email protected]>
All four scaling coefficients can take signed values.
Make tmp a signed 64-bit integer and switch to div_s64() to preserve
signs during 64-bit divisions.
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 6a2d4ae80652..a7297b4ba17e 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -22,7 +22,7 @@
int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2)
{
- unsigned long long tmp;
+ s64 tmp;
s32 rem;
u32 mult;
u32 neg;
@@ -39,10 +39,10 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
*val2 = rescale->denominator;
return IIO_VAL_FRACTIONAL;
case IIO_VAL_FRACTIONAL_LOG2:
- tmp = *val * 1000000000LL;
- do_div(tmp, rescale->denominator);
+ tmp = (s64)*val * 1000000000LL;
+ tmp = div_s64(tmp, rescale->denominator);
tmp *= rescale->numerator;
- do_div(tmp, 1000000000LL);
+ tmp = div_s64(tmp, 1000000000LL);
*val = tmp;
return scale_type;
case IIO_VAL_INT_PLUS_NANO:
--
2.34.0
From: Liam Beguin <[email protected]>
Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types.
Add support for these to allow using the iio-rescaler with them.
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index d0669fd8eac5..190a83e08008 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -17,11 +17,15 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/property.h>
+#include <linux/units.h>
int rescale_process_scale(struct rescale *rescale, int scale_type,
int *val, int *val2)
{
unsigned long long tmp;
+ s32 rem;
+ u32 mult;
+ u32 neg;
switch (scale_type) {
case IIO_VAL_FRACTIONAL:
@@ -40,6 +44,37 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
tmp *= rescale->numerator;
do_div(tmp, 1000000000LL);
*val = tmp;
+ return scale_type;
+ case IIO_VAL_INT_PLUS_NANO:
+ case IIO_VAL_INT_PLUS_MICRO:
+ mult = scale_type == IIO_VAL_INT_PLUS_NANO ? NANO : MICRO;
+
+ /*
+ * For IIO_VAL_INT_PLUS_{MICRO,NANO} scale types if either *val
+ * OR *val2 is negative the schan scale is negative, i.e.
+ * *val = 1 and *val2 = -0.5 yields -1.5 not -0.5.
+ */
+ neg = *val < 0 || *val2 < 0;
+
+ tmp = (s64)abs(*val) * abs(rescale->numerator);
+ *val = div_s64_rem(tmp, abs(rescale->denominator), &rem);
+
+ tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator);
+ tmp = div_s64(tmp, abs(rescale->denominator));
+
+ *val += div_s64_rem(tmp, mult, val2);
+
+ /*
+ * If only one of the rescaler elements or the schan scale is
+ * negative, the combined scale is negative.
+ */
+ if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
+ if (*val)
+ *val = -*val;
+ else
+ *val2 = -*val2;
+ }
+
return scale_type;
default:
return -EOPNOTSUPP;
--
2.34.0
From: Liam Beguin <[email protected]>
Make use of well-defined SI metric prefixes to improve code readability.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 0000a58bab9d..27f99ce67b94 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -49,11 +49,11 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
}
fallthrough;
case IIO_VAL_FRACTIONAL_LOG2:
- tmp = (s64)*val * 1000000000LL;
+ tmp = (s64)*val * NANO;
tmp = div_s64(tmp, rescale->denominator);
tmp *= rescale->numerator;
- tmp = div_s64_rem(tmp, 1000000000LL, &rem);
+ tmp = div_s64_rem(tmp, NANO, &rem);
*val = tmp;
if (!rem)
@@ -69,7 +69,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
*val2 = rem / (int)tmp;
if (rem2)
- *val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
+ *val2 += div_s64((s64)rem2 * NANO, tmp);
return IIO_VAL_INT_PLUS_NANO;
case IIO_VAL_INT_PLUS_NANO:
@@ -330,8 +330,8 @@ static int rescale_current_sense_amplifier_props(struct device *dev,
* gain_div / (gain_mult * sense), while trying to keep the
* numerator/denominator from overflowing.
*/
- factor = gcd(sense, 1000000);
- rescale->numerator = 1000000 / factor;
+ factor = gcd(sense, MICRO);
+ rescale->numerator = MICRO / factor;
rescale->denominator = sense / factor;
factor = gcd(rescale->numerator, gain_mult);
@@ -359,8 +359,8 @@ static int rescale_current_sense_shunt_props(struct device *dev,
return ret;
}
- factor = gcd(shunt, 1000000);
- rescale->numerator = 1000000 / factor;
+ factor = gcd(shunt, MICRO);
+ rescale->numerator = MICRO / factor;
rescale->denominator = shunt / factor;
return 0;
--
2.34.0
From: Liam Beguin <[email protected]>
The iio-rescale driver supports various combinations of scale types and
offsets. These can often result in large integer multiplications. Make
sure these calculations are done right by adding a set of kunit test
cases that build on top of iio-test-format.
To run these tests, add the following to .kunitconfig
$ cat .kunitconfig
CONFIG_IIO=y
CONFIG_IIO_RESCALE_KUNIT_TEST=y
CONFIG_KUNIT=y
Then run:
$ ./tools/testing/kunit/kunit.py run --kunitconfig .kunitconfig
Signed-off-by: Liam Beguin <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
drivers/iio/test/Kconfig | 10 +
drivers/iio/test/Makefile | 1 +
drivers/iio/test/iio-test-rescale.c | 705 ++++++++++++++++++++++++++++
3 files changed, 716 insertions(+)
create mode 100644 drivers/iio/test/iio-test-rescale.c
diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
index 679a7794af20..872ed4ed140a 100644
--- a/drivers/iio/test/Kconfig
+++ b/drivers/iio/test/Kconfig
@@ -4,6 +4,16 @@
#
# Keep in alphabetical order
+config IIO_RESCALE_KUNIT_TEST
+ bool "Test IIO rescale conversion functions"
+ depends on KUNIT && !IIO_RESCALE
+ default KUNIT_ALL_TESTS
+ help
+ If you want to run tests on the iio-rescale code say Y here.
+
+ This takes advantage of ARCH=um to run tests and should be used by
+ developers to tests their changes to the rescaling logic.
+
config IIO_TEST_FORMAT
bool "Test IIO formatting functions"
depends on KUNIT=y
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile
index 467519a2027e..f15ae0a6394f 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -4,5 +4,6 @@
#
# Keep in alphabetical order
+obj-$(CONFIG_IIO_RESCALE_KUNIT_TEST) += iio-test-rescale.o ../afe/iio-rescale.o
obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o
CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
diff --git a/drivers/iio/test/iio-test-rescale.c b/drivers/iio/test/iio-test-rescale.c
new file mode 100644
index 000000000000..526f87fa3514
--- /dev/null
+++ b/drivers/iio/test/iio-test-rescale.c
@@ -0,0 +1,705 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kunit tests for IIO rescale conversions
+ *
+ * Copyright (c) 2021 Liam Beguin <[email protected]>
+ */
+
+#include <kunit/test.h>
+#include <linux/gcd.h>
+#include <linux/iio/afe/rescale.h>
+#include <linux/iio/iio.h>
+#include <linux/overflow.h>
+
+struct rescale_tc_data {
+ const char *name;
+
+ const s32 numerator;
+ const s32 denominator;
+ const s32 offset;
+
+ const int schan_val;
+ const int schan_val2;
+ const int schan_off;
+ const int schan_scale_type;
+
+ const char *expected;
+ const char *expected_off;
+};
+
+const struct rescale_tc_data scale_cases[] = {
+ /*
+ * Typical use cases
+ */
+ {
+ .name = "typical IIO_VAL_INT, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 42,
+ .expected = "5210.918114143",
+ },
+ {
+ .name = "typical IIO_VAL_INT, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 42,
+ .expected = "-5210.918114143",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 42,
+ .schan_val2 = 20,
+ .expected = "260.545905707",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 42,
+ .schan_val2 = 20,
+ .expected = "-260.545905707",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, positive",
+ .numerator = 42,
+ .denominator = 53,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
+ .expected = "0.049528301",
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, negative",
+ .numerator = -42,
+ .denominator = 53,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
+ .expected = "-0.049528301",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456,
+ .expected = "1240.710106203",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 1234,
+ .expected = "1240.84789",
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 1234,
+ .expected = "-1240.84789",
+ },
+ /*
+ * Use cases with small scales involving divisions
+ */
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 261/509 scaled by 90/1373754273",
+ .numerator = 261,
+ .denominator = 509,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 90,
+ .schan_val2 = 1373754273,
+ .expected = "0.000000033594",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 90/1373754273 scaled by 261/509",
+ .numerator = 90,
+ .denominator = 1373754273,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 261,
+ .schan_val2 = 509,
+ .expected = "0.000000033594",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 760/1373754273 scaled by 427/2727",
+ .numerator = 760,
+ .denominator = 1373754273,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 427,
+ .schan_val2 = 2727,
+ .expected = "0.000000086626",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 761/1373754273 scaled by 427/2727",
+ .numerator = 761,
+ .denominator = 1373754273,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 427,
+ .schan_val2 = 2727,
+ .expected = "0.000000086740",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 5/32768 scaled by 3/10000",
+ .numerator = 5,
+ .denominator = 32768,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 3,
+ .schan_val2 = 10000,
+ .expected = "0.0000000457763671875",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 1",
+ .numerator = 6,
+ .denominator = 6,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, -1 < scale < 0",
+ .numerator = -6,
+ .denominator = 6,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "-0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, 0 < scale < 2",
+ .numerator = 8,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL, -2 < scale < 0",
+ .numerator = -8,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 1,
+ .schan_val2 = 3,
+ .expected = "-1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, 760/32768 scaled by 15/22",
+ .numerator = 760,
+ .denominator = 32768,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 15,
+ .schan_val2 = 22,
+ .expected = "0.000000082946",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, 761/32768 scaled by 15/22",
+ .numerator = 761,
+ .denominator = 32768,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 15,
+ .schan_val2 = 22,
+ .expected = "0.000000083055",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 1",
+ .numerator = 16,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 4,
+ .expected = "0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, -1 < scale < 0",
+ .numerator = -16,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 4,
+ .expected = "-0.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, 0 < scale < 2",
+ .numerator = 8,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 1,
+ .expected = "1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_FRACTIONAL_LOG2, -2 < scale < 0",
+ .numerator = -8,
+ .denominator = 3,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 1,
+ .schan_val2 = 1,
+ .expected = "-1.3333333333333333",
+ },
+ {
+ .name = "small IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 1,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 5,
+ .schan_val2 = 1234,
+ .expected = "2.500617",
+ },
+ {
+ .name = "small IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -1,
+ .denominator = 2,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 5,
+ .schan_val2 = 1234,
+ .expected = "-2.500617",
+ },
+ /*
+ * INT_PLUS_{MICRO,NANO} positive/negative corner cases
+ */
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, both negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
+ .expected = "1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative",
+ .numerator = -1000000,
+ .denominator = -8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, 4 negative",
+ .numerator = -1000000,
+ .denominator = -8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = -123456,
+ .expected = "-1240.710106203",
+ },
+ {
+ .name = "negative IIO_VAL_INT_PLUS_NANO, negative, *val = 0",
+ .numerator = 1,
+ .denominator = -10,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 0,
+ .schan_val2 = 123456789,
+ .expected = "-0.012345678",
+ },
+ /*
+ * INT_PLUS_{MICRO,NANO} decimal part overflow
+ */
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "-1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = 123456789,
+ .expected = "-1256.01200856",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "16557.914267",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .expected = "-16557.914267",
+ },
+ {
+ .name = "decimal overflow IIO_VAL_INT_PLUS_MICRO, negative schan",
+ .numerator = 1000000,
+ .denominator = 8060,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = -10,
+ .schan_val2 = 123456789,
+ .expected = "-16557.914267",
+ },
+ /*
+ * 32-bit overflow conditions
+ */
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL, positive",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = S32_MAX,
+ .schan_val2 = 1,
+ .expected = "214748364.7",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = S32_MAX,
+ .schan_val2 = 1,
+ .expected = "-214748364.7",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL_LOG2, positive",
+ .numerator = S32_MAX,
+ .denominator = 4096,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 4096,
+ .schan_val2 = 16,
+ .expected = "32767.99998474121",
+ },
+ {
+ .name = "overflow IIO_VAL_FRACTIONAL_LOG2, negative",
+ .numerator = S32_MAX,
+ .denominator = 4096,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = -4096,
+ .schan_val2 = 16,
+ .expected = "-32767.99998474121",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, positive",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "-1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_NANO, negative schan",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = -10,
+ .schan_val2 = S32_MAX,
+ .expected = "-1.214748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, positive",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "215.748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, negative",
+ .numerator = -2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = S32_MAX,
+ .expected = "-215.748364",
+ },
+ {
+ .name = "overflow IIO_VAL_INT_PLUS_MICRO, negative schan",
+ .numerator = 2,
+ .denominator = 20,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = -10,
+ .schan_val2 = S32_MAX,
+ .expected = "-215.748364",
+ },
+};
+
+const struct rescale_tc_data offset_cases[] = {
+ /*
+ * Typical use cases
+ */
+ {
+ .name = "typical IIO_VAL_INT, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 123,
+ .schan_val2 = 0,
+ .schan_off = 14,
+ .expected_off = "24", /* 23.872 */
+ },
+ {
+ .name = "typical IIO_VAL_INT, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_INT,
+ .schan_val = 12,
+ .schan_val2 = 0,
+ .schan_off = 14,
+ .expected_off = "-88", /* -88.83333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 12,
+ .schan_val2 = 34,
+ .schan_off = 14,
+ .expected_off = "3510", /* 3510.333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL,
+ .schan_val = 12,
+ .schan_val2 = 34,
+ .schan_off = 14,
+ .expected_off = "-3482", /* -3482.333333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 12,
+ .schan_val2 = 16,
+ .schan_off = 14,
+ .expected_off = "6739299", /* 6739299.333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_FRACTIONAL_LOG2, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_FRACTIONAL_LOG2,
+ .schan_val = 12,
+ .schan_val2 = 16,
+ .schan_off = 14,
+ .expected_off = "-6739271", /* -6739271.333333333 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "135", /* 135.8951219647469 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_NANO, negative",
+ .offset = -1234,
+ .schan_scale_type = IIO_VAL_INT_PLUS_NANO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "-107", /* -107.89512196474689 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, positive",
+ .offset = 1234,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "23", /* 23.246438560723952 */
+ },
+ {
+ .name = "typical IIO_VAL_INT_PLUS_MICRO, negative",
+ .offset = -12345,
+ .schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
+ .schan_val = 10,
+ .schan_val2 = 123456789,
+ .schan_off = 14,
+ .expected_off = "-78", /* -78.50185091745313 */
+ },
+};
+
+static void case_to_desc(const struct rescale_tc_data *t, char *desc)
+{
+ strcpy(desc, t->name);
+}
+
+KUNIT_ARRAY_PARAM(iio_rescale_scale, scale_cases, case_to_desc);
+KUNIT_ARRAY_PARAM(iio_rescale_offset, offset_cases, case_to_desc);
+
+/**
+ * iio_str_to_nano() - Parse a fixed-point string to get an
+ * IIO_VAL_INT_PLUS_NANO value
+ * @str: The string to parse
+ * @nano: The number as an integer
+ *
+ * Returns 0 on success, or a negative error code if the string cound not be
+ * parsed.
+ */
+static int iio_str_to_nano(const char *str, s64 *nano)
+{
+ int fract_mult = 100000000LL;
+ int tmp, tmp2;
+ int ret = 0;
+
+ ret = iio_str_to_fixpoint(str, fract_mult, &tmp, &tmp2);
+ if (ret < 0)
+ return ret;
+
+ if (tmp < 0)
+ tmp2 *= -1;
+
+ *nano = (s64)tmp * 10 * fract_mult + tmp2;
+
+ return ret;
+}
+
+/**
+ * iio_test_relative_error_ppm() - Compute relative error (in parts-per-million)
+ * between two fixed-point strings
+ * @real_str: The real value as a string
+ * @exp_str: The expected value as a string
+ *
+ * Returns a negative error code if the strings cound not be parsed, or the
+ * relative error in parts-per-million.
+ */
+static int iio_test_relative_error_ppm(const char *real_str, const char *exp_str)
+{
+ s64 real, exp, err;
+ int ret;
+
+ ret = iio_str_to_nano(real_str, &real);
+ if (ret < 0)
+ return ret;
+
+ ret = iio_str_to_nano(exp_str, &exp);
+ if (ret < 0)
+ return ret;
+
+ if (!exp) {
+ pr_err("Expected value is null, relative error is undefined\n");
+ return -EINVAL;
+ }
+
+ err = 1000000 * abs(exp - real);
+ err = div64_u64(err, abs(exp));
+ return (int)err;
+}
+
+static void iio_rescale_test_scale(struct kunit *test)
+{
+ struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
+ char *buff = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+ struct rescale rescale;
+ int values[2];
+ int rel_ppm;
+ int ret;
+
+ rescale.numerator = t->numerator;
+ rescale.denominator = t->denominator;
+ rescale.offset = t->offset;
+ values[0] = t->schan_val;
+ values[1] = t->schan_val2;
+
+ ret = rescale_process_scale(&rescale, t->schan_scale_type,
+ &values[0], &values[1]);
+
+ ret = iio_format_value(buff, ret, 2, values);
+ KUNIT_EXPECT_EQ(test, (int)strlen(buff), ret);
+
+ rel_ppm = iio_test_relative_error_ppm(buff, t->expected);
+ KUNIT_EXPECT_GE_MSG(test, rel_ppm, 0, "failed to compute ppm\n");
+
+ KUNIT_EXPECT_EQ_MSG(test, rel_ppm, 0,
+ "\t real=%s"
+ "\texpected=%s\n",
+ buff, t->expected);
+}
+
+static void iio_rescale_test_offset(struct kunit *test)
+{
+ struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
+ char *buff_off = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
+ struct rescale rescale;
+ int values[2];
+ int ret;
+
+ rescale.numerator = t->numerator;
+ rescale.denominator = t->denominator;
+ rescale.offset = t->offset;
+ values[0] = t->schan_val;
+ values[1] = t->schan_val2;
+
+ ret = rescale_process_offset(&rescale, t->schan_scale_type,
+ t->schan_val, t->schan_val2, t->schan_off,
+ &values[0], &values[1]);
+
+ ret = iio_format_value(buff_off, ret, 2, values);
+ KUNIT_EXPECT_EQ(test, (int)strlen(buff_off), ret);
+
+ KUNIT_EXPECT_STREQ(test, strim(buff_off), t->expected_off);
+}
+
+static struct kunit_case iio_rescale_test_cases[] = {
+ KUNIT_CASE_PARAM(iio_rescale_test_scale, iio_rescale_scale_gen_params),
+ KUNIT_CASE_PARAM(iio_rescale_test_offset, iio_rescale_offset_gen_params),
+ {}
+};
+
+static struct kunit_suite iio_rescale_test_suite = {
+ .name = "iio-rescale",
+ .test_cases = iio_rescale_test_cases,
+};
+kunit_test_suite(iio_rescale_test_suite);
--
2.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
---
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 27f99ce67b94..8cf392500f18 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -393,10 +393,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 / MICRO;
+ factor = gcd(tmp, MICRO);
+ rescale->numerator = MICRO / factor;
+ rescale->denominator = tmp / factor;
+
+ rescale->offset = -1 * ((r0 * iexc) / MICRO * MILLI);
+
+ return 0;
+}
+
enum rescale_variant {
CURRENT_SENSE_AMPLIFIER,
CURRENT_SENSE_SHUNT,
VOLTAGE_DIVIDER,
+ TEMP_SENSE_RTD,
};
static const struct rescale_cfg rescale_cfg[] = {
@@ -412,6 +454,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[] = {
@@ -421,6 +467,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.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 8cf392500f18..a8e5771d4e79 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -434,11 +434,37 @@ static int rescale_temp_sense_rtd_props(struct device *dev,
return 0;
}
+static int rescale_temp_transducer_props(struct device *dev,
+ struct rescale *rescale)
+{
+ s32 offset = 0;
+ s32 sense = 1;
+ s32 alpha;
+ int ret;
+
+ device_property_read_u32(dev, "sense-offset-millicelsius", &offset);
+ device_property_read_u32(dev, "sense-resistor-ohms", &sense);
+ ret = device_property_read_u32(dev, "alpha-ppm-per-celsius", &alpha);
+ if (ret) {
+ dev_err(dev, "failed to read alpha-ppm-per-celsius: %d\n", ret);
+ return ret;
+ }
+
+ rescale->numerator = MICRO;
+ rescale->denominator = alpha * sense;
+
+ rescale->offset = div_s64((s64)offset * rescale->denominator,
+ rescale->numerator);
+
+ return 0;
+}
+
enum rescale_variant {
CURRENT_SENSE_AMPLIFIER,
CURRENT_SENSE_SHUNT,
VOLTAGE_DIVIDER,
TEMP_SENSE_RTD,
+ TEMP_TRANSDUCER,
};
static const struct rescale_cfg rescale_cfg[] = {
@@ -458,6 +484,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[] = {
@@ -469,6 +499,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.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
.../iio/afe/temperature-sense-rtd.yaml | 101 ++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
new file mode 100644
index 000000000000..336ce96371db
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-rtd.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/afe/temperature-sense-rtd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Temperature Sense RTD
+
+maintainers:
+ - Liam Beguin <[email protected]>
+
+description: |
+ RTDs (Resistance Temperature Detectors) are a kind of temperature sensors
+ used to get a linear voltage to temperature reading within a give range
+ (usually 0 to 100 degrees Celsius).
+
+ When an io-channel measures the output voltage across an RTD such as a
+ PT1000, the interesting measurement is almost always the corresponding
+ temperature, not the voltage output. This binding describes such a circuit.
+
+ The general transfer function here is (using SI units)
+
+ V = R(T) * iexc
+ R(T) = r0 * (1 + alpha * T)
+ T = 1 / (alpha * r0 * iexc) * (V - r0 * iexc)
+
+ The following circuit matches what's in the examples section.
+
+ 5V0
+ -----
+ |
+ +---+----+
+ | R 5k |
+ +---+----+
+ |
+ V 1mA
+ |
+ +---- Vout
+ |
+ +---+----+
+ | PT1000 |
+ +---+----+
+ |
+ -----
+ GND
+
+properties:
+ compatible:
+ const: temperature-sense-rtd
+
+ io-channels:
+ maxItems: 1
+ description: |
+ Channel node of a voltage io-channel.
+
+ '#io-channel-cells':
+ const: 0
+
+ excitation-current-microamp:
+ description: The current fed through the RTD sensor.
+
+ alpha-ppm-per-celsius:
+ description: |
+ alpha can also be expressed in micro-ohms per ohm Celsius. It's a linear
+ approximation of the resistance versus temperature relationship
+ between 0 and 100 degrees Celsius.
+
+ alpha = (R_100 - R_0) / (100 * R_0)
+
+ Where, R_100 is the resistance of the sensor at 100 degrees Celsius, and
+ R_0 (or r-naught-ohms) is the resistance of the sensor at 0 degrees
+ Celsius.
+
+ Pure platinum has an alpha of 3925. Industry standards such as IEC60751
+ and ASTM E-1137 specify an alpha of 3850.
+
+ r-naught-ohms:
+ description: |
+ Resistance of the sensor at 0 degrees Celsius.
+ Common values are 100 for PT100, 500 for PT500, and 1000 for PT1000
+
+additionalProperties: false
+required:
+ - compatible
+ - io-channels
+ - excitation-current-microamp
+ - alpha-ppm-per-celsius
+ - r-naught-ohms
+
+examples:
+ - |
+ pt1000_1: temperature-sensor0 {
+ compatible = "temperature-sense-rtd";
+ #io-channel-cells = <0>;
+ io-channels = <&temp_adc1 0>;
+
+ excitation-current-microamp = <1000>; /* i = U/R = 5 / 5000 */
+ alpha-ppm-per-celsius = <3908>;
+ r-naught-ohms = <1000>;
+ };
+...
--
2.34.0
From: Liam Beguin <[email protected]>
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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Peter Rosin <[email protected]>
---
.../iio/afe/temperature-transducer.yaml | 114 ++++++++++++++++++
1 file changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
new file mode 100644
index 000000000000..cfbf5350db27
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/temperature-transducer.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/afe/temperature-transducer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Temperature Transducer
+
+maintainers:
+ - Liam Beguin <[email protected]>
+
+description: |
+ A temperature transducer is a device that converts a thermal quantity
+ into any other physical quantity. This binding applies to temperature to
+ voltage (like the LTC2997), and temperature to current (like the AD590)
+ linear transducers.
+ In both cases these are assumed to be connected to a voltage ADC.
+
+ When an io-channel measures the output voltage of a temperature analog front
+ end such as a temperature transducer, the interesting measurement is almost
+ always the corresponding temperature, not the voltage output. This binding
+ describes such a circuit.
+
+ The general transfer function here is (using SI units)
+ V(T) = Rsense * Isense(T)
+ T = (Isense(T) / alpha) + offset
+ T = 1 / (Rsense * alpha) * (V + offset * Rsense * alpha)
+
+ When using a temperature to voltage transducer, Rsense is set to 1.
+
+ The following circuits show a temperature to current and a temperature to
+ voltage transducer that can be used with this binding.
+
+ VCC
+ -----
+ |
+ +---+---+
+ | AD590 | VCC
+ +---+---+ -----
+ | |
+ V proportional to T +----+----+
+ | D+ --+ |
+ +---- Vout | LTC2997 +--- Vout
+ | D- --+ |
+ +---+----+ +---------+
+ | Rsense | |
+ +---+----+ -----
+ | GND
+ -----
+ GND
+
+properties:
+ compatible:
+ const: temperature-transducer
+
+ io-channels:
+ maxItems: 1
+ description: |
+ Channel node of a voltage io-channel.
+
+ '#io-channel-cells':
+ const: 0
+
+ sense-offset-millicelsius:
+ description: |
+ Temperature offset.
+ This offset is commonly used to convert from Kelvins to degrees Celsius.
+ In that case, sense-offset-millicelsius would be set to <(-273150)>.
+ default: 0
+
+ sense-resistor-ohms:
+ description: |
+ The sense resistor.
+ By default sense-resistor-ohms cancels out the resistor making the
+ circuit behave like a temperature transducer.
+ default: 1
+
+ alpha-ppm-per-celsius:
+ description: |
+ Sometimes referred to as output gain, slope, or temperature coefficient.
+
+ alpha is expressed in parts per million which can be micro-amps per
+ degrees Celsius or micro-volts per degrees Celsius. The is the main
+ characteristic of a temperature transducer and should be stated in the
+ datasheet.
+
+additionalProperties: false
+
+required:
+ - compatible
+ - io-channels
+ - alpha-ppm-per-celsius
+
+examples:
+ - |
+ ad950: temperature-sensor-0 {
+ compatible = "temperature-transducer";
+ #io-channel-cells = <0>;
+ io-channels = <&temp_adc 3>;
+
+ sense-offset-millicelsius = <(-273150)>; /* Kelvin to degrees Celsius */
+ sense-resistor-ohms = <8060>;
+ alpha-ppm-per-celsius = <1>; /* 1 uA/K */
+ };
+ - |
+ znq_tmp: temperature-sensor-1 {
+ compatible = "temperature-transducer";
+ #io-channel-cells = <0>;
+ io-channels = <&temp_adc 2>;
+
+ sense-offset-millicelsius = <(-273150)>; /* Kelvin to degrees Celsius */
+ alpha-ppm-per-celsius = <4000>; /* 4 mV/K */
+ };
+...
--
2.34.0
On Wed, Dec 22, 2021 at 5:46 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> In preparation for the addition of kunit tests, expose the logic
> responsible for combining channel scales.
...
> #include <linux/gcd.h>
> #include <linux/iio/consumer.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/afe/rescale.h>
It should go before the consumer.h, no?
And I would rather move the entire IIO group of headers...
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
... somewhere here (with blank line above).
>
> -struct rescale;
...
> +#ifndef __IIO_RESCALE_H__
> +#define __IIO_RESCALE_H__
> +
> +#include <linux/iio/iio.h>
Missed types.h and forward declarations like
struct device;
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> All four scaling coefficients can take signed values.
> Make tmp a signed 64-bit integer and switch to div_s64() to preserve
> signs during 64-bit divisions.
Sounds to me like a fix with all necessary stuff needed about it:
- Fixes tag
- moving to the beginning of the series, where other fixes are
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> The approximation caused by integer divisions can be costly on smaller
> scale values since the decimal part is significant compared to the
> integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
> cases to maintain accuracy.
...
> - tmp = div_s64(tmp, 1000000000LL);
> +
> + tmp = div_s64_rem(tmp, 1000000000LL, &rem);
> *val = tmp;
> - return scale_type;
> +
It seems you may add this blank line to one of the previous patches, may you?
> + if (!rem)
> + return scale_type;
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> Reduce the risk of integer overflow by doing the scale calculation on
> a 64-bit integer. Since the rescaling is only performed on *val, reuse
> the IIO_VAL_FRACTIONAL_LOG2 case.
...
> - tmp = 1 << *val2;
At some point this should be BIT()
Rule of thumb (in accordance with C standard), always use unsigned
value as left operand of the _left_ shift.
> + if (scale_type == IIO_VAL_FRACTIONAL)
> + tmp = *val2;
> + else
> + tmp = 1 << *val2;
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> Make use of well-defined SI metric prefixes to improve code readability.
...
> case IIO_VAL_FRACTIONAL_LOG2:
> - tmp = (s64)*val * 1000000000LL;
> + tmp = (s64)*val * NANO;
> tmp = div_s64(tmp, rescale->denominator);
> tmp *= rescale->numerator;
>
> - tmp = div_s64_rem(tmp, 1000000000LL, &rem);
> + tmp = div_s64_rem(tmp, NANO, &rem);
> *val = tmp;
Thanks! The important part of this conversion is to get one trick,
i.e. NANO and GIGA are both represented by 10^9. We need to be sure
that here we use the proper sign of the power of these numbers. So
please double check in all cases that the chosen SI prefixes are
correct from the power sign point of view, e.g. it is 10^-9 and not
10^9 or otherwise.
...
> *val2 = rem / (int)tmp;
> if (rem2)
> - *val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
> + *val2 += div_s64((s64)rem2 * NANO, tmp);
Ditto here and for the rest
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> The iio-rescale driver supports various combinations of scale types and
> offsets. These can often result in large integer multiplications. Make
> sure these calculations are done right by adding a set of kunit test
> cases that build on top of iio-test-format.
...
> + int fract_mult = 100000000LL;
Perhaps also change to use the prefix?
...
> + *nano = (s64)tmp * 10 * fract_mult + tmp2;
I'm also puzzled what the meaning of the 10 is here?
...
> + err = 1000000 * abs(exp - real);
Prefix?
...
> + err = div64_u64(err, abs(exp));
> + return (int)err;
return div64_u64();
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> From: Liam Beguin <[email protected]>
>
> A temperature transducer is a device that converts a thermal quantity
> into any other physical quantity. This patch add support for temperature
This patch add --> Add a
> 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.
...
> + rescale->numerator = MICRO;
Same comment, please double check we imply 10^-6 and not 10^6 here.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Wed, Dec 22, 2021 at 12:21:01PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 5:46 AM Liam Beguin <[email protected]> wrote:
> >
> > From: Liam Beguin <[email protected]>
> >
> > In preparation for the addition of kunit tests, expose the logic
> > responsible for combining channel scales.
>
> ...
>
> > #include <linux/gcd.h>
> > #include <linux/iio/consumer.h>
> > #include <linux/iio/iio.h>
> > +#include <linux/iio/afe/rescale.h>
>
> It should go before the consumer.h, no?
I don't mind making the change, but why should it go before consumer.h?
> And I would rather move the entire IIO group of headers...
I can do that too. Do we have a convention for the ordering of #includes?
What's usually the rule/guideline for this?
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/property.h>
>
> ... somewhere here (with blank line above).
>
> >
> > -struct rescale;
>
> ...
>
> > +#ifndef __IIO_RESCALE_H__
> > +#define __IIO_RESCALE_H__
> > +
> > +#include <linux/iio/iio.h>
>
> Missed types.h and forward declarations like
> struct device;
Okay. will add linux/types.h
Cheers,
Liam
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 02:25:31PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
> >
> > From: Liam Beguin <[email protected]>
> >
> > All four scaling coefficients can take signed values.
> > Make tmp a signed 64-bit integer and switch to div_s64() to preserve
> > signs during 64-bit divisions.
>
> Sounds to me like a fix with all necessary stuff needed about it:
> - Fixes tag
> - moving to the beginning of the series, where other fixes are
Will do
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
> >
> > From: Liam Beguin <[email protected]>
> >
> > Reduce the risk of integer overflow by doing the scale calculation on
> > a 64-bit integer. Since the rescaling is only performed on *val, reuse
> > the IIO_VAL_FRACTIONAL_LOG2 case.
>
> ...
>
> > - tmp = 1 << *val2;
>
> At some point this should be BIT()
I'm not against changing this, but (to me at least) 1 << *val2 seems
more explicit as we're not working with bitfields. No?
> Rule of thumb (in accordance with C standard), always use unsigned
> value as left operand of the _left_ shift.
Right, that makes sense! In practice though, since we'll most likely
never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
enough to simply typecast?
tmp = 1 << (unsigned int)*val2;
Cheers,
Liam
> > + if (scale_type == IIO_VAL_FRACTIONAL)
> > + tmp = *val2;
> > + else
> > + tmp = 1 << *val2;
>
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 02:33:52PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
> >
> > From: Liam Beguin <[email protected]>
> >
> > Make use of well-defined SI metric prefixes to improve code readability.
>
> ...
>
> > case IIO_VAL_FRACTIONAL_LOG2:
> > - tmp = (s64)*val * 1000000000LL;
> > + tmp = (s64)*val * NANO;
> > tmp = div_s64(tmp, rescale->denominator);
> > tmp *= rescale->numerator;
> >
> > - tmp = div_s64_rem(tmp, 1000000000LL, &rem);
> > + tmp = div_s64_rem(tmp, NANO, &rem);
> > *val = tmp;
>
> Thanks! The important part of this conversion is to get one trick,
> i.e. NANO and GIGA are both represented by 10^9. We need to be sure
> that here we use the proper sign of the power of these numbers. So
> please double check in all cases that the chosen SI prefixes are
> correct from the power sign point of view, e.g. it is 10^-9 and not
> 10^9 or otherwise.
I get the difference, but I guess I'm not sure I understand how you want me to
use them. I was using NANO here as that made most sense for me.
If we go by the positive vs. negative powers of ten, I should always use
GIGA as we're multiplying by 10^9 and dividing by 10^9. Is that what you
expected?
IMO, that wouldn't be as clear, but I believe you know better.
Cheers,
Liam
> ...
>
> > *val2 = rem / (int)tmp;
> > if (rem2)
> > - *val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
> > + *val2 += div_s64((s64)rem2 * NANO, tmp);
>
> Ditto here and for the rest
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 8:20 PM Liam Beguin <[email protected]> wrote:
> On Wed, Dec 22, 2021 at 12:21:01PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 5:46 AM Liam Beguin <[email protected]> wrote:
...
> > > #include <linux/iio/consumer.h>
> > > #include <linux/iio/iio.h>
> > > +#include <linux/iio/afe/rescale.h>
> >
> > It should go before the consumer.h, no?
>
> I don't mind making the change, but why should it go before consumer.h?
'a' is earlier than 'c' in the alphabet, no?
...
> > And I would rather move the entire IIO group of headers...
>
> I can do that too. Do we have a convention for the ordering of #includes?
> What's usually the rule/guideline for this?
Guidelines suggest sorting without clear instructions. But in IIO and
pin control I suggest people use this kind of grouping.
> > > #include <linux/module.h>
> > > #include <linux/of.h>
> > > #include <linux/of_device.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/property.h>
> >
> > ... somewhere here (with blank line above).
> >
> > > -struct rescale;
...
> > Missed types.h and forward declarations like
> > struct device;
>
> Okay. will add linux/types.h
What about forward declaration?
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <[email protected]> wrote:
> On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
...
> > > - tmp = 1 << *val2;
> >
> > At some point this should be BIT()
Forgot to add, If it's 64-bit, then BIT_ULL().
> I'm not against changing this, but (to me at least) 1 << *val2 seems
> more explicit as we're not working with bitfields. No?
You may add a comment. You may use int_pow(), but it will be suboptimal.
> > Rule of thumb (in accordance with C standard), always use unsigned
> > value as left operand of the _left_ shift.
>
> Right, that makes sense! In practice though, since we'll most likely
> never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> enough to simply typecast?
>
> tmp = 1 << (unsigned int)*val2;
No, it's about the _left_ operand.
I haven't checked if tmp is 64-bit, then even that would be still wrong.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 8:51 PM Liam Beguin <[email protected]> wrote:
> On Wed, Dec 22, 2021 at 02:33:52PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
...
> > Thanks! The important part of this conversion is to get one trick,
> > i.e. NANO and GIGA are both represented by 10^9. We need to be sure
> > that here we use the proper sign of the power of these numbers. So
> > please double check in all cases that the chosen SI prefixes are
> > correct from the power sign point of view, e.g. it is 10^-9 and not
> > 10^9 or otherwise.
>
> I get the difference, but I guess I'm not sure I understand how you want me to
> use them. I was using NANO here as that made most sense for me.
>
> If we go by the positive vs. negative powers of ten, I should always use
> GIGA as we're multiplying by 10^9 and dividing by 10^9. Is that what you
> expected?
You should get the proper power after the operation.
Write a formula (mathematically speaking) and check each of them for this.
10^-5/10^-9 == 1*10^4 (Used NANO)
10^-5/10^9 == 1/10^-14 (Used GIGA)
See the difference?
In the similar way for values of e.g. 10^5.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 02:38:13PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
> >
> > From: Liam Beguin <[email protected]>
> >
> > The iio-rescale driver supports various combinations of scale types and
> > offsets. These can often result in large integer multiplications. Make
> > sure these calculations are done right by adding a set of kunit test
> > cases that build on top of iio-test-format.
>
> ...
>
> > + int fract_mult = 100000000LL;
>
> Perhaps also change to use the prefix?
Argh.. I missed this file. Sorry, will update.
> ...
>
> > + *nano = (s64)tmp * 10 * fract_mult + tmp2;
>
> I'm also puzzled what the meaning of the 10 is here?
That comes from iio_str_to_fixpoint().
I sould've added a comment to make it more explicit as details escape me
right now... Will fix.
> ...
>
> > + err = 1000000 * abs(exp - real);
>
> Prefix?
Ok
> ...
>
> > + err = div64_u64(err, abs(exp));
> > + return (int)err;
>
> return div64_u64();
will do.
Cheers,
Liam
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 08:52:30PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 8:20 PM Liam Beguin <[email protected]> wrote:
> > On Wed, Dec 22, 2021 at 12:21:01PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 22, 2021 at 5:46 AM Liam Beguin <[email protected]> wrote:
>
> ...
>
> > > > #include <linux/iio/consumer.h>
> > > > #include <linux/iio/iio.h>
> > > > +#include <linux/iio/afe/rescale.h>
> > >
> > > It should go before the consumer.h, no?
> >
> > I don't mind making the change, but why should it go before consumer.h?
>
> 'a' is earlier than 'c' in the alphabet, no?
>
> ...
>
> > > And I would rather move the entire IIO group of headers...
> >
> > I can do that too. Do we have a convention for the ordering of #includes?
> > What's usually the rule/guideline for this?
>
> Guidelines suggest sorting without clear instructions. But in IIO and
> pin control I suggest people use this kind of grouping.
Understood, will update.
> > > > #include <linux/module.h>
> > > > #include <linux/of.h>
> > > > #include <linux/of_device.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/property.h>
> > >
> > > ... somewhere here (with blank line above).
> > >
> > > > -struct rescale;
>
> ...
>
> > > Missed types.h and forward declarations like
> > > struct device;
> >
> > Okay. will add linux/types.h
>
> What about forward declaration?
I'm not sure I understand what you mean here.
Cheers,
Liam
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 9:42 PM Liam Beguin <[email protected]> wrote:
> On Wed, Dec 22, 2021 at 08:52:30PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 8:20 PM Liam Beguin <[email protected]> wrote:
> > > On Wed, Dec 22, 2021 at 12:21:01PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Dec 22, 2021 at 5:46 AM Liam Beguin <[email protected]> wrote:
...
> > > > Missed types.h and forward declarations like
^^^^ (1)
> > > > struct device;
> > >
> > > Okay. will add linux/types.h
^^^^ (2)
> > What about forward declaration?
>
> I'm not sure I understand what you mean here.
In (1) I have mentioned header and forward declaration. You agreed in
(2) to add a header. What about forward declaration?
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <[email protected]> wrote:
> > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> ...
>
> > > > - tmp = 1 << *val2;
> > >
> > > At some point this should be BIT()
>
> Forgot to add, If it's 64-bit, then BIT_ULL().
>
> > I'm not against changing this, but (to me at least) 1 << *val2 seems
> > more explicit as we're not working with bitfields. No?
>
> You may add a comment. You may use int_pow(), but it will be suboptimal.
>
> > > Rule of thumb (in accordance with C standard), always use unsigned
> > > value as left operand of the _left_ shift.
> >
> > Right, that makes sense! In practice though, since we'll most likely
> > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> > enough to simply typecast?
> >
> > tmp = 1 << (unsigned int)*val2;
>
> No, it's about the _left_ operand.
> I haven't checked if tmp is 64-bit, then even that would be still wrong.
Okay so your recommendation is to not use a left shift?
I can look into that but given how unlikely it is to fall into those bad
cases, I'd rather keep things as they are. Would that be okay?
Also, I don't think using BIT() or BIT_ULL() would address this as they
both do the same shift, with no extra checks.
Cheers,
Liam
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 09:50:28PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 9:42 PM Liam Beguin <[email protected]> wrote:
> > On Wed, Dec 22, 2021 at 08:52:30PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 22, 2021 at 8:20 PM Liam Beguin <[email protected]> wrote:
> > > > On Wed, Dec 22, 2021 at 12:21:01PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Dec 22, 2021 at 5:46 AM Liam Beguin <[email protected]> wrote:
>
> ...
>
> > > > > Missed types.h and forward declarations like
>
> ^^^^ (1)
>
> > > > > struct device;
> > > >
> > > > Okay. will add linux/types.h
>
> ^^^^ (2)
>
> > > What about forward declaration?
> >
> > I'm not sure I understand what you mean here.
>
> In (1) I have mentioned header and forward declaration. You agreed in
> (2) to add a header. What about forward declaration?
Oh, understood, sorry I misread your message.
I'll add `struct device;` above `struct rescale;`.
Cheers,
Liam
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <[email protected]> wrote:
> On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <[email protected]> wrote:
> > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
...
> > > > > - tmp = 1 << *val2;
> > > >
> > > > At some point this should be BIT()
> >
> > Forgot to add, If it's 64-bit, then BIT_ULL().
> >
> > > I'm not against changing this, but (to me at least) 1 << *val2 seems
> > > more explicit as we're not working with bitfields. No?
> >
> > You may add a comment. You may use int_pow(), but it will be suboptimal.
> >
> > > > Rule of thumb (in accordance with C standard), always use unsigned
> > > > value as left operand of the _left_ shift.
> > >
> > > Right, that makes sense! In practice though, since we'll most likely
> > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> > > enough to simply typecast?
> > >
> > > tmp = 1 << (unsigned int)*val2;
> >
> > No, it's about the _left_ operand.
> > I haven't checked if tmp is 64-bit, then even that would be still wrong.
>
> Okay so your recommendation is to not use a left shift?
No, I recommend not to use int type as a _leftside_ operand.
BIT() / BIT_ULL() does a left shift anyway.
> I can look into that but given how unlikely it is to fall into those bad
> cases, I'd rather keep things as they are. Would that be okay?
> Also, I don't think using BIT() or BIT_ULL() would address this as they
> both do the same shift, with no extra checks.
They do slightly different versions of it. They use an unsigned int type.
Open coded or not, it's up to you. Just convert to unsigned int.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Wed, Dec 22, 2021 at 11:32:24PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <[email protected]> wrote:
> > On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <[email protected]> wrote:
> > > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
>
> ...
>
> > > > > > - tmp = 1 << *val2;
> > > > >
> > > > > At some point this should be BIT()
> > >
> > > Forgot to add, If it's 64-bit, then BIT_ULL().
> > >
> > > > I'm not against changing this, but (to me at least) 1 << *val2 seems
> > > > more explicit as we're not working with bitfields. No?
> > >
> > > You may add a comment. You may use int_pow(), but it will be suboptimal.
> > >
> > > > > Rule of thumb (in accordance with C standard), always use unsigned
> > > > > value as left operand of the _left_ shift.
> > > >
> > > > Right, that makes sense! In practice though, since we'll most likely
> > > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> > > > enough to simply typecast?
> > > >
> > > > tmp = 1 << (unsigned int)*val2;
> > >
> > > No, it's about the _left_ operand.
> > > I haven't checked if tmp is 64-bit, then even that would be still wrong.
> >
> > Okay so your recommendation is to not use a left shift?
>
> No, I recommend not to use int type as a _leftside_ operand.
> BIT() / BIT_ULL() does a left shift anyway.
Oh, got it. Sorry for misreading your message.
would something like this be good enough?
s64 tmp;
u64 tmp2;
tmp2 = 1 << *val2;
tmp = tmp2;
...
How can I validate this?
> > I can look into that but given how unlikely it is to fall into those bad
> > cases, I'd rather keep things as they are. Would that be okay?
>
> > Also, I don't think using BIT() or BIT_ULL() would address this as they
> > both do the same shift, with no extra checks.
>
> They do slightly different versions of it. They use an unsigned int type.
>
> Open coded or not, it's up to you. Just convert to unsigned int.
>
> --
> With Best Regards,
> Andy Shevchenko
Cheers,
Liam
On Sat, Jan 8, 2022 at 6:34 PM Liam Beguin <[email protected]> wrote:
> On Wed, Dec 22, 2021 at 11:32:24PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <[email protected]> wrote:
> > > On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <[email protected]> wrote:
> > > > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > > > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <[email protected]> wrote:
...
> > > > > > > - tmp = 1 << *val2;
> > > > > >
> > > > > > At some point this should be BIT()
> > > >
> > > > Forgot to add, If it's 64-bit, then BIT_ULL().
> > > >
> > > > > I'm not against changing this, but (to me at least) 1 << *val2 seems
> > > > > more explicit as we're not working with bitfields. No?
> > > >
> > > > You may add a comment. You may use int_pow(), but it will be suboptimal.
> > > >
> > > > > > Rule of thumb (in accordance with C standard), always use unsigned
> > > > > > value as left operand of the _left_ shift.
> > > > >
> > > > > Right, that makes sense! In practice though, since we'll most likely
> > > > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> > > > > enough to simply typecast?
> > > > >
> > > > > tmp = 1 << (unsigned int)*val2;
> > > >
> > > > No, it's about the _left_ operand.
> > > > I haven't checked if tmp is 64-bit, then even that would be still wrong.
> > >
> > > Okay so your recommendation is to not use a left shift?
> >
> > No, I recommend not to use int type as a _leftside_ operand.
> > BIT() / BIT_ULL() does a left shift anyway.
>
> Oh, got it. Sorry for misreading your message.
> would something like this be good enough?
>
> s64 tmp;
> u64 tmp2;
> tmp2 = 1 << *val2;
This still has a UB according to the C standard. That's why
BIT()/BIT_ULL() is preferable to use since they don't have such
issues. You may open code it, of course (since I remember you wished
to show that this is not a bit, but a number).
> tmp = tmp2;
> How can I validate this?
By understanding the C standard? I dunno, actually. GCC will generate
correct code, it's just a special warning you may get when supplying a
parameter (Linux kernel doesn't use that one even on W=2 IIRC).
-Wshift-overflow=2
> > > I can look into that but given how unlikely it is to fall into those bad
> > > cases, I'd rather keep things as they are. Would that be okay?
> >
> > > Also, I don't think using BIT() or BIT_ULL() would address this as they
> > > both do the same shift, with no extra checks.
> >
> > They do slightly different versions of it. They use an unsigned int type.
> >
> > Open coded or not, it's up to you. Just convert to unsigned int.
--
With Best Regards,
Andy Shevchenko