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 <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> Changes in v2:
> - Fix rescale_read_raw() handle channels with offset but no scale.
> - Link to v1: https://lore.kernel.org/r/[email protected]
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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