linux-kernel - Re: [PATCH v5 1/2] dt-bindings: hwmon: add tmp464.yaml

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <YhQDGHD3S0qwP2OB@localhost.localdomain>
Date:   Mon, 21 Feb 2022 22:24:40 +0100
From:   Krzysztof Adamski <krzysztof.adamski@...ia.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-hwmon@...r.kernel.org, Agathe Porte <agathe.porte@...ia.com>,
        Jean Delvare <jdelvare@...e.com>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] dt-bindings: hwmon: add tmp464.yaml

Dnia Mon, Feb 21, 2022 at 08:16:15AM -0800, Guenter Roeck napisał(a):
>>I still thing we should have the same format here and in tmp421, for
>>consistency. If use the same property name, "ti,n-factor" but on tmp421
>>you have use 32bit value while here you have to use 8bit (which is weird
>>in DT, BTW), it might be confusing.
>>Back when we did this for TMP421, there was some discussion and we
>>settled on this approach, why do it differently now?
>>
>
>I seem to recall from that discussion that there was supposedly no way to
>express negative numbers in devicetree. Obviously that is incorrect.

Well, I would still argue it *is* correct. DT only support unsigned
numbers and, really, only 32 or 64 bit. See the chapter 2.2.4 Properties
in:
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf

Devicetree also supports array of bytes, and this is how we get the
/bits/ magic but this is just a syntactic suggar. The same is true about
negative values. Just decompile your compiled DTB and you will see.
To put it in other words - DTS does support negative values, DTB don't.j

>In addition to that, I strongly suspect that the tmp421 code as written
>does not work. Its value range is specified as 0..255, but it is read with
>	err = of_property_read_s32(child, "ti,n-factor", &val);
>and range checked with
>	if (val > 127 || val < -128) {
>                dev_err(dev, "n-factor for channel %d invalid (%d)\n",
>                       i, val);
>                return -EINVAL;
>        }
>
>That just looks wrong. Either the value range is 0..255 and checked
>as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
>accordingly. This made me look into the code and I found how negative
>numbers are supposed to be handled.

It worked for me when I tested that. I could redo the test, if you are
unsure. The code also looks good to me. I wasn't convinced for this
format in yaml but after the whole discussion we had, we settled on
that, with Robs blessing :)

Here's the actual discussion where all this was considered:
https://patchwork.kernel.org/project/linux-hwmon/patch/3ff7b4cc57dab2073fa091072366c1e524631729.1632984254.git.krzysztof.adamski@nokia.com/

I'm not saying the way we do this for tmp421 is better or even right,
all I say is that it would make sense to be consistent and not redo this
discussion every time we have this problem.

Krzysztof