[PATCH v4 0/6] iio: processed channel handling fixes + Intel Dollar Cove TI PMIC ADC driver

[PATCH v4 0/6] iio: processed channel handling fixes + Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Hi All,

Here is v4 of my patch to add an IIO driver for the Intel Dollar Cove TI
PMIC ADC. This has turned more into a series with fixes / changes
to iio_convert_raw_to_processed() and iio_read_channel_processed_scale(),
with the new driver tagged on as the last patch :)

Changes in v4:
- 2 new bug-fixes for iio_convert_raw_to_processed()
- Factor the bugfixed code multiply a s64 and an iio (type, val, val2)
  triplet out of iio_convert_raw_to_processed() into a new
  iio_multiply_value() helper
- Add a kunit test for iio_multiply_value()
- Redo the "Improve iio_read_channel_processed_scale() precision"
  patch using the iio_multiply_value() helper

Changes in v3:
- "iio: Improve iio_read_channel_processed_scale() precision"
  - Use div_s64() instead of div_u64() to fix -1.0 - 0.0 range
  - Directly return IIO_VAL_INT from valid cases and drop the final
    return ret after the switch-case
- "iio: adc: Add Intel Dollar Cove TI PMIC ADC driver"
  - Use new more compact DC_TI_ADC_DATA_REG_CH(x) macro
  - Use regmap_set_bits() regmap_clear_bits() where applicable
  - Use regmap_bulk_read() + be16_to_cpu() to read ADC value
  - Use sign_extend32() for vbat_zse and vbat_ge reading in probe()

Changes in v2:
- Add new "iio: Improve iio_read_channel_processed_scale() precision"
  patch to the series
- Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
- Add IIO_CHAN_INFO_PROCESSED which applies calibration and
  scaling for the VBat channel
- Address some other small review remarks

Regards,

Hans


Hans de Goede (6):
  iio: consumers: Fix handling of negative channel scale in
    iio_convert_raw_to_processed()
  iio: consumers: Fix offset handling in iio_convert_raw_to_processed()
  iio: consumers: Add an iio_multiply_value() helper function
  iio: Improve iio_read_channel_processed_scale() precision
  iio: test: Add kunit tests for iio_multiply_value()
  iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

 drivers/iio/adc/Kconfig              |  11 +
 drivers/iio/adc/Makefile             |   1 +
 drivers/iio/adc/intel_dc_ti_adc.c    | 327 +++++++++++++++++++++++++++
 drivers/iio/inkern.c                 |  77 ++++---
 drivers/iio/test/Kconfig             |  12 +
 drivers/iio/test/Makefile            |   1 +
 drivers/iio/test/iio-test-multiply.c | 209 +++++++++++++++++
 include/linux/iio/consumer.h         |  18 ++
 8 files changed, 621 insertions(+), 35 deletions(-)
 create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c
 create mode 100644 drivers/iio/test/iio-test-multiply.c

-- 
2.49.0

[PATCH v4 1/6] iio: consumers: Fix handling of negative channel scale in iio_convert_raw_to_processed()

[Hans de Goede]

There is an issue with the handling of negative channel scales
in iio_convert_raw_to_processed_unlocked() when the channel-scale
is of the IIO_VAL_INT_PLUS_[MICRO|NANO] type:

Things work for channel-scale values > -1.0 and < 0.0 because of
the use of signed values in:

	*processed += div_s64(raw64 * (s64)scale_val2 * scale, 1000000LL);

Things will break however for scale values < -1.0. Lets for example say
that raw = 2, (caller-provided)scale = 10 and (channel)scale_val = -1.5.

The result should then be 2 * 10 * -1.5 = -30.

channel-scale = -1.5 means scale_val = -1 and scale_val2 = 500000,
now lets see what gets stored in processed:

1. *processed = raw64 * scale_val * scale;
2. *processed += raw64 * scale_val2 * scale / 1000000LL;

1. Sets processed to 2 * -1 * 10 = -20
2. Adds 2 * 500000 * 10 / 1000000 = 10 to processed

And the end result is processed = -20 + 10 = -10, which is not correct.

Fix this by always using the abs value of both scale_val and scale_val2
and if either is negative multiply the end-result by -1.

Note there seems to be an unwritten rule about negative
IIO_VAL_INT_PLUS_[MICRO|NANO] values that:

i.   values > -1.0 and < 0.0 are written as val=0 val2=-xxx
ii.  values <= -1.0 are written as val=-xxx val2=xxx

But iio_format_value() will also correctly display a third option:

iii. values <= -1.0 written as val=-xxx val2=-xxx

Since iio_format_value() uses abs(val) when val2 < 0.

This fix also makes iio_convert_raw_to_processed() properly handle
channel-scales using this third option.

Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value")
Cc: Matteo Martelli <matteomartelli3@gmail.com>
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v4:
- New patch in v4 of this patch-set
---
 drivers/iio/inkern.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index c174ebb7d5e6..508534f96962 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -11,6 +11,7 @@
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/iio-opaque.h>
@@ -604,7 +605,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 {
 	int scale_type, scale_val, scale_val2;
 	int offset_type, offset_val, offset_val2;
-	s64 raw64 = raw;
+	s64 denominator, raw64 = raw;
 
 	offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
 				       IIO_CHAN_INFO_OFFSET);
@@ -648,20 +649,15 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 		*processed = raw64 * scale_val * scale;
 		break;
 	case IIO_VAL_INT_PLUS_MICRO:
-		if (scale_val2 < 0)
-			*processed = -raw64 * scale_val * scale;
-		else
-			*processed = raw64 * scale_val * scale;
-		*processed += div_s64(raw64 * (s64)scale_val2 * scale,
-				      1000000LL);
-		break;
 	case IIO_VAL_INT_PLUS_NANO:
-		if (scale_val2 < 0)
-			*processed = -raw64 * scale_val * scale;
-		else
-			*processed = raw64 * scale_val * scale;
-		*processed += div_s64(raw64 * (s64)scale_val2 * scale,
-				      1000000000LL);
+		switch (scale_type) {
+		case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
+		case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
+		}
+		*processed = raw64 * scale * abs(scale_val);
+		*processed += div_s64(raw64 * scale * abs(scale_val2), denominator);
+		if (scale_val < 0 || scale_val2 < 0)
+			*processed *= -1;
 		break;
 	case IIO_VAL_FRACTIONAL:
 		*processed = div_s64(raw64 * (s64)scale_val * scale,
-- 
2.49.0

[PATCH v4 2/6] iio: consumers: Fix offset handling in iio_convert_raw_to_processed()

[Hans de Goede]

Fix iio_convert_raw_to_processed() offset handling for channels without
a scale attribute.

The offset has been applied to the raw64 value not to the original raw
value. Use the raw64 value so that the offset is taken into account.

Fixes: 14b457fdde38 ("iio: inkern: apply consumer scale when no channel scale is available")
Cc: Liam Beguin <liambeguin@gmail.com>
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v4:
- New patch in v4 of this patch-set
---
 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 508534f96962..a75f53d11937 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -640,7 +640,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 		 * If no channel scaling is available apply consumer scale to
 		 * raw value and return.
 		 */
-		*processed = raw * scale;
+		*processed = raw64 * scale;
 		return 0;
 	}
 
