ati_remote2 loadable keymap support

PATCH In review Peter Stokes 3 participants 6 messages Apr 5, 2008 – Jun 3, 2008

Subsystems (3)

HID Input drivers ATI_REMOTE2 driver

lore

Share & tools

lore

Peter Stokes

author

2008-04-05 18:33 UTC #0 lore raw

PATCH 1/1 ati_remote2 loadable keymap support

The attached patch adds support for loadable key maps support to the ati_remote2 driver.

This patch is similar to the previous version that I originally posted on 16th February 2008. As requested, I have removed the code that reconfigured this driver to utilise the input core's built in soft auto repeat functionality. Those changes are pending further clarification on how buttons that should not be auto repeated (e.g. mouse buttons) can be excluded from the soft auto repeat implementation.

Quoted 4 lines · On Tuesday 04 March 2008 22:17:49 Ville Syrjälä wrote:

However, since that seems unlikely to happen, and the input core already has support for keymaps I'm fine with adding the set/getkeycode stuff. What I'd like to drop is the support for five different keymaps since AFAICS that could be handled in a more generic way in user space.

I do not agree that the opportunity to remap the resultant keycode generated from any given physical key on the remote dependant upon which 'mode' the remote is currently in should be dropped. Consequently I have left the implementation in the attached patch.

I feel that a valid way of thinking about this implementation is that the hardware scancodes consist of the mode and key bytes combined. Given any such implementation it could be assumed that the device has 5 x 46 = 230 keys, it therefore seems entirely reasonable to me to provide a mechanism by which any of those keys can be remapped.

Quoted 6 lines · On Tuesday 04 March 2008 20:38:22 Ville Syrjälä wrote:

I think you could implement the multiple keymaps thing rather trivially in user space by having a small daemon listening on the event device and loading a new keymap when a mode key is pressed. That would limit the changes to the driver, and it would not require any kernel changes when if you would need to adapt it to a device that uses a different driver.

I do not agree that it this functionality would be trivial to implement in user space. The primary purpose behind making these changes is to provide full access to all of the keys within X windows, as such one is dependant upon the X input system interfacing to the event device. The goal is to make this device behave as a regular keyboard, not requiring any special case code in any application the user might wish to control using it.

I do not feel that my proposed code changes are particularly invasive. I would accept that the static table outlining the default keycode mappings represents a reasonable change. Whilst this table could be eliminated (and generated programmatically) I feel that it serves in some part towards documenting the implementation.

All of the changes proposed have no effect upon the operation of the driver out-of-the-box they merely provide a mechanism by which an end user can configure their setup to function as they desire.

Best regards

Peter Stokes

Signed-off-by Peter Stokes <[email protected]>

drivers/input/misc/ati_remote2.c +163 ~~-90~~

@@ -2,7 +2,7 @@
  * ati_remote2 - ATI/Philips USB RF remote driver
  *
  * Copyright (C) 2005 Ville Syrjala <[email protected]>
- * Copyright (C) 2007 Peter Stokes <[email protected]>
+ * Copyright (C) 2007 Peter Stokes <[email protected]>
  *
  * 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 @@
  * 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,61 +45,66 @@
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
+
+static u8 ati_remote2_modes[] = {
+	0x01, /* AUX1 */
+	0x02, /* AUX2 */
+	0x04, /* AUX3 */
+	0x08, /* AUX4 */
+	0x10, /* PC   */
+};
+
 static struct {
-	int hw_code;
-	int key_code;
-} ati_remote2_key_table[] = {
-	{ 0x00, KEY_0 },
-	{ 0x01, KEY_1 },
-	{ 0x02, KEY_2 },
-	{ 0x03, KEY_3 },
-	{ 0x04, KEY_4 },
-	{ 0x05, KEY_5 },
-	{ 0x06, KEY_6 },
-	{ 0x07, KEY_7 },
-	{ 0x08, KEY_8 },
-	{ 0x09, KEY_9 },
-	{ 0x0c, KEY_POWER },
-	{ 0x0d, KEY_MUTE },
-	{ 0x10, KEY_VOLUMEUP },
-	{ 0x11, KEY_VOLUMEDOWN },
-	{ 0x20, KEY_CHANNELUP },
-	{ 0x21, KEY_CHANNELDOWN },
-	{ 0x28, KEY_FORWARD },
-	{ 0x29, KEY_REWIND },
-	{ 0x2c, KEY_PLAY },
-	{ 0x30, KEY_PAUSE },
-	{ 0x31, KEY_STOP },
-	{ 0x37, KEY_RECORD },
-	{ 0x38, KEY_DVD },
-	{ 0x39, KEY_TV },
-	{ 0x54, KEY_MENU },
-	{ 0x58, KEY_UP },
-	{ 0x59, KEY_DOWN },
-	{ 0x5a, KEY_LEFT },
-	{ 0x5b, KEY_RIGHT },
-	{ 0x5c, KEY_OK },
-	{ 0x78, KEY_A },
-	{ 0x79, KEY_B },
-	{ 0x7a, KEY_C },
-	{ 0x7b, KEY_D },
-	{ 0x7c, KEY_E },
-	{ 0x7d, KEY_F },
-	{ 0x82, KEY_ENTER },
-	{ 0x8e, KEY_VENDOR },
-	{ 0x96, KEY_COFFEE },
-	{ 0xa9, BTN_LEFT },
-	{ 0xaa, BTN_RIGHT },
-	{ 0xbe, KEY_QUESTION },
-	{ 0xd5, KEY_FRONT },
-	{ 0xd0, KEY_EDIT },
-	{ 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 }
+	u8  hwcode;
+	u16 keycode[ARRAY_SIZE(ati_remote2_modes)];
+} ati_remote2_keycodes[] = {
+/*	 hwcode   AUX1             AUX2             AUX3             AUX4             PC                */
+	{ 0x00, { KEY_0,           KEY_0,           KEY_0,           KEY_0,           KEY_0           } },
+	{ 0x01, { KEY_1,           KEY_1,           KEY_1,           KEY_1,           KEY_1           } },
+	{ 0x02, { KEY_2,           KEY_2,           KEY_2,           KEY_2,           KEY_2           } },
+	{ 0x03, { KEY_3,           KEY_3,           KEY_3,           KEY_3,           KEY_3           } },
+	{ 0x04, { KEY_4,           KEY_4,           KEY_4,           KEY_4,           KEY_4           } },
+	{ 0x05, { KEY_5,           KEY_5,           KEY_5,           KEY_5,           KEY_5           } },
+	{ 0x06, { KEY_6,           KEY_6,           KEY_6,           KEY_6,           KEY_6           } },
+	{ 0x07, { KEY_7,           KEY_7,           KEY_7,           KEY_7,           KEY_7           } },
+	{ 0x08, { KEY_8,           KEY_8,           KEY_8,           KEY_8,           KEY_8           } },
+	{ 0x09, { KEY_9,           KEY_9,           KEY_9,           KEY_9,           KEY_9           } },
+	{ 0x0c, { KEY_POWER,       KEY_POWER,       KEY_POWER,       KEY_POWER,       KEY_POWER       } },
+	{ 0x0d, { KEY_MUTE,        KEY_MUTE,        KEY_MUTE,        KEY_MUTE,        KEY_MUTE        } },
+	{ 0x10, { KEY_VOLUMEUP,    KEY_VOLUMEUP,    KEY_VOLUMEUP,    KEY_VOLUMEUP,    KEY_VOLUMEUP    } },
+	{ 0x11, { KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN  } },
+	{ 0x20, { KEY_CHANNELUP,   KEY_CHANNELUP,   KEY_CHANNELUP,   KEY_CHANNELUP,   KEY_CHANNELUP   } },
+	{ 0x21, { KEY_CHANNELDOWN, KEY_CHANNELDOWN, KEY_CHANNELDOWN, KEY_CHANNELDOWN, KEY_CHANNELDOWN } },
+	{ 0x28, { KEY_FORWARD,     KEY_FORWARD,     KEY_FORWARD,     KEY_FORWARD,     KEY_FORWARD     } },
+	{ 0x29, { KEY_REWIND,      KEY_REWIND,      KEY_REWIND,      KEY_REWIND,      KEY_REWIND      } },
+	{ 0x2c, { KEY_PLAY,        KEY_PLAY,        KEY_PLAY,        KEY_PLAY,        KEY_PLAY        } },
+	{ 0x30, { KEY_PAUSE,       KEY_PAUSE,       KEY_PAUSE,       KEY_PAUSE,       KEY_PAUSE       } },
+	{ 0x31, { KEY_STOP,        KEY_STOP,        KEY_STOP,        KEY_STOP,        KEY_STOP        } },
+	{ 0x37, { KEY_RECORD,      KEY_RECORD,      KEY_RECORD,      KEY_RECORD,      KEY_RECORD      } },
+	{ 0x38, { KEY_DVD,         KEY_DVD,         KEY_DVD,         KEY_DVD,         KEY_DVD         } },
+	{ 0x39, { KEY_TV,          KEY_TV,          KEY_TV,          KEY_TV,          KEY_TV          } },
+	{ 0x3F, { KEY_PROG1,       KEY_PROG2,       KEY_PROG3,       KEY_PROG4,       KEY_PC          } },
+	{ 0x54, { KEY_MENU,        KEY_MENU,        KEY_MENU,        KEY_MENU,        KEY_MENU        } },
+	{ 0x58, { KEY_UP,          KEY_UP,          KEY_UP,          KEY_UP,          KEY_UP          } },
+	{ 0x59, { KEY_DOWN,        KEY_DOWN,        KEY_DOWN,        KEY_DOWN,        KEY_DOWN        } },
+	{ 0x5a, { KEY_LEFT,        KEY_LEFT,        KEY_LEFT,        KEY_LEFT,        KEY_LEFT        } },
+	{ 0x5b, { KEY_RIGHT,       KEY_RIGHT,       KEY_RIGHT,       KEY_RIGHT,       KEY_RIGHT       } },
+	{ 0x5c, { KEY_OK,          KEY_OK,          KEY_OK,          KEY_OK,          KEY_OK          } },
+	{ 0x78, { KEY_A,           KEY_A,           KEY_A,           KEY_A,           KEY_A           } },
+	{ 0x79, { KEY_B,           KEY_B,           KEY_B,           KEY_B,           KEY_B           } },
+	{ 0x7a, { KEY_C,           KEY_C,           KEY_C,           KEY_C,           KEY_C           } },
+	{ 0x7b, { KEY_D,           KEY_D,           KEY_D,           KEY_D,           KEY_D           } },
+	{ 0x7c, { KEY_E,           KEY_E,           KEY_E,           KEY_E,           KEY_E           } },
+	{ 0x7d, { KEY_F,           KEY_F,           KEY_F,           KEY_F,           KEY_F           } },
+	{ 0x82, { KEY_ENTER,       KEY_ENTER,       KEY_ENTER,       KEY_ENTER,       KEY_ENTER       } },
+	{ 0x8e, { KEY_VENDOR,      KEY_VENDOR,      KEY_VENDOR,      KEY_VENDOR,      KEY_VENDOR      } },
+	{ 0x96, { KEY_COFFEE,      KEY_COFFEE,      KEY_COFFEE,      KEY_COFFEE,      KEY_COFFEE      } },
+	{ 0xa9, { BTN_LEFT,        BTN_LEFT,        BTN_LEFT,        BTN_LEFT,        BTN_LEFT        } },
+	{ 0xaa, { BTN_RIGHT,       BTN_RIGHT,       BTN_RIGHT,       BTN_RIGHT,       BTN_RIGHT,      } },
+	{ 0xbe, { KEY_QUESTION,    KEY_QUESTION,    KEY_QUESTION,    KEY_QUESTION,    KEY_QUESTION,   } },
+	{ 0xd0, { KEY_EDIT,        KEY_EDIT,        KEY_EDIT,        KEY_EDIT,        KEY_EDIT        } },
+	{ 0xd5, { KEY_FRONT,       KEY_FRONT,       KEY_FRONT,       KEY_FRONT,       KEY_FRONT       } },
+	{ 0xf9, { KEY_INFO,        KEY_INFO,        KEY_INFO,        KEY_INFO,        KEY_INFO        } }
 };
 
 struct ati_remote2 {

@@ -117,6 +122,8 @@
 
 	char name[64];
 	char phys[64];
+
+	u32 keycode[ARRAY_SIZE(ati_remote2_keycodes)][ARRAY_SIZE(ati_remote2_modes)];
 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);

