Re: [PATCH v9 1/2] dt-bindings: usb: cdns3: Add cdns,cdnsp compatible string

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

On Thu, May 21, 2026 at 08:16:51AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] The schema does not enforce the presence of the `dr_mode` property when `cdns,cdnsp` is matched, despite the commit message explicitly requiring it.
> - [Medium] Array bounds (`minItems` and `maxItems`) for `reg`, `reg-names`, and `interrupt-names` are not explicitly constrained inside the conditionals.
> - [Medium] Use of the empty schema `{}` as a wildcard for compatible strings breaks dt-schema validation and explicit documentation rules.
> --
> 
> commit 1f787c873a0b8a21200340bba7dffd95f1916193
> Author: Pawel Laszczak <pawell@cadence.com>
> 
> dt-bindings: usb: cdns3: Add cdns,cdnsp compatible string
> 
> Introduce a new generic fallback compatible string cdns,cdnsp for
> Cadence USBSSP controllers without the Dual-Role Device (DRD) register
> block. It restricts the resources to host and device configurations,
> requiring dr_mode to be explicitly set.
> 
> > diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> > --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> > @@ -17,22 +17,24 @@ description:
> >  
> >  properties:
> >    compatible:
> > -    const: cdns,usb3
> > +    oneOf:
> > +      - const: cdns,usb3
> > +      - items:
> > +          - {}
> 
> [Severity: Medium]
> The commit message mentions using an empty schema wildcard {} to allow
> future vendor SoC compatibles to be prepended. Does this break dt-schema
> meta-validation during dt_binding_check? The tooling usually expects
> concrete compatible strings to be explicitly documented via an enum rather
> than using a wildcard.

Obviously this is intentional.

> > +          - const: cdns,cdnsp
> 
> [ ... ]
> 
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: cdns,cdnsp
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: XHCI Host controller registers
> > +            - description: DEVICE controller registers
> > +        reg-names:
> > +          items:
> > +            - const: xhci
> > +            - const: dev
> 
> [Severity: Medium]
> In dt-schema, specifying an items list inside conditional branches does not
> automatically override the top-level minItems and maxItems length constraints.
> 
> Since the top-level schema allows 2 to 3 items, would omitting maxItems: 2
> for reg and reg-names in this 'then' branch allow a 3-item configuration
> to incorrectly pass validation? Similarly, does omitting minItems: 3 in the
> 'else' branch allow a 2-item configuration to pass for cdns,usb3?

This looks valid, easily fixed by adding the correct fooItems
properties.

> 
> > +        interrupts:
> > +          maxItems: 2
> > +        interrupt-names:
> > +          items:
> > +            - const: host
> > +            - const: peripheral
> > +        dr_mode:
> > +          enum: [host, peripheral]
> 
> [Severity: Medium]
> The commit message states that dr_mode must be explicitly set to either
> 'host' or 'peripheral' when this compatible is matched.
> 
> Should dr_mode be added to a required list in this block? Without it, the
> property remains optional, and device trees missing dr_mode could silently
> pass validation.


This is a good point, because the default dr_mode is otg.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]