[v2,3/5,RFC] gpio: pca953x: hack to map LEVEL irqs to EDGE irqs (may loose interrupts/hang)

Message ID 637f00dd7da98cfeffdc6080914a5f67b433da50.1522868423.git.hns@goldelico.com
State New
Headers show
Series
  • none
Related show

Commit Message

H. Nikolaus Schaller April 4, 2018, 7 p.m.
So far the interrupt type from the DTS is ignored, i.e.

	interrupt-parent = <&pcal6524>
	interrupts = <10 IRQ_TYPE_EDGE_RISING>

does not overwrite a

	devm_request_threaded_irq(..., IRQ_TYPE_LEVEL_LOW, ...)

in driver code. Therefore, the pca953x driver rejects the
setup of the irq because it can only handle EDGE interrupts
so far.

This hack translates level interrupts to edge interrupts
for the pca953x chips. This is enough for initial testing,
but not a good solution since interrupts may be lost.

If for example the connected chip requests a IRQ_TYPE_LEVEL_LOW
this may have the reason that there may be multiple different
interrupt sources in the chip - wired-or together to the
input of the pca953x. Now if we do edge detecion only,
the first interrupt will generate an EDGE_FALLING, but a
second one won't ever if the first interrupt wasn't already
processed.

IMHO a better solution would be to make the pca953x interrupt
handler check if the irq input is still in the active
level and run the device specific handler again.

But this is a major rework that I must leave to others
with more knowledge and time to work on it. This RFC
patch is for having a reminder.

Not Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpio/gpio-pca953x.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andy Shevchenko April 10, 2018, 2:38 p.m. | #1
On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> in driver code. Therefore, the pca953x driver rejects the
> setup of the irq because it can only handle EDGE interrupts
> so far.
>
> This hack translates level interrupts to edge interrupts
> for the pca953x chips. This is enough for initial testing,
> but not a good solution since interrupts may be lost.
>
> If for example the connected chip requests a IRQ_TYPE_LEVEL_LOW
> this may have the reason that there may be multiple different
> interrupt sources in the chip - wired-or together to the
> input of the pca953x. Now if we do edge detecion only,
> the first interrupt will generate an EDGE_FALLING, but a
> second one won't ever if the first interrupt wasn't already
> processed.
>
> IMHO a better solution would be to make the pca953x interrupt
> handler check if the irq input is still in the active
> level and run the device specific handler again.

What I thought about is to enable such exclusively for "L" variants of
the chips and check latched values.
H. Nikolaus Schaller April 10, 2018, 4:06 p.m. | #2
Hi Andy,

> Am 10.04.2018 um 16:38 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> in driver code. Therefore, the pca953x driver rejects the
>> setup of the irq because it can only handle EDGE interrupts
>> so far.
>> 
>> This hack translates level interrupts to edge interrupts
>> for the pca953x chips. This is enough for initial testing,
>> but not a good solution since interrupts may be lost.
>> 
>> If for example the connected chip requests a IRQ_TYPE_LEVEL_LOW
>> this may have the reason that there may be multiple different
>> interrupt sources in the chip - wired-or together to the
>> input of the pca953x. Now if we do edge detecion only,
>> the first interrupt will generate an EDGE_FALLING, but a
>> second one won't ever if the first interrupt wasn't already
>> processed.
>> 
>> IMHO a better solution would be to make the pca953x interrupt
>> handler check if the irq input is still in the active
>> level and run the device specific handler again.
> 
> What I thought about is to enable such exclusively for "L" variants of
> the chips and check latched values.

Yes, something like this, although we have to think about if the same can't
be done for other chips.

The question is who will pick up this topic and when. My job for this chip
was to check if it can basically work and the hack shows that, but others
must make the driver perfect.

So for the moment I keep it in our workplace and remove it from the
patch set.

I am sure that there will be some more kernel developers as soon
as the Pyra handheld hardware is more widely available.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index c70acba710c7..c21cfdee2eb6 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -521,6 +521,11 @@  static int pca953x_irq_set_type(struct irq_data *d, unsigned int type)
 	int bank_nb = d->hwirq / BANK_SZ;
 	u8 mask = 1 << (d->hwirq % BANK_SZ);
 
+	if (type & IRQ_TYPE_LEVEL_LOW)
+		type |= IRQ_TYPE_EDGE_FALLING;
+	if (type & IRQ_TYPE_LEVEL_HIGH)
+		type |= IRQ_TYPE_EDGE_RISING;
+
 	if (!(type & IRQ_TYPE_EDGE_BOTH)) {
 		dev_err(&chip->client->dev, "irq %d: unsupported type %d\n",
 			d->irq, type);