Re: [PATCH v3 1/2] iio: Improve iio_read_channel_processed_scale() precision

[Hans de Goede <hansg@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4108 bytes --]

Hi,

On 10-Aug-25 9:25 PM, Hans de Goede wrote:
> Hi David,
> 
> On 29-Jul-25 7:26 PM, David Lechner wrote:
>> On 7/27/25 4:06 PM, 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.
>>>
>>> While at it also error out for IIO_VAL_* values which
>>> iio_read_channel_processed_scale() does not know how to handle.
>>>
>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>> ---
>>> Changes in v3:
>>> - 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
>>>
>>> Changes in v2:
>>> - New patch in v2 of this patch-series
>>> ---
>>>  drivers/iio/inkern.c | 24 ++++++++++++++++++++----
>>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index c174ebb7d5e6..46900be16ff8 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -714,20 +714,36 @@ 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;
>>> +		switch (ret) {
>>> +		case IIO_VAL_INT:
>>> +			*val *= scale;
>>> +			return IIO_VAL_INT;
>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>> +			*val *= scale;
>>> +			*val += div_s64((s64)val2 * scale, 1000000LL);
>>> +			return IIO_VAL_INT;
>>> +		case IIO_VAL_INT_PLUS_NANO:
>>> +			*val *= scale;
>>> +			*val += div_s64((s64)val2 * scale, 1000000000LL);
>>> +			return IIO_VAL_INT;
>>
>> I would feel better if we had some kunit tests on this function since
>> the negative values can be tricky. I.e. something similar to
>> iio_test_iio_format_value_fixedpoint() that tests the 4 possible
>> interesting cases for val and val2.
>>
>> I think that would find a bug here. For example, if the processed
>> value is -1.5 with IIO_VAL_INT_PLUS_MICRO, then *val would be
>> -1 and *val2 would be 500_000 (before applying scale). And suppose
>> scale is 2. The expected result would be -1.5 * 2 = -3. But the math
>> here is:
>>
>> -1 * 2 + 500_000 * 2 / 1_000_000 = -1 != -3
> 
> Ack, after looking at iio_format_value and the kunit test for this
> I believe I know how this is supposed to work now.
> 
> Note that it seems that iio_convert_raw_to_processed_unlocked()
> also seems to get this wrong when the channel scale attribute
> is smaller then -1, e.g. your -1.5. Actually it seems that
> the code in iio_convert_raw_to_processed_unlocked() is making
> the exact same mistake you are highlighting in my code :)
> 
> I'll prepare a patch series to try and deal with this.

Ok, attached is a teaser of the patch-series I'm working on.

The first 2 patches fix 2 bugs in iio_convert_raw_to_processed()
which I noticed while working on this.

The third patch factors multiplication of a type, val,
val2 integer triplet by some integer number and storing
the result in an integer out into a new iio_mutiply_value()
helper. For re-use and so that a kunit-test can be written.

Patch 4 is a new version of the patch improving
iio_read_channel_processed_scale() precision using this helper.

What is missing for posting a new version is a kunit test
for the new helper. I hope to be able to work on that
tomorrow.

Regards,

Hans



[-- Attachment #2: 0001-iio-consumers-Fix-handling-of-negative-channel-scale.patch --]
[-- Type: text/x-patch, Size: 3693 bytes --]

From 841797bc90e32f366a1ee4eaa5f3a26747546f7c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Sun, 10 Aug 2025 21:49:35 +0200
Subject: [PATCH 1/4] iio: consumers: Fix handling of negative channel scale in
 iio_convert_raw_to_processed()

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

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index c174ebb7d5e6..6c16224619f4 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -604,7 +604,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 +648,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 = 1000000LL; break;
+		case IIO_VAL_INT_PLUS_NANO: denominator = 1000000000LL; 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


[-- Attachment #3: 0002-iio-consumers-Fix-offset-handling-in-iio_convert_raw.patch --]
[-- Type: text/x-patch, Size: 1227 bytes --]

From 62e1c0dd18648ecf262bb2b5b7460c339e7521c6 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Sun, 10 Aug 2025 22:48:01 +0200
Subject: [PATCH 2/4] iio: consumers: Fix offset handling in
 iio_convert_raw_to_processed()

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 6c16224619f4..9f43ae4b5bde 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -639,7 +639,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


[-- Attachment #4: 0003-iio-consumers-Add-an-iio_multiply_value-helper-funct.patch --]
[-- Type: text/x-patch, Size: 5140 bytes --]

From 3ecd9534379532b69fdb905807a56f231aa19135 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Sun, 10 Aug 2025 22:40:45 +0200
Subject: [PATCH 3/4] iio: consumers: Add an iio_multiply_value() helper
 function

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 9f43ae4b5bde..e3f1d0944e28 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -598,13 +598,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 = 1000000LL; break;
+		case IIO_VAL_INT_PLUS_NANO: denominator = 1000000000LL; 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);
@@ -643,31 +676,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 = 1000000LL; break;
-		case IIO_VAL_INT_PLUS_NANO: denominator = 1000000000LL; 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


[-- Attachment #5: 0004-iio-Improve-iio_read_channel_processed_scale-precisi.patch --]
[-- Type: text/x-patch, Size: 1861 bytes --]

From f8372957c6b51ae30e95015e151d42aec709c179 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Sun, 10 Aug 2025 23:03:41 +0200
Subject: [PATCH 4/4] iio: Improve iio_read_channel_processed_scale() precision

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 instead of DYI code
---
 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 e3f1d0944e28..ad65a03e1684 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -721,20 +721,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