@@ -159,11 +166,84 @@
 	usb_kill_urb(ar2->urb[1]);
 }
 
+static int ati_remote2_lookup(u8 hwcode)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ati_remote2_keycodes); i++)
+		if (ati_remote2_keycodes[i].hwcode == hwcode)
+			return i;
+
+	return -1;
+}
+
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			*keycode = ar2->keycode[index][mode];
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int ati_remote2_setkeycode(struct input_dev *idev,
+				  int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int old_keycode = -1;
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	if (keycode < 0 || keycode > KEY_MAX)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			old_keycode = ar2->keycode[index][mode];
+			ar2->keycode[index][mode] = keycode;
+		}
+	}
+
+	set_bit(keycode, idev->keybit);
+
+	for (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			if (ar2->keycode[index][mode] == old_keycode)
+				return 0;
+		}
+	}
+
+	clear_bit(old_keycode, idev->keybit);
+
+	return 0;
+}
+
+
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[0];
-	int channel, mode;
+	u8 channel, mode;
 
 	channel = data[0] >> 4;

@@ -187,22 +267,12 @@
 	input_sync(idev);
 }
 
-static int ati_remote2_lookup(unsigned int hw_code)
-{
-	int i;
-
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
-		if (ati_remote2_key_table[i].hw_code == hw_code)
-			return i;
-
-	return -1;
-}
-
 static void ati_remote2_input_key(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[1];
-	int channel, mode, hw_code, index;
+	u8 channel, mode;
+	int index;
 
 	channel = data[0] >> 4;

@@ -218,12 +288,10 @@
 		return;
 	}
 
