RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi <asmaa@nvidia.com>]

> > +	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> > +	val |= BIT(offset);
> > +	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> 
> What exactly does this do? It appears to clear the interrupt, if i understand
> mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is
> correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear
> to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled?
> 
> Asmaa>>
> YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. Otherwise, we
> will not receive further interrupts.

> If the interrupt status bit is set, as soon as you unmask the interrupt, the hardware should fire
> the interrupt. At least, that is how interrupt controllers usually work.

> A typical pattern is that the interrupt fires. You mask it, ack it, and then do what is needed to
> actually handle the interrupt. While doing the handling, the hardware can indicate the interrupt
> again. But since it is masked nothing happened. This avoids your interrupt handler going
> recursive.
> Once the handler has finished, the interrupt is unmasked. At this point it actually fires, triggering
> the interrupt handler again.

Asmaa>> mlxbf2_gpio_irq_enable seems to be called only once when the driver is loaded.
And I will actually remove mlxbf2_gpio_irq_ack because it is not being called at all.
After further investigation, that function is called via chained_irq_enter which is itself invoked in
the interrupt handler. It should have looked something like this:

static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr)
{
    chained_irq_enter(gc->irq->chip, desc);
    // rest of the code here
    chained_irq_exit(gc->irq->chip, desc);
}

But in our case, we decided to directly request the irq  instead of passing a flow-handler to
gpiochip_set_chained_irqchip, because the irq has to be marked as shared (IRQF_SHARED).
gpio-mt7621.c does something similar.
Moreover, whenever an interrupt is fired by HW, it is automatically disabled/masked until
it is explicitly cleared by Software. And this line takes care of it in mlxbf2_gpio_irq_handler:
writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);

After a HW reset, all gpio interrupts are disabled by default by HW. The HW will not signal
any gpio interrupt as long as all bits in YU_GPIO_CAUSE_OR_EVTEN0 are 0.
In mlxbf2_gpio_irq_enable, we configure a specific gpio as an interrupt by writing 1 to
YU_GPIO_CAUSE_OR_EVTEN0. I just wanted to make sure there is no trash value in
YU_GPIO_CAUSE_OR_CLRCAUSE before enabling gpio interrupt support.
So pending interrupts in YU_GPIO_CAUSE_OR_CLRCAUSE only matters if
YU_GPIO_CAUSE_OR_EVTEN0 is set accordingly.
Does this answer your question?

> Please also get your email client fixed. I wrap my emails at around 75 characters. Your mailer
> has destroyed it. Your text should also be wrapped at about 75 characters.

Asmaa>> Sorry about that. I wrapped my outlook emails around 75 characters, I hope it works.