-- 
2.49.0

[PATCH v4 3/6] iio: consumers: Add an iio_multiply_value() helper function

[Hans de Goede]

The channel-scale handling in iio_convert_raw_to_processed() in essence
does the following:

processed  = raw * caller-provided-scale * channel-scale

Which can also be written as:

multiplier = raw * caller-provided-scale
iio-val    = channel-scale
processed  = multiplier * iio-val

Where iioval is a set of IIO_VAL_* type + val + val2 integers, being
able to handle multiplication of iio-values like this is something
which is useful to have in general and as previous bugfixes to
iio_convert_raw_to_processed() have shown also tricky to implement.

Split the iio-value multiplication code from iio_convert_raw_to_processed()
out into a new iio_multiply_value() helper. This serves multiple purposes:

1. Having this split out allows writing a kunit test for this.
2. Having this split out allows re-use to get better precision
   when scaling values in iio_read_channel_processed_scale().

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v4:
- New patch in v4 of this patch-set
---
 drivers/iio/inkern.c         | 64 +++++++++++++++++++++---------------
 include/linux/iio/consumer.h | 18 ++++++++++
 2 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index a75f53d11937..af1da729d18e 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -599,13 +599,46 @@ int iio_read_channel_average_raw(struct iio_channel *chan, int *val)
 }
 EXPORT_SYMBOL_GPL(iio_read_channel_average_raw);
 
