dt_binding_check report false alarm?

dt_binding_check report false alarm?

[William Zhang]

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

Hi,

It seems dt_binding_check reports a false error when run on this
modified yaml. I picked this simple file just to demostrate this issue.
Basically I made the interrupts and interrupt-names as optional
properties. But when there are two interrupts present, then
interrupt-names are required.  However in the example, I don't define
interrupts and interrupt-name at all, the dt binding check reports error
that interrupt-names are required:

make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTEX    Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts
  DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb
Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property
	From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml

This does not make sense to me as name is required only when there are
two interrupts. Can someone familar with this please help to check if I
miss anything in the yaml file or it is indeed a bug in binding check?

Thanks,
William

---
 .../devicetree/bindings/crypto/fsl-imx-scc.yaml     | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
index 563a31605d2b..c37a3a64a78c 100644
--- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
+++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
@@ -32,11 +32,18 @@ properties:
   clock-names:
     const: ipg
 
+allOf:
+  - if:
+      properties:
+        interrupts:
+          minItems: 2
+    then:
+      required:
+        - interrupt-names
+
 required:
   - compatible
   - reg
-  - interrupts
-  - interrupt-names
   - clocks
   - clock-names
 
@@ -49,6 +56,4 @@ examples:
         reg = <0x53fac000 0x4000>;
         clocks = <&clks 111>;
         clock-names = "ipg";
-        interrupts = <49>, <50>;
-        interrupt-names = "scm", "smn";
     };
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: dt_binding_check report false alarm?

[Conor Dooley]

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

Hey William,

On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
> Hi,
> 
> It seems dt_binding_check reports a false error when run on this
> modified yaml. I picked this simple file just to demostrate this issue.
> Basically I made the interrupts and interrupt-names as optional
> properties. But when there are two interrupts present, then
> interrupt-names are required.  However in the example, I don't define
> interrupts and interrupt-name at all, the dt binding check reports error
> that interrupt-names are required:

Rob and Krzysztof would know more than me, but since they're not
around...

> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> index 563a31605d2b..c37a3a64a78c 100644
> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> @@ -32,11 +32,18 @@ properties:
>    clock-names:
>      const: ipg
>  
> +allOf:
> +  - if:
> +      properties:
> +        interrupts:
> +          minItems: 2

...I don't think you can actually do this and "minItems: 2" will always
evaluate to true because it is an assignment. Don't hold me to that
though! The standard pattern here is to do:
allOf:
  - if:
      properties:
        compatible:
          contains:
            const: foo
    then:
      required:
        - interrupt-names

Cheers,
Conor.

> +    then:
> +      required:
> +        - interrupt-names
> +
>  required:
>    - compatible
>    - reg
> -  - interrupts
> -  - interrupt-names
>    - clocks
>    - clock-names
>  
> @@ -49,6 +56,4 @@ examples:
>          reg = <0x53fac000 0x4000>;
>          clocks = <&clks 111>;
>          clock-names = "ipg";
> -        interrupts = <49>, <50>;
> -        interrupt-names = "scm", "smn";
>      };
> -- 
> 2.34.1
> 



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

Re: dt_binding_check report false alarm?

[William Zhang]

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

Sorry for the multiple emails. Our mail relay server was not working 
properly.

Hi Conor,

On 05/25/2023 06:23 AM, Conor Dooley wrote:
> Hey William,
> 
> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
>> Hi,
>>
>> It seems dt_binding_check reports a false error when run on this
>> modified yaml. I picked this simple file just to demostrate this issue.
>> Basically I made the interrupts and interrupt-names as optional
>> properties. But when there are two interrupts present, then
>> interrupt-names are required.  However in the example, I don't define
>> interrupts and interrupt-name at all, the dt binding check reports error
>> that interrupt-names are required:
> 
> Rob and Krzysztof would know more than me, but since they're not
> around...
> 
>> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>> index 563a31605d2b..c37a3a64a78c 100644
>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>> @@ -32,11 +32,18 @@ properties:
>>     clock-names:
>>       const: ipg
>>   
>> +allOf:
>> +  - if:
>> +      properties:
>> +        interrupts:
>> +          minItems: 2
> 
> ...I don't think you can actually do this and "minItems: 2" will always
> evaluate to true because it is an assignment. Don't hold me to that
> though! The standard pattern here is to do:
> allOf:
>    - if:
>        properties:
>          compatible:
>            contains:
>              const: foo
>      then:
>        required:
>          - interrupt-names
> 
> Cheers,
> Conor.
> 
Our device can use one or two interrupt, or choose to not use interrupt 
at all(polling mode). Interrupt names is only required when there are 
two interrupts(so the driver code can tell which is which).  So I will 
need to check if it contains two interrupts. My check does work if I 
have two interrupt but don't have interrupt name, the check catches the 
error.  If I have one interrupt without interrupt name, the check pass. 
Only when I does not have interrupt and interrupt name,  it falsely 
report error.  Looks to me that it does not treat minItem = 0 case 
properly.


