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: <238e6fb0-cd77-4772-3e92-23941dc74403@roeck-us.net>
Date:   Mon, 21 Feb 2022 14:11:17 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Krzysztof Adamski <krzysztof.adamski@...ia.com>
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

On 2/21/22 13:24, Krzysztof Adamski wrote:
> 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 :)
> 

It is hard for me to believe that you can enter, say, 255 into the dts file
and of_property_read_s32() reads it as -1. If so, of_property_read_s32()
could never support values of 128 and above, which seems unlikely.

Now, I can imagine that one can enter 0xffffffff and of_property_read_s32()
returns -1, but that isn't what is documented for tmp421.

Guenter

> 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