Hi Conor, On Fri, 24 Nov 2023 at 17:55, Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxx> > > > > > > > > -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
