Re: [PATCH v6 1/2] dt-bindings: usb: cdns3: Add no_drd property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] The schema enforces strict positional ordering for 'reg-names' and 'interrupt-names', directly contradicting the commit message's claim of 'flexible ordering'.
- [Medium] The `reg` array positional descriptions in the base schema incorrectly document the XHCI registers as OTG registers when `no_drd` is true.
- [Medium] The `else` block is missing a `minItems: 3` constraint for `reg-names`, allowing mismatched array lengths between `reg` and `reg-names`.
- [Medium] The proposed property `no_drd` violates standard Device Tree naming conventions by using an underscore and missing a vendor prefix.
- [Low] Introducing a redundant boolean property (`no_drd`) to indicate a missing hardware block is a hardware description anti-pattern.
--

commit ef0683895d184cd4e0adacbed88f838da9544b55
Author: Pawel Laszczak <pawell@cadence.com>

dt-bindings: usb: cdns3: Add no_drd property

This commit introduces a new boolean property no_drd for Cadence
USBSS/USBSSP controllers. It updates the schema to adjust reg, interrupts,
and dr_mode requirements based on the presence of this property.

> To achieve this, the schema is updated with an if-then-else logic
> and 'reg-names'/'interrupt-names' use enums to allow flexible
> ordering during validation.

Does the if-then-else logic actually allow flexible ordering?

Looking at the conditional blocks below, the items are defined as a list
of const constraints. In JSON Schema, providing an array of schemas to
items enforces strict positional validation, meaning the elements must
appear in the exact order specified.

> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> index 2d95fb7321aff..717892a05dcdf 100644
> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> @@ -20,19 +20,21 @@ properties:
>      const: cdns,usb3
>  
>    reg:
> +    minItems: 2
>      items:
>        - description: OTG controller registers
>        - description: XHCI Host controller registers
>        - description: DEVICE controller registers

Does this cause an issue when the new property is set to true?

When restricted to 2 items in the then block below, the first item must
be xhci. However, the base schema defines the first item description as
OTG controller registers. It seems the XHCI register at index 0 will be
validated against and incorrectly described by the inherited OTG
description. Fixed positional descriptions generally cannot be used if
elements can be omitted from the beginning of a sequence.

[ ... ]

> @@ -79,6 +81,13 @@ properties:
>      description: Enable resetting of PHY if Rx fail is detected
>      type: boolean
>  
> +  no_drd:

This isn't a bug, but device tree property names should generally use
lowercase alphanumeric characters and hyphens instead of underscores.
Would it also be better to include a vendor prefix to avoid namespace
collisions, such as cdns,no-drd?

> +    description:
> +      Indicates that the Dual-Role Device (DRD) register block is not
> +      implemented or is inaccessible. In this case, the controller
> +      must operate in a fixed peripheral or host mode.
> +    type: boolean

Is a dedicated boolean flag needed here?

It seems the hardware configuration could be natively deduced by simply
checking if otg is absent from reg-names. Relying on a negative feature
flag instead of resource presence is usually discouraged in hardware
descriptions.

[ ... ]

> +    else:
> +      properties:
> +        reg:
> +          minItems: 3
> +        reg-names:
> +          items:
> +            - const: otg
> +            - const: xhci
> +            - const: dev

Could this allow mismatched array lengths between reg and reg-names?

The reg property explicitly requires minItems: 3 here, but reg-names is
not given a corresponding minItems constraint in this block. It will
inherit minItems: 2 from the base schema, which might allow validation to
succeed if a device tree defines 3 reg entries but only 2 reg-names.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-b4-no_drd_config-v6-0-e7789cd0e581@cadence.com?part=1