On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > On Tue, 19 May 2015 09:40:21 +0100 > Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > >> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@xxxxxxx> wrote: >> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: >> >> >> But surely the GPIO block has its own status register, so are >> >> you saying that this register is unreliable? >> > >> > When the GPIO is used as interrupt, the gpio block does not report the >> > status anymore. Which leaves us stuck with SPISR. >> >> >> >> I can think of a few reasons, like transient IRQs etc but >> >> what is actually causing this? >> > >> > I won't say the obvious. >> >> Yeah I see your problem now :( >> >> I think it's better to fix the access functions so that you can >> cross-call to the GIC driver to get the SPISR flag out though. >> Let's see what Marc says. >> >> >> Which GPIO driver is this? Is it upstream? >> > >> > Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to >> > think that we ought to switch to use some gpio poll driver rather than >> > using gpio-key. >> >> There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah >> that's possible. But honestly I think it's better to deal with this >> problem for real because IRQ is more efficient. >> >> So the way I perceive it this is the real problem: >> >> +static int gic_irq_get_irqchip_state(struct irq_data *d, >> + enum irqchip_irq_state which, bool *val) >> +{ >> + switch (which) { >> (...) >> + case IRQCHIP_STATE_ACTIVE: >> + *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET); >> + break; case: read >> from 0xd04 (SPISR) instead, because that makes more >> sense to me, or am I wrong at it? >> >> + case IRQCHIP_STATE_LINE_LEVEL: >> + *val = gic_peek_irq(d, GIC_DIST_SPISR); >> + break; >> >> And then put a define into <linux/irqchip/arm-gic.h> for >> GIC_DIST_SPISR. > > What worries me here is that the PENDING state should already give you > the right level of information (this is what the GIC-400 TRM says). The > only reason why SPISR exists is that software can write to the PENDING > register, while SPISR is RO. > > If reading the pending state doesn't work, then I'd like to know > exactly *why*. Only then we can add support for LINE_LEVEL using SPISR > (which has to be GIC-400 specific, as it is not architected). IS_PENDING and IS_ACTIVE works fine for the ISR context. However, the nature of the register is meant for IRQ handling and not to read the status of a GPIO. By the time the gpio_key throws a work queue and check the status of the PENDING register, it is no long relevant. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny. -- 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