Document bindings for the Nuvoton NCT7802Y driver.
Signed-off-by: Oskar Senft <[email protected]>
---
.../bindings/hwmon/nuvoton,nct7802.yaml | 92 +++++++++++++++++++
1 file changed, 92 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..7512acbc9d1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
@@ -0,0 +1,92 @@
+# 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 <[email protected]>
+
+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
+
+ temperature-sensors:
+ type: object
+ description:
+ |
+ Configuration for temperature sensors. If temperature-sensors
+ is not provided at all, the internal temperature sensor will be
+ enabled. If temperature-sensors is provided, only the listed
+ sensors will be configured as specified. The runtime configuration
+ for un-listed sensors will not be touched in the chip. This allows
+ sensors to be configured at power-up time from an EEPROM connected
+ to the chip and at the sime time allows to override these settings
+ using device tree configuration.
+
+ properties:
+ ltd:
+ type: object
+ description: Internal Temperature Sensor ("LTD")
+
+ rtd3:
+ type: object
+ description:
+ |
+ Remote Temperature Sensor ("RTD3"). This sensor only supports
+ thermistor mode.
+
+ patternProperties:
+ "^rtd[1-2]$":
+ type: object
+ description: Remote Temperature Sensor ("RTDx")
+ properties:
+ "type":
+ description: Sensor type (3=thermal diode, 4=thermistor).
+ allOf:
+ - $ref: "http://devicetree.org/schemas/types.yaml#/definitions/uint32"
+ - items:
+ - enum: [ 3, 4 ]
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ nct7802@28 {
+ compatible = "nuvoton,nct7802";
+ reg = <0x28>;
+
+ temperature-sensors {
+ ltd {
+ status = "disabled";
+ };
+
+ rtd1 {
+ status = "okay";
+ type = <4> /* thermistor */;
+ };
+ };
+ };
+ };
--
2.33.0.464.g1972c5931b-goog
On Mon, Sep 20, 2021 at 08:46:26PM -0400, Oskar Senft wrote:
> Document bindings for the Nuvoton NCT7802Y driver.
>
> Signed-off-by: Oskar Senft <[email protected]>
> ---
> .../bindings/hwmon/nuvoton,nct7802.yaml | 92 +++++++++++++++++++
> 1 file changed, 92 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..7512acbc9d1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
> @@ -0,0 +1,92 @@
> +# 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 <[email protected]>
Should be someone that cares about this h/w, not who applies patches.
> +
> +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
> +
> + temperature-sensors:
> + type: object
> + description:
> + |
Drop. Not needed if formatting is not needed to be maintained. (Also
belongs on the prior line.)
> + Configuration for temperature sensors. If temperature-sensors
> + is not provided at all, the internal temperature sensor will be
> + enabled. If temperature-sensors is provided, only the listed
> + sensors will be configured as specified. The runtime configuration
> + for un-listed sensors will not be touched in the chip. This allows
> + sensors to be configured at power-up time from an EEPROM connected
> + to the chip and at the sime time allows to override these settings
> + using device tree configuration.
> +
> + properties:
> + ltd:
> + type: object
> + description: Internal Temperature Sensor ("LTD")
No child properties?
> +
> + rtd3:
> + type: object
> + description:
> + |
> + Remote Temperature Sensor ("RTD3"). This sensor only supports
> + thermistor mode.
No child properties?
> +
> + patternProperties:
> + "^rtd[1-2]$":
> + type: object
> + description: Remote Temperature Sensor ("RTDx")
> + properties:
> + "type":
Don't need quotes.
> + description: Sensor type (3=thermal diode, 4=thermistor).
2nd time I've seen this property this week[1]. Needs to be more specific
than just 'type'.
> + allOf:
Don't need allOf.
> + - $ref: "http://devicetree.org/schemas/types.yaml#/definitions/uint32"
> + - items:
> + - enum: [ 3, 4 ]
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nct7802@28 {
> + compatible = "nuvoton,nct7802";
> + reg = <0x28>;
> +
> + temperature-sensors {
> + ltd {
> + status = "disabled";
Don't show status in examples.
> + };
> +
> + rtd1 {
> + status = "okay";
> + type = <4> /* thermistor */;
> + };
> + };
> + };
> + };
> --
[1] https://lore.kernel.org/all/CAL_Jsq+NXuF+F7OE3vyEbTUj6sxyMHVWHXbCuPPoFaKjpyZREQ@mail.gmail.com/
Hi Rob
> > +maintainers:
> > + - Guenter Roeck <[email protected]>
>
> Should be someone that cares about this h/w, not who applies patches.
Hmm, ok. After talking with Guenter, I thought that would be him. But
I can add myself, too, since we're obviously using that HW. Is that
what you mean?
> > + properties:
> > + ltd:
> > + type: object
> > + description: Internal Temperature Sensor ("LTD")
>
> No child properties?
Yes. We really just want the ability to enable / disable that sensor.
What's the correct way in the YAML to describe that? Same for RTD3.
> > + "type":
> > + description: Sensor type (3=thermal diode, 4=thermistor).
>
> 2nd time I've seen this property this week[1]. Needs to be more specific
> than just 'type'.
Ha yes, the example in [1] came from this patch. I went with this name
to stay in-line with the sysfs name, being "tempX_type". In the
hardware this would be called "mode".
My original proposal [2] was to have this property a string list named
"nuvoton,rtd-modes" with a set of accepted values, i.e. basically an
enum. Splitting this string list into individual sensors makes sense.
The other question that remains open (at least in my view), is whether
naming the sensors "ltd, rtd1, rtd2, rtd3" is the right approach or if
we should really go to naming them "sensor@X" with a reg property set
to X. Note that ltd and rtd3 do not accept any additional
configuration beyond "is enabled" (i.e. "status").
> > + temperature-sensors {
> > + ltd {
> > + status = "disabled";
>
> Don't show status in examples.
Hmm, ok. I found it useful to make clear that a sensor can be
disabled, but maybe that's just always the case?
I appreciate your other comments and will fix them in the next version
of the patch. But I'd like to get clarity wrt. recommended sensor and
property naming in the device tree before sending that.
Thoughts?
Thanks
Oskar.
> [1] https://lore.kernel.org/all/CAL_Jsq+NXuF+F7OE3vyEbTUj6sxyMHVWHXbCuPPoFaKjpyZREQ@mail.gmail.com/
[2] https://lore.kernel.org/all/[email protected]/
On Fri, Sep 24, 2021 at 11:17:51AM -0400, Oskar Senft wrote:
> Hi Rob
>
> > > +maintainers:
> > > + - Guenter Roeck <[email protected]>
> >
> > Should be someone that cares about this h/w, not who applies patches.
>
> Hmm, ok. After talking with Guenter, I thought that would be him. But
> I can add myself, too, since we're obviously using that HW. Is that
> what you mean?
FWIW, this happens to be one of the cases where I am also the driver
author, and I still have a test board with the chip.
Guenter
>
> > > + properties:
> > > + ltd:
> > > + type: object
> > > + description: Internal Temperature Sensor ("LTD")
> >
> > No child properties?
>
> Yes. We really just want the ability to enable / disable that sensor.
> What's the correct way in the YAML to describe that? Same for RTD3.
>
> > > + "type":
> > > + description: Sensor type (3=thermal diode, 4=thermistor).
> >
> > 2nd time I've seen this property this week[1]. Needs to be more specific
> > than just 'type'.
>
> Ha yes, the example in [1] came from this patch. I went with this name
> to stay in-line with the sysfs name, being "tempX_type". In the
> hardware this would be called "mode".
>
> My original proposal [2] was to have this property a string list named
> "nuvoton,rtd-modes" with a set of accepted values, i.e. basically an
> enum. Splitting this string list into individual sensors makes sense.
>
> The other question that remains open (at least in my view), is whether
> naming the sensors "ltd, rtd1, rtd2, rtd3" is the right approach or if
> we should really go to naming them "sensor@X" with a reg property set
> to X. Note that ltd and rtd3 do not accept any additional
> configuration beyond "is enabled" (i.e. "status").
>
> > > + temperature-sensors {
> > > + ltd {
> > > + status = "disabled";
> >
> > Don't show status in examples.
> Hmm, ok. I found it useful to make clear that a sensor can be
> disabled, but maybe that's just always the case?
>
> I appreciate your other comments and will fix them in the next version
> of the patch. But I'd like to get clarity wrt. recommended sensor and
> property naming in the device tree before sending that.
>
> Thoughts?
>
> Thanks
> Oskar.
>
> > [1] https://lore.kernel.org/all/CAL_Jsq+NXuF+F7OE3vyEbTUj6sxyMHVWHXbCuPPoFaKjpyZREQ@mail.gmail.com/
> [2] https://lore.kernel.org/all/[email protected]/
On Fri, Sep 24, 2021 at 10:18 AM Oskar Senft <[email protected]> wrote:
>
> Hi Rob
>
> > > +maintainers:
> > > + - Guenter Roeck <[email protected]>
> >
> > Should be someone that cares about this h/w, not who applies patches.
>
> Hmm, ok. After talking with Guenter, I thought that would be him. But
> I can add myself, too, since we're obviously using that HW. Is that
> what you mean?
Okay, seems it is Guenter in this case.
>
> > > + properties:
> > > + ltd:
> > > + type: object
> > > + description: Internal Temperature Sensor ("LTD")
> >
> > No child properties?
>
> Yes. We really just want the ability to enable / disable that sensor.
> What's the correct way in the YAML to describe that? Same for RTD3.
Okay, you need a 'additionalProperties: false' in the schema. (status
will automagically be allowed)
>
> > > + "type":
> > > + description: Sensor type (3=thermal diode, 4=thermistor).
> >
> > 2nd time I've seen this property this week[1]. Needs to be more specific
> > than just 'type'.
>
> Ha yes, the example in [1] came from this patch. I went with this name
> to stay in-line with the sysfs name, being "tempX_type". In the
> hardware this would be called "mode".
>
> My original proposal [2] was to have this property a string list named
> "nuvoton,rtd-modes" with a set of accepted values, i.e. basically an
> enum. Splitting this string list into individual sensors makes sense.
>
> The other question that remains open (at least in my view), is whether
> naming the sensors "ltd, rtd1, rtd2, rtd3" is the right approach or if
> we should really go to naming them "sensor@X" with a reg property set
> to X. Note that ltd and rtd3 do not accept any additional
> configuration beyond "is enabled" (i.e. "status").
If X is not made up numbering (i.e. corresponds to something in the
datasheet), then using addresses and generic node names are preferred.
Alignment with other similar h/w is also preferred.
> > > + temperature-sensors {
> > > + ltd {
> > > + status = "disabled";
> >
> > Don't show status in examples.
> Hmm, ok. I found it useful to make clear that a sensor can be
> disabled, but maybe that's just always the case?
Yeah, this case is a bit special. The node not being present also disables it.
The problem is generally we don't want disabled examples as that turns
off some validation. I have a check for this that I plan to add, but I
don't have a way to have exceptions.
What you could do is just comment out the node. Then you show it, but
don't compile it.
Rob
Hi Rob
> > > > + temperature-sensors {
> > > > + ltd {
> > > > + status = "disabled";
> > >
> > > Don't show status in examples.
> > Hmm, ok. I found it useful to make clear that a sensor can be
> > disabled, but maybe that's just always the case?
>
> Yeah, this case is a bit special. The node not being present also disables it.
Oh, I didn't realize that a missing node defaults to "disabled". What
I want to achieve is that if a node is not present, we don't configure
it. The reason behind this is that the HW provides a mechanism to
configure itself at power-up from a connected EEPROM. In that case
we'd still want the list the nct7802 in the DTS, but without
configuration. This effectively is the current behavior.
From what I understand from [1] and follow-ups, having the extra
"temperature-sensors" level is actually not what we want here and I
proposed a different solution in [2].
On that background, I'm wondering how we could have compatibility with
the previous behavior, where the individual sensors were not listed,
and just defaulted to whatever the HW came up with, whether that was
power-on defaults or loaded from an EEPROM.
What the code currently does is to check for the presence of
"temperature-sensors" and only attempt to configure any of them if
that top level node exists. This enables backwards-compatibility.
Going forward, I would have done the same for sensor@X and only
explicitly enable / disable the sensor if the node is present. If it's
not present, I'd use the power-on / EEPROM-provided defaults.
Thanks
Oskar.
[1] https://lore.kernel.org/linux-hwmon/[email protected]/
[2] https://lore.kernel.org/linux-hwmon/CABoTLcQYHZbsgzXN7XXKQdDn8S-YsuE+ks9WShAEKcBJojEfcQ@mail.gmail.com/
On Fri, Oct 08, 2021 at 09:07:47AM -0400, Oskar Senft wrote:
> Hi Rob
>
> > > > > + temperature-sensors {
> > > > > + ltd {
> > > > > + status = "disabled";
> > > >
> > > > Don't show status in examples.
> > > Hmm, ok. I found it useful to make clear that a sensor can be
> > > disabled, but maybe that's just always the case?
> >
> > Yeah, this case is a bit special. The node not being present also disables it.
> Oh, I didn't realize that a missing node defaults to "disabled". What
> I want to achieve is that if a node is not present, we don't configure
> it. The reason behind this is that the HW provides a mechanism to
> configure itself at power-up from a connected EEPROM. In that case
> we'd still want the list the nct7802 in the DTS, but without
> configuration. This effectively is the current behavior.
>
> From what I understand from [1] and follow-ups, having the extra
> "temperature-sensors" level is actually not what we want here and I
> proposed a different solution in [2].
>
Turns out this chip has another level of complexity, where a channel
can either be a temperature sensor or a voltage sensor. So, from dt
perspective, we don't have separate scoped for the different sensor
types.
I don't really like [2] to indicate voltage vs. temperature using "mode"
(it maps both sensor more and type into a single property),
but I agree that two levels doesn't really make sense here either.
That is where child naming may come in. We have "sensor" in your proposal,
and "input" in the tmp421 proposal. My thought on that was that we could
use the child name to distinguish sensor types.
temperature-sensor@1 { /* RTD1 */
reg = <0x1>;
status = "okay";
mode = "thermistor"; /* Any of "thermistor", "thermal-diode" */
};
voltage-sensor@3 { /* RTD3 */
reg = <0x3>;
status = "okay";
};
or maybe
sensor@1 { /* RTD1 */
reg = <0x1>;
status = "okay";
type = "temperature-sensor";
mode = "thermistor"; /* Any of "thermistor", "thermal-diode" */
};
sensor@3 { /* RTD3 */
reg = <0x3>;
status = "okay";
type = "voltage-sensor";
};
> On that background, I'm wondering how we could have compatibility with
> the previous behavior, where the individual sensors were not listed,
> and just defaulted to whatever the HW came up with, whether that was
> power-on defaults or loaded from an EEPROM.
>
> What the code currently does is to check for the presence of
> "temperature-sensors" and only attempt to configure any of them if
> that top level node exists. This enables backwards-compatibility.
> Going forward, I would have done the same for sensor@X and only
> explicitly enable / disable the sensor if the node is present. If it's
> not present, I'd use the power-on / EEPROM-provided defaults.
>
Makes sense to me.
Guenter
> Thanks
> Oskar.
>
> [1] https://lore.kernel.org/linux-hwmon/[email protected]/
> [2] https://lore.kernel.org/linux-hwmon/CABoTLcQYHZbsgzXN7XXKQdDn8S-YsuE+ks9WShAEKcBJojEfcQ@mail.gmail.com/