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>
---
 .../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.
+
 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

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.

Why?

Of *all* the other interrupt controller bindings, do you see a property 
for number of interrupts? No (well, maybe a few slipped in). You 
shouldn't need one either.

Rob

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?

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
> 

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 
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
>>

-- 
Thanks and Regards,
Varshini Rajendran.

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
>>>
> 

Answering to myself (again) and to Conor...

On 09/07/2024 at 16:06, Nicolas.Ferre@microchip.com wrote:
> 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"

Well, after a brief talk with Varshini and a review of the code, I'm not 
so sure it's worth re-writing this part anymore Conor...
It'll need changing 3-4 files (2 drivers and the "common" .h/.c files, 
because of the type change of ".data"); handling the special case of 
sama5d2 (smr_cache thing) and touching lot more code than what is done 
in v3 of this patch series.

Original design was probably not optimal, but well, it's simple, 
understandable and except if there is a big benefit in moving, I would 
prefer to keep it like this.
If you agree, we can ask Varshini to re-post a separated IRQ-focused 
series for handling sam9x75 changes.

Best regards,
   Nicolas

> 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, does not 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
>>>>
>>
>

On Thu, Jul 11, 2024 at 02:42:01PM +0200, Nicolas Ferre wrote:
> Answering to myself (again) and to Conor...
> 
> On 09/07/2024 at 16:06, Nicolas.Ferre@microchip.com wrote:
> > 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"
> 
> Well, after a brief talk with Varshini and a review of the code, I'm not so
> sure it's worth re-writing this part anymore Conor...
> It'll need changing 3-4 files (2 drivers and the "common" .h/.c files,
> because of the type change of ".data"); handling the special case of sama5d2
> (smr_cache thing) and touching lot more code than what is done in v3 of this
> patch series.
> 
> Original design was probably not optimal, but well, it's simple,
> understandable and except if there is a big benefit in moving, I would
> prefer to keep it like this.
> If you agree, we can ask Varshini to re-post a separated IRQ-focused series
> for handling sam9x75 changes.

I dunno, it's up to the folks that care about the driver whether they
want to do restructuring, not me. The nr-irqs property stays NAKed though,
since the information is determinable from the compatible.

Thanks,
Conor.

On Tue, 09 Jul 2024 15:06:29 +0100,
<Nicolas.Ferre@microchip.com> wrote:
> 
> 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).

In general, the least cruft we add to the DT after the facts, the
better. If the compatible string is good enough to identify the
device, I don't think we need to overthink it, specially as there is
no upside to non-standard properties here -- from what I understand,
the number of available interrupts is a property of the HW, and not
something that can be configured, making it part of the programming
model, just like the layout of registers.

But I'm not in a deciding position anymore, and this is only my
(educated) opinion.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

On 09/07/2024 at 16:06, Nicolas.Ferre@microchip.com wrote:
> On 09/07/2024 at 08:13, Varshini Rajendran - I67070 wrote:

[..]

> 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).

s/break the/does not break the/

> Best regards,
>     Nicolas
(sorry for the noise)

[..]