lkml.org 
[lkml]   [2022]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-02-22 12:24    [W:0.202 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site