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

[Krzysztof Kozlowski]

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