Re: [RFC] iio: afe: rescale: Add support for converting scale avail table

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Hi Jonathan,

Le mer., juil. 13 2022 at 17:34:01 +0100, Jonathan Cameron 
<jic23@xxxxxxxxxx> a écrit :
> On Mon, 11 Jul 2022 20:37:14 +0100
> Paul Cercueil <paul@xxxxxxxxxxxxxxx> 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.
> >
> >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >  ---
> >   drivers/iio/afe/iio-rescale.c   | 75 
> > +++++++++++++++++++++++++++++++++
> >   include/linux/iio/afe/rescale.h |  2 +
> >   2 files changed, 77 insertions(+)
> >
> >
> >   Hi Jonathan,
> >
> >   I'm trying to add support for converting the scale_available 
> > attribute
> >   in the iio-rescale driver.
> >
> >   The code below works fine… as long as all the possible scales 
> > returned
> >   by the underlying IIO device driver are translated to the exact 
> > same
> >   type. The problem then is that rescale_process_scale() can return 
> > many
> >   different types, while rescale_read_avail() only supports 
> > returning one
> >   type.
> >
> >   I don't really know what would be the way forward. Should the
> >   .read_avail callback support returning multiple types? Should
> >   rescale_process_scale() have an option to force all the types to be
> >   converted to a specific one?
>
> Ah.  I don't have a particularly strong view either way.  Didn't have
> the complexities of the rescale driver in mind when we read_avail() 
> was
> added (pesky hindsight ;)
>
> If you do go with read_avail() being able to return types then a 
> reasonably
> clean way to do it might be to add a new
> IIO_AVAIL_LIST_WITH_TYPE
> and have *vals be a 3xN array with the type as the final integer.
> That should only impact the core code in a few places and means no 
> changes
> to existing drivers.

That's a smart idea. I will try that.

Thanks!
-Paul


> >  diff --git a/drivers/iio/afe/iio-rescale.c 
> > b/drivers/iio/afe/iio-rescale.c
> >  index 6949d2151025..8b00ff3de733 100644
> >  --- a/drivers/iio/afe/iio-rescale.c
> >  +++ b/drivers/iio/afe/iio-rescale.c
> >  @@ -232,6 +232,19 @@ 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) {
> >  +			return iio_read_avail_channel_attribute(rescale->source,
> >  +								vals, type,
> >  +								length,
> >  +								IIO_CHAN_INFO_SCALE);
> >  +		} else if (rescale->scale_len) {
> >  +			*type = rescale->scale_type;
> >  +			*length = rescale->scale_len;
> >  +			*vals = rescale->scale_data;
> >  +			return IIO_AVAIL_LIST;
> >  +		}
> >  +		fallthrough;
> >   	default:
> >   		return -EINVAL;
> >   	}
> >  @@ -266,11 +279,63 @@ 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;
> >  +
> >  +	ret = iio_read_avail_channel_attribute(rescale->source, 
> > &scale_raw,
> >  +					       &type, &length,
> >  +					       IIO_CHAN_INFO_SCALE);
> >  +	if (ret < 0)
> >  +		return ret;
> >  +
> >  +	/* TODO: Support IIO_AVAIL_RANGE */
> >  +	if (ret != IIO_AVAIL_LIST)
> >  +		return -ENOTSUPP;
> >  +
> >  +	length <<= type == IIO_VAL_INT;
> >  +
> >  +	data = devm_kzalloc(dev, sizeof(*data) * length, GFP_KERNEL);
> >  +	if (!data)
> >  +		return -ENOMEM;
> >  +
> >  +	if (type == IIO_VAL_INT) {
> >  +		/* Convert from integer to fractional form to ease processing */
> >  +		for (i = 0; i < length / 2; i++) {
> >  +			data[i * 2] = scale_raw[i];
> >  +			data[i * 2 + 1] = 1;
> >  +		}
> >  +
> >  +		type = IIO_VAL_FRACTIONAL;
> >  +	} else {
> >  +		/* Copy raw scale info into our own buffer */
> >  +		memcpy(data, scale_raw, sizeof(*scale_raw) * length);
> >  +	}
> >  +
> >  +	for (i = 0; i < length; i += 2) {
> >  +		ret = rescale_process_scale(rescale, type,
> >  +					    &data[i], &data[i + 1]);
> >  +		if (ret < 0)
> >  +			return ret;
> >  +
> >  +		type = ret;
> >  +	}
> >  +
> >  +	rescale->scale_type = type;
> >  +	rescale->scale_len = length;
> >  +	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 +368,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;
> >  +		}
> >  +	}
> >  +
> >   	return 0;
> >   }
> >
> >  diff --git a/include/linux/iio/afe/rescale.h 
> > b/include/linux/iio/afe/rescale.h
> >  index 6eecb435488f..8618955695df 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_type, scale_len;
> >  +	int *scale_data;
> >   };
> >
> >   int rescale_process_scale(struct rescale *rescale, int scale_type,