Re: [PATCH v4 1/3] dt-bindings: phy: Add Stingray USB PHY binding document

[Rob Herring]

On Sun, Mar 10, 2019 at 10:32 PM Srinath Mannam
<srinath.mannam@xxxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> Please find my comments below,
>
> On Sat, Feb 23, 2019 at 1:05 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > On Fri, Feb 22, 2019 at 11:29 AM Srinath Mannam
> > <srinath.mannam@xxxxxxxxxxxx> wrote:
> > >
> > > Hi Rob,
> > >
> > > Thanks for the review, Please find my comments below in line.
> > >
> > > On Fri, Feb 22, 2019 at 10:50 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Feb 20, 2019 at 04:04:00PM +0530, Srinath Mannam wrote:
> > > > > Add DT binding document for Stingray USB PHY.
> > > > >
> > > > > Signed-off-by: Srinath Mannam <srinath.mannam@xxxxxxxxxxxx>
> > > > > Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> > > > > Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
> > > > > ---
> > > > >  .../bindings/phy/brcm,stingray-usb-phy.txt         | 62 ++++++++++++++++++++++
> > > > >  1 file changed, 62 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/phy/brcm,stingray-usb-phy.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/phy/brcm,stingray-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,stingray-usb-phy.txt
> > > > > new file mode 100644
> > > > > index 0000000..da19236
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/phy/brcm,stingray-usb-phy.txt
> > > > > @@ -0,0 +1,62 @@
> > > > > +Broadcom Stingray USB PHY
> > > > > +
> > > > > +Required properties:
> > > > > + - compatible : should be one of the listed compatibles
> > > > > +     - "brcm,sr-usb-combo-phy" is a combo PHY has one SS PHY and one HS PHY.
> > > > > +     - "brcm,sr-usb-hs-phy" has a single HS PHY.
> > > > > + - reg: offset and length of the PHY blocks registers
> > > > > + - address-cells: should be 1
> > > > > + - size-cells: should be 0
> > > > > +
> > > > > +Sub-nodes:
> > > > > +  brcm,sr-usb-combo-phy have two sub-nodes for one SS PHY and one HS PHY.
> > > > > +
> > > > > +Sub-nodes required properties:
> > > > > + - reg: required for brcm,sr-usb-phy model PHY.
> > > > > +     reg value 0 is HS PHY and 1 is SS PHY.
> > > > > + - phy-cells: generic PHY binding; must be 0
> > > > > +
> > > > > +Refer to phy/phy-bindings.txt for the generic PHY binding properties
> > > > > +
> > > > > +Example:
> > > > > +     usbphy0: usb-phy@0 {
> > > > > +             compatible = "brcm,sr-usb-combo-phy";
> > > > > +             reg = <0x00000000 0x100>;
> > > > > +             #address-cells = <1>;
> > > > > +             #size-cells = <0>;
> > > > > +
> > > > > +             usb0_phy0: phy@0 {
> > > > > +                     reg = <0>;
> > > > > +                     #phy-cells = <0>;
> > > > > +             };
> > > > > +
> > > > > +             usb0_phy1: phy@1 {
> > > > > +                     reg = <1>;
> > > > > +                     #phy-cells = <0>;
> > > > > +             };
> > > >
> > > > Again, you don't need child nodes here. There are not any per child
> > > > resources. Clients can refer to <&usbphy0 1> just as easily as
> > > > <&usb0_phy1>. This is why we have #phy-cells.
> > > This phy controller is combo PHY it has one Super Speed USB PHY and
> > > one High Speed USB PHY.
> > > We required to create two PHY devices inside driver to initialize and
> > > service(reset) both SS and HS PHYs separately.
> > > That is the reason we used two child nodes.
> >
> > What you do in the driver is your business. That is independent of the
> > binding. Go look at other phy drivers which use #phy-cells=1.
> > .of_xlate() function is what converts the phy cells to a struct phy.
> >
> I have followed exactly same pattern available in open source.
> ex: Documentation/devicetree/bindings/phy/brcm-sata-phy.txt
> In this also, two child nodes are used with #phy-cells 0.

You'll notice DT maintainers did not review that binding (only changes
to it). There's no shortage of DT examples of how not to do things.

Rob