Mika Westerberg [off-list ref] writes:

On Mon, Nov 23, 2009 at 07:50:20PM +0100, ext Ferenc Wagner wrote:

quoted

Dmitry Torokhov [off-list ref] writes:

quoted

On Mon, Nov 23, 2009 at 05:42:08PM +0100, Ferenc Wagner wrote:

quoted

Mika Westerberg [off-list ref] writes:

quoted

Added new field to struct gpio_keys_button: irqflags which can
be used to specify exact irqflags the platform wants. If not specified,
it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.

--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c

@@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 		goto fail3;
 	}
 
-	error = request_irq(irq, gpio_keys_isr,
-			    IRQF_SHARED |
-			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			    desc, bdata);
+	if (button->irqflags == 0) {
+		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+			   IRQF_SHARED;
+	} else {
+		irqflags = button->irqflags;
+	}
+
+	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
 	if (error) {
 		dev_err(dev, "Unable to claim irq %d; error %d\n",
 			irq, error);

linux/interrupt.h says:

    When requesting an interrupt without specifying a IRQF_TRIGGER, the
    setting should be assumed to be "as already configured", which may
    be as per machine or firmware initialisation. 

So I'm not sure it's a good idea to exclude 0 from the possible irqflag
values.

Hmm... OTOH we can't just require the new irq flags to be always
specified, that would break all current users. Do we need a flag
indicating that the button will be using custom IRQ flags?

Or let for example -1 mean 0, which is ugly, to say the least.

How about adding (again) new field: bool custom_irqflags?

Thinking about it, since multiple buttons can share a single IRQ, this
can't be a per-button field after all.  Perhaps a better solution would
be adding a new mapping to the platform data:

struct gpio_keys_irq {
        int irq;
        int irqflags;
        int wakeup;                 /* replace the per-button field */
}

struct gpio_keys_platform_data {
        struct gpio_keys_button *buttons;
        int nbuttons;
        unsigned int rep:1;
        struct gpio_keys_irq *irqs; /* new member */
        int nirqs;                  /* new member */
};

quoted

Anyway, I'd much prefer an implementation which doesn't work against
having multiple buttons on a single IRQ line, which is the case for
example in arch/mips/bcm47xx/gpio.c.  Then the IRQ could still be
disabled if all its buttons are disabled, unless it's shared with
another driver (and IRQF_SHARED would control this aspect sharing).

Do you mean something like:

	if (button->custom_irqflags) {
		if (button->irqflags & IRQF_SHARED) {
			/* cannot mute this button */
		} else {
			...
	}

Not quite, because the default irqflags contain IRQF_SHARED.  More like:

disable_button (this);
if (button->custom_irqflags && !(button->irqflags & IRQF_SHARED)
    && (all buttons on this IRQ are disabled)) {
        disable_irq (gpio_to_irq (this));
}

In our case, we only have 1:1 mapping between GPIO lines and buttons so
current version works but if there are any (possible) users for this kind
of setup I can implement it.

I quoted (and own) a platform with such arrangement.  I'm somewhat short
on time to work on this hobby project, but of course I don't expect you
to do the heavy lifting.  Unfortunately I haven't yet managed to get a
clear picture about how to properly attack it (the problem is GPIO
access from the IRQ handler, if you didn't read the thread), so I
couldn't produce acceptable code (even though in my case gpio_get_value
can't sleep, so I've got working code).  Meanwhile, I'm trying to do my
best to not let things go in a direction which makes this change harder
that it currently is.  I'm really sorry this hurts your efforts.

Dmitry, what do you think?

Yep.
-- 
Thanks,
Feri.