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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]

On Sun, Nov 19, 2023 at 08:57:28PM +0530, Anand Moon wrote:
> Hi Conor,
> 
> On Sun, 19 Nov 2023 at 19:28, Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sun, Nov 19, 2023 at 08:04:50AM +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.
> >
> > But no comment in the commit message about the new property for the
> > "peer hub". $subject saying "dt-bindings: usb: Add the binding example
> > for the Genesys Logic GL3523 hub" is misleading when the meaningful
> > parts of the patch are unrelated to the example.
> >
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > > 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          | 63 +++++++++++++++++--
> > >  1 file changed, 59 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > index ee08b9c3721f..f8e88477fa11 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,44 @@ 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:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      phandle to the peer hub on the controller.
> >
> > What is this, why is it needed? Please explain it in your commit
> > message.
> >
> Ok, GL3523 integrates Genesys Logic self-developed USB 3.1 Gen 1
> Super Speed transmitter/receiver physical layer (PHY) and USB 2.0
> High-Speed PHY
> 
> peer-hub is used to cross-connect those phy nodes so that it can help
> hub power on/off simultaneously.

I said please explain it in your commit message, but on reflection I
think that would be insufficient. Extending the description to explain
what the peer-hub is would be great too. "peer hub on the controller"
doesn't seem to make sense to me either, as the peer hub phandle is to
another phy, not to the controller. I think that would probably also be
resolved by explaining what the peer hub is in a more detailed manner.

If this is purely a genesys thing, the property should grow a genesys,
prefix also.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]