Re: [PATCH v3 4/6] pinctrl: meson: Enable GPIO IRQs

[Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>]

On 02/12/15 11:37, Carlo Caione wrote:
> On Wed, Dec 2, 2015 at 10:03 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 01/12/15 19:41, Carlo Caione wrote:
>>> On Tue, Dec 1, 2015 at 8:16 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>>>> On 01/12/15 16:24, Carlo Caione wrote:
>>>>> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> [...]
> 
>>> In v2 I had the set of fwspec to track number and trigger type of the
>>> IRQ, so it was straightforward. With this patch I have moved away from
>>> that solution (as you suggested) and I'm using the 'amlogic,irqs-gpio'
>>> parameter to track down the IRQ numbers (but not the trigger type).
>>> It's the same solution we have in drivers/irqchip/irq-crossbar.c where
>>> the trigger type is hardcoded in allocate_gic_irq().
>>> If I need to save both the IRQ and the trigger type at this point I
>>> wonder if it's better to go back to the set of fwspec or convert the
>>> fwspec to of_phandle_args and save that.
>>
>> No. This should come from the interrupt specifier you are getting from
>> the device. You should never make up that information.
>>
>> Your amlogic,irqs-gpio property gives you a list of downstream
>> interrupts. The device connected to your pinctrl HW provides you with
>> the upstream interrupt number (which you will map to one of your
>> downstream IRQ) and crucially the trigger type. Please look at how the
>> TI cross bar works (again).
> 
> Ok, this definitely makes sense and I'm going to fix it in the next
> revision, thanks for the explanation. Still I fail to see how the TI
> cross bar driver is actually doing what you are suggesting here. If
> I'm correctly reading the code here
> http://lxr.free-electrons.com/source/drivers/irqchip/irq-crossbar.c#L101
> the driver is hardcoding the trigger type for the downstream IRQ to be
> passed to the GIC code. But probably I'm missing something obvious.

Damn. No, you're right. I'll fix that. Thanks for the heads up, and
apologies for the shouting! ;-)

>>>>> +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>>> +{
>>>>> +     struct meson_domain *domain = to_meson_domain(chip);
>>>>> +     struct meson_pinctrl *pc = domain->pinctrl;
>>>>> +     struct meson_bank *bank;
>>>>> +     struct irq_fwspec irq_data;
>>>>> +     unsigned int hwirq, irq;
>>>>> +
>>>>> +     hwirq = domain->data->pin_base + offset;
>>>>> +
>>>>> +     if (meson_get_bank(domain, hwirq, &bank))
>>>>> +             return -ENXIO;
>>>>> +
>>>>> +     irq_data.param_count = 2;
>>>>> +     irq_data.param[0] = hwirq;
>>>>> +
>>>>> +     /* dummy. It will be changed later in meson_irq_set_type */
>>>>> +     irq_data.param[1] = IRQ_TYPE_EDGE_RISING;
>>>>
>>>> Blah. Worse than I though... How do you end-up here? Why can't you
>>>> obtain the corresponding of_phandle_args instead of making things up?
>>>
>>> because I do not have a of_phandle. This is basically the .to_irq hook
>>> of the gpio_chip. This code is being called programmatically from the
>>> gpiolib. No DTS/OF involved here.
>>>
>>>> This looks mad. Do you really need this?
>>>
>>> Well, I'm open to any suggestion on how improve this mess.
>>
>> The question to answer is: in what circumstances do you have to convert
>> a GPIO into an IRQ at runtime? The only case should be "when you
>> discover a device having an interrupt pointing to your pinctrl". And in
>> this case, you have all the information to reconfigure the HW and assign
>> the interrupt.
>>
>> I really don't get why you want or need to involve gpiolib in this.
> 
> Again probably I'm missing something (and Linus probably could help
> here) but the only place I see the .to_irq hook (that is
> meson_gpio_to_irq() in the driver code) being called is from
> gpiod_to_irq() function in the gpiolib code.
> 
> One practical case in which that code path is involved is when (for
> example) I have something like 'cd-gpios = <&gpio GPIOX 0>;' for the
> card detection IRQ in the MMC node and in this case I fail to see an
> easy way to get the trigger type without touching the MMC / gpiolib
> code (any idea?).
> This function is not called at all when in the DTS I explicitly have
> the interrupt specifier defined using the 'interrupts = <...>'
> property and in that case I have all the information I need to map the
> downstream IRQ.

I do think that the moment you want to describe an interrupt (and have
the HW that can trigger one), you should end up describing this as a
real interrupt, and not as a "shake this pin" kind of thing.

The former gives you strong guarantees as to how this is going to be
processed, while the later looks like a hack to paper over missing
functionalities in past kernels...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
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