RE: [PATCH v6 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Stanley Chang[昌育德]]

Hi Conor,

> On Fri, Jun 30, 2023 at 07:33:45AM +0000, Stanley Chang[昌育德] wrote:
> > Hi Rob,
> >
> > > On Thu, Jun 29, 2023 at 01:45:12PM +0800, Stanley Chang wrote:
> > > > Add the documentation explain the property about Realtek USB PHY driver.
> > >
> > > In the subject, drop "the doc about the". It's redundant. And perhaps add 'DHC
> > > RTD SoC' if this isn't for *all* Realtek SoCs.
> > >
> > > > Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> > > > controller. Added the driver to drive the USB 2.0 PHY transceivers.
> > >
> > > driver? This is a binding for the h/w.
> >
> > I mean, the driver is drivers/phy/realtek/phy-rtk-usb2.c
> > I will revise as
> >     dt-bindings: phy: realtek: Add the Realtek DHC RTD SoC USB 2.0 PHY
> >
> >     Add the documentation explain the property about Realtek USB PHY driver.
> >
> >     Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> >     controller and uses phy-rtk-usb2 as driver for USB 2.0 PHY transceiver..
> 
> No, you should mention nothing to do with how a particular operating
> system chooses to structure its code here. Bindings describe hardware,
> and the commit message should reflect that.

Okay. I should remove any related driver description.
Here, only describe the hardware about SoCs.
For example,
     dt-bindings: phy: realtek: Add the Realtek DHC RTD SoC USB 2.0 PHY
 
     Document the USB PHY bindings for Realtek SoCs.
     Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
     controller and using USB 2.0 PHY transceiver.

> >
> > > > +$id: http://devicetree.org/schemas/phy/realtek,usb2phy.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Realtek DHC SoCs USB 2.0 PHY
> > > > +
> > > > +maintainers:
> > > > +  - Stanley Chang <stanley_chang@xxxxxxxxxxx>
> > > > +
> > > > +description:
> > >
> > > You need '|' if formatting (line breaks) are important.
> >
> > I think I need it. I will add it.
> >
> > > > +  realtek,inverse-hstx-sync-clock:
> > > > +    description:
> > > > +      For one of the phys of RTD1619b SoC, the synchronous clock of the
> > > > +      high-speed tx must be inverted.
> > >
> > > "invert" assumes I know what non-inverted means. I do not. Better to state in
> > > terms of active high, low, falling edge, rising edge, etc.
> >
> > Meaning, the clock must be reversed.
> 
> Maybe that means something to Rob, but "reversed" doesn't seem any more
> meaningful than inverse. I agree that it should be described in terms of
> "active high" etc, as they have well understood meanings.

Here I describe a clock sequence is "inverted", not "high" or "low".
Add the property, then hstx-sync-clock sequence is being inverted.

> > > > +    type: boolean
> > > > +
> > > > +  realtek,driving-level:
> > > > +    description:
> > > > +      Control the magnitude of High speed Dp/Dm output swing.
> > > > +      For a different board or port, the original magnitude maybe not
> > > meet
> > > > +      the specification. In this situation we can adjust the value to meet
> > > > +      the specification.
> > >
> > > What are the units?
> >
> > There is no unit. It is only a gain for adjusting the magnitude.
> 
> Gain has units too.

The magnitude of High speed Dp/Dm output is voltage.

> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    default: 8
> > > > +    minimum: 0
> > > > +    maximum: 31
> > > > +
> > > > +  realtek,driving-compensate:
> > >
> > > compensate what?
> >
> > It is to compensate the driving level.
> 
> Should it be called "driving-level-compensate" then?
Yes, it can be named "driving-level-compensate".
I will rename it.

> > In other word, to adjust the driving level.
> 
> So, "realtek,driving-level" sets the gain and
> "realtek,driving-compensate" adjusts the driving level.
> By that logic, is this also a gain?

Yes, the real driving level is
driving level (reading from otp) + driving-level-compensate.

> Also this property is only for the RTD1315e? That should be
> described in/constrained by the binding itself, not in the text
> description alone IMO.
> 

Does your meaning is as follows?
if:
  properties:
    compatible:
      contains:
        enum:
          - realtek,rtd1315e-usb2phy
then:
  properties:
    realtek,driving-level-compensate:
      description:
        For RTD1315e SoC, the driving level can be adjusted by reading the
        efuse table. This property provides drive compensation.
        If the magnitude of High speed Dp/Dm output swing still not meet the
        specification, then we can set this value to meet the specification.
      $ref: /schemas/types.yaml#/definitions/int32
      default: 0
      minimum: -8
      maximum: 8

Thanks,
Stanley