-	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) {
+	if (!((1 << mode) & mode_mask))
+		return;
+
+	if (data[2] == 0x3f) {
 		/*
 		 * For some incomprehensible reason the mouse pad generates
 		 * events which look identical to the events from the last

@@ -236,14 +304,9 @@
 
 		if (data[1] == 0)
 			ar2->mode = mode;
-
-		hw_code |= mode << 8;
 	}
 
-	if (!((1 << mode) & mode_mask))
-		return;
-
-	index = ati_remote2_lookup(hw_code);
+	index = ati_remote2_lookup(data[2]);
 	if (index < 0) {
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown code byte (%02x %02x %02x %02x)\n",

@@ -260,8 +323,7 @@
 	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 (data[2] == 0xa9 || data[2] == 0xaa)
 			return;
 
 		if (!time_after_eq(jiffies, ar2->jiffies))

@@ -276,7 +338,7 @@
 		return;
 	}
 
-	input_event(idev, EV_KEY, ati_remote2_key_table[index].key_code, data[1]);
+	input_event(idev, EV_KEY, ar2->keycode[index][mode], data[1]);
 	input_sync(idev);
 }

@@ -334,10 +396,11 @@
 			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 }
 
+
 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)

@@ -347,11 +410,15 @@
 	input_set_drvdata(idev, ar2);
 
 	idev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) | BIT_MASK(EV_REL);
-	idev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
-		BIT_MASK(BTN_RIGHT);
+	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 (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			ar2->keycode[index][mode] = ati_remote2_keycodes[index].keycode[mode];
+			set_bit(ar2->keycode[index][mode], idev->keybit);
+		}
+	}
 
 	idev->rep[REP_DELAY]  = 250;
 	idev->rep[REP_PERIOD] = 33;

@@ -359,6 +426,9 @@
 	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;

@@ -532,6 +602,9 @@
 	else
 		printk(KERN_INFO "ati_remote2: " DRIVER_DESC " " DRIVER_VERSION "\n");
 
+	mode_mask &= 0x1F;
+	channel_mask &= 0xFFFF;
+
 	return r;
 }

Ville Syrjälä

maintainer

2008-04-08 20:18 UTC #1 lore raw

Quoted 35 lines · Peter Stokes

The attached patch adds support for loadable key maps support to the ati_remote2 driver.
This patch is similar to the previous version that I originally posted on 16th February 2008. As requested, I have removed the code that reconfigured this driver to utilise the input core's built in soft auto repeat functionality. Those changes are pending further clarification on how buttons that should not be auto repeated (e.g. mouse buttons) can be excluded from the soft auto repeat implementation.
Quoted 4 lines · On Tuesday 04 March 2008 22:17:49 Ville Syrjälä wrote:
However, since that seems unlikely to happen, and the input core already has support for keymaps I'm fine with adding the set/getkeycode stuff. What I'd like to drop is the support for five different keymaps since AFAICS that could be handled in a more generic way in user space.
I do not agree that the opportunity to remap the resultant keycode generated from any given physical key on the remote dependant upon which 'mode' the remote is currently in should be dropped. Consequently I have left the implementation in the attached patch.
I feel that a valid way of thinking about this implementation is that the hardware scancodes consist of the mode and key bytes combined. Given any such implementation it could be assumed that the device has 5 x 46 = 230 keys, it therefore seems entirely reasonable to me to provide a mechanism by which any of those keys can be remapped.
Quoted 6 lines · On Tuesday 04 March 2008 20:38:22 Ville Syrjälä wrote:
I think you could implement the multiple keymaps thing rather trivially in user space by having a small daemon listening on the event device and loading a new keymap when a mode key is pressed. That would limit the changes to the driver, and it would not require any kernel changes when if you would need to adapt it to a device that uses a different driver.

Quoted 6 lines

I do not agree that it this functionality would be trivial to implement in user space. The primary purpose behind making these changes is to provide full access to all of the keys within X windows, as such one is dependant upon the X input system interfacing to the event device. The goal is to make this device behave as a regular keyboard, not requiring any special case code in any application the user might wish to control using it.

Userspace keymap handling would not require any special case code in many applications. It would just need a small daemon that would react to the mode keys and reload the keymap. Getting such a thing packaged/distributed might be a bigger challenge though, so if you really want to have it in the kernel driver I can live with it :)

Quoted 5 lines

I do not feel that my proposed code changes are particularly invasive. I would accept that the static table outlining the default keycode mappings represents a reasonable change. Whilst this table could be eliminated (and generated programmatically) I feel that it serves in some part towards documenting the implementation.

It does increase the module size though.

Quoted 16 lines

All of the changes proposed have no effect upon the operation of the driver out-of-the-box they merely provide a mechanism by which an end user can configure their setup to function as they desire.
Best regards
Peter Stokes
Signed-off-by Peter Stokes <[email protected]>

drivers/input/misc/ati_remote2.c line 5

Quoted 5 lines

drivers/input/misc/ati_remote2.c +1 -1

@@ -2,7 +2,7 @@
  * ati_remote2 - ATI/Philips USB RF remote driver
  *
  * Copyright (C) 2005 Ville Syrjala <[email protected]>
- * Copyright (C) 2007 Peter Stokes <[email protected]>
+ * Copyright (C) 2007 Peter Stokes <[email protected]>

2008?

Quoted 13 lines

drivers/input/misc/ati_remote2.c +1 -1

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

drivers/input/misc/ati_remote2.c line 32

Quoted 7 lines

drivers/input/misc/ati_remote2.c +1 -1

@@ -27,7 +27,7 @@
  * 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".
  */

Unrelated change. I saw a couple of other unrelated changes too (just changing the whitespace) further down.

drivers/input/misc/ati_remote2.c line 55

Quoted 13 lines

drivers/input/misc/ati_remote2.c +8 -0

@@ -45,61 +45,66 @@
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
+
+static u8 ati_remote2_modes[] = {
+	0x01, /* AUX1 */
+	0x02, /* AUX2 */
+	0x04, /* AUX3 */
+	0x08, /* AUX4 */
+	0x10, /* PC   */
+};

It seems you only use this table to get the number of modes. A define would suffice.

Quoted 49 lines

drivers/input/misc/ati_remote2.c +0 ~~-48~~

 static struct {
-	int hw_code;
-	int key_code;
-} ati_remote2_key_table[] = {
-	{ 0x00, KEY_0 },
-	{ 0x01, KEY_1 },
-	{ 0x02, KEY_2 },
-	{ 0x03, KEY_3 },
-	{ 0x04, KEY_4 },
-	{ 0x05, KEY_5 },
-	{ 0x06, KEY_6 },
-	{ 0x07, KEY_7 },
-	{ 0x08, KEY_8 },
-	{ 0x09, KEY_9 },
-	{ 0x0c, KEY_POWER },
-	{ 0x0d, KEY_MUTE },
-	{ 0x10, KEY_VOLUMEUP },
-	{ 0x11, KEY_VOLUMEDOWN },
-	{ 0x20, KEY_CHANNELUP },
-	{ 0x21, KEY_CHANNELDOWN },
-	{ 0x28, KEY_FORWARD },
-	{ 0x29, KEY_REWIND },
-	{ 0x2c, KEY_PLAY },
-	{ 0x30, KEY_PAUSE },
-	{ 0x31, KEY_STOP },
-	{ 0x37, KEY_RECORD },
-	{ 0x38, KEY_DVD },
-	{ 0x39, KEY_TV },
-	{ 0x54, KEY_MENU },
-	{ 0x58, KEY_UP },
-	{ 0x59, KEY_DOWN },
-	{ 0x5a, KEY_LEFT },
-	{ 0x5b, KEY_RIGHT },
-	{ 0x5c, KEY_OK },
-	{ 0x78, KEY_A },
-	{ 0x79, KEY_B },
-	{ 0x7a, KEY_C },
-	{ 0x7b, KEY_D },
-	{ 0x7c, KEY_E },
-	{ 0x7d, KEY_F },
-	{ 0x82, KEY_ENTER },
-	{ 0x8e, KEY_VENDOR },
-	{ 0x96, KEY_COFFEE },
-	{ 0xa9, BTN_LEFT },
-	{ 0xaa, BTN_RIGHT },
-	{ 0xbe, KEY_QUESTION },
-	{ 0xd5, KEY_FRONT },
-	{ 0xd0, KEY_EDIT },
-	{ 0xf9, KEY_INFO },

drivers/input/misc/ati_remote2.c line 61

Quoted 10 lines

drivers/input/misc/ati_remote2.c +4 -6

-	{ (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 }
+	u8  hwcode;
+	u16 keycode[ARRAY_SIZE(ati_remote2_modes)];
+} ati_remote2_keycodes[] = {
+/*	 hwcode   AUX1             AUX2             AUX3             AUX4             PC                */

checkpatch.pl doesn't like these long lines.

drivers/input/misc/ati_remote2.c line 126

Quoted 7 lines

drivers/input/misc/ati_remote2.c +2 -0

 struct ati_remote2 {

@@ -117,6 +122,8 @@
 
 	char name[64];
 	char phys[64];
+
+	u32 keycode[ARRAY_SIZE(ati_remote2_keycodes)][ARRAY_SIZE(ati_remote2_modes)];

Somehow it would seem more natural to me if you would swap the array dimensions. But that's a minor issue and you can leave it like this if you prefer.

Quoted 27 lines

drivers/input/misc/ati_remote2.c +20 -0

 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);

@@ -159,11 +166,84 @@
 	usb_kill_urb(ar2->urb[1]);
 }
 
+static int ati_remote2_lookup(u8 hwcode)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ati_remote2_keycodes); i++)
+		if (ati_remote2_keycodes[i].hwcode == hwcode)
+			return i;
+
+	return -1;
+}
+
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+

drivers/input/misc/ati_remote2.c line 198

Quoted 10 lines

drivers/input/misc/ati_remote2.c +10 -0

+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			*keycode = ar2->keycode[index][mode];
+			return 0;
+		}
+	}

Is there a reason you using the high bits like a mask? IIRC the hardware mode byte isn't a mask. So something like this should do:

mode = scancode >> 8;
if (mode < 0 || mode > MAX_MODE)

	return -EINVAL;
index = ati_remote2_lookup(scancode & 0xff);
if (index < 0)
	return -EINVAL;

I'm not sure if mode_mask should affect the keymap handling at all since both can be changed runtime.

Quoted 4 lines

drivers/input/misc/ati_remote2.c +4 -0
+
+	return -EINVAL;
+}
+

drivers/input/misc/ati_remote2.c line 212

Quoted 10 lines

drivers/input/misc/ati_remote2.c +10 -0

+static int ati_remote2_setkeycode(struct input_dev *idev,
+				  int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int old_keycode = -1;
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+

Same comments as above.

drivers/input/misc/ati_remote2.c line 217

Quoted 5 lines

drivers/input/misc/ati_remote2.c +5 -0

+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	if (keycode < 0 || keycode > KEY_MAX)

Are KEY_RESERVED/KEY_MAX valid here?

Quoted 7 lines

drivers/input/misc/ati_remote2.c +7 -0

+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			old_keycode = ar2->keycode[index][mode];
+			ar2->keycode[index][mode] = keycode;
+		}

drivers/input/misc/ati_remote2.c line 234

Quoted 10 lines

drivers/input/misc/ati_remote2.c +10 -0

+	}
+
+	set_bit(keycode, idev->keybit);
+
+	for (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			if (ar2->keycode[index][mode] == old_keycode)
+				return 0;
+		}
+	}

Because of using the scancode high bits as a mask this could leave some bits set even though they are no longer in the map.

Quoted 59 lines

drivers/input/misc/ati_remote2.c +14 ~~-19~~

+
+	clear_bit(old_keycode, idev->keybit);
+
+	return 0;
+}
+
+
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[0];
-	int channel, mode;
+	u8 channel, mode;
 
 	channel = data[0] >> 4;

@@ -187,22 +267,12 @@
 	input_sync(idev);
 }
 
-static int ati_remote2_lookup(unsigned int hw_code)
-{
-	int i;
-
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
-		if (ati_remote2_key_table[i].hw_code == hw_code)
-			return i;
-
-	return -1;
-}
-
 static void ati_remote2_input_key(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[1];
-	int channel, mode, hw_code, index;
+	u8 channel, mode;
+	int index;
 
 	channel = data[0] >> 4;

@@ -218,12 +288,10 @@
 		return;
 	}
 
