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@xxxxxxxxx/
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
