Re: [PATCH 5/7] dt-bindings: pinctrl: realtek: add RTD1315E pinctrl binding

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi TY Chang,

thanks for your patch!

On Wed, Jul 26, 2023 at 11:06 AM TY Chang <tychang@xxxxxxxxxxx> wrote:

> Add device tree bindings for RTD1315E.
>
> Signed-off-by: TY Chang <tychang@xxxxxxxxxxx>

Maybe you could write a short paragraph about the RTD1315E so we
know what this is? I guess it is some SoC with some intended use case?

(...)
> +description: |
> +  Binding for Realtek DHC RTD1315E SoC pin control.

Same text should go here in that case.

> +        realtek,pdrive:
> +          description: |
> +            An integer describing the level to adjust PMOS output driving capability.
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          minimum: 0
> +          maximum: 7
> +
> +        realtek,ndrive:
> +          description: |
> +            An integer describing the level to adjust NMOS output driving capability.
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          minimum: 0
> +          maximum: 7

I would rename these realtek,drive-strength-p and realtek,drive-strength-n.

You need to explain what is meant with PMOS and NMOS here. If it is what
I think it is, I think some ASCII art would be handy!

You can reuse my ASCII art from Documentation/driver-api/gpio/driver.rst:

                     VDD
                      |
            OD    ||--+
         +--/ ---o||     P-MOS-FET
         |        ||--+
    IN --+            +----- out
         |        ||--+
         +--/ ----||     N-MOS-FET
            OS    ||--+
                      |
                     GND

Maybe you wanna delete the OD switch if these drivers don't support that.

What does the values 0..7 actually correspond to? Is it the number of
transistors/driver stages simply? Then write that.

We need to think whether this is so generically useful that it should simply
be drive-strength-pmos and drive-strength-nmos, simply put, as other
SoCs may implement the same. What do people think?

> +        realtek,dcycle:
> +          description: |
> +            An integer describing the level to adjust output duty cycle.
> +            Valid arguments are described as below:
> +            0: 0ns
> +            2: + 0.25ns
> +            3: + 0.5ns
> +            4: -0.25ns
> +            5: -0.5ns
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          enum: [ 0, 2, 3, 4, 5 ]

This does not explain the duty cycle of *what*?

It looks really useful so please explain thoroughly what it does.

I guess this is not PWM because then you could use
PIN_CONFIG_MODE_PWM.

Yours,
Linus Walleij