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