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

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

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.

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
(---).

> +
> +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.

> +
> +  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.

> +
> +  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.

> +
> +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




Best regards,
Krzysztof

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

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

Hi Krzysztof,

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


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
>