Re: [PATCH] gpio: Add Tegra186 support

[Linus Walleij]

On Wed, Nov 23, 2016 at 8:44 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
>> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
>> <thierry.reding@xxxxxxxxx> wrote:
>>
>> > From: Thierry Reding <treding@xxxxxxxxxx>
>> >
>> > Tegra186 has two GPIO controllers that are largely register compatible
>> > between one another but are completely different from the controller
>> > found on earlier generations.
>> >
>> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>>
>> I would prefer if you could try to convert this driver to use
>> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
>> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
>> It would save us so much trouble and so much complicated
>> code to maintain for this custom irqdomain.
>>
>> I suggest you to look into the mechanisms mentioned in my
>> previous mail for how to poke holes in the single linear
>> irqdomain used by this mechanism.
>>
>> As it seems, you only have one parent interrupt with all
>> these IRQs cascading off it as far as I can tell.
>
> Like I said in other email, I don't think this will work because the
> GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> parent interrupt is used. We could possibly live with a single IRQ
> handler and data, but we definitely need to request multiple IRQs if
> we want to be able to use all GPIOs as interrupts.

Sorry if I missed that.

Actually it works.

If you look at gpiochip_set_chained_irqchip() it just calls
irq_set_chained_handler_and_data() for the parent interrupt.

There is a loop afterwards that call irq_set_parent()
on all child IRQs but to the kernel this is just a noop I
never fully grasped that thing. Maybe it should even be deleted.
It just sets the parent in the irq descriptor, and the IRQ core only
ever use this on threaded interrupts, so maybe it should
really just be set on those (nested) interrupts.

Maybe Marc/tglx could shed some light on the intended
use of irq_set_parent() given that only two MFD drivers
and the gpiolib uses it. I think I need to dig into some
commitlogs.

If it is semantically required to call irq_set_parent() with the
right child/parent, we could add an
gpiochip_set_chained_irqchip_interval(first, last) to set the
parent for just an interval of the offsets.

Yours,
Linus Walleij