| Message ID | 20230902-iio-rescale-only-offset-v2-1-988b807754c8@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: afe: rescale: Accept only offset channels | expand |
On Sat, 02 Sep 2023 21:46:20 +0200 Linus Walleij <linus.walleij@linaro.org> wrote: > As noted by Jonathan Cameron: it is perfectly legal for a channel > to have an offset but no scale in addition to the raw interface. > The conversion will imply that scale is 1:1. > > Make rescale_configure_channel() accept just scale, or just offset > to process a channel. > > When a user asks for IIO_CHAN_INFO_OFFSET in rescale_read_raw() > we now have to deal with the fact that OFFSET could be present > but SCALE missing. Add code to simply scale 1:1 in this case. > > Link: https://lore.kernel.org/linux-iio/CACRpkdZXBjHU4t-GVOCFxRO-AHGxKnxMeHD2s4Y4PuC29gBq6g@mail.gmail.com/ > Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels") > Fixes: 9decacd8b3a4 ("iio: afe: rescale: Fix boolean logic bug") > Reported-by: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Peter, can you take a look at this v2? Thanks, Jonathan > --- > Changes in v2: > - Fix rescale_read_raw() handle channels with offset but no scale. > - Link to v1: https://lore.kernel.org/r/20230830-iio-rescale-only-offset-v1-1-40ab9f4436c7@linaro.org > --- > drivers/iio/afe/iio-rescale.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 1f280c360701..56e5913ab82d 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -214,8 +214,18 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > return ret < 0 ? ret : -EOPNOTSUPP; > } > > - ret = iio_read_channel_scale(rescale->source, &scale, &scale2); > - return rescale_process_offset(rescale, ret, scale, scale2, > + if (iio_channel_has_info(rescale->source->channel, > + IIO_CHAN_INFO_SCALE)) { > + ret = iio_read_channel_scale(rescale->source, &scale, &scale2); > + return rescale_process_offset(rescale, ret, scale, scale2, > + schan_off, val, val2); > + } > + > + /* > + * If we get here we have no scale so scale 1:1 but apply > + * rescaler and offset, if any. > + */ > + return rescale_process_offset(rescale, IIO_VAL_FRACTIONAL, 1, 1, > schan_off, val, val2); > default: > return -EINVAL; > @@ -280,8 +290,9 @@ static int rescale_configure_channel(struct device *dev, > chan->type = rescale->cfg->type; > > if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) && > - iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { > - dev_info(dev, "using raw+scale source channel\n"); > + (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) || > + iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) { > + dev_info(dev, "using raw+scale/offset source channel\n"); > } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) { > dev_info(dev, "using processed channel\n"); > rescale->chan_processed = true; > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230830-iio-rescale-only-offset-f28e05bd2deb > > Best regards,
Hi! Sorry for the delay, and thank you Jonathan for the reminder. 2023-09-02 at 21:46, Linus Walleij wrote: > As noted by Jonathan Cameron: it is perfectly legal for a channel > to have an offset but no scale in addition to the raw interface. > The conversion will imply that scale is 1:1. > > Make rescale_configure_channel() accept just scale, or just offset > to process a channel. > > When a user asks for IIO_CHAN_INFO_OFFSET in rescale_read_raw() > we now have to deal with the fact that OFFSET could be present > but SCALE missing. Add code to simply scale 1:1 in this case. > > Link: https://lore.kernel.org/linux-iio/CACRpkdZXBjHU4t-GVOCFxRO-AHGxKnxMeHD2s4Y4PuC29gBq6g@mail.gmail.com/ > Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels") > Fixes: 9decacd8b3a4 ("iio: afe: rescale: Fix boolean logic bug") > Reported-by: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Changes in v2: > - Fix rescale_read_raw() handle channels with offset but no scale. > - Link to v1: https://lore.kernel.org/r/20230830-iio-rescale-only-offset-v1-1-40ab9f4436c7@linaro.org > --- > drivers/iio/afe/iio-rescale.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 1f280c360701..56e5913ab82d 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -214,8 +214,18 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > return ret < 0 ? ret : -EOPNOTSUPP; > } > > - ret = iio_read_channel_scale(rescale->source, &scale, &scale2); > - return rescale_process_offset(rescale, ret, scale, scale2, > + if (iio_channel_has_info(rescale->source->channel, > + IIO_CHAN_INFO_SCALE)) { > + ret = iio_read_channel_scale(rescale->source, &scale, &scale2); > + return rescale_process_offset(rescale, ret, scale, scale2, > + schan_off, val, val2); > + } > + > + /* > + * If we get here we have no scale so scale 1:1 but apply > + * rescaler and offset, if any. > + */ > + return rescale_process_offset(rescale, IIO_VAL_FRACTIONAL, 1, 1, > schan_off, val, val2); > default: > return -EINVAL; > @@ -280,8 +290,9 @@ static int rescale_configure_channel(struct device *dev, > chan->type = rescale->cfg->type; > > if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) && > - iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { > - dev_info(dev, "using raw+scale source channel\n"); > + (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) || > + iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) { > + dev_info(dev, "using raw+scale/offset source channel\n"); If the rules really are that when not provided scale is 1 and offset 0 (reasonable of course) then the above still looks suspect to me. Should this part not simply be if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW)) { dev_info(dev, "using raw source channel\n"); in that case? Or was "raw + processed" some kind of special case that we want to handle as processed? If that's the case then we need to have more complex logic. Cheers, Peter > } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) { > dev_info(dev, "using processed channel\n"); > rescale->chan_processed = true; > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230830-iio-rescale-only-offset-f28e05bd2deb > > Best regards,
On Sun, Oct 15, 2023 at 12:38 AM Peter Rosin <peda@axentia.se> wrote: > 2023-09-02 at 21:46, Linus Walleij wrote: > > if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) && > > - iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { > > - dev_info(dev, "using raw+scale source channel\n"); > > + (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) || > > + iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) { > > + dev_info(dev, "using raw+scale/offset source channel\n"); > > If the rules really are that when not provided scale is 1 and offset 0 > (reasonable of course) then the above still looks suspect to me. Should > this part not simply be > > if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW)) { > dev_info(dev, "using raw source channel\n"); > > in that case? The patch is based on Jonathan's comment that while we currently support raw+scale, having just raw+offset provided is a possibility. The if()-clause above (which I guess you are commenting) is meant as "take this path if scale or offset or both are provided". Just raw (with neither offset or rescale) doesn't make sense, since the AFE rescaler does just offsetting and rescaling, in that case the user should just use the raw channel. Also it would then take precedence over a processed channel (which applies rescale and offset internally) which doesn't make sense to me. > Or was "raw + processed" some kind of special case that we want to handle > as processed? If that's the case then we need to have more complex logic. Processed is on the else-path, which will be tried only when neither scale nor offset is provided: > } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) { > dev_info(dev, "using processed channel\n"); > rescale->chan_processed = true; I'm not sure I fully understood the remark, please elaborate if I got it wrong! Yours, Linus Walleij
Hi! 2023-10-16 at 10:39, Linus Walleij wrote: > On Sun, Oct 15, 2023 at 12:38 AM Peter Rosin <peda@axentia.se> wrote: >> 2023-09-02 at 21:46, Linus Walleij wrote: > >>> if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) && >>> - iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { >>> - dev_info(dev, "using raw+scale source channel\n"); >>> + (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) || >>> + iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) { >>> + dev_info(dev, "using raw+scale/offset source channel\n"); >> >> If the rules really are that when not provided scale is 1 and offset 0 >> (reasonable of course) then the above still looks suspect to me. Should >> this part not simply be >> >> if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW)) { >> dev_info(dev, "using raw source channel\n"); >> >> in that case? > > The patch is based on Jonathan's comment that while we currently > support raw+scale, having just raw+offset provided is a possibility. > > The if()-clause above (which I guess you are commenting) is meant > as "take this path if scale or offset or both are provided". > > Just raw (with neither offset or rescale) doesn't make sense, since And I don't see why not. That's the crux. > the AFE rescaler does just offsetting and rescaling, in that case the > user should just use the raw channel. Also it would then take > precedence over a processed channel (which applies rescale and > offset internally) which doesn't make sense to me. Why isn't it perfectly fine for a device to provide only a raw channel and then expect that to be interpreted as the real unit? Why would it need a processed channel when no processing is going on? E.g. a device reporting the temp in the expected unit in one of its registers. Or whatever with such a friendly register. And if the above holds, it should also be perfectly fine to run that through the rescaler. > >> Or was "raw + processed" some kind of special case that we want to handle >> as processed? If that's the case then we need to have more complex logic. > > Processed is on the else-path, which will be tried only when neither > scale nor offset is provided: > >> } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) { >> dev_info(dev, "using processed channel\n"); >> rescale->chan_processed = true; > > I'm not sure I fully understood the remark, please elaborate if I got it wrong! I agree that the patch does exactly as you intend. I question if what you intend is correct, but since I don't know the rules, I'd simply like to have the rules clarified. Is that clearer? Cheers, Peter
On Mon, Oct 16, 2023 at 12:05 PM Peter Rosin <peda@axentia.se> wrote: > > Just raw (with neither offset or rescale) doesn't make sense, since > > And I don't see why not. That's the crux. OK I can implement that, but then we need to define the priority of "just raw" vs "processed". It is quite common that ADC drivers provide raw and processed. Which one goes first? Right now the priority is: 1. Raw + scale, if scale exists else 2. Processed After this patch the priority would be: 1. Raw+scale OR Raw+offset if either scale or offset exists else 2. Processed How do you expect a raw channel to be prioritized? I can only put it last, as putting it second would break existing users that provide both raw and processed. Is this how you imagine this to work? Further, that could be a separate patch on top of this so it is a little bit of feature creepy to push into this patch, but I can make a 2-patch series if you like. It basically does not block applying this one patch on the way there. > > the AFE rescaler does just offsetting and rescaling, in that case the > > user should just use the raw channel. Also it would then take > > precedence over a processed channel (which applies rescale and > > offset internally) which doesn't make sense to me. > > Why isn't it perfectly fine for a device to provide only a raw > channel and then expect that to be interpreted as the real unit? You're right there is no problem with that. The only problem I have with it is how to prioritize it. Would need Jonathan's feedback here too though, I might be missing something. > Why would it need a processed channel when no processing is > going on? E.g. a device reporting the temp in the expected unit > in one of its registers. Or whatever with such a friendly > register. Good point. Unless someone would call that a "processed channel" albeit processed in hardware. But this definition of raw == raw register reads works for me. > > I'm not sure I fully understood the remark, please elaborate if I got it wrong! > > I agree that the patch does exactly as you intend. I question if > what you intend is correct, but since I don't know the rules, I'd > simply like to have the rules clarified. Like I know the rules :D Whenever I do anything in IIO I feel like Socrates, all I know is that I know nothing. Yours, Linus Walleij
On Mon, 16 Oct 2023 12:05:32 +0200 Peter Rosin <peda@axentia.se> wrote: > Hi! > > 2023-10-16 at 10:39, Linus Walleij wrote: > > On Sun, Oct 15, 2023 at 12:38 AM Peter Rosin <peda@axentia.se> wrote: > >> 2023-09-02 at 21:46, Linus Walleij wrote: > > > >>> if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) && > >>> - iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { > >>> - dev_info(dev, "using raw+scale source channel\n"); > >>> + (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) || > >>> + iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) { > >>> + dev_info(dev, "using raw+scale/offset source channel\n"); > >> > >> If the rules really are that when not provided scale is 1 and offset 0 > >> (reasonable of course) then the above still looks suspect to me. Should > >> this part not simply be > >> > >> if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW)) { > >> dev_info(dev, "using raw source channel\n"); > >> > >> in that case? > > > > The patch is based on Jonathan's comment that while we currently > > support raw+scale, having just raw+offset provided is a possibility. > > > > The if()-clause above (which I guess you are commenting) is meant > > as "take this path if scale or offset or both are provided". > > > > Just raw (with neither offset or rescale) doesn't make sense, since > > And I don't see why not. That's the crux. > > > the AFE rescaler does just offsetting and rescaling, in that case the > > user should just use the raw channel. Also it would then take > > precedence over a processed channel (which applies rescale and > > offset internally) which doesn't make sense to me. > > Why isn't it perfectly fine for a device to provide only a raw > channel and then expect that to be interpreted as the real unit? > Why would it need a processed channel when no processing is > going on? E.g. a device reporting the temp in the expected unit > in one of its registers. Or whatever with such a friendly > register. In that case it should report a processed value to indicate that. It's admittedly a bit of a corner case given it's not processed by the kernel - there is an argument that this (more or less) only happens when someone has processed a raw ADC count but in theory that's not necessarily true. There are a few examples of drivers passing through the register value as processed in tree - normally when there is a microprocessor doing some fusion of signals or similar. Raw gets reported on it's own in a few other cases, such as when there are no known units - that happens for things like light intensity, proximity (which is often reflected light intensity). For those I'm not sure the rescaler is useful. > > And if the above holds, it should also be perfectly fine to run > that through the rescaler. > > > > >> Or was "raw + processed" some kind of special case that we want to handle > >> as processed? If that's the case then we need to have more complex logic. > > > > Processed is on the else-path, which will be tried only when neither > > scale nor offset is provided: > > > >> } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) { > >> dev_info(dev, "using processed channel\n"); > >> rescale->chan_processed = true; > > > > I'm not sure I fully understood the remark, please elaborate if I got it wrong! > > I agree that the patch does exactly as you intend. I question if > what you intend is correct, but since I don't know the rules, I'd > simply like to have the rules clarified. > > Is that clearer? > > Cheers, > Peter
Hi! 2023-10-17 at 11:05, Jonathan Cameron wrote: > On Mon, 16 Oct 2023 12:05:32 +0200 > Peter Rosin <peda@axentia.se> wrote: >> 2023-10-16 at 10:39, Linus Walleij wrote: >>> Just raw (with neither offset or rescale) doesn't make sense, since >> >> And I don't see why not. That's the crux. >> >>> the AFE rescaler does just offsetting and rescaling, in that case the >>> user should just use the raw channel. Also it would then take >>> precedence over a processed channel (which applies rescale and >>> offset internally) which doesn't make sense to me. >> >> Why isn't it perfectly fine for a device to provide only a raw >> channel and then expect that to be interpreted as the real unit? >> Why would it need a processed channel when no processing is >> going on? E.g. a device reporting the temp in the expected unit >> in one of its registers. Or whatever with such a friendly >> register. > > In that case it should report a processed value to indicate that. > It's admittedly a bit of a corner case given it's not processed by > the kernel - there is an argument that this (more or less) only > happens when someone has processed a raw ADC count but in theory > that's not necessarily true. > > There are a few examples of drivers passing through the register value > as processed in tree - normally when there > is a microprocessor doing some fusion of signals or similar. > > Raw gets reported on it's own in a few other cases, such as when > there are no known units - that happens for things like light intensity, > proximity (which is often reflected light intensity). > For those I'm not sure the rescaler is useful. Excellent, thanks for the clarification! Reviewed-by: Peter Rosin <peda@axentia.se> Cheers, Peter
On Tue, 17 Oct 2023 16:00:22 +0200 Peter Rosin <peda@axentia.se> wrote: > Hi! > > 2023-10-17 at 11:05, Jonathan Cameron wrote: > > On Mon, 16 Oct 2023 12:05:32 +0200 > > Peter Rosin <peda@axentia.se> wrote: > >> 2023-10-16 at 10:39, Linus Walleij wrote: > > > > >>> Just raw (with neither offset or rescale) doesn't make sense, since > >> > >> And I don't see why not. That's the crux. > >> > >>> the AFE rescaler does just offsetting and rescaling, in that case the > >>> user should just use the raw channel. Also it would then take > >>> precedence over a processed channel (which applies rescale and > >>> offset internally) which doesn't make sense to me. > >> > >> Why isn't it perfectly fine for a device to provide only a raw > >> channel and then expect that to be interpreted as the real unit? > >> Why would it need a processed channel when no processing is > >> going on? E.g. a device reporting the temp in the expected unit > >> in one of its registers. Or whatever with such a friendly > >> register. > > > > In that case it should report a processed value to indicate that. > > It's admittedly a bit of a corner case given it's not processed by > > the kernel - there is an argument that this (more or less) only > > happens when someone has processed a raw ADC count but in theory > > that's not necessarily true. > > > > There are a few examples of drivers passing through the register value > > as processed in tree - normally when there > > is a microprocessor doing some fusion of signals or similar. > > > > Raw gets reported on it's own in a few other cases, such as when > > there are no known units - that happens for things like light intensity, > > proximity (which is often reflected light intensity). > > For those I'm not sure the rescaler is useful. > > Excellent, thanks for the clarification! > > Reviewed-by: Peter Rosin <peda@axentia.se> Thanks, Applied to the fixes-togreg branch of iio.git. I'll just let this sit in linux-next for a day or so before a pull request (I have a few other fixes queued). That will almost certainly get queued for the merge window given timing. Thanks, Jonathan > > Cheers, > Peter
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 1f280c360701..56e5913ab82d 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -214,8 +214,18 @@ static int rescale_read_raw(struct iio_dev *indio_dev, return ret < 0 ? ret : -EOPNOTSUPP; } - ret = iio_read_channel_scale(rescale->source, &scale, &scale2); - return rescale_process_offset(rescale, ret, scale, scale2, + if (iio_channel_has_info(rescale->source->channel, + IIO_CHAN_INFO_SCALE)) { + ret = iio_read_channel_scale(rescale->source, &scale, &scale2); + return rescale_process_offset(rescale, ret, scale, scale2, + schan_off, val, val2); + } + + /* + * If we get here we have no scale so scale 1:1 but apply + * rescaler and offset, if any. + */ + return rescale_process_offset(rescale, IIO_VAL_FRACTIONAL, 1, 1, schan_off, val, val2); default: return -EINVAL; @@ -280,8 +290,9 @@ static int rescale_configure_channel(struct device *dev, chan->type = rescale->cfg->type; if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) && - iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { - dev_info(dev, "using raw+scale source channel\n"); + (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) || + iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) { + dev_info(dev, "using raw+scale/offset source channel\n"); } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) { dev_info(dev, "using processed channel\n"); rescale->chan_processed = true;
As noted by Jonathan Cameron: it is perfectly legal for a channel to have an offset but no scale in addition to the raw interface. The conversion will imply that scale is 1:1. Make rescale_configure_channel() accept just scale, or just offset to process a channel. When a user asks for IIO_CHAN_INFO_OFFSET in rescale_read_raw() we now have to deal with the fact that OFFSET could be present but SCALE missing. Add code to simply scale 1:1 in this case. Link: https://lore.kernel.org/linux-iio/CACRpkdZXBjHU4t-GVOCFxRO-AHGxKnxMeHD2s4Y4PuC29gBq6g@mail.gmail.com/ Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels") Fixes: 9decacd8b3a4 ("iio: afe: rescale: Fix boolean logic bug") Reported-by: Jonathan Cameron <jic23@kernel.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Changes in v2: - Fix rescale_read_raw() handle channels with offset but no scale. - Link to v1: https://lore.kernel.org/r/20230830-iio-rescale-only-offset-v1-1-40ab9f4436c7@linaro.org --- drivers/iio/afe/iio-rescale.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230830-iio-rescale-only-offset-f28e05bd2deb Best regards,