Re: [PATCH v1 3/9] iio: afe: rescale: use core to get processed value

[Jonathan Cameron]

On Sat, 29 May 2021 20:59:11 -0400
Liam Beguin <liambeguin@xxxxxxxxx> wrote:

> From: Liam Beguin <lvb@xxxxxxxxxx>
> 
> Make use of the IIO core to compute the source channel's processed
> value. This reduces duplication and will facilitate the addition of
> offsets in the iio-rescale driver.

Fairly sure you just dropped a lot or precision if the devices do provide
a scale.
> 
> Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx>
> ---
>  drivers/iio/afe/iio-rescale.c | 37 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index e42ea2b1707d..4d0813b274d1 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -38,36 +38,20 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct rescale *rescale = iio_priv(indio_dev);
> -	unsigned long long tmp;
>  	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		return iio_read_channel_raw(rescale->source, val);
> +		ret = iio_read_channel_processed(rescale->source, val);

iio_read_channel_processed() provides a IIO_VAL_INT as you can see.
Now that can be heavily truncated.  In some cases it is always 0
(e.g. device reading very small currents only).

To maintain that scaling we deliberately combine it with the
scaling from the afe, hopefully maintaining that precision because
the scale value has much higher degree of precision.

We could probably do this better than currently with a more complex
conversion function.

It might seem like a good idea to fix up iio_read_channel_processed
to return IIO_VAL_INT_PLUS_MICRO or similar, but potentially that
would still throw away precision, for example if the scale is
expressed as IIO_VAL_FRACTIONAL to a high degree of precision.

Jonathan

>  
> +		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		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;
> -		}
> +		*val = rescale->numerator;
> +		if (rescale->denominator == 1)
> +			return IIO_VAL_INT;
> +		*val2 = rescale->denominator;
> +
> +		return IIO_VAL_FRACTIONAL;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -130,9 +114,8 @@ static int rescale_configure_channel(struct device *dev,
>  	chan->ext_info = rescale->ext_info;
>  	chan->type = rescale->cfg->type;
>  
> -	if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
> -	    !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) {
> -		dev_err(dev, "source channel does not support raw/scale\n");
> +	if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW)) {
> +		dev_err(dev, "source channel does not support raw\n");
>  		return -EINVAL;
>  	}
>