On Tue, Mar 28, 2017 at 12:36 PM, Gregory CLEMENT
<gregory.clement@xxxxxxxxxxxxxxxxxx> wrote:
> On lun., mars 27 2017, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>>> + u32 virq = irq_linear_revmap(d, hwirq +
>>> + i * GPIO_PER_REG);
>>
>> Use irq_find_mapping() instead please.
>
> As we are in the interrupt handler I chose to use this function because
> according to its documentation: "This is a fast path alternative to
> irq_find_mapping() that can be called directly by irq controller code to
> save a handful of instructions".
>
> The only restriction is "It is always safe to call, but won't find irqs
> mapped using the radix tree.". So I think that for this driver it is
> okay.
So you are relying on the core code in gpiolib to select a linear
map. That is implicit semantics, and either all drivers using
GPIOLIB_IRQCHIP should be changed to irq_linear_revmap()
or all stay with irq_find_mapping().
In this case, I doubt it that you are actually so timing critical that
it matters. Please use irq_find_mapping().
>>> + nr_irq_parent = of_irq_count(np);
>>> + spin_lock_init(&info->irq_lock);
>>> +
>>> + if (!nr_irq_parent) {
>>> + dev_err(&pdev->dev, "Invalid or no IRQ\n");
>>> + return 0;
>>> + }
>>
>> What if it is > 1? That doesn't seem to work but will pass this
>> check silently.
>
> If we have nr_irq_parent > 1, it will work and it is actually expected.
Ah, I get it. Nice.
>>> + ret = gpiochip_irqchip_add(gc, irqchip, 0,
>>> + handle_level_irq, IRQ_TYPE_NONE);
>>
>> If you also set up the handler in .set_type() you can assign
>> handle_bad_irq() here and let .set_type set the right handler
>> as e.g. drivers/gpio/gpio-pl061.c.
>
> Well the hardware can only manage the edge trigger, so there is no
> benefit to modify it each time we want to change the kind of edge we
> want (raising or falling). But your comment make me realized that I used
> the wrong one, I will move to handle_edge_irq in the v4.
Ooops, yeah handle_edge_irq() is what calls the ACK callback.
>>> + for (i = 0; i < nrirqs; i++) {
>>> + struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
>>> +
>>> + d->mask = 1 << (i % GPIO_PER_REG);
>>> + }
>>
>> What is this? It looks like a big hack. At least put in a fat
>> comment about what is going on and why.
>
> I can reuse a part of the commit log here: "The only unusual "feature"
> is that many interrupts are connected to the parent interrupt
> controller. But we do not take advantage of this and use the chained irq
> with all of them."
What you're doing is mocking around with core irqchip semantics.
Is ->mask really supposed to be played around with from the outsid
like this?
Anyways: BIT(i % GPIO_PER_REG) is nicer.
>>> + for (i = 0; i < nr_irq_parent; i++) {
>>> + int irq = irq_of_parse_and_map(np, i);
>>
>> I think gpiochip_irqchip_add() will do this for you already,
>> as it calls irq_create_mapping() for all offsets which will call
>> irq_of_parse_and_map() am I right?
>
> After reading the code, it doesn't seem it is the case. At least there
> is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht
> we need here is to associate each IRQ to the same GPIO handler.
>
> I can still try without this line to confirm it.
It has irq_create_mapping(gpiochip->irqdomain, offset); that get
called for every IRQ, and that will eventually call irq_of_parse_and_map()
if the IRQs are defined in the device tree. (IIRC)
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
