Re: [PATCH v5 15/27] dt-bindings: interrupt-controller: Document the property microchip,nr-irqs

[<Nicolas.Ferre@microchip.com>]

On 09/07/2024 at 08:13, Varshini Rajendran - I67070 wrote:
> On 03/07/24 9:11 pm, Conor Dooley wrote:
>> On Wed, Jul 03, 2024 at 03:58:14PM +0530, Varshini Rajendran wrote:
>>> Add the description and conditions to the device tree documentation
>>> for the property microchip,nr-irqs.
>>>
>>> Signed-off-by: Varshini Rajendran<varshini.rajendran@microchip.com>
>> This needs to be part of patch 14.
>>
>>> ---
>>>    .../bindings/interrupt-controller/atmel,aic.yaml     | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.yaml b/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.yaml
>>> index 9c5af9dbcb6e..06e5f92e7d53 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.yaml
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/atmel,aic.yaml
>>> @@ -54,6 +54,10 @@ properties:
>>>        $ref: /schemas/types.yaml#/definitions/uint32-array
>>>        description: u32 array of external irqs.
>>>    
>>> +  microchip,nr-irqs:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    description: u32 array of nr_irqs.
>> This makes no sense, did you just copy from above? Why would the number
>> of irqs be an array? Why can't you determine this from the compatble?
>>
> Sorry for the bad description. I will correct it in the next version.
> 
> For the second part of the question, this change was done as a step to
> resolve having a new compatible while having practically the same IP
> pointed out in the v3 of the series [1]. It is kind of looping back to
> the initial idea now. Even if this is added as a driver data, it
> overrides the expectation from the comment in [1]. Please suggest. I

In your v3 patch, indeed you were extracting the number of IRQs from the 
compatibility string (aka, from device tree...). It's my preferred 
solution as well.

So, come back to v3 [1] and address what Conor said in v4 "...having 
specific $soc_aic5_of_init() functions for each SoC seems silly when 
usually only the number of interrupts changes. The number of IRQs could 
be in the match data and you could use aic5_of_init in your 
IRQCHIP_DECLARE directly"

I think that we can convince Marc/Thomas that it's the best option as it 
prevents introducing another non-standard property to the DT, break the 
ABI (and was used happily for years).

Best regards,
   Nicolas

[1]
https://lore.kernel.org/lkml/87ee1e3c365686bc60e92ba3972dc1a5@kernel.org/


> also read Rob's concerns on having a device tree property for number of
> irqs.
> 
> [1]
> https://lore.kernel.org/lkml/87ee1e3c365686bc60e92ba3972dc1a5@kernel.org/
> 
>> Thanks,
>> Conor.
>>
>>> +
>>>    allOf:
>>>      - $ref: /schemas/interrupt-controller.yaml#
>>>      - if:
>>> @@ -71,6 +75,14 @@ allOf:
>>>            atmel,external-irqs:
>>>              minItems: 1
>>>              maxItems: 1
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: microchip,sam9x7-aic
>>> +    then:
>>> +      required:
>>> +        - microchip,nr-irqs
>>>    
>>>    required:
>>>      - compatible
>>> -- 
>>> 2.25.1
>>>
>