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

[Ban Feng]

Hi Krzysztof,

On Tuesday, December 5, 2023, Krzysztof Kozlowski
<krzysztof.kozlowski at 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 at linaro.org> wrote:
> >>
> >> On 04/12/2023 06:56, baneric926 at gmail.com wrote:
> >>> From: Ban Feng <kcfeng0 at nuvoton.com>
> >>>
> >>> This change documents the device tree bindings for the Nuvoton
> >>> NCT7362Y, NCT7363Y driver.
> >>>
> >>> Signed-off-by: Ban Feng <kcfeng0 at 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 at 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
>