linux-kernel - Re: [PATCH v2] iio: afe: rescale: Accept only offset channels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <948548a0-d132-4f5c-819e-40bacb367be4@axentia.se>
Date:   Mon, 16 Oct 2023 12:05:32 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Liam Beguin <liambeguin@...il.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: afe: rescale: Accept only offset channels

Hi!

2023-10-16 at 10:39, Linus Walleij wrote:
> On Sun, Oct 15, 2023 at 12:38 AM Peter Rosin <peda@...ntia.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