Re: [linux-meson] Re: [PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>]

On Tue, Nov 24, 2015 at 9:28 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On Mon, 23 Nov 2015 11:16:54 +0100
> Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
>
>> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>>
>> On Meson8 and Meson8b SoCs there are 8 independent filtered GPIO
>> interrupt modules that can be programmed to use any of the GPIOs in the
>> chip as an interrupt source.
>>
>> For each GPIO IRQ we have:
>>
>> GPIOs --> [mux]--> [polarity]--> [filter]--> [edge select]--> GIC
>>
>> The eight GPIO interrupts respond to mask/unmask/clear/etc.. just like
>> any other interrupt in the chip. The difference for the GPIO interrupts
>> is that they can be filtered and conditioned.
>>
>> This patch adds support for the external GPIOs interrupts and enables
>> them for Meson8 and Meson8b SoCs.
>>
>> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> ---
>
> [...]
>
>> +     for (i = 0; i < pc->num_gic_irqs; i++) {
>> +             struct of_phandle_args oirq;
>> +
>> +             of_irq_parse_one(node, i, &oirq);
>> +             irq_of_phandle_args_to_fwspec(&oirq, &pc->gic_irqs[i]);
>> +
>> +             pc->irq_map[i] = IRQ_FREE;
>> +     }
>
> The whole thing feels weird. Why do you need to keep a set of fwspecs?

We only have 8 IRQs on the GIC side that we can use for the GPIOs. The
set of fwspec is used to track these IRQs.
At probe time we read from the DTS how many and which IRQs are
reserved on the GIC for the GPIOs. Every time a GPIO IRQ is installed,
we use one of these IRQ lines.

> All you need is a range of interrupts that would be conveniently
> represented by a bitmap (assuming your interrupts space is a mostly
> contiguous range).

I could do that but it feels to me like we end up hiding from the DTS
some information that naturally should be in there: a pinctrl device
that is using 8 interrupt lines of its interrupt controller. IMO this
is not a special case that requires some special treatment.

> Overall, this patch is quite hard to review. Can you please split the
> GPIO management from the irqchip side?

Sure. Fix in v2.

Thanks for the review,

-- 
Carlo Caione
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html