[PATCH 0/2] ati_remote2 patches

[PATCH 0/2] ati_remote2 patches

[Ville Syrjala]

I posted these patches earlier and I believe all issues found during
the review were addressed.

Dmitry, please apply.

[PATCH 1/2] ati_remote2: Add loadable keymap support

[Ville Syrjala]

Support for loadable keymaps. The driver now supports individual keymaps for
each of the five modes (AUX1-AUX4 and PC) of the remote. To achieve this the
keymap scancode is interpreted as a combination of the mode and actual button
scancode. The original keycode patches were done by Peter Stokes <linux-2UcvvYt7w9fQXOPxS62xeg@public.gmane.org>
but I modified it quite a lot.

Signed-off-by: Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
---
 drivers/input/misc/ati_remote2.c |  130 +++++++++++++++++++++++++++++---------
 1 files changed, 101 insertions(+), 29 deletions(-)

diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
index f2709b8..b80b2fa 100644
--- a/drivers/input/misc/ati_remote2.c
+++ b/drivers/input/misc/ati_remote2.c
@@ -1,8 +1,8 @@
 /*
  * ati_remote2 - ATI/Philips USB RF remote driver
  *
- * Copyright (C) 2005 Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
- * Copyright (C) 2007 Peter Stokes <linux-c3qaBEMxZpTYB/AZRZ3tHL0Ud+EcFu5g@public.gmane.org>
+ * Copyright (C) 2005-2008 Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
+ * Copyright (C) 2007-2008 Peter Stokes <linux-2UcvvYt7w9fQXOPxS62xeg@public.gmane.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2
@@ -12,7 +12,7 @@
 #include <linux/usb/input.h>
 
 #define DRIVER_DESC    "ATI/Philips USB RF remote driver"
-#define DRIVER_VERSION "0.2"
+#define DRIVER_VERSION "0.3"
 
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_VERSION(DRIVER_VERSION);
@@ -27,7 +27,7 @@ MODULE_LICENSE("GPL");
  * A remote's "channel" may be altered by pressing and holding the "PC" button for
  * approximately 3 seconds, after which the button will slowly flash the count of the
  * currently configured "channel", using the numeric keypad enter a number between 1 and
- * 16 and then the "PC" button again, the button will slowly flash the count of the
+ * 16 and then press the "PC" button again, the button will slowly flash the count of the
  * newly configured "channel".
  */
 