-	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) {
+	if (!((1 << mode) & mode_mask))
+		return;
+
+	if (data[2] == 0x3f) {
 		/*
 		 * For some incomprehensible reason the mouse pad generates
 		 * events which look identical to the events from the last

drivers/input/misc/ati_remote2.c line 244

Quoted 9 lines

drivers/input/misc/ati_remote2.c +0 -4

@@ -236,14 +304,9 @@
 
 		if (data[1] == 0)
 			ar2->mode = mode;
-
-		hw_code |= mode << 8;
 	}
 
-	if (!((1 << mode) & mode_mask))
-		return;

Moving this test before updating ar2->mode could leave the driver unaware of the actual mode it's in. I think it could leave to key events getting through from the mouse at least when mode_mask is suitably changed runtime. Was there a reason for this change?

Quoted 69 lines

drivers/input/misc/ati_remote2.c +16 ~~-10~~

-
-	index = ati_remote2_lookup(hw_code);
+	index = ati_remote2_lookup(data[2]);
 	if (index < 0) {
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown code byte (%02x %02x %02x %02x)\n",

@@ -260,8 +323,7 @@
 	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 (data[2] == 0xa9 || data[2] == 0xaa)
 			return;
 
 		if (!time_after_eq(jiffies, ar2->jiffies))

@@ -276,7 +338,7 @@
 		return;
 	}
 
-	input_event(idev, EV_KEY, ati_remote2_key_table[index].key_code, data[1]);
+	input_event(idev, EV_KEY, ar2->keycode[index][mode], data[1]);
 	input_sync(idev);
 }

@@ -334,10 +396,11 @@
 			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 }
 
+
 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)

@@ -347,11 +410,15 @@
 	input_set_drvdata(idev, ar2);
 
 	idev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) | BIT_MASK(EV_REL);
-	idev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
-		BIT_MASK(BTN_RIGHT);
+	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 (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			ar2->keycode[index][mode] = ati_remote2_keycodes[index].keycode[mode];
+			set_bit(ar2->keycode[index][mode], idev->keybit);
+		}
+	}
 
 	idev->rep[REP_DELAY]  = 250;
 	idev->rep[REP_PERIOD] = 33;

@@ -359,6 +426,9 @@
 	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;

drivers/input/misc/ati_remote2.c line 607

Quoted 6 lines

drivers/input/misc/ati_remote2.c +3 -0

@@ -532,6 +602,9 @@
 	else
 		printk(KERN_INFO "ati_remote2: " DRIVER_DESC " " DRIVER_VERSION "\n");
 
+	mode_mask &= 0x1F;
+	channel_mask &= 0xFFFF;
+

As I said these can be changed runtime, so masking them should happen every time they are used. A nicer solution would be to actually reject invalid values.

Quoted 3 lines

drivers/input/misc/ati_remote2.c +0 -0
 	return r;
 }
 

Signature · 7 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov

maintainer

2008-04-17 19:00 UTC #2 lore raw

Quoted 8 lines · Ville Syrj?l?

drivers/input/misc/ati_remote2.c line 61
Quoted 4 lines · Peter Stokes
drivers/input/misc/ati_remote2.c +4 -0
+	u8  hwcode;
+	u16 keycode[ARRAY_SIZE(ati_remote2_modes)];
+} ati_remote2_keycodes[] = {
+/*	 hwcode   AUX1             AUX2             AUX3             AUX4             PC                */
checkpatch.pl doesn't like these long lines.

In this case screw checkpatch, sometimes longer lines are better than the alternative.

Peter, is an updated patch addressing other Ville's concerns coming?

Signature · 1 line

Dmitry

Peter Stokes

author

2008-04-18 20:49 UTC #3 lore raw

Quoted 10 lines · On Thursday 17 April 2008 20:00:23 Dmitry Torokhov wrote:

Quoted 8 lines · Ville Syrj?l?
Review
Quoted 5 lines · Peter Stokes
(quoted diff) +4 -0
+	u8  hwcode;
+	u16 keycode[ARRAY_SIZE(ati_remote2_modes)];
+} ati_remote2_keycodes[] = {
+/*	 hwcode   AUX1             AUX2             AUX3             AUX4  
          PC                */
checkpatch.pl doesn't like these long lines.

Quoted 4 lines

In this case screw checkpatch, sometimes longer lines are better than the alternative.
Peter, is an updated patch addressing other Ville's concerns coming?

Please find attached an updated patch. Sorry for the delay.

Best regards

Peter

Signed-off-by Peter Stokes <[email protected]>

drivers/input/misc/ati_remote2.c +157 ~~-87~~

@@ -2,7 +2,7 @@
  * ati_remote2 - ATI/Philips USB RF remote driver
  *
  * Copyright (C) 2005 Ville Syrjala <[email protected]>
- * Copyright (C) 2007 Peter Stokes <[email protected]>
+ * Copyright (C) 2008 Peter Stokes <[email protected]>
  *
  * 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 @@
  * 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,61 +45,66 @@
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 
+
+static u8 ati_remote2_modes[] = {
+	0x01, /* AUX1 */
+	0x02, /* AUX2 */
+	0x04, /* AUX3 */
+	0x08, /* AUX4 */
+	0x10, /* PC   */
+};
+
 static struct {
-	int hw_code;
-	int key_code;
-} ati_remote2_key_table[] = {
-	{ 0x00, KEY_0 },
-	{ 0x01, KEY_1 },
-	{ 0x02, KEY_2 },
-	{ 0x03, KEY_3 },
-	{ 0x04, KEY_4 },
-	{ 0x05, KEY_5 },
-	{ 0x06, KEY_6 },
-	{ 0x07, KEY_7 },
-	{ 0x08, KEY_8 },
-	{ 0x09, KEY_9 },
-	{ 0x0c, KEY_POWER },
-	{ 0x0d, KEY_MUTE },
-	{ 0x10, KEY_VOLUMEUP },
-	{ 0x11, KEY_VOLUMEDOWN },
-	{ 0x20, KEY_CHANNELUP },
-	{ 0x21, KEY_CHANNELDOWN },
-	{ 0x28, KEY_FORWARD },
-	{ 0x29, KEY_REWIND },
-	{ 0x2c, KEY_PLAY },
-	{ 0x30, KEY_PAUSE },
-	{ 0x31, KEY_STOP },
-	{ 0x37, KEY_RECORD },
-	{ 0x38, KEY_DVD },
-	{ 0x39, KEY_TV },
-	{ 0x54, KEY_MENU },
-	{ 0x58, KEY_UP },
-	{ 0x59, KEY_DOWN },
-	{ 0x5a, KEY_LEFT },
-	{ 0x5b, KEY_RIGHT },
-	{ 0x5c, KEY_OK },
-	{ 0x78, KEY_A },
-	{ 0x79, KEY_B },
-	{ 0x7a, KEY_C },
-	{ 0x7b, KEY_D },
-	{ 0x7c, KEY_E },
-	{ 0x7d, KEY_F },
-	{ 0x82, KEY_ENTER },
-	{ 0x8e, KEY_VENDOR },
-	{ 0x96, KEY_COFFEE },
-	{ 0xa9, BTN_LEFT },
-	{ 0xaa, BTN_RIGHT },
-	{ 0xbe, KEY_QUESTION },
-	{ 0xd5, KEY_FRONT },
-	{ 0xd0, KEY_EDIT },
-	{ 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 }
+	u8  hwcode;
+	u16 keycode[ARRAY_SIZE(ati_remote2_modes)];
+} ati_remote2_keycodes[] = {
+/*	 hwcode   AUX1             AUX2             AUX3             AUX4             PC                */
+	{ 0x00, { KEY_0,           KEY_0,           KEY_0,           KEY_0,           KEY_0           } },
+	{ 0x01, { KEY_1,           KEY_1,           KEY_1,           KEY_1,           KEY_1           } },
+	{ 0x02, { KEY_2,           KEY_2,           KEY_2,           KEY_2,           KEY_2           } },
+	{ 0x03, { KEY_3,           KEY_3,           KEY_3,           KEY_3,           KEY_3           } },
+	{ 0x04, { KEY_4,           KEY_4,           KEY_4,           KEY_4,           KEY_4           } },
+	{ 0x05, { KEY_5,           KEY_5,           KEY_5,           KEY_5,           KEY_5           } },
+	{ 0x06, { KEY_6,           KEY_6,           KEY_6,           KEY_6,           KEY_6           } },
+	{ 0x07, { KEY_7,           KEY_7,           KEY_7,           KEY_7,           KEY_7           } },
+	{ 0x08, { KEY_8,           KEY_8,           KEY_8,           KEY_8,           KEY_8           } },
+	{ 0x09, { KEY_9,           KEY_9,           KEY_9,           KEY_9,           KEY_9           } },
+	{ 0x0c, { KEY_POWER,       KEY_POWER,       KEY_POWER,       KEY_POWER,       KEY_POWER       } },
+	{ 0x0d, { KEY_MUTE,        KEY_MUTE,        KEY_MUTE,        KEY_MUTE,        KEY_MUTE        } },
+	{ 0x10, { KEY_VOLUMEUP,    KEY_VOLUMEUP,    KEY_VOLUMEUP,    KEY_VOLUMEUP,    KEY_VOLUMEUP    } },
+	{ 0x11, { KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN,  KEY_VOLUMEDOWN  } },
+	{ 0x20, { KEY_CHANNELUP,   KEY_CHANNELUP,   KEY_CHANNELUP,   KEY_CHANNELUP,   KEY_CHANNELUP   } },
+	{ 0x21, { KEY_CHANNELDOWN, KEY_CHANNELDOWN, KEY_CHANNELDOWN, KEY_CHANNELDOWN, KEY_CHANNELDOWN } },
+	{ 0x28, { KEY_FORWARD,     KEY_FORWARD,     KEY_FORWARD,     KEY_FORWARD,     KEY_FORWARD     } },
+	{ 0x29, { KEY_REWIND,      KEY_REWIND,      KEY_REWIND,      KEY_REWIND,      KEY_REWIND      } },
+	{ 0x2c, { KEY_PLAY,        KEY_PLAY,        KEY_PLAY,        KEY_PLAY,        KEY_PLAY        } },
+	{ 0x30, { KEY_PAUSE,       KEY_PAUSE,       KEY_PAUSE,       KEY_PAUSE,       KEY_PAUSE       } },
+	{ 0x31, { KEY_STOP,        KEY_STOP,        KEY_STOP,        KEY_STOP,        KEY_STOP        } },
+	{ 0x37, { KEY_RECORD,      KEY_RECORD,      KEY_RECORD,      KEY_RECORD,      KEY_RECORD      } },
+	{ 0x38, { KEY_DVD,         KEY_DVD,         KEY_DVD,         KEY_DVD,         KEY_DVD         } },
+	{ 0x39, { KEY_TV,          KEY_TV,          KEY_TV,          KEY_TV,          KEY_TV          } },
+	{ 0x3F, { KEY_PROG1,       KEY_PROG2,       KEY_PROG3,       KEY_PROG4,       KEY_PC          } },
+	{ 0x54, { KEY_MENU,        KEY_MENU,        KEY_MENU,        KEY_MENU,        KEY_MENU        } },
+	{ 0x58, { KEY_UP,          KEY_UP,          KEY_UP,          KEY_UP,          KEY_UP          } },
+	{ 0x59, { KEY_DOWN,        KEY_DOWN,        KEY_DOWN,        KEY_DOWN,        KEY_DOWN        } },
+	{ 0x5a, { KEY_LEFT,        KEY_LEFT,        KEY_LEFT,        KEY_LEFT,        KEY_LEFT        } },
+	{ 0x5b, { KEY_RIGHT,       KEY_RIGHT,       KEY_RIGHT,       KEY_RIGHT,       KEY_RIGHT       } },
+	{ 0x5c, { KEY_OK,          KEY_OK,          KEY_OK,          KEY_OK,          KEY_OK          } },
+	{ 0x78, { KEY_A,           KEY_A,           KEY_A,           KEY_A,           KEY_A           } },
+	{ 0x79, { KEY_B,           KEY_B,           KEY_B,           KEY_B,           KEY_B           } },
+	{ 0x7a, { KEY_C,           KEY_C,           KEY_C,           KEY_C,           KEY_C           } },
+	{ 0x7b, { KEY_D,           KEY_D,           KEY_D,           KEY_D,           KEY_D           } },
+	{ 0x7c, { KEY_E,           KEY_E,           KEY_E,           KEY_E,           KEY_E           } },
+	{ 0x7d, { KEY_F,           KEY_F,           KEY_F,           KEY_F,           KEY_F           } },
+	{ 0x82, { KEY_ENTER,       KEY_ENTER,       KEY_ENTER,       KEY_ENTER,       KEY_ENTER       } },
+	{ 0x8e, { KEY_VENDOR,      KEY_VENDOR,      KEY_VENDOR,      KEY_VENDOR,      KEY_VENDOR      } },
+	{ 0x96, { KEY_COFFEE,      KEY_COFFEE,      KEY_COFFEE,      KEY_COFFEE,      KEY_COFFEE      } },
+	{ 0xa9, { BTN_LEFT,        BTN_LEFT,        BTN_LEFT,        BTN_LEFT,        BTN_LEFT        } },
+	{ 0xaa, { BTN_RIGHT,       BTN_RIGHT,       BTN_RIGHT,       BTN_RIGHT,       BTN_RIGHT,      } },
+	{ 0xbe, { KEY_QUESTION,    KEY_QUESTION,    KEY_QUESTION,    KEY_QUESTION,    KEY_QUESTION,   } },
+	{ 0xd0, { KEY_EDIT,        KEY_EDIT,        KEY_EDIT,        KEY_EDIT,        KEY_EDIT        } },
+	{ 0xd5, { KEY_FRONT,       KEY_FRONT,       KEY_FRONT,       KEY_FRONT,       KEY_FRONT       } },
+	{ 0xf9, { KEY_INFO,        KEY_INFO,        KEY_INFO,        KEY_INFO,        KEY_INFO        } }
 };
 
 struct ati_remote2 {

@@ -117,6 +122,8 @@
 
 	char name[64];
 	char phys[64];
+
+	u32 keycode[ARRAY_SIZE(ati_remote2_keycodes)][ARRAY_SIZE(ati_remote2_modes)];
 };
 
 static int ati_remote2_probe(struct usb_interface *interface, const struct usb_device_id *id);

@@ -159,11 +166,84 @@
 	usb_kill_urb(ar2->urb[1]);
 }
 
+static int ati_remote2_lookup(u8 hwcode)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ati_remote2_keycodes); i++)
+		if (ati_remote2_keycodes[i].hwcode == hwcode)
+			return i;
+
+	return -1;
+}
+
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	if (((scancode >> 8) & (mode_mask & 0x1F)) != (scancode >> 8))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			*keycode = ar2->keycode[index][mode];
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int ati_remote2_setkeycode(struct input_dev *idev,
+				  int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int old_keycode = -1;
+	int index, mode;
+
+	if (((scancode >> 8) & (mode_mask & 0x1F)) != (scancode >> 8))
+		return -EINVAL;
+
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	if (keycode < 0 || keycode > KEY_MAX)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			old_keycode = ar2->keycode[index][mode];
+			ar2->keycode[index][mode] = keycode;
+		}
+	}
+
+	set_bit(keycode, idev->keybit);
+
+	for (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			if (ar2->keycode[index][mode] == old_keycode)
+				return 0;
+		}
+	}
+
+	clear_bit(old_keycode, idev->keybit);
+
+	return 0;
+}
+
+
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[0];
-	int channel, mode;
+	u8 channel, mode;
 
 	channel = data[0] >> 4;

