Re: [PATCH v5 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:
- [High] Lost functionality: The optional 'wakeup' interrupt is incorrectly forbidden when 'no_drd' is true.
- [Medium] Devicetree naming convention violation and architectural anti-pattern for 'no_drd'.
- [Medium] Logic contradiction: Schema modifications enforce strict ordering, completely preventing the claimed 'flexible ordering'.
- [Medium] Schema documentation and indexing mismatch for 'reg' and 'interrupts' descriptions.
- [Medium] Validation weakening: The fallback 'else' block inadvertently drops the strict constraint for the 4th optional 'wakeup' interrupt.
--

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

dt-bindings: usb: cdns3: Add no_drd property

Introduce a new boolean property 'no_drd' for Cadence USBSS/USBSSP
controllers to support hardware configurations where the Dual-Role Device (DRD)
register block is missing or inaccessible. The schema is updated with an
if-then-else logic and enums for register and interrupt names.

>    When 'no_drd' is absent, the binding maintains backward compatibility
>    by requiring all 3 resource sets (otg, host, dev).
>    
>    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 new if-then-else block actually allow flexible ordering? The
added logic redefines the 'items' arrays as exact sequences (e.g., using
const for each index), which enforces strict tuple validation per index.
This seems to require elements to exactly match the defined order, preventing
the flexibility described here.

> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> index 2d95fb7321aff..5d9bea62729c1 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

Since this base schema strictly defines index 0 of 'reg' as OTG controller
registers, will this cause issues when 'no_drd' is true? In that case, the
OTG block is omitted, forcing 'reg-names' index 0 to be 'xhci' and index 1
to be 'dev'.

Could this cause validation tooling and generated documentation to incorrectly
document and label the 'xhci' register as 'OTG controller registers'?

[ ... ]

> @@ -79,6 +81,13 @@ properties:
>      description: Enable resetting of PHY if Rx fail is detected
>      type: boolean
>  
> +  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

Standard devicetree property names normally use hyphens instead of underscores.
Should this be named 'cdns,no-drd' to include the vendor prefix and prevent
namespace collisions?

Additionally, is using a negative boolean property to denote missing hardware
the best approach here? Devicetree typically describes present hardware, so
missing blocks are often inferred from property lengths or described via a
distinct compatible string.

[ ... ]

> @@ -93,6 +102,46 @@ allOf:
>    - $ref: usb-drd.yaml#
>    - $ref: usb-xhci.yaml#
>  
> +  - if:
> +      properties:
> +        no_drd: true
> +      required:
> +        - no_drd
> +    then:
> +      required:
> +        - dr_mode
> +      properties:
> +        reg:
> +          maxItems: 2
> +        reg-names:
> +          items:
> +            - const: xhci
> +            - const: dev
> +        interrupts:
> +          maxItems: 2
> +        interrupt-names:
> +          items:
> +            - const: host
> +            - const: peripheral

The original binding allows an optional 4th interrupt named 'wakeup', which
is cleared by the xhci core.

Since this conditional block explicitly restricts 'interrupts' to maxItems: 2,
will hardware configurations that utilize the 'wakeup' line fail schema
validation when operating without the DRD block?

> +        dr_mode:
> +          enum: [host, peripheral]
> +    else:
> +      properties:
> +        reg:
> +          minItems: 3
> +        reg-names:
> +          items:
> +            - const: otg
> +            - const: xhci
> +            - const: dev
> +        interrupts:
> +          minItems: 3
> +        interrupt-names:
> +          items:
> +            - const: host
> +            - const: peripheral
> +            - const: otg

The base schema now allows the interrupt-names elements to be any of the four
enums. Because this 'else' block only overrides the first three items with
'const' constraints and omits the 4th item entirely, does the 4th interrupt
fall back to the generic enum?

Could this inadvertently allow duplicate invalid configurations (like
[host, peripheral, otg, host]) instead of strictly requiring 'wakeup' as the
original schema did?

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