| Message ID | 20220218150908.1947772-1-linux@roeck-us.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/2] dt-bindings: hwmon: add tmp464.yaml | expand |
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 :) Here's the actual discussion where all this was considered: https://patchwork.kernel.org/project/linux-hwmon/patch/3ff7b4cc57dab2073fa091072366c1e524631729.1632984254.git.krzysztof.adamski@nokia.com/ 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
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@nokia.com/ > > 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
diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml new file mode 100644 index 000000000000..14f6a3412b8c --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml @@ -0,0 +1,114 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TMP464 and TMP468 temperature sensors + +maintainers: + - Agathe Porte <agathe.porte@nokia.com> + +description: | + ±0.0625°C Remote and Local temperature sensor + https://www.ti.com/lit/ds/symlink/tmp464.pdf + https://www.ti.com/lit/ds/symlink/tmp468.pdf + +properties: + compatible: + enum: + - ti,tmp464 + - ti,tmp468 + + reg: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +required: + - compatible + - reg + +additionalProperties: false + +patternProperties: + "^channel@([0-8])$": + type: object + description: | + Represents channels of the device and their specific configuration. + + properties: + reg: + description: | + The channel number. 0 is local channel, 1-8 are remote channels. + items: + minimum: 0 + maximum: 8 + + label: + description: | + A descriptive name for this channel, like "ambient" or "psu". + + 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/int8 + items: + minimum: -128 + maximum: 127 + + required: + - reg + + additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@4b { + compatible = "ti,tmp464"; + reg = <0x4b>; + }; + }; + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@4b { + compatible = "ti,tmp464"; + reg = <0x4b>; + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0x0>; + label = "local"; + }; + + channel@1 { + reg = <0x1>; + ti,n-factor = /bits/ 8 <(-10)>; + label = "external"; + }; + + channel@2 { + reg = <0x2>; + ti,n-factor = /bits/ 8 <0x10>; + label = "somelabel"; + }; + + channel@3 { + reg = <0x3>; + status = "disabled"; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index fca970a46e77..f51bc7c7e439 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19489,6 +19489,13 @@ S: Maintained F: Documentation/hwmon/tmp401.rst F: drivers/hwmon/tmp401.c +TMP464 HARDWARE MONITOR DRIVER +M: Agathe Porte <agathe.porte@nokia.com> +M: Guenter Roeck <linux@roeck-us.net> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml + TMP513 HARDWARE MONITOR DRIVER M: Eric Tremblay <etremblay@distech-controls.com> L: linux-hwmon@vger.kernel.org