| Message ID | 20201101232211.1194304-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: afe: iio-rescale: Support processed channels | expand |
On Mon, Nov 2, 2020 at 12:22 AM 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> Any comments on this? The reason the AB8500 does not provide any scaling is that it is not linear so what can I do. Processed is what we can provide... Yours, Linus Walleij
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! 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) |
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? 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... 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) | >
On Mon, Nov 2, 2020 at 12:22 AM 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> Did we reach any conclusion on this? I really need to use the rescaler on an ADC that only handles processed channels... I'm sorry that I can't make this ADC disappear :D Yours, Linus Walleij
On 2020-12-12 13:26, Linus Walleij wrote: > On Mon, Nov 2, 2020 at 12:22 AM 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> > > Did we reach any conclusion on this? I really need to use > the rescaler on an ADC that only handles processed channels... > > I'm sorry that I can't make this ADC disappear :D Hi! My conclusion was that the patch is buggy since it presents inconsistent information. That needs to be fixed one way or the other. If the offending information cannot be filtered out for some reason, I don't know what to do. Details in my previous comment [1]. BTW, I still do not know the answer to the .read_avail question at the end of that message, and I don't have time to dig into it. Sorry. Cheers, Peter [1] https://lore.kernel.org/linux-iio/320464d8-659c-01de-0e08-34e4c744ef16@axentia.se/
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) | > > >
On Sun, 13 Dec 2020 00:22:17 +0100 Peter Rosin <peda@axentia.se> wrote: > On 2020-12-12 13:26, Linus Walleij wrote: > > On Mon, Nov 2, 2020 at 12:22 AM 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> > > > > Did we reach any conclusion on this? I really need to use > > the rescaler on an ADC that only handles processed channels... > > > > I'm sorry that I can't make this ADC disappear :D > > Hi! > > My conclusion was that the patch is buggy since it presents inconsistent > information. That needs to be fixed one way or the other. If the offending > information cannot be filtered out for some reason, I don't know what to > do. Details in my previous comment [1]. BTW, I still do not know the answer > to the .read_avail question at the end of that message, and I don't have > time to dig into it. Sorry. Unless I'm missing something, I think it presents no information unless we strangely have a driver providing read_avail for _RAW but only _PROCESSED channels which is a bug. I'm not that bothered about missing information in this particular, somewhat obscure, corner case. So I think we should take the patch as it stands. It's missed the merge window now anyway unfortunately. So Peter, I would suggest we take this and perhaps revisit to tidy up loose corners when we all have more time. Thanks, Jonathan > > Cheers, > Peter > > [1] https://lore.kernel.org/linux-iio/320464d8-659c-01de-0e08-34e4c744ef16@axentia.se/
On Mon, Nov 2, 2020 at 1:23 AM 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. ... > 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; A nit: Move this to the end of the branch, so in both branches the ret = will be the last code line (for better readability). > + *val = 1; > + *val2 = 1; > + } else { > + ret = iio_read_channel_scale(rescale->source, val, val2); > + }
On 2020-12-13 13:16, Jonathan Cameron wrote: > On Sun, 13 Dec 2020 00:22:17 +0100 > Peter Rosin <peda@axentia.se> wrote: > >> On 2020-12-12 13:26, Linus Walleij wrote: >>> On Mon, Nov 2, 2020 at 12:22 AM 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> >>> >>> Did we reach any conclusion on this? I really need to use >>> the rescaler on an ADC that only handles processed channels... >>> >>> I'm sorry that I can't make this ADC disappear :D >> >> Hi! >> >> My conclusion was that the patch is buggy since it presents inconsistent >> information. That needs to be fixed one way or the other. If the offending >> information cannot be filtered out for some reason, I don't know what to >> do. Details in my previous comment [1]. BTW, I still do not know the answer >> to the .read_avail question at the end of that message, and I don't have >> time to dig into it. Sorry. > > Unless I'm missing something, I think it presents no information unless > we strangely have a driver providing read_avail for _RAW but only > _PROCESSED channels which is a bug. I'm not that bothered about > missing information in this particular, somewhat obscure, corner case. > > So I think we should take the patch as it stands. It's missed the > merge window now anyway unfortunately. So Peter, I would suggest we > take this and perhaps revisit to tidy up loose corners when we all have > more time. My concern was a driver with a raw channel, including read_avail, providing raw sample values but that no easy conversion existed to get from that to the processed values. One option for the driver in that case would be to provide these raw values, but then have no scaling info. I.e. the way I see it, it is perfectly reasonable for a driver to provide raw with read_avail, no scaling but also processed values. And that gets transformed by the rescaler into the processed values being presented as raw, with rescaling added on top, but with the read_avail info for this new raw channel being completely wrong. For the intended driver (ab8500-gpadc) this is not the case (it has no read_avail for its raw channel). But it does have a raw channel, so adding read_avail seems easy and I can easily see other drivers already doing it. Haven't checked that though... But if you say that this never happens, fine. Otherwise, since it's too late for the merge window anyway, the patch might as well be updated such that the rescaler blocks the read_avail channel in this situation, if it exists. Cheers, Peter
On Mon, 14 Dec 2020 09:34:40 +0100 Peter Rosin <peda@axentia.se> wrote: > On 2020-12-13 13:16, Jonathan Cameron wrote: > > On Sun, 13 Dec 2020 00:22:17 +0100 > > Peter Rosin <peda@axentia.se> wrote: > > > >> On 2020-12-12 13:26, Linus Walleij wrote: > >>> On Mon, Nov 2, 2020 at 12:22 AM 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> > >>> > >>> Did we reach any conclusion on this? I really need to use > >>> the rescaler on an ADC that only handles processed channels... > >>> > >>> I'm sorry that I can't make this ADC disappear :D > >> > >> Hi! > >> > >> My conclusion was that the patch is buggy since it presents inconsistent > >> information. That needs to be fixed one way or the other. If the offending > >> information cannot be filtered out for some reason, I don't know what to > >> do. Details in my previous comment [1]. BTW, I still do not know the answer > >> to the .read_avail question at the end of that message, and I don't have > >> time to dig into it. Sorry. > > > > Unless I'm missing something, I think it presents no information unless > > we strangely have a driver providing read_avail for _RAW but only > > _PROCESSED channels which is a bug. I'm not that bothered about > > missing information in this particular, somewhat obscure, corner case. > > > > So I think we should take the patch as it stands. It's missed the > > merge window now anyway unfortunately. So Peter, I would suggest we > > take this and perhaps revisit to tidy up loose corners when we all have > > more time. > > My concern was a driver with a raw channel, including read_avail, providing > raw sample values but that no easy conversion existed to get from that to > the processed values. One option for the driver in that case would be to > provide these raw values, but then have no scaling info. Generally I resist this a lot. The reason is that it is impossible to write generic userspace software against it. The one time we did let this happen was with some of the heart rate sensors (pulse oximeters) where the algorithm to derive the eventual value is both complex - based on published literature, and proprietary (what was actually readily usable). What the measurement being provided to userspace was is well documented, but not how on earth you get from that to something useable for what the sensor is designed to measure. > I.e. the way I see > it, it is perfectly reasonable for a driver to provide raw with read_avail, > no scaling but also processed values. Why? What use would the raw values actually be? There are a couple of historical drivers where they evolved to this state, but it is not one we would normally accept. We go to a lot of effort to try and avoid this. > And that gets transformed by the > rescaler into the processed values being presented as raw, with rescaling > added on top, but with the read_avail info for this new raw channel being > completely wrong. > > For the intended driver (ab8500-gpadc) this is not the case (it has no > read_avail for its raw channel). But it does have a raw channel, so adding > read_avail seems easy and I can easily see other drivers already doing it. > Haven't checked that though... Drat. I'd failed to register this is one of those corner cases. > > But if you say that this never happens, fine. Otherwise, since it's too > late for the merge window anyway, the patch might as well be updated such > that the rescaler blocks the read_avail channel in this situation, if it > exists. That's fair enough. A sanity check and then suitable warning message to explain why it is blocked makes sense. Jonathan > > Cheers, > Peter
On 2020-12-14 16:07, Jonathan Cameron wrote: > On Mon, 14 Dec 2020 09:34:40 +0100 > Peter Rosin <peda@axentia.se> wrote: > >> On 2020-12-13 13:16, Jonathan Cameron wrote: >>> On Sun, 13 Dec 2020 00:22:17 +0100 >>> Peter Rosin <peda@axentia.se> wrote: >>> >>>> On 2020-12-12 13:26, Linus Walleij wrote: >>>>> On Mon, Nov 2, 2020 at 12:22 AM 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> >>>>> >>>>> Did we reach any conclusion on this? I really need to use >>>>> the rescaler on an ADC that only handles processed channels... >>>>> >>>>> I'm sorry that I can't make this ADC disappear :D >>>> >>>> Hi! >>>> >>>> My conclusion was that the patch is buggy since it presents inconsistent >>>> information. That needs to be fixed one way or the other. If the offending >>>> information cannot be filtered out for some reason, I don't know what to >>>> do. Details in my previous comment [1]. BTW, I still do not know the answer >>>> to the .read_avail question at the end of that message, and I don't have >>>> time to dig into it. Sorry. >>> >>> Unless I'm missing something, I think it presents no information unless >>> we strangely have a driver providing read_avail for _RAW but only >>> _PROCESSED channels which is a bug. I'm not that bothered about >>> missing information in this particular, somewhat obscure, corner case. >>> >>> So I think we should take the patch as it stands. It's missed the >>> merge window now anyway unfortunately. So Peter, I would suggest we >>> take this and perhaps revisit to tidy up loose corners when we all have >>> more time. >> >> My concern was a driver with a raw channel, including read_avail, providing >> raw sample values but that no easy conversion existed to get from that to >> the processed values. One option for the driver in that case would be to >> provide these raw values, but then have no scaling info. > > Generally I resist this a lot. The reason is that it is impossible to write > generic userspace software against it. The one time we did let this happen > was with some of the heart rate sensors (pulse oximeters) where the algorithm > to derive the eventual value is both complex - based on published literature, > and proprietary (what was actually readily usable). What the measurement being > provided to userspace was is well documented, but not how on earth you get from > that to something useable for what the sensor is designed to measure. > >> I.e. the way I see >> it, it is perfectly reasonable for a driver to provide raw with read_avail, >> no scaling but also processed values. > > Why? What use would the raw values actually be? There are a couple of historical > drivers where they evolved to this state, but it is not one we would normally accept. > We go to a lot of effort to try and avoid this. Drivers that have eveloved over time is exactly one such reason. E.g. a driver starts out by not caring about wrong measurements at one end of the spectrum because it is "linear enough" for the first use, someone comes along and fixes that. But by that time it's impossible to completely remove the raw channel because that would be a regression for some reason. And there you are. A driver with raw plus read_avail, no scaling but a processed channel. Or something like that... >> And that gets transformed by the >> rescaler into the processed values being presented as raw, with rescaling >> added on top, but with the read_avail info for this new raw channel being >> completely wrong. >> >> For the intended driver (ab8500-gpadc) this is not the case (it has no >> read_avail for its raw channel). But it does have a raw channel, so adding >> read_avail seems easy and I can easily see other drivers already doing it. >> Haven't checked that though... > > Drat. I'd failed to register this is one of those corner cases. I'm not sure, I just browsed the code. Maybe I misread it? Cheers, Peter >> But if you say that this never happens, fine. Otherwise, since it's too >> late for the merge window anyway, the patch might as well be updated such >> that the rescaler blocks the read_avail channel in this situation, if it >> exists. > > That's fair enough. A sanity check and then suitable warning message to explain > why it is blocked makes sense.
On Mon, 14 Dec 2020 16:30:22 +0100 Peter Rosin <peda@axentia.se> wrote: > On 2020-12-14 16:07, Jonathan Cameron wrote: > > On Mon, 14 Dec 2020 09:34:40 +0100 > > Peter Rosin <peda@axentia.se> wrote: > > > >> On 2020-12-13 13:16, Jonathan Cameron wrote: > >>> On Sun, 13 Dec 2020 00:22:17 +0100 > >>> Peter Rosin <peda@axentia.se> wrote: > >>> > >>>> On 2020-12-12 13:26, Linus Walleij wrote: > >>>>> On Mon, Nov 2, 2020 at 12:22 AM 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> > >>>>> > >>>>> Did we reach any conclusion on this? I really need to use > >>>>> the rescaler on an ADC that only handles processed channels... > >>>>> > >>>>> I'm sorry that I can't make this ADC disappear :D > >>>> > >>>> Hi! > >>>> > >>>> My conclusion was that the patch is buggy since it presents inconsistent > >>>> information. That needs to be fixed one way or the other. If the offending > >>>> information cannot be filtered out for some reason, I don't know what to > >>>> do. Details in my previous comment [1]. BTW, I still do not know the answer > >>>> to the .read_avail question at the end of that message, and I don't have > >>>> time to dig into it. Sorry. > >>> > >>> Unless I'm missing something, I think it presents no information unless > >>> we strangely have a driver providing read_avail for _RAW but only > >>> _PROCESSED channels which is a bug. I'm not that bothered about > >>> missing information in this particular, somewhat obscure, corner case. > >>> > >>> So I think we should take the patch as it stands. It's missed the > >>> merge window now anyway unfortunately. So Peter, I would suggest we > >>> take this and perhaps revisit to tidy up loose corners when we all have > >>> more time. > >> > >> My concern was a driver with a raw channel, including read_avail, providing > >> raw sample values but that no easy conversion existed to get from that to > >> the processed values. One option for the driver in that case would be to > >> provide these raw values, but then have no scaling info. > > > > Generally I resist this a lot. The reason is that it is impossible to write > > generic userspace software against it. The one time we did let this happen > > was with some of the heart rate sensors (pulse oximeters) where the algorithm > > to derive the eventual value is both complex - based on published literature, > > and proprietary (what was actually readily usable). What the measurement being > > provided to userspace was is well documented, but not how on earth you get from > > that to something useable for what the sensor is designed to measure. > > > >> I.e. the way I see > >> it, it is perfectly reasonable for a driver to provide raw with read_avail, > >> no scaling but also processed values. > > > > Why? What use would the raw values actually be? There are a couple of historical > > drivers where they evolved to this state, but it is not one we would normally accept. > > We go to a lot of effort to try and avoid this. > > Drivers that have eveloved over time is exactly one such reason. E.g. a driver > starts out by not caring about wrong measurements at one end of the spectrum > because it is "linear enough" for the first use, someone comes along and fixes > that. But by that time it's impossible to completely remove the raw channel > because that would be a regression for some reason. And there you are. A > driver with raw plus read_avail, no scaling but a processed channel. Or > something like that... Yup, that's pretty much what tends to happen. I've gotten a lot stricter on checking datasheets to try and stop this happening, but still possible more will slip through (particularly as can't always get the datasheet) > > >> And that gets transformed by the > >> rescaler into the processed values being presented as raw, with rescaling > >> added on top, but with the read_avail info for this new raw channel being > >> completely wrong. > >> > >> For the intended driver (ab8500-gpadc) this is not the case (it has no > >> read_avail for its raw channel). But it does have a raw channel, so adding > >> read_avail seems easy and I can easily see other drivers already doing it. > >> Haven't checked that though... > > > > Drat. I'd failed to register this is one of those corner cases. > > I'm not sure, I just browsed the code. Maybe I misread it? It's doing both - you were right. I think there are only a small number of drivers that have that history. Looks superficially like it's easy enough to catch this corner case and block it - so lets do that. Jonathan > > Cheers, > Peter > > >> But if you say that this never happens, fine. Otherwise, since it's too > >> late for the merge window anyway, the patch might as well be updated such > >> that the rescaler blocks the read_avail channel in this situation, if it > >> exists. > > > > That's fair enough. A sanity check and then suitable warning message to explain > > why it is blocked makes sense. >
On Mon, Dec 14, 2020 at 5:34 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Mon, 14 Dec 2020 16:30:22 +0100 Peter Rosin <peda@axentia.se> wrote: > > >> And that gets transformed by the > > >> rescaler into the processed values being presented as raw, with rescaling > > >> added on top, but with the read_avail info for this new raw channel being > > >> completely wrong. > > >> > > >> For the intended driver (ab8500-gpadc) this is not the case (it has no > > >> read_avail for its raw channel). But it does have a raw channel, so adding > > >> read_avail seems easy and I can easily see other drivers already doing it. > > >> Haven't checked that though... > > > > > > Drat. I'd failed to register this is one of those corner cases. > > > > I'm not sure, I just browsed the code. Maybe I misread it? > > It's doing both - you were right. I think there are only a small number of > drivers that have that history. > > Looks superficially like it's easy enough to catch this corner case and > block it - so lets do that. Sorry if I am a bit confused here. I don't understand what I am supposed to do to proceed with using this driver with the ab8500 GPADC... Shall I fix something in the AB8500 GPADC as a prerequisite? In that case I think I need some more pointers... Yours, Linus Walleij
On Mon, 4 Jan 2021 15:45:07 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Dec 14, 2020 at 5:34 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > On Mon, 14 Dec 2020 16:30:22 +0100 Peter Rosin <peda@axentia.se> wrote: > > > > >> And that gets transformed by the > > > >> rescaler into the processed values being presented as raw, with rescaling > > > >> added on top, but with the read_avail info for this new raw channel being > > > >> completely wrong. > > > >> > > > >> For the intended driver (ab8500-gpadc) this is not the case (it has no > > > >> read_avail for its raw channel). But it does have a raw channel, so adding > > > >> read_avail seems easy and I can easily see other drivers already doing it. > > > >> Haven't checked that though... > > > > > > > > Drat. I'd failed to register this is one of those corner cases. > > > > > > I'm not sure, I just browsed the code. Maybe I misread it? > > > > It's doing both - you were right. I think there are only a small number of > > drivers that have that history. > > > > Looks superficially like it's easy enough to catch this corner case and > > block it - so lets do that. > > Sorry if I am a bit confused here. I don't understand what I am supposed > to do to proceed with using this driver with the ab8500 GPADC... > > Shall I fix something in the AB8500 GPADC as a prerequisite? > In that case I think I need some more pointers... I confess I'm a bit lost, but I 'think' the problem we had left was around read_avail which doesn't play well if we it defined for the _raw value in the provider, but not the _processed value. So if we detect their is a _processed channel (which we are going to use) we just need to make sure that we don't pass the read_avail for _raw through to be exposed by the rescale driver as the consumer as it will be garbage. Best plan is probably to just pretend the read_avail for the provider doesn't exist in this case. @Peter, does that cover it of are there other similar cases? It definitely also wants a big fat comment saying why we are hiding this! Jonathan > > Yours, > Linus Walleij
On 2021-01-04 18:11, Jonathan Cameron wrote: > On Mon, 4 Jan 2021 15:45:07 +0100 > Linus Walleij <linus.walleij@linaro.org> wrote: > >> On Mon, Dec 14, 2020 at 5:34 PM Jonathan Cameron >> <Jonathan.Cameron@huawei.com> wrote: >>> On Mon, 14 Dec 2020 16:30:22 +0100 Peter Rosin <peda@axentia.se> wrote: >> >>>>>> And that gets transformed by the >>>>>> rescaler into the processed values being presented as raw, with rescaling >>>>>> added on top, but with the read_avail info for this new raw channel being >>>>>> completely wrong. >>>>>> >>>>>> For the intended driver (ab8500-gpadc) this is not the case (it has no >>>>>> read_avail for its raw channel). But it does have a raw channel, so adding >>>>>> read_avail seems easy and I can easily see other drivers already doing it. >>>>>> Haven't checked that though... >>>>> >>>>> Drat. I'd failed to register this is one of those corner cases. >>>> >>>> I'm not sure, I just browsed the code. Maybe I misread it? >>> >>> It's doing both - you were right. I think there are only a small number of >>> drivers that have that history. >>> >>> Looks superficially like it's easy enough to catch this corner case and >>> block it - so lets do that. >> >> Sorry if I am a bit confused here. I don't understand what I am supposed >> to do to proceed with using this driver with the ab8500 GPADC... >> >> Shall I fix something in the AB8500 GPADC as a prerequisite? >> In that case I think I need some more pointers... > > I confess I'm a bit lost, but I 'think' the problem we had > left was around read_avail which doesn't play well if we > it defined for the _raw value in the provider, but not the _processed value. > > So if we detect their is a _processed channel (which we are going to use) we > just need to make sure that we don't pass the read_avail for _raw through > to be exposed by the rescale driver as the consumer as it will be garbage. > Best plan is probably to just pretend the read_avail for the provider doesn't > exist in this case. > > @Peter, does that cover it of are there other similar cases? Yes, that's it. Just hide _raw in read_avail if we are proceding with _processed as _raw. > It definitely also wants a big fat comment saying why we are hiding this! Yup. Cheers, Peter
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) |
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> --- drivers/iio/afe/iio-rescale.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)