Re: [PATCH v2] gpio: pl061: use all specified IRQs for chained handler

[Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>]

Hi!

On 22/02/17 13:47, Linus Walleij wrote:
>> There are several implementations of PL061 which lack GPIOINTR signal in
>> hardware and only have individual GPIOMIS[7:0] interrupts. It's possible
>> to support these variants with minimal changes to the driver just
>> requesting all these IRQs for the same chained handler. While the solution
>> seems to be suboptimal, this is just a quirk for some particular IPs.
>>
>> Power Management (wakeup) is not expected to work with these IPs. Only the
>> basic GPIO functionality.
>>
>> One in-tree example is arch/arm/boot/dts/axm55xx.dtsi, pl061 instances have
>> 8 IRQs defined, but current driver supports only the first one, so only one
>> pin would work as IRQ trigger.
>>
>> Reported-by: Sławomir Stępień <slawomir.stepien@xxxxxxxxx>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
>> Cc: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
>> Cc: linux-gpio@xxxxxxxxxxxxxxx
>> Cc: Sławomir Stępień <slawomir.stepien@xxxxxxxxx>
>> ---
>> Changes since v1:
>> - Added AMBA_NR_IRQS loop limit
> Nice in a way, but is this really what we want to do?
> 
> This means that the platform has assigned individual IRQs to
> all of the lines and no IRQ to the accumulated IRQ GPIOINTR
> if I understand it correctly.
> 
> In that case these IRQs are 1-to-1 and should be modeled using
> a hierarchical irqdomain, because the loop in pl061_irq_handler()
> is unnecessary, we already know which IRQ line has been fired,
> right?
> 
> I think we need to add a mechanism in the gpiolib core to handle
> also hierarchical irqdomains and this is a good opportunity.
> 
> I would make a patch that:
> 
> - If the device has 1 IRQ line, assume it is GPIOINTR and work
>   as before.
> 
> - If the component has 8 IRQ lines, create a hierarchical IRQdomain
>   and chip using a gpiolib core helper.

This was an option of course, the only this is, PL061 spec says, there is
GPIOINTR and if someone has forgot it, we can support it with a quirk.
It's not specified to work this way at all, so why spend any resources
and add second implementation to the driver?

But if you are sure it's the way to go -- I can provide another patch, as
I do not have strong opinion on this story.

> - If not 1 or 8 lines, bail out with an error.

Well, I'd divide it into "1" and "!= 1" cases... Who knows, maybe
only 4 GPIOs will be implemented...

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html