Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Jason,

On 07/05/2016 04:41 PM, Jason Cooper wrote:
> Hey Sebastian, Mason,
> 
> * Please fix mailer to wrap text at a sane length.  I've re-wrapped and
> trimmed.
> 
> On Tue, Jul 05, 2016 at 02:30:12PM +0200, Sebastian Frias wrote:
>> On 07/04/2016 02:11 PM, Mason wrote:
> ...
>>>>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
>>>
>>> In the *actual* submission, we can't use a wildcard like smp87xx
>>> we'll have to use an actual part number.
>>
>> Are you sure?
>> That would hinder genericity.
>> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").
> 
> sigma,smp-irqrouter.txt should be fine.  The devicetree maintainers
> should yelp if they want something different.
> 
>> To me there's no need to link the compatible string of a given HW
>> module with that of the chip name the module it is embedded into.  For
>> example, the generic USB3 driver is "generic-xhci".  While this module
>> is not generic to be embedded in chips from different manufacturers,
>> it is supposed to be generic within Sigma, and multiple Sigma chips
>> (with potentially different denominations) can use it.
>>
>>>
>>>>  drivers/irqchip/Makefile                           |   1 +
>>>>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
>>>
>>> Likewise, I don't like the "_v2" suffix, it's too generic.
>>> Actual submission should use something more specific.
>>
>> Well, the other driver is irq-tango.c that is generic as well.
>> I prefer versioning, as it is unrelated with the actual chip name.
> 
> Is there a name, similar to 'tango', for this version of the IP?
> Something that would spark recognition for someone looking for "the damn
> driver for this XYZ irqchip I have".  If not, irq-tango_v2.c is fine.
> 

Thanks for your comments.
So, aside from some naming issues, do you think the driver is ok?
Indeed, I'm not sure about the following:

1) I had to unroll irq_of_parse_and_map() in order to get the HW
IRQ declared in the device tree so that I can associate it with
a given domain. Is there another way?

2) I'm not sure about the DT specification, in particular, the use
of child nodes to declare different domains, but it works.
Advise and/or hints regarding this are welcome.

3) I'm calling this an irq router to somehow highlight the fact
that it is not a simple interrupt controller. Indeed it does
not latch the IRQ lines by itself, and does not supports edge
detection.
Does anybody else has a better idea for the name?

4) Do I have to do something more to handle the affinity stuff?

5) checkpatch.pl reports warnings, etc. but I guess it's ok for
now since it is a RFC 

6) Do we need a lock in the cascade_irq callback set with
irq_set_chained_handler_and_data() ?

Thanks in advance.
Best regards,

Sebastian