>> +    then:
>> +      required:
>> +        - interrupt-names
>> +
>>   required:
>>     - compatible
>>     - reg
>> -  - interrupts
>> -  - interrupt-names
>>     - clocks
>>     - clock-names
>>   
>> @@ -49,6 +56,4 @@ examples:
>>           reg = <0x53fac000 0x4000>;
>>           clocks = <&clks 111>;
>>           clock-names = "ipg";
>> -        interrupts = <49>, <50>;
>> -        interrupt-names = "scm", "smn";
>>       };
>> -- 
>> 2.34.1
>>
> 
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: dt_binding_check report false alarm?

[Conor Dooley]

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

On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote:
> Sorry for the multiple emails. Our mail relay server was not working
> properly.

I only got one /shrug

> On 05/25/2023 06:23 AM, Conor Dooley wrote:
> > Hey William,
> > 
> > On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
> > > Hi,
> > > 
> > > It seems dt_binding_check reports a false error when run on this
> > > modified yaml. I picked this simple file just to demostrate this issue.
> > > Basically I made the interrupts and interrupt-names as optional
> > > properties. But when there are two interrupts present, then
> > > interrupt-names are required.  However in the example, I don't define
> > > interrupts and interrupt-name at all, the dt binding check reports error
> > > that interrupt-names are required:
> > 
> > Rob and Krzysztof would know more than me, but since they're not
> > around...
> > 
> > > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> > > index 563a31605d2b..c37a3a64a78c 100644
> > > --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> > > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> > > @@ -32,11 +32,18 @@ properties:
> > >     clock-names:
> > >       const: ipg
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        interrupts:
> > > +          minItems: 2
> > 
> > ...I don't think you can actually do this and "minItems: 2" will always
> > evaluate to true because it is an assignment. Don't hold me to that
> > though! The standard pattern here is to do:
> > allOf:
> >    - if:
> >        properties:
> >          compatible:
> >            contains:
> >              const: foo
> >      then:
> >        required:
> >          - interrupt-names
> > 
> > Cheers,
> > Conor.
> > 
> Our device can use one or two interrupt, or choose to not use interrupt at
> all(polling mode). Interrupt names is only required when there are two
> interrupts(so the driver code can tell which is which).  So I will need to
> check if it contains two interrupts. My check does work if I have two
> interrupt but don't have interrupt name, the check catches the error.  If I
> have one interrupt without interrupt name, the check pass. Only when I does
> not have interrupt and interrupt name,  it falsely report error.  Looks to
> me that it does not treat minItem = 0 case properly.

Right. I would not bother with the "only interrupt-names when 2
interrupts" stuff & do the simple thing of always making it required
when you have interrupts.
Then you can use allOf and oneOf to allow for both schemes for the new
device and keep enforcement of 2 items for the existing one.

Cheers,
Conor.

> > > +    then:
> > > +      required:
> > > +        - interrupt-names
> > > +
> > >   required:
> > >     - compatible
> > >     - reg
> > > -  - interrupts
> > > -  - interrupt-names
> > >     - clocks
> > >     - clock-names
> > > @@ -49,6 +56,4 @@ examples:
> > >           reg = <0x53fac000 0x4000>;
> > >           clocks = <&clks 111>;
> > >           clock-names = "ipg";
> > > -        interrupts = <49>, <50>;
> > > -        interrupt-names = "scm", "smn";
> > >       };
> > > -- 
> > > 2.34.1
> > > 
> > 
> > 



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

