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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 22 Jul 2022 09:52:34 +0100
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi Peter,
> 
> Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@axentia.se> 
> 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@crapouillou.net>
> >>  ---
> >>   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.

Indeed both nonsensical for PROCESSED channels to have a scale and in the
rare corner case where it happens you shouldn't apply it anyway.

I'm struggling to think of when it might happen due to maintaining
backwards compatibility and similar (the reason we have channels with
both PROCESSED and RAW) but in those cases I don't think we'd have
SCALE because they tend to be devices with nasty non linear transfer
functions where SCALE isn't appropriate.

Jonathan


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