diff mbox

Input: add MAX7359 key switch controller driver, v2

Message ID 20090916085749.GL2756@core.coreip.homeip.net
State New, archived
Headers show

Commit Message

Dmitry Torokhov Sept. 16, 2009, 8:57 a.m. UTC
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@samsung.com> 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@samsung.com> 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!

Comments

Joonyoung Shim Sept. 18, 2009, 8:14 a.m. UTC | #1
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<m.szyprowski@samsung.com> 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@samsung.com> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Sept. 18, 2009, 4:39 p.m. UTC | #2
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.
Joonyoung Shim Sept. 19, 2009, 12:07 a.m. UTC | #3
2009/9/19 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> 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.
diff mbox

Patch

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