Re: dt_binding_check report false alarm?

[William Zhang]

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



On 05/25/2023 08:33 AM, Conor Dooley wrote:
> On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote:
>> Sorry for the multiple emails. Our mail relay server was not working
>> properly.
> 
> I only got one /shrug
> 
That's good.  Maybe it only flushed queued email to internal email accounts.

>> On 05/25/2023 06:23 AM, Conor Dooley wrote:
>>> Hey William,
>>>
>>> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
>>>> Hi,
>>>>
>>>> It seems dt_binding_check reports a false error when run on this
>>>> modified yaml. I picked this simple file just to demostrate this issue.
>>>> Basically I made the interrupts and interrupt-names as optional
>>>> properties. But when there are two interrupts present, then
>>>> interrupt-names are required.  However in the example, I don't define
>>>> interrupts and interrupt-name at all, the dt binding check reports error
>>>> that interrupt-names are required:
>>>
>>> Rob and Krzysztof would know more than me, but since they're not
>>> around...
>>>
>>>> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>>> index 563a31605d2b..c37a3a64a78c 100644
>>>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>>> @@ -32,11 +32,18 @@ properties:
>>>>      clock-names:
>>>>        const: ipg
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        interrupts:
>>>> +          minItems: 2
>>>
>>> ...I don't think you can actually do this and "minItems: 2" will always
>>> evaluate to true because it is an assignment. Don't hold me to that
>>> though! The standard pattern here is to do:
>>> allOf:
>>>     - if:
>>>         properties:
>>>           compatible:
>>>             contains:
>>>               const: foo
>>>       then:
>>>         required:
>>>           - interrupt-names
>>>
>>> Cheers,
>>> Conor.
>>>
>> Our device can use one or two interrupt, or choose to not use interrupt at
>> all(polling mode). Interrupt names is only required when there are two
>> interrupts(so the driver code can tell which is which).  So I will need to
>> check if it contains two interrupts. My check does work if I have two
>> interrupt but don't have interrupt name, the check catches the error.  If I
>> have one interrupt without interrupt name, the check pass. Only when I does
>> not have interrupt and interrupt name,  it falsely report error.  Looks to
>> me that it does not treat minItem = 0 case properly.
> 
> Right. I would not bother with the "only interrupt-names when 2
> interrupts" stuff & do the simple thing of always making it required
> when you have interrupts.
> Then you can use allOf and oneOf to allow for both schemes for the new
> device and keep enforcement of 2 items for the existing one.
> 
> Cheers,
> Conor.
> 
I agree it is better to keep it simple. Sorry I am still new to yaml but 
what is the best way to check if interrupt property exist?  What I can 
think of is similar

  - if:
       properties:
         interrupts:
           minItems: 1
     then:
       required:
         - interrupt-names

But this still reports the same error when there is no interrupt.

>>>> +    then:
>>>> +      required:
>>>> +        - interrupt-names
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> -  - interrupts
>>>> -  - interrupt-names
>>>>      - clocks
>>>>      - clock-names
>>>> @@ -49,6 +56,4 @@ examples:
>>>>            reg = <0x53fac000 0x4000>;
>>>>            clocks = <&clks 111>;
>>>>            clock-names = "ipg";
>>>> -        interrupts = <49>, <50>;
>>>> -        interrupt-names = "scm", "smn";
>>>>        };
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>>
> 
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: dt_binding_check report false alarm?

[Robin Murphy]

