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

[Conor Dooley <conor@kernel.org>]

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

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.

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