Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Mon, 20 Nov 2023 15:45:39 +0000
From:   Conor Dooley <conor@kernel.org>
To:     Anand Moon <linux.amoon@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Icenowy Zheng <uwu@icenowy.me>, linux-usb@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the
 Genesys Logic GL3523 hub
Message-ID: <20231120-grinch-upbeat-05f7a32a99fa@spud>
References: <20231119023454.1591-1-linux.amoon@gmail.com>
 <20231119023454.1591-2-linux.amoon@gmail.com>
 <20231119-phrasing-reverse-bbc1fde515d5@spud>
 <CANAwSgQ6H9FUEBKz7sCf4kUZSMnCfyXG-cpGTMZoT15W9187Kg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="i584DEwwEuaJNfQd"
Content-Disposition: inline
In-Reply-To: <CANAwSgQ6H9FUEBKz7sCf4kUZSMnCfyXG-cpGTMZoT15W9187Kg@mail.gmail.com>
Precedence: bulk


--i584DEwwEuaJNfQd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Nov 19, 2023 at 08:57:28PM +0530, Anand Moon wrote:
> Hi Conor,
>=20
> 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 GL3=
523
> > > 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.yam=
l 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
>=20
> 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.

--i584DEwwEuaJNfQd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZVt/IwAKCRB4tDGHoIJi
0l7WAP4jO2VytV+gaR4Fr/4ZJpmrfQ2P5ysd6CPf71fn0gDBSgEA0EeG15JAP0T2
UF4SNHfWgFn93keokaU5a5twE8yTlQA=
=o5XT
-----END PGP SIGNATURE-----

--i584DEwwEuaJNfQd--