On Mon, 16 Nov 2020 09:18:24 +0100
Peter Rosin <peda@xxxxxxxxxx> wrote:
> Hi!
>
> On 2020-11-15 18:44, Jonathan Cameron wrote:
> > On Mon, 2 Nov 2020 00:22:11 +0100
> > Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >
> > 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) |
> >
>
