Re: [PATCH v2 01/16] dt-bindings: media: fsl-pxp: convert to yaml

Krzysztof Kozlowski
2023-01-13T16:54:47Z

On 13/01/2023 16:09, Michael Tretter wrote:
>>> +properties:
>>> + compatible:
>>> + oneOf:
>>> + - const: fsl,imx6ul-pxp
>>> + - const: fsl,imx6ull-pxp
>>> + - const: fsl,imx7d-pxp
>>
>> These three are an enum.
>
> These are alternatives to the 'items:' entry below.
>
> Are you suggesting to use the following statement?
>
> oneOf:
> - enum:
> - fsl,imx6ul-pxp
> - fsl,imx6ull-pxp
> - fsl,imx7d-pxp
> - items:
> - enum:
> - fsl,imx6sll-pxp
> - fsl,imx6sx-pxp

Yes.

>
> Why is this better than the one that I used?

Because that's the convention - use enum for enumeration which nicely
groups all of them and is the easiest to read.

>
>>
>>> + - items:
>>> + - enum:
>>> + - fsl,imx6sll-pxp
>>> + - fsl,imx6sx-pxp
>>> + - const: fsl,imx6ull-pxp
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + minItems: 1
>>> + maxItems: 2
>>> +
>>> + clocks:
>>> + maxItems: 1
>>> +
>>> + clock-names:
>>> + const: axi
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> + - clocks
>>> + - clock-names
>>> +
>>> +allOf:
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - fsl,imx6sx-pxp
>>> + then:
>>> + properties:
>>> + interrupts:
>>> + numItems: 1
>>
>> That's not correct syntax... I am surprised that it works. Did you test
>> the bindings?
>
> I copied this syntax from renesas,wdt.yaml and ran
>
> make ARCH=arm dtbs_check DT_SCHEMA_FILES=fsl,imx6ull-pxp.yaml
>
> with SOC_IMX7D=y, SOC_IMX6UL=y, SOC_IMX6SLL=y, and SOC_IMX6SX=y. The latter
> two were not enabled in the v1, which is why it didn't catch the missing
> compatibles.
>
> On a closer look, I just saw that the checker ignored the schema due to the
> incorrect syntax and didn't produce any further errors. With the syntax fixed,
> the checker now produces also a few more errors about power-domains, which I
> will fix in v3.
>
> Is this syntax correct?
>
> allOf:
> - if:
> properties:
> compatible:
> contains:
> enum:
> - fsl,imx6sx-pxp
> then:
> properties:
> interrupts:
> minItems: 1

Drop this one

> maxItems: 1
> else:
> properties:
> interrupts:
> minItems: 2
> maxItems: 2
>
> Michael

Rest is ok

Best regards,
Krzysztof