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
