| Message ID | 20220218150908.1947772-1-linux@roeck-us.net |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v5,1/2] dt-bindings: hwmon: add tmp464.yaml | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success | |
| robh/patch-applied | success |
Dnia Fri, Feb 18, 2022 at 07:09:07AM -0800, Guenter Roeck napisał(a): >From: Agathe Porte <agathe.porte@nokia.com> > >Add basic description of the tmp464 driver DT bindings. > >Signed-off-by: Agathe Porte <agathe.porte@nokia.com> >Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com> >Signed-off-by: Guenter Roeck <linux@roeck-us.net> >--- >v5: >- Dropped ti,n-factor from channel@0 example. Added additional > channel to examples to show positive ti,n-factor property. > >v4: >- No changes > >v3: >- Addedd support for TMP468. >- Changed number of channels from 0..3 (which was wrong anyway) to 0..8. >- Changed value range for ti,n-factor to int8, with an example for > a negative value. >- Added myself as driver maintainer. > > .../devicetree/bindings/hwmon/ti,tmp464.yaml | 114 ++++++++++++++++++ > MAINTAINERS | 7 ++ > 2 files changed, 121 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml > >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 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? Krzysztof
On 2/21/22 01:07, Krzysztof Adamski wrote: > Dnia Fri, Feb 18, 2022 at 07:09:07AM -0800, Guenter Roeck napisał(a): >> From: Agathe Porte <agathe.porte@nokia.com> >> >> Add basic description of the tmp464 driver DT bindings. >> >> Signed-off-by: Agathe Porte <agathe.porte@nokia.com> >> Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> v5: >> - Dropped ti,n-factor from channel@0 example. Added additional >> channel to examples to show positive ti,n-factor property. >> >> v4: >> - No changes >> >> v3: >> - Addedd support for TMP468. >> - Changed number of channels from 0..3 (which was wrong anyway) to 0..8. >> - Changed value range for ti,n-factor to int8, with an example for >> a negative value. >> - Added myself as driver maintainer. >> >> .../devicetree/bindings/hwmon/ti,tmp464.yaml | 114 ++++++++++++++++++ >> MAINTAINERS | 7 ++ >> 2 files changed, 121 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml >> >> 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 > > 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. 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. We can go either way, but whatever it is should be correct and be confirmed to work. Rob, any thoughts ? Guenter
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
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 "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? Krzysztof Krzysztof
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
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