Hi Linus,

Thanks for your review.

Hi TY Chang,

thanks for your patch!

On Wed, Jul 26, 2023 at 11:06 AM TY Chang [off-list ref] wrote:

quoted

Add device tree bindings for RTD1315E.

Signed-off-by: TY Chang <redacted>

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.

(...)

quoted

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

Same text should go here in that case.

quoted

+        realtek,pdrive:
+          description: |
+            An integer describing the level to adjust PMOS output driving

capability.

quoted

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

capability.

quoted

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

quoted

+        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