[media] rc: call input_sync after scancode reports

Commit Message

Dmitry Torokhov June 23, 2011, 6:40 p.m. UTC

Hi Jarod,

On Thu, Jun 23, 2011 at 01:58:06PM -0400, Jarod Wilson wrote:
> @@ -623,6 +624,7 @@ static void ir_do_keydown(struct rc_dev *dev, int scancode,
>  			  u32 keycode, u8 toggle)
>  {
>  	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
> +	input_sync(dev->input_dev);
>  
>  	/* Repeat event? */
>  	if (dev->keypressed &&

It looks like we would be issuing up to 3 input_sync() for a single
keypress... Order of events is wrong too (we first issue MSC_SCAN for
new key and then release old key). How about we change it a bit like in
the patch below?

Comments

Jarod Wilson June 23, 2011, 7:47 p.m. UTC | #1

Dmitry Torokhov wrote:
> Hi Jarod,
>
> On Thu, Jun 23, 2011 at 01:58:06PM -0400, Jarod Wilson wrote:
>> @@ -623,6 +624,7 @@ static void ir_do_keydown(struct rc_dev *dev, int scancode,
>>   			  u32 keycode, u8 toggle)
>>   {
>>   	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
>> +	input_sync(dev->input_dev);
>>
>>   	/* Repeat event? */
>>   	if (dev->keypressed&&
>
> It looks like we would be issuing up to 3 input_sync() for a single
> keypress... Order of events is wrong too (we first issue MSC_SCAN for
> new key and then release old key). How about we change it a bit like in
> the patch below?

Yeah, your patch does result in a nicer overall flow of things (esp. the 
ordering of the release, which I hadn't noticed), and eliminates the 
extra unnecessary syncs. I've got one tiny tweak, where I just pass a 
true/false to ir_do_keyup to say whether or not it should do a sync to 
further reduce some code duplication. Building and testing here locally 
to make sure it does behave as expected, will then send it along.

Patch

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index f57cd56..237cf84 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -597,6 +597,7 @@  void rc_repeat(struct rc_dev *dev)
 	spin_lock_irqsave(&dev->keylock, flags);
 
 	input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
+	input_sync(dev->input_dev);
 
 	if (!dev->keypressed)
 		goto out;
@@ -622,29 +623,31 @@  EXPORT_SYMBOL_GPL(rc_repeat);
 static void ir_do_keydown(struct rc_dev *dev, int scancode,
 			  u32 keycode, u8 toggle)
 {
-	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
-
-	/* Repeat event? */
-	if (dev->keypressed &&
-	    dev->last_scancode == scancode &&
-	    dev->last_toggle == toggle)
-		return;
+	bool new_event = !dev->keypressed ||
+			 dev->last_scancode != scancode ||
+			 dev->last_toggle != toggle;
+
+	if (new_event && dev->keypressed) {
+		/* Release old keypress */
+		IR_dprintk(1, "keyup previous key 0x%04x\n", dev->last_keycode);
+		input_report_key(dev->input_dev, dev->last_keycode, 0);
+		dev->keypressed = false;
+	}
 
-	/* Release old keypress */
-	ir_do_keyup(dev);
+	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
 
-	dev->last_scancode = scancode;
-	dev->last_toggle = toggle;
-	dev->last_keycode = keycode;
+	if (new_event && keycode != KEY_RESERVED) {
+		/* Register a keypress */
+		dev->keypressed = true;
+		IR_dprintk(1, "%s: key down event, key 0x%04x, scancode 0x%04x\n",
+			   dev->input_name, keycode, scancode);
+		input_report_key(dev->input_dev, dev->last_keycode, 1);
 
-	if (keycode == KEY_RESERVED)
-		return;
+		dev->last_scancode = scancode;
+		dev->last_toggle = toggle;
+		dev->last_keycode = keycode;
+	}
 
-	/* Register a keypress */
-	dev->keypressed = true;
-	IR_dprintk(1, "%s: key down event, key 0x%04x, scancode 0x%04x\n",
-		   dev->input_name, keycode, scancode);
-	input_report_key(dev->input_dev, dev->last_keycode, 1);
 	input_sync(dev->input_dev);
 }