+int iio_multiply_value(int *result, s64 multiplier,
+		       unsigned int type, int val, int val2)
+{
+	s64 denominator;
+
+	switch (type) {
+	case IIO_VAL_INT:
+		*result = multiplier * val;
+		return IIO_VAL_INT;
+	case IIO_VAL_INT_PLUS_MICRO:
+	case IIO_VAL_INT_PLUS_NANO:
+		switch (type) {
+		case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
+		case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
+		}
+		*result = multiplier * abs(val);
+		*result += div_s64(multiplier * abs(val2), denominator);
+		if (val < 0 || val2 < 0)
+			*result *= -1;
+		return IIO_VAL_INT;
+	case IIO_VAL_FRACTIONAL:
+		*result = div_s64(multiplier * val, val2);
+		return IIO_VAL_INT;
+	case IIO_VAL_FRACTIONAL_LOG2:
+		*result = (multiplier * val) >> val2;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(iio_multiply_value);
+
 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;
 	int offset_type, offset_val, offset_val2;
-	s64 denominator, raw64 = raw;
+	s64 raw64 = raw;
+	int ret;
 
 	offset_type = iio_channel_read(chan, &offset_val, &offset_val2,
 				       IIO_CHAN_INFO_OFFSET);
@@ -644,31 +677,10 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
 		return 0;
 	}
 
-	switch (scale_type) {
-	case IIO_VAL_INT:
-		*processed = raw64 * scale_val * scale;
-		break;
-	case IIO_VAL_INT_PLUS_MICRO:
-	case IIO_VAL_INT_PLUS_NANO:
-		switch (scale_type) {
-		case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
-		case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
-		}
-		*processed = raw64 * scale * abs(scale_val);
-		*processed += div_s64(raw64 * scale * abs(scale_val2), denominator);
-		if (scale_val < 0 || scale_val2 < 0)
-			*processed *= -1;
-		break;
-	case IIO_VAL_FRACTIONAL:
-		*processed = div_s64(raw64 * (s64)scale_val * scale,
-				     scale_val2);
-		break;
-	case IIO_VAL_FRACTIONAL_LOG2:
-		*processed = (raw64 * (s64)scale_val * scale) >> scale_val2;
-		break;
-	default:
-		return -EINVAL;
-	}
+	ret = iio_multiply_value(processed, raw64 * scale,
+				 scale_type, scale_val, scale_val2);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 6a4479616479..c8c6261c81f9 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -381,6 +381,24 @@ int iio_read_channel_offset(struct iio_channel *chan, int *val,
 int iio_read_channel_scale(struct iio_channel *chan, int *val,
 			   int *val2);
 
+/**
+ * iio_multiply_value() - Multiply an iio value
+ * @result:	Destination pointer for the multiplication result
+ * @multiplier:	Multiplier.
+ * @type:	One of the IIO_VAL_* constants. This decides how the val
+ *		and val2 parameters are interpreted.
+ * @val:	Value being multiplied.
+ * @val2:	Value being multiplied. val2 use depends on type.
+ *
+ * Multiply an iio value with a s64 multiplier storing the result as
+ * IIO_VAL_INT. This is typically used for scaling.
+ *
+ * Returns:
+ * IIO_VAL_INT on success or a negative error-number on failure.
+ */
+int iio_multiply_value(int *result, s64 multiplier,
+		       unsigned int type, int val, int val2);
+
 /**
  * iio_convert_raw_to_processed() - Converts a raw value to a processed value
  * @chan:		The channel being queried
-- 
2.49.0

[PATCH v4 4/6] iio: Improve iio_read_channel_processed_scale() precision

[Hans de Goede]

Before this change iio_read_channel_processed_scale() always assumes that
channels which advertise IIO_CHAN_INFO_PROCESSED capability return
IIO_VAL_INT on success.

Ignoring any fractional values from drivers which return
IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
might become non fractional after scaling so these should be taken into
account for better precision.

Use the new iio_multiply_value() helper to do proper scaling taking
the fractionional values into account.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v4:
- Use the new iio_multiply_value() helper
---
 drivers/iio/inkern.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index af1da729d18e..2a1ecef2b820 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -722,20 +722,19 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
 				     unsigned int scale)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
-	int ret;
+	int ret, val2;
 
 	guard(mutex)(&iio_dev_opaque->info_exist_lock);
 	if (!chan->indio_dev->info)
 		return -ENODEV;
 
 	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
-		ret = iio_channel_read(chan, val, NULL,
+		ret = iio_channel_read(chan, val, &val2,
 				       IIO_CHAN_INFO_PROCESSED);
 		if (ret < 0)
 			return ret;
-		*val *= scale;
 
-		return ret;
+		return iio_multiply_value(val, scale, ret, *val, val2);
 	} else {
 		ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW);
 		if (ret < 0)
-- 
2.49.0

[PATCH v4 5/6] iio: test: Add kunit tests for iio_multiply_value()

[Hans de Goede]

Add kunit tests for iio_multiply_value().

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v4:
- New patch in v4 of this patch-set
---
 drivers/iio/test/Kconfig             |  12 ++
 drivers/iio/test/Makefile            |   1 +
 drivers/iio/test/iio-test-multiply.c | 209 +++++++++++++++++++++++++++
 3 files changed, 222 insertions(+)
 create mode 100644 drivers/iio/test/iio-test-multiply.c

diff --git a/drivers/iio/test/Kconfig b/drivers/iio/test/Kconfig
index 7a181cac3cc9..6e65e929791c 100644
--- a/drivers/iio/test/Kconfig
+++ b/drivers/iio/test/Kconfig
@@ -41,3 +41,15 @@ config IIO_FORMAT_KUNIT_TEST
 	  to the KUnit documentation in Documentation/dev-tools/kunit/.
 
 	  If unsure, say N.
+
+config IIO_MULTIPLY_KUNIT_TEST
+	tristate "Test IIO multiply functions" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  build unit tests for the IIO multiply functions.
+
+	  For more information on KUnit and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile
index e9a4cf1ff57f..0c846bc21acd 100644
--- a/drivers/iio/test/Makefile
+++ b/drivers/iio/test/Makefile
@@ -7,4 +7,5 @@
 obj-$(CONFIG_IIO_RESCALE_KUNIT_TEST) += iio-test-rescale.o
 obj-$(CONFIG_IIO_FORMAT_KUNIT_TEST) += iio-test-format.o
 obj-$(CONFIG_IIO_GTS_KUNIT_TEST) += iio-test-gts.o
+obj-$(CONFIG_IIO_MULTIPLY_KUNIT_TEST) += iio-test-multiply.o
 CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
diff --git a/drivers/iio/test/iio-test-multiply.c b/drivers/iio/test/iio-test-multiply.c
new file mode 100644
index 000000000000..41b5821ffcf5
--- /dev/null
+++ b/drivers/iio/test/iio-test-multiply.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Unit tests for IIO multiply functions
+ *
+ * Copyright (c) 2025 Hans de Goede <hans@hansg.org>
+ * Based on iio-test-format.c which is:
+ * Copyright (c) 2020 Lars-Peter Clausen <lars@metafoo.de>
+ */
+
+#include <kunit/test.h>
+#include <linux/iio/consumer.h>
+
+static void __iio_test_iio_multiply_value_integer(struct kunit *test, s64 multiplier)
+{
+	int ret, result, val;
+
+	val = 42;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT, val, 0);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, multiplier * val);
+
+	val = -23;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT, val, 0);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, multiplier * val);
+
+	val = 0;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT, val, 0);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, multiplier * val);
+}
+
+static void iio_test_iio_multiply_value_integer(struct kunit *test)
+{
+	__iio_test_iio_multiply_value_integer(test, 20);
+	__iio_test_iio_multiply_value_integer(test, -20);
+}
+
+static void __iio_test_iio_multiply_value_fixedpoint(struct kunit *test, s64 multiplier)
+{
+	int ret, result, val, val2;
+
+	/* positive >= 1 (1.5) */
+	val = 1;
+	val2 = 500000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_MICRO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * 15, 10));
+
+	val = 1;
+	val2 = 500000000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_NANO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * 15, 10));
+
+	/* positive < 1 (0.5) */
+	val = 0;
+	val2 = 500000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_MICRO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * 5, 10));
+
+	val = 0;
+	val2 = 500000000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_NANO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * 5, 10));
+
+	/* negative <= -1 (-1.5) */
+	val = -1;
+	val2 = 500000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_MICRO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * -15, 10));
+
+	val = -1;
+	val2 = 500000000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_NANO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * -15, 10));
+
+	/* negative > -1 (-0.5) */
+	val = 0;
+	val2 = -500000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_MICRO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * -5, 10));
+
+	val = 0;
+	val2 = -500000000;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_INT_PLUS_NANO, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * -5, 10));
+}
+
+static void iio_test_iio_multiply_value_fixedpoint(struct kunit *test)
+{
+	__iio_test_iio_multiply_value_fixedpoint(test, 20);
+	__iio_test_iio_multiply_value_fixedpoint(test, -20);
+}
+
+static void __iio_test_iio_multiply_value_fractional(struct kunit *test, s64 multiplier)
+{
+	int ret, result, val, val2;
+
+	/* positive < 1 (1/10)*/
+	val = 1;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * val, val2));
+
+	/* positive >= 1 (100/3)*/
+	val = 100;
+	val2 = 3;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * val, val2));
+
+	/* negative > -1 (-1/10) */
+	val = -1;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * val, val2));
+
+	/* negative <= -1 (-200/3)*/
+	val = -200;
+	val2 = 3;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * val, val2));
+
+	/* Zero (0/-10) */
+	val = 0;
+	val2 = -10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, div_s64(multiplier * val, val2));
+}
+
+static void iio_test_iio_multiply_value_fractional(struct kunit *test)
+{
+	__iio_test_iio_multiply_value_fractional(test, 20);
+	__iio_test_iio_multiply_value_fractional(test, -20);
+}
+
+static void __iio_test_iio_multiply_value_fractional_log2(struct kunit *test, s64 multiplier)
+{
+	int ret, result, val, val2;
+
+	/* positive < 1 (123/1024) */
+	val = 123;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL_LOG2, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, (multiplier * val) >> val2);
+
+	/* positive >= 1 (1234567/1024) */
+	val = 1234567;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL_LOG2, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, (multiplier * val) >> val2);
+
+	/* negative > -1 (-123/1024) */
+	val = -123;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL_LOG2, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, (multiplier * val) >> val2);
+
+	/* negative <= -1 (-1234567/1024) */
+	val = -1234567;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL_LOG2, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, (multiplier * val) >> val2);
+
+	/* Zero (0/1024) */
+	val = 0;
+	val2 = 10;
+	ret = iio_multiply_value(&result, multiplier, IIO_VAL_FRACTIONAL_LOG2, val, val2);
+	KUNIT_EXPECT_EQ(test, ret, IIO_VAL_INT);
+	KUNIT_EXPECT_EQ(test, result, (multiplier * val) >> val2);
+}
+
+static void iio_test_iio_multiply_value_fractional_log2(struct kunit *test)
+{
+	__iio_test_iio_multiply_value_fractional_log2(test, 20);
+	__iio_test_iio_multiply_value_fractional_log2(test, -20);
+}
+
+static struct kunit_case iio_multiply_test_cases[] = {
+		KUNIT_CASE(iio_test_iio_multiply_value_integer),
+		KUNIT_CASE(iio_test_iio_multiply_value_fixedpoint),
+		KUNIT_CASE(iio_test_iio_multiply_value_fractional),
+		KUNIT_CASE(iio_test_iio_multiply_value_fractional_log2),
+		{ }
+};
+
+static struct kunit_suite iio_multiply_test_suite = {
+	.name = "iio-multiply",
+	.test_cases = iio_multiply_test_cases,
+};
+kunit_test_suite(iio_multiply_test_suite);
+
+MODULE_AUTHOR("Hans de Goede <hans@hansg.org>");
+MODULE_DESCRIPTION("Test IIO multiply functions");
+MODULE_LICENSE("GPL");
-- 
2.49.0