@@ -187,22 +267,12 @@
 	input_sync(idev);
 }
 
-static int ati_remote2_lookup(unsigned int hw_code)
-{
-	int i;
-
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
-		if (ati_remote2_key_table[i].hw_code == hw_code)
-			return i;
-
-	return -1;
-}
-
 static void ati_remote2_input_key(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[1];
-	int channel, mode, hw_code, index;
+	u8 channel, mode;
+	int index;
 
 	channel = data[0] >> 4;

@@ -218,12 +288,7 @@
 		return;
 	}
 
-	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) {
+	if (data[2] == 0x3f) {
 		/*
 		 * For some incomprehensible reason the mouse pad generates
 		 * events which look identical to the events from the last

@@ -236,14 +301,12 @@
 
 		if (data[1] == 0)
 			ar2->mode = mode;
-
-		hw_code |= mode << 8;
 	}
 
 	if (!((1 << mode) & mode_mask))
 		return;
 
-	index = ati_remote2_lookup(hw_code);
+	index = ati_remote2_lookup(data[2]);
 	if (index < 0) {
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown code byte (%02x %02x %02x %02x)\n",

@@ -260,8 +323,7 @@
 	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 (data[2] == 0xa9 || data[2] == 0xaa)
 			return;
 
 		if (!time_after_eq(jiffies, ar2->jiffies))

@@ -276,7 +338,7 @@
 		return;
 	}
 
-	input_event(idev, EV_KEY, ati_remote2_key_table[index].key_code, data[1]);
+	input_event(idev, EV_KEY, ar2->keycode[index][mode], data[1]);
 	input_sync(idev);
 }

@@ -334,10 +396,11 @@
 			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 }
 
+
 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)

@@ -347,11 +410,15 @@
 	input_set_drvdata(idev, ar2);
 
 	idev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) | BIT_MASK(EV_REL);
-	idev->keybit[BIT_WORD(BTN_MOUSE)] = BIT_MASK(BTN_LEFT) |
-		BIT_MASK(BTN_RIGHT);
+	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 (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			ar2->keycode[index][mode] = ati_remote2_keycodes[index].keycode[mode];
+			set_bit(ar2->keycode[index][mode], idev->keybit);
+		}
+	}
 
 	idev->rep[REP_DELAY]  = 250;
 	idev->rep[REP_PERIOD] = 33;

@@ -359,6 +426,9 @@
 	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;

Peter Stokes

author

2008-04-18 19:42 UTC #4 lore raw

In reply to Ville Syrjälä

Ville,

My apologies for not responding to your feedback sooner. I had not noticed the comments that you had inserted within the patch. Please find my responses below:

Quoted 81 lines · On Tuesday 08 April 2008 21:18:26 Ville Syrjälä wrote:

Quoted 35 lines · Peter Stokes
The attached patch adds support for loadable key maps support to the ati_remote2 driver.
This patch is similar to the previous version that I originally posted on 16th February 2008. As requested, I have removed the code that reconfigured this driver to utilise the input core's built in soft auto repeat functionality. Those changes are pending further clarification on how buttons that should not be auto repeated (e.g. mouse buttons) can be excluded from the soft auto repeat implementation.
Quoted 4 lines · On Tuesday 04 March 2008 22:17:49 Ville Syrjälä wrote:
However, since that seems unlikely to happen, and the input core already has support for keymaps I'm fine with adding the set/getkeycode stuff. What I'd like to drop is the support for five different keymaps since AFAICS that could be handled in a more generic way in user space.
I do not agree that the opportunity to remap the resultant keycode generated from any given physical key on the remote dependant upon which 'mode' the remote is currently in should be dropped. Consequently I have left the implementation in the attached patch.
I feel that a valid way of thinking about this implementation is that the hardware scancodes consist of the mode and key bytes combined. Given any such implementation it could be assumed that the device has 5 x 46 = 230 keys, it therefore seems entirely reasonable to me to provide a mechanism by which any of those keys can be remapped.
Quoted 6 lines · On Tuesday 04 March 2008 20:38:22 Ville Syrjälä wrote:
I think you could implement the multiple keymaps thing rather trivially in user space by having a small daemon listening on the event device and loading a new keymap when a mode key is pressed. That would limit the changes to the driver, and it would not require any kernel changes when if you would need to adapt it to a device that uses a different driver.
Quoted 7 lines
I do not agree that it this functionality would be trivial to implement in user space. The primary purpose behind making these changes is to provide full access to all of the keys within X windows, as such one is dependant upon the X input system interfacing to the event device. The goal is to make this device behave as a regular keyboard, not requiring any special case code in any application the user might wish to control using it.
Userspace keymap handling would not require any special case code in many applications. It would just need a small daemon that would react to the mode keys and reload the keymap. Getting such a thing packaged/distributed might be a bigger challenge though, so if you really want to have it in the kernel driver I can live with it :)
Quoted 5 lines
I do not feel that my proposed code changes are particularly invasive. I would accept that the static table outlining the default keycode mappings represents a reasonable change. Whilst this table could be eliminated (and generated programmatically) I feel that it serves in some part towards documenting the implementation.
It does increase the module size though.
Quoted 13 lines
All of the changes proposed have no effect upon the operation of the driver out-of-the-box they merely provide a mechanism by which an end user can configure their setup to function as they desire.
Best regards
Peter Stokes
Signed-off-by Peter Stokes <[email protected]>
Quoted 9 lines
--- linux-2.6.24-orig/drivers/input/misc/ati_remote2.c 2008-01-24
22:58:37.000000000 +0000 +++
linux-2.6.24/drivers/input/misc/ati_remote2.c 2008-04-05
18:34:31.000000000 +0100 @@ -2,7 +2,7 @@
* ati_remote2 - ATI/Philips USB RF remote driver
*
* Copyright (C) 2005 Ville Syrjala <[email protected]>
- * Copyright (C) 2007 Peter Stokes <[email protected]>
+ * Copyright (C) 2007 Peter Stokes <[email protected]>

Quoted 2 lines

2008?

The primary intention behind that change was to merely update my email address. If you feel the year should be updated 2008 then that is fine with me.

Quoted 10 lines

Quoted 10 lines
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2
drivers/input/misc/ati_remote2.c +1 -1
@@ -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"
 

Quoted 15 lines

Quoted 11 lines
drivers/input/misc/ati_remote2.c +0 -0
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_VERSION(DRIVER_VERSION);
@@ -27,7 +27,7 @@
  * 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".
*/
Unrelated change. I saw a couple of other unrelated changes too (just changing the whitespace) further down.

I was just updating my original poor English. I felt the clarification was worthwhile but insufficiently important to submit a patch just to change a comment.

Quoted 4 lines

Quoted 4 lines
drivers/input/misc/ati_remote2.c +0 -0
@@ -45,61 +45,66 @@
 };
 MODULE_DEVICE_TABLE(usb, ati_remote2_id_table);
 

Quoted 12 lines

drivers/input/misc/ati_remote2.c line 55
Quoted 8 lines
drivers/input/misc/ati_remote2.c +8 -0
+
+static u8 ati_remote2_modes[] = {
+	0x01, /* AUX1 */
+	0x02, /* AUX2 */
+	0x04, /* AUX3 */
+	0x08, /* AUX4 */
+	0x10, /* PC   */
+};
It seems you only use this table to get the number of modes. A define would suffice.

That is indeed correct. This table is present in order to provide clear documentation of the mapping layout at the top of the file.

Quoted 54 lines

drivers/input/misc/ati_remote2.c +0 ~~-53~~

 static struct {
-	int hw_code;
-	int key_code;
-} ati_remote2_key_table[] = {
-	{ 0x00, KEY_0 },
-	{ 0x01, KEY_1 },
-	{ 0x02, KEY_2 },
-	{ 0x03, KEY_3 },
-	{ 0x04, KEY_4 },
-	{ 0x05, KEY_5 },
-	{ 0x06, KEY_6 },
-	{ 0x07, KEY_7 },
-	{ 0x08, KEY_8 },
-	{ 0x09, KEY_9 },
-	{ 0x0c, KEY_POWER },
-	{ 0x0d, KEY_MUTE },
-	{ 0x10, KEY_VOLUMEUP },
-	{ 0x11, KEY_VOLUMEDOWN },
-	{ 0x20, KEY_CHANNELUP },
-	{ 0x21, KEY_CHANNELDOWN },
-	{ 0x28, KEY_FORWARD },
-	{ 0x29, KEY_REWIND },
-	{ 0x2c, KEY_PLAY },
-	{ 0x30, KEY_PAUSE },
-	{ 0x31, KEY_STOP },
-	{ 0x37, KEY_RECORD },
-	{ 0x38, KEY_DVD },
-	{ 0x39, KEY_TV },
-	{ 0x54, KEY_MENU },
-	{ 0x58, KEY_UP },
-	{ 0x59, KEY_DOWN },
-	{ 0x5a, KEY_LEFT },
-	{ 0x5b, KEY_RIGHT },
-	{ 0x5c, KEY_OK },
-	{ 0x78, KEY_A },
-	{ 0x79, KEY_B },
-	{ 0x7a, KEY_C },
-	{ 0x7b, KEY_D },
-	{ 0x7c, KEY_E },
-	{ 0x7d, KEY_F },
-	{ 0x82, KEY_ENTER },
-	{ 0x8e, KEY_VENDOR },
-	{ 0x96, KEY_COFFEE },
-	{ 0xa9, BTN_LEFT },
-	{ 0xaa, BTN_RIGHT },
-	{ 0xbe, KEY_QUESTION },
-	{ 0xd5, KEY_FRONT },
-	{ 0xd0, KEY_EDIT },
-	{ 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 },

Quoted 9 lines

drivers/input/misc/ati_remote2.c
Quoted 6 lines
drivers/input/misc/ati_remote2.c +4 -1
-	{ 0, KEY_RESERVED }
+	u8  hwcode;
+	u16 keycode[ARRAY_SIZE(ati_remote2_modes)];
+} ati_remote2_keycodes[] = {
+/*	 hwcode   AUX1             AUX2             AUX3             AUX4    
        PC                */
checkpatch.pl doesn't like these long lines.

To limit the line length for these lines seemed unwise to me. Especially in light of their primary purpose being to provide clear documentation of the mapping implementation.

My reasoning with both these tables was two fold. Firstly, canvassing other files within the input system indicated that there are a number of 'large' tables. Secondly, this is a driver for a remote control. I am envisaging most people will be using this device on a relatively high specification PC and not on an embedded device. Accordingly I concluded the memory footprints were affordable.

Quoted 56 lines

drivers/input/misc/ati_remote2.c line 127
Quoted 8 lines
drivers/input/misc/ati_remote2.c +2 -0
 struct ati_remote2 {
@@ -117,6 +122,8 @@
 
 	char name[64];
 	char phys[64];
+
+	u32
 keycode[ARRAY_SIZE(ati_remote2_keycodes)][ARRAY_SIZE(ati_remote2_modes)];
Somehow it would seem more natural to me if you would swap the array dimensions. But that's a minor issue and you can leave it like this if you prefer.
Quoted 27 lines
(quoted diff) +0 -0
 };
 
static int ati_remote2_probe(struct usb_interface *interface, const
struct usb_device_id *id); @@ -159,11 +166,84 @@
drivers/input/misc/ati_remote2.c +20 -0
 	usb_kill_urb(ar2->urb[1]);
 }
 
+static int ati_remote2_lookup(u8 hwcode)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ati_remote2_keycodes); i++)
+		if (ati_remote2_keycodes[i].hwcode == hwcode)
+			return i;
+
+	return -1;
+}
+
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+
drivers/input/misc/ati_remote2.c line 198
Quoted 10 lines
drivers/input/misc/ati_remote2.c +10 -0
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			*keycode = ar2->keycode[index][mode];
+			return 0;
+		}
+	}
Is there a reason you using the high bits like a mask? IIRC the hardware mode byte isn't a mask. So something like this should do:
mode = scancode >> 8;
if (mode < 0 || mode > MAX_MODE)

