* [PATCH] ati_remote2 loadable keymap support
@ 2008-04-05 18:30 Peter Stokes
2008-04-08 20:18 ` Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Peter Stokes @ 2008-04-05 18:30 UTC (permalink / raw)
To: linux-input; +Cc: Ville Syrjälä, Jiri Kosina, dmitry.torokhov
[-- Attachment #1: Type: text/plain, Size: 2943 bytes --]
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.
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.
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
[-- Attachment #2: ati_remote2_keycode.patch --]
[-- Type: text/x-diff, Size: 13089 bytes --]
Signed-off-by: Peter Stokes <linux@dadeos.co.uk>
--- 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 <syrjala@sci.fi>
- * Copyright (C) 2007 Peter Stokes <linux@dadeos.freeserve.co.uk>
+ * Copyright (C) 2007 Peter Stokes <linux@dadeos.co.uk>
*
* 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;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ati_remote2 loadable keymap support
2008-04-05 18:30 [PATCH] ati_remote2 loadable keymap support Peter Stokes
@ 2008-04-08 20:18 ` Ville Syrjälä
2008-04-17 19:00 ` Dmitry Torokhov
2008-04-18 19:42 ` Peter Stokes
0 siblings, 2 replies; 6+ messages in thread
From: Ville Syrjälä @ 2008-04-08 20:18 UTC (permalink / raw)
To: Peter Stokes; +Cc: linux-input, Jiri Kosina, dmitry.torokhov
On Sat, Apr 05, 2008 at 07:30:10PM +0100, Peter Stokes wrote:
> 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.
>
> 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.
>
> 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.
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 :)
> 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.
> 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 <linux@dadeos.co.uk>
>
>
> --- 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 <syrjala@sci.fi>
> - * Copyright (C) 2007 Peter Stokes <linux@dadeos.freeserve.co.uk>
> + * Copyright (C) 2007 Peter Stokes <linux@dadeos.co.uk>
2008?
> *
> * 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".
> */
Unrelated change. I saw a couple of other unrelated changes too (just
changing the whitespace) further down.
>
> @@ -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.
> 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 */
checkpatch.pl doesn't like these long lines.
> 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.
> };
>
> 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;
> + }
> + }
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.
> +
> + 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;
> +
Same comments as above.
> + index = ati_remote2_lookup(scancode & 0xFF);
> + if (index == -1)
> + return -EINVAL;
> +
> + if (keycode < 0 || keycode > KEY_MAX)
Are KEY_RESERVED/KEY_MAX valid here?
> + 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;
> + }
> + }
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.
> +
> + 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;
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?
> -
> - 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;
> +
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.
> return r;
> }
>
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ati_remote2 loadable keymap support
2008-04-08 20:18 ` Ville Syrjälä
@ 2008-04-17 19:00 ` Dmitry Torokhov
2008-04-18 20:49 ` Peter Stokes
2008-04-18 19:42 ` Peter Stokes
1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2008-04-17 19:00 UTC (permalink / raw)
To: Ville Syrj?l?; +Cc: Peter Stokes, linux-input, Jiri Kosina
On Tue, Apr 08, 2008 at 11:18:26PM +0300, Ville Syrj?l? wrote:
> On Sat, Apr 05, 2008 at 07:30:10PM +0100, Peter Stokes wrote:
> > + 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?
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ati_remote2 loadable keymap support
2008-04-17 19:00 ` Dmitry Torokhov
@ 2008-04-18 20:49 ` Peter Stokes
0 siblings, 0 replies; 6+ messages in thread
From: Peter Stokes @ 2008-04-18 20:49 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Ville Syrj?l?, linux-input, Jiri Kosina
[-- Attachment #1: Type: text/plain, Size: 715 bytes --]
On Thursday 17 April 2008 20:00:23 Dmitry Torokhov wrote:
> On Tue, Apr 08, 2008 at 11:18:26PM +0300, Ville Syrj?l? wrote:
> > On Sat, Apr 05, 2008 at 07:30:10PM +0100, Peter Stokes wrote:
> > > + 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?
Please find attached an updated patch. Sorry for the delay.
Best regards
Peter
[-- Attachment #2: ati_remote2_keycode.patch --]
[-- Type: text/x-diff, Size: 12891 bytes --]
Signed-off-by: Peter Stokes <linux@dadeos.co.uk>
--- 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-18 20:18:32.000000000 +0100
@@ -2,7 +2,7 @@
* ati_remote2 - ATI/Philips USB RF remote driver
*
* Copyright (C) 2005 Ville Syrjala <syrjala@sci.fi>
- * Copyright (C) 2007 Peter Stokes <linux@dadeos.freeserve.co.uk>
+ * Copyright (C) 2008 Peter Stokes <linux@dadeos.co.uk>
*
* 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ati_remote2 loadable keymap support
2008-04-08 20:18 ` Ville Syrjälä
2008-04-17 19:00 ` Dmitry Torokhov
@ 2008-04-18 19:42 ` Peter Stokes
2008-06-03 18:45 ` Ville Syrjälä
1 sibling, 1 reply; 6+ messages in thread
From: Peter Stokes @ 2008-04-18 19:42 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: linux-input, Jiri Kosina, dmitry.torokhov
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:
On Tuesday 08 April 2008 21:18:26 Ville Syrjälä wrote:
> On Sat, Apr 05, 2008 at 07:30:10PM +0100, Peter Stokes wrote:
> > 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.
> >
> > 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.
> >
> > 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.
>
> 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 :)
>
> > 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.
>
> > 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 <linux@dadeos.co.uk>
> >
> >
> > --- 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 <syrjala@sci.fi>
> > - * Copyright (C) 2007 Peter Stokes <linux@dadeos.freeserve.co.uk>
> > + * Copyright (C) 2007 Peter Stokes <linux@dadeos.co.uk>
>
> 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.
> > *
> > * 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".
> > */
>
> 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.
> > @@ -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.
>
That is indeed correct. This table is present in order to provide clear
documentation of the mapping layout at the top of the file.
> > 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 */
>
> 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.
> > 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.
>
> > };
> >
> > 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;
> > + }
> > + }
>
> 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.
>
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.
> > +
> > + 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;
> > +
>
> Same comments as above.
>
> > + 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.
> > + 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;
> > + }
> > + }
>
> 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?
> > +
> > + 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;
>
> 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.
> > -
> > - 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;
> > +
>
> 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.
> > return r;
> > }
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ati_remote2 loadable keymap support
2008-04-18 19:42 ` Peter Stokes
@ 2008-06-03 18:45 ` Ville Syrjälä
0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2008-06-03 18:45 UTC (permalink / raw)
To: Peter Stokes; +Cc: linux-input, Jiri Kosina, dmitry.torokhov
On Fri, Apr 18, 2008 at 08:42:29PM +0100, Peter Stokes wrote:
>
> 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.
> On Tuesday 08 April 2008 21:18:26 Ville Syrjälä wrote:
> > On Sat, Apr 05, 2008 at 07:30:10PM +0100, Peter Stokes wrote:
> > > +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;
> > > + }
> > > + }
> >
> > 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.
> >
>
> 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.
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.
> 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.
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).
> > > +
> > > + 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;
> > > +
> >
> > Same comments as above.
> >
> > > + 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.
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.
> > > + 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;
> > > + }
> > > + }
> >
> > 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?
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.
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-06-03 18:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-04-05 18:30 [PATCH] ati_remote2 loadable keymap support Peter Stokes
2008-04-08 20:18 ` Ville Syrjälä
2008-04-17 19:00 ` Dmitry Torokhov
2008-04-18 20:49 ` Peter Stokes
2008-04-18 19:42 ` Peter Stokes
2008-06-03 18:45 ` Ville Syrjälä
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).