* [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings @ 2021-09-21 0:46 Oskar Senft 2021-09-21 0:46 ` [PATCH v2 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft 2021-09-23 21:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Rob Herring 0 siblings, 2 replies; 8+ messages in thread From: Oskar Senft @ 2021-09-21 0:46 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Rob Herring, linux-hwmon, linux-kernel, devicetree Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-09-21 0:46 [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Oskar Senft @ 2021-09-21 0:46 ` Oskar Senft 2021-09-23 21:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Rob Herring 1 sibling, 0 replies; 8+ messages in thread From: Oskar Senft @ 2021-09-21 0:46 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Rob Herring, linux-hwmon, linux-kernel, devicetree Cc: 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, ®); 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-09-21 0:46 [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Oskar Senft 2021-09-21 0:46 ` [PATCH v2 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft @ 2021-09-23 21:37 ` Rob Herring 2021-09-24 15:17 ` Oskar Senft 1 sibling, 1 reply; 8+ messages in thread From: Rob Herring @ 2021-09-23 21:37 UTC (permalink / raw) To: Oskar Senft Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel, devicetree 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-09-23 21:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Rob Herring @ 2021-09-24 15:17 ` Oskar Senft 2021-09-25 13:36 ` Guenter Roeck 2021-09-27 15:21 ` Rob Herring 0 siblings, 2 replies; 8+ messages in thread From: Oskar Senft @ 2021-09-24 15:17 UTC (permalink / raw) To: Rob Herring Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel, devicetree 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-09-24 15:17 ` Oskar Senft @ 2021-09-25 13:36 ` Guenter Roeck 2021-09-27 15:21 ` Rob Herring 1 sibling, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2021-09-25 13:36 UTC (permalink / raw) To: Oskar Senft Cc: Rob Herring, Jean Delvare, linux-hwmon, linux-kernel, devicetree 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-09-24 15:17 ` Oskar Senft 2021-09-25 13:36 ` Guenter Roeck @ 2021-09-27 15:21 ` Rob Herring 2021-10-08 13:07 ` Oskar Senft 1 sibling, 1 reply; 8+ messages in thread From: Rob Herring @ 2021-09-27 15:21 UTC (permalink / raw) To: Oskar Senft Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, linux-kernel, devicetree 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-09-27 15:21 ` Rob Herring @ 2021-10-08 13:07 ` Oskar Senft 2021-10-08 13:25 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Oskar Senft @ 2021-10-08 13:07 UTC (permalink / raw) To: Rob Herring Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, linux-kernel, devicetree 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-10-08 13:07 ` Oskar Senft @ 2021-10-08 13:25 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2021-10-08 13:25 UTC (permalink / raw) To: Oskar Senft Cc: Rob Herring, Jean Delvare, Linux HWMON List, linux-kernel, devicetree 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-08 13:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-21 0:46 [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Oskar Senft 2021-09-21 0:46 ` [PATCH v2 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft 2021-09-23 21:37 ` [PATCH v2 1/2] dt-bindings: hwmon: Add nct7802 bindings Rob Herring 2021-09-24 15:17 ` Oskar Senft 2021-09-25 13:36 ` Guenter Roeck 2021-09-27 15:21 ` Rob Herring 2021-10-08 13:07 ` Oskar Senft 2021-10-08 13:25 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.