Message ID | 20231204055650.788388-2-kcfeng0@nuvoton.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | hwmon: Driver for Nuvoton NCT736X | expand |
On 04/12/2023 06:56, baneric926@gmail.com wrote: > From: Ban Feng <kcfeng0@nuvoton.com> > > This change documents the device tree bindings for the Nuvoton > NCT7362Y, NCT7363Y driver. > > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com> > --- > .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++ > MAINTAINERS | 6 ++ > 2 files changed, 86 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > new file mode 100644 > index 000000000000..f98fd260a20f > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > @@ -0,0 +1,80 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > + > +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nuvoton NCT736X Hardware Monitoring IC > + > +maintainers: > + - Ban Feng <kcfeng0@nuvoton.com> > + > +description: | > + The NCT736X is a Fan controller which provides up to 16 independent > + FAN input monitors, and up to 16 independent PWM output with SMBus interface. > + Besides, NCT7363Y has a built-in watchdog timer which is used for > + conditionally generating a system reset output (INT#). > + > +additionalProperties: false Please place it just like other bindings are placing it. Not in some random order. See example-schema. You should use common fan properties. If it was not merged yet, you must rebase on patchset on LKML and mention the dependency in the change log (---). > + > +properties: > + compatible: > + enum: > + - nuvoton,nct7362 > + - nuvoton,nct7363 > + > + reg: > + maxItems: 1 > + > + nuvoton,pwm-mask: > + description: | > + each bit means PWMx enable/disable setting, where x = 0~15. > + 0: disabled, 1: enabled > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0x0 > + maximum: 0xFFFF > + default: 0x0 Use pwms, not own property for this. > + > + nuvoton,fanin-mask: > + description: | > + each bit means FANINx monitoring enable/disable setting, > + where x = 0~15. > + 0: disabled, 1: enabled > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0x0 > + maximum: 0xFFFF > + default: 0x0 Use properties from common fan bindings. > + > + nuvoton,wdt-timeout: > + description: | > + Watchdog Timer time configuration for NCT7363Y, as below > + 0: 15 sec (default) > + 1: 7.5 sec > + 2: 3.75 sec > + 3: 30 sec > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2, 3] > + default: 0 Nope, reference watchdog.yaml and use its properties. See other watchdog bindings for examples. > + > +required: > + - compatible > + - reg > + - nuvoton,pwm-mask > + - nuvoton,fanin-mask > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + nct7363@22 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Best regards, Krzysztof
Hi Krzysztof, On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 04/12/2023 06:56, baneric926@gmail.com wrote: > > From: Ban Feng <kcfeng0@nuvoton.com> > > > > This change documents the device tree bindings for the Nuvoton > > NCT7362Y, NCT7363Y driver. > > > > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com> > > --- > > .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++ > > MAINTAINERS | 6 ++ > > 2 files changed, 86 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > > > > diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > > new file mode 100644 > > index 000000000000..f98fd260a20f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > > @@ -0,0 +1,80 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > + > > +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Nuvoton NCT736X Hardware Monitoring IC > > + > > +maintainers: > > + - Ban Feng <kcfeng0@nuvoton.com> > > + > > +description: | > > + The NCT736X is a Fan controller which provides up to 16 independent > > + FAN input monitors, and up to 16 independent PWM output with SMBus interface. > > + Besides, NCT7363Y has a built-in watchdog timer which is used for > > + conditionally generating a system reset output (INT#). > > + > > +additionalProperties: false > > Please place it just like other bindings are placing it. Not in some > random order. See example-schema. ok, I'll move additionalProperties to the correct place. > > You should use common fan properties. If it was not merged yet, you must > rebase on patchset on LKML and mention the dependency in the change log > (---). please kindly see below > > > + > > +properties: > > + compatible: > > + enum: > > + - nuvoton,nct7362 > > + - nuvoton,nct7363 > > + > > + reg: > > + maxItems: 1 > > + > > + nuvoton,pwm-mask: > > + description: | > > + each bit means PWMx enable/disable setting, where x = 0~15. > > + 0: disabled, 1: enabled > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 0x0 > > + maximum: 0xFFFF > > + default: 0x0 > > Use pwms, not own property for this. NCT736X has 16 funtion pins, they can be GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO. We would like to add such a property that can configure the function pin for pin0~7 and pin10~17. My original design is only for PWM/FANIN, however, I noticed that we can change the design to "nuvoton,pin[0-7]$" and "nuvoton,pin[10-17]$", like example in adt7475.yaml. I'm not sure if this can be accepted or not, please kindly review this. > > > + > > + nuvoton,fanin-mask: > > + description: | > > + each bit means FANINx monitoring enable/disable setting, > > + where x = 0~15. > > + 0: disabled, 1: enabled > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 0x0 > > + maximum: 0xFFFF > > + default: 0x0 > > Use properties from common fan bindings. Ditto > > > + > > + nuvoton,wdt-timeout: > > + description: | > > + Watchdog Timer time configuration for NCT7363Y, as below > > + 0: 15 sec (default) > > + 1: 7.5 sec > > + 2: 3.75 sec > > + 3: 30 sec > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1, 2, 3] > > + default: 0 > > Nope, reference watchdog.yaml and use its properties. See other watchdog > bindings for examples. NCT7363 has a built-in watchdog timer which is used for conditionally generating a system reset output (INT#) if the microcontroller or microprocessor stops to periodically send a pulse signal or interface communication access signal like SCL signal from SMBus interface. We only consider "Watchdog Timer Configuration Register" enable bit and timeout setting. Should we still need to add struct watchdog_device to struct nct736x_data? > > > + > > +required: > > + - compatible > > + - reg > > + - nuvoton,pwm-mask > > + - nuvoton,fanin-mask > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + nct7363@22 { > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation ok, I'll change nct7363@22 to hwmon@22. Thanks, Ban
On 05/12/2023 10:31, Ban Feng wrote: > Hi Krzysztof, > > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 04/12/2023 06:56, baneric926@gmail.com wrote: >>> From: Ban Feng <kcfeng0@nuvoton.com> >>> >>> This change documents the device tree bindings for the Nuvoton >>> NCT7362Y, NCT7363Y driver. >>> >>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com> >>> --- >>> .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++ >>> MAINTAINERS | 6 ++ >>> 2 files changed, 86 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml >>> new file mode 100644 >>> index 000000000000..f98fd260a20f >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml >>> @@ -0,0 +1,80 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> + >>> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Nuvoton NCT736X Hardware Monitoring IC >>> + >>> +maintainers: >>> + - Ban Feng <kcfeng0@nuvoton.com> >>> + >>> +description: | >>> + The NCT736X is a Fan controller which provides up to 16 independent >>> + FAN input monitors, and up to 16 independent PWM output with SMBus interface. >>> + Besides, NCT7363Y has a built-in watchdog timer which is used for >>> + conditionally generating a system reset output (INT#). >>> + >>> +additionalProperties: false >> >> Please place it just like other bindings are placing it. Not in some >> random order. See example-schema. > > ok, I'll move additionalProperties to the correct place. > >> >> You should use common fan properties. If it was not merged yet, you must >> rebase on patchset on LKML and mention the dependency in the change log >> (---). > > please kindly see below > >> >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - nuvoton,nct7362 >>> + - nuvoton,nct7363 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + nuvoton,pwm-mask: >>> + description: | >>> + each bit means PWMx enable/disable setting, where x = 0~15. >>> + 0: disabled, 1: enabled >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + minimum: 0x0 >>> + maximum: 0xFFFF >>> + default: 0x0 >> >> Use pwms, not own property for this. > > NCT736X has 16 funtion pins, they can be > GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO. > We would like to add such a property that can configure the function > pin for pin0~7 and pin10~17. It looks you are writing custom pinctrl instead of using standard bindings and frameworks. > > My original design is only for PWM/FANIN, however, > I noticed that we can change the design to "nuvoton,pin[0-7]$" and > "nuvoton,pin[10-17]$", like example in adt7475.yaml. > I'm not sure if this can be accepted or not, please kindly review this. It looks like the same problem as with other fan/Nuvoton bindings we discussed some time ago on the lists. Please do not duplicate threads, work and reviews: https://lore.kernel.org/all/20230607101827.8544-5-zev@bewilderbeest.net/ I already gave same comments there. >>> + nuvoton,wdt-timeout: >>> + description: | >>> + Watchdog Timer time configuration for NCT7363Y, as below >>> + 0: 15 sec (default) >>> + 1: 7.5 sec >>> + 2: 3.75 sec >>> + 3: 30 sec >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1, 2, 3] >>> + default: 0 >> >> Nope, reference watchdog.yaml and use its properties. See other watchdog >> bindings for examples. > > NCT7363 has a built-in watchdog timer which is used for conditionally > generating a system reset > output (INT#) if the microcontroller or microprocessor stops to > periodically send a pulse signal or > interface communication access signal like SCL signal from SMBus interface. > > We only consider "Watchdog Timer Configuration Register" enable bit > and timeout setting. > Should we still need to add struct watchdog_device to struct nct736x_data? I do not see correlation between watchdog.yaml and some struct. I did not write anything about driver, so I don't understand your comments. Anyway, I don't like that we are duplicating entire effort and our review. Please join existing discussions, threads and build on top of it. Best regards, Krzysztof
Hi Krzysztof, On Tuesday, December 5, 2023, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/12/2023 10:31, Ban Feng wrote: > > Hi Krzysztof, > > > > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 04/12/2023 06:56, baneric926@gmail.com wrote: > >>> From: Ban Feng <kcfeng0@nuvoton.com> > >>> > >>> This change documents the device tree bindings for the Nuvoton > >>> NCT7362Y, NCT7363Y driver. > >>> > >>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com> > >>> --- > >>> .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++ > >>> MAINTAINERS | 6 ++ > >>> 2 files changed, 86 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > >>> new file mode 100644 > >>> index 000000000000..f98fd260a20f > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml > >>> @@ -0,0 +1,80 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> + > >>> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Nuvoton NCT736X Hardware Monitoring IC > >>> + > >>> +maintainers: > >>> + - Ban Feng <kcfeng0@nuvoton.com> > >>> + > >>> +description: | > >>> + The NCT736X is a Fan controller which provides up to 16 independent > >>> + FAN input monitors, and up to 16 independent PWM output with SMBus interface. > >>> + Besides, NCT7363Y has a built-in watchdog timer which is used for > >>> + conditionally generating a system reset output (INT#). > >>> + > >>> +additionalProperties: false > >> > >> Please place it just like other bindings are placing it. Not in some > >> random order. See example-schema. > > > > ok, I'll move additionalProperties to the correct place. > > > >> > >> You should use common fan properties. If it was not merged yet, you must > >> rebase on patchset on LKML and mention the dependency in the change log > >> (---). > > > > please kindly see below > > > >> > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - nuvoton,nct7362 > >>> + - nuvoton,nct7363 > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + nuvoton,pwm-mask: > >>> + description: | > >>> + each bit means PWMx enable/disable setting, where x = 0~15. > >>> + 0: disabled, 1: enabled > >>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>> + minimum: 0x0 > >>> + maximum: 0xFFFF > >>> + default: 0x0 > >> > >> Use pwms, not own property for this. > > > > NCT736X has 16 funtion pins, they can be > > GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO. > > We would like to add such a property that can configure the function > > pin for pin0~7 and pin10~17. > > It looks you are writing custom pinctrl instead of using standard > bindings and frameworks. Please kindly see below > > > > > > My original design is only for PWM/FANIN, however, > > I noticed that we can change the design to "nuvoton,pin[0-7]$" and > > "nuvoton,pin[10-17]$", like example in adt7475.yaml. > > I'm not sure if this can be accepted or not, please kindly review this. > > It looks like the same problem as with other fan/Nuvoton bindings we > discussed some time ago on the lists. > > Please do not duplicate threads, work and reviews: > https://lore.kernel.org/all/20230607101827.8544-5-zev@bewilderbeest.net/ > > I already gave same comments there. Thanks for your kindly sharing. I noticed that [1] defines some useful properties, pwms and tach-ch, like you mentioned. Therefore, I'll modify our design to follow the common fan properties in v2. [1] https://lists.openwall.net/linux-kernel/2023/11/07/406 > > > >>> + nuvoton,wdt-timeout: > >>> + description: | > >>> + Watchdog Timer time configuration for NCT7363Y, as below > >>> + 0: 15 sec (default) > >>> + 1: 7.5 sec > >>> + 2: 3.75 sec > >>> + 3: 30 sec > >>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>> + enum: [0, 1, 2, 3] > >>> + default: 0 > >> > >> Nope, reference watchdog.yaml and use its properties. See other watchdog > >> bindings for examples. > > > > NCT7363 has a built-in watchdog timer which is used for conditionally > > generating a system reset > > output (INT#) if the microcontroller or microprocessor stops to > > periodically send a pulse signal or > > interface communication access signal like SCL signal from SMBus interface. > > > > We only consider "Watchdog Timer Configuration Register" enable bit > > and timeout setting. > > Should we still need to add struct watchdog_device to struct nct736x_data? > > I do not see correlation between watchdog.yaml and some struct. I did > not write anything about driver, so I don't understand your comments. > > Anyway, I don't like that we are duplicating entire effort and our > review. Please join existing discussions, threads and build on top of it. > Thanks, I'll remove unused function in hwmon subsystem, and consider a watchdog subsystem design if necessary. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml new file mode 100644 index 000000000000..f98fd260a20f --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NCT736X Hardware Monitoring IC + +maintainers: + - Ban Feng <kcfeng0@nuvoton.com> + +description: | + The NCT736X is a Fan controller which provides up to 16 independent + FAN input monitors, and up to 16 independent PWM output with SMBus interface. + Besides, NCT7363Y has a built-in watchdog timer which is used for + conditionally generating a system reset output (INT#). + +additionalProperties: false + +properties: + compatible: + enum: + - nuvoton,nct7362 + - nuvoton,nct7363 + + reg: + maxItems: 1 + + nuvoton,pwm-mask: + description: | + each bit means PWMx enable/disable setting, where x = 0~15. + 0: disabled, 1: enabled + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0x0 + maximum: 0xFFFF + default: 0x0 + + nuvoton,fanin-mask: + description: | + each bit means FANINx monitoring enable/disable setting, + where x = 0~15. + 0: disabled, 1: enabled + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0x0 + maximum: 0xFFFF + default: 0x0 + + nuvoton,wdt-timeout: + description: | + Watchdog Timer time configuration for NCT7363Y, as below + 0: 15 sec (default) + 1: 7.5 sec + 2: 3.75 sec + 3: 30 sec + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + default: 0 + +required: + - compatible + - reg + - nuvoton,pwm-mask + - nuvoton,fanin-mask + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + nct7363@22 { + compatible = "nuvoton,nct7363"; + reg = <0x22>; + + nuvoton,pwm-mask = <0x003F>; + nuvoton,fanin-mask = <0x003F>; + nuvoton,wdt-timeout = <0x3>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 012df8ccf34e..eef44c13278c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14900,6 +14900,12 @@ S: Maintained F: Documentation/devicetree/bindings/hwmon/nuvoton,nct6775.yaml F: drivers/hwmon/nct6775-i2c.c +NCT736X HARDWARE MONITOR DRIVER +M: Ban Feng <kcfeng0@nuvoton.com> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml + NETDEVSIM M: Jakub Kicinski <kuba@kernel.org> S: Maintained