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@xxxxxxxxxxxx>
>
>
> --- 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@xxxxxx>
> - * Copyright (C) 2007 Peter Stokes <linux@xxxxxxxxxxxxxxxxxxxxxx>
> + * Copyright (C) 2007 Peter Stokes <linux@xxxxxxxxxxxx>
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@xxxxxx
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