[PATCH v4 6/6] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.

The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".

Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
binding to the "chtdc_ti_adc" MFD cell of the MFD driver.

The "cht" in the cell name comes from Cherry Trail, but it turns out that
just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
not include the cht part in its name.

This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
from the Acer A1-840 Android kernel source-code archive named:
"App. Guide_Acer_20151221_A_A.zip"
which is distributed by Acer from the Acer A1-840 support page:
https://www.acer.com/us-en/support/product-support/A1-840/downloads

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v4:
- No changes

Changes in v3:
- Use new more compact DC_TI_ADC_DATA_REG_CH(x) macro
- Use regmap_set_bits(), regmap_clear_bits() where applicable
- Use regmap_bulk_read() + be16_to_cpu() to read ADC value
- Use sign_extend32() for vbat_zse and vbat_ge reading in probe()

Changes in v2:
- Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
- Add IIO_CHAN_INFO_PROCESSED which applies calibration and
  scaling for the VBat channel
- Address some other small review remarks
---
 drivers/iio/adc/Kconfig           |  11 +
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/intel_dc_ti_adc.c | 327 ++++++++++++++++++++++++++++++
 3 files changed, 339 insertions(+)
 create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ea3ba1397392..51a5fc6efbe1 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -723,6 +723,17 @@ config INGENIC_ADC
 	  This driver can also be built as a module. If so, the module will be
 	  called ingenic_adc.
 
+config INTEL_DC_TI_ADC
+	tristate "Intel Bay / Cherry Trail Dollar Cove TI ADC driver"
+	depends on INTEL_SOC_PMIC_CHTDC_TI
+	help
+	  Say yes here to have support for the Dollar Cove TI PMIC ADC device.
+	  Depending on platform configuration, this general purpose ADC can be
+	  used for sensors such as battery voltage and thermal resistors.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called intel_dc_ti_adc.
+
 config INTEL_MRFLD_ADC
 	tristate "Intel Merrifield Basin Cove ADC driver"
 	depends on INTEL_SOC_PMIC_MRFLD
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 09ae6edb2650..da99ba88b4e1 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
 obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
 obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
 obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
+obj-$(CONFIG_INTEL_DC_TI_ADC) += intel_dc_ti_adc.o
 obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
