dt_binding_check report false alarm?

[William Zhang]

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
>>>>
>>>
>>>
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4212 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20230525/d102059a/attachment-0001.p7s>