On 25/05/2023 4:23 pm, William Zhang wrote:
> Sorry for the multiple emails. Our mail relay server was not working 
> properly.
> 
> Hi Conor,
> 
> On 05/25/2023 06:23 AM, Conor Dooley wrote:
>> Hey William,
>>
>> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
>>> Hi,
>>>
>>> It seems dt_binding_check reports a false error when run on this
>>> modified yaml. I picked this simple file just to demostrate this issue.
>>> Basically I made the interrupts and interrupt-names as optional
>>> properties. But when there are two interrupts present, then
>>> interrupt-names are required.  However in the example, I don't define
>>> interrupts and interrupt-name at all, the dt binding check reports error
>>> that interrupt-names are required:
>>
>> Rob and Krzysztof would know more than me, but since they're not
>> around...
>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml 
>>> b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>> index 563a31605d2b..c37a3a64a78c 100644
>>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>> @@ -32,11 +32,18 @@ properties:
>>>     clock-names:
>>>       const: ipg
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        interrupts:
>>> +          minItems: 2
>>
>> ...I don't think you can actually do this and "minItems: 2" will always
>> evaluate to true because it is an assignment. Don't hold me to that
>> though! The standard pattern here is to do:
>> allOf:
>>    - if:
>>        properties:
>>          compatible:
>>            contains:
>>              const: foo
>>      then:
>>        required:
>>          - interrupt-names
>>
>> Cheers,
>> Conor.
>>
> Our device can use one or two interrupt, or choose to not use interrupt 
> at all(polling mode). Interrupt names is only required when there are 
> two interrupts(so the driver code can tell which is which).  So I will 
> need to check if it contains two interrupts. My check does work if I 
> have two interrupt but don't have interrupt name, the check catches the 
> error.  If I have one interrupt without interrupt name, the check pass. 
> Only when I does not have interrupt and interrupt name,  it falsely 
> report error.  Looks to me that it does not treat minItem = 0 case 
> properly.

Note that minItems = 0 is *not* the same as the property being absent 
(that would represent an empty property, i.e. just "interrupts;")

The conditional schema fragment only says that an "interrupts" property 
must have at least two entries in order to match. However it doesn't say 
that the property is required, and thus the example DT (without the 
property) does not fail to match the constraints of the given schema, 
and thus the condition ends up true.

It certainly took me several goes to get my head round how conditionals 
work, and the notion that what goes under the "if:" is a schema 
definition in its own right :)

Robin.

> 
> 
>>> +    then:
>>> +      required:
>>> +        - interrupt-names
>>> +
>>>   required:
>>>     - compatible
>>>     - reg
>>> -  - interrupts
>>> -  - interrupt-names
>>>     - clocks
>>>     - clock-names
>>> @@ -49,6 +56,4 @@ examples:
>>>           reg = <0x53fac000 0x4000>;
>>>           clocks = <&clks 111>;
>>>           clock-names = "ipg";
>>> -        interrupts = <49>, <50>;
>>> -        interrupt-names = "scm", "smn";
>>>       };
>>> -- 
>>> 2.34.1
>>>
>>
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: dt_binding_check report false alarm?

[Conor Dooley]

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

On Thu, May 25, 2023 at 10:10:46AM -0700, William Zhang wrote:
> 
> 
> On 05/25/2023 08:33 AM, Conor Dooley wrote:
> > On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote:
> > > Sorry for the multiple emails. Our mail relay server was not working
> > > properly.
> > 
> > I only got one /shrug
> > 
> That's good.  Maybe it only flushed queued email to internal email accounts.
> 
> > > On 05/25/2023 06:23 AM, Conor Dooley wrote:
> > > > Hey William,
> > > > 
> > > > On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
> > > > > Hi,
> > > > > 
> > > > > It seems dt_binding_check reports a false error when run on this
> > > > > modified yaml. I picked this simple file just to demostrate this issue.
> > > > > Basically I made the interrupts and interrupt-names as optional
> > > > > properties. But when there are two interrupts present, then
> > > > > interrupt-names are required.  However in the example, I don't define
> > > > > interrupts and interrupt-name at all, the dt binding check reports error
> > > > > that interrupt-names are required:
> > > > 
> > > > Rob and Krzysztof would know more than me, but since they're not
> > > > around...
> > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> > > > > index 563a31605d2b..c37a3a64a78c 100644
> > > > > --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> > > > > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> > > > > @@ -32,11 +32,18 @@ properties:
> > > > >      clock-names:
> > > > >        const: ipg
> > > > > +allOf:
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        interrupts:
> > > > > +          minItems: 2
> > > > 
> > > > ...I don't think you can actually do this and "minItems: 2" will always
> > > > evaluate to true because it is an assignment. Don't hold me to that
> > > > though! The standard pattern here is to do:
> > > > allOf:
> > > >     - if:
> > > >         properties:
> > > >           compatible:
> > > >             contains:
> > > >               const: foo
> > > >       then:
> > > >         required:
> > > >           - interrupt-names

