Re: [PATCH 1/3] dt-bindings: phy: add CIX Sky1 USB/DP combo PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: phy: cix,sky1-usbdp-phy: missing OF graph port for usb-switch
- [Low] dt-bindings: phy: cix,sky1-usbdp-phy: redundant usb-port and dp-port nodes
- [Low] dt-bindings: phy: cix,sky1-usbdp-phy: software policy in cix,default-conf
--

commit 04fdff6aa671993772ad90b645872bcdaa857848
Author: Gary Yang <gary.yang@cixtech.com>

dt-bindings: phy: add CIX Sky1 USB/DP combo PHY

This commit adds the Device Tree binding schema for the CIX Sky1 USB/DP
combo PHY, describing its USB and DisplayPort lane child nodes, Type-C
switch properties, and S5 system controller phandle.

> diff --git a/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml b/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
> new file mode 100644
> index 0000000000000..a5b6943f84ebd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
> @@ -0,0 +1,150 @@

[ ... ]

> +  orientation-switch:
> +    description:
> +      The PHY handles Type-C plug orientation switching.
> +    type: boolean
> +
> +  mode-switch:
> +    description:
> +      The PHY handles multiplexing of the USB and DisplayPort lanes.
> +    type: boolean

[Severity: Medium]
How does a Type-C connector or port controller link to this PHY's
orientation-switch and mode-switch without an OF graph port or ports node?

In the Type-C mux and switch subsystem,
fwnode_typec_switch_get()->fwnode_connection_find_matches() and
fwnode_typec_mux_get()->fwnode_connection_find_matches() match
orientation-switch in typec_switch_match() and mode-switch in
typec_mux_match() by traversing OF graph endpoints via
fwnode_graph_devcon_matches().

Because this schema sets additionalProperties: false without defining a port
or ports node, any board Device Tree that adds an OF graph connection from a
usb-c-connector to this PHY will fail dtbs_check validation, and without the
graph link the Type-C subsystem cannot resolve the switch or mux.

Should this binding reference /schemas/usb/usb-switch.yaml# (or
/schemas/usb/usb-switch-ports.yaml#) instead of redefining orientation-switch
and mode-switch locally without graph endpoints?

> +
> +  cix,usbphy-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to the S5 system controller
> +          - description: PHY instance index within the system controller
> +    description:
> +      Phandle to the system controller used for lane muxing and
> +      Type-C orientation control, followed by the PHY instance index.
> +
> +  cix,default-conf:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    enum: [0, 1, 2, 3]
> +    default: 3
> +    description: |
> +      Lane mode the PHY is put in until a consumer changes it through the
> +      Type-C mux. The value is a bit mask: 0 = none, 1 = USB, 2 = DP,
> +      3 = USB and DP.

[Severity: Low]
Does cix,default-conf describe a hardware characteristic, or is it encoding
initial driver software policy in Device Tree?

Which lane mode the driver configures prior to receiving a Type-C mux or PHY
consumer state update appears to be runtime driver policy rather than static
board or SoC hardware description. Could this property be dropped from the
binding?

> +
> +patternProperties:
> +  "^usb-port@[0-9a-f]+$":
> +    type: object
> +    description:
> +      The USB lane of the combo PHY.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +      "#phy-cells":
> +        const: 0
> +
> +    required:
> +      - reg
> +      - "#phy-cells"
> +
> +    additionalProperties: false
> +
> +  "^dp-port@[0-9a-f]+$":
> +    type: object
> +    description:
> +      The DisplayPort lane of the combo PHY.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +      "#phy-cells":
> +        const: 0
> +
> +    required:
> +      - reg
> +      - "#phy-cells"
> +
> +    additionalProperties: false

[Severity: Low]
Are the usb-port and dp-port child nodes necessary when neither subnode has
any dedicated Device Tree resources?

Each child node only contains reg and #phy-cells = <0>, with no per-lane
clocks, resets, power supplies, or distinct MMIO register ranges. Could the
parent PHY node set #phy-cells = <1> (for example, taking PHY_TYPE_USB3 and
PHY_TYPE_DP) and omit these child nodes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102082112.2306326-1-gary.yang@cixtech.com?part=1