[PATCH] dt-bindings: hwmon: Add nct7802 bindings

[PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

Document bindings for the Nuvoton NCT7802Y driver.

Signed-off-by: Oskar Senft <osk@google.com>
---
 .../bindings/hwmon/nuvoton,nct7802.yaml       | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
new file mode 100644
index 000000000000..bc4b69aeb116
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7802.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NCT7802Y Hardware Monitoring IC
+
+maintainers:
+  - Guenter Roeck <linux@roeck-us.net>
+
+description: |
+  The NCT7802Y is a hardware monitor IC which supports one on-die and up to
+  5 remote temperature sensors with SMBus interface.
+
+  Datasheets:
+    https://www.nuvoton.com/export/resource-files/Nuvoton_NCT7802Y_Datasheet_V12.pdf
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,nct7802
+
+  reg:
+    maxItems: 1
+
+  nuvoton,rtd-modes:
+    description: |
+      Select modes for the three RTDs.
+
+      The order is RTD1, RTD2, RTD3.
+
+      Valid values for RTD1 and RTD2 are:
+        "closed",
+        "current",
+        "thermistor",
+        "voltage"
+
+      Valid values for RTD3 are:
+        "closed",
+        "thermistor",
+        "voltage"
+    type: stringlist
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        nct7802@28 {
+            compatible = "nuvoton,nct7802";
+            reg = <0x28>;
+            nuvoton,sensor-modes =
+              /* RTD1 */ "thermistor",
+              /* RTD2 */ "voltage",
+              /* RTD3 */ "closed";
+        };
+    };
-- 
2.33.0.309.g3052b89438-goog

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/10/21 6:03 AM, Oskar Senft wrote:
> Document bindings for the Nuvoton NCT7802Y driver.
> 
> Signed-off-by: Oskar Senft <osk@google.com>
> ---
>   .../bindings/hwmon/nuvoton,nct7802.yaml       | 66 +++++++++++++++++++
>   1 file changed, 66 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
> new file mode 100644
> index 000000000000..bc4b69aeb116
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7802.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NCT7802Y Hardware Monitoring IC
> +
> +maintainers:
> +  - Guenter Roeck <linux@roeck-us.net>
> +
> +description: |
> +  The NCT7802Y is a hardware monitor IC which supports one on-die and up to
> +  5 remote temperature sensors with SMBus interface.
> +
> +  Datasheets:
> +    https://www.nuvoton.com/export/resource-files/Nuvoton_NCT7802Y_Datasheet_V12.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,nct7802
> +
> +  reg:
> +    maxItems: 1
> +
> +  nuvoton,rtd-modes:
> +    description: |
> +      Select modes for the three RTDs.
> +
At the very least, "RTD" should be defined. The datasheet doesn't say explicitly,
but I suspect it means "Remote Temperature Diode".

> +      The order is RTD1, RTD2, RTD3.
> +
> +      Valid values for RTD1 and RTD2 are:
> +        "closed",
> +        "current",
> +        "thermistor",
> +        "voltage"
> +
> +      Valid values for RTD3 are:
> +        "closed",
> +        "thermistor",
> +        "voltage"
> +    type: stringlist

I am not sure what "closed" means (the datasheet doesn't say), but I suspect it means
that the sensor is disabled (?). For the other modes, the translation to the standard
ABI is:

current -> 3 (thermal diode)
thermistor -> 4 (thermistor)
voltage -> 2 (3904 transistor)

That makes me wonder if it would be better to define generic thermal sensor
properties (possibly aligned with the ABI) instead of chip specific ones.
That would probably require sub-nodes to be able to express if a specific
sensor is enabled/disabled. Rob, any thoughts ?

Thanks,
Guenter

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        nct7802@28 {
> +            compatible = "nuvoton,nct7802";
> +            reg = <0x28>;
> +            nuvoton,sensor-modes =
> +              /* RTD1 */ "thermistor",
> +              /* RTD2 */ "voltage",
> +              /* RTD3 */ "closed";
> +        };
> +    };
> 

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

Hi Guenter

Thanks for the quick feedback!

