Message ID | 20210910130337.2025426-1-osk@google.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | dt-bindings: hwmon: Add nct7802 bindings | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | fail | build log |
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"; > + }; > + }; >
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.
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.
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. >
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. >
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.
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
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.
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. >
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.
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. >
> 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.
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
> 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.
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.
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
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"; + }; + };
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