Re: [PATCH v4 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Anand Moon <linux.amoon@gmail.com>]

Hi Conor,

On Fri, 24 Nov 2023 at 17:55, Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Nov 24, 2023 at 04:18:23PM +0530, Anand Moon wrote:
> > Hi Conor
> >
> > On Thu, 23 Nov 2023 at 23:26, Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Wed, Nov 22, 2023 at 11:53:46PM +0530, Anand Moon wrote:
> > > > Add the binding example for the USB3.1 Genesys Logic GL3523
> > > > integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> > > > hub.
> > > >
> > > > Onboard USB hub supports USB 3.x and USB 2.0 peer controllers.
> > > > which has a common reset pin and power supply.
> > > > peer-hub phandle each peer controller with proper gpio reset
> > > > and help each peer power on during initialization
> > > > and power off during suspend.
> > > >
> > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > ---
> > > > v4: Fix the description of peer-hub and update the commit message.
> > > > Schematics of the Odroid N2+
> > > > https://dn.odroid.com/S922X/ODROID-N2/Schematic/odroid-n2_rev0.6_20210121.pdf
> > > > V3: fix the dt_binding_check error, added new example for Genesys GL3523
> > > > v2: added Genesys GL3523 binding
> > > > v1: none
> > > > ---
> > > >  .../bindings/usb/genesys,gl850g.yaml          | 67 +++++++++++++++++--
> > > >  1 file changed, 63 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > index ee08b9c3721f..bc3b3f4c8473 100644
> > > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > @@ -9,9 +9,6 @@ title: Genesys Logic USB hub controller
> > > >  maintainers:
> > > >    - Icenowy Zheng <uwu@icenowy.me>
> > > >
> > > > -allOf:
> > > > -  - $ref: usb-device.yaml#
> > > > -
> > > >  properties:
> > > >    compatible:
> > > >      enum:
> > > > @@ -27,12 +24,48 @@ properties:
> > > >
> > > >    vdd-supply:
> > > >      description:
> > > > -      the regulator that provides 3.3V core power to the hub.
> > > > +      phandle to the regulator that provides power to the hub.
> > > > +
> > > > +  peer-hub:
> > >
> > > Should the property not be "peer-controller"? Your description refers to
> > > them as such.
> >
> > No, as per my understanding, peer-hub represents a complete USB hub.
> > See the lock diagram in the below link.
> >
> > >
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      onboard USB hub supports USB 3.x and USB 2.0 peer controllers.
> > >
> > >
> > > > +      which has a common reset pin and power supply.
> > > > +      peer-hub phandle each peer controller with proper gpio reset
>
> This is what I don't get. You say "peer-hub phandle each peer
> controller..". It is hard for me to understand that portion of the
> sentence, but the interchanging of "hub" and "controller" is
> confusing. The title of the binding says "hub controller", so maybe it
> is better to use that here.
>
> > > > +      and help each peer power on during initialization
> > > > +      and power off during suspend.
> > >
> > > I generally hate to talk about non-native speakers grammar etc, but what
> > > you have here is in need of a lot of improvement. The below is my
> > > attempt to understand what you are trying to say:
> > >
> > > "For onboard hubs that support USB 3.x and USB 2.0 controllers with
> > > shared resets and power supplies, this property is used to identify
> > > the controllers with which these are shared."
>
> "For onboard hub controllers that support USB 3.x and USB 2.0 hubs
> with shared resets and power supplies, this property is used to identify
> the hubs with which these are shared."
>
Thanks for your review comments.
Ok will update this in the next version.

> I re-worded this again to try and remove the use of "controller".
> Do you think that this still makes sense?
>
> > Sorry for the poor grammar, I will update this in the next v5.
> >
> > > Also - this is one particular system, what prevents there being a hub
> > > that has more than 2 controllers? Also, as you insist that this is
> > > generic, and not just for genesys, should this not be defined in a
> > > common location?
> >
> > Here is the block diagram of the Genesys GL3523 hub.
> > [0] https://www.genesyslogic.com.tw/en/product_view.php?show=67 [Block Diagram]
> >
> > It has two USB 2.0 and USB 3.1 controllers, so using peer-hub node
> > the onboard hub module will bring up this hub.
> >
> > There are many examples that use similar properties hence it is generic.
> >
> > # Documentation/devicetree/bindings/usb/cypress,hx3.yaml
> > # Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
> > # Documentation/devicetree/bindings/usb/realtek,rts5411.yaml
> > # Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > # Documentation/devicetree/bindings/usb/vialab,vl817.yaml
>
> Which brings me back to the unanswered question, should this not be
> defined in a common location given there are several devices using it?
> I assume because it only applies to hub controllers and not other types
> of devices.
>
> Also, the descriptions that I saw when looking at some of those other
> bindings are similarly poor. I can't bring myself to care any more,
> just clean up the ambiguous wording here and I'll ack the next version,
> I don't expect you to sort out the wording in other bindings.
>
Ok
> Cheers,
> Conor.

Thanks
-Anand