2009-06-19 17:47:19

by Trilok Soni

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hi,

On Mon, May 11, 2009 at 8:42 AM, Dmitry
Torokhov<[email protected]> wrote:
> On Mon, May 11, 2009 at 11:34:54AM +0900, Kim Kyuwon wrote:
>> Hi Trilok,
>>
>> On Sun, May 10, 2009 at 2:27 AM, Trilok Soni <[email protected]> wrote:
>> > Hi Kim,
>> >
>> > On Sat, May 9, 2009 at 7:39 AM, Kim Kyuwon <[email protected]> wrote:
>> >> The Maxim MAX7359 is a I2C interfaced key switch controller which provides
>> >> microprocessors with management of up to 64 key switches.
>> >> This patch adds support for the MAX7359 key switch controller.
>> >>
>> >> Signed-off-by: Kim Kyuwon <[email protected]>
>> >
>> >
>> > Thanks for implementing review comments. Please add
>> >
>> > Reviewed-by: Trilok Soni <[email protected]>
>>
>> Sorry, I didn't know I should put the "Reviewed-by" in the reviewed
>> patch.(I'm still kernelnewbie :) )
>
> You are not supposed to add it until somebody explicitely asks you to
> do that, same with Acked-by... The reason is that these tags constitute
> certain endorsement of the patch and while one person may review the
> patch he/she may not yet feel that review process is complete. Hope this
> makes some sense...
>
>> If possible, I want Dmitry to add
>> "Reviewed-by" when he sends this driver to mainline, or I will send
>> new patch again.
>>
>
> I have it added to my copy.

I was looking at Palm Pre released linux-2.6.24 patch today at their
site http://palm.cdnetworks.net/opensource/1.0.1/linux-2.6.24-patch.gz
and I found that Palm Pre also uses the same switch controller but
they seems to have written another version of this driver.

---Trilok Soni


2009-07-13 08:52:13

by Trilok Soni

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hi Dmitry,

On Fri, Jun 19, 2009 at 11:08 PM, Trilok Soni<[email protected]> wrote:
> Hi,
>
> On Mon, May 11, 2009 at 8:42 AM, Dmitry
> Torokhov<[email protected]> wrote:
>> On Mon, May 11, 2009 at 11:34:54AM +0900, Kim Kyuwon wrote:
>>> Hi Trilok,
>>>
>>> On Sun, May 10, 2009 at 2:27 AM, Trilok Soni <[email protected]> wrote:
>>> > Hi Kim,
>>> >
>>> > On Sat, May 9, 2009 at 7:39 AM, Kim Kyuwon <[email protected]> wrote:
>>> >> The Maxim MAX7359 is a I2C interfaced key switch controller which provides
>>> >> microprocessors with management of up to 64 key switches.
>>> >> This patch adds support for the MAX7359 key switch controller.
>>> >>
>>> >> Signed-off-by: Kim Kyuwon <[email protected]>
>>> >
>>> >
>>> > Thanks for implementing review comments. Please add
>>> >
>>> > Reviewed-by: Trilok Soni <[email protected]>
>>>
>>> Sorry, I didn't know I should put the "Reviewed-by" in the reviewed
>>> patch.(I'm still kernelnewbie :) )
>>
>> You are not supposed to add it until somebody explicitely asks you to
>> do that, same with Acked-by... The reason is that these tags constitute
>> certain endorsement of the patch and while one person may review the
>> patch he/she may not yet feel that review process is complete. Hope this
>> makes some sense...
>>
>>> If possible, I want Dmitry to add
>>> "Reviewed-by" when he sends this driver to mainline, or I will send
>>> new patch again.
>>>
>>
>> I have it added to my copy.

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
:)

http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-13 09:31:55

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

On Mon, Jul 13, 2009 at 02:22:10PM +0530, Trilok Soni wrote:
> Hi Dmitry,
>
>
> 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!

--
Dmitry


Input: max7359 - use threaded IRQs

From: Dmitry Torokhov <[email protected]>

Convert max7359 driver to use IRQ threading instead of using
workqueue.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/input/keyboard/max7359_keypad.c | 34 +++++++++++++------------------
1 files changed, 14 insertions(+), 20 deletions(-)


diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
index e359db3..c823c0b 100644
--- a/drivers/input/keyboard/max7359_keypad.c
+++ b/drivers/input/keyboard/max7359_keypad.c
@@ -57,8 +57,6 @@ 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;

@@ -103,10 +101,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;

@@ -116,26 +114,23 @@ static void max7359_worker(struct work_struct *work)
release = val & 0x40;
code = (row << 3) + col;

+ 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"));
+ return IRQ_HANDLED;
}

