[PATCH] ati_remote2 autorepeat and loadable keymap support

[PATCH] ati_remote2 autorepeat and loadable keymap support

[Peter Stokes]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

The attached patch reconfigures the ati_remote2 driver to use soft-autorepeat 
functionality and adds support for loadable key maps.

I have reconfigure the driver to use the input system's built-in autorepeat 
functionality as the device only appears to be able to produce key repeat 
notifications at a fixed period. Switching to the software autorepeat 
functionality provides more precise configuration of the timings requested 
for repeat-delay and repeat-rate.

As this device is exposed as a combined keyboard and mouse, this change 
somewhat depends upon the suggested modification to the core soft-autorepeat 
functionality as outlined in my previous post to the linux-input mailing list 
(on 12th Feb 2008 entitled "Soft-autorepeat functionality"), without that 
modification, the mouse buttons are autorepeated :-(

The loadable keymap support exposes the ability to map 5 separate keycodes to 
each key (depending on which "mode" the remote control is currently in). 
Additionally, I have attempted to ensure that the scancodes used to map 
keycodes to the keys lie outside of the range normally covered by regular 
keyboards so as to avoid requests to remap the keys on the remote from being 
intercepted by a normal keyboard.

Any feedback would be gratefully received.

Thanks in advance.

Peter Stokes

[-- Attachment #2: ati_remote2_keycode.patch --]
[-- Type: text/x-diff, Size: 13479 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-02-16 15:16:08.000000000 +0000
@@ -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 {
@@ -112,11 +117,12 @@
 	void *buf[2];
 	dma_addr_t buf_dma[2];
 
-	unsigned long jiffies;
 	int mode;
 
 	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 +165,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 +266,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 +287,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 +303,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",
@@ -255,20 +317,9 @@
 	case 0:	/* release */
 		break;
 	case 1:	/* press */
-		ar2->jiffies = jiffies + msecs_to_jiffies(idev->rep[REP_DELAY]);
 		break;
 	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)
-			return;
-
-		if (!time_after_eq(jiffies, ar2->jiffies))
-			return;
-
-		ar2->jiffies = jiffies + msecs_to_jiffies(idev->rep[REP_PERIOD]);
-		break;
+		return; /* Autorepeat handled by input module */
 	default:
 		dev_err(&ar2->intf[1]->dev,
 			"Unknown state byte (%02x %02x %02x %02x)\n",
@@ -276,7 +327,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 +385,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,18 +399,22 @@
 	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);
 
-	idev->rep[REP_DELAY]  = 250;
-	idev->rep[REP_PERIOD] = 33;
+	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->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 +588,9 @@
 	else
 		printk(KERN_INFO "ati_remote2: " DRIVER_DESC " " DRIVER_VERSION "\n");
 
+	mode_mask &= 0x1F;
+	channel_mask &= 0xFFFF;
+
 	return r;
 }
 

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

On Sat, 16 Feb 2008, Peter Stokes wrote:

> As this device is exposed as a combined keyboard and mouse, this change 
> somewhat depends upon the suggested modification to the core 
> soft-autorepeat functionality as outlined in my previous post to the 
> linux-input mailing list (on 12th Feb 2008 entitled "Soft-autorepeat 
> functionality"), without that modification, the mouse buttons are 
> autorepeated :-(

I don't think that the solution you proposed in that mail, i.e.

--- linux-2.6.24-orig/drivers/input/input.c	2008-01-24 22:58:37.000000000 +0000
+++ linux-2.6.24/drivers/input/input.c	2008-02-11 20:02:03.000000000 +0000
@@ -123,6 +123,7 @@
 static void input_start_autorepeat(struct input_dev *dev, int code)
 {
 	if (test_bit(EV_REP, dev->evbit) &&
+	    code < KEY_MIN_INTERESTING &&
 	    dev->rep[REP_PERIOD] && dev->rep[REP_DELAY] &&
 	    dev->timer.data) {
 		dev->repeat_key = code;

should be used. We really can't generally assume that there is no key 
above KEY_MIN_INTERESTING that shouldn't be autorepeated. The 
KEY_MIN_INTERESTING is, if I remember correctly, just a hack that 
modaliases don't grow excessively.

What about changing the 
ati_remote2-autorepeat-and-loadable-keymap-support.patch (as it is now in 
-mm) so that it registers two independent input devices, one for keyboard 
and one for mouse, and depedning on the type of arriving event, you decide 
which input device to use?

You can then easily set autorepeat only for one of them, right?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Sat, Feb 16, 2008 at 04:22:43PM +0000, Peter Stokes wrote:
> The attached patch reconfigures the ati_remote2 driver to use soft-autorepeat 
> functionality and adds support for loadable key maps.

Why was this submitted (and even accepted) without cc:ing me?

> I have reconfigure the driver to use the input system's built-in autorepeat 
> functionality as the device only appears to be able to produce key repeat 
> notifications at a fixed period. Switching to the software autorepeat 
> functionality provides more precise configuration of the timings requested 
> for repeat-delay and repeat-rate.

The soft-autorepeat support should be split into a separate patch. I don't
need such fast repeat but if it helps people I'm fine with it.

> As this device is exposed as a combined keyboard and mouse, this change 
> somewhat depends upon the suggested modification to the core soft-autorepeat 
> functionality as outlined in my previous post to the linux-input mailing list 
> (on 12th Feb 2008 entitled "Soft-autorepeat functionality"), without that 
> modification, the mouse buttons are autorepeated :-(
> 
> The loadable keymap support exposes the ability to map 5 separate keycodes to 
> each key (depending on which "mode" the remote control is currently in). 
> Additionally, I have attempted to ensure that the scancodes used to map 
> keycodes to the keys lie outside of the range normally covered by regular 
> keyboards so as to avoid requests to remap the keys on the remote from being 
> intercepted by a normal keyboard.

I thought the idea of input devices was to reflect the hardware and the
keymaps should be handled in userspace. If that's not the case then I think
the keymap support code should not be inside the driver but instead inside
the input core. We don't want such invasive changes in every driver do
we?

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 807 bytes --]

On Tue, 4 Mar 2008, Ville Syrjälä wrote:

> > The attached patch reconfigures the ati_remote2 driver to use soft-autorepeat 
> > functionality and adds support for loadable key maps.
> Why was this submitted (and even accepted) without cc:ing me?

I don't seem to see you in MAINTAINERS entry, so that's probably why you 
didn't receive confirmation when Andrew has put this patch into -mm.

Merging into -mm doesn't mean that the patch is accepted at all, it just 
exposes it for wider audience for feedback and testing.


With respect to autorepeat -- I think that the easiest solution would be 
registering two separate input devices, one for keyboard and one for 
mouse, and perform the switching in the driver, depending on the type of 
received events.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 02:20:19PM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Ville Syrjälä wrote:
> 
> > > The attached patch reconfigures the ati_remote2 driver to use soft-autorepeat 
> > > functionality and adds support for loadable key maps.
> > Why was this submitted (and even accepted) without cc:ing me?
> 
> I don't seem to see you in MAINTAINERS entry, so that's probably why you 
> didn't receive confirmation when Andrew has put this patch into -mm.

I guess MODULE_AUTHOR() isn't enough then. I'll send a patch for
MAINTAINERS.

> Merging into -mm doesn't mean that the patch is accepted at all, it just 
> exposes it for wider audience for feedback and testing.
> 
> 
> With respect to autorepeat -- I think that the easiest solution would be 
> registering two separate input devices, one for keyboard and one for 
> mouse, and perform the switching in the driver, depending on the type of 
> received events.

Hmm. IIRC I had one USB mouse which also sent repeat events and the
driver didn't seem to filter them out which is why I added checks to
DirectFB's input driver to filter them in user space. Was that a bug
in the usbhid driver or does it depend on the actual hardware? I still
have that mouse so I can re-test the behavior if needed. I guess the
only issue with registering two input devices whould be that user space
might need some way to tell that they are really part of the same
remote.

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1201 bytes --]

On Tue, 4 Mar 2008, Ville Syrjälä wrote:

> > I don't seem to see you in MAINTAINERS entry, so that's probably why you 
> > didn't receive confirmation when Andrew has put this patch into -mm.
> I guess MODULE_AUTHOR() isn't enough then. I'll send a patch for
> MAINTAINERS.

I don't know the exact algorithm Andrew uses to send out CCs when patch is 
merged, but MAINTAINERS entry would definitely help.

> Hmm. IIRC I had one USB mouse which also sent repeat events and the 
> driver didn't seem to filter them out which is why I added checks to 
> DirectFB's input driver to filter them in user space. Was that a bug in 
> the usbhid driver or does it depend on the actual hardware? I still have 
> that mouse so I can re-test the behavior if needed. 

Button events from mice definitely shouldn't be auto-repeated, if they are 
reported probably by the hardware (i.e. HID_UP_BUTTON usage page, 
MOUSE/POINTER application).

If you could recompile your kernel with CONFIG_HID_DEBUG, modprobe the 
'hid' module with 'debug=1' parameter, and send me the output that appears 
in kernel log when you press (and hold) the button, I will look into it.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 03:01:44PM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Ville Syrjälä wrote:
> 
> > > I don't seem to see you in MAINTAINERS entry, so that's probably why you 
> > > didn't receive confirmation when Andrew has put this patch into -mm.
> > I guess MODULE_AUTHOR() isn't enough then. I'll send a patch for
> > MAINTAINERS.
> 
> I don't know the exact algorithm Andrew uses to send out CCs when patch is 
> merged, but MAINTAINERS entry would definitely help.
> 
> > Hmm. IIRC I had one USB mouse which also sent repeat events and the 
> > driver didn't seem to filter them out which is why I added checks to 
> > DirectFB's input driver to filter them in user space. Was that a bug in 
> > the usbhid driver or does it depend on the actual hardware? I still have 
> > that mouse so I can re-test the behavior if needed. 
> 
> Button events from mice definitely shouldn't be auto-repeated, if they are 
> reported probably by the hardware (i.e. HID_UP_BUTTON usage page, 
> MOUSE/POINTER application).
> 
> If you could recompile your kernel with CONFIG_HID_DEBUG, modprobe the 
> 'hid' module with 'debug=1' parameter, and send me the output that appears 
> in kernel log when you press (and hold) the button, I will look into it.

I'll do that tonight.

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

On Tue, Mar 04, 2008 at 03:01:44PM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Ville Syrjälä wrote:
> > Hmm. IIRC I had one USB mouse which also sent repeat events and the 
> > driver didn't seem to filter them out which is why I added checks to 
> > DirectFB's input driver to filter them in user space. Was that a bug in 
> > the usbhid driver or does it depend on the actual hardware? I still have 
> > that mouse so I can re-test the behavior if needed. 
> 
> Button events from mice definitely shouldn't be auto-repeated, if they are 
> reported probably by the hardware (i.e. HID_UP_BUTTON usage page, 
> MOUSE/POINTER application).
> 
> If you could recompile your kernel with CONFIG_HID_DEBUG, modprobe the 
> 'hid' module with 'debug=1' parameter, and send me the output that appears 
> in kernel log when you press (and hold) the button, I will look into it.

I've attached the bzipped log.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

[-- Attachment #2: mouse.log.bz2 --]
[-- Type: application/octet-stream, Size: 9604 bytes --]

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2207 bytes --]

On Tue, 4 Mar 2008, Ville Syrjälä wrote:

> I've attached the bzipped log.

Thanks. So between these two events:

Mar  4 20:14:31 [kernel] hid-debug: input Button.0001 = 1
Mar  4 20:14:31 [kernel] hid-debug: input Button.0002 = 0
Mar  4 20:14:31 [kernel] hid-debug: input Button.0003 = 0
Mar  4 20:14:31 [kernel] hid-debug: input Button.0004 = 0
Mar  4 20:14:31 [kernel] hid-debug: input Button.0005 = 0
Mar  4 20:14:31 [kernel] hid-debug: input GenericDesktop.X = 0
Mar  4 20:14:31 [kernel] hid-debug: input GenericDesktop.Y = 0
Mar  4 20:14:31 [kernel] hid-debug: input GenericDesktop.Wheel = 0
Mar  4 20:14:31 [kernel] hid-debug: input Consumer.HorizontalWheel = 0
Mar  4 20:14:32 [kernel] drivers/hid/hid-core.c: report (size 6) (numbered)
Mar  4 20:14:32 [kernel] drivers/hid/hid-core.c: report 17 (size 5) =  01 01 00 00 00
Mar  4 20:14:32 [kernel] hid-debug: input Button.0001 = 1
Mar  4 20:14:32 [kernel] hid-debug: input Button.0002 = 0
Mar  4 20:14:32 [kernel] hid-debug: input Button.0003 = 0
Mar  4 20:14:32 [kernel] hid-debug: input Button.0004 = 0
Mar  4 20:14:32 [kernel] hid-debug: input Button.0005 = 0
Mar  4 20:14:32 [kernel] hid-debug: input GenericDesktop.X = 1
Mar  4 20:14:32 [kernel] hid-debug: input GenericDesktop.Y = 0
Mar  4 20:14:32 [kernel] hid-debug: input GenericDesktop.Wheel = 0
Mar  4 20:14:32 [kernel] hid-debug: input Consumer.HorizontalWheel = 0
Mar  4 20:14:32 [kernel] drivers/hid/hid-core.c: report (size 4) (numbered)
Mar  4 20:14:32 [kernel] drivers/hid/hid-core.c: report 5 (size 3) =  00 0e 00
Mar  4 20:14:32 [kernel] hid-debug: input ff00.ff0b = 0
Mar  4 20:14:32 [kernel] hid-debug: input ff00.ff0d = 14
Mar  4 20:14:32 [kernel] drivers/hid/hid-core.c: report (size 6) (numbered)
Mar  4 20:14:32 [kernel] drivers/hid/hid-core.c: report 17 (size 5) =  01 01 00 00 00
Mar  4 20:14:32 [kernel] hid-debug: input Button.0001 = 1
Mar  4 20:14:32 [kernel] hid-debug: input Button.0002 = 0
Mar  4 20:14:32 [kernel] hid-debug: input Button.0003 = 0
Mar  4 20:14:32 [kernel] hid-debug: input Button.0004 = 0
Mar  4 20:14:32 [kernel] hid-debug: input Button.0005 = 0

you got autorepeated Key.LeftBtn?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Peter Stokes]

On Tuesday 04 March 2008 12:47:15 Ville Syrjälä wrote:
> On Sat, Feb 16, 2008 at 04:22:43PM +0000, Peter Stokes wrote:
> > The attached patch reconfigures the ati_remote2 driver to use
> > soft-autorepeat functionality and adds support for loadable key maps.
>
> Why was this submitted (and even accepted) without cc:ing me?
>

I am very sorry, that was my fault, I should have cc'd you on the original 
mail.

> > I have reconfigure the driver to use the input system's built-in
> > autorepeat functionality as the device only appears to be able to produce
> > key repeat notifications at a fixed period. Switching to the software
> > autorepeat functionality provides more precise configuration of the
> > timings requested for repeat-delay and repeat-rate.
>
> The soft-autorepeat support should be split into a separate patch. I don't
> need such fast repeat but if it helps people I'm fine with it.
>

My reasoning behind modifying the ati_remote2 driver to use the  
soft-autorepeat implementation provided by the core input system was based 
upon the following:

* It states, in section 1.8 of "Documentation/input/input-programming.txt", 
the following:

  1.8 Key autorepeat
  ~~~~~~~~~~~~~~~~~~

  ... is simple. It is handled by the input.c module. Hardware autorepeat is
  not used, because it's not present in many devices and even where it is
  present, it is broken sometimes (at keyboards: Toshiba notebooks). To enable
  autorepeat for your device, just set EV_REP in dev->evbit. All will be
  handled by the input system.

* Using soft-autorepeat provides a more accurate behavior (the initial delay 
and the repeat rate behave as configured, as opposed to being rounded up to 
the nearest multiple of the hardware's, apparently fixed, repeat 
notifications (the hardware based repeat behavior also introduces timing 
aliasing where the actual interval between successive repeats is 
inconsistent).

* Using soft-autorepeat makes the code in ati_remote2 slightly simpler.

I am happy to produce a separate patch containing only the changes necessary 
to switch the ati_remote2 driver over to use the soft-autorepeat behavior, if 
that is indeed the consensus regarding the best approach to take.

As for ensuring that the mouse buttons on this device do not have auto-repeat 
behavior applied to them. I was very unsure of my proposed solution, as I 
attempted to express in my initial email on the subject. It does however 
strike me that if mouse buttons (and perhaps other button/key codes) should 
not be auto-repeated then these codes should be excluded from the auto-repeat 
implementation within the input core. Experimentation using my Microsoft 
Internet Keyboard appeared to indicate that regular keys where repeated but 
the extra buttons (things like launch email, launch web browser etc.) are not 
(my investigations appeared to indicate, contrary to section 1.8 of the 
documentation quoted above, that the repeat behavior was being performed by 
the hardware and not by the input system's soft-autorepeat implementation). 
This behavior appears to approximately coincide with the boundary described 
by the KEY_MIN_INTERESTING define but I had no idea whether that was merely 
coincidence.

I am happy to implement multiple input devices, one for the mouse, and one for 
the keyboard. If my understanding is correct, this would break backwards 
compatibility as the two devices would be exposed by the evdev driver as two 
separate event devices?

If anyone can suggest the best approach to this problem I would be happy to 
develop the necessary patches to implement the chosen solution.
 

> > As this device is exposed as a combined keyboard and mouse, this change
> > somewhat depends upon the suggested modification to the core
> > soft-autorepeat functionality as outlined in my previous post to the
> > linux-input mailing list (on 12th Feb 2008 entitled "Soft-autorepeat
> > functionality"), without that modification, the mouse buttons are
> > autorepeated :-(
> >
> > The loadable keymap support exposes the ability to map 5 separate
> > keycodes to each key (depending on which "mode" the remote control is
> > currently in). Additionally, I have attempted to ensure that the
> > scancodes used to map keycodes to the keys lie outside of the range
> > normally covered by regular keyboards so as to avoid requests to remap
> > the keys on the remote from being intercepted by a normal keyboard.
>
> I thought the idea of input devices was to reflect the hardware and the
> keymaps should be handled in userspace. If that's not the case then I think
> the keymap support code should not be inside the driver but instead inside
> the input core. We don't want such invasive changes in every driver do
> we?

If I may explain my reasoning behind proposing the changes associated with the 
loadable keymap support. I would welcome any feedback on my reasoning and 
approach.

My initial problem was that some of the keycodes mapped in the ati_remote2 
driver have values greater than 255 and as such I am unable to obtain the 
input from pressing those keys in X windows (perhaps I'm missing some 
required configuration of X windows somewhere?). Upon further investigation 
into this I noticed that the input core provides a mechanism for altering the 
keymap configuration but the ati_remote2 driver is not compatible with it.

Initially I simply modified the ati_remote2 to use the mechanism provided by 
the input core. Having done that, it occurred to me that the mode buttons of 
of the remote could be employed to effectively provide five sets of key 
mappings and I thought that this might be of some use to someone somewhere...

I appreciate that the implementation I have suggested is probably not in line 
with the original intended functionality of the loadable keymap support in 
the input system. But it does get round my issue with X windows....

It also occurred to me that perhaps the multiple-keyboards should be exposed 
as separate input devices, but again, if my understanding is correct, that 
would break backwards compatibility.

Any suggestions on better approaches would certainly be greatfully relieved.

Thank you for taking the time to read my rather long-winded email ;-)

Best regards

Peter Stokes


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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 07:50:56PM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Ville Syrjälä wrote:
> 
> > I've attached the bzipped log.
> 
> Thanks. So between these two events:
<snip> 
> you got autorepeated Key.LeftBtn?

I'm not sure actually. I made a new log and tried to actually
read it this time :)

When I press the button I see this:
drivers/hid/hid-core.c: report (size 6) (numbered)
drivers/hid/hid-core.c: report 17 (size 5) =  01 00 00 00 00
hid-debug: input Button.0001 = 1
hid-debug: input Button.0002 = 0
hid-debug: input Button.0003 = 0
hid-debug: input Button.0004 = 0
hid-debug: input Button.0005 = 0
hid-debug: input GenericDesktop.X = 0
hid-debug: input GenericDesktop.Y = 0
hid-debug: input GenericDesktop.Wheel = 0
hid-debug: input Consumer.HorizontalWheel = 0

When I release the button I get this:
drivers/hid/hid-core.c: report (size 6) (numbered)
drivers/hid/hid-core.c: report 17 (size 5) =  00 00 00 00 00
hid-debug: input Button.0001 = 0
hid-debug: input Button.0002 = 0
hid-debug: input Button.0003 = 0
hid-debug: input Button.0004 = 0
hid-debug: input Button.0005 = 0
hid-debug: input GenericDesktop.X = 0
hid-debug: input GenericDesktop.Y = 0
hid-debug: input GenericDesktop.Wheel = 0
hid-debug: input Consumer.HorizontalWheel = 0

In between I only see this report 5 + report 21 sequence constantly
repeated. But that sequence is constantly repeated even when I don't
touch the mouse at all. It makes inspecting the log somewhat annoying
since the amount of data grows fast. To add insult to injury this device
seems to take part in radio warfare against my normal mouse :(

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 548 bytes --]

On Tue, 4 Mar 2008, Ville Syrjälä wrote:

> > Thanks. So between these two events:
> > you got autorepeated Key.LeftBtn?
> I'm not sure actually. I made a new log and tried to actually read it 
> this time :)

Actually, this is the very same issue we discussed with respect to 
ati_remote2 changes ... this device has Keyboard usages as well as 
Mouse/Pointer. As we set the EV_REP (because of the Keyboard usage), this 
inputdevice-wide flag applies also to the mouse button events and they get 
repeated.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 09:28:56PM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Ville Syrjälä wrote:
> 
> > > Thanks. So between these two events:
> > > you got autorepeated Key.LeftBtn?
> > I'm not sure actually. I made a new log and tried to actually read it 
> > this time :)
> 
> Actually, this is the very same issue we discussed with respect to 
> ati_remote2 changes ... this device has Keyboard usages as well as 
> Mouse/Pointer. As we set the EV_REP (because of the Keyboard usage), this 
> inputdevice-wide flag applies also to the mouse button events and they get 
> repeated.

Except this device does actually register two input devices since the
endpoints for the keyboard and mouse are separate, and here it even
makes sense since they are physically separate devices.

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 06:55:49PM +0000, Peter Stokes wrote:
> On Tuesday 04 March 2008 12:47:15 Ville Syrjälä wrote:
> > On Sat, Feb 16, 2008 at 04:22:43PM +0000, Peter Stokes wrote:
> > > The attached patch reconfigures the ati_remote2 driver to use
> > > soft-autorepeat functionality and adds support for loadable key maps.
> >
> > Why was this submitted (and even accepted) without cc:ing me?
> >
> 
> I am very sorry, that was my fault, I should have cc'd you on the original 
> mail.

Apology accepted. No harm done :)

> > > I have reconfigure the driver to use the input system's built-in
> > > autorepeat functionality as the device only appears to be able to produce
> > > key repeat notifications at a fixed period. Switching to the software
> > > autorepeat functionality provides more precise configuration of the
> > > timings requested for repeat-delay and repeat-rate.
> >
> > The soft-autorepeat support should be split into a separate patch. I don't
> > need such fast repeat but if it helps people I'm fine with it.
> >
> 
> My reasoning behind modifying the ati_remote2 driver to use the  
> soft-autorepeat implementation provided by the core input system was based 
> upon the following:
> 
> * It states, in section 1.8 of "Documentation/input/input-programming.txt", 
> the following:
> 
>   1.8 Key autorepeat
>   ~~~~~~~~~~~~~~~~~~
> 
>   ... is simple. It is handled by the input.c module. Hardware autorepeat is
>   not used, because it's not present in many devices and even where it is
>   present, it is broken sometimes (at keyboards: Toshiba notebooks). To enable
>   autorepeat for your device, just set EV_REP in dev->evbit. All will be
>   handled by the input system.
> 
> * Using soft-autorepeat provides a more accurate behavior (the initial delay 
> and the repeat rate behave as configured, as opposed to being rounded up to 
> the nearest multiple of the hardware's, apparently fixed, repeat 
> notifications (the hardware based repeat behavior also introduces timing 
> aliasing where the actual interval between successive repeats is 
> inconsistent).
> 
> * Using soft-autorepeat makes the code in ati_remote2 slightly simpler.

Right. I think the only downside is additional timer interrupts to
handle the soft repeat. Probably the effect is too minimal to even
consider.

Dmitry did suggest using soft-repeat when I originally submitted the
driver but I had no need for a faster repeat rate so I didn't change the
driver to use it.

> I am happy to produce a separate patch containing only the changes necessary 
> to switch the ati_remote2 driver over to use the soft-autorepeat behavior, if 
> that is indeed the consensus regarding the best approach to take.

Yes, I'd like one patch per feature.

> As for ensuring that the mouse buttons on this device do not have auto-repeat 
> behavior applied to them. I was very unsure of my proposed solution, as I 
> attempted to express in my initial email on the subject. It does however 
> strike me that if mouse buttons (and perhaps other button/key codes) should 
> not be auto-repeated then these codes should be excluded from the auto-repeat 
> implementation within the input core. Experimentation using my Microsoft 
> Internet Keyboard appeared to indicate that regular keys where repeated but 
> the extra buttons (things like launch email, launch web browser etc.) are not 
> (my investigations appeared to indicate, contrary to section 1.8 of the 
> documentation quoted above, that the repeat behavior was being performed by 
> the hardware and not by the input system's soft-autorepeat implementation). 
> This behavior appears to approximately coincide with the boundary described 
> by the KEY_MIN_INTERESTING define but I had no idea whether that was merely 
> coincidence.



> 
> I am happy to implement multiple input devices, one for the mouse, and one for 
> the keyboard. If my understanding is correct, this would break backwards 
> compatibility as the two devices would be exposed by the evdev driver as two 
> separate event devices?
> 
> If anyone can suggest the best approach to this problem I would be happy to 
> develop the necessary patches to implement the chosen solution.

Ah, the backwards compatibility angle. It would be rude of us to break
the behaviour like that. It probably wouldn't affect my typical use case
(MPlayer + DirectFB) since I typically only use the keyboard part of the
remote. But if there are people using both "components" of the remote they
would have to change their configuration or in the worst case their code to
handle such a change. Not nice at all.

> > > As this device is exposed as a combined keyboard and mouse, this change
> > > somewhat depends upon the suggested modification to the core
> > > soft-autorepeat functionality as outlined in my previous post to the
> > > linux-input mailing list (on 12th Feb 2008 entitled "Soft-autorepeat
> > > functionality"), without that modification, the mouse buttons are
> > > autorepeated :-(
> > >
> > > The loadable keymap support exposes the ability to map 5 separate
> > > keycodes to each key (depending on which "mode" the remote control is
> > > currently in). Additionally, I have attempted to ensure that the
> > > scancodes used to map keycodes to the keys lie outside of the range
> > > normally covered by regular keyboards so as to avoid requests to remap
> > > the keys on the remote from being intercepted by a normal keyboard.
> >
> > I thought the idea of input devices was to reflect the hardware and the
> > keymaps should be handled in userspace. If that's not the case then I think
> > the keymap support code should not be inside the driver but instead inside
> > the input core. We don't want such invasive changes in every driver do
> > we?
> 
> If I may explain my reasoning behind proposing the changes associated with the 
> loadable keymap support. I would welcome any feedback on my reasoning and 
> approach.
> 
> My initial problem was that some of the keycodes mapped in the ati_remote2 
> driver have values greater than 255 and as such I am unable to obtain the 
> input from pressing those keys in X windows (perhaps I'm missing some 
> required configuration of X windows somewhere?). Upon further investigation 
> into this I noticed that the input core provides a mechanism for altering the 
> keymap configuration but the ati_remote2 driver is not compatible with it.

Ah, the dreaded X angle :) A bit of googling tells me that the X guys
don't have a real fix coming any time soon. I suppose that is one reason
for having some kind of keymap support in the input core. I personally
don't care for it but there are apprently too few people who can digest
the Xorg code so I suppose it has a compelling reason for existence.

> Initially I simply modified the ati_remote2 to use the mechanism provided by 
> the input core. Having done that, it occurred to me that the mode buttons of 
> of the remote could be employed to effectively provide five sets of key 
> mappings and I thought that this might be of some use to someone somewhere...
> 
> I appreciate that the implementation I have suggested is probably not in line 
> with the original intended functionality of the loadable keymap support in 
> the input system. But it does get round my issue with X windows....
> 
> It also occurred to me that perhaps the multiple-keyboards should be exposed 
> as separate input devices, but again, if my understanding is correct, that 
> would break backwards compatibility.
> 
> Any suggestions on better approaches would certainly be greatfully relieved.

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 think the only problem is the grab thingy. I'm not sure if the Xorg 
evdev driver grabs the device, but if it does then the daemon wouldn't
be able to see the events. DirectFB's input driver does grab the device 
to prevent events leaking to the console (ctrl-c combination was
rather unpleasant without the grab). One solution would be a more
light weight grab that would only prevent the console from receiving the
events but would let other applications to see them. I remeber seeing
some discussion around device grab in the past. I wonder if anything
useful came of it...

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1230 bytes --]

On Tue, 4 Mar 2008, Ville Syrjälä wrote:

> > Actually, this is the very same issue we discussed with respect to 
> > ati_remote2 changes ... this device has Keyboard usages as well as 
> > Mouse/Pointer. As we set the EV_REP (because of the Keyboard usage), 
> > this inputdevice-wide flag applies also to the mouse button events and 
> > they get repeated.
> Except this device does actually register two input devices since the
> endpoints for the keyboard and mouse are separate, and here it even
> makes sense since they are physically separate devices.

Yes, but unfortunately both interfaces contain Keyboard usage in their 
report descriptor -- please see the debug output you sent me. The first 
interface's descriptor states to send Keyboard usages in Field(1) of INPUT 
report, the second one presents it in Field(1) of INPUT[1] report.

I guess that one of the report descriptors contains redundant information, 
not both interfaces are actually sending Keyboard/Consumer/etc events, 
right?

The possibility for this particular device might be fixing the report 
descriptor on the fly, before it enters the parser. We do it for certain 
broken HID devices already.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 09:42:22PM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Ville Syrjälä wrote:
> 
> > > Actually, this is the very same issue we discussed with respect to 
> > > ati_remote2 changes ... this device has Keyboard usages as well as 
> > > Mouse/Pointer. As we set the EV_REP (because of the Keyboard usage), 
> > > this inputdevice-wide flag applies also to the mouse button events and 
> > > they get repeated.
> > Except this device does actually register two input devices since the
> > endpoints for the keyboard and mouse are separate, and here it even
> > makes sense since they are physically separate devices.
> 
> Yes, but unfortunately both interfaces contain Keyboard usage in their 
> report descriptor -- please see the debug output you sent me. The first 
> interface's descriptor states to send Keyboard usages in Field(1) of INPUT 
> report, the second one presents it in Field(1) of INPUT[1] report.
> 
> I guess that one of the report descriptors contains redundant information, 
> not both interfaces are actually sending Keyboard/Consumer/etc events, 
> right?

I suppose not. HID is more or less gibberish to me but at least I haven't
seen any extra keyboard events from the mouse or anything like that. The
mouse has just two buttons and a vertical wheel. The keyboard has the normal
keys, volume and playback control keys and some 10 extra keys like sleep,
mail, web etc. It also has an f-lock key which I suppose has something to
do with the fact that they've re-labeled the function keys with some
web/email/whatever texts and the F<number> is printed on the side
of the key.

I've used the device so little that I honestly didn't even notice that
the mouse claims to support pretty much every kind of event there is.
Maybe they were just lazy when making this device and didn't want to think
about the actual features of the device and instead just added every
possible thing to the descriptors.

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Peter Stokes]

On Tuesday 04 March 2008 20:38:22 Ville Syrjälä wrote:
> On Tue, Mar 04, 2008 at 06:55:49PM +0000, Peter Stokes wrote:
> > On Tuesday 04 March 2008 12:47:15 Ville Syrjälä wrote:
> > > On Sat, Feb 16, 2008 at 04:22:43PM +0000, Peter Stokes wrote:
> > > > The attached patch reconfigures the ati_remote2 driver to use
> > > > soft-autorepeat functionality and adds support for loadable key maps.
> > >
> > > Why was this submitted (and even accepted) without cc:ing me?
> >
> > I am very sorry, that was my fault, I should have cc'd you on the
> > original mail.
>
> Apology accepted. No harm done :)
>

Thank you :-)

> > > > I have reconfigure the driver to use the input system's built-in
> > > > autorepeat functionality as the device only appears to be able to
> > > > produce key repeat notifications at a fixed period. Switching to the
> > > > software autorepeat functionality provides more precise configuration
> > > > of the timings requested for repeat-delay and repeat-rate.
> > >
> > > The soft-autorepeat support should be split into a separate patch. I
> > > don't need such fast repeat but if it helps people I'm fine with it.
> >
> > My reasoning behind modifying the ati_remote2 driver to use the
> > soft-autorepeat implementation provided by the core input system was
> > based upon the following:
> >
> > * It states, in section 1.8 of
> > "Documentation/input/input-programming.txt", the following:
> >
> >   1.8 Key autorepeat
> >   ~~~~~~~~~~~~~~~~~~
> >
> >   ... is simple. It is handled by the input.c module. Hardware autorepeat
> > is not used, because it's not present in many devices and even where it
> > is present, it is broken sometimes (at keyboards: Toshiba notebooks). To
> > enable autorepeat for your device, just set EV_REP in dev->evbit. All
> > will be handled by the input system.
> >
> > * Using soft-autorepeat provides a more accurate behavior (the initial
> > delay and the repeat rate behave as configured, as opposed to being
> > rounded up to the nearest multiple of the hardware's, apparently fixed,
> > repeat notifications (the hardware based repeat behavior also introduces
> > timing aliasing where the actual interval between successive repeats is
> > inconsistent).
> >
> > * Using soft-autorepeat makes the code in ati_remote2 slightly simpler.
>
> Right. I think the only downside is additional timer interrupts to
> handle the soft repeat. Probably the effect is too minimal to even
> consider.
>
> Dmitry did suggest using soft-repeat when I originally submitted the
> driver but I had no need for a faster repeat rate so I didn't change the
> driver to use it.
>
> > I am happy to produce a separate patch containing only the changes
> > necessary to switch the ati_remote2 driver over to use the
> > soft-autorepeat behavior, if that is indeed the consensus regarding the
> > best approach to take.
>
> Yes, I'd like one patch per feature.
>

I'll do that (do need to sort out exactly how to implement it first though)

> > As for ensuring that the mouse buttons on this device do not have
> > auto-repeat behavior applied to them. I was very unsure of my proposed
> > solution, as I attempted to express in my initial email on the subject.
> > It does however strike me that if mouse buttons (and perhaps other
> > button/key codes) should not be auto-repeated then these codes should be
> > excluded from the auto-repeat implementation within the input core.
> > Experimentation using my Microsoft Internet Keyboard appeared to indicate
> > that regular keys where repeated but the extra buttons (things like
> > launch email, launch web browser etc.) are not (my investigations
> > appeared to indicate, contrary to section 1.8 of the documentation quoted
> > above, that the repeat behavior was being performed by the hardware and
> > not by the input system's soft-autorepeat implementation). This behavior
> > appears to approximately coincide with the boundary described by the
> > KEY_MIN_INTERESTING define but I had no idea whether that was merely
> > coincidence.
> >
> >
> >
> >
> > I am happy to implement multiple input devices, one for the mouse, and
> > one for the keyboard. If my understanding is correct, this would break
> > backwards compatibility as the two devices would be exposed by the evdev
> > driver as two separate event devices?
> >
> > If anyone can suggest the best approach to this problem I would be happy
> > to develop the necessary patches to implement the chosen solution.
>
> Ah, the backwards compatibility angle. It would be rude of us to break
> the behaviour like that. It probably wouldn't affect my typical use case
> (MPlayer + DirectFB) since I typically only use the keyboard part of the
> remote. But if there are people using both "components" of the remote they
> would have to change their configuration or in the worst case their code to
> handle such a change. Not nice at all.
>

The backwards compatibility wouldn't be a problem for me either (I'm using X 
windows and MythTV). I felt that the original choice of keymappings 
represents the closest match between the images physically printed on the 
keys and the descriptions contained in the standard keycode defines but 
unfortunately they result in some fairly crucial keys (like 'ok' for example) 
being undetectable in X windows :-( 

I suspect that that very few people are currently using this driver for this 
very reason, and upon that entirely unsubstantiated assumption I'd suggest 
that if it is deemed that the best approach is to expose the mouse and 
keyboard functionality as two separate devices then that would probably be 
acceptable?

My personal feeling is that, if mouse buttons (and other keycodes) should not 
be repeated, then they should be excluded from the soft-autorepeat 
functionality offered by the input core.

> > > > As this device is exposed as a combined keyboard and mouse, this
> > > > change somewhat depends upon the suggested modification to the core
> > > > soft-autorepeat functionality as outlined in my previous post to the
> > > > linux-input mailing list (on 12th Feb 2008 entitled "Soft-autorepeat
> > > > functionality"), without that modification, the mouse buttons are
> > > > autorepeated :-(
> > > >
> > > > The loadable keymap support exposes the ability to map 5 separate
> > > > keycodes to each key (depending on which "mode" the remote control is
> > > > currently in). Additionally, I have attempted to ensure that the
> > > > scancodes used to map keycodes to the keys lie outside of the range
> > > > normally covered by regular keyboards so as to avoid requests to
> > > > remap the keys on the remote from being intercepted by a normal
> > > > keyboard.
> > >
> > > I thought the idea of input devices was to reflect the hardware and the
> > > keymaps should be handled in userspace. If that's not the case then I
> > > think the keymap support code should not be inside the driver but
> > > instead inside the input core. We don't want such invasive changes in
> > > every driver do we?
> >
> > If I may explain my reasoning behind proposing the changes associated
> > with the loadable keymap support. I would welcome any feedback on my
> > reasoning and approach.
> >
> > My initial problem was that some of the keycodes mapped in the
> > ati_remote2 driver have values greater than 255 and as such I am unable
> > to obtain the input from pressing those keys in X windows (perhaps I'm
> > missing some required configuration of X windows somewhere?). Upon
> > further investigation into this I noticed that the input core provides a
> > mechanism for altering the keymap configuration but the ati_remote2
> > driver is not compatible with it.
>
> Ah, the dreaded X angle :) A bit of googling tells me that the X guys
> don't have a real fix coming any time soon. I suppose that is one reason
> for having some kind of keymap support in the input core. I personally
> don't care for it but there are apprently too few people who can digest
> the Xorg code so I suppose it has a compelling reason for existence.
>
> > Initially I simply modified the ati_remote2 to use the mechanism provided
> > by the input core. Having done that, it occurred to me that the mode
> > buttons of of the remote could be employed to effectively provide five
> > sets of key mappings and I thought that this might be of some use to
> > someone somewhere...
> >
> > I appreciate that the implementation I have suggested is probably not in
> > line with the original intended functionality of the loadable keymap
> > support in the input system. But it does get round my issue with X
> > windows....
> >
> > It also occurred to me that perhaps the multiple-keyboards should be
> > exposed as separate input devices, but again, if my understanding is
> > correct, that would break backwards compatibility.
> >
> > Any suggestions on better approaches would certainly be greatfully
> > relieved.
>
> 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 think the only problem is the grab thingy. I'm not sure if the Xorg
> evdev driver grabs the device, but if it does then the daemon wouldn't
> be able to see the events. DirectFB's input driver does grab the device
> to prevent events leaking to the console (ctrl-c combination was
> rather unpleasant without the grab). One solution would be a more
> light weight grab that would only prevent the console from receiving the
> events but would let other applications to see them. I remeber seeing
> some discussion around device grab in the past. I wonder if anything
> useful came of it...

When I initially implemented the loadable keymap support using the input core 
built in handling I hit the problem that I wanted to override the keys on the 
remote control (the reason for looking into all of this) but some of the 
scancode were taken by the standard PS2 keyboard driver. I reasoned that most 
people wouldn't want to break their regular keyboard mappings so I then 
implemented my own get/setkeycode functions in order to place the remote's 
scancodes outside of the normal range produced by regular keyboards. Once I'd 
had to implement my own versions of those functions it seemed trivial to 
provide the 'layered' keyboard implementation.

I must confess I don't have an immediate requirement for this functionality it 
just seemed an easy and potentially useful thing to provide...

My understanding of the X windows situation is that the X server protocol only 
allows a single byte for keycodes,and as the server protocol is an network 
standard it's not a case of changing some code it's a case of changing the 
standard (something that isn't going to happen particularly quickly!). Hence 
this seemed like a reasonable, if not entirely elegant, way around it...


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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 619 bytes --]

On Tue, 4 Mar 2008, Ville Syrjälä wrote:

> I've used the device so little that I honestly didn't even notice that 
> the mouse claims to support pretty much every kind of event there is. 
> Maybe they were just lazy when making this device and didn't want to 
> think about the actual features of the device and instead just added 
> every possible thing to the descriptors.

Exactly. We could either fix the report descriptor upon initialization, or 
put some quirk into event handler, that will make sure that the mouse 
buttons won't get repeated for this particular device.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Ville Syrjälä]

On Tue, Mar 04, 2008 at 09:34:29PM +0000, Peter Stokes wrote:
> On Tuesday 04 March 2008 20:38:22 Ville Syrjälä wrote:
> > On Tue, Mar 04, 2008 at 06:55:49PM +0000, Peter Stokes wrote:
> > > I am happy to produce a separate patch containing only the changes
> > > necessary to switch the ati_remote2 driver over to use the
> > > soft-autorepeat behavior, if that is indeed the consensus regarding the
> > > best approach to take.
> >
> > Yes, I'd like one patch per feature.
> >
> 
> I'll do that (do need to sort out exactly how to implement it first though)

Right.

> > > As for ensuring that the mouse buttons on this device do not have
> > > auto-repeat behavior applied to them. I was very unsure of my proposed
> > > solution, as I attempted to express in my initial email on the subject.
> > > It does however strike me that if mouse buttons (and perhaps other
> > > button/key codes) should not be auto-repeated then these codes should be
> > > excluded from the auto-repeat implementation within the input core.
> > > Experimentation using my Microsoft Internet Keyboard appeared to indicate
> > > that regular keys where repeated but the extra buttons (things like
> > > launch email, launch web browser etc.) are not (my investigations
> > > appeared to indicate, contrary to section 1.8 of the documentation quoted
> > > above, that the repeat behavior was being performed by the hardware and
> > > not by the input system's soft-autorepeat implementation). This behavior
> > > appears to approximately coincide with the boundary described by the
> > > KEY_MIN_INTERESTING define but I had no idea whether that was merely
> > > coincidence.
> > >
> > >
> > >
> > >
> > > I am happy to implement multiple input devices, one for the mouse, and
> > > one for the keyboard. If my understanding is correct, this would break
> > > backwards compatibility as the two devices would be exposed by the evdev
> > > driver as two separate event devices?
> > >
> > > If anyone can suggest the best approach to this problem I would be happy
> > > to develop the necessary patches to implement the chosen solution.
> >
> > Ah, the backwards compatibility angle. It would be rude of us to break
> > the behaviour like that. It probably wouldn't affect my typical use case
> > (MPlayer + DirectFB) since I typically only use the keyboard part of the
> > remote. But if there are people using both "components" of the remote they
> > would have to change their configuration or in the worst case their code to
> > handle such a change. Not nice at all.
> >
> 
> The backwards compatibility wouldn't be a problem for me either (I'm using X 
> windows and MythTV). I felt that the original choice of keymappings 
> represents the closest match between the images physically printed on the 
> keys and the descriptions contained in the standard keycode defines but 
> unfortunately they result in some fairly crucial keys (like 'ok' for example) 
> being undetectable in X windows :-( 
> 
> I suspect that that very few people are currently using this driver for this 
> very reason, and upon that entirely unsubstantiated assumption I'd suggest 
> that if it is deemed that the best approach is to expose the mouse and 
> keyboard functionality as two separate devices then that would probably be 
> acceptable?
> 
> My personal feeling is that, if mouse buttons (and other keycodes) should not 
> be repeated, then they should be excluded from the soft-autorepeat 
> functionality offered by the input core.

I think that repeating buttons are a good thing in some cases (autofire 
in games for example). Then again such games could implement autofire
themselves.

However I don't see why the repeat vs. no repeat case should depend on
the device reporting or not reporting keyboard keys. I don't see any real
connection with a device having some keyboard keys and sending repeat
events for buttons.

Let's say you have a HID keyboard which does in fact have an integrated
trackball/whatever (at least keyboard with scroll wheels exists so adding
a complete trackball doesn't seem that far fetched) but instead of having
separate end points for each "device" it would just have one endpoint.
That device would experience the same problem that my HID mouse has.

> > 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 think the only problem is the grab thingy. I'm not sure if the Xorg
> > evdev driver grabs the device, but if it does then the daemon wouldn't
> > be able to see the events. DirectFB's input driver does grab the device
> > to prevent events leaking to the console (ctrl-c combination was
> > rather unpleasant without the grab). One solution would be a more
> > light weight grab that would only prevent the console from receiving the
> > events but would let other applications to see them. I remeber seeing
> > some discussion around device grab in the past. I wonder if anything
> > useful came of it...
> 
> When I initially implemented the loadable keymap support using the input core 
> built in handling I hit the problem that I wanted to override the keys on the 
> remote control (the reason for looking into all of this) but some of the 
> scancode were taken by the standard PS2 keyboard driver. I reasoned that most 
> people wouldn't want to break their regular keyboard mappings so I then 
> implemented my own get/setkeycode functions in order to place the remote's 
> scancodes outside of the normal range produced by regular keyboards. Once I'd 
> had to implement my own versions of those functions it seemed trivial to 
> provide the 'layered' keyboard implementation.
> 
> I must confess I don't have an immediate requirement for this functionality it 
> just seemed an easy and potentially useful thing to provide...
> 
> My understanding of the X windows situation is that the X server protocol only 
> allows a single byte for keycodes,and as the server protocol is an network 
> standard it's not a case of changing some code it's a case of changing the 
> standard (something that isn't going to happen particularly quickly!). Hence 
> this seemed like a reasonable, if not entirely elegant, way around it...

http://bugs.freedesktop.org/show_bug.cgi?id=11227
has some ideas about compressing the keycodes to the 8-255 range for
each device by using device specific keymaps.

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.

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

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 643 bytes --]

On Wed, 5 Mar 2008, Ville Syrjälä wrote:

> However I don't see why the repeat vs. no repeat case should depend on 
> the device reporting or not reporting keyboard keys. I don't see any 
> real connection with a device having some keyboard keys and sending 
> repeat events for buttons.

The problem is, that currently EV_REP is per-device flag. Once we set it, 
it is valid for all EV_KEY events (which includes keyboard events, but 
also mouse buttons).

I think it might be good to redesign this a little bit, but I'd like to 
hear Dmitry's opinion here, let's see when he will appear.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Jiri Kosina]

On Tue, 4 Mar 2008, Jiri Kosina wrote:

> > However I don't see why the repeat vs. no repeat case should depend on 
> > the device reporting or not reporting keyboard keys. I don't see any 
> > real connection with a device having some keyboard keys and sending 
> > repeat events for buttons.
> The problem is, that currently EV_REP is per-device flag. Once we set 
> it, it is valid for all EV_KEY events (which includes keyboard events, 
> but also mouse buttons).
> I think it might be good to redesign this a little bit, but I'd like to 
> hear Dmitry's opinion here, let's see when he will appear.

Dmitry,

do you have any opinion/idea what might be the best soltuion please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ati_remote2 autorepeat and loadable keymap support

[Dmitry Torokhov]

On Tue, Mar 25, 2008 at 09:23:02AM +0100, Jiri Kosina wrote:
> On Tue, 4 Mar 2008, Jiri Kosina wrote:
> 
> > > However I don't see why the repeat vs. no repeat case should depend on 
> > > the device reporting or not reporting keyboard keys. I don't see any 
> > > real connection with a device having some keyboard keys and sending 
> > > repeat events for buttons.
> > The problem is, that currently EV_REP is per-device flag. Once we set 
> > it, it is valid for all EV_KEY events (which includes keyboard events, 
> > but also mouse buttons).
> > I think it might be good to redesign this a little bit, but I'd like to 
> > hear Dmitry's opinion here, let's see when he will appear.
> 
> Dmitry,
> 
> do you have any opinion/idea what might be the best soltuion please?
> 

I think that for now it is driver's responsibility to implement "fancy"
autorepeat, if needed. I think good implementation should respect repeat
rate settings requested from userspace though...

If there are lots of drivers implementing autorepeat themselves and we
start seeing common patterns we could start moving it to the core,
probably by implementing some helper functions. Does this make sense?

-- 
Dmitry