Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible

From: <Conor.Dooley@microchip.com>
To: <heinrich.schuchardt@canonical.com>, <Conor.Dooley@microchip.com>
Cc: <sagar.kadam@sifive.com>, <atishp@atishpatra.org>,
	<paul.walmsley@sifive.com>, <krzysztof.kozlowski+dt@linaro.org>,
	<devicetree@vger.kernel.org>, <linux-riscv@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <aou@eecs.berkeley.edu>,
	<Daire.McNamara@microchip.com>, <palmer@dabbelt.com>,
	<robh+dt@kernel.org>
Subject: Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
Date: Thu, 25 Aug 2022 20:03:54 +0000	[thread overview]
Message-ID: <38981c6e-40cc-7468-b1ef-f4b3f341f3fd@microchip.com> (raw)
In-Reply-To: <7b635894-5a91-9294-75ab-2fad8f657577@canonical.com>

On 25/08/2022 20:49, Heinrich Schuchardt wrote:
> On 8/25/22 20:56, Conor.Dooley@microchip.com wrote:
>> On 25/08/2022 19:36, Heinrich Schuchardt wrote:
>>> On 8/25/22 20:04, Conor Dooley wrote:
>>>> From: Conor Dooley <conor.dooley@microchip.com>

>>>> +allOf:
>>>> +  - $ref: /schemas/cache-controller.yaml#
>>>>    -then:
>>>> -  properties:
>>>> -    interrupts:
>>>> -      description: |
>>>> -        Must contain entries for DirError, DataError and DataFail signals.
>>>> -      maxItems: 3
>>>> -    cache-sets:
>>>> -      const: 1024
>>>> -
>>>> -else:
>>>> -  properties:
>>>> -    interrupts:
>>>> -      description: |
>>>> -        Must contain entries for DirError, DataError, DataFail, DirFail signals.
>>>> -      minItems: 4
>>>> -    cache-sets:
>>>> -      const: 2048
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - sifive,fu740-c000-ccache
>>>> +              - microchip,mpfs-ccache
>>>> +
>>>> +    then:
>>>> +      properties:
>>>> +        interrupts:
>>>> +          description: |
>>>> +            Must contain entries for DirError, DataError, DataFail, DirFail signals.
>>>> +          minItems: 4
> 
> Above you indicated that you want strict limits for the interrupt count.
> You expect exactly 4 items here. Having 5 entries would not be correct.
> Please, add 'maxItems: 4'.

Outside of this diff, because of how the particular binding was
structured, there is:
  interrupts:
    minItems: 3
    items:
      - description: DirError interrupt
      - description: DataError interrupt
      - description: DataFail interrupt
      - description: DirFail interrupt

AFAIU, "maxItems: 4" is redundant because all possible items are listed.

> 
>>>> +
>>>> +    else:
>>>> +      properties:
>>>> +        interrupts:
>>>> +          description: |
>>>> +            Must contain entries for DirError, DataError and DataFail signals.
>>>> +          maxItems: 3
> 
> The item count should be exactly 3. Having 2 entries would not be correct.
> Please, add 'minItems: 3'.

Again, this is set by the section I pasted above - although this time
explicitly.

Hope that explains things, not the easiest binding to understand from
a diff alone. Possibly I should have passed a "-U" argument while
creating the patches to get an easier-to-follow diff.

Thanks for your (prompt) reviews,
Conor.

>>>> +
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: sifive,fu740-c000-ccache
>>>> +
>>>> +    then:
>>>> +      properties:
>>>> +        cache-sets:
>>>> +          const: 2048
>>>> +
>>>> +    else:
>>>> +      properties:
>>>> +        cache-sets:
>>>> +          const: 1024
>>>>      additionalProperties: false
>>>>    