Re: [PATCH] Input: add MAX7359 key switch controller driver, v2
From: Dmitry Torokhov
Date: Wed Sep 16 2009 - 04:58:07 EST
On Wed, Jul 15, 2009 at 09:15:34AM +0200, Marek Szyprowski wrote:
> Hello,
>
> On Tuesday, July 14, 2009 12:23 PM Trilok Soni wrote:
>
> > On Tue, Jul 14, 2009 at 3:48 PM, Marek
> > Szyprowski<m.szyprowski@xxxxxxxxxxx> wrote:
> > > Hello,
> > >
> > > On Tuesday, July 14, 2009 11:12 AM, Trilok Soni wrote:
> > >
> > >> On Tue, Jul 14, 2009 at 2:37 PM, Marek
> > >> Szyprowski<m.szyprowski@xxxxxxxxxxx> wrote:
> > >> > Hello,
> > >> >
> > >> > On Tuesday, July 14, 2009 10:25 AM, Dmitry Torokhov wrote:
> > >> >
> > >> >> On Tue, Jul 14, 2009 at 08:28:05AM +0200, Marek Szyprowski wrote:
> > >> >> > Hello,
> > >> >> > On Tuesday, July 14, 2009 5:10 AM, Kim Kyuwon wrote:
> > >> >> > > Dmitry Torokhov wrote:
> > >> >> > > > On Mon, Jul 13, 2009 at 02:22:10PM +0530, Trilok Soni wrote:
> > >> >> > > >> I don't see this driver picked up yet in your -next branch. We should
> > >> >> > > >> target this driver to be mainlined in next merge window. This is very
> > >> >> > > >> important driver for some of the embedded systems, including palm pre
> > >> >> > > >> :)
> > >> >> > > > I was wondering if somebody could test the patch below and if it still
> > >> >> > > > works then I will apply to the next branch. Thanks!
> > >> >> > > >
> > >> >> > >
> > >> >> > > Dear Marek,
> > >> >> > >
> > >> >> > > Because I don't have the NCP board(which includes the max7359 keypad)
> > >> >> > > now, I can't test this patch. Marek, could you please test this patch?
> > >> >> >
> > >> >> > I would like to, but I could not find the base version to which I can apply
> > >> >> > that patch. I've tried v2 version posted in '[PATCH] Input: add MAX7359 key
> > >> >> > switch controller driver, v2' mail from Sat 2009-05-09 04:10 with 2 patches
> > >> >> > posted in replies to that main, but the latest patch still fails to apply.
> > >> >> >
> > >> >> > Could someone send me a complete patch, so I can do a test?
> > >> >> >
> > >> >>
> > >> >> Sending everything as attachments, maybe that will help...
> > >> >
> > >> > Ok. I've did the tests.
> > >> >
> > >> > MAX7359 keypad driver works after your patch, but reports much more events than
> > >> > the previous version. In this test I pressed quickly the first button on the
> > >> > keypad.
> > >> >
> > >> > Old version:
> > >> > NCP:~# hexdump /dev/input/event0
> > >> > 0000000 0037 0000 e733 000b 0001 00e7 0001 0000
> > >> > 0000010 0037 0000 e748 000b 0000 0000 0000 0000
> > >> > 0000020 0037 0000 94e2 000d 0001 00e7 0000 0000
> > >> > 0000030 0037 0000 94f3 000d 0000 0000 0000 0000
> > >> >
> > >>
> > >> Please use evtest instead. It will give better output atleast.
> > >
> > > Ok.
> > >
> > > Old version (clean v2 patch):
> > >
> > > NCP:~# evtest /dev/input/event0
> > > Input driver version is 1.0.0
> > > Input device ID: bus 0x18 vendor 0x0 product 0x0 version 0x0
> > > Input device name: "max7359"
> > > Supported events:
> > > Event type 0 (Sync)
> > > Event type 1 (Key)
> > > Event code 107 (End)
> > > Event code 139 (Menu)
> > > Event code 148 (Prog1)
> > > Event code 149 (Prog2)
> > > Event code 177 (ScrollUp)
> > > Event code 178 (ScrollDown)
> > > Event code 212 (Camera)
> > > Event code 231 (?)
> > > Event code 474 (?)
> > > Event type 20 (Repeat)
> > > Testing ... (interrupt to exit)
> > > Event: time 38.740081, type 1 (Key), code 139 (Menu), value 1
> > > Event: time 38.740101, -------------- Report Sync ------------
> > > Event: time 38.850061, type 1 (Key), code 139 (Menu), value 0
> > > Event: time 38.850077, -------------- Report Sync ------------
> > >
> > > New version (updated platform definition to use struct matrix_keymap_data instead of
> > max7359_keypad_platform_data):
> > >
> > > NCP:~# evtest /dev/input/event0
> > > Input driver version is 1.0.0
> > > Input device ID: bus 0x18 vendor 0x0 product 0x0 version 0x0
> > > Input device name: "max7359"
> > > Supported events:
> > > Event type 0 (Sync)
> > > Event type 1 (Key)
> > > Event code 107 (End)
> > > Event code 139 (Menu)
> > > Event code 148 (Prog1)
> > > Event code 149 (Prog2)
> > > Event code 177 (ScrollUp)
> > > Event code 178 (ScrollDown)
> > > Event code 212 (Camera)
> > > Event code 231 (?)
> > > Event code 474 (?)
> > > Event type 4 (Misc)
> > > Event code 4 (ScanCode)
> > > Event type 20 (Repeat)
> > > Testing ... (interrupt to exit)
> > > Event: time 75.680066, type 4 (Misc), code 4 (ScanCode), value 01
> > > Event: time 75.680095, type 1 (Key), code 139 (Menu), value 1
> > > Event: time 75.680107, -------------- Report Sync ------------
> > > Event: time 75.700072, type 4 (Misc), code 4 (ScanCode), value 3f
> > > Event: time 75.700095, -------------- Report Sync ------------
> > > Event: time 75.830064, type 4 (Misc), code 4 (ScanCode), value 01
> > > Event: time 75.830093, type 1 (Key), code 139 (Menu), value 0
> > > Event: time 75.830100, -------------- Report Sync ------------
> > > Event: time 75.850073, type 4 (Misc), code 4 (ScanCode), value 3f
> > > Event: time 75.850097, -------------- Report Sync ------------
> > >
> > > Something is definitely different. It looks that I missed a patch that added some additional events,
> > because I don't think that the
> > > threaded irq patch would cause this.
> > >
> >
> > Nope, it is not because of threaded irq patch but MSC_SCAN event
> > generation. Not to worry.
>
> I'm sorry for the commotion, but I did the test in a wrong way. I
> thought Dmitry has sent me a patch with the threaded irq already
> integrated. Joonyoung Shim has pointed me that I was wrong. I had to
> apply the threaded irq patch on top of the patch Dmitry has sent me.
>
> To sum up - the threaded irq version does not work here on ARM S3C6410
> NCP board. In /proc/interrupts I only noticed that only 1 interrupt
> has been triggered. No events are reported. Same was with Melfas
> Touchscreen driver (also only 1 interrupt triggered).
>
Now that most issues have with threaded IRQs have been fixed in mainline
would you mind retesting the threaded IRQ patch? Below is the latest
version of it.
Thanks!
--
Dmitry
Input: max7359 - use threaded IRQs
From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
Convert max7359 driver to use IRQ threading instead of using
workqueue.
Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---
drivers/input/keyboard/max7359_keypad.c | 47 ++++++++-----------------------
1 files changed, 12 insertions(+), 35 deletions(-)
diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
index 8b3ee14..768c204 100644
--- a/drivers/input/keyboard/max7359_keypad.c
+++ b/drivers/input/keyboard/max7359_keypad.c
@@ -58,12 +58,8 @@ struct max7359_keypad {
/* matrix key code map */
unsigned short keycodes[MAX7359_MAX_KEY_NUM];
- struct work_struct work;
-
struct input_dev *input_dev;
struct i2c_client *client;
-
- u32 irq;
};
static int max7359_write_reg(struct i2c_client *client, u8 reg, u8 val)
@@ -106,10 +102,10 @@ static void max7359_build_keycode(struct max7359_keypad *keypad,
__clear_bit(KEY_RESERVED, input_dev->keybit);
}
-static void max7359_worker(struct work_struct *work)
+/* runs in an IRQ thread -- can (and will!) sleep */
+static irqreturn_t max7359_interrupt(int irq, void *dev_id)
{
- struct max7359_keypad *keypad =
- container_of(work, struct max7359_keypad, work);
+ struct max7359_keypad *keypad = dev_id;
struct input_dev *input_dev = keypad->input_dev;
int val, row, col, release, code;
@@ -120,25 +116,13 @@ static void max7359_worker(struct work_struct *work)
code = MATRIX_SCAN_CODE(row, col, MAX7359_ROW_SHIFT);
+ dev_dbg(&keypad->client->dev,
+ "key[%d:%d] %s\n", row, col, release ? "release" : "press");
+
input_event(input_dev, EV_MSC, MSC_SCAN, code);
input_report_key(input_dev, keypad->keycodes[code], !release);
input_sync(input_dev);
- enable_irq(keypad->irq);
-
- dev_dbg(&keypad->client->dev, "key[%d:%d] %s\n", row, col,
- (release ? "release" : "press"));
-}
-
-static irqreturn_t max7359_interrupt(int irq, void *dev_id)
-{
- struct max7359_keypad *keypad = dev_id;
-
- if (!work_pending(&keypad->work)) {
- disable_irq_nosync(keypad->irq);
- schedule_work(&keypad->work);
- }
-
return IRQ_HANDLED;
}
@@ -226,8 +210,6 @@ static int __devinit max7359_probe(struct i2c_client *client,
keypad->client = client;
keypad->input_dev = input_dev;
- keypad->irq = client->irq;
- INIT_WORK(&keypad->work, max7359_worker);
input_dev->name = client->name;
input_dev->id.bustype = BUS_I2C;
@@ -245,8 +227,8 @@ static int __devinit max7359_probe(struct i2c_client *client,
max7359_build_keycode(keypad, keymap_data);
- error = request_irq(keypad->irq, max7359_interrupt,
- IRQF_TRIGGER_LOW, client->name, keypad);
+ error = request_threaded_irq(client->irq, NULL, max7359_interrupt,
+ IRQF_TRIGGER_LOW, client->name, keypad);
if (error) {
dev_err(&client->dev, "failed to register interrupt\n");
goto failed_free_mem;
@@ -268,7 +250,7 @@ static int __devinit max7359_probe(struct i2c_client *client,
return 0;
failed_free_irq:
- free_irq(keypad->irq, keypad);
+ free_irq(client->irq, keypad);
failed_free_mem:
input_free_device(input_dev);
kfree(keypad);
@@ -279,9 +261,8 @@ static int __devexit max7359_remove(struct i2c_client *client)
{
struct max7359_keypad *keypad = i2c_get_clientdata(client);
- cancel_work_sync(&keypad->work);
+ free_irq(client->irq, keypad);
input_unregister_device(keypad->input_dev);
- free_irq(keypad->irq, keypad);
i2c_set_clientdata(client, NULL);
kfree(keypad);
@@ -291,22 +272,18 @@ static int __devexit max7359_remove(struct i2c_client *client)
#ifdef CONFIG_PM
static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
{
- struct max7359_keypad *keypad = i2c_get_clientdata(client);
-
max7359_fall_deepsleep(client);
if (device_may_wakeup(&client->dev))
- enable_irq_wake(keypad->irq);
+ enable_irq_wake(client->irq);
return 0;
}
static int max7359_resume(struct i2c_client *client)
{
- struct max7359_keypad *keypad = i2c_get_clientdata(client);
-
if (device_may_wakeup(&client->dev))
- disable_irq_wake(keypad->irq);
+ disable_irq_wake(client->irq);
/* Restore the default setting */
max7359_take_catnap(client);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/