@@ -45,9 +45,18 @@ static struct usb_device_id ati_remote2_id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
-static struct {
-	int hw_code;
-	int key_code;
+enum {
+	ATI_REMOTE2_AUX1,
+	ATI_REMOTE2_AUX2,
+	ATI_REMOTE2_AUX3,
+	ATI_REMOTE2_AUX4,
+	ATI_REMOTE2_PC,
+	ATI_REMOTE2_MODES,
+};
+
+static const struct {
+	u8  hw_code;
+	u16 keycode;
 } ati_remote2_key_table[] = {
 	{ 0x00, KEY_0 },
 	{ 0x01, KEY_1 },
@@ -73,6 +82,7 @@ static struct {
 	{ 0x37, KEY_RECORD },
 	{ 0x38, KEY_DVD },
 	{ 0x39, KEY_TV },
+	{ 0x3f, KEY_PROG1 }, /* AUX1-AUX4 and PC */
 	{ 0x54, KEY_MENU },
 	{ 0x58, KEY_UP },
 	{ 0x59, KEY_DOWN },
@@ -91,15 +101,9 @@ static struct {
 	{ 0xa9, BTN_LEFT },
 	{ 0xaa, BTN_RIGHT },
 	{ 0xbe, KEY_QUESTION },
-	{ 0xd5, KEY_FRONT },
 	{ 0xd0, KEY_EDIT },
+	{ 0xd5, KEY_FRONT },
 	{ 0xf9, KEY_INFO },
-	{ (0x00 << 8) | 0x3f, KEY_PROG1 },
-	{ (0x01 << 8) | 0x3f, KEY_PROG2 },
-	{ (0x02 << 8) | 0x3f, KEY_PROG3 },
-	{ (0x03 << 8) | 0x3f, KEY_PROG4 },
-	{ (0x04 << 8) | 0x3f, KEY_PC },
-	{ 0, KEY_RESERVED }
 };
 
 struct ati_remote2 {
@@ -117,6 +121,9 @@ struct ati_remote2 {
 
 	char name[64];
 	char phys[64];
+
+	/* Each mode (AUX1-AUX4 and PC) can have an independent keymap. */
+	u16 keycode[ATI_REMOTE2_MODES][ARRAY_SIZE(ati_remote2_key_table)];
 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);
@@ -172,7 +179,7 @@ static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
 
 	mode = data[0] & 0x0F;
 
-	if (mode > 4) {
+	if (mode > ATI_REMOTE2_PC) {
 		dev_err(&ar2->intf[0]->dev,
 			"Unknown mode byte (%02x %02x %02x %02x)\n",
 			data[3], data[2], data[1], data[0]);
@@ -191,7 +198,7 @@ static int ati_remote2_lookup(unsigned int hw_code)
 {
 	int i;
 
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
+	for (i = 0; i < ARRAY_SIZE(ati_remote2_key_table); i++)
 		if (ati_remote2_key_table[i].hw_code == hw_code)
 			return i;
 
@@ -211,7 +218,7 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 
 	mode = data[0] & 0x0F;
 
-	if (mode > 4) {
+	if (mode > ATI_REMOTE2_PC) {
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown mode byte (%02x %02x %02x %02x)\n",
 			data[3], data[2], data[1], data[0]);
@@ -219,10 +226,6 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 	}
 
 	hw_code = data[2];
-	/*
-	 * Mode keys (AUX1-AUX4, PC) all generate the same code byte.
-	 * Use the mode byte to figure out which one was pressed.
-	 */
 	if (hw_code == 0x3f) {
 		/*
 		 * For some incomprehensible reason the mouse pad generates
@@ -236,8 +239,6 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 
 		if (data[1] == 0)
 			ar2->mode = mode;
-
-		hw_code |= mode << 8;
 	}
 
 	if (!((1 << mode) & mode_mask))
@@ -260,8 +261,8 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 	case 2:	/* repeat */
 
 		/* No repeat for mouse buttons. */
-		if (ati_remote2_key_table[index].key_code == BTN_LEFT ||
-		    ati_remote2_key_table[index].key_code == BTN_RIGHT)
+		if (ar2->keycode[mode][index] == BTN_LEFT ||
+		    ar2->keycode[mode][index] == BTN_RIGHT)
 			return;
 
 		if (!time_after_eq(jiffies, ar2->jiffies))
@@ -276,7 +277,7 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 		return;
 	}
 
-	input_event(idev, EV_KEY, ati_remote2_key_table[index].key_code, data[1]);
+	input_event(idev, EV_KEY, ar2->keycode[mode][index], data[1]);
 	input_sync(idev);
 }
 
@@ -334,10 +335,60 @@ static void ati_remote2_complete_key(struct urb *urb)
 			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 }
 
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	mode = scancode >> 8;
+	if (mode > ATI_REMOTE2_PC || !((1 << mode) & mode_mask))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index < 0)
+		return -EINVAL;
+
+	*keycode = ar2->keycode[mode][index];
+	return 0;
+}
+
+static int ati_remote2_setkeycode(struct input_dev *idev, int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode, old_keycode;
+
+	mode = scancode >> 8;
+	if (mode > ATI_REMOTE2_PC || !((1 << mode) & mode_mask))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index < 0)
+		return -EINVAL;
+
+	if (keycode < KEY_RESERVED || keycode > KEY_MAX)
+		return -EINVAL;
+
+	old_keycode = ar2->keycode[mode][index];
+	ar2->keycode[mode][index] = keycode;
+	set_bit(keycode, idev->keybit);
+
+	for (mode = 0; mode < ATI_REMOTE2_MODES; mode++) {
+		for (index = 0; index < ARRAY_SIZE(ati_remote2_key_table); index++) {
+			if (ar2->keycode[mode][index] == old_keycode)
+				return 0;
+		}
+	}
+
+	clear_bit(old_keycode, idev->keybit);
+
+	return 0;
+}
+
 static int ati_remote2_input_init(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev;
-	int i, retval;
+	int index, mode, retval;
 
 	idev = input_allocate_device();
 	if (!idev)
@@ -350,8 +401,26 @@ static int ati_remote2_input_init(struct ati_remote2 *ar2)
 	idev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
 		BIT_MASK(BTN_RIGHT);
 	idev->relbit[0] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
-		set_bit(ati_remote2_key_table[i].key_code, idev->keybit);
+
+	for (mode = 0; mode < ATI_REMOTE2_MODES; mode++) {
+		for (index = 0; index < ARRAY_SIZE(ati_remote2_key_table); index++) {
+			ar2->keycode[mode][index] = ati_remote2_key_table[index].keycode;
+			set_bit(ar2->keycode[mode][index], idev->keybit);
+		}
+	}
+
+	/* AUX1-AUX4 and PC generate the same scancode. */
+	index = ati_remote2_lookup(0x3f);
+	ar2->keycode[ATI_REMOTE2_AUX1][index] = KEY_PROG1;
+	ar2->keycode[ATI_REMOTE2_AUX2][index] = KEY_PROG2;
+	ar2->keycode[ATI_REMOTE2_AUX3][index] = KEY_PROG3;
+	ar2->keycode[ATI_REMOTE2_AUX4][index] = KEY_PROG4;
+	ar2->keycode[ATI_REMOTE2_PC][index] = KEY_PC;
+	set_bit(KEY_PROG1, idev->keybit);
+	set_bit(KEY_PROG2, idev->keybit);
+	set_bit(KEY_PROG3, idev->keybit);
+	set_bit(KEY_PROG4, idev->keybit);
+	set_bit(KEY_PC, idev->keybit);
 
 	idev->rep[REP_DELAY]  = 250;
 	idev->rep[REP_PERIOD] = 33;
@@ -359,6 +428,9 @@ static int ati_remote2_input_init(struct ati_remote2 *ar2)
 	idev->open = ati_remote2_open;
 	idev->close = ati_remote2_close;
 
+	idev->getkeycode = ati_remote2_getkeycode;
+	idev->setkeycode = ati_remote2_setkeycode;
+
 	idev->name = ar2->name;
 	idev->phys = ar2->phys;
 
-- 
1.5.4.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ati_remote2: Add autosuspend support

[Ville Syrjala]

Add autosuspend support to ati_remote2.

Signed-off-by: Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
---
 drivers/input/misc/ati_remote2.c |  133 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
index b80b2fa..27d9c02 100644
--- a/drivers/input/misc/ati_remote2.c
+++ b/drivers/input/misc/ati_remote2.c
@@ -45,6 +45,13 @@ static struct usb_device_id ati_remote2_id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
+static DEFINE_MUTEX(ati_remote2_mutex);
+
+enum {
+	ATI_REMOTE2_OPENED = 0x1,
+	ATI_REMOTE2_SUSPENDED = 0x2,
+};
+
 enum {
 	ATI_REMOTE2_AUX1,
 	ATI_REMOTE2_AUX2,
@@ -124,46 +131,103 @@ struct ati_remote2 {
 
 	/* Each mode (AUX1-AUX4 and PC) can have an independent keymap. */
 	u16 keycode[ATI_REMOTE2_MODES][ARRAY_SIZE(ati_remote2_key_table)];
+
+	unsigned int flags;
 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);
 static void ati_remote2_disconnect(struct usb_interface *interface);
+static int ati_remote2_suspend(struct usb_interface *interface, pm_message_t message);
+static int ati_remote2_resume(struct usb_interface *interface);
 
 static struct usb_driver ati_remote2_driver = {
 	.name       = "ati_remote2",
 	.probe      = ati_remote2_probe,
 	.disconnect = ati_remote2_disconnect,
 	.id_table   = ati_remote2_id_table,
+	.suspend    = ati_remote2_suspend,
+	.resume     = ati_remote2_resume,
+	.supports_autosuspend = 1,
 };
 
-static int ati_remote2_open(struct input_dev *idev)
+static int ati_remote2_submit_urbs(struct ati_remote2 *ar2)
 {
-	struct ati_remote2 *ar2 = input_get_drvdata(idev);
 	int r;
 
 	r = usb_submit_urb(ar2->urb[0], GFP_KERNEL);
 	if (r) {
 		dev_err(&ar2->intf[0]->dev,
-			"%s: usb_submit_urb() = %d\n", __FUNCTION__, r);
+			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 		return r;
 	}
 	r = usb_submit_urb(ar2->urb[1], GFP_KERNEL);
 	if (r) {
 		usb_kill_urb(ar2->urb[0]);
 		dev_err(&ar2->intf[1]->dev,
-			"%s: usb_submit_urb() = %d\n", __FUNCTION__, r);
+			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 		return r;
 	}
 
 	return 0;
 }
 
+static void ati_remote2_kill_urbs(struct ati_remote2 *ar2)
+{
+	usb_kill_urb(ar2->urb[1]);
+	usb_kill_urb(ar2->urb[0]);
+}
+
+static int ati_remote2_open(struct input_dev *idev)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int r;
+
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	r = usb_autopm_get_interface(ar2->intf[0]);
+	if (r) {
+		dev_err(&ar2->intf[0]->dev,
+			"%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
+		goto fail1;
+	}
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED)) {
+		r = ati_remote2_submit_urbs(ar2);
+		if (r)
+			goto fail2;
+	}
+
+	ar2->flags |= ATI_REMOTE2_OPENED;
+
+	mutex_unlock(&ati_remote2_mutex);
+
+	usb_autopm_put_interface(ar2->intf[0]);
+
+	return 0;
+
+ fail2:
+	mutex_unlock(&ati_remote2_mutex);
+	usb_autopm_put_interface(ar2->intf[0]);
+ fail1:
+	return r;
+}
+
 static void ati_remote2_close(struct input_dev *idev)
 {
 	struct ati_remote2 *ar2 = input_get_drvdata(idev);
 
-	usb_kill_urb(ar2->urb[0]);
-	usb_kill_urb(ar2->urb[1]);
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED))
+		ati_remote2_kill_urbs(ar2);
+
+	ar2->flags &= ~ATI_REMOTE2_OPENED;
+
+	mutex_unlock(&ati_remote2_mutex);
 }
 
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
@@ -288,6 +352,7 @@ static void ati_remote2_complete_mouse(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(ar2->udev);
 		ati_remote2_input_mouse(ar2);
 		break;
 	case -ENOENT:
@@ -298,6 +363,7 @@ static void ati_remote2_complete_mouse(struct urb *urb)
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 		return;
 	default:
+		usb_mark_last_busy(ar2->udev);
 		dev_err(&ar2->intf[0]->dev,
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 	}
@@ -315,6 +381,7 @@ static void ati_remote2_complete_key(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(ar2->udev);
 		ati_remote2_input_key(ar2);
 		break;
 	case -ENOENT:
@@ -325,6 +392,7 @@ static void ati_remote2_complete_key(struct urb *urb)
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 		return;
 	default:
+		usb_mark_last_busy(ar2->udev);
 		dev_err(&ar2->intf[1]->dev,
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 	}
@@ -562,6 +630,8 @@ static int ati_remote2_probe(struct usb_interface *interface, const struct usb_d
 
 	usb_set_intfdata(interface, ar2);
 
+	interface->needs_remote_wakeup = 1;
+
 	return 0;
 
  fail2:
@@ -594,6 +664,57 @@ static void ati_remote2_disconnect(struct usb_interface *interface)
 	kfree(ar2);
 }
 
+static int ati_remote2_suspend(struct usb_interface *interface,
+			       pm_message_t message)
+{
+	struct ati_remote2 *ar2;
+	struct usb_host_interface *alt = interface->cur_altsetting;
+
+	if (alt->desc.bInterfaceNumber)
+		return 0;
+
+	ar2 = usb_get_intfdata(interface);
+
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (ar2->flags & ATI_REMOTE2_OPENED)
+		ati_remote2_kill_urbs(ar2);
+
+	ar2->flags |= ATI_REMOTE2_SUSPENDED;
+
+	mutex_unlock(&ati_remote2_mutex);
+
+	return 0;
+}
+
+static int ati_remote2_resume(struct usb_interface *interface)
+{
+	struct ati_remote2 *ar2;
+	struct usb_host_interface *alt = interface->cur_altsetting;
+	int r = 0;
+
+	if (alt->desc.bInterfaceNumber)
+		return 0;
+
+	ar2 = usb_get_intfdata(interface);
+
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (ar2->flags & ATI_REMOTE2_OPENED)
+		r = ati_remote2_submit_urbs(ar2);
+
+	if (!r)
+		ar2->flags &= ~ATI_REMOTE2_SUSPENDED;
+
+	mutex_unlock(&ati_remote2_mutex);
+
+	return r;
+}
+
 static int __init ati_remote2_init(void)
 {
 	int r;
-- 
1.5.4.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ati_remote2 patches

[Dmitry Torokhov]

On Wed, Jun 25, 2008 at 01:56:09PM +0300, Ville Syrjala wrote:
> I posted these patches earlier and I believe all issues found during
> the review were addressed.
> 
> Dmitry, please apply.

Applied to my tree, will be in next shortly.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ati_remote2: Add loadable keymap support

[Ville Syrjala]

Support for loadable keymaps. The driver now supports individual keymaps
for each of the five modes (AUX1-AUX4 and PC) of the remote. To achieve
this the keymap scancode is interpreted as a combination of the mode and
actual button scancode. The original keycode patches were done by Peter
Stokes <linux-2UcvvYt7w9fQXOPxS62xeg@public.gmane.org> but I modified it quite a lot.

Signed-off-by: Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
---
 drivers/input/misc/ati_remote2.c |  130 +++++++++++++++++++++++++++++---------
 1 files changed, 101 insertions(+), 29 deletions(-)

diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
index f2709b8..b80b2fa 100644
--- a/drivers/input/misc/ati_remote2.c
+++ b/drivers/input/misc/ati_remote2.c
@@ -1,8 +1,8 @@
 /*
  * ati_remote2 - ATI/Philips USB RF remote driver
  *
- * Copyright (C) 2005 Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
- * Copyright (C) 2007 Peter Stokes <linux-c3qaBEMxZpTYB/AZRZ3tHL0Ud+EcFu5g@public.gmane.org>
+ * Copyright (C) 2005-2008 Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org>
+ * Copyright (C) 2007-2008 Peter Stokes <linux-2UcvvYt7w9fQXOPxS62xeg@public.gmane.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2
@@ -12,7 +12,7 @@
 #include <linux/usb/input.h>
 
 #define DRIVER_DESC    "ATI/Philips USB RF remote driver"
-#define DRIVER_VERSION "0.2"
+#define DRIVER_VERSION "0.3"
 
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_VERSION(DRIVER_VERSION);
@@ -27,7 +27,7 @@ MODULE_LICENSE("GPL");
  * A remote's "channel" may be altered by pressing and holding the "PC" button for
  * approximately 3 seconds, after which the button will slowly flash the count of the
  * currently configured "channel", using the numeric keypad enter a number between 1 and
- * 16 and then the "PC" button again, the button will slowly flash the count of the
+ * 16 and then press the "PC" button again, the button will slowly flash the count of the
  * newly configured "channel".
  */
 
@@ -45,9 +45,18 @@ static struct usb_device_id ati_remote2_id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
-static struct {
-	int hw_code;
-	int key_code;
+enum {
+	ATI_REMOTE2_AUX1,
+	ATI_REMOTE2_AUX2,
+	ATI_REMOTE2_AUX3,
+	ATI_REMOTE2_AUX4,
+	ATI_REMOTE2_PC,
+	ATI_REMOTE2_MODES,
+};
+
+static const struct {
+	u8  hw_code;
+	u16 keycode;
 } ati_remote2_key_table[] = {
 	{ 0x00, KEY_0 },
 	{ 0x01, KEY_1 },
@@ -73,6 +82,7 @@ static struct {
 	{ 0x37, KEY_RECORD },
 	{ 0x38, KEY_DVD },
 	{ 0x39, KEY_TV },
+	{ 0x3f, KEY_PROG1 }, /* AUX1-AUX4 and PC */
 	{ 0x54, KEY_MENU },
 	{ 0x58, KEY_UP },
 	{ 0x59, KEY_DOWN },
@@ -91,15 +101,9 @@ static struct {
 	{ 0xa9, BTN_LEFT },
 	{ 0xaa, BTN_RIGHT },
 	{ 0xbe, KEY_QUESTION },
-	{ 0xd5, KEY_FRONT },
 	{ 0xd0, KEY_EDIT },
+	{ 0xd5, KEY_FRONT },
 	{ 0xf9, KEY_INFO },
-	{ (0x00 << 8) | 0x3f, KEY_PROG1 },
-	{ (0x01 << 8) | 0x3f, KEY_PROG2 },
-	{ (0x02 << 8) | 0x3f, KEY_PROG3 },
-	{ (0x03 << 8) | 0x3f, KEY_PROG4 },
-	{ (0x04 << 8) | 0x3f, KEY_PC },
-	{ 0, KEY_RESERVED }
 };
 
 struct ati_remote2 {
@@ -117,6 +121,9 @@ struct ati_remote2 {
 
 	char name[64];
 	char phys[64];
+
+	/* Each mode (AUX1-AUX4 and PC) can have an independent keymap. */
+	u16 keycode[ATI_REMOTE2_MODES][ARRAY_SIZE(ati_remote2_key_table)];
 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);
@@ -172,7 +179,7 @@ static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
 
 	mode = data[0] & 0x0F;
 
-	if (mode > 4) {
+	if (mode > ATI_REMOTE2_PC) {
 		dev_err(&ar2->intf[0]->dev,
 			"Unknown mode byte (%02x %02x %02x %02x)\n",
 			data[3], data[2], data[1], data[0]);
@@ -191,7 +198,7 @@ static int ati_remote2_lookup(unsigned int hw_code)
 {
 	int i;
 
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
+	for (i = 0; i < ARRAY_SIZE(ati_remote2_key_table); i++)
 		if (ati_remote2_key_table[i].hw_code == hw_code)
 			return i;
 
@@ -211,7 +218,7 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 
 	mode = data[0] & 0x0F;
 
-	if (mode > 4) {
+	if (mode > ATI_REMOTE2_PC) {
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown mode byte (%02x %02x %02x %02x)\n",
 			data[3], data[2], data[1], data[0]);
@@ -219,10 +226,6 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 	}
 
 	hw_code = data[2];
-	/*
-	 * Mode keys (AUX1-AUX4, PC) all generate the same code byte.
-	 * Use the mode byte to figure out which one was pressed.
-	 */
 	if (hw_code == 0x3f) {
 		/*
 		 * For some incomprehensible reason the mouse pad generates
@@ -236,8 +239,6 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 
 		if (data[1] == 0)
 			ar2->mode = mode;
-
-		hw_code |= mode << 8;
 	}
 
 	if (!((1 << mode) & mode_mask))
@@ -260,8 +261,8 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 	case 2:	/* repeat */
 
 		/* No repeat for mouse buttons. */
-		if (ati_remote2_key_table[index].key_code == BTN_LEFT ||
-		    ati_remote2_key_table[index].key_code == BTN_RIGHT)
+		if (ar2->keycode[mode][index] == BTN_LEFT ||
+		    ar2->keycode[mode][index] == BTN_RIGHT)
 			return;
 
 		if (!time_after_eq(jiffies, ar2->jiffies))
@@ -276,7 +277,7 @@ static void ati_remote2_input_key(struct ati_remote2 *ar2)
 		return;
 	}
 
-	input_event(idev, EV_KEY, ati_remote2_key_table[index].key_code, data[1]);
+	input_event(idev, EV_KEY, ar2->keycode[mode][index], data[1]);
 	input_sync(idev);
 }
 
@@ -334,10 +335,60 @@ static void ati_remote2_complete_key(struct urb *urb)
 			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 }
 
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	mode = scancode >> 8;
+	if (mode > ATI_REMOTE2_PC || !((1 << mode) & mode_mask))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index < 0)
+		return -EINVAL;
+
+	*keycode = ar2->keycode[mode][index];
+	return 0;
+}
+
+static int ati_remote2_setkeycode(struct input_dev *idev, int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode, old_keycode;
+
+	mode = scancode >> 8;
+	if (mode > ATI_REMOTE2_PC || !((1 << mode) & mode_mask))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index < 0)
+		return -EINVAL;
+
+	if (keycode < KEY_RESERVED || keycode > KEY_MAX)
+		return -EINVAL;
+
+	old_keycode = ar2->keycode[mode][index];
+	ar2->keycode[mode][index] = keycode;
+	set_bit(keycode, idev->keybit);
+
+	for (mode = 0; mode < ATI_REMOTE2_MODES; mode++) {
+		for (index = 0; index < ARRAY_SIZE(ati_remote2_key_table); index++) {
+			if (ar2->keycode[mode][index] == old_keycode)
+				return 0;
+		}
+	}
+
+	clear_bit(old_keycode, idev->keybit);
+
+	return 0;
+}
+
 static int ati_remote2_input_init(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev;
-	int i, retval;
+	int index, mode, retval;
 
 	idev = input_allocate_device();
 	if (!idev)
@@ -350,8 +401,26 @@ static int ati_remote2_input_init(struct ati_remote2 *ar2)
 	idev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
 		BIT_MASK(BTN_RIGHT);
 	idev->relbit[0] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
-		set_bit(ati_remote2_key_table[i].key_code, idev->keybit);
+
+	for (mode = 0; mode < ATI_REMOTE2_MODES; mode++) {
+		for (index = 0; index < ARRAY_SIZE(ati_remote2_key_table); index++) {
+			ar2->keycode[mode][index] = ati_remote2_key_table[index].keycode;
+			set_bit(ar2->keycode[mode][index], idev->keybit);
+		}
+	}
+
+	/* AUX1-AUX4 and PC generate the same scancode. */
+	index = ati_remote2_lookup(0x3f);
+	ar2->keycode[ATI_REMOTE2_AUX1][index] = KEY_PROG1;
+	ar2->keycode[ATI_REMOTE2_AUX2][index] = KEY_PROG2;
+	ar2->keycode[ATI_REMOTE2_AUX3][index] = KEY_PROG3;
+	ar2->keycode[ATI_REMOTE2_AUX4][index] = KEY_PROG4;
+	ar2->keycode[ATI_REMOTE2_PC][index] = KEY_PC;
+	set_bit(KEY_PROG1, idev->keybit);
+	set_bit(KEY_PROG2, idev->keybit);
+	set_bit(KEY_PROG3, idev->keybit);
+	set_bit(KEY_PROG4, idev->keybit);
+	set_bit(KEY_PC, idev->keybit);
 
 	idev->rep[REP_DELAY]  = 250;
 	idev->rep[REP_PERIOD] = 33;
@@ -359,6 +428,9 @@ static int ati_remote2_input_init(struct ati_remote2 *ar2)
 	idev->open = ati_remote2_open;
 	idev->close = ati_remote2_close;
 
+	idev->getkeycode = ati_remote2_getkeycode;
+	idev->setkeycode = ati_remote2_setkeycode;
+
 	idev->name = ar2->name;
 	idev->phys = ar2->phys;
 
-- 
1.5.3.7

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ati_remote2: Add autosuspend support

[Ville Syrjala]

Add USB autosuspend support to ati_remote2.

Signed-off-by: Ville Syrjala <syrjala@sci.fi>
---
The only issue I have so far with this patch is that the first key press
is lost when the device wakes up.

Do I need to worry about the remote wakeup thingy? At least remote wakeup
doesn't seem to be enabled (according to lsusb, short except below) and the
device wakes up just fine. There is a LED on the receiver which dims but
doesn't completely turn off when the device is suspended.

Bus 003 Device 003: ID 0471:0602 Philips 
Device Descriptor:
  Configuration Descriptor:
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
Device Status:     0x0000
  (Bus Powered)

 drivers/input/misc/ati_remote2.c |  140 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 134 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
index b80b2fa..47071bf 100644
--- a/drivers/input/misc/ati_remote2.c
+++ b/drivers/input/misc/ati_remote2.c
@@ -45,6 +45,13 @@ static struct usb_device_id ati_remote2_id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
+static DEFINE_MUTEX(ati_remote2_mutex);
+
+enum {
+	ATI_REMOTE2_OPENED = 0x1,
+	ATI_REMOTE2_SUSPENDED = 0x2,
+};
+
 enum {
 	ATI_REMOTE2_AUX1,
 	ATI_REMOTE2_AUX2,
@@ -124,46 +131,112 @@ struct ati_remote2 {
 
 	/* Each mode (AUX1-AUX4 and PC) can have an independent keymap. */
 	u16 keycode[ATI_REMOTE2_MODES][ARRAY_SIZE(ati_remote2_key_table)];
+
+	unsigned int flags;
 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);
 static void ati_remote2_disconnect(struct usb_interface *interface);
+static int ati_remote2_suspend(struct usb_interface *interface, pm_message_t message);
+static int ati_remote2_resume(struct usb_interface *interface);
 
 static struct usb_driver ati_remote2_driver = {
 	.name       = "ati_remote2",
 	.probe      = ati_remote2_probe,
 	.disconnect = ati_remote2_disconnect,
 	.id_table   = ati_remote2_id_table,
+	.suspend    = ati_remote2_suspend,
+	.resume     = ati_remote2_resume,
+	.supports_autosuspend = 1,
 };
 
-static int ati_remote2_open(struct input_dev *idev)
+static int ati_remote2_submit_urbs(struct ati_remote2 *ar2)
 {
-	struct ati_remote2 *ar2 = input_get_drvdata(idev);
 	int r;
 
 	r = usb_submit_urb(ar2->urb[0], GFP_KERNEL);
 	if (r) {
 		dev_err(&ar2->intf[0]->dev,
-			"%s: usb_submit_urb() = %d\n", __FUNCTION__, r);
+			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 		return r;
 	}
 	r = usb_submit_urb(ar2->urb[1], GFP_KERNEL);
 	if (r) {
 		usb_kill_urb(ar2->urb[0]);
 		dev_err(&ar2->intf[1]->dev,
-			"%s: usb_submit_urb() = %d\n", __FUNCTION__, r);
+			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 		return r;
 	}
 
 	return 0;
 }
 
+static void ati_remote2_kill_urbs(struct ati_remote2 *ar2)
+{
+	usb_kill_urb(ar2->urb[1]);
+	usb_kill_urb(ar2->urb[0]);
+}
+
+static int ati_remote2_open(struct input_dev *idev)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int r;
+
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	r = usb_autopm_get_interface(ar2->intf[0]);
+	if (r) {
+		dev_err(&ar2->intf[0]->dev,
+			"%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
+		goto fail1;
+	}
+	r = usb_autopm_get_interface(ar2->intf[1]);
+	if (r) {
+		dev_err(&ar2->intf[1]->dev,
+			"%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
+		goto fail2;
+	}
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED)) {
+		r = ati_remote2_submit_urbs(ar2);
+		if (r)
+			goto fail3;
+	}
+
+	ar2->flags |= ATI_REMOTE2_OPENED;
+
+	mutex_unlock(&ati_remote2_mutex);
+
+	usb_autopm_put_interface(ar2->intf[1]);
+	usb_autopm_put_interface(ar2->intf[0]);
+
+	return 0;
+
+ fail3:
+	mutex_unlock(&ati_remote2_mutex);
+	usb_autopm_put_interface(ar2->intf[1]);
+ fail2:
+	usb_autopm_put_interface(ar2->intf[0]);
+ fail1:
+	return r;
+}
+
 static void ati_remote2_close(struct input_dev *idev)
 {
 	struct ati_remote2 *ar2 = input_get_drvdata(idev);
 
-	usb_kill_urb(ar2->urb[0]);
-	usb_kill_urb(ar2->urb[1]);
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED))
+		ati_remote2_kill_urbs(ar2);
+
+	ar2->flags &= ~ATI_REMOTE2_OPENED;
+
+	mutex_unlock(&ati_remote2_mutex);
 }
 
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
@@ -288,6 +361,7 @@ static void ati_remote2_complete_mouse(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(ar2->udev);
 		ati_remote2_input_mouse(ar2);
 		break;
 	case -ENOENT:
@@ -298,6 +372,7 @@ static void ati_remote2_complete_mouse(struct urb *urb)
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 		return;
 	default:
+		usb_mark_last_busy(ar2->udev);
 		dev_err(&ar2->intf[0]->dev,
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 	}
@@ -315,6 +390,7 @@ static void ati_remote2_complete_key(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(ar2->udev);
 		ati_remote2_input_key(ar2);
 		break;
 	case -ENOENT:
@@ -325,6 +401,7 @@ static void ati_remote2_complete_key(struct urb *urb)
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 		return;
 	default:
+		usb_mark_last_busy(ar2->udev);
 		dev_err(&ar2->intf[1]->dev,
 			"%s(): urb status = %d\n", __FUNCTION__, urb->status);
 	}
@@ -594,6 +671,57 @@ static void ati_remote2_disconnect(struct usb_interface *interface)
 	kfree(ar2);
 }
 
+static int ati_remote2_suspend(struct usb_interface *interface,
+			       pm_message_t message)
+{
+	struct ati_remote2 *ar2;
+	struct usb_host_interface *alt = interface->cur_altsetting;
+
+	if (alt->desc.bInterfaceNumber)
+		return 0;
+
+	ar2 = usb_get_intfdata(interface);
+
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (ar2->flags & ATI_REMOTE2_OPENED)
+		ati_remote2_kill_urbs(ar2);
+
+	ar2->flags |= ATI_REMOTE2_SUSPENDED;
+
+	mutex_unlock(&ati_remote2_mutex);
+
+	return 0;
+}
+
+static int ati_remote2_resume(struct usb_interface *interface)
+{
+	struct ati_remote2 *ar2;
+	struct usb_host_interface *alt = interface->cur_altsetting;
+	int r = 0;
+
+	if (alt->desc.bInterfaceNumber)
+		return 0;
+
+	ar2 = usb_get_intfdata(interface);
+
+	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
+
+	mutex_lock(&ati_remote2_mutex);
+
+	if (ar2->flags & ATI_REMOTE2_OPENED)
+		r = ati_remote2_submit_urbs(ar2);
+
+	if (!r)
+		ar2->flags &= ~ATI_REMOTE2_SUSPENDED;
+
+	mutex_unlock(&ati_remote2_mutex);
+
+	return r;
+}
+
 static int __init ati_remote2_init(void)
 {
 	int r;
-- 
1.5.3.7

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Oliver Neukum]

Am Dienstag 03 Juni 2008 20:45:47 schrieb Ville Syrjala:
> +static int ati_remote2_open(struct input_dev *idev)
> +{
> +       struct ati_remote2 *ar2 = input_get_drvdata(idev);
> +       int r;
> +
> +       dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
> +
> +       r = usb_autopm_get_interface(ar2->intf[0]);
> +       if (r) {
> +               dev_err(&ar2->intf[0]->dev,
> +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> +               goto fail1;
> +       }
> +       r = usb_autopm_get_interface(ar2->intf[1]);
> +       if (r) {
> +               dev_err(&ar2->intf[1]->dev,
> +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> +               goto fail2;
> +       }

If you have two interfaces on one device upping the count for one of them
is enough.

> +
> +       mutex_lock(&ati_remote2_mutex);

Too late. You can race with disconnect()

And you should set needs_remote_wakeup.

	Regards
		Oliver

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Ville Syrjälä]

On Tue, Jun 03, 2008 at 10:11:09PM +0200, Oliver Neukum wrote:
> Am Dienstag 03 Juni 2008 20:45:47 schrieb Ville Syrjala:
> > +static int ati_remote2_open(struct input_dev *idev)
> > +{
> > +       struct ati_remote2 *ar2 = input_get_drvdata(idev);
> > +       int r;
> > +
> > +       dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
> > +
> > +       r = usb_autopm_get_interface(ar2->intf[0]);
> > +       if (r) {
> > +               dev_err(&ar2->intf[0]->dev,
> > +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> > +               goto fail1;
> > +       }
> > +       r = usb_autopm_get_interface(ar2->intf[1]);
> > +       if (r) {
> > +               dev_err(&ar2->intf[1]->dev,
> > +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> > +               goto fail2;
> > +       }
> 
> If you have two interfaces on one device upping the count for one of them
> is enough.

OK. But now I wonder why do I even bother calling this function. It
doesn't seem to do anything particularly useful.

> > +
> > +       mutex_lock(&ati_remote2_mutex);
> 
> Too late. You can race with disconnect()

Hmm. Do you mean open() vs. disconnect()? Doesn't the input_dev's locking
take care of that? ati_remote2_mutex is there just to make ar2->flags
handling and urb submitting/killing atomic, it didn't even exist before
this autosuspend patch. Or perhaps I'm missing something...

> And you should set needs_remote_wakeup.

What exactly does that do? As I said remote wakeup isn't enabled on the
device but it still wakes up just fine.

-- 
Ville Syrjälä
syrjala-ORSVBvAovxo@public.gmane.org
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Oliver Neukum]

Am Mittwoch 04 Juni 2008 10:20:21 schrieb Ville Syrjälä:
> On Tue, Jun 03, 2008 at 10:11:09PM +0200, Oliver Neukum wrote:
> > Am Dienstag 03 Juni 2008 20:45:47 schrieb Ville Syrjala:
> > > +static int ati_remote2_open(struct input_dev *idev)
> > > +{
> > > +       struct ati_remote2 *ar2 = input_get_drvdata(idev);
> > > +       int r;
> > > +
> > > +       dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
> > > +
> > > +       r = usb_autopm_get_interface(ar2->intf[0]);
> > > +       if (r) {
> > > +               dev_err(&ar2->intf[0]->dev,
> > > +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> > > +               goto fail1;
> > > +       }
> > > +       r = usb_autopm_get_interface(ar2->intf[1]);
> > > +       if (r) {
> > > +               dev_err(&ar2->intf[1]->dev,
> > > +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> > > +               goto fail2;
> > > +       }
> > 
> > If you have two interfaces on one device upping the count for one of them
> > is enough.
> 
> OK. But now I wonder why do I even bother calling this function. It
> doesn't seem to do anything particularly useful.

You call it for two reasons

1. To safely enable remote wakeup
2. To make sure the device starts out awake

> > > +
> > > +       mutex_lock(&ati_remote2_mutex);
> > 
> > Too late. You can race with disconnect()
> 
> Hmm. Do you mean open() vs. disconnect()? Doesn't the input_dev's locking
> take care of that? ati_remote2_mutex is there just to make ar2->flags
> handling and urb submitting/killing atomic, it didn't even exist before
> this autosuspend patch. Or perhaps I'm missing something...

Hm. Anybody on the list an expert on locking in the input layer?

> > And you should set needs_remote_wakeup.
> 
> What exactly does that do? As I said remote wakeup isn't enabled on the
> device but it still wakes up just fine.

Strictly speaking, it shouldn't. We should do it correctly to
a) make sure it works with all versions
b) make sure it can handle the command correctly

	Regards
		Oliver


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

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Dmitry Torokhov]

On Wed, Jun 04, 2008 at 10:32:57AM +0200, Oliver Neukum wrote:
> Am Mittwoch 04 Juni 2008 10:20:21 schrieb Ville Syrj?l?:
> > On Tue, Jun 03, 2008 at 10:11:09PM +0200, Oliver Neukum wrote:
> > > Am Dienstag 03 Juni 2008 20:45:47 schrieb Ville Syrjala:
> > > > +
> > > > +???????mutex_lock(&ati_remote2_mutex);
> > > 
> > > Too late. You can race with disconnect()
> > 
> > Hmm. Do you mean open() vs. disconnect()? Doesn't the input_dev's locking
> > take care of that? ati_remote2_mutex is there just to make ar2->flags
> > handling and urb submitting/killing atomic, it didn't even exist before
> > this autosuspend patch. Or perhaps I'm missing something...
> 
> Hm. Anybody on the list an expert on locking in the input layer?
> 

Input core only protects open() and close(); connect() and
disconnect() belong to respective bus's implementation the device is
sitting on and input core has no authority over it.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Alan Stern]

On Tue, 17 Jun 2008, Dmitry Torokhov wrote:

> Input core only protects open() and close(); connect() and
> disconnect() belong to respective bus's implementation the device is
> sitting on and input core has no authority over it.

What about open vs. unregister?  The input core must have some 
protection for those two.

Alan Stern

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Dmitry Torokhov]

On Tue, Jun 17, 2008 at 10:39:15AM -0400, Alan Stern wrote:
> On Tue, 17 Jun 2008, Dmitry Torokhov wrote:
> 
> > Input core only protects open() and close(); connect() and
> > disconnect() belong to respective bus's implementation the device is
> > sitting on and input core has no authority over it.
> 
> What about open vs. unregister?  The input core must have some 
> protection for those two.
> 

input_unregister_device() sets dev->going_away at the very beginning
and input_open_device() will fail with -ENODEV when trying to open such
devices. dev->going_away (among other things) is protected by
dev->mutex.

Do you see any issues with this scheme?

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Alan Stern]

On Tue, 17 Jun 2008, Dmitry Torokhov wrote:

> On Tue, Jun 17, 2008 at 10:39:15AM -0400, Alan Stern wrote:
> > On Tue, 17 Jun 2008, Dmitry Torokhov wrote:
> > 
> > > Input core only protects open() and close(); connect() and
> > > disconnect() belong to respective bus's implementation the device is
> > > sitting on and input core has no authority over it.
> > 
> > What about open vs. unregister?  The input core must have some 
> > protection for those two.
> > 
> 
> input_unregister_device() sets dev->going_away at the very beginning
> and input_open_device() will fail with -ENODEV when trying to open such
> devices. dev->going_away (among other things) is protected by
> dev->mutex.
> 
> Do you see any issues with this scheme?

It depends on how much code is protected by dev->mutex.

The real issue involving open vs. unregister comes down to this.  It 
should not be possible for either:

    (1) an input_unregister_device() call to return while an
	input_open_device() call is in progress; or

    (2) an input_open_device() call to be made after
	input_unregister_device() has returned.

Your description above suggests that (2) can never happen, but I can't
tell for sure.  And what about (1)?

Alan Stern

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Dmitry Torokhov]

On Tue, Jun 17, 2008 at 01:24:25PM -0400, Alan Stern wrote:
> On Tue, 17 Jun 2008, Dmitry Torokhov wrote:
> 
> > On Tue, Jun 17, 2008 at 10:39:15AM -0400, Alan Stern wrote:
> > > On Tue, 17 Jun 2008, Dmitry Torokhov wrote:
> > > 
> > > > Input core only protects open() and close(); connect() and
> > > > disconnect() belong to respective bus's implementation the device is
> > > > sitting on and input core has no authority over it.
> > > 
> > > What about open vs. unregister?  The input core must have some 
> > > protection for those two.
> > > 
> > 
> > input_unregister_device() sets dev->going_away at the very beginning
> > and input_open_device() will fail with -ENODEV when trying to open such
> > devices. dev->going_away (among other things) is protected by
> > dev->mutex.
> > 
> > Do you see any issues with this scheme?
> 
> It depends on how much code is protected by dev->mutex.
> 
> The real issue involving open vs. unregister comes down to this.  It 
> should not be possible for either:
> 
>     (1) an input_unregister_device() call to return while an
> 	input_open_device() call is in progress; or

Theoretically it is possible with wierd scheduling i guess. What I can
guarantee is that either input_open_device() fails or if it succeeds
input_close_device() will be called for the device at some point but
before input_unregister_device() returns. Does this help?

> 
>     (2) an input_open_device() call to be made after
> 	input_unregister_device() has returned.

As soon as input_unregister_device() starts executing
input_open_device() will start failing. Additionally, when
unregistering a device we forcefully disconnect all attached input
handlers so by the time input_unregister_device() is finished there
should be noone who can call input_open_device().

-- 
Dmitry

Re: [PATCH 2/2] ati_remote2: Add autosuspend support

[Alan Stern]

On Tue, 17 Jun 2008, Dmitry Torokhov wrote:

> > > input_unregister_device() sets dev->going_away at the very beginning
> > > and input_open_device() will fail with -ENODEV when trying to open such
> > > devices. dev->going_away (among other things) is protected by
> > > dev->mutex.
> > > 
> > > Do you see any issues with this scheme?
> > 
> > It depends on how much code is protected by dev->mutex.
> > 
> > The real issue involving open vs. unregister comes down to this.  It 
> > should not be possible for either:
> > 
> >     (1) an input_unregister_device() call to return while an
> > 	input_open_device() call is in progress; or
> 
> Theoretically it is possible with wierd scheduling i guess. What I can
> guarantee is that either input_open_device() fails or if it succeeds
> input_close_device() will be called for the device at some point but
> before input_unregister_device() returns. Does this help?

Yes, that's right.  I expressed myself poorly above; what I meant was 
that it should be impossible for input_unregister_device() to return 
while a call to an input driver's open method is in progress.  Since 
the entire method call is protected by dev->mutex, this is guaranteed.

> > 
> >     (2) an input_open_device() call to be made after
> > 	input_unregister_device() has returned.
> 
> As soon as input_unregister_device() starts executing
> input_open_device() will start failing. Additionally, when
> unregistering a device we forcefully disconnect all attached input
> handlers so by the time input_unregister_device() is finished there
> should be noone who can call input_open_device().

Again, what I meant was that the open method should never be called 
after input_unregister_device() has returned, and again this is 
guaranteed.

So it all looks good from the point of view of an input device driver.  
The driver knows that after it has called input_unregister_device(),
the input core cannot have any threads for that device executing inside
the driver.

Alan Stern