Re: [PATCH 3/4] iio: afe/rescale: Add support for converting scale avail table

[Peter Rosin]

Hi!

2022-07-21 at 21:15, Paul Cercueil wrote:
> When the IIO channel has a scale_available attribute, we want the values
> contained to be properly converted the same way the scale value is.
> 
> Since rescale_process_scale() may change the encoding type, we must
> convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE.
> 
> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> ---
>  drivers/iio/afe/iio-rescale.c   | 85 +++++++++++++++++++++++++++++++++
>  include/linux/iio/afe/rescale.h |  2 +
>  2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 6949d2151025..5c9970b93384 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -232,6 +232,18 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
>  		*type = IIO_VAL_INT;
>  		return iio_read_avail_channel_raw(rescale->source,
>  						  vals, length);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (rescale->chan_processed) {

I think it is wrong to simply feed the info-scale to the source channel if it
happens to be processed. It still needs the inverse rescale. But see below.

> +			return iio_read_avail_channel_attribute(rescale->source,
> +								vals, type,
> +								length,
> +								IIO_CHAN_INFO_SCALE);
> +		} else if (rescale->scale_len) {
> +			*length = rescale->scale_len;
> +			*vals = rescale->scale_data;
> +			return IIO_AVAIL_LIST_WITH_TYPE;
> +		}
> +		fallthrough;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -266,11 +278,74 @@ static ssize_t rescale_write_ext_info(struct iio_dev *indio_dev,
>  					  buf, len);
>  }
>  
> +static int rescale_init_scale_avail(struct device *dev, struct rescale *rescale)
> +{
> +	int ret, type, length, *data;
> +	const int *scale_raw;
> +	unsigned int i;
> +	size_t out_len;
> +
> +	ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw,
> +					       &type, &length,
> +					       IIO_CHAN_INFO_SCALE);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case IIO_AVAIL_LIST_WITH_TYPE:
> +		out_len = length;
> +		break;
> +	case IIO_AVAIL_LIST:
> +		if (type == IIO_VAL_INT)
> +			out_len = length * 3 / 1;
> +		else
> +			out_len = length * 3 / 2;
> +		break;
> +	default:
> +		/* TODO: Support IIO_AVAIL_RANGE */
> +		return -EOPNOTSUPP;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (ret == IIO_AVAIL_LIST_WITH_TYPE) {
> +		memcpy(data, scale_raw, sizeof(*scale_raw) * length);
> +	} else if (type == IIO_VAL_INT) {
> +		for (i = 0; i < length; i++) {
> +			data[i * 3 + 0] = scale_raw[i];
> +			data[i * 3 + 2] = IIO_VAL_INT;
> +		}
> +	} else {
> +		for (i = 0; i < length / 2; i++) {
> +			data[i * 3 + 0] = scale_raw[i * 2];
> +			data[i * 3 + 1] = scale_raw[i * 2 + 1];
> +			data[i * 3 + 2] = type;
> +		}
> +	}
> +
> +	for (i = 0; i < out_len; i += 3) {
> +		ret = rescale_process_scale(rescale, data[i + 2],
> +					    &data[i], &data[i + 1]);
> +		if (ret < 0)
> +			return ret;
> +
> +		data[i + 2] = ret;
> +	}
> +
> +	rescale->scale_len = out_len;
> +	rescale->scale_data = data;
> +
> +	return 0;
> +}
> +
>  static int rescale_configure_channel(struct device *dev,
>  				     struct rescale *rescale)
>  {
>  	struct iio_chan_spec *chan = &rescale->chan;
>  	struct iio_chan_spec const *schan = rescale->source->channel;
> +	int ret;
>  
>  	chan->indexed = 1;
>  	chan->output = schan->output;
> @@ -303,6 +378,16 @@ static int rescale_configure_channel(struct device *dev,
>  	    !rescale->chan_processed)
>  		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
>  
> +	if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) {
> +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE);
> +
> +		if (!rescale->chan_processed) {
> +			ret = rescale_init_scale_avail(dev, rescale);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +

Does a (sane) processed channel really have a scale? That seems a bit fringe.
Wouldn't it be better to conditionally publish availability of info-scale so
that it isn't visible for processed channels and dodge that rabbit-hole? In
either case, the above commented implementation of info-scale for rescaled
processed channels is wrong (I think...).

Cheers,
Peter

>  	return 0;
>  }
>  
> diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
> index 6eecb435488f..74de2962f864 100644
> --- a/include/linux/iio/afe/rescale.h
> +++ b/include/linux/iio/afe/rescale.h
> @@ -26,6 +26,8 @@ struct rescale {
>  	s32 numerator;
>  	s32 denominator;
>  	s32 offset;
> +	int scale_len;
> +	int *scale_data;
>  };
>  
>  int rescale_process_scale(struct rescale *rescale, int scale_type,