Quoted 9 lines

	return -EINVAL;
index = ati_remote2_lookup(scancode & 0xff);
if (index < 0)
	return -EINVAL;
I'm not sure if mode_mask should affect the keymap handling at all since both can be changed runtime.

The reason I am using the high bits as a mode mask is in order to address the issues I felt some people might have regarding the interpretation of the device that allows mapping separate keycodes to each physical button dependant upon which mode the remote control is currently in. This approach address two issues.

Firstly, whilst the input system provides a mechanism to configure the scancode to keycode mapping on a per-device basis (each device can have it's own mapping table) unfortunately the mechanism by which the mapping tables can be manipulated (via the 'setkeycodes' utility) does not allow for the device that should be modified to be specified. What actually appears to happen is that the first device that accepts the specified scancode as being one it can produce changes it's mapping for that scancode to use the new keycode provided. The practical upshot of this is that I wish to be able to remap the keycodes associated with the remote control but if the scancodes the remote control reports are within the normal range associated with a regular keyboard (i.e. <=255) then the keyboard driver accepts the change first. This not only results in the ati_remote2 not even seeing the request for the new mapping it also has the undesirable side effect of corrupting the mappings for the regular keyboard. To overcome this problem, I decided that instead of using the mode byte as is (i.e. a number between 0 and 4) I would convert it into a bit mask, thus moving all the remote's reported scancodes (0x0100 -> 0x1FF9) outside to the regular keyboard range.

Secondly. whilst I decided to provide the flexibility of a mechanism by which a single key may be mapped to different keycodes dependant upon which mode the remote is currently in, I expected that not everyone would need that functionality. A potential downside to providing the flexible implementation could be that if a user simply wanted to change the mapping for a given physical button regardless of what mode the remote is currently in then it might be necessary to submit five requests to update the mappings for each mode. By converting the mode byte into a mode bit mask it allows a mapping request to simultaneously update the mapping for a single physical button for all modes. So, for example:

setkeycodes 0x015C 28 - Would map the OK button for AUX1 mode to be 'enter'
setkeycodes 0x1F5C 28 - Would map the OK button for all modes to be 'enter'

The usage of the mode_mask module parameter in the 'ati_remote2_getkeycode' and 'ati_remote2_setkeycode' is only used determine whether the driver, as currently configured, will respond to the specified scancode. If the driver will not currently respond, because a given mode is masked out, then it is not possible to request or modify the keymapping for a physical button in that mode.

Quoted 17 lines

Quoted 4 lines

drivers/input/misc/ati_remote2.c +4 -0
+
+	return -EINVAL;
+}
+

