linux-kernel - RE: [PATCH] gpio: pca953x: add irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <EADF0A36011179459010BDF5142A457501C9F468C7@pdsmsx502.ccr.corp.intel.com>
Date:	Sat, 4 Jul 2009 09:57:17 +0800
From:	"Du, Alek" <alek.du@...el.com>
To:	David Brownell <david-b@...bell.net>,
	"peterz@...radead.org" <peterz@...radead.org>
CC:	Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: RE: [PATCH] gpio: pca953x: add irq_chip for irq demuxing

David,

Thanks for the review.
Basically my idea is *not* actually demuxing the I2C gpio interrupts. Since the demuxing need send I2C command to the GPIO chip, which has big latency.
My idea is, when this GPIO chip hooked to an edge sensitive interrupt line, we could use this interrupt handler to call all second level IRQs. The hooked edge detect interrupt line "automatically" masks the interrupt when the GPIO pin state is not restored to original state.

>> +static void pca953x_irq_handler(unsigned irq, struct irq_desc *desc)
>> +{
>> +	struct pca953x_chip *chip = (struct pca953x_chip *)get_irq_data(irq);
>> +	int i;
>> +
>> +	if (desc->chip->ack)
>> +		desc->chip->ack(irq);
>
>This top-level IRQ chaining handler is clelarly incorrect.
>Reading from a pca9539 spec I have handy:
>
>	Resetting the interrupt circuit is achieved when
>	data on the port is changed to the original setting,
>	data is read from the port that generated the
>	interrupt, or in a Stop event.
>
>You're not guaranteeing *any* of those happens.  Since
>the chip's nINT output is level-active, it's possible
>that the hardware is still raising this interrupt when
>this hander returns ...

That's why we connected the nINT output to an edge sensitive interrupt line.
The IRQ would be triggered only once if the nINT keeps level-low. The driver that requires the gpio pin IRQ needs to clear the interrupt source.

>> +	/* we must call all sub-irqs, since there is no way to read
>> +	 * I2C gpio expander's status in irq context. The driver itself
>> +	 * would be reponsible to check if the irq is for him.
>> +	 */
>> +	for (i = 0; i < chip->gpio_chip.ngpio; i++)
>> +		generic_handle_irq(chip->irq_base + i);
>
>You should only do that for pins configured as inputs.
>The nIRQ signal is not triggered for changes on output pins.
Ok, but those output pins, actually nobody will install IRQ handlers for them.

>> +	if (chip->irq_base) {
>> +		int i;
>> +
>> +		set_irq_type(client->irq,
>> +				IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING);
>
>Won't work on all hardware.  And surely you'd want just
>EDGE_FALLING, so you get only half as many IRQs, if you
>happen to be hooking this up to an interrupt line which
>supports edge-sensitive IRQ triggers (and not through
>some kind of signal inverter)?

I set the IRQ type to EDGE_FALLING and EDGE_RISING because the nINT signal of the GPIO is low-active. So when one of the input pins transit from high/low, it will trigger two interrupts on the hooked up interrupt line.


Thanks,
Alek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/