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 2 potential issue(s) to consider:
- [Medium] dt-bindings: phy: cix,sky1-usbdp-phy: missing OF graph port/ports for Type-C
- [Low] dt-bindings: phy: cix,sky1-usbdp-phy: dummy child nodes and policy property
--

commit 861d429c69ca8c486ffb6fda39bbf7f07f828ee7
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 SoC
USB/DP combo PHY, describing its clocks, resets, system controller phandle,
Type-C switch properties, and child nodes for the USB and DisplayPort lanes.

> 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
[ ... ]
> +  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
[ ... ]
> +  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.
> +
> +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]
Why are child nodes and cix,default-conf needed in the binding here?

The usb-port and dp-port child nodes only contain reg and #phy-cells = <0>
with no per-lane hardware resources (such as separate clocks, resets,
regulators, or OF graph ports), and the driver in cix_udphy_probe() does not
read the child reg property at all. When combo PHY sub-lanes have no
independent DT resources, could the parent PHY node expose both lanes
directly using #phy-cells = <1> (for example taking PHY_TYPE_USB3 and
PHY_TYPE_DP) instead of requiring #address-cells, #size-cells, and dummy
child nodes with arbitrary unit addresses?

Similarly, cix,default-conf describes the initial software lane mode prior
to Type-C mux updates rather than a hardware property of the PHY, and both
the binding and driver already default to 3. Could cix,default-conf be
dropped from the binding so the driver initializes its default state
directly?

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - clocks
> +  - resets
> +  - reset-names
> +  - cix,usbphy-syscon
> +
> +additionalProperties: false

[Severity: Medium]
How can a Type-C connector or controller link to this PHY when
orientation-switch and mode-switch are used?

In the Type-C mux and switch core (fwnode_typec_switch_get() ->
fwnode_connection_find_matches() -> fwnode_graph_devcon_matches() ->
typec_switch_match() and fwnode_typec_mux_get() -> typec_mux_match() in
drivers/usb/typec/mux.c), the boolean orientation-switch and mode-switch
properties on a provider node are matched by traversing OF graph endpoints
(port or ports) from the Type-C connector node to the provider node:

drivers/usb/typec/mux.c:typec_switch_match() {
	if (id && !fwnode_property_present(fwnode, id))
		return NULL;
	...
}

However, Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
defines orientation-switch and mode-switch inline and sets
additionalProperties: false without defining port or ports properties or
referencing /schemas/usb/usb-switch.yaml# and
/schemas/usb/usb-switch-ports.yaml#.

As a result, any board Device Tree that connects a Type-C connector to this
PHY via OF graph will fail dtbs_check validation, while omitting the OF
graph links prevents fwnode_typec_switch_get() and fwnode_typec_mux_get()
from discovering the PHY's switch and mux at runtime. Should this schema
reference usb-switch.yaml and define the required OF graph port/ports nodes?

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