drivers/input/misc/ati_remote2.c line 212

Quoted 10 lines

drivers/input/misc/ati_remote2.c +10 -0

+static int ati_remote2_setkeycode(struct input_dev *idev,
+				  int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int old_keycode = -1;
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+

Same comments as above.

Quoted 8 lines

drivers/input/misc/ati_remote2.c line 217
Quoted 5 lines
drivers/input/misc/ati_remote2.c +5 -0
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	if (keycode < 0 || keycode > KEY_MAX)
Are KEY_RESERVED/KEY_MAX valid here?

Yes, this is merely checking that the keycode provided is within the range expected by the linux input system.

Quoted 12 lines

drivers/input/misc/ati_remote2.c +12 -0

+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			old_keycode = ar2->keycode[index][mode];
+			ar2->keycode[index][mode] = keycode;
+		}
+	}
+
+	set_bit(keycode, idev->keybit);
+
+	for (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {

Quoted 9 lines

drivers/input/misc/ati_remote2.c line 234
Quoted 5 lines
drivers/input/misc/ati_remote2.c +5 -0
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			if (ar2->keycode[index][mode] == old_keycode)
+				return 0;
+		}
+	}
Because of using the scancode high bits as a mask this could leave some bits set even though they are no longer in the map.

No, in this function the 'mode' variable is an index into the ar2->keycode table (i.e. a number between 0 and 4). The implementation is attempting to ascertain whether or not the 'old_keycode' is still in use within the key table. The implementation does not filter the contents of idev->keybit based upon whether a given keycode is only used by the remote in modes that are currently masked off. Is that what you meant?

Quoted 66 lines

drivers/input/misc/ati_remote2.c +14 ~~-21~~

+
+	clear_bit(old_keycode, idev->keybit);
+
+	return 0;
+}
+
+
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[0];
-	int channel, mode;
+	u8 channel, mode;
 
 	channel = data[0] >> 4;

@@ -187,22 +267,12 @@
 	input_sync(idev);
 }
 
-static int ati_remote2_lookup(unsigned int hw_code)
-{
-	int i;
-
-	for (i = 0; ati_remote2_key_table[i].key_code != KEY_RESERVED; i++)
-		if (ati_remote2_key_table[i].hw_code == hw_code)
-			return i;
-
-	return -1;
-}
-
 static void ati_remote2_input_key(struct ati_remote2 *ar2)
 {
 	struct input_dev *idev = ar2->idev;
 	u8 *data = ar2->buf[1];
-	int channel, mode, hw_code, index;
+	u8 channel, mode;
+	int index;
 
 	channel = data[0] >> 4;

@@ -218,12 +288,10 @@
 		return;
 	}
 
