Message-ID: <ee65a9d6d40d4099987db5ff1ad1753f@realtek.com>
Date:   Thu, 1 Jun 2023 10:49:00 +0000
From:   Stanley Chang[昌育德] 
        <stanley_chang@...ltek.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Flavio Suligoi <f.suligoi@...m.it>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>,
        Douglas Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Ray Chi <raychi@...gle.com>,
        Michael Grzeschik <m.grzeschik@...gutronix.de>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: RE: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

Hi Krzysztof,

> Thank you for your patch. There is something to discuss/improve.
> 
> Actually a lot... The bindings are not suitable for review.

Thanks for your patience in reviewing my patches.

Most of the properties are about the phy parameters.
Is the phy parameter data suitable to be placed in DTS?
I referenced other phy drivers. 
These parameters should not be defined in dts.
I would move the parameters to the driver.

> > +  realtek,usb:
> > +    description: The phandler of realtek dwc3 node
> 
> "phandler"? Except obvious typo, drop "The phandler of" and describe what is
> it for.

realtek,usb is a phandle of syscon used to control realtek dwc3 register.

> > +    $ref: /schemas/types.yaml#/definitions/phandle
> 
> Anyway, it shouldn't be here. No, no.

Can I use it for phandle of syscon?

> > +  realtek,port-index:
> > +    description: The index of USB 2.0 PHY
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> No. No reason for this. You have reg.

This index is used for phy parameters. I will move it to phy driver.

> > +
> > +  realtek,phyN:
> > +    description: The total amount of USB 2.0 PHY
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> No. Compatible defines it.

This amount is used for phy parameters. I will move it to phy driver.


> > +
> > +  phy0:
> > +    description: The child node of PHY for the parameter v1.
> 
> ??? Open other phy bindings and use them as example.

phy0 Child node is defined to assign the phy parameter.
I will remove it.

> > +    type: object
> > +    properties:
> > +      realtek,phy-data-page0-size:
> > +        description: PHY data page 0 size
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      realtek,phy-data-page0-addr:
> > +        description: PHY data page 0 address
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > +      realtek,phy-data-page0-A00:
> > +        description: PHY data page 0 value
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > +      realtek,phy-data-page1-size:
> > +        description: PHY data page 1 size
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      realtek,phy-data-page1-addr:
> > +        description: PHY data page 1 address
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > +      realtek,phy-data-page1-A00:
> > +        description: PHY data page 1 value
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > +      realtek,phy-data-page2-size:
> > +        description: PHY data page 2 size
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      realtek,phy-data-page2-addr:
> > +        description: PHY data page 2 address
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > +      realtek,phy-data-page2-A00:
> > +        description: PHY data page 2 value
> > +        $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> > +      realtek,do-toggle:
> > +        description: Do PHY parameter toggle when port status change
> > +        type: boolean
> > +
> > +      realtek,check-efuse:
> > +        description: Enable to fix PHY parameter from reading otp table
> > +        type: boolean
> > +
> > +      realtek,use-default-parameter:
> > +        description: Don't set parameter and use default value
> > +        type: boolean
> > +
> > +      realtek,is-double-sensitivity-mode:
> > +        description: Enable double sensitivity mode
> > +        type: boolean
> > +
> > +      realtek,ldo-page0-e4-compensate:
> > +        description: Adjust the PHY parameter for page 0 0xE4 for ldo
> mode
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      realtek,page0-e4-compensate:
> > +        description: Adjust the PHY parameter for page 0 0xE4
> > +          for efuse table v2
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> I don't understand what's all this for. Most of these descriptions do not explain
> anything except duplicating name of property.

I'll simplify these properties and remove the one about the phy parameter.

> One huge NAK for these bindings. It looks like copy-paste from downstream
> stuff which should never be sent as is to upstream. I am sorry for being harsh,
> but amount of questions, coding and naming styles, incorrect choices is just too
> big to handle in one review.
> 

I will refactor this dts based on your comments.

Thanks,
Stanley

Powered by blists - more mailing lists