new file mode 100644
index 000000000000..fcf76e651135
--- /dev/null
+++ b/drivers/iio/adc/intel_dc_ti_adc.c
@@ -0,0 +1,327 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Dollar Cove TI PMIC GPADC Driver
+ *
+ * Copyright (C) 2014 Intel Corporation (Ramakrishna Pallala <ramakrishna.pallala@intel.com>)
+ * Copyright (C) 2024 - 2025 Hans de Goede <hansg@kernel.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/wait.h>
+
+#include <linux/iio/driver.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+
+#define DC_TI_ADC_CNTL_REG			0x50
+#define DC_TI_ADC_START				BIT(0)
+#define DC_TI_ADC_CH_SEL			GENMASK(2, 1)
+#define DC_TI_ADC_EN				BIT(5)
+#define DC_TI_ADC_EN_EXT_BPTH_BIAS		BIT(6)
+
+#define DC_TI_VBAT_ZSE_GE_REG			0x53
+#define DC_TI_VBAT_GE				GENMASK(3, 0)
+#define DC_TI_VBAT_ZSE				GENMASK(7, 4)
+
+/* VBAT GE gain correction is in 0.0015 increments, ZSE is in 1.0 increments */
+#define DC_TI_VBAT_GE_STEP			15
+#define DC_TI_VBAT_GE_DIV			10000
+
+#define DC_TI_ADC_DATA_REG_CH(x)		(0x54 + 2 * (x))
+
+enum dc_ti_adc_id {
+	DC_TI_ADC_VBAT,
+	DC_TI_ADC_PMICTEMP,
+	DC_TI_ADC_BATTEMP,
+	DC_TI_ADC_SYSTEMP0,
+};
+
+struct dc_ti_adc_info {
+	struct mutex lock; /* Protects against concurrent accesses to the ADC */
+	wait_queue_head_t wait;
+	struct device *dev;
+	struct regmap *regmap;
+	int vbat_zse;
+	int vbat_ge;
+	bool conversion_done;
+};
+
+static const struct iio_chan_spec dc_ti_adc_channels[] = {
+	{
+		.indexed = 1,
+		.type = IIO_VOLTAGE,
+		.channel = DC_TI_ADC_VBAT,
+		.address = DC_TI_ADC_DATA_REG_CH(0),
+		.datasheet_name = "CH0",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_PROCESSED),
+	}, {
+		.indexed = 1,
+		.type = IIO_TEMP,
+		.channel = DC_TI_ADC_PMICTEMP,
+		.address = DC_TI_ADC_DATA_REG_CH(1),
+		.datasheet_name = "CH1",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.indexed = 1,
+		.type = IIO_TEMP,
+		.channel = DC_TI_ADC_BATTEMP,
+		.address = DC_TI_ADC_DATA_REG_CH(2),
+		.datasheet_name = "CH2",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.indexed = 1,
+		.type = IIO_TEMP,
+		.channel = DC_TI_ADC_SYSTEMP0,
+		.address = DC_TI_ADC_DATA_REG_CH(3),
+		.datasheet_name = "CH3",
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}
+};
+
+static struct iio_map dc_ti_adc_default_maps[] = {
+	IIO_MAP("CH0", "chtdc_ti_battery", "VBAT"),
+	IIO_MAP("CH1", "chtdc_ti_battery", "PMICTEMP"),
+	IIO_MAP("CH2", "chtdc_ti_battery", "BATTEMP"),
+	IIO_MAP("CH3", "chtdc_ti_battery", "SYSTEMP0"),
+	{ }
+};
+
+static irqreturn_t dc_ti_adc_isr(int irq, void *data)
+{
+	struct dc_ti_adc_info *info = data;
+
+	info->conversion_done = true;
+	wake_up(&info->wait);
+	return IRQ_HANDLED;
+}
+
+static int dc_ti_adc_scale(struct dc_ti_adc_info *info,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2)
+{
+	if (chan->channel != DC_TI_ADC_VBAT)
+		return -EINVAL;
+
+	/* Vbat ADC scale is 4.6875 mV / unit */
+	*val = 4;
+	*val2 = 687500;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int dc_ti_adc_raw_to_processed(struct dc_ti_adc_info *info,
+				      struct iio_chan_spec const *chan,
+				      int raw, int *val, int *val2)
+{
+	if (chan->channel != DC_TI_ADC_VBAT)
+		return -EINVAL;
+
+	/* Apply calibration */
+	raw -= info->vbat_zse;
+	raw = raw * (DC_TI_VBAT_GE_DIV - info->vbat_ge * DC_TI_VBAT_GE_STEP) /
+	      DC_TI_VBAT_GE_DIV;
+	/* Vbat ADC scale is 4.6875 mV / unit */
+	raw *= 46875;
+
+	/* raw is now in 10000 units / mV, convert to milli + milli/1e6 */
+	*val = raw / 10000;
+	*val2 = (raw % 10000) * 100;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int dc_ti_adc_sample(struct dc_ti_adc_info *info,
+			    struct iio_chan_spec const *chan, int *val)
+{
+	int ret, ch = chan->channel;
+	__be16 buf;
+
+	info->conversion_done = false;
+
+	/*
+	 * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
+	 * not be sent together. Hence send the commands separately
+	 */
+	ret = regmap_set_bits(info->regmap, DC_TI_ADC_CNTL_REG, DC_TI_ADC_EN);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				 DC_TI_ADC_CH_SEL,
+				 FIELD_PREP(DC_TI_ADC_CH_SEL, ch));
+	if (ret)
+		return ret;
+
+	/*
+	 * As per PMIC Vendor, a minimum of 50 micro seconds delay is required
+	 * between ADC Enable and ADC START commands. This is also recommended
+	 * by Intel Hardware team after the timing analysis of GPADC signals.
+	 * Since the I2C Write transaction to set the channel number also
+	 * imparts 25 micro seconds of delay, so we need to wait for another
+	 * 25 micro seconds before issuing ADC START command.
+	 */
+	fsleep(25);
+
+	ret = regmap_set_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+			      DC_TI_ADC_START);
+	if (ret)
+		return ret;
+
+	/* TI (PMIC Vendor) recommends 5 sec timeout for conversion */
+	ret = wait_event_timeout(info->wait, info->conversion_done, 5 * HZ);
+	if (ret == 0) {
+		ret = -ETIMEDOUT;
+		goto disable_adc;
+	}
+
+	ret = regmap_bulk_read(info->regmap, chan->address, &buf, sizeof(buf));
+	if (ret)
+		goto disable_adc;
+
+	/* The ADC values are 10 bits */
+	*val = be16_to_cpu(buf) & GENMASK(9, 0);
+
+disable_adc:
+	regmap_clear_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+			  DC_TI_ADC_START | DC_TI_ADC_EN);
+	return ret;
+}
+
+static int dc_ti_adc_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2, long mask)
+{
+	struct dc_ti_adc_info *info = iio_priv(indio_dev);
+	int ret;
+
+	if (mask == IIO_CHAN_INFO_SCALE)
+		return dc_ti_adc_scale(info, chan, val, val2);
+
+	guard(mutex)(&info->lock);
+
+	/*
+	 * If channel BPTHERM has been selected, first enable the BPTHERM BIAS
+	 * which provides the VREF Voltage reference to convert BPTHERM Input
+	 * voltage to temperature.
+	 * As per PMIC Vendor specifications, BPTHERM BIAS should be enabled
+	 * 35 ms before ADC_EN command.
+	 */
+	if (chan->channel == DC_TI_ADC_BATTEMP) {
+		ret = regmap_set_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				      DC_TI_ADC_EN_EXT_BPTH_BIAS);
+		if (ret)
+			return ret;
+		msleep(35);
+	}
+
+	ret = dc_ti_adc_sample(info, chan, val);
+
+	if (chan->channel == DC_TI_ADC_BATTEMP)
+		regmap_clear_bits(info->regmap, DC_TI_ADC_CNTL_REG,
+				  DC_TI_ADC_EN_EXT_BPTH_BIAS);
+
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PROCESSED:
+		return dc_ti_adc_raw_to_processed(info, chan, *val, val, val2);
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info dc_ti_adc_iio_info = {
+	.read_raw = dc_ti_adc_read_raw,
+};
+
+static int dc_ti_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
+	struct dc_ti_adc_info *info;
+	struct iio_dev *indio_dev;
+	unsigned int val;
+	int irq, ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	ret = devm_mutex_init(dev, &info->lock);
+	if (ret)
+		return ret;
+
+	init_waitqueue_head(&info->wait);
+
+	info->dev = dev;
+	info->regmap = pmic->regmap;
+
+	indio_dev->name = "dc_ti_adc";
+	indio_dev->channels = dc_ti_adc_channels;
+	indio_dev->num_channels = ARRAY_SIZE(dc_ti_adc_channels);
+	indio_dev->info = &dc_ti_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = regmap_read(info->regmap, DC_TI_VBAT_ZSE_GE_REG, &val);
+	if (ret)
+		return ret;
+
+	info->vbat_zse = sign_extend32(FIELD_GET(DC_TI_VBAT_ZSE, val), 3);
+	info->vbat_ge = sign_extend32(FIELD_GET(DC_TI_VBAT_GE, val), 3);
+
+	dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
+
+	ret = devm_iio_map_array_register(dev, indio_dev, dc_ti_adc_default_maps);
+	if (ret)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_adc_isr,
+					IRQF_ONESHOT, indio_dev->name, info);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct platform_device_id dc_ti_adc_ids[] = {
+	{ .name = "chtdc_ti_adc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, dc_ti_adc_ids);
+
+static struct platform_driver dc_ti_adc_driver = {
+	.driver = {
+		.name	= "dc_ti_adc",
+	},
+	.probe		= dc_ti_adc_probe,
+	.id_table	= dc_ti_adc_ids,
+};
+module_platform_driver(dc_ti_adc_driver);
+
+MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_DESCRIPTION("Intel Dollar Cove (TI) GPADC Driver");
+MODULE_LICENSE("GPL");
-- 
2.49.0

Re: [PATCH v4 1/6] iio: consumers: Fix handling of negative channel scale in iio_convert_raw_to_processed()

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
>
> There is an issue with the handling of negative channel scales
> in iio_convert_raw_to_processed_unlocked() when the channel-scale
> is of the IIO_VAL_INT_PLUS_[MICRO|NANO] type:
>
> Things work for channel-scale values > -1.0 and < 0.0 because of
> the use of signed values in:
>
>         *processed += div_s64(raw64 * (s64)scale_val2 * scale, 1000000LL);
>
> Things will break however for scale values < -1.0. Lets for example say
> that raw = 2, (caller-provided)scale = 10 and (channel)scale_val = -1.5.
>
> The result should then be 2 * 10 * -1.5 = -30.
>
> channel-scale = -1.5 means scale_val = -1 and scale_val2 = 500000,
> now lets see what gets stored in processed:
>
> 1. *processed = raw64 * scale_val * scale;
> 2. *processed += raw64 * scale_val2 * scale / 1000000LL;
>
> 1. Sets processed to 2 * -1 * 10 = -20
> 2. Adds 2 * 500000 * 10 / 1000000 = 10 to processed
>
> And the end result is processed = -20 + 10 = -10, which is not correct.
>
> Fix this by always using the abs value of both scale_val and scale_val2
> and if either is negative multiply the end-result by -1.
>
> Note there seems to be an unwritten rule about negative
> IIO_VAL_INT_PLUS_[MICRO|NANO] values that:
>
> i.   values > -1.0 and < 0.0 are written as val=0 val2=-xxx
> ii.  values <= -1.0 are written as val=-xxx val2=xxx
>
> But iio_format_value() will also correctly display a third option:
>
> iii. values <= -1.0 written as val=-xxx val2=-xxx
>
> Since iio_format_value() uses abs(val) when val2 < 0.
>
> This fix also makes iio_convert_raw_to_processed() properly handle
> channel-scales using this third option.

...

> +               switch (scale_type) {
> +               case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
> +               case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
> +               }

Now wondering if checkpatch et al. are happy with this style. Not a
big deal personally to me, but if we have warnings from the tools
perhaps it's better to avoid them.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/6] iio: consumers: Add an iio_multiply_value() helper function

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
>
> The channel-scale handling in iio_convert_raw_to_processed() in essence
> does the following:
>
> processed  = raw * caller-provided-scale * channel-scale
>
> Which can also be written as:
>
> multiplier = raw * caller-provided-scale
> iio-val    = channel-scale
> processed  = multiplier * iio-val
>
> Where iioval is a set of IIO_VAL_* type + val + val2 integers, being

Typo, should be iio-val.
In some cases you use iio-value. Can you make the same things to be
spelled in the same way?

> able to handle multiplication of iio-values like this is something
> which is useful to have in general and as previous bugfixes to

and, as

> iio_convert_raw_to_processed() have shown also tricky to implement.

shown. also

> Split the iio-value multiplication code from iio_convert_raw_to_processed()
> out into a new iio_multiply_value() helper. This serves multiple purposes:
>
> 1. Having this split out allows writing a kunit test for this.

KUnit

> 2. Having this split out allows re-use to get better precision
>    when scaling values in iio_read_channel_processed_scale().

...

> +int iio_multiply_value(int *result, s64 multiplier,
> +                      unsigned int type, int val, int val2)
> +{
> +       s64 denominator;
> +
> +       switch (type) {
> +       case IIO_VAL_INT:
> +               *result = multiplier * val;
> +               return IIO_VAL_INT;
> +       case IIO_VAL_INT_PLUS_MICRO:
> +       case IIO_VAL_INT_PLUS_NANO:
> +               switch (type) {
> +               case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
> +               case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
> +               }
> +               *result = multiplier * abs(val);
> +               *result += div_s64(multiplier * abs(val2), denominator);
> +               if (val < 0 || val2 < 0)
> +                       *result *= -1;
> +               return IIO_VAL_INT;
> +       case IIO_VAL_FRACTIONAL:
> +               *result = div_s64(multiplier * val, val2);
> +               return IIO_VAL_INT;
> +       case IIO_VAL_FRACTIONAL_LOG2:
> +               *result = (multiplier * val) >> val2;
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(iio_multiply_value);

Is it in the namespace? I think we already discussed with Jonathan
that IIO_CORE or something can be started if not yet for this kind of
exported symbols (and it doesn't matter if the rest are still using
global namespace, we can address that later on).

...

> +/**
> + * iio_multiply_value() - Multiply an iio value

IIO ?

> + * @result:    Destination pointer for the multiplication result
> + * @multiplier:        Multiplier.
> + * @type:      One of the IIO_VAL_* constants. This decides how the val

@val

> + *             and val2 parameters are interpreted.

@val2

> + * @val:       Value being multiplied.
> + * @val2:      Value being multiplied. val2 use depends on type.
> + *
> + * Multiply an iio value with a s64 multiplier storing the result as

IIO ?

> + * IIO_VAL_INT. This is typically used for scaling.
> + *
> + * Returns:
> + * IIO_VAL_INT on success or a negative error-number on failure.
> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/6] iio: test: Add kunit tests for iio_multiply_value()

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Add kunit tests for iio_multiply_value().

KUnit

...

> +#include <kunit/test.h>

I believe we still need header inclusions to what we are using outside
of the KUnit and IIO consumer scopes, such as types.h, math64.h.

> +#include <linux/iio/consumer.h>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 6/6] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
> Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
> The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.
>
> The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
> denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".
>
> Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
> binding to the "chtdc_ti_adc" MFD cell of the MFD driver.
>
> The "cht" in the cell name comes from Cherry Trail, but it turns out that
> just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
> Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
> not include the cht part in its name.
>
> This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
> from the Acer A1-840 Android kernel source-code archive named:
> "App. Guide_Acer_20151221_A_A.zip"
> which is distributed by Acer from the Acer A1-840 support page:
> https://www.acer.com/us-en/support/product-support/A1-840/downloads

...

> +config INTEL_DC_TI_ADC
> +       tristate "Intel Bay / Cherry Trail Dollar Cove TI ADC driver"

I would spell it in full
"Intel Bay Trail / Cherry Trail ..."

> +       depends on INTEL_SOC_PMIC_CHTDC_TI

...

> +       /*
> +        * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
> +        * not be sent together. Hence send the commands separately

Missing period.

> +        */

...

> +       /*
> +        * As per PMIC Vendor, a minimum of 50 micro seconds delay is required

We can also use SI unit acronyms, like µs.

> +        * between ADC Enable and ADC START commands. This is also recommended
> +        * by Intel Hardware team after the timing analysis of GPADC signals.
> +        * Since the I2C Write transaction to set the channel number also
> +        * imparts 25 micro seconds of delay, so we need to wait for another
> +        * 25 micro seconds before issuing ADC START command.
> +        */

...

> +       /* TI (PMIC Vendor) recommends 5 sec timeout for conversion */

seconds (to be consistent with other comments) or s (to be consistent
with SI). Interestingly, you have all three variants among the
comments already.

...

> +       /* The ADC values are 10 bits */

I would write it as "...are 10-bit wide"

...

> +       /*
> +        * If channel BPTHERM has been selected, first enable the BPTHERM BIAS
> +        * which provides the VREF Voltage reference to convert BPTHERM Input
> +        * voltage to temperature.

> +        * As per PMIC Vendor specifications, BPTHERM BIAS should be enabled
> +        * 35 ms before ADC_EN command.

I would move this comment to be directly before msleep() call.

> +        */
> +       if (chan->channel == DC_TI_ADC_BATTEMP) {
> +               ret = regmap_set_bits(info->regmap, DC_TI_ADC_CNTL_REG,
> +                                     DC_TI_ADC_EN_EXT_BPTH_BIAS);
> +               if (ret)
> +                       return ret;
> +               msleep(35);
> +       }

> +static int dc_ti_adc_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
> +       struct dc_ti_adc_info *info;
> +       struct iio_dev *indio_dev;
> +       unsigned int val;
> +       int irq, ret;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0)
> +               return irq;
> +
> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       info = iio_priv(indio_dev);
> +
> +       ret = devm_mutex_init(dev, &info->lock);
> +       if (ret)
> +               return ret;
> +
> +       init_waitqueue_head(&info->wait);
> +
> +       info->dev = dev;
> +       info->regmap = pmic->regmap;
> +
> +       indio_dev->name = "dc_ti_adc";
> +       indio_dev->channels = dc_ti_adc_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(dc_ti_adc_channels);
> +       indio_dev->info = &dc_ti_adc_iio_info;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +       ret = regmap_read(info->regmap, DC_TI_VBAT_ZSE_GE_REG, &val);
> +       if (ret)
> +               return ret;
> +
> +       info->vbat_zse = sign_extend32(FIELD_GET(DC_TI_VBAT_ZSE, val), 3);
> +       info->vbat_ge = sign_extend32(FIELD_GET(DC_TI_VBAT_GE, val), 3);
> +
> +       dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
> +
> +       ret = devm_iio_map_array_register(dev, indio_dev, dc_ti_adc_default_maps);
> +       if (ret)
> +               return ret;
> +
> +       ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_adc_isr,
> +                                       IRQF_ONESHOT, indio_dev->name, info);
> +       if (ret)
> +               return ret;
> +
> +       return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static const struct platform_device_id dc_ti_adc_ids[] = {
> +       { .name = "chtdc_ti_adc", },

Inner comma is not required.

> +       { }
> +};

...

> +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");

Wondering if the address is still alive. In this case perhaps even ask
for SoB/Co-developed-by? Otherwise if you still wish to have this
credit make it in the form of "First Last (Intel)".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/6] iio: processed channel handling fixes + Intel Dollar Cove TI PMIC ADC driver

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Here is v4 of my patch to add an IIO driver for the Intel Dollar Cove TI
> PMIC ADC. This has turned more into a series with fixes / changes
> to iio_convert_raw_to_processed() and iio_read_channel_processed_scale(),
> with the new driver tagged on as the last patch :)

