Message-ID: <20220222112315.GA1702745@roeck-us.net>
Date:   Tue, 22 Feb 2022 03:23:15 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Krzysztof Adamski <krzysztof.adamski@...ia.com>
Cc:     Rob Herring <robh+dt@...nel.org>, linux-hwmon@...r.kernel.org,
        Agathe Porte <agathe.porte@...ia.com>,
        Jean Delvare <jdelvare@...e.com>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] dt-bindings: hwmon: add tmp464.yaml

On Tue, Feb 22, 2022 at 08:23:35AM +0100, Krzysztof Adamski wrote:
> Dnia Mon, Feb 21, 2022 at 02:11:17PM -0800, Guenter Roeck napisał(a):
> > 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.
> > 
> 
> Yes, you are correct, you have to enter either <(-1)> or <0xffffffff>
> (which is the same thing). I was quite confused on how to specify that
> in DT bindings and apparently maybe we did not understand each other
> well enough back when the patch was submitted. The code and the
> description is correct, though, so the question is how to properly set

Here is where we disagree. The bindings say:

    items:
      minimum: 0
      maximum: 255

Based on this, the following devicetree configuration should be correct.

        tmp423a@4c {
                compatible = "ti,tmp423";
                reg = <0x4c>;
		#address-cells = <1>;
                #size-cells = <0>;
 
                channel@1 {
                    reg = <1>;
                    ti,n-factor = <255>;
                };
        };

However, it results in:

tmp421 4-004c: n-factor for channel 1 invalid (255)
tmp421: probe of 4-004c failed with error -22

Either the range is 0 ... 255 and we need to accept 0 ... 255,
or the range is -128 ... 127 and we need to accept -128 ... 127.

> "minimum" and "maximum" value.
> 
> I think I should at least update the example of tmp421 in the binding to
> use <(-1)> notation to make it obvious that it works this way. But I
> guess we need Rob to help us understand how this should be specified.
> 
> In any case, if you drop usage of /bits 8/ and keep the property naitive
> 32 bit, both tmp421 and tmp464 bindings will be compatible with each
> other.
> 
> @Rob, if I want a property ti,n-factor be in in range from <(-128)> to
> <127>, so that we can use of_property_read_s32() and then use just one
> byte of that, how to specify that in yaml file? For TMP421, we currently
> have:
> 
>  ti,n-factor:
>    description: |
>      The value (two's complement) to be programmed in the channel specific N correction register.
>      For remote channels only.
>    $ref: /schemas/types.yaml#/definitions/uint32
>    items:
>      minimum: 0
>      maximum: 255
> 
> which is confusing.
> 
> Guenter is proposing to use
>  $ref: /schemas/types.yaml#/definitions/int8
>  items:
>    minimum: -128
>    maximum: 127
> 
> and of_property_read_u8() for the same property on another driver, so
> usage of /bits/ 8 is required. Which approach is better in your opinion?
> 

I could declare the property as int32, use of_property_read_s32, and
validate the range in the driver. However, the range still needs
to match the documentation.

Guenter

Powered by blists - more mailing lists