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

[Krzysztof Adamski <krzysztof.adamski@nokia.com>]

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