Re: [PATCH v6 1/3] dt-bindings: usb: Add CIX Sky1 USBSSP controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] dt-bindings: soc: cix: non-standard #syscon-cells property in system-control
- [Low] dt-bindings: usb: cix,sky1-usbssp: redundant unprefixed disable properties
--

commit 6118aaded5d0bbade230e227b29df8b8fcf97408
Author: Hongliang Yang (杨宏亮) <hongliang.yang@cixtech.com>

dt-bindings: usb: Add CIX Sky1 USBSSP controller

This commit adds the Device Tree binding schema for the CIX Sky1 Cadence
USBSSP controller and updates the CIX Sky1 system controller binding and
MAINTAINERS file.

> diff --git a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
> index a01a515222c6d..d19ef4e3cac02 100644
> --- a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
> +++ b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
> @@ -27,6 +27,9 @@ properties:
>    '#reset-cells':
>      const: 1
>  
> +  '#syscon-cells':
> +    const: 2
> +

[Severity: Low]
Should #syscon-cells be dropped from cix,sky1-system-control.yaml?

A syscon node represents a miscellaneous collection of registers rather than a
provider class with a fixed specifier cell count, and different consumers of
 the same syscon node may take 0, 1, 2, or more arguments. In
drivers/mfd/syscon.c, syscon_regmap_lookup_by_phandle_args() parses phandle
arguments with a fixed count passed by the caller rather than querying a
#syscon-cells property on the provider node:

drivers/mfd/syscon.c:syscon_regmap_lookup_by_phandle_args() {
	...
	rc = of_parse_phandle_with_fixed_args(np, property, arg_count,
			0, &args);
	...
}

No other binding under Documentation/devicetree/bindings/ defines
#syscon-cells, and cix,usb-syscon in cix,sky1-usbssp.yaml already specifies
and constrains its two arguments via its phandle-array items schema.

> diff --git a/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
> new file mode 100644
> index 0000000000000..621526d32d7c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
[ ... ]
> +  reset-names:
> +    items:
> +      - const: apb
> +      - const: core
> +
> +  ssp-disable:
> +    type: boolean
> +    description: Disable SuperSpeed Plus mode of the controller.
> +
> +  u3-port-disable:
> +    type: boolean
> +    description: The board does not route the USB3 port of the controller.

[Severity: Low]
Do ssp-disable and u3-port-disable on the parent glue node duplicate standard
USB properties that are already available on the child cdns,usb3 controller
node?

In Documentation/devicetree/bindings/usb/cdns,usb3.yaml (and usb.yaml), speed
limits and available PHYs are already described on the child node:

  maximum-speed:
    enum: [super-speed-plus, super-speed, high-speed, full-speed]

  phys:
    minItems: 1
    maxItems: 2

The example in this binding already sets maximum-speed and phys on the child
usb@91d0000 node. Adding custom unprefixed ssp-disable and u3-port-disable
properties on the wrapper node creates two places to describe the same
hardware configuration. Could the glue driver inspect the child node's
maximum-speed and phys properties instead of introducing these two properties?

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