Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Johan Hovold]

On Fri, Oct 04, 2024 at 04:57:37PM +0300, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB, but also
> connector orientation handling between.
> 
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>

> +  clocks:
> +    items:
> +      - description: XO Clock
> +
> +  clock-names:
> +    items:
> +      - const: xo

> +  vdd-supply:
> +    description: power supply (1.07V)
> +
> +  vdd33-supply:
> +    description: power supply (3.3V)
> +
> +  vdd33-cap-supply:
> +    description: power supply (3.3V)
> +
> +  vddar-supply:
> +    description: power supply (1.07V)
> +
> +  vddat-supply:
> +    description: power supply (1.07V)
> +
> +  vddio-supply:
> +    description: power supply (1.2V or 1.8V)

> +required:
> +  - compatible
> +  - reg

Presumably all of the supplies are also required.

Similar for clocks, etc.

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        typec-mux@8 {
> +            compatible = "parade,ps8830";
> +            reg = <0x8>;
> +
> +            vdd-supply = <&vreg_rtmr_1p15>;
> +            vdd33-supply = <&vreg_rtmr_3p3>;
> +            vdd33-cap-supply = <&vreg_rtmr_3p3>;
> +            vddar-supply = <&vreg_rtmr_1p15>;
> +            vddat-supply = <&vreg_rtmr_1p15>;
> +            vddio-supply = <&vreg_rtmr_1p8>;
> +
> +            reset-gpios = <&pm8550_gpios 10 GPIO_ACTIVE_HIGH>;

The reset line is active low.

> +
> +            retimer-switch;
> +            orientation-switch;
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;

Please add a newline before each child node for readability.

> +                    usb_con_ss: endpoint {

We typically avoid adding unused labels to the examples, but perhaps
here it serves as documentation?

> +                        remote-endpoint = <&typec_con_ss>;
> +                    };
> +                };

Add newline here too, and similar below.

> +                port@1 {
> +                    reg = <1>;
> +                    phy_con_ss: endpoint {
> +                        remote-endpoint = <&usb_phy_ss>;
> +                        data-lanes = <3 2 1 0>;
> +                    };
> +                };
> +                port@2 {
> +                    reg = <2>;
> +                    usb_con_sbu: endpoint {
> +                        remote-endpoint = <&typec_dp_aux>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> +...

Johan