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

[Ban Feng <baneric926@gmail.com>]

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