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

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Hi Peter,

Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@xxxxxxxxxx> 
a écrit :
> 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.

Yes, when I started working on that patchset, processed channels 
weren't a thing, and I don't think I understood what they are about.

> >  +			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...).

I could set the IIO_CHAN_INFO_SCALE only for non-processed channels, 
since this is what I can test with.

Cheers,
-Paul

> >   	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,