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] [day] [month] [year] [list]

Message-ID: <ba502655bea5481aaee9209195f2bf79@realtek.com>
Date:   Wed, 23 Aug 2023 07:56:48 +0000
From:   TY_Chang[張子逸] <tychang@...ltek.com>
To:     Linus Walleij <linus.walleij@...aro.org>
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" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 5/7] dt-bindings: pinctrl: realtek: add RTD1315E pinctrl binding

Hi Linus,

Thanks for your review.

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

I will add it in the next version.

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

Sure, I think naming them like that makes it clearer.

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

I will add these in the next version. The values 0..7 is the level of the driving strength. 
These leves can impact the rising/falling time of the waveform, assisting in achieving
the desired transfer speed.

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

This is not PWM. The duty cycle here is to adjust the proportion of positive and negative waveforms, and is adjusted in nanosecond(ns).

>Yours,
>Linus Walleij
>

Thanks,
TY Chang