> > > Our device can use one or two interrupt, or choose to not use interrupt at
> > > all(polling mode). Interrupt names is only required when there are two
> > > interrupts(so the driver code can tell which is which).  So I will need to
> > > check if it contains two interrupts. My check does work if I have two
> > > interrupt but don't have interrupt name, the check catches the error.  If I
> > > have one interrupt without interrupt name, the check pass. Only when I does
> > > not have interrupt and interrupt name,  it falsely report error.  Looks to
> > > me that it does not treat minItem = 0 case properly.
> > 
> > Right. I would not bother with the "only interrupt-names when 2
> > interrupts" stuff & do the simple thing of always making it required
> > when you have interrupts.
> > Then you can use allOf and oneOf to allow for both schemes for the new
> > device and keep enforcement of 2 items for the existing one.

> I agree it is better to keep it simple. Sorry I am still new to yaml but
> what is the best way to check if interrupt property exist?  What I can think
> of is similar
> 
>  - if:
>       properties:
>         interrupts:
>           minItems: 1
>     then:
>       required:
>         - interrupt-names
> 
> But this still reports the same error when there is no interrupt.

Yeah, you don't need to do something like that.
It's hard to say exactly without your actual patch, but I think you're
over complicating things :)
What you seem to want to do (from where I am sitting) is something like:
	dependencies:
	  interrupts: interrupt-names

and not put interrupts/interrupt-names in the required bit.
That way, if you have interrupts, you need to have interrupt-names too
but you can have neither.

But yeah, without something resembling the "real" patch, rather than
hacking at the fsl one, it is kinda hard to say.

Cheers,
Conor.


> 
> > > > > +    then:
> > > > > +      required:
> > > > > +        - interrupt-names
> > > > > +
> > > > >    required:
> > > > >      - compatible
> > > > >      - reg
> > > > > -  - interrupts
> > > > > -  - interrupt-names
> > > > >      - clocks
> > > > >      - clock-names
> > > > > @@ -49,6 +56,4 @@ examples:
> > > > >            reg = <0x53fac000 0x4000>;
> > > > >            clocks = <&clks 111>;
> > > > >            clock-names = "ipg";
> > > > > -        interrupts = <49>, <50>;
> > > > > -        interrupt-names = "scm", "smn";
> > > > >        };
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > > 
> > > > 
> > 
> > 



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

Re: dt_binding_check report false alarm?

[Rob Herring]

On Wed, 24 May 2023 22:02:41 -0700, William Zhang wrote:
> Hi,
> 
> It seems dt_binding_check reports a false error when run on this
> modified yaml. I picked this simple file just to demostrate this issue.
> Basically I made the interrupts and interrupt-names as optional
> properties. But when there are two interrupts present, then
> interrupt-names are required.  However in the example, I don't define
> interrupts and interrupt-name at all, the dt binding check reports error
> that interrupt-names are required:
> 
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>   LINT    Documentation/devicetree/bindings
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   DTEX    Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts
>   DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb
> Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property
> 	From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> 
> This does not make sense to me as name is required only when there are
> two interrupts. Can someone familar with this please help to check if I
> miss anything in the yaml file or it is indeed a bug in binding check?
> 
> Thanks,
> William
> 
> ---
>  .../devicetree/bindings/crypto/fsl-imx-scc.yaml     | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230525050241.3700-1-william.zhang@broadcom.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: dt_binding_check report false alarm?

[William Zhang]

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



