| Message ID | 12984255aac11a3edfc0e6278e1a1cac70ce97ec.1631021349.git.krzysztof.adamski@nokia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add per channel properies support in tmp421 | expand |
On 9/7/21 6:46 AM, Krzysztof Adamski wrote: > Add binding description for the per temperature channel configuration > like labels and n-factor. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> Up to Rob to decide, but it seems to me that can be squashed with the other dt patch in the series (which on its own doesn't really add much value). Guenter > --- > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > index 53940e146ee6..56085fdf1b57 100644 > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > @@ -24,12 +24,49 @@ properties: > reg: > maxItems: 1 > > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > required: > - compatible > - reg > > additionalProperties: false > > +patternProperties: > + "^input@([0-4])$": > + type: object > + description: | > + Represents channels of the device and their specific configuration. > + > + properties: > + reg: > + description: | > + The channel number. 0 is local channel, 1-4 are remote channels > + items: > + minimum: 0 > + maximum: 4 > + > + label: > + description: | > + A descriptive name for this channel, like "ambient" or "psu". > + > + n-factor: > + description: | > + The value (two's complement) to be programmed in the channel specific N correction register. > + For remote channels only. > + items: > + minimum: 0 > + maximum: 1 > + > + required: > + - reg > + > + additionalProperties: false > + > examples: > - | > i2c { > @@ -41,3 +78,32 @@ examples: > reg = <0x4c>; > }; > }; > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + sensor@4c { > + compatible = "ti,tmp422"; > + reg = <0x4c>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + input@0 { > + reg = <0x0>; > + n-factor = <0x1>; > + label = "local"; > + }; > + > + input@1 { > + reg = <0x1>; > + n-factor = <0x0>; > + label = "somelabel"; > + }; > + > + input@2 { > + reg = <0x2>; > + status = "disabled"; > + }; > + }; > + }; >
Dnia Tue, Sep 07, 2021 at 08:46:53AM -0700, Guenter Roeck napisał(a): >On 9/7/21 6:46 AM, Krzysztof Adamski wrote: >>Add binding description for the per temperature channel configuration >>like labels and n-factor. >> >>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > >Up to Rob to decide, but it seems to me that can be squashed with the other >dt patch in the series (which on its own doesn't really add much value). > My intention was, I guess, to clearly differentiate between what is the existing state of bindings and what I propose as an addition, so that it is not required to read the rest of the patches to review my proposal from DT perspective. I can squash the two patches, though, so let me know about the decision, Rob. Krzysztof
On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote: > Add binding description for the per temperature channel configuration > like labels and n-factor. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) I'd keep this separate... > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > index 53940e146ee6..56085fdf1b57 100644 > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > @@ -24,12 +24,49 @@ properties: > reg: > maxItems: 1 > > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > required: > - compatible > - reg > > additionalProperties: false > > +patternProperties: > + "^input@([0-4])$": > + type: object > + description: | > + Represents channels of the device and their specific configuration. > + > + properties: > + reg: > + description: | > + The channel number. 0 is local channel, 1-4 are remote channels > + items: > + minimum: 0 > + maximum: 4 > + > + label: > + description: | > + A descriptive name for this channel, like "ambient" or "psu". > + > + n-factor: ti,n-factor Needs a type reference too. > + description: | > + The value (two's complement) to be programmed in the channel specific N correction register. > + For remote channels only. > + items: > + minimum: 0 > + maximum: 1 > + > + required: > + - reg > + > + additionalProperties: false > + > examples: > - | > i2c { > @@ -41,3 +78,32 @@ examples: > reg = <0x4c>; > }; > }; > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + sensor@4c { > + compatible = "ti,tmp422"; > + reg = <0x4c>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + input@0 { > + reg = <0x0>; > + n-factor = <0x1>; > + label = "local"; > + }; > + > + input@1 { > + reg = <0x1>; > + n-factor = <0x0>; > + label = "somelabel"; > + }; > + > + input@2 { > + reg = <0x2>; > + status = "disabled"; > + }; > + }; > + }; > -- > 2.31.1 > >
On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote: > On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote: > > Add binding description for the per temperature channel configuration > > like labels and n-factor. > > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > > --- > > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > I'd keep this separate... > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > index 53940e146ee6..56085fdf1b57 100644 > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > @@ -24,12 +24,49 @@ properties: > > reg: > > maxItems: 1 > > > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > required: > > - compatible > > - reg > > > > additionalProperties: false > > > > +patternProperties: > > + "^input@([0-4])$": > > + type: object > > + description: | > > + Represents channels of the device and their specific configuration. > > + > > + properties: > > + reg: > > + description: | > > + The channel number. 0 is local channel, 1-4 are remote channels > > + items: > > + minimum: 0 > > + maximum: 4 > > + > > + label: > > + description: | > > + A descriptive name for this channel, like "ambient" or "psu". > > + > > + n-factor: > > ti,n-factor n-factor isn't just supported by TI sensors, though it isn't always called n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they also refer to the factor as "N" in the datasheet. So question is if we make this ti,n-factor and maxim,n-factor, or if we make it generic and define some kind of generic units. Thoughts ? My personal preference would be a generic definition, but is not a strong preference. In regard to units, the n-factor is, as the name says, a factor. Default value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 it is 0.706542 to 1.747977. So the scondary question is if the value written should be the register value (as proposed here) or the absolute factor (eg in micro-units). > > Needs a type reference too. > > > + description: | > > + The value (two's complement) to be programmed in the channel specific N correction register. > > + For remote channels only. > > + items: > > + minimum: 0 > > + maximum: 1 > > + > > + required: > > + - reg > > + > > + additionalProperties: false > > + > > examples: > > - | > > i2c { > > @@ -41,3 +78,32 @@ examples: > > reg = <0x4c>; > > }; > > }; > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sensor@4c { > > + compatible = "ti,tmp422"; > > + reg = <0x4c>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + input@0 { > > + reg = <0x0>; > > + n-factor = <0x1>; > > + label = "local"; > > + }; In the context or other sensors, question here is if we can make the bindings generic. We have been discussing this for NCT7802Y. The main question for me is how to handle different sensor types. TMP421 is easy because it only has one type of sensors, but there are other devices which also have, for example, voltage and/or current sensors. NCT7802 is an example for that. We just had a set of bindings for that chip proposed at https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/ Would it be possible to determine a generic scheme that works for all chips ? I can see two problems: - How to express sensor types. The NCT7802 submission proposes another level of indirection, ie temperature-sensors { > > + > > + input@1 { > > + reg = <0x1>; > > + n-factor = <0x0>; > > + label = "somelabel"; > > + }; > > + > > + input@2 { > > + reg = <0x2>; > > + status = "disabled"; > > + }; > > + }; > > + }; }; The second question is how to express sensor index. One option is the solution suggested here, ie to use reg=<> as sensor index. The second is the solution suggested in the 7802 bindings, where the (chip specific) name is used as sensor index. + temperature-sensors { + ltd { + status = "disabled"; + }; + + rtd1 { + status = "okay"; + type = <4> /* thermistor */; + }; + }; I personally don't have a strong opinion either way, but I would like to see a single solution for all sensor chips. Rob, do you have a preference ? Thanks, Guenter
On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote: > > On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote: > > > Add binding description for the per temperature channel configuration > > > like labels and n-factor. > > > > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > > > --- > > > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > > > 1 file changed, 66 insertions(+) > > > > I'd keep this separate... > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > index 53940e146ee6..56085fdf1b57 100644 > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > @@ -24,12 +24,49 @@ properties: > > > reg: > > > maxItems: 1 > > > > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > required: > > > - compatible > > > - reg > > > > > > additionalProperties: false > > > > > > +patternProperties: > > > + "^input@([0-4])$": > > > + type: object > > > + description: | > > > + Represents channels of the device and their specific configuration. > > > + > > > + properties: > > > + reg: > > > + description: | > > > + The channel number. 0 is local channel, 1-4 are remote channels > > > + items: > > > + minimum: 0 > > > + maximum: 4 > > > + > > > + label: > > > + description: | > > > + A descriptive name for this channel, like "ambient" or "psu". > > > + > > > + n-factor: > > > > ti,n-factor > > n-factor isn't just supported by TI sensors, though it isn't always called > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they > also refer to the factor as "N" in the datasheet. > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make > it generic and define some kind of generic units. Thoughts ? My personal > preference would be a generic definition, but is not a strong preference. generic if the units are generic. Though if the register value is opaque to s/w, then maybe register value is fine. > In regard to units, the n-factor is, as the name says, a factor. Default > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 > it is 0.706542 to 1.747977. So the scondary question is if the value > written should be the register value (as proposed here) or the absolute > factor (eg in micro-units). A range, but the register value can only be 0 or 1? > > > > > Needs a type reference too. > > > > > + description: | > > > + The value (two's complement) to be programmed in the channel specific N correction register. > > > + For remote channels only. > > > + items: > > > + minimum: 0 > > > + maximum: 1 > > > + > > > + required: > > > + - reg > > > + > > > + additionalProperties: false > > > + > > > examples: > > > - | > > > i2c { > > > @@ -41,3 +78,32 @@ examples: > > > reg = <0x4c>; > > > }; > > > }; > > > + - | > > > + i2c { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + sensor@4c { > > > + compatible = "ti,tmp422"; > > > + reg = <0x4c>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + input@0 { > > > + reg = <0x0>; > > > + n-factor = <0x1>; > > > + label = "local"; > > > + }; > > In the context or other sensors, question here is if we can make the > bindings generic. We have been discussing this for NCT7802Y. The main > question for me is how to handle different sensor types. TMP421 is > easy because it only has one type of sensors, but there are other > devices which also have, for example, voltage and/or current sensors. > NCT7802 is an example for that. We just had a set of bindings for that > chip proposed at > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/ > > Would it be possible to determine a generic scheme that works for all > chips ? I can see two problems: > - How to express sensor types. The NCT7802 submission proposes another level > of indirection, ie > > temperature-sensors { > > > + > > > + input@1 { > > > + reg = <0x1>; > > > + n-factor = <0x0>; > > > + label = "somelabel"; > > > + }; > > > + > > > + input@2 { > > > + reg = <0x2>; > > > + status = "disabled"; > > > + }; > > > + }; > > > + }; > }; I think the function should be within the node. Otherwise, the addressing becomes weird (e.g. input@3 is under current-sensors or something) with seemingly separate address spaces. > The second question is how to express sensor index. One option is the solution > suggested here, ie to use reg=<> as sensor index. The second is the solution > suggested in the 7802 bindings, where the (chip specific) name is used as > sensor index. > > + temperature-sensors { > + ltd { > + status = "disabled"; > + }; > + > + rtd1 { > + status = "okay"; > + type = <4> /* thermistor */; 'type' is a bit generic. We don't want the same property name to possibly have multiple definitions. > + }; > + }; > > I personally don't have a strong opinion either way, but I would like to see > a single solution for all sensor chips. > > Rob, do you have a preference ? If it is how you address an instance of something which seems to be the case here, then 'reg' should be used. Rob
On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote: > On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote: > > > On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote: > > > > Add binding description for the per temperature channel configuration > > > > like labels and n-factor. > > > > > > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > > > > --- > > > > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > > > > 1 file changed, 66 insertions(+) > > > > > > I'd keep this separate... > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > index 53940e146ee6..56085fdf1b57 100644 > > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > @@ -24,12 +24,49 @@ properties: > > > > reg: > > > > maxItems: 1 > > > > > > > > + '#address-cells': > > > > + const: 1 > > > > + > > > > + '#size-cells': > > > > + const: 0 > > > > + > > > > required: > > > > - compatible > > > > - reg > > > > > > > > additionalProperties: false > > > > > > > > +patternProperties: > > > > + "^input@([0-4])$": > > > > + type: object > > > > + description: | > > > > + Represents channels of the device and their specific configuration. > > > > + > > > > + properties: > > > > + reg: > > > > + description: | > > > > + The channel number. 0 is local channel, 1-4 are remote channels > > > > + items: > > > > + minimum: 0 > > > > + maximum: 4 > > > > + > > > > + label: > > > > + description: | > > > > + A descriptive name for this channel, like "ambient" or "psu". > > > > + > > > > + n-factor: > > > > > > ti,n-factor > > > > n-factor isn't just supported by TI sensors, though it isn't always called > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they > > also refer to the factor as "N" in the datasheet. > > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make > > it generic and define some kind of generic units. Thoughts ? My personal > > preference would be a generic definition, but is not a strong preference. > > generic if the units are generic. Though if the register value is > opaque to s/w, then maybe register value is fine. > > > In regard to units, the n-factor is, as the name says, a factor. Default > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 > > it is 0.706542 to 1.747977. So the scondary question is if the value > > written should be the register value (as proposed here) or the absolute > > factor (eg in micro-units). > > A range, but the register value can only be 0 or 1? > No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421. > > > > > > > > Needs a type reference too. > > > > > > > + description: | > > > > + The value (two's complement) to be programmed in the channel specific N correction register. > > > > + For remote channels only. > > > > + items: > > > > + minimum: 0 > > > > + maximum: 1 > > > > + > > > > + required: > > > > + - reg > > > > + > > > > + additionalProperties: false > > > > + > > > > examples: > > > > - | > > > > i2c { > > > > @@ -41,3 +78,32 @@ examples: > > > > reg = <0x4c>; > > > > }; > > > > }; > > > > + - | > > > > + i2c { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + sensor@4c { > > > > + compatible = "ti,tmp422"; > > > > + reg = <0x4c>; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + input@0 { > > > > + reg = <0x0>; > > > > + n-factor = <0x1>; > > > > + label = "local"; > > > > + }; > > > > In the context or other sensors, question here is if we can make the > > bindings generic. We have been discussing this for NCT7802Y. The main > > question for me is how to handle different sensor types. TMP421 is > > easy because it only has one type of sensors, but there are other > > devices which also have, for example, voltage and/or current sensors. > > NCT7802 is an example for that. We just had a set of bindings for that > > chip proposed at > > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/ > > > > Would it be possible to determine a generic scheme that works for all > > chips ? I can see two problems: > > - How to express sensor types. The NCT7802 submission proposes another level > > of indirection, ie > > > > temperature-sensors { > > > > + > > > > + input@1 { > > > > + reg = <0x1>; > > > > + n-factor = <0x0>; > > > > + label = "somelabel"; > > > > + }; > > > > + > > > > + input@2 { > > > > + reg = <0x2>; > > > > + status = "disabled"; > > > > + }; > > > > + }; > > > > + }; > > }; > > I think the function should be within the node. Otherwise, the > addressing becomes weird (e.g. input@3 is under current-sensors or > something) with seemingly separate address spaces. > Sorry, can you translate that for a DT non-expert ? Or, in other words, how would / should one express a chip with sets of, say, current-sensors, voltage sensors, and temperature sensors. Each would have a different number of channels and different parameters. > > The second question is how to express sensor index. One option is the solution > > suggested here, ie to use reg=<> as sensor index. The second is the solution > > suggested in the 7802 bindings, where the (chip specific) name is used as > > sensor index. > > > > + temperature-sensors { > > + ltd { > > + status = "disabled"; > > + }; > > + > > + rtd1 { > > + status = "okay"; > > + type = <4> /* thermistor */; > > 'type' is a bit generic. We don't want the same property name to > possibly have multiple definitions. > How about sensor-type ? > > + }; > > + }; > > > > I personally don't have a strong opinion either way, but I would like to see > > a single solution for all sensor chips. > > > > Rob, do you have a preference ? > > If it is how you address an instance of something which seems to be > the case here, then 'reg' should be used. > Ok. Thanks, Guenter
Hi > > > + temperature-sensors { > > > + ltd { > > > + status = "disabled"; > > > + }; > > > + > > > + rtd1 { > > > + status = "okay"; > > > + type = <4> /* thermistor */; > > > > 'type' is a bit generic. We don't want the same property name to > > possibly have multiple definitions. > > > How about sensor-type ? In the datasheet this is called "mode". I called it "type" since it's tempX_type in sysfs and I wanted to stay in sync with that. > > If it is how you address an instance of something which seems to be > > the case here, then 'reg' should be used. > > The reason I didn't do that is because you'd effectively have to duplicate the ID in both the name (e.g. sensor@1) and the reg property (e.g. reg = <1>). But maybe that's just what is is in device tree, I can live with that. However, we'd also have to find out whether the "local" sensor ("LTD") would simply be #4, as it is in sysfs today. I'd also be ok with that as it would keep sysfs and device tree "in sync" wrt. naming. Thanks Oskar.
Resend to Rob's correct email address. I'm sorry. > > > + temperature-sensors { > > > + ltd { > > > + status = "disabled"; > > > + }; > > > + > > > + rtd1 { > > > + status = "okay"; > > > + type = <4> /* thermistor */; > > > > 'type' is a bit generic. We don't want the same property name to > > possibly have multiple definitions. > > > How about sensor-type ? In the datasheet this is called "mode". I called it "type" since it's tempX_type in sysfs and I wanted to stay in sync with that. > > If it is how you address an instance of something which seems to be > > the case here, then 'reg' should be used. > > The reason I didn't do that is because you'd effectively have to duplicate the ID in both the name (e.g. sensor@1) and the reg property (e.g. reg = <1>). But maybe that's just what is is in device tree, I can live with that. However, we'd also have to find out whether the "local" sensor ("LTD") would simply be #4, as it is in sysfs today. I'd also be ok with that as it would keep sysfs and device tree "in sync" wrt. naming. Thanks Oskar.
Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a): >> >> ti,n-factor > >n-factor isn't just supported by TI sensors, though it isn't always called >n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they >also refer to the factor as "N" in the datasheet. > >So question is if we make this ti,n-factor and maxim,n-factor, or if we make >it generic and define some kind of generic units. Thoughts ? My personal >preference would be a generic definition, but is not a strong preference. That was exactly my way of thinking here - many sensors have n-factor parameter and this is the name I see most often. That being said, maybe it should be "nfactor" instead of "n-factor", as this is what tmp513 is using? >In regard to units, the n-factor is, as the name says, a factor. Default >value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 >it is 0.706542 to 1.747977. So the scondary question is if the value >written should be the register value (as proposed here) or the absolute >factor (eg in micro-units). Since expressing the fractional values in DT isn't well supported and (at least here) hardware guys like to think in terms of register values so this is what I proposed. Also, I just noticed that, for example, TMP531 is using register values as well. >> > + i2c { >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + >> > + sensor@4c { >> > + compatible = "ti,tmp422"; >> > + reg = <0x4c>; >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + >> > + input@0 { >> > + reg = <0x0>; >> > + n-factor = <0x1>; >> > + label = "local"; >> > + }; > >In the context or other sensors, question here is if we can make the >bindings generic. We have been discussing this for NCT7802Y. The main >question for me is how to handle different sensor types. TMP421 is >easy because it only has one type of sensors, but there are other >devices which also have, for example, voltage and/or current sensors. >NCT7802 is an example for that. We just had a set of bindings for that >chip proposed at >https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/ > >Would it be possible to determine a generic scheme that works for all >chips ? Just wanted to note that the bindings I propse were not completely made up by me. I based them on existing ina3221 bindings so I feel like my proposition is at least in line with that one. > I can see two problems: >- How to express sensor types. The NCT7802 submission proposes another level > of indirection, ie > > temperature-sensors { >> > + >> > + input@1 { >> > + reg = <0x1>; >> > + n-factor = <0x0>; >> > + label = "somelabel"; >> > + }; >> > + >> > + input@2 { >> > + reg = <0x2>; >> > + status = "disabled"; >> > + }; >> > + }; >> > + }; > }; > >The second question is how to express sensor index. One option is the solution >suggested here, ie to use reg=<> as sensor index. The second is the solution >suggested in the 7802 bindings, where the (chip specific) name is used as >sensor index. > >+ temperature-sensors { >+ ltd { >+ status = "disabled"; >+ }; >+ >+ rtd1 { >+ status = "okay"; >+ type = <4> /* thermistor */; >+ }; >+ }; > >I personally don't have a strong opinion either way, but I would like to see >a single solution for all sensor chips. For me, the problem with using this style is that it is sometimes hard to come up with good names, especially in simple devices where all you have are channels.. which BTW may be called differently as well. So we would end up having some device with channel0, channel1, etc, and others might have input0, input1, etc. This argument goes the other way around as well - some devies have no way to easily enumerate their "subdevices" it would be hard to assing proper numbers to them. For this reason I think it would make sense to actually use both schemes - reg for cases where the enumeration makes sesne (when we have channels, inputs, etc) - names otherwise, when there is no natual way to enumerate the devices by an index.
On Wed, Sep 22, 2021 at 09:22:33AM +0200, Krzysztof Adamski wrote: > Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a): > > > > > > ti,n-factor > > > > n-factor isn't just supported by TI sensors, though it isn't always called > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they > > also refer to the factor as "N" in the datasheet. > > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make > > it generic and define some kind of generic units. Thoughts ? My personal > > preference would be a generic definition, but is not a strong preference. > > That was exactly my way of thinking here - many sensors have n-factor > parameter and this is the name I see most often. > > That being said, maybe it should be "nfactor" instead of "n-factor", as > this is what tmp513 is using? > > > In regard to units, the n-factor is, as the name says, a factor. Default > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 > > it is 0.706542 to 1.747977. So the scondary question is if the value > > written should be the register value (as proposed here) or the absolute > > factor (eg in micro-units). > > Since expressing the fractional values in DT isn't well supported and > (at least here) hardware guys like to think in terms of register values > so this is what I proposed. Also, I just noticed that, for example, > TMP531 is using register values as well. > I never see "someone else does that" as valid argument. Also, DT does support fractional values, via units. It is perfectly valid to describe a voltage as micro-volt, for example. If the agreement is to use raw register values, I think the property name should be prefixed with the vendor name, since it won't be a standard property. I'll defer on Rob for that, though. Thanks, Guenter
Dnia Wed, Sep 22, 2021 at 05:39:26AM -0700, Guenter Roeck napisał(a): >On Wed, Sep 22, 2021 at 09:22:33AM +0200, Krzysztof Adamski wrote: >> Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a): >> > > >> > > ti,n-factor >> > >> > n-factor isn't just supported by TI sensors, though it isn't always called >> > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they >> > also refer to the factor as "N" in the datasheet. >> > >> > So question is if we make this ti,n-factor and maxim,n-factor, or if we make >> > it generic and define some kind of generic units. Thoughts ? My personal >> > preference would be a generic definition, but is not a strong preference. >> >> That was exactly my way of thinking here - many sensors have n-factor >> parameter and this is the name I see most often. >> >> That being said, maybe it should be "nfactor" instead of "n-factor", as >> this is what tmp513 is using? >> >> > In regard to units, the n-factor is, as the name says, a factor. Default >> > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 >> > it is 0.706542 to 1.747977. So the scondary question is if the value >> > written should be the register value (as proposed here) or the absolute >> > factor (eg in micro-units). >> >> Since expressing the fractional values in DT isn't well supported and >> (at least here) hardware guys like to think in terms of register values >> so this is what I proposed. Also, I just noticed that, for example, >> TMP531 is using register values as well. >> > >I never see "someone else does that" as valid argument. It is not an argument for "so I should be allowed too" but more like "so it is generic enough to make sense for more than a single case" :) > Also, DT does support fractional values, via units. It is perfectly > valid to describe a voltage as micro-volt, for example. True. But doing so for unit-less values isn't as obvious. For real fractions we don't even know what the resolution should be and then we also may have those rounding errors etc (while with register values we know precisely what we get). As usual, we have some pros and cons of both approaches. While I agree raw values are not perfect, I still think it makes more sense so I vote for them. But my vote, obviously, isn't that important here so I'll let you guys decide. >If the agreement is to use raw register values, I think the property name >should be prefixed with the vendor name, since it won't be a standard >property. I'll defer on Rob for that, though. Fair enough. If we go that route, we should use "ti,nfactor" (without dash) to be consistent with ti513? Krzysztof
On Wed, Sep 22, 2021 at 08:32:00PM +0200, Krzysztof Adamski wrote: > Dnia Wed, Sep 22, 2021 at 05:39:26AM -0700, Guenter Roeck napisał(a): > > On Wed, Sep 22, 2021 at 09:22:33AM +0200, Krzysztof Adamski wrote: > > > Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a): > > > > > > > > > > ti,n-factor > > > > > > > > n-factor isn't just supported by TI sensors, though it isn't always called > > > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they > > > > also refer to the factor as "N" in the datasheet. > > > > > > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make > > > > it generic and define some kind of generic units. Thoughts ? My personal > > > > preference would be a generic definition, but is not a strong preference. > > > > > > That was exactly my way of thinking here - many sensors have n-factor > > > parameter and this is the name I see most often. > > > > > > That being said, maybe it should be "nfactor" instead of "n-factor", as > > > this is what tmp513 is using? > > > > > > > In regard to units, the n-factor is, as the name says, a factor. Default > > > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 > > > > it is 0.706542 to 1.747977. So the scondary question is if the value > > > > written should be the register value (as proposed here) or the absolute > > > > factor (eg in micro-units). > > > > > > Since expressing the fractional values in DT isn't well supported and > > > (at least here) hardware guys like to think in terms of register values > > > so this is what I proposed. Also, I just noticed that, for example, > > > TMP531 is using register values as well. > > > > > > > I never see "someone else does that" as valid argument. > > It is not an argument for "so I should be allowed too" but more like "so > it is generic enough to make sense for more than a single case" :) > > > Also, DT does support fractional values, via units. It is perfectly > > valid to describe a voltage as micro-volt, for example. > > True. But doing so for unit-less values isn't as obvious. For real > fractions we don't even know what the resolution should be and then we > also may have those rounding errors etc (while with register values we > know precisely what we get). As usual, we have some pros and > cons of both approaches. While I agree raw values are not perfect, I > still think it makes more sense so I vote for them. But my vote, > obviously, isn't that important here so I'll let you guys decide. > I really have to pass on this one, and leave it up to Rob to decide. Personally I really really really dislike raw values, but I understand that this makes me biased. I do realize that converting from a fractional value to a register value is inherently complex and open to interpretation. For example. if we define fractional values, what should 1.007000 translate to ? It would either be 1.008 or 1.004641. Using the register value (0xff, or -1 for 1.004641) would definitely be simpler and avoid calculations and rounding. Guenter > > If the agreement is to use raw register values, I think the property name > > should be prefixed with the vendor name, since it won't be a standard > > property. I'll defer on Rob for that, though. > > Fair enough. If we go that route, we should use "ti,nfactor" (without > dash) to be consistent with ti513? > > Krzysztof
On Tue, Sep 21, 2021 at 3:52 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote: > > On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote: > > > > On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote: > > > > > Add binding description for the per temperature channel configuration > > > > > like labels and n-factor. > > > > > > > > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > > > > > --- > > > > > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > > > > > 1 file changed, 66 insertions(+) > > > > > > > > I'd keep this separate... > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > > index 53940e146ee6..56085fdf1b57 100644 > > > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > > @@ -24,12 +24,49 @@ properties: > > > > > reg: > > > > > maxItems: 1 > > > > > > > > > > + '#address-cells': > > > > > + const: 1 > > > > > + > > > > > + '#size-cells': > > > > > + const: 0 > > > > > + > > > > > required: > > > > > - compatible > > > > > - reg > > > > > > > > > > additionalProperties: false > > > > > > > > > > +patternProperties: > > > > > + "^input@([0-4])$": > > > > > + type: object > > > > > + description: | > > > > > + Represents channels of the device and their specific configuration. > > > > > + > > > > > + properties: > > > > > + reg: > > > > > + description: | > > > > > + The channel number. 0 is local channel, 1-4 are remote channels > > > > > + items: > > > > > + minimum: 0 > > > > > + maximum: 4 > > > > > + > > > > > + label: > > > > > + description: | > > > > > + A descriptive name for this channel, like "ambient" or "psu". > > > > > + > > > > > + n-factor: > > > > > > > > ti,n-factor > > > > > > n-factor isn't just supported by TI sensors, though it isn't always called > > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they > > > also refer to the factor as "N" in the datasheet. > > > > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make > > > it generic and define some kind of generic units. Thoughts ? My personal > > > preference would be a generic definition, but is not a strong preference. > > > > generic if the units are generic. Though if the register value is > > opaque to s/w, then maybe register value is fine. > > > > > In regard to units, the n-factor is, as the name says, a factor. Default > > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 > > > it is 0.706542 to 1.747977. So the scondary question is if the value > > > written should be the register value (as proposed here) or the absolute > > > factor (eg in micro-units). > > > > A range, but the register value can only be 0 or 1? > > > No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421. Okay, then the schema below is wrong. > > > > Needs a type reference too. > > > > > > > > > + description: | > > > > > + The value (two's complement) to be programmed in the channel specific N correction register. > > > > > + For remote channels only. > > > > > + items: > > > > > + minimum: 0 > > > > > + maximum: 1 > > > > > + > > > > > + required: > > > > > + - reg > > > > > + > > > > > + additionalProperties: false > > > > > + > > > > > examples: > > > > > - | > > > > > i2c { > > > > > @@ -41,3 +78,32 @@ examples: > > > > > reg = <0x4c>; > > > > > }; > > > > > }; > > > > > + - | > > > > > + i2c { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + sensor@4c { > > > > > + compatible = "ti,tmp422"; > > > > > + reg = <0x4c>; > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + input@0 { > > > > > + reg = <0x0>; > > > > > + n-factor = <0x1>; > > > > > + label = "local"; > > > > > + }; > > > > > > In the context or other sensors, question here is if we can make the > > > bindings generic. We have been discussing this for NCT7802Y. The main > > > question for me is how to handle different sensor types. TMP421 is > > > easy because it only has one type of sensors, but there are other > > > devices which also have, for example, voltage and/or current sensors. > > > NCT7802 is an example for that. We just had a set of bindings for that > > > chip proposed at > > > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/ > > > > > > Would it be possible to determine a generic scheme that works for all > > > chips ? I can see two problems: > > > - How to express sensor types. The NCT7802 submission proposes another level > > > of indirection, ie > > > > > > temperature-sensors { > > > > > + > > > > > + input@1 { > > > > > + reg = <0x1>; > > > > > + n-factor = <0x0>; > > > > > + label = "somelabel"; > > > > > + }; > > > > > + > > > > > + input@2 { > > > > > + reg = <0x2>; > > > > > + status = "disabled"; > > > > > + }; > > > > > + }; > > > > > + }; > > > }; > > > > I think the function should be within the node. Otherwise, the > > addressing becomes weird (e.g. input@3 is under current-sensors or > > something) with seemingly separate address spaces. > > > > Sorry, can you translate that for a DT non-expert ? Or, in other words, > how would / should one express a chip with sets of, say, current-sensors, > voltage sensors, and temperature sensors. Each would have a different > number of channels and different parameters. If each kind of sensor is a different number space (e.g. 0-2), then how you have it with 2 levels of nodes is appropriate. If you only have one set of channel or input numbers, then they should all have the same parent node. So is it current sensors 0,1,2 and temperature sensors 0,1,2, or just input channels 0,1,2,3,4,5? > > > The second question is how to express sensor index. One option is the solution > > > suggested here, ie to use reg=<> as sensor index. The second is the solution > > > suggested in the 7802 bindings, where the (chip specific) name is used as > > > sensor index. > > > > > > + temperature-sensors { > > > + ltd { > > > + status = "disabled"; > > > + }; > > > + > > > + rtd1 { > > > + status = "okay"; > > > + type = <4> /* thermistor */; > > > > 'type' is a bit generic. We don't want the same property name to > > possibly have multiple definitions. > > > How about sensor-type ? Sure. And you are going to define a common set of type numbers? Rob
On Thu, Sep 23, 2021 at 10:30:59AM -0500, Rob Herring wrote: > On Tue, Sep 21, 2021 at 3:52 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote: > > > On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote: > > > > > On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote: > > > > > > Add binding description for the per temperature channel configuration > > > > > > like labels and n-factor. > > > > > > > > > > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > > > > > > --- > > > > > > .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ > > > > > > 1 file changed, 66 insertions(+) > > > > > > > > > > I'd keep this separate... > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > > > index 53940e146ee6..56085fdf1b57 100644 > > > > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > > > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml > > > > > > @@ -24,12 +24,49 @@ properties: > > > > > > reg: > > > > > > maxItems: 1 > > > > > > > > > > > > + '#address-cells': > > > > > > + const: 1 > > > > > > + > > > > > > + '#size-cells': > > > > > > + const: 0 > > > > > > + > > > > > > required: > > > > > > - compatible > > > > > > - reg > > > > > > > > > > > > additionalProperties: false > > > > > > > > > > > > +patternProperties: > > > > > > + "^input@([0-4])$": > > > > > > + type: object > > > > > > + description: | > > > > > > + Represents channels of the device and their specific configuration. > > > > > > + > > > > > > + properties: > > > > > > + reg: > > > > > > + description: | > > > > > > + The channel number. 0 is local channel, 1-4 are remote channels > > > > > > + items: > > > > > > + minimum: 0 > > > > > > + maximum: 4 > > > > > > + > > > > > > + label: > > > > > > + description: | > > > > > > + A descriptive name for this channel, like "ambient" or "psu". > > > > > > + > > > > > > + n-factor: > > > > > > > > > > ti,n-factor > > > > > > > > n-factor isn't just supported by TI sensors, though it isn't always called > > > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they > > > > also refer to the factor as "N" in the datasheet. > > > > > > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make > > > > it generic and define some kind of generic units. Thoughts ? My personal > > > > preference would be a generic definition, but is not a strong preference. > > > > > > generic if the units are generic. Though if the register value is > > > opaque to s/w, then maybe register value is fine. > > > > > > > In regard to units, the n-factor is, as the name says, a factor. Default > > > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421 > > > > it is 0.706542 to 1.747977. So the scondary question is if the value > > > > written should be the register value (as proposed here) or the absolute > > > > factor (eg in micro-units). > > > > > > A range, but the register value can only be 0 or 1? > > > > > No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421. > > Okay, then the schema below is wrong. > > > > > > Needs a type reference too. > > > > > > > > > > > + description: | > > > > > > + The value (two's complement) to be programmed in the channel specific N correction register. > > > > > > + For remote channels only. > > > > > > + items: > > > > > > + minimum: 0 > > > > > > + maximum: 1 > > > > > > + > > > > > > + required: > > > > > > + - reg > > > > > > + > > > > > > + additionalProperties: false > > > > > > + > > > > > > examples: > > > > > > - | > > > > > > i2c { > > > > > > @@ -41,3 +78,32 @@ examples: > > > > > > reg = <0x4c>; > > > > > > }; > > > > > > }; > > > > > > + - | > > > > > > + i2c { > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + sensor@4c { > > > > > > + compatible = "ti,tmp422"; > > > > > > + reg = <0x4c>; > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + input@0 { > > > > > > + reg = <0x0>; > > > > > > + n-factor = <0x1>; > > > > > > + label = "local"; > > > > > > + }; > > > > > > > > In the context or other sensors, question here is if we can make the > > > > bindings generic. We have been discussing this for NCT7802Y. The main > > > > question for me is how to handle different sensor types. TMP421 is > > > > easy because it only has one type of sensors, but there are other > > > > devices which also have, for example, voltage and/or current sensors. > > > > NCT7802 is an example for that. We just had a set of bindings for that > > > > chip proposed at > > > > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/ > > > > > > > > Would it be possible to determine a generic scheme that works for all > > > > chips ? I can see two problems: > > > > - How to express sensor types. The NCT7802 submission proposes another level > > > > of indirection, ie > > > > > > > > temperature-sensors { > > > > > > + > > > > > > + input@1 { > > > > > > + reg = <0x1>; > > > > > > + n-factor = <0x0>; > > > > > > + label = "somelabel"; > > > > > > + }; > > > > > > + > > > > > > + input@2 { > > > > > > + reg = <0x2>; > > > > > > + status = "disabled"; > > > > > > + }; > > > > > > + }; > > > > > > + }; > > > > }; > > > > > > I think the function should be within the node. Otherwise, the > > > addressing becomes weird (e.g. input@3 is under current-sensors or > > > something) with seemingly separate address spaces. > > > > > > > Sorry, can you translate that for a DT non-expert ? Or, in other words, > > how would / should one express a chip with sets of, say, current-sensors, > > voltage sensors, and temperature sensors. Each would have a different > > number of channels and different parameters. > > If each kind of sensor is a different number space (e.g. 0-2), then > how you have it with 2 levels of nodes is appropriate. If you only > have one set of channel or input numbers, then they should all have > the same parent node. So is it current sensors 0,1,2 and temperature > sensors 0,1,2, or just input channels 0,1,2,3,4,5? > Each sensor type has its own number space. > > > > The second question is how to express sensor index. One option is the solution > > > > suggested here, ie to use reg=<> as sensor index. The second is the solution > > > > suggested in the 7802 bindings, where the (chip specific) name is used as > > > > sensor index. > > > > > > > > + temperature-sensors { > > > > + ltd { > > > > + status = "disabled"; > > > > + }; > > > > + > > > > + rtd1 { > > > > + status = "okay"; > > > > + type = <4> /* thermistor */; > > > > > > 'type' is a bit generic. We don't want the same property name to > > > possibly have multiple definitions. > > > > > How about sensor-type ? > > Sure. And you are going to define a common set of type numbers? > I guess we would have to. Guenter
Dnia Thu, Sep 23, 2021 at 05:29:51PM -0700, Guenter Roeck napisał(a): >> If each kind of sensor is a different number space (e.g. 0-2), then >> how you have it with 2 levels of nodes is appropriate. If you only >> have one set of channel or input numbers, then they should all have >> the same parent node. So is it current sensors 0,1,2 and temperature >> sensors 0,1,2, or just input channels 0,1,2,3,4,5? >> > >Each sensor type has its own number space. > But many sensors will have only one type of channels - like several temperature sensors and nothing else. Like several temperature channels on a temperature sensor, or several fans on a fan controller. In such cases, we already define them with 1-level structure, like: - npcm750-pwm-fan - aspeed-pwm-tacho - ina3221 In many cases the channels are "shared" - we have 3 voltage, 3 current and 3 power sensors but in fact they are not separate sensors but 3 channels each able to measure 3 different things and they may share some common properties in each channel (so current, voltage and power may be calculated bases on the same shunt resistor or correction factor). An example being adi,ltc2992. In those cases it doesn't make sense to have two levels as how would you describe the shared parent? Call it generic "channels"? So maybe it makes sense to have 2 levels for complex devices that can measure several independent entities or for devices which do not have a clear concept of enumerated "channels" or "inputs", but we could skip it for most others? After all, what is the benefit of having this additional level if all we have is something like: temperature-sensors { temperature1 { }; temperature2 { }; temperature3 { }; }; For most devices having an "index" or "reg" makes much more sense so: temperature@1, or channel@1 feels like a more natural way to describe them. In any case, I'm quite confused right now on what would be the conclusion of this discussion. How would you like the DT for TMP421 to look like, after all? As a side note, should the description of the tmp421 bindings be in tmp421.yaml file (as it is in current patchset), or should it be actually called "ti,tmp421.yaml"? Krzysztof
On Fri, Sep 24, 2021 at 09:53:16AM +0200, Krzysztof Adamski wrote: > Dnia Thu, Sep 23, 2021 at 05:29:51PM -0700, Guenter Roeck napisał(a): > > > If each kind of sensor is a different number space (e.g. 0-2), then > > > how you have it with 2 levels of nodes is appropriate. If you only > > > have one set of channel or input numbers, then they should all have > > > the same parent node. So is it current sensors 0,1,2 and temperature > > > sensors 0,1,2, or just input channels 0,1,2,3,4,5? > > > > > > > Each sensor type has its own number space. > > > > But many sensors will have only one type of channels - like several > temperature sensors and nothing else. Like several temperature channels > on a temperature sensor, or several fans on a fan controller. > > In such cases, we already define them with 1-level structure, like: > - npcm750-pwm-fan > - aspeed-pwm-tacho > - ina3221 > > In many cases the channels are "shared" - we have 3 voltage, 3 current and 3 > power sensors but in fact they are not separate sensors but 3 channels > each able to measure 3 different things and they may share some common > properties in each channel (so current, voltage and power may be > calculated bases on the same shunt resistor or correction factor). An > example being adi,ltc2992. In those cases it doesn't make sense to have > two levels as how would you describe the shared parent? Call it generic > "channels"? > > So maybe it makes sense to have 2 levels for complex devices that can > measure several independent entities or for devices which do not have a > clear concept of enumerated "channels" or "inputs", but we could skip it > for most others? After all, what is the benefit of having this > additional level if all we have is something like: > > temperature-sensors { > temperature1 { > }; > > temperature2 { > }; > > temperature3 { > }; > }; I see your point. I think it would make sense to only use the two-level approach for devices with more than one type of sensors. Thanks, Guenter > > For most devices having an "index" or "reg" makes much more sense so: > temperature@1, or channel@1 feels like a more natural way to describe > them. > > In any case, I'm quite confused right now on what would be the > conclusion of this discussion. How would you like the DT for TMP421 to > look like, after all? > > As a side note, should the description of the tmp421 bindings be in > tmp421.yaml file (as it is in current patchset), or should it be > actually called "ti,tmp421.yaml"? > > Krzysztof
> > In many cases the channels are "shared" - we have 3 voltage, 3 current and 3 > > power sensors but in fact they are not separate sensors but 3 channels > > each able to measure 3 different things and they may share some common > > properties in each channel (so current, voltage and power may be > > calculated bases on the same shunt resistor or correction factor). An > > example being adi,ltc2992. In those cases it doesn't make sense to have > > two levels as how would you describe the shared parent? Call it generic > > "channels"? So in that case (e.g. for the nct7802, see [1]) do we want just 1-level, maybe like this: nct7802@28 { compatible = "nuvoton,nct7802"; reg = <0x28>; sensor@1 { /* RTD1 */ reg = <0x1>; status = "okay"; mode = "thermistor"; /* Any of "thermistor", "thermal-diode", "voltage" */ }; sensor@2 { /* RTD2 */ reg = <0x2>; status = "okay"; mode = "thermal-diode"; /* Any of "thermistor", "thermal-diode", "voltage" */ }; sensor@3 { /* RTD3 */ reg = <0x3>; status = "okay"; mode = "voltage"; /* Any of "thermistor", "voltage" */ }; sensor@4 { /* LTD */ reg = <0x4>; /* using the same number as in sysfs */ status = "okay"; /* No mode configuration for LTD */ }; }; In this example, RTD1, RTD2 and LTD would be temperature sensors and RTD3 would be a voltage sensor. Would that make more sense? Is the use of strings acceptable? Thanks Oskar. [1] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/
On Fri, Sep 24, 2021 at 11:37:00AM -0400, Oskar Senft wrote: > > > In many cases the channels are "shared" - we have 3 voltage, 3 current and 3 > > > power sensors but in fact they are not separate sensors but 3 channels > > > each able to measure 3 different things and they may share some common > > > properties in each channel (so current, voltage and power may be > > > calculated bases on the same shunt resistor or correction factor). An > > > example being adi,ltc2992. In those cases it doesn't make sense to have > > > two levels as how would you describe the shared parent? Call it generic > > > "channels"? > > So in that case (e.g. for the nct7802, see [1]) do we want just > 1-level, maybe like this: > > nct7802@28 { > compatible = "nuvoton,nct7802"; > reg = <0x28>; > > sensor@1 { /* RTD1 */ > reg = <0x1>; > status = "okay"; > mode = "thermistor"; /* Any of "thermistor", "thermal-diode", > "voltage" */ > }; > > sensor@2 { /* RTD2 */ > reg = <0x2>; > status = "okay"; > mode = "thermal-diode"; /* Any of "thermistor", > "thermal-diode", "voltage" */ > }; > > sensor@3 { /* RTD3 */ > reg = <0x3>; > status = "okay"; > mode = "voltage"; /* Any of "thermistor", "voltage" */ > }; > > sensor@4 { /* LTD */ > reg = <0x4>; /* using the same number as in sysfs */ Numbering in sysfs is not relevant here; the index should always start with 0. > status = "okay"; > /* No mode configuration for LTD */ > }; > }; > > In this example, RTD1, RTD2 and LTD would be temperature sensors and > RTD3 would be a voltage sensor. > > Would that make more sense? Is the use of strings acceptable? > I don't think so. I am quite sure that rtd3 is still a temperature, and I am not sure if other sensor types on that chip may need dt configuration. I have limited internet acces for the next week or so; I'll look into this further after I am back. Guenter > Thanks > Oskar. > > [1] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/
Hi Guenter > Numbering in sysfs is not relevant here; the index should always start with 0. Ok, in that case, I'll encode LTD as @0 and RTD1..3 as @1..@3. > > In this example, RTD1, RTD2 and LTD would be temperature sensors and > > RTD3 would be a voltage sensor. > > > > Would that make more sense? Is the use of strings acceptable? > > > I don't think so. I am quite sure that rtd3 is still a temperature, > and I am not sure if other sensor types on that chip may need dt > configuration. Reading the existing nct7802_in_is_visible() and nct7802_temp_is_visible() [1] in I read that RTD3 could either be voltage or temperature. I'll go ahead and propose another patch version on [3] for that. [1] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L778 [2] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L679 [3] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/
On 10/8/21 5:55 AM, Oskar Senft wrote: > Hi Guenter > >> Numbering in sysfs is not relevant here; the index should always start with 0. > Ok, in that case, I'll encode LTD as @0 and RTD1..3 as @1..@3. > >>> In this example, RTD1, RTD2 and LTD would be temperature sensors and >>> RTD3 would be a voltage sensor. >>> >>> Would that make more sense? Is the use of strings acceptable? >>> >> I don't think so. I am quite sure that rtd3 is still a temperature, >> and I am not sure if other sensor types on that chip may need dt >> configuration. > Reading the existing nct7802_in_is_visible() and > nct7802_temp_is_visible() [1] in I read that RTD3 could either be > voltage or temperature. > Ah yes, you are correct. The same applies to RTD1 and RTD2, actually, Sorry, it has been too long since I wrote the driver. And it really needs a conversion to the new hwmon API. Guenter > I'll go ahead and propose another patch version on [3] for that. > > [1] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L778 > [2] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L679 > [3] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/ >
diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml index 53940e146ee6..56085fdf1b57 100644 --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml @@ -24,12 +24,49 @@ properties: reg: maxItems: 1 + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + required: - compatible - reg additionalProperties: false +patternProperties: + "^input@([0-4])$": + type: object + description: | + Represents channels of the device and their specific configuration. + + properties: + reg: + description: | + The channel number. 0 is local channel, 1-4 are remote channels + items: + minimum: 0 + maximum: 4 + + label: + description: | + A descriptive name for this channel, like "ambient" or "psu". + + n-factor: + description: | + The value (two's complement) to be programmed in the channel specific N correction register. + For remote channels only. + items: + minimum: 0 + maximum: 1 + + required: + - reg + + additionalProperties: false + examples: - | i2c { @@ -41,3 +78,32 @@ examples: reg = <0x4c>; }; }; + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@4c { + compatible = "ti,tmp422"; + reg = <0x4c>; + #address-cells = <1>; + #size-cells = <0>; + + input@0 { + reg = <0x0>; + n-factor = <0x1>; + label = "local"; + }; + + input@1 { + reg = <0x1>; + n-factor = <0x0>; + label = "somelabel"; + }; + + input@2 { + reg = <0x2>; + status = "disabled"; + }; + }; + };
Add binding description for the per temperature channel configuration like labels and n-factor. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- .../devicetree/bindings/hwmon/tmp421.yaml | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+)