Mika Westerberg <ext-mika.1.westerberg@xxxxxxxxx> writes:
> On Mon, Nov 23, 2009 at 07:50:20PM +0100, ext Ferenc Wagner wrote:
>> Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> writes:
>>
>>> On Mon, Nov 23, 2009 at 05:42:08PM +0100, Ferenc Wagner wrote:
>>>> Mika Westerberg <ext-mika.1.westerberg@xxxxxxxxx> writes:
>>>>
>>>>> 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 */
};
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