Several nit-picks here and there, but in general I have no objections.
Reviewed-by: Andy Shevchenko <andy@kernel.org>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/6] iio: consumers: Fix handling of negative channel scale in iio_convert_raw_to_processed()

[Jonathan Cameron]

On Mon, 11 Aug 2025 21:19:36 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
> >
> > There is an issue with the handling of negative channel scales
> > in iio_convert_raw_to_processed_unlocked() when the channel-scale
> > is of the IIO_VAL_INT_PLUS_[MICRO|NANO] type:
> >
> > Things work for channel-scale values > -1.0 and < 0.0 because of
> > the use of signed values in:
> >
> >         *processed += div_s64(raw64 * (s64)scale_val2 * scale, 1000000LL);
> >
> > Things will break however for scale values < -1.0. Lets for example say
> > that raw = 2, (caller-provided)scale = 10 and (channel)scale_val = -1.5.
> >
> > The result should then be 2 * 10 * -1.5 = -30.
> >
> > channel-scale = -1.5 means scale_val = -1 and scale_val2 = 500000,
> > now lets see what gets stored in processed:
> >
> > 1. *processed = raw64 * scale_val * scale;
> > 2. *processed += raw64 * scale_val2 * scale / 1000000LL;
> >
> > 1. Sets processed to 2 * -1 * 10 = -20
> > 2. Adds 2 * 500000 * 10 / 1000000 = 10 to processed
> >
> > And the end result is processed = -20 + 10 = -10, which is not correct.
> >
> > Fix this by always using the abs value of both scale_val and scale_val2
> > and if either is negative multiply the end-result by -1.
> >
> > Note there seems to be an unwritten rule about negative
> > IIO_VAL_INT_PLUS_[MICRO|NANO] values that:
> >
> > i.   values > -1.0 and < 0.0 are written as val=0 val2=-xxx
> > ii.  values <= -1.0 are written as val=-xxx val2=xxx
> >
> > But iio_format_value() will also correctly display a third option:
> >
> > iii. values <= -1.0 written as val=-xxx val2=-xxx
> >
> > Since iio_format_value() uses abs(val) when val2 < 0.
> >
> > This fix also makes iio_convert_raw_to_processed() properly handle
> > channel-scales using this third option.  
> 
> ...
> 
> > +               switch (scale_type) {
> > +               case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
> > +               case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
> > +               }  
> 
> Now wondering if checkpatch et al. are happy with this style. Not a
> big deal personally to me, but if we have warnings from the tools
> perhaps it's better to avoid them.
Agreed, whilst it's a minor thing let's burn some lines to keep this more
standard looking.

		case IIO_VAL_INT_PLUS_MICRO:
			denominator = MICRO;
			break;

		case IIO_VAL_INT_PLUS_NANO:
			denominator = NANO;
			break;
		}

