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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdYzLiXSLpU63Nn84b+p3Nz5Ls-o94HsoAq514LvGkSiVg@mail.gmail.com>
Date:   Mon, 7 Aug 2023 14:52:36 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     TY Chang <tychang@...ltek.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] dt-bindings: pinctrl: realtek: add RTD1315E pinctrl binding

Hi TY Chang,

thanks for your patch!

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

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

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