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

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

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.

Regards,

Hans