diff mbox series

[v1,1/2] dt-bindings: hwmon: Add nct736x bindings

Message ID 20231204055650.788388-2-kcfeng0@nuvoton.com
State New
Headers show
Series [v1,1/2] dt-bindings: hwmon: Add nct736x bindings | expand

Commit Message

Ban Feng Dec. 4, 2023, 5:56 a.m. UTC 
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

Comments

Krzysztof Kozlowski Dec. 5, 2023, 3:49 p.m. UTC | #1 
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
diff mbox series

Patch

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