Hi Krzysztof, On Tuesday, December 5, 2023, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 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@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. 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@xxxxxxxxxxxxxxxxx/ > > 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 >