> 
> 

Re: [PATCH v4 3/6] iio: consumers: Add an iio_multiply_value() helper function

[Jonathan Cameron]

> > +int iio_multiply_value(int *result, s64 multiplier,
> > +                      unsigned int type, int val, int val2)
> > +{
> > +       s64 denominator;
> > +
> > +       switch (type) {
> > +       case IIO_VAL_INT:
> > +               *result = multiplier * val;
> > +               return IIO_VAL_INT;
> > +       case IIO_VAL_INT_PLUS_MICRO:
> > +       case IIO_VAL_INT_PLUS_NANO:
> > +               switch (type) {
> > +               case IIO_VAL_INT_PLUS_MICRO: denominator = MICRO; break;
> > +               case IIO_VAL_INT_PLUS_NANO: denominator = NANO; break;
> > +               }
> > +               *result = multiplier * abs(val);
> > +               *result += div_s64(multiplier * abs(val2), denominator);
> > +               if (val < 0 || val2 < 0)
> > +                       *result *= -1;
> > +               return IIO_VAL_INT;
> > +       case IIO_VAL_FRACTIONAL:
> > +               *result = div_s64(multiplier * val, val2);
> > +               return IIO_VAL_INT;
> > +       case IIO_VAL_FRACTIONAL_LOG2:
> > +               *result = (multiplier * val) >> val2;
> > +               return IIO_VAL_INT;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(iio_multiply_value);  
> 
> Is it in the namespace? I think we already discussed with Jonathan
> that IIO_CORE or something can be started if not yet for this kind of
> exported symbols (and it doesn't matter if the rest are still using
> global namespace, we can address that later on).

I think we are only exporting this for the unit test?  If so IIO_UNIT_TEST
namespace might be better?  We can move it to a more general one if it
starts getting use other that for testing (or directly in this file
where the export isn't needed).


> 
> ...
> 
> > +/**
> > + * iio_multiply_value() - Multiply an iio value  
> 

Re: [PATCH v4 4/6] iio: Improve iio_read_channel_processed_scale() precision

[David Lechner]

On 8/11/25 10:54 AM, Hans de Goede wrote:
> Before this change iio_read_channel_processed_scale() always assumes that
> channels which advertise IIO_CHAN_INFO_PROCESSED capability return
> IIO_VAL_INT on success.
> 
> Ignoring any fractional values from drivers which return
> IIO_VAL_INT_PLUS_MICRO / IIO_VAL_INT_PLUS_NANO. These fractional values
> might become non fractional after scaling so these should be taken into
> account for better precision.
> 
> Use the new iio_multiply_value() helper to do proper scaling taking
> the fractionional values into account.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v4:
> - Use the new iio_multiply_value() helper
> ---
>  drivers/iio/inkern.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index af1da729d18e..2a1ecef2b820 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -722,20 +722,19 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
>  				     unsigned int scale)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
> -	int ret;
> +	int ret, val2;
>  
>  	guard(mutex)(&iio_dev_opaque->info_exist_lock);
>  	if (!chan->indio_dev->info)
>  		return -ENODEV;
>  
>  	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
> -		ret = iio_channel_read(chan, val, NULL,
> +		ret = iio_channel_read(chan, val, &val2,
>  				       IIO_CHAN_INFO_PROCESSED);
>  		if (ret < 0)
>  			return ret;
> -		*val *= scale;
>  
> -		return ret;
> +		return iio_multiply_value(val, scale, ret, *val, val2);

Passing val to iio_multiply_value() as 2 different args is a bit
confusing.

I would add one more local variable for the processes val. e.g.

int ret, pval, pval2;


>  	} else {
>  		ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW);
>  		if (ret < 0)

Re: [PATCH v4 5/6] iio: test: Add kunit tests for iio_multiply_value()

[Jonathan Cameron]

On Mon, 11 Aug 2025 17:54:52 +0200
Hans de Goede <hansg@kernel.org> wrote:

> Add kunit tests for iio_multiply_value().
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
Very nice!

I don't have any comments to add to those of other reviewers for
any of the series.

Re: [PATCH v4 0/6] iio: processed channel handling fixes + Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Hi All,

On 11-Aug-25 5:54 PM, Hans de Goede wrote:
> Hi All,
> 
> Here is v4 of my patch to add an IIO driver for the Intel Dollar Cove TI
> PMIC ADC. This has turned more into a series with fixes / changes
> to iio_convert_raw_to_processed() and iio_read_channel_processed_scale(),
> with the new driver tagged on as the last patch :)

Thank you for all the review comments.

I was away for vacation and I'm catching up on email now.

I agree with all the review comments and I'll prepare a v5
addressing the review comments when I can make some time for this.

Regards,

Hans




> Changes in v4:
> - 2 new bug-fixes for iio_convert_raw_to_processed()
> - Factor the bugfixed code multiply a s64 and an iio (type, val, val2)
>   triplet out of iio_convert_raw_to_processed() into a new
>   iio_multiply_value() helper
> - Add a kunit test for iio_multiply_value()
> - Redo the "Improve iio_read_channel_processed_scale() precision"
>   patch using the iio_multiply_value() helper
> 
> Changes in v3:
> - "iio: Improve iio_read_channel_processed_scale() precision"
>   - Use div_s64() instead of div_u64() to fix -1.0 - 0.0 range
>   - Directly return IIO_VAL_INT from valid cases and drop the final
>     return ret after the switch-case
> - "iio: adc: Add Intel Dollar Cove TI PMIC ADC driver"
>   - Use new more compact DC_TI_ADC_DATA_REG_CH(x) macro
>   - Use regmap_set_bits() regmap_clear_bits() where applicable
>   - Use regmap_bulk_read() + be16_to_cpu() to read ADC value
>   - Use sign_extend32() for vbat_zse and vbat_ge reading in probe()
> 
> Changes in v2:
> - Add new "iio: Improve iio_read_channel_processed_scale() precision"
>   patch to the series
> - Add IIO_CHAN_INFO_SCALE and provide ADC scale info for Vbat
> - Add IIO_CHAN_INFO_PROCESSED which applies calibration and
>   scaling for the VBat channel
> - Address some other small review remarks
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (6):
>   iio: consumers: Fix handling of negative channel scale in
>     iio_convert_raw_to_processed()
>   iio: consumers: Fix offset handling in iio_convert_raw_to_processed()
>   iio: consumers: Add an iio_multiply_value() helper function
>   iio: Improve iio_read_channel_processed_scale() precision
>   iio: test: Add kunit tests for iio_multiply_value()
>   iio: adc: Add Intel Dollar Cove TI PMIC ADC driver
> 
>  drivers/iio/adc/Kconfig              |  11 +
>  drivers/iio/adc/Makefile             |   1 +
>  drivers/iio/adc/intel_dc_ti_adc.c    | 327 +++++++++++++++++++++++++++
>  drivers/iio/inkern.c                 |  77 ++++---
>  drivers/iio/test/Kconfig             |  12 +
>  drivers/iio/test/Makefile            |   1 +
>  drivers/iio/test/iio-test-multiply.c | 209 +++++++++++++++++
>  include/linux/iio/consumer.h         |  18 ++
>  8 files changed, 621 insertions(+), 35 deletions(-)
>  create mode 100644 drivers/iio/adc/intel_dc_ti_adc.c
>  create mode 100644 drivers/iio/test/iio-test-multiply.c
> 

Re: [PATCH v4 6/6] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede]

Hi Andy,

On 11-Aug-25 9:52 PM, Andy Shevchenko wrote:
> On Mon, Aug 11, 2025 at 5:55 PM Hans de Goede <hansg@kernel.org> wrote:
>>
>> Intel has 2 completely different "Dollar Cove" PMICs for its Bay Trail /
>> Cherry Trail SoCs. One is made by X-Powers and is called the AXP288.
>> The AXP288's GPADC is already supported by the X-Powers AXP288 ADC driver.
>>
>> The other "Dollar Cove" PMIC is made by TI and does not have any clear TI
>> denomination, its MFD driver calls it the "Intel Dollar Cove TI PMIC".
>>
>> Add a driver for the Intel Dollar Cove TI PMIC's general purpose ADC,
>> binding to the "chtdc_ti_adc" MFD cell of the MFD driver.
>>
>> The "cht" in the cell name comes from Cherry Trail, but it turns out that
>> just like the AXP288 the Intel Dollar Cove TI PMIC is also used with both
>> Intel Bay Trail and Intel Cherry Trail SoCs, so this new ADC driver does
>> not include the cht part in its name.
>>
>> This is loosely based on kernel/drivers/iio/adc/iio_dc_ti_gpadc.c
>> from the Acer A1-840 Android kernel source-code archive named:
>> "App. Guide_Acer_20151221_A_A.zip"
>> which is distributed by Acer from the Acer A1-840 support page:
>> https://www.acer.com/us-en/support/product-support/A1-840/downloads

Thank you for the review. I've addressed all your remarks for v5.

...

>> +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
> 
> Wondering if the address is still alive. In this case perhaps even ask
> for SoB/Co-developed-by? Otherwise if you still wish to have this
> credit make it in the form of "First Last (Intel)".

Good point, I asked about co-maintaining and got a bounce so I'll replace
this with "First Last (Intel)".

Regards,

Hans