Re: [PATCH] ati_remote2 loadable keymap support

["Ville Syrjälä" <syrjala@sci.fi>]

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