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

[Guenter Roeck]

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