On 05/12/2023 10:31, Ban Feng wrote: > Hi Krzysztof, > > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> On 04/12/2023 06:56, baneric926@xxxxxxxxx wrote: >>> From: Ban Feng <kcfeng0@xxxxxxxxxxx> >>> >>> This change documents the device tree bindings for the Nuvoton >>> NCT7362Y, NCT7363Y driver. >>> >>> Signed-off-by: Ban Feng <kcfeng0@xxxxxxxxxxx> >>> --- >>> .../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@xxxxxxxxxxx> >>> + >>> +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@xxxxxxxxxxxxxxxxx/ 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
