| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANAwSgSRp96fjGfHw2rRNW8S3iKSzL7QTBV4N8_+vPxDoos1gA@mail.gmail.com>
Date: Fri, 24 Nov 2023 22:22:26 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Icenowy Zheng <uwu@...nowy.me>,
Neil Armstrong <neil.armstrong@...aro.org>,
linux-amlogic@...ts.infradead.org, linux-usb@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] dt-bindings: usb: Add the binding example for the
Genesys Logic GL3523 hub
Hi Conor,
On Fri, 24 Nov 2023 at 17:55, Conor Dooley <conor@...nel.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@...nel.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@...il.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@...nowy.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
Powered by blists - more mailing lists