[EXT] Re: [PATCH v4 4/7] dt-bindings: usb: ci-hdrc-usb2: add restrictions for reg, interrupts, clock and clock-names properties

[Xu Yang]

Hi Krzysztof,

> 
> On 31/01/2024 09:24, Xu Yang wrote:
> > Hi Krzysztof,
> >
> >>
> >> On 19/01/2024 08:19, Xu Yang wrote:
> >>> Change reg, interrupts, clock and clock-names as common properties and add
> >>> restrictions on them for different compatibles.
> >>>
> >>> Signed-off-by: Xu Yang <xu.yang_2 at nxp.com>
> >>>
> >>> ---
> >>> Changes in v4:
> >>>  - new patch since v3's discussion
> >>>  - split the reg, interrupts, clock and clock-names properties into
> >>>    common part and device-specific
> >>> ---
> >>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 +++++++++++++++---
> >>>  1 file changed, 102 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
> >> hdrc-usb2.yaml
> >>> index b7e664f7395b..78e30ca0a8ca 100644
> >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> @@ -73,22 +73,10 @@ properties:
> >>>                - nuvoton,npcm845-udc
> >>>            - const: nuvoton,npcm750-udc
> >>>
> >>> -  reg:
> >>> -    minItems: 1
> >>> -    maxItems: 2
> >>> -
> >>> -  interrupts:
> >>> -    minItems: 1
> >>> -    maxItems: 2
> >>> -
> >>> -  clocks:
> >>> -    minItems: 1
> >>> -    maxItems: 3
> >>> -
> >>> -  clock-names:
> >>> -    minItems: 1
> >>> -    maxItems: 3
> >>
> >> Why all these are gone? They are supposed to be here. Your if:then: only
> >> customizes them.
> >
> > I have also concerns of whether to make this part common.
> > I will revert this later.
> 
> Revert? No. This patch must be correct.

I mean only this part keeps unchanged like before.

> >
> >>
> >>> -
> >>> +  reg: true
> >>> +  interrupts: true
> >>> +  clocks: true
> >>> +  clock-names: true
> >>
> >> No. These are not booleans on other variants.
> >
> > Okay.
> >
> >>
> >>>    dr_mode: true
> >>>
> >>>    power-domains:
> >>> @@ -412,6 +400,104 @@ allOf:
> >>>          samsung,picophy-pre-emp-curr-control: false
> >>>          samsung,picophy-dc-vol-level-adjust: false
> >>>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          oneOf:
> >>> +            - items:
> >>> +                - const: fsl,imx27-usb
> >>
> >> No, the syntax you need is contains:.
> >>
> >> Look at existing code - there is no single binding with oneOf: in if: block.
> >
> > I wonder why 'make dt_binding_check' does not report this issue if the syntax
> > is not correct?
> 
> I did not say syntax is incorrect.
> 
> 
> >
> > So I need to add contains as below, right?
> >
> >   - if:
> >       properties:
> >         compatible:
> >           contains:
> >             oneOf:
> >               - items:
> >                   - const: fsl,imx27-usb
> >               - items:
> >                   - enum:
> >                       - fsl,imx25-usb
> >                       - fsl,imx35-usb
> >                   - const: fsl,imx27-usb
> >
> > The purpose of this code is to match:
> >
> >   - compatible = "fsl,imx27-usb";
> >   - compatible = "fsl,imx25-usb", "fsl,imx27-usb";
> >   - compatible = "fsl,imx35-usb", "fsl,imx27-usb";
> >
> > but should not match:
> >
> >   - compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> >
> > Is this feasible?
> 
> So maybe they are not compatible? Your patch creates some unusual

Yes, they are not fully compatible.

> constraints for all the variants, which is probably result of huge one
> binding for all implementations of re-used IP block. I don't think that
> this huge if: you add here and further in the patch helps. Just like for
> other re-used IP blocks, this should have common part and
> per-device/per-family/per-implementation binding.

Actually I've tested all dts files (not only imx parts) against this dt-binding yaml.

Then I'll rework this patch to focus on imx parts. I'm not sure if someone will add
restrictions for their family/device in the future.

Thanks,
Xu Yang