-	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) {
+	if (!((1 << mode) & mode_mask))
+		return;
+
+	if (data[2] == 0x3f) {
 		/*
 		 * For some incomprehensible reason the mouse pad generates
 		 * events which look identical to the events from the last

@@ -236,14 +304,9 @@
 
 		if (data[1] == 0)
 			ar2->mode = mode;
-
-		hw_code |= mode << 8;
 	}

Quoted 8 lines

drivers/input/misc/ati_remote2.c line 244
Quoted 2 lines
drivers/input/misc/ati_remote2.c +0 -2
-	if (!((1 << mode) & mode_mask))
-		return;
Moving this test before updating ar2->mode could leave the driver unaware of the actual mode it's in. I think it could leave to key events getting through from the mouse at least when mode_mask is suitably changed runtime. Was there a reason for this change?

Will undo.

Quoted 74 lines

drivers/input/misc/ati_remote2.c +4 -6

-
-	index = ati_remote2_lookup(hw_code);
+	index = ati_remote2_lookup(data[2]);
 	if (index < 0) {
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown code byte (%02x %02x %02x %02x)\n",

@@ -260,8 +323,7 @@
 	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 (data[2] == 0xa9 || data[2] == 0xaa)
 			return;
 
 		if (!time_after_eq(jiffies, ar2->jiffies))

@@ -276,7 +338,7 @@
 		return;
 	}
 
-	input_event(idev, EV_KEY, ati_remote2_key_table[index].key_code,
 data[1]); +	input_event(idev, EV_KEY, ar2->keycode[index][mode],
 data[1]); input_sync(idev);
 }

@@ -334,10 +396,11 @@
 			"%s(): usb_submit_urb() = %d\n", __FUNCTION__, r);
 }
 
+
 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)

@@ -347,11 +410,15 @@
 	input_set_drvdata(idev, ar2);
 
 	idev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |

BIT_MASK(EV_REL); - idev->keybit[BIT_WORD(BTN_MOUSE)] =
BIT_MASK(BTN_LEFT) |

(quoted diff) +1 -1

-		BIT_MASK(BTN_RIGHT);
+	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);

drivers/input/misc/ati_remote2.c +4 -2

-	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 (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			ar2->keycode[index][mode] =

ati_remote2_keycodes[index].keycode[mode];

drivers/input/misc/ati_remote2.c +6 -0

+			set_bit(ar2->keycode[index][mode], idev->keybit);
+		}
+	}
 
 	idev->rep[REP_DELAY]  = 250;
 	idev->rep[REP_PERIOD] = 33;

@@ -359,6 +426,9 @@
 	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;

@@ -532,6 +602,9 @@
 	else
 		printk(KERN_INFO "ati_remote2: " DRIVER_DESC " " DRIVER_VERSION "\n");

Quoted 8 lines

drivers/input/misc/ati_remote2.c line 607
Quoted 3 lines
drivers/input/misc/ati_remote2.c +3 -0
+	mode_mask &= 0x1F;
+	channel_mask &= 0xFFFF;
+
As I said these can be changed runtime, so masking them should happen every time they are used. A nicer solution would be to actually reject invalid values.

Will add masking where they are used instead.

Quoted 2 lines

drivers/input/misc/ati_remote2.c line 609
Quoted 2 lines
drivers/input/misc/ati_remote2.c +0 -0
 	return r;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ville Syrjälä

maintainer

2008-06-03 18:45 UTC #5 lore raw

Quoted 6 lines · Peter Stokes

Ville,
My apologies for not responding to your feedback sooner. I had not noticed the comments that you had inserted within the patch. Please find my responses below:

Sorry it took so long to respond. I'll just throw some comments here but I'll also post a modifed patch that I cooked up.

Quoted 52 lines · On Tuesday 08 April 2008 21:18:26 Ville Syrjälä wrote:

Quoted 26 lines · Peter Stokes
Quoted 9 lines
drivers/input/misc/ati_remote2.c +9 -0
+static int ati_remote2_getkeycode(struct input_dev *idev,
+				  int scancode, int *keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+
drivers/input/misc/ati_remote2.c line 198
Quoted 10 lines
drivers/input/misc/ati_remote2.c +10 -0
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			*keycode = ar2->keycode[index][mode];
+			return 0;
+		}
+	}
Is there a reason you using the high bits like a mask? IIRC the hardware mode byte isn't a mask. So something like this should do:
mode = scancode >> 8;
if (mode < 0 || mode > MAX_MODE)
Quoted 9 lines
	return -EINVAL;
index = ati_remote2_lookup(scancode & 0xff);
if (index < 0)
	return -EINVAL;
I'm not sure if mode_mask should affect the keymap handling at all since both can be changed runtime.
The reason I am using the high bits as a mode mask is in order to address the issues I felt some people might have regarding the interpretation of the device that allows mapping separate keycodes to each physical button dependant upon which mode the remote control is currently in. This approach address two issues.
Firstly, whilst the input system provides a mechanism to configure the scancode to keycode mapping on a per-device basis (each device can have it's own mapping table) unfortunately the mechanism by which the mapping tables can be manipulated (via the 'setkeycodes' utility) does not allow for the device that should be modified to be specified. What actually appears to happen is that the first device that accepts the specified scancode as being one it can produce changes it's mapping for that scancode to use the new keycode provided. The practical upshot of this is that I wish to be able to remap the keycodes associated with the remote control but if the scancodes

Quoted 8 lines

the remote control reports are within the normal range associated with a regular keyboard (i.e. <=255) then the keyboard driver accepts the change first. This not only results in the ati_remote2 not even seeing the request for the new mapping it also has the undesirable side effect of corrupting the mappings for the regular keyboard. To overcome this problem, I decided that instead of using the mode byte as is (i.e. a number between 0 and 4) I would convert it into a bit mask, thus moving all the remote's reported scancodes (0x0100 -> 0x1FF9) outside to the regular keyboard range.

That just tells me that setkeycodes is the wrong tool for this job. Trying to work around bugs/misfeatures in a simple tool like that inside the kernel is no good. Writing a small tool that directly uses the event device (or patching setkeycodes) is trivial enough.

Quoted 14 lines

Secondly. whilst I decided to provide the flexibility of a mechanism by which a single key may be mapped to different keycodes dependant upon which mode the remote is currently in, I expected that not everyone would need that functionality. A potential downside to providing the flexible implementation could be that if a user simply wanted to change the mapping for a given physical button regardless of what mode the remote is currently in then it might be necessary to submit five requests to update the mappings for each mode. By converting the mode byte into a mode bit mask it allows a mapping request to simultaneously update the mapping for a single physical button for all modes. So, for example:
setkeycodes 0x015C 28 - Would map the OK button for AUX1 mode to be 'enter'
setkeycodes 0x1F5C 28 - Would map the OK button for all modes to be 'enter'

Quoted 6 lines

The usage of the mode_mask module parameter in the 'ati_remote2_getkeycode' and 'ati_remote2_setkeycode' is only used determine whether the driver, as currently configured, will respond to the specified scancode. If the driver will not currently respond, because a given mode is masked out, then it is not possible to request or modify the keymapping for a physical button in that mode.

I understand but I still think it's the wrong thing to do. The API is documented to do 1:1 mapping and you're making it do something else. Also getkeycode can't really handle a mask since you'd need to return multiple keycodes for one scancode which would make API strangely assymmetric (and not to mention confusing for unwary users).

Quoted 26 lines

Quoted 17 lines
Quoted 4 lines
drivers/input/misc/ati_remote2.c +4 -0
+
+	return -EINVAL;
+}
+
drivers/input/misc/ati_remote2.c line 212
Quoted 10 lines
drivers/input/misc/ati_remote2.c +10 -0
+static int ati_remote2_setkeycode(struct input_dev *idev,
+				  int scancode, int keycode)
+{
+	struct ati_remote2 *ar2 = input_get_drvdata(idev);
+	int old_keycode = -1;
+	int index, mode;
+
+	if (((scancode >> 8) & mode_mask) != (scancode >> 8))
+		return -EINVAL;
+
Same comments as above.
Quoted 8 lines
drivers/input/misc/ati_remote2.c line 217
Quoted 5 lines
drivers/input/misc/ati_remote2.c +5 -0
+	index = ati_remote2_lookup(scancode & 0xFF);
+	if (index == -1)
+		return -EINVAL;
+
+	if (keycode < 0 || keycode > KEY_MAX)
Are KEY_RESERVED/KEY_MAX valid here?

Quoted 2 lines

Yes, this is merely checking that the keycode provided is within the range expected by the linux input system.

I meant that it now accepts KEY_RESERVED and KEY_MAX. Are those supposed to be valid keycodes? Hmm, I suppose KEY_MAX at least is supposed to valid judging from REP_MAX and FF_STATUS_MAX.

Quoted 22 lines

Quoted 12 lines
Quoted 12 lines
drivers/input/misc/ati_remote2.c +12 -0
+		return -EINVAL;
+
+	for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+		if ((1 << mode) & (scancode >> 8)) {
+			old_keycode = ar2->keycode[index][mode];
+			ar2->keycode[index][mode] = keycode;
+		}
+	}
+
+	set_bit(keycode, idev->keybit);
+
+	for (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
Quoted 9 lines
drivers/input/misc/ati_remote2.c line 234
Quoted 5 lines
drivers/input/misc/ati_remote2.c +5 -0
+		for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
+			if (ar2->keycode[index][mode] == old_keycode)
+				return 0;
+		}
+	}
Because of using the scancode high bits as a mask this could leave some bits set even though they are no longer in the map.

Quoted 6 lines

No, in this function the 'mode' variable is an index into the ar2->keycode table (i.e. a number between 0 and 4). The implementation is attempting to ascertain whether or not the 'old_keycode' is still in use within the key table. The implementation does not filter the contents of idev->keybit based upon whether a given keycode is only used by the remote in modes that are currently masked off. Is that what you meant?

No. I meant that scancode is a mask so you could end up updating several keys in the keycode table but you clear the keybit only for the last keycode to be overwritten. One or more of the other overwritten keycodes could have also disappeared from the keycode table but still be present in keybit.

Example:
Let's say you have KEY_FOO and KEY_BAR mapped to the same button in modes AUX1 and AUX2. No other key is mapped to KEY_FOO or KEY_BAR. You then call setkeycode with the mask set to AUX1|AUX2 and KEY_NEW. The code would basically do this:

old_keycode = KEY_FOO;
keycode[button][AUX1] = KEY_NEW;
old_keycode = KEY_BAR;
keycode[button][AUX2] = KEY_NEW;
set_bit(KEY_NEW, keybit);
clear_bit(KEY_BAR, keybit);

In the end KEY_FOO would still be present in keybit even though it was overwritten in the keycode table.

But anyways I got rid of this mask feature in my modifed patch.

Signature · 7 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html