> > +  nuvoton,rtd-modes:
> > +    description: |
> > +      Select modes for the three RTDs.
> > +
> At the very least, "RTD" should be defined. The datasheet doesn't say explicitly,
> but I suspect it means "Remote Temperature Diode".
Ha, good point. As I understand, RTD means "Resistance Temperature
Detector". But TBH, I'm not sure how that squares with Nuvoton's use
of "LTD" for the local sensor ... sigh.

> > +      Valid values for RTD1 and RTD2 are:
> > +        "closed",
> > +        "current",
> > +        "thermistor",
> > +        "voltage"
> I am not sure what "closed" means (the datasheet doesn't say), but I suspect it means
> that the sensor is disabled (?). For the other modes, the translation to the standard
> ABI is:
Thanks for that pointer, I now found that in
Documentation/hwmon/sysfs-interface. Given that there's no definition
for "disabled", I guess I'll just leave that out of the device tree
binding for now? That way we'll stay consistent with the sysfs ABI.

That gives us the following mapping for sysfs / device tree -> nct7802 HW:
2 (3904 transistor) -> 3 (voltage)
3 (thermal diode) -> 1 (current)
4 (thermistor) -> 2 (thermistor)

I'll update the device tree binding to be an array then. I also update
the temp_type functions to support all 3 values.

Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/10/21 1:44 PM, Oskar Senft wrote:
> Hi Guenter
> 
> Thanks for the quick feedback!
> 
>>> +  nuvoton,rtd-modes:
>>> +    description: |
>>> +      Select modes for the three RTDs.
>>> +
>> At the very least, "RTD" should be defined. The datasheet doesn't say explicitly,
>> but I suspect it means "Remote Temperature Diode".
> Ha, good point. As I understand, RTD means "Resistance Temperature
> Detector". But TBH, I'm not sure how that squares with Nuvoton's use
> of "LTD" for the local sensor ... sigh.
> 
>>> +      Valid values for RTD1 and RTD2 are:
>>> +        "closed",
>>> +        "current",
>>> +        "thermistor",
>>> +        "voltage"
>> I am not sure what "closed" means (the datasheet doesn't say), but I suspect it means
>> that the sensor is disabled (?). For the other modes, the translation to the standard
>> ABI is:
> Thanks for that pointer, I now found that in
> Documentation/hwmon/sysfs-interface. Given that there's no definition
> for "disabled", I guess I'll just leave that out of the device tree
> binding for now? That way we'll stay consistent with the sysfs ABI.
> 

Sure there is. A possible set of bindings - in that case for tmp421 -
is suggested with the series at
https://lore.kernel.org/linux-hwmon/cover.1631021349.git.krzysztof.adamski@nokia.com/

That specifically includes the ability to enable or disable channels
using the standard 'status' property. While that series is primarily
for the n-factor property supported by the tmp421, the same approach
can be used for [temperature] sensor properties on other chips as well.

I put [temperature] in [] because we'd need to find a means to express
if the sub-nodes are for temperature, voltage, or something else, but
I think the basic principle is sound.

Guenter

> That gives us the following mapping for sysfs / device tree -> nct7802 HW:
> 2 (3904 transistor) -> 3 (voltage)
> 3 (thermal diode) -> 1 (current)
> 4 (thermistor) -> 2 (thermistor)
> 
> I'll update the device tree binding to be an array then. I also update
> the temp_type functions to support all 3 values.
> 
> Oskar.
> 

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

Hi Guenter

> https://lore.kernel.org/linux-hwmon/cover.1631021349.git.krzysztof.adamski@nokia.com/
>
> That specifically includes the ability to enable or disable channels
> using the standard 'status' property. While that series is primarily
> for the n-factor property supported by the tmp421, the same approach
> can be used for [temperature] sensor properties on other chips as well.

Good pointer! I should be able to replicate that for the LTD (@0) and
RTDs (1, 2, 3) in a similar way.

> I put [temperature] in [] because we'd need to find a means to express
> if the sub-nodes are for temperature, voltage, or something else, but
> I think the basic principle is sound.
Following the example from tmp421, this could then be like this:

i2c {
    #address-cells = <1>;
    #size-cells = <0>;

    nct7802@28 {
        compatible = "nuvoton,nct7802";
        reg = <0x28>;
        #address-cells = <1>;
        #size-cells = <0>;

        /* LTD */
        input@0 {
            reg = <0x0>;
            status = "okay";
            /* No "mode" attribute here*/
            label = "local temp";
        };

        /* RTD1 */
        input@1 {
            reg = <0x1>;
            mode = <0x2>; /* 3904 transistor */
            label = "voltage mode";
        };

        input@2 {
            reg = <0x2>;
            mode = <0x4>; /* thermistor */
            label = "thermistor mode";
        };

        /* RTD3 */
        input@3 {
            reg = <0x3>;
            mode = <0x3>; /* thermal diode */
            label = "current mode";
            status = "disabled";
        };
    };
};

I noticed that "nct7802_temp_is_visible" only allows the temperature
sensor to be visible for current and thermistor but not voltage. Is
that right?

Before I go and change the driver further, I'd like to make sure we
agree on the interface.

Also: Is nct7802_temp_is_visible called again after temp_type_store
was called (I didn't try it)?

Thanks
Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On Tue, Sep 14, 2021 at 08:41:36AM -0400, Oskar Senft wrote:
> Hi Guenter
> 
> > https://lore.kernel.org/linux-hwmon/cover.1631021349.git.krzysztof.adamski@nokia.com/
> >
> > That specifically includes the ability to enable or disable channels
> > using the standard 'status' property. While that series is primarily
> > for the n-factor property supported by the tmp421, the same approach
> > can be used for [temperature] sensor properties on other chips as well.
> 
> Good pointer! I should be able to replicate that for the LTD (@0) and
> RTDs (1, 2, 3) in a similar way.
> 
> > I put [temperature] in [] because we'd need to find a means to express
> > if the sub-nodes are for temperature, voltage, or something else, but
> > I think the basic principle is sound.
> Following the example from tmp421, this could then be like this:
> 

Something like that, only we'll need something to distinguish
temperature sensors from other sensor types, eg voltage or current.
Maybe a "type" property. I'd suggest "sensor-type", but we have
non-sensor attributes such as fan count and pwm values which should
be covered as well. But it looks like a good start for a set of
generic sensor properties.

> i2c {
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     nct7802@28 {
>         compatible = "nuvoton,nct7802";
>         reg = <0x28>;
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         /* LTD */
>         input@0 {
>             reg = <0x0>;
>             status = "okay";

Not sure what the default is here ('okay' or 'disabled').
We'd also need to define what to do if there is no data
for a given sensor.

>             /* No "mode" attribute here*/
>             label = "local temp";
>         };
> 
>         /* RTD1 */
>         input@1 {
>             reg = <0x1>;
>             mode = <0x2>; /* 3904 transistor */
>             label = "voltage mode";

That isn't the idea for "label", as "label" would be expected to
show up as tempX_label (and a label of "voltage mode" would be odd).
The label should indicate where the sensor is located on a board,
such as "inlet" or "outlet".

>         };
> 
>         input@2 {
>             reg = <0x2>;
>             mode = <0x4>; /* thermistor */
>             label = "thermistor mode";
>         };
> 
>         /* RTD3 */
>         input@3 {
>             reg = <0x3>;
>             mode = <0x3>; /* thermal diode */
>             label = "current mode";
>             status = "disabled";
>         };
>     };
> };
> 
> I noticed that "nct7802_temp_is_visible" only allows the temperature
> sensor to be visible for current and thermistor but not voltage. Is
> that right?
> 
No, that is a bug.

> Before I go and change the driver further, I'd like to make sure we
> agree on the interface.
> 
> Also: Is nct7802_temp_is_visible called again after temp_type_store
> was called (I didn't try it)?
> 
No. That would not be the idea. If enabling / disabling a sensor
is supposed with the _enable attribute (and/or with devicetree),
the affected sensor should always be instantiated, and reading
sensor data should return -ENODATA if the sensor is disabled.

Thanks,
Guenter

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

Hi Guenter

> > Following the example from tmp421, this could then be like this:
>
> Something like that, only we'll need something to distinguish
> temperature sensors from other sensor types, eg voltage or current.
> Maybe a "type" property. I'd suggest "sensor-type", but we have
> non-sensor attributes such as fan count and pwm values which should
> be covered as well. But it looks like a good start for a set of
> generic sensor properties.
Would it be acceptable to simply number the sensors and document which
sensor has which number?

Something like this:
0 = LTD
1 = RTD1
2 = RTD2
3 = RTD3
4 = FAN1
5 = FAN2
6 = FAN3

Would we also want to be able to define PWMs? From what I can tell the
driver does not support running individual pins in GPIO mode, right?
So I'm not quite clear what "disabling PWM" would actually mean.

Anyway, if we simply go by "sensor number", that would mean that we'd
have different attributes depending on the sensor number. Would that
be ok?

Also, I'm sorry, I think I just realized that in "voltage mode" we
don't seem to get a temperature reading. I hadn't actually looked
through more of the datasheet except for the single MODE register
before. But I don't think this makes a difference for what I proposed
so far?

> >         /* LTD */
> >         input@0 {
> >             reg = <0x0>;
> >             status = "okay";
>
> Not sure what the default is here ('okay' or 'disabled').
> We'd also need to define what to do if there is no data
> for a given sensor.
I think I'd like to keep previous behavior unmodified. From what I can
tell previous behavior was:
- xTDs enabled by default
- RTD modes unmodified, i.e. defaulting to whatever the HW comes up with

The NCT7802Y can self-program from an EEPROM, so I assume we should
honor the "power-up configuration" obtained from there? I.e. if no
configuration is provided in the device tree, the driver should use
whatever configuration the chip has when the driver is loaded.

> >             label = "voltage mode";
>
> That isn't the idea for "label", as "label" would be expected to
> show up as tempX_label (and a label of "voltage mode" would be odd).
> The label should indicate where the sensor is located on a board,
> such as "inlet" or "outlet".
Yes, absolutely. This was a bad example on my part. In my
understanding "label" is just a string that we pass through.

Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/14/21 10:11 AM, Oskar Senft wrote:
> Hi Guenter
> 
>>> Following the example from tmp421, this could then be like this:
>>
>> Something like that, only we'll need something to distinguish
>> temperature sensors from other sensor types, eg voltage or current.
>> Maybe a "type" property. I'd suggest "sensor-type", but we have
>> non-sensor attributes such as fan count and pwm values which should
>> be covered as well. But it looks like a good start for a set of
>> generic sensor properties.
> Would it be acceptable to simply number the sensors and document which
> sensor has which number?
> 
> Something like this:
> 0 = LTD
> 1 = RTD1
> 2 = RTD2
> 3 = RTD3
> 4 = FAN1
> 5 = FAN2
> 6 = FAN3
> 
That might be a possibility, though it would have to be well defined
for each chip (nct7802 also has voltage sensors). We'll have to discuss
this with Rob.

Personally I think I would prefer using a type qualifier - that seems
cleaner. But that is really a matter of opinion.

> Would we also want to be able to define PWMs? From what I can tell the
> driver does not support running individual pins in GPIO mode, right?
> So I'm not quite clear what "disabling PWM" would actually mean.
> 
The ABI states that fans should run at full speed in that case,
though that may be chip dependent (some chips stop the fan if pwm
control is turned off).

> Anyway, if we simply go by "sensor number", that would mean that we'd
> have different attributes depending on the sensor number. Would that
> be ok?
> 
That is a question for Rob to answer.

> Also, I'm sorry, I think I just realized that in "voltage mode" we
> don't seem to get a temperature reading. I hadn't actually looked
> through more of the datasheet except for the single MODE register
> before. But I don't think this makes a difference for what I proposed
> so far?
> 

We don't ? I thought this reflects temperature measurement with a
transistor instead of a diode (which would be current based).
Hard to say - the datasheet is a bit vague in that regard.

>>>          /* LTD */
>>>          input@0 {
>>>              reg = <0x0>;
>>>              status = "okay";
>>
>> Not sure what the default is here ('okay' or 'disabled').
>> We'd also need to define what to do if there is no data
>> for a given sensor.
> I think I'd like to keep previous behavior unmodified. From what I can
> tell previous behavior was:
> - xTDs enabled by default
> - RTD modes unmodified, i.e. defaulting to whatever the HW comes up with
> 
> The NCT7802Y can self-program from an EEPROM, so I assume we should
> honor the "power-up configuration" obtained from there? I.e. if no
> configuration is provided in the device tree, the driver should use
> whatever configuration the chip has when the driver is loaded.
> 
Definitely yes. My question was more what to do if the information
in devicetree nodes is incomplete.

Thanks,
Guenter

>>>              label = "voltage mode";
>>
>> That isn't the idea for "label", as "label" would be expected to
>> show up as tempX_label (and a label of "voltage mode" would be odd).
>> The label should indicate where the sensor is located on a board,
>> such as "inlet" or "outlet".
> Yes, absolutely. This was a bad example on my part. In my
> understanding "label" is just a string that we pass through.
> 
> Oskar.
> 

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

    Hi Guenter

> > Would it be acceptable to simply number the sensors and document which
> > sensor has which number?
> >
> > Something like this:
> > 0 = LTD
> > 1 = RTD1
> > ...
> >
> That might be a possibility, though it would have to be well defined
> for each chip (nct7802 also has voltage sensors). We'll have to discuss
> this with Rob.
>
> Personally I think I would prefer using a type qualifier - that seems
> cleaner. But that is really a matter of opinion.

Another existing way I found is in ltc2978. Following that, we could
do it as follows:

i2c {
    #address-cells = <1>;
    #size-cells = <0>;

    nct7802@28 {
        compatible = "nuvoton,nct7802";
        reg = <0x28>;
        #address-cells = <1>;
        #size-cells = <0>;

        sensors {
            ltd {
                status = "okay";
                label = "my local temperature";
            };

            rtd1 {
                status = "okay";
                mode = <0x2>; /* 3904 transistor */
                label = "other temperature";
            };

            rtd3 {
                status = "okay";
                mode = <0x3>; /* thermal diode */
                label = "3rd temperature";
           };
        };
    };
};


> > The NCT7802Y can self-program from an EEPROM, so I assume we should
> > honor the "power-up configuration" obtained from there? I.e. if no
> > configuration is provided in the device tree, the driver should use
> > whatever configuration the chip has when the driver is loaded.
> >
> Definitely yes. My question was more what to do if the information
> in devicetree nodes is incomplete.
I think there are two cases:
1) If the new "sensor" tree is missing, the driver should behave as it
does today to not break existing users.
2) If the new "sensor" tree is present, then each of the sensors that
should be disabled needs to have "status = 'okay'" and have the mode
set (unless it's LTD). In the above example, rtd2 is missing and would
therefore be considered disabled.

Does that make sense? I still need to find out whether this is
actually valid DT and how to express that in the YAML, though ...

Thanks
Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/16/21 12:19 PM, Oskar Senft wrote:
>      Hi Guenter
> 
>>> Would it be acceptable to simply number the sensors and document which
>>> sensor has which number?
>>>
>>> Something like this:
>>> 0 = LTD
>>> 1 = RTD1
>>> ...
>>>
>> That might be a possibility, though it would have to be well defined
>> for each chip (nct7802 also has voltage sensors). We'll have to discuss
>> this with Rob.
>>
>> Personally I think I would prefer using a type qualifier - that seems
>> cleaner. But that is really a matter of opinion.
> 
> Another existing way I found is in ltc2978. Following that, we could
> do it as follows:
> 
> i2c {
>      #address-cells = <1>;
>      #size-cells = <0>;
> 
>      nct7802@28 {
>          compatible = "nuvoton,nct7802";
>          reg = <0x28>;
>          #address-cells = <1>;
>          #size-cells = <0>;
> 
>          sensors {
>              ltd {
>                  status = "okay";
>                  label = "my local temperature";
>              };
> 
>              rtd1 {
>                  status = "okay";
>                  mode = <0x2>; /* 3904 transistor */
>                  label = "other temperature";
>              };
> 
>              rtd3 {
>                  status = "okay";
>                  mode = <0x3>; /* thermal diode */
>                  label = "3rd temperature";
>             };
>          };
>      };
> };
> 

Ah, using the node name as indication for both sensor type and
index ? SGTM, though we'd really need input from Rob.
I guess one could also consider something more generic like
"temperature-sensor@0", "voltage-sensor@0", and so on (instead
of [mis-]using reg and a sensor-type field).

Thanks,
Guenter

> 
>>> The NCT7802Y can self-program from an EEPROM, so I assume we should
>>> honor the "power-up configuration" obtained from there? I.e. if no
>>> configuration is provided in the device tree, the driver should use
>>> whatever configuration the chip has when the driver is loaded.
>>>
>> Definitely yes. My question was more what to do if the information
>> in devicetree nodes is incomplete.
> I think there are two cases:
> 1) If the new "sensor" tree is missing, the driver should behave as it
> does today to not break existing users.
> 2) If the new "sensor" tree is present, then each of the sensors that
> should be disabled needs to have "status = 'okay'" and have the mode
> set (unless it's LTD). In the above example, rtd2 is missing and would
> therefore be considered disabled.
> 
> Does that make sense? I still need to find out whether this is
> actually valid DT and how to express that in the YAML, though ...
> 
> Thanks
> Oskar.
> 

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

> Ah, using the node name as indication for both sensor type and
> index ? SGTM, though we'd really need input from Rob.
> I guess one could also consider something more generic like
> "temperature-sensor@0", "voltage-sensor@0", and so on (instead
> of [mis-]using reg and a sensor-type field).

Hmm, in that case, maybe we should just remove the "sensors" section.

i2c {
    #address-cells = <1>;
    #size-cells = <0>;

    nct7802@28 {
        compatible = "nuvoton,nct7802";
        reg = <0x28>;
        #address-cells = <1>;
        #size-cells = <0>;

        temperature-sensor@0 { /* LTD */
            status = "okay";
            label = "my local temperature";
        };

        temperature-sensor@1 { /* RTD1 */
            status = "okay";
            mode = <0x2>; /* 3904 transistor */
            label = "other temperature";
        };

        temperature-sensor@3 { */ RTD3 */
            status = "okay";
            mode = <0x3>; /* thermal diode */
            label = "3rd temperature";
       };
   };
};

Now, with "sensors" removed and everything at "top-level", we'll need
to decide what to do if individual sensors are missing. I guess in
that case I would just leave the affected sensors alone, i.e. neither
configure nor disable them and instead read their status from HW. That
way prior uses of the nct7802 in device trees will continue to behave
as before as does the EEPROM-style configuration.

I would like to focus on the implementation of the temperature-sensor
entries for now (i.e. LTD, RTD1, RTD2, RTD3). Support for other sensor
types could be added in a separate change. Would that be acceptable?

Thanks
Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/16/21 12:53 PM, Oskar Senft wrote:
>> Ah, using the node name as indication for both sensor type and
>> index ? SGTM, though we'd really need input from Rob.
>> I guess one could also consider something more generic like
>> "temperature-sensor@0", "voltage-sensor@0", and so on (instead
>> of [mis-]using reg and a sensor-type field).
> 
> Hmm, in that case, maybe we should just remove the "sensors" section.
> 
> i2c {
>      #address-cells = <1>;
>      #size-cells = <0>;
> 
>      nct7802@28 {
>          compatible = "nuvoton,nct7802";
>          reg = <0x28>;
>          #address-cells = <1>;
>          #size-cells = <0>;
> 
>          temperature-sensor@0 { /* LTD */
>              status = "okay";
>              label = "my local temperature";
>          };
> 
>          temperature-sensor@1 { /* RTD1 */
>              status = "okay";
>              mode = <0x2>; /* 3904 transistor */
>              label = "other temperature";
>          };
> 
>          temperature-sensor@3 { */ RTD3 */
>              status = "okay";
>              mode = <0x3>; /* thermal diode */
>              label = "3rd temperature";
>         };
>     };
> };
> 

I think there was a reason for "sensors", because there can be other
bindings on the same level. Documentation/devicetree/bindings/hwmon/ltc2978.txt
lists "regulators", for example.

Where did you find the "sensors" example for ltc2978 ? I don't see it
in the upstream kernel. Or was that derived from the official "regulators"
bindings ?

> Now, with "sensors" removed and everything at "top-level", we'll need
> to decide what to do if individual sensors are missing. I guess in
> that case I would just leave the affected sensors alone, i.e. neither
> configure nor disable them and instead read their status from HW. That
> way prior uses of the nct7802 in device trees will continue to behave
> as before as does the EEPROM-style configuration.
> 
> I would like to focus on the implementation of the temperature-sensor
> entries for now (i.e. LTD, RTD1, RTD2, RTD3). Support for other sensor
> types could be added in a separate change. Would that be acceptable?
> 

Yes, let's do that. I'd like us to keep the "sensors" subnode to have a clear
association and differentiator to other sub-nodes such as "regulators".
Open is if we can use "temperature-sensor@0" or if it would have to be
a chip specific "ltd", but I think we can sort that out after suggesting
an initial set of bindings to Rob.

Thanks,
Guenter

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

> I think there was a reason for "sensors", because there can be other
> bindings on the same level. Documentation/devicetree/bindings/hwmon/ltc2978.txt
> lists "regulators", for example.
>
> Where did you find the "sensors" example for ltc2978 ? I don't see it
> in the upstream kernel. Or was that derived from the official "regulators"
> bindings ?

Yes, I just followed the structure from there, renaming "regulators"
as "sensors".


> Yes, let's do that. I'd like us to keep the "sensors" subnode to have a clear
> association and differentiator to other sub-nodes such as "regulators".
> Open is if we can use "temperature-sensor@0" or if it would have to be
> a chip specific "ltd", but I think we can sort that out after suggesting
> an initial set of bindings to Rob.
Makes sense, I'll do that. Now I just need to figure out how to
express that in YAML ...

I'll send a new patch as soon as I figured that out and got it to work.

Thanks for your input!

Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

Ok, I experimented with that and I think I'm starting to get an idea
how the DT bindings YAML works.

> > Yes, let's do that. I'd like us to keep the "sensors" subnode to have a clear
> > association and differentiator to other sub-nodes such as "regulators".
> > Open is if we can use "temperature-sensor@0" or if it would have to be
> > a chip specific "ltd", but I think we can sort that out after suggesting
> > an initial set of bindings to Rob.

However, I found that when I use the name@x syntax, the schema
validator also requires the use of a reg or ranges property. But then
doing so requires to set the #address-cells and #size-cells
properties, which - I think - makes things weird.

So these two examples are options that validate:
    i2c {
        #address-cells = <1>;
        #size-cells = <0>;

        nct7802@28 {
            compatible = "nuvoton,nct7802";
            reg = <0x28>;

            temperature-sensors {
                ltd {
                  status = "disabled";
                  label = "mainboard temperature";
                };

                rtd1 {
                  status = "okay";
                  label = "inlet temperature";
                  type = <4> /* thermistor */;
                };
            };
        };
    };

or

    i2c {
        #address-cells = <1>;
        #size-cells = <0>;

        nct7802@28 {
            compatible = "nuvoton,nct7802";
            reg = <0x28>;

            temperature-sensors {
                #address-cells = <1>;
                #size-cells = <0>;

                sensor@0 {
                  reg = <0>;
                  status = "disabled";
                  label = "mainboard temperature";
                };

                sensor@1 {
                  reg = <1>;
                  status = "okay";
                  label = "inlet temperature";
                  type = <4> /* thermistor */;
                };
            };
        };
    };

In the second case we end up having to duplicate information, i.e.
"sensor@1" and "reg = <1>". Also, I have not yet found a way to
validate that the "@x" is identical to the "reg = <x>". I believe that
this is just how it is in device trees, but I want to make sure this
is what we want?

Thoughts?

Thanks
Oskar.

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On Thu, Sep 16, 2021 at 11:09:16PM -0400, Oskar Senft wrote:
> Ok, I experimented with that and I think I'm starting to get an idea
> how the DT bindings YAML works.
> 
> > > Yes, let's do that. I'd like us to keep the "sensors" subnode to have a clear
> > > association and differentiator to other sub-nodes such as "regulators".
> > > Open is if we can use "temperature-sensor@0" or if it would have to be
> > > a chip specific "ltd", but I think we can sort that out after suggesting
> > > an initial set of bindings to Rob.
> 
> However, I found that when I use the name@x syntax, the schema
> validator also requires the use of a reg or ranges property. But then
> doing so requires to set the #address-cells and #size-cells
> properties, which - I think - makes things weird.
> 
> So these two examples are options that validate:
>     i2c {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         nct7802@28 {
>             compatible = "nuvoton,nct7802";
>             reg = <0x28>;
> 
>             temperature-sensors {
>                 ltd {
>                   status = "disabled";
>                   label = "mainboard temperature";
>                 };
> 
>                 rtd1 {
>                   status = "okay";
>                   label = "inlet temperature";
>                   type = <4> /* thermistor */;
>                 };
>             };
>         };
>     };
> 
> or
> 
>     i2c {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         nct7802@28 {
>             compatible = "nuvoton,nct7802";
>             reg = <0x28>;
> 
>             temperature-sensors {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 sensor@0 {
>                   reg = <0>;
>                   status = "disabled";
>                   label = "mainboard temperature";
>                 };
> 
>                 sensor@1 {
>                   reg = <1>;
>                   status = "okay";
>                   label = "inlet temperature";
>                   type = <4> /* thermistor */;
>                 };
>             };
>         };
>     };
> 
> In the second case we end up having to duplicate information, i.e.
> "sensor@1" and "reg = <1>". Also, I have not yet found a way to
> validate that the "@x" is identical to the "reg = <x>". I believe that
> this is just how it is in device trees, but I want to make sure this
> is what we want?
> 
> Thoughts?
> 
Comparing those two, I prefer the first option. Can you write that up
in a yaml file to present to Rob ? If he doesn't like it, we can still
suggest the second variant as an alternative.

Thanks,
Guenter

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/10/21 1:44 PM, Oskar Senft wrote:
> Hi Guenter
> 
> Thanks for the quick feedback!
> 
>>> +  nuvoton,rtd-modes:
>>> +    description: |
>>> +      Select modes for the three RTDs.
>>> +
>> At the very least, "RTD" should be defined. The datasheet doesn't say explicitly,
>> but I suspect it means "Remote Temperature Diode".
> Ha, good point. As I understand, RTD means "Resistance Temperature
> Detector". But TBH, I'm not sure how that squares with Nuvoton's use
> of "LTD" for the local sensor ... sigh.
> 
>>> +      Valid values for RTD1 and RTD2 are:
>>> +        "closed",
>>> +        "current",
>>> +        "thermistor",
>>> +        "voltage"
>> I am not sure what "closed" means (the datasheet doesn't say), but I suspect it means
>> that the sensor is disabled (?). For the other modes, the translation to the standard
>> ABI is:
> Thanks for that pointer, I now found that in
> Documentation/hwmon/sysfs-interface. Given that there's no definition
> for "disabled", I guess I'll just leave that out of the device tree
> binding for now? That way we'll stay consistent with the sysfs ABI.
> 

Sure there is. A possible set of bindings - in that case for tmp421 -
is suggested with the series at
https://lore.kernel.org/linux-hwmon/cover.1631021349.git.krzysztof.adamski@nokia.com/

That specifically includes the ability to enable or disable channels
using the standard 'status' property. While that series is primarily
for the n-factor property supported by the tmp421, the same approach
can be used for all temperature sensor properties.

Guenter

> That gives us the following mapping for sysfs / device tree -> nct7802 HW:
> 2 (3904 transistor) -> 3 (voltage)
> 3 (thermal diode) -> 1 (current)
> 4 (thermistor) -> 2 (thermistor)
> 
> I'll update the device tree binding to be an array then. I also update
> the temp_type functions to support all 3 values.
> 
> Oskar.
> 

Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Rob Herring]

On Fri, 10 Sep 2021 09:03:37 -0400, Oskar Senft wrote:
> Document bindings for the Nuvoton NCT7802Y driver.
> 
> Signed-off-by: Oskar Senft <osk@google.com>
> ---
>  .../bindings/hwmon/nuvoton,nct7802.yaml       | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml: properties:nuvoton,rtd-modes:type: 'anyOf' conditional failed, one must be fixed:
	'stringlist' is not one of ['array', 'boolean', 'integer', 'null', 'number', 'object', 'string']
	'stringlist' is not of type 'array'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml: properties:nuvoton,rtd-modes:type: 'stringlist' is not one of ['boolean', 'object']
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml: properties:nuvoton,rtd-modes: 'oneOf' conditional failed, one must be fixed:
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml: properties:nuvoton,rtd-modes: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	Additional properties are not allowed ('type' was unexpected)
		hint: A vendor string property with exact values has an implicit type
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml: properties:nuvoton,rtd-modes: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	'boolean' was expected
		hint: A vendor boolean property can use "type: boolean"
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml: ignoring, error in schema: properties: nuvoton,rtd-modes: type
warning: no schema found in file: ./Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.example.dt.yaml:0:0: /example-0/i2c/nct7802@28: failed to match any schema with compatible: ['nuvoton,nct7802']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1526504

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.