Re: [PATCH] iio: afe: iio-rescale: Support processed channels

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Nov 2020 09:18:24 +0100
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> On 2020-11-15 18:44, Jonathan Cameron wrote:
> > On Mon,  2 Nov 2020 00:22:11 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >   
> >> It happens that an ADC will only provide raw or processed
> >> voltage conversion channels. (adc/ab8500-gpadc.c).
> >> On the Samsung GT-I9070 this is used for a light sensor
> >> and current sense amplifier so we need to think of something.
> >>
> >> The idea is to allow processed channels and scale them
> >> with 1/1 and then the rescaler can modify the result
> >> on top.
> >>
> >> Cc: Peter Rosin <peda@axentia.se>
> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>  
> > 
> > Sorry, I kept leaving this to last as it was in the 'needed thought'
> > pile - then running out of time and not getting to it.
> > 
> > Anyhow, I think this is the best we can do for the situation
> > you describe so I'm happy with this.
> > 
> > @Peter, I definitely want your input on this one as well though
> > before I apply it!  
> 
> Yes, sorry about the delay. Same pile here, amplified with way too much
> to do at work. My immediate reaction was that this is not that simple,
> but after looking at it for a few minutes I also came to think that it's
> perhaps the best that can be done.
> 
> But it's been a while, so it just took a while for things to dawn on me.
> 
> The rescaler passes on IIO_CHAN_INFO_RAW as-is to the underlying
> driver in the .read_avail op, and this patch xforms the processed
> channel into the raw channel. So that is a mismatch. I don't think
> it's easily fixable in the general case because the processed channel
> rarely, if ever, implements .read_avail?

There is nothing stopping them doing so if we have a particular usecase
that requires it.  To be honest, very few drivers implement read_avail
at all yet!

> And I don't know if it
> is allowed to return -EINVAL for the .read_avail op for the raw
> channel, because that would be the obvious patch to squash-in...

I'm not sure it matters. As things stand the rescale_configure_channel
queries if read_avail is available for the _RAW element only.
So currently we'd just not register that at all if only processed
is available.

It might be a nice to have, but there are plenty of other cases
where read_avail isn't provided and this driver might be used
so I'm not that fussed.

Thanks,

Jonathan

> 
> Cheers,
> Peter
> 
> 
> > Jonathan
> > 
> >   
> >> ---
> >>  drivers/iio/afe/iio-rescale.c | 31 +++++++++++++++++++++++++++----
> >>  1 file changed, 27 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> >> index e42ea2b1707d..ea90034cb257 100644
> >> --- a/drivers/iio/afe/iio-rescale.c
> >> +++ b/drivers/iio/afe/iio-rescale.c
> >> @@ -29,6 +29,7 @@ struct rescale {
> >>  	struct iio_channel *source;
> >>  	struct iio_chan_spec chan;
> >>  	struct iio_chan_spec_ext_info *ext_info;
> >> +	bool chan_processed;
> >>  	s32 numerator;
> >>  	s32 denominator;
> >>  };
> >> @@ -43,10 +44,27 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> >>  
> >>  	switch (mask) {
> >>  	case IIO_CHAN_INFO_RAW:
> >> -		return iio_read_channel_raw(rescale->source, val);
> >> +		if (rescale->chan_processed)
> >> +			/*
> >> +			 * When only processed channels are supported, we
> >> +			 * read the processed data and scale it by 1/1
> >> +			 * augmented with whatever the rescaler has calculated.
> >> +			 */
> >> +			return iio_read_channel_processed(rescale->source, val);
> >> +		else
> >> +			return iio_read_channel_raw(rescale->source, val);
> >>  
> >>  	case IIO_CHAN_INFO_SCALE:
> >> -		ret = iio_read_channel_scale(rescale->source, val, val2);
> >> +		if (rescale->chan_processed) {
> >> +			/*
> >> +			 * Processed channels are scaled 1-to-1
> >> +			 */
> >> +			ret = IIO_VAL_FRACTIONAL;
> >> +			*val = 1;
> >> +			*val2 = 1;
> >> +		} else {
> >> +			ret = iio_read_channel_scale(rescale->source, val, val2);
> >> +		}
> >>  		switch (ret) {
> >>  		case IIO_VAL_FRACTIONAL:
> >>  			*val *= rescale->numerator;
> >> @@ -132,8 +150,13 @@ static int rescale_configure_channel(struct device *dev,
> >>  
> >>  	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");
> >> -		return -EINVAL;
> >> +		if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) {
> >> +			dev_info(dev, "using processed channel\n");
> >> +			rescale->chan_processed = true;
> >> +		} else {
> >> +			dev_err(dev, "source channel does not support raw+scale or processed data\n");
> >> +			return -EINVAL;
> >> +		}
> >>  	}
> >>  
> >>  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  
> >   
>