[PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[PATCH v2 1/2] 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       | 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 <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
+
+  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

[PATCH v2 2/2] hwmon: (nct7802) Make temperature sensors configurable.

[Oskar Senft]

This change allows the temperature sensors (both LTD and RTD) to be
configurable via device tree bindings. When the new settings are not
present, the driver defaults to the previous behavior where the RTDs
are left alone and LTD is explicitly enabled.

Signed-off-by: Oskar Senft <osk@google.com>
---
 drivers/hwmon/nct7802.c | 141 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 127 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index 604af2f6103a..ac2299b110a1 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -51,6 +51,20 @@ static const u8 REG_VOLTAGE_LIMIT_MSB_SHIFT[2][5] = {
 #define REG_CHIP_ID		0xfe
 #define REG_VERSION_ID		0xff
 
+/*
+ * Resistance temperature detector (RTD) modes according to 7.2.32 Mode
+ * Selection Register
+ */
+
+#define MODE_RTD_MASK		0x3
+#define MODE_LTD_EN		0x40
+
+/*
+ * Bit offset for sensors modes in REG_MODE.
+ * Valid for index 0..2, indicating RTD1..3.
+ */
+#define MODE_BIT_OFFSET_RTD(index) ((index) * 2)
+
 /*
  * Data structures and manipulation thereof
  */
@@ -74,7 +88,9 @@ static ssize_t temp_type_show(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	return sprintf(buf, "%u\n", (mode >> (2 * sattr->index) & 3) + 2);
+	return sprintf(buf, "%u\n",
+			((mode >> MODE_BIT_OFFSET_RTD(sattr->index)) &
+				MODE_RTD_MASK) + 2);
 }
 
 static ssize_t temp_type_store(struct device *dev,
@@ -94,7 +110,8 @@ static ssize_t temp_type_store(struct device *dev,
 	if (type < 3 || type > 4)
 		return -EINVAL;
 	err = regmap_update_bits(data->regmap, REG_MODE,
-			3 << 2 * sattr->index, (type - 2) << 2 * sattr->index);
+			MODE_RTD_MASK << MODE_BIT_OFFSET_RTD(sattr->index),
+			(type - 2) << MODE_BIT_OFFSET_RTD(sattr->index));
 	return err ? : count;
 }
 
@@ -688,18 +705,18 @@ static umode_t nct7802_temp_is_visible(struct kobject *kobj,
 	if (err < 0)
 		return 0;
 
-	if (index < 10 &&
-	    (reg & 03) != 0x01 && (reg & 0x03) != 0x02)		/* RD1 */
+	if (index >= 0 && index < 20 &&				/* RD1, RD 2*/
+	    ((reg >> MODE_BIT_OFFSET_RTD(index / 10)) & MODE_RTD_MASK) != 0x01 &&
+	    ((reg >> MODE_BIT_OFFSET_RTD(index / 10)) & MODE_RTD_MASK) != 0x02)
 		return 0;
 
-	if (index >= 10 && index < 20 &&
-	    (reg & 0x0c) != 0x04 && (reg & 0x0c) != 0x08)	/* RD2 */
-		return 0;
-	if (index >= 20 && index < 30 && (reg & 0x30) != 0x20)	/* RD3 */
+	if (index >= 20 && index < 30 &&			/* RD3 */
+	    (reg >> MODE_BIT_OFFSET_RTD(index / 10) & MODE_RTD_MASK != 0x02))
 		return 0;
 
-	if (index >= 30 && index < 38)				/* local */
-		return attr->mode;
+	if (index >= 30 && index < 38 &&			/* local */
+	    (reg & MODE_LTD_EN) != MODE_LTD_EN)
+		return 0;
 
 	err = regmap_read(data->regmap, REG_PECI_ENABLE, &reg);
 	if (err < 0)
@@ -1038,8 +1055,97 @@ static const struct regmap_config nct7802_regmap_config = {
 	.volatile_reg = nct7802_regmap_is_volatile,
 };
 
-static int nct7802_init_chip(struct nct7802_data *data)
+static bool nct7802_configure_temperature_sensors_from_device_tree(
+	struct device *dev, unsigned char *mode_mask, unsigned char *mode_val)
 {
+	struct device_node *sensors, *sensor;
+	int err;
+	u8 idx;
+	u32 type;
+
+	if (!dev->of_node)
+		return false;
+
+	sensors = of_get_child_by_name(dev->of_node, "temperature-sensors");
+	if (!sensors)
+		return false;
+
+	for_each_child_of_node(sensors, sensor) {
+		if (!strcmp(sensor->name, "ltd")) {
+			if (!of_device_is_available(sensor)) {
+				*mode_val &= ~MODE_LTD_EN;
+				*mode_mask |= MODE_LTD_EN;
+			} else {
+				*mode_val |= MODE_LTD_EN;
+				*mode_mask |= MODE_LTD_EN;
+			}
+			continue;
+		}
+
+		/* Check for rtdX, with X being the sensor number */
+		if (strcmp(sensor->name, "rtd") > 0) {
+			err = kstrtou8(
+				sensor->name + strlen("rdt"), 10, &idx);
+			if (err) {
+				dev_err(dev,
+					"Unparseable sensor number in '%s'\n",
+					sensor->name);
+				continue;
+			}
+
+			if (idx < 1 || idx > 3) {
+				dev_err(dev,
+					"Invalid sensor number in '%s'\n",
+					sensor->name);
+				continue;
+			}
+
+			if (!of_device_is_available(sensor)) {
+				*mode_val &= ~(MODE_RTD_MASK
+					<< MODE_BIT_OFFSET_RTD(idx-1));
+				*mode_mask |= MODE_RTD_MASK
+					<< MODE_BIT_OFFSET_RTD(idx-1);
+				continue;
+			}
+
+			if (idx == 3) {
+				/* RTD3 only supports thermistor mode */
+				type = 4;
+			} else if (of_property_count_u32_elems(sensor, "type")
+				> 0) {
+				if (of_property_read_u32_index(
+					sensor, "type", 0, &type)) {
+					dev_err(dev,
+						"Could not read type property for '%s'\n",
+						sensor->name);
+					continue;
+				}
+
+				/* Only current and thermistor modes are valid
+				 * for temperature sensors.
+				 */
+				if (type < 3 || type > 4) {
+					dev_err(dev,
+						"Type %u not supported for '%s'\n",
+						type, sensor->name);
+					continue;
+				}
+			}
+
+			/* Everything ok, so write sensor configuration */
+			*mode_val |= ((type - 2) & MODE_RTD_MASK)
+				<< MODE_BIT_OFFSET_RTD(idx-1);
+			*mode_mask |= MODE_RTD_MASK
+				<< MODE_BIT_OFFSET_RTD(idx-1);
+		}
+	}
+
+	return true;
+}
+
+static int nct7802_init_chip(struct device *dev, struct nct7802_data *data)
+{
+	unsigned char mode_val = 0, mode_mask = 0;
 	int err;
 
 	/* Enable ADC */
@@ -1047,8 +1153,15 @@ static int nct7802_init_chip(struct nct7802_data *data)
 	if (err)
 		return err;
 
-	/* Enable local temperature sensor */
-	err = regmap_update_bits(data->regmap, REG_MODE, 0x40, 0x40);
+	if (!nct7802_configure_temperature_sensors_from_device_tree(dev,
+			&mode_mask, &mode_val)) {
+		/* Enable local temperature sensor by default */
+		mode_val |= MODE_LTD_EN;
+		mode_mask |= MODE_LTD_EN;
+	}
+
+	err = regmap_update_bits(data->regmap, REG_MODE, mode_mask,
+		mode_val);
 	if (err)
 		return err;
 
@@ -1074,7 +1187,7 @@ static int nct7802_probe(struct i2c_client *client)
 	mutex_init(&data->access_lock);
 	mutex_init(&data->in_alarm_lock);
 
-	ret = nct7802_init_chip(data);
+	ret = nct7802_init_chip(dev, data);
 	if (ret < 0)
 		return ret;
 
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[Rob Herring]

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 <osk@google.com>
> ---
>  .../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 <linux@roeck-us.net>

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/

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

Hi Rob

> > +maintainers:
> > +  - Guenter Roeck <linux@roeck-us.net>
>
> 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/20210910130337.2025426-1-osk@google.com/

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On Fri, Sep 24, 2021 at 11:17:51AM -0400, Oskar Senft wrote:
> Hi Rob
> 
> > > +maintainers:
> > > +  - Guenter Roeck <linux@roeck-us.net>
> >
> > 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/20210910130337.2025426-1-osk@google.com/

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[Rob Herring]

On Fri, Sep 24, 2021 at 10:18 AM Oskar Senft <osk@google.com> wrote:
>
> Hi Rob
>
> > > +maintainers:
> > > +  - Guenter Roeck <linux@roeck-us.net>
> >
> > 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

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[Oskar Senft]

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/20210924114636.GB2694238@roeck-us.net/
[2] https://lore.kernel.org/linux-hwmon/CABoTLcQYHZbsgzXN7XXKQdDn8S-YsuE+ks9WShAEKcBJojEfcQ@mail.gmail.com/

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

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/20210924114636.GB2694238@roeck-us.net/
> [2] https://lore.kernel.org/linux-hwmon/CABoTLcQYHZbsgzXN7XXKQdDn8S-YsuE+ks9WShAEKcBJojEfcQ@mail.gmail.com/