-static irqreturn_t max7359_interrupt(int irq, void *dev_id)
+static irqreturn_t max7359_hardirq(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;
+ disable_irq_nosync(keypad->irq);
+ return IRQ_WAKE_THREAD;
}

/*
@@ -223,7 +218,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;
@@ -241,8 +235,9 @@ 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(keypad->irq,
+ max7359_hardirq, max7359_interrupt,
+ IRQF_TRIGGER_LOW, client->name, keypad);
if (error) {
dev_err(&client->dev, "failed to register interrupt\n");
goto failed_free_mem;
@@ -275,9 +270,8 @@ static int __devexit max7359_remove(struct i2c_client *client)
{
struct max7359_keypad *keypad = i2c_get_clientdata(client);

- cancel_work_sync(&keypad->work);
- input_unregister_device(keypad->input_dev);
free_irq(keypad->irq, keypad);
+ input_unregister_device(keypad->input_dev);
i2c_set_clientdata(client, NULL);
kfree(keypad);

2009-07-14 03:09:38

by Kim Kyuwon

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Dmitry Torokhov wrote:
> On Mon, Jul 13, 2009 at 02:22:10PM +0530, Trilok Soni wrote:
>> Hi Dmitry,
>>
>>
>> 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?

Sincerely,
Kyuwon

2009-07-14 06:29:06

by Marek Szyprowski

[permalink] [raw]
Subject: RE: [PATCH] Input: add MAX7359 key switch controller driver, v2

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?

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center


2009-07-14 08:25:07

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

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...

--
Dmitry


Attachments:
(No filename) (1.21 kB)
melfas-mcs-5000-fixes.patch (7.56 kB)
input-add-touchscreen-driver-for-melfas-mcs-5000-controller.patch (12.63 kB)
input-add-max7359-key-switch-controller-driver-v2.patch (11.06 kB)
max7359-threaded-irq.patch (4.26 kB)
Download all attachments

2009-07-14 08:53:32

by Trilok Soni

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hi Dmitry,

On Tue, Jul 14, 2009 at 1:54 PM, Dmitry
Torokhov<[email protected]> 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...

I never saw/reviewed melfas touchscreen driver on linux-input ML, did
I missed something?

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-14 09:08:21

by Marek Szyprowski

[permalink] [raw]
Subject: RE: [PATCH] Input: add MAX7359 key switch controller driver, v2

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


New version:
NCP:~# hexdump /dev/input/event0
0000000 0110 0000 4f07 0009 0004 0004 0000 0000
0000010 0110 0000 4f30 0009 0001 00e7 0001 0000
0000020 0110 0000 4f3b 0009 0000 0000 0000 0000
0000030 0110 0000 9d43 0009 0004 0004 003f 0000
0000040 0110 0000 9d5d 0009 0000 0000 0000 0000
0000050 0110 0000 fcb6 000a 0004 0004 0000 0000
0000060 0110 0000 fcd2 000a 0001 00e7 0000 0000
0000070 0110 0000 fcd9 000a 0000 0000 0000 0000
0000080 0110 0000 4ae9 000b 0004 0004 003f 0000
0000090 0110 0000 4b02 000b 0000 0000 0000 0000

Melfas-MCS-5000 touch screen driver stopped working after your patch. The 'v3' version worked fine here. Tests has been done on
2.6.30 kernel on NCP board (I had to cherrypick a gpio-matrix keypad driver to compile the updated MAX7359 driver, but this
shouldn't matter at all).

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

2009-07-14 09:11:39

by Trilok Soni

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hi Marek,

On Tue, Jul 14, 2009 at 2:37 PM, Marek
Szyprowski<[email protected]> 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.

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-14 10:19:32

by Marek Szyprowski

[permalink] [raw]
Subject: RE: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hello,

On Tuesday, July 14, 2009 11:12 AM, Trilok Soni wrote:

> On Tue, Jul 14, 2009 at 2:37 PM, Marek
> Szyprowski<[email protected]> 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.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

2009-07-14 10:23:20

by Trilok Soni

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hi Market,

On Tue, Jul 14, 2009 at 3:48 PM, Marek
Szyprowski<[email protected]> wrote:
> Hello,
>
> On Tuesday, July 14, 2009 11:12 AM, Trilok Soni wrote:
>
>> On Tue, Jul 14, 2009 at 2:37 PM, Marek
>> Szyprowski<[email protected]> 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.

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-15 07:16:25

by Marek Szyprowski

[permalink] [raw]
Subject: RE: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hello,

On Tuesday, July 14, 2009 12:23 PM Trilok Soni wrote:

> On Tue, Jul 14, 2009 at 3:48 PM, Marek
> Szyprowski<[email protected]> wrote:
> > Hello,
> >
> > On Tuesday, July 14, 2009 11:12 AM, Trilok Soni wrote:
> >
> >> On Tue, Jul 14, 2009 at 2:37 PM, Marek
> >> Szyprowski<[email protected]> 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).

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

2009-09-16 08:57:57

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

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<[email protected]> wrote:
> > > Hello,
> > >
> > > On Tuesday, July 14, 2009 11:12 AM, Trilok Soni wrote:
> > >
> > >> On Tue, Jul 14, 2009 at 2:37 PM, Marek
> > >> Szyprowski<[email protected]> 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 <[email protected]>

Convert max7359 driver to use IRQ threading instead of using
workqueue.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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);

2009-09-18 08:14:40

by Joonyoung Shim

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

On 9/16/2009 5:57 PM, Dmitry Torokhov wrote:
> 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<[email protected]> wrote:
>>>> Hello,
>>>>
>>>> On Tuesday, July 14, 2009 11:12 AM, Trilok Soni wrote:
>>>>
>>>>> On Tue, Jul 14, 2009 at 2:37 PM, Marek
>>>>> Szyprowski<[email protected]> 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:
>>>> 잾vent type 0 (Sync)
>>>> 잾vent type 1 (Key)
>>>> 잾vent code 107 (End)
>>>> 잾vent code 139 (Menu)
>>>> 잾vent code 148 (Prog1)
>>>> 잾vent code 149 (Prog2)
>>>> 잾vent code 177 (ScrollUp)
>>>> 잾vent code 178 (ScrollDown)
>>>> 잾vent code 212 (Camera)
>>>> 잾vent code 231 (?)
>>>> 잾vent code 474 (?)
>>>> 잾vent 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:
>>>> 잾vent type 0 (Sync)
>>>> 잾vent type 1 (Key)
>>>> 잾vent code 107 (End)
>>>> 잾vent code 139 (Menu)
>>>> 잾vent code 148 (Prog1)
>>>> 잾vent code 149 (Prog2)
>>>> 잾vent code 177 (ScrollUp)
>>>> 잾vent code 178 (ScrollDown)
>>>> 잾vent code 212 (Camera)
>>>> 잾vent code 231 (?)
>>>> 잾vent code 474 (?)
>>>> 잾vent type 4 (Misc)
>>>> 잾vent code 4 (ScanCode)
>>>> 잾vent 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!
>

Hi, Dmitry.

I tested your patch with threaded IRQ. Because the interrupt of the
MAX7359 device is the low level detecting, i should add the IRQF_ONESHOT
flag on request_threaded_irq(). If it is added IRQF_ONESHOT flag, the it
operates well.

Do you want i send the patch?

2009-09-18 16:39:35

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

Hi Joonyoung,

On Fri, Sep 18, 2009 at 05:14:39PM +0900, Joonyoung Shim wrote:
>
> Hi, Dmitry.
>
> I tested your patch with threaded IRQ. Because the interrupt of the
> MAX7359 device is the low level detecting, i should add the IRQF_ONESHOT
> flag on request_threaded_irq(). If it is added IRQF_ONESHOT flag, the it
> operates well.

Thank you for testing.

> Do you want i send the patch?

No, I fixed it locally. I took the liberty of marking the patch as
"Tested-by" you, I hope you don't mind.

--
Dmitry

2009-09-19 00:07:13

by Joonyoung Shim

[permalink] [raw]
Subject: Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

2009/9/19 Dmitry Torokhov <[email protected]>:
> Hi Joonyoung,
>
> On Fri, Sep 18, 2009 at 05:14:39PM +0900, Joonyoung Shim wrote:
>>
>> Hi, Dmitry.
>>
>> I tested your patch with threaded IRQ. Because the interrupt of the
>> MAX7359 device is the low level detecting, i should add the IRQF_ONESHOT
>> flag on request_threaded_irq(). If it is added IRQF_ONESHOT flag, the it
>> operates well.
>
> Thank you for testing.
>
>> Do you want i send the patch?
>
> No, I fixed it locally. I took the liberty of marking the patch as
> "Tested-by" you, I hope you don't mind.
>

OK. Thanks.

--
- Joonyoung Shim