On 05/25/2023 11:12 AM, Conor Dooley wrote:
> On Thu, May 25, 2023 at 10:10:46AM -0700, William Zhang wrote:
>>
>>
>> On 05/25/2023 08:33 AM, Conor Dooley wrote:
>>> On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote:
>>>> Sorry for the multiple emails. Our mail relay server was not working
>>>> properly.
>>>
>>> I only got one /shrug
>>>
>> That's good.  Maybe it only flushed queued email to internal email accounts.
>>
>>>> On 05/25/2023 06:23 AM, Conor Dooley wrote:
>>>>> Hey William,
>>>>>
>>>>> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> It seems dt_binding_check reports a false error when run on this
>>>>>> modified yaml. I picked this simple file just to demostrate this issue.
>>>>>> Basically I made the interrupts and interrupt-names as optional
>>>>>> properties. But when there are two interrupts present, then
>>>>>> interrupt-names are required.  However in the example, I don't define
>>>>>> interrupts and interrupt-name at all, the dt binding check reports error
>>>>>> that interrupt-names are required:
>>>>>
>>>>> Rob and Krzysztof would know more than me, but since they're not
>>>>> around...
>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>>>>> index 563a31605d2b..c37a3a64a78c 100644
>>>>>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>>>>> @@ -32,11 +32,18 @@ properties:
>>>>>>       clock-names:
>>>>>>         const: ipg
>>>>>> +allOf:
>>>>>> +  - if:
>>>>>> +      properties:
>>>>>> +        interrupts:
>>>>>> +          minItems: 2
>>>>>
>>>>> ...I don't think you can actually do this and "minItems: 2" will always
>>>>> evaluate to true because it is an assignment. Don't hold me to that
>>>>> though! The standard pattern here is to do:
>>>>> allOf:
>>>>>      - if:
>>>>>          properties:
>>>>>            compatible:
>>>>>              contains:
>>>>>                const: foo
>>>>>        then:
>>>>>          required:
>>>>>            - interrupt-names
> 
>>>> Our device can use one or two interrupt, or choose to not use interrupt at
>>>> all(polling mode). Interrupt names is only required when there are two
>>>> interrupts(so the driver code can tell which is which).  So I will need to
>>>> check if it contains two interrupts. My check does work if I have two
>>>> interrupt but don't have interrupt name, the check catches the error.  If I
>>>> have one interrupt without interrupt name, the check pass. Only when I does
>>>> not have interrupt and interrupt name,  it falsely report error.  Looks to
>>>> me that it does not treat minItem = 0 case properly.
>>>
>>> Right. I would not bother with the "only interrupt-names when 2
>>> interrupts" stuff & do the simple thing of always making it required
>>> when you have interrupts.
>>> Then you can use allOf and oneOf to allow for both schemes for the new
>>> device and keep enforcement of 2 items for the existing one.
> 
>> I agree it is better to keep it simple. Sorry I am still new to yaml but
>> what is the best way to check if interrupt property exist?  What I can think
>> of is similar
>>
>>   - if:
>>        properties:
>>          interrupts:
>>            minItems: 1
>>      then:
>>        required:
>>          - interrupt-names
>>
>> But this still reports the same error when there is no interrupt.
> 
> Yeah, you don't need to do something like that.
> It's hard to say exactly without your actual patch, but I think you're
> over complicating things :)
> What you seem to want to do (from where I am sitting) is something like:
> 	dependencies:
> 	  interrupts: interrupt-names
> 
> and not put interrupts/interrupt-names in the required bit.
> That way, if you have interrupts, you need to have interrupt-names too
> but you can have neither.
> 
> But yeah, without something resembling the "real" patch, rather than
> hacking at the fsl one, it is kinda hard to say.
> 
> Cheers,
> Conor.
> 
> 
Yeah this dependencies rule works and it is much simple. Just need to 
add the bracket around the interrupt-names to make it work:
   dependencies:
     interrupts: [interrupt-names]
Do we have any document list all these supported keywords/rules/syntax 
for yaml dts?

My file is actually brcm,brcmnand.yaml but it is little bit complicated 
to demonstrate the issue so just randomly pick this simple one.  Thanks 
Conor, Robin and all the for help.   Will submit an official patches for 
brcmnand.yaml change.

>>
>>>>>> +    then:
>>>>>> +      required:
>>>>>> +        - interrupt-names
>>>>>> +
>>>>>>     required:
>>>>>>       - compatible
>>>>>>       - reg
>>>>>> -  - interrupts
>>>>>> -  - interrupt-names
>>>>>>       - clocks
>>>>>>       - clock-names
>>>>>> @@ -49,6 +56,4 @@ examples:
>>>>>>             reg = <0x53fac000 0x4000>;
>>>>>>             clocks = <&clks 111>;
>>>>>>             clock-names = "ipg";
>>>>>> -        interrupts = <49>, <50>;
>>>>>> -        interrupt-names = "scm", "smn";
>>>>>>         };
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: dt_binding_check report false alarm?

[Conor Dooley]

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

On Thu, May 25, 2023 at 10:56:45PM -0700, William Zhang wrote:

> Do we have any document list all these supported keywords/rules/syntax for
> yaml dts?

The example schema is a good place to look:
https://docs.kernel.org/devicetree/bindings/writing-schema.html?highlight=example+schema#annotated-example-schema

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

Re: dt_binding_check report false alarm?

[Rob Herring]

On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
> Hi,
> 
> It seems dt_binding_check reports a false error when run on this
> modified yaml. I picked this simple file just to demostrate this issue.
> Basically I made the interrupts and interrupt-names as optional
> properties. But when there are two interrupts present, then
> interrupt-names are required.  However in the example, I don't define
> interrupts and interrupt-name at all, the dt binding check reports error
> that interrupt-names are required:
> 
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>   LINT    Documentation/devicetree/bindings
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   DTEX    Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts
>   DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb
> Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property
> 	From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> 
> This does not make sense to me as name is required only when there are
> two interrupts. Can someone familar with this please help to check if I
> miss anything in the yaml file or it is indeed a bug in binding check?
> 
> Thanks,
> William
> 
> ---
>  .../devicetree/bindings/crypto/fsl-imx-scc.yaml     | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> index 563a31605d2b..c37a3a64a78c 100644
> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
> @@ -32,11 +32,18 @@ properties:
>    clock-names:
>      const: ipg
>  
> +allOf:
> +  - if:
> +      properties:
> +        interrupts:
> +          minItems: 2

This is a quirk of json-schema in that the 'if' will be true if the 
property is not present. Adding this to the 'if' should fix the problem:

required:
  - interrupts

> +    then:
> +      required:
> +        - interrupt-names
> +
>  required:
>    - compatible
>    - reg
> -  - interrupts
> -  - interrupt-names
>    - clocks
>    - clock-names
>  
> @@ -49,6 +56,4 @@ examples:
>          reg = <0x53fac000 0x4000>;
>          clocks = <&clks 111>;
>          clock-names = "ipg";
> -        interrupts = <49>, <50>;
> -        interrupt-names = "scm", "smn";
>      };
> -- 
> 2.34.1
> 

Re: dt_binding_check report false alarm?

[William Zhang]

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



On 06/08/2023 07:57 AM, Rob Herring wrote:
> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote:
>> Hi,
>>
>> It seems dt_binding_check reports a false error when run on this
>> modified yaml. I picked this simple file just to demostrate this issue.
>> Basically I made the interrupts and interrupt-names as optional
>> properties. But when there are two interrupts present, then
>> interrupt-names are required.  However in the example, I don't define
>> interrupts and interrupt-name at all, the dt binding check reports error
>> that interrupt-names are required:
>>
>> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>    LINT    Documentation/devicetree/bindings
>>    CHKDT   Documentation/devicetree/bindings/processed-schema.json
>>    SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>>    DTEX    Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts
>>    DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb
>> Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property
>> 	From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>>
>> This does not make sense to me as name is required only when there are
>> two interrupts. Can someone familar with this please help to check if I
>> miss anything in the yaml file or it is indeed a bug in binding check?
>>
>> Thanks,
>> William
>>
>> ---
>>   .../devicetree/bindings/crypto/fsl-imx-scc.yaml     | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>> index 563a31605d2b..c37a3a64a78c 100644
>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml
>> @@ -32,11 +32,18 @@ properties:
>>     clock-names:
>>       const: ipg
>>   
>> +allOf:
>> +  - if:
>> +      properties:
>> +        interrupts:
>> +          minItems: 2
> 
> This is a quirk of json-schema in that the 'if' will be true if the
> property is not present. Adding this to the 'if' should fix the problem:
> 
> required:
>    - interrupts
> 
Thanks Rob!  This works!

>> +    then:
>> +      required:
>> +        - interrupt-names
>> +
>>   required:
>>     - compatible
>>     - reg
>> -  - interrupts
>> -  - interrupt-names
>>     - clocks
>>     - clock-names
>>   
>> @@ -49,6 +56,4 @@ examples:
>>           reg = <0x53fac000 0x4000>;
>>           clocks = <&clks 111>;
>>           clock-names = "ipg";
>> -        interrupts = <49>, <50>;
>> -        interrupt-names = "scm", "smn";
>>       };
>> -- 
>> 2.34.1
>>
> 
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]