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

[Conor Dooley]

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.

> +    $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
> +      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."

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?

Cheers,
Conor.

>  
>  required:
>    - compatible
>    - reg
>  
> +allOf:
> +  - $ref: usb-device.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - usb5e3,608
> +    then:
> +      properties:
> +        peer-hub: false
> +        vdd-supply: false
> +        reset-gpios: true
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - usb5e3,610
> +              - usb5e3,620
> +    then:
> +      properties:
> +        peer-hub: true
> +        vdd-supply: true
> +        reset-gpios: true
> +
>  additionalProperties: false
>  
>  examples:
> @@ -49,3 +82,29 @@ examples:
>              reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    usb {
> +        dr_mode = "host";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* 2.0 hub on port 1 */
> +        hub_2_0: hub@1 {
> +            compatible = "usb5e3,610";
> +            reg = <1>;
> +            peer-hub = <&hub_3_0>;
> +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +            vdd-supply = <&vcc_5v>;
> +        };
> +
> +        /* 3.1 hub on port 4 */
> +        hub_3_0: hub@2 {
> +            compatible = "usb5e3,620";
> +            reg = <2>;
> +            peer-hub = <&hub_2_0>;
> +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +            vdd-supply = <&vcc_5v>;
> +        };
> +    };
> -- 
> 2.42.0
>

Attachment: signature.asc
Description: PGP signature