Re: Review of patch for MS USB keyboard

Khelben Blackstaff <[email protected]>
Newsgroups gmane.linux.usb.devel
Message-ID <[email protected]>
Thank you for replying so quickly.
I changed my errors according to your instructions.
I include the revised patches.

Στις Fri, 21 Sep 2007 15:23:24 +0200 (CEST)
Ο/Η Jiri Kosina <[email protected]> έγραψε:

> > 1 hunk) I included my keyboard (subid f9) in the matching
> 
> Probably renaming the macro would also be good, when it is now used to 
> match not only the 4K keyboard.

done that. renamed it to IS_MS_KB from IS_MS_NEK4K

> 
> > 2 hunk) Added HID code 0x182 which is the Favorites key.  At first i 
> >   named it KEY_FAVORITES but found out that keycodes > 255 don't work in 
> >   the X server so renamed it to KEY_BOOKMARKS
> 
> Hmm, so they keep on breaking the HID specification, oh well :/ 0x182 
> should be "Programmable button configuration".
> 

I checked that one more time. the bookmarks key emits 0x182
I checked the HID Usage tables 1.12 and you are right. It is
programmable button. I don't know why they chose 0x182.

> > Also i changed KEY_SPELLCHECK to KEY_FIND because KEY_SPELLCHECK > 255 I 
> > know KEY_FIND itsn't right but just till X supports larger keycodes.
> 
> This is not acceptable, sorry.
> 
> X should accept > 255 codes through evdev, right?

Yes, you are right. X accepts >255 keycodes if i use evdev.
Reverted it back to the correct definition of KEY_SPELLCHECK

> 
> > Added HID codes 0x1b6,0x1b7 which are the Camera(Media) and Sound 
> > buttons
> 
> OK, 0x1b6 and 0x1b7 are currently reserved by the specification, so we can 
> add them, but it will very probably break when new usage tables are out :(

Checked that too. The codes are 0x1b6 and 0x1b7 as i said, but
these seem correct. Page 83 of the 1.12 specification mentions
0x1b6 as "image browser" and 0x1b7 as "audio browser". The icons
on these two keys show the same thing. A camera and a headset.

> 
> > 4 hunk) Changed Zoom In/Out to Scroll Up/Down again due to the 255 
> >   issue. (Also Scroll is more useful than zoom i guess.
> 
> Again, not acceptable, sorry.

Reverted it back to the correct definition.

> Thanks for making the patch, it'll need some cleanups before I could 
> accept it though, see above.
> 
> BTW, it would be nice if you could somehow teach your mail client to wrap 
> lines properly.

I am sorry for that. I was using another box and sent it through the
web interface of gmail. Now i send it through sylpheed-claws. I hope
it is correct now.


Στις Fri, 21 Sep 2007 09:44:52 -0400
Ο/Η "Dmitry Torokhov" <[email protected]> έγραψε:

> I don't think we have logoff. But based on the description we coud
> probably map it to KEY_SCREENLOCK (AKA KEY_COFFEE).
> 

I searched for some "lock" key but didn't find anything, that is why
i gave it an irrelevant name. I saw KEY_COFFEE and wondered what use did it
have. It didn't occur to me that when i lock the screen i will go for coffee :)
I changed it to KEY_COFFEE as you suggested, but i have read in the
specification that code 0x19c is Logoff. So, i guess a KEY_LOGOFF should be
added to input.h and 0x19c should be specified as KEY_LOGOFF. I didn't do
that in the patches because i didn't know which is the correct way to do it.

Στις Fri, 21 Sep 2007 12:23:07 +0300
Ο/Η "Khelben Blackstaff" <[email protected]> έγραψε:

> 5 hunk) This is the original part from the mm patch with the following
> changes. 
>              b) I didn't understand what the set_bit(BTN_X) do. The
> keys work even with it
>                  commented, so i commented. I guess it is there for a
> reason so maybe my
>                  commenting is wrong.
> 6 hunk)  Changed BTN_1-5 to F13-F17 again due to > 255 for the keys to work
> in X
> 

Reverted my changes to this part back to the original code included in
the -mm tree, because the changes are not needed since X supports > 255
codes with evdev.
Noticed something though. xev reports button events and not key events
as if mouse buttons were pressed. Also, in console buttons 1,2,3 react
as the left/middle/right mouse buttons. Is this the wanted behavior ?
Wouldn't it be better if they reacted as some keys ? (at first i
thought of KEY_PROGX but only 4 KEY_PROG keys exist).
I don't have much knowledge about this so excuse me if what i suggest
is stupid.

Thank you again.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
from_2.6.22_v2.diff (text/x-patch, 4.6 KB)
--- drivers/hid/hid-input.orig	2007-09-21 11:11:28.000000000 +0300
+++ drivers/hid/hid-input.c	2007-09-22 11:35:53.000000000 +0300
@@ -73,6 +73,10 @@
 #define map_abs_clear(c)	do { map_abs(c); clear_bit(c, bit); } while (0)
 #define map_key_clear(c)	do { map_key(c); clear_bit(c, bit); } while (0)
 
+/* hardware needing special handling due to colliding MSVENDOR page usages */
+#define IS_CHICONY_TACTICAL_PAD(x) (x->vendor == 0x04f2 && device->product == 0x0418)
+#define IS_MS_KB(x) (x->vendor == 0x045e && (x->product == 0x00db || x->product == 0x00f9))
+
 #ifdef CONFIG_USB_HIDINPUT_POWERBOOK
 
 struct hidinput_key_translation {
@@ -566,6 +570,7 @@
 				case 0x0e5: map_key_clear(KEY_BASSBOOST);	break;
 				case 0x0e9: map_key_clear(KEY_VOLUMEUP);	break;
 				case 0x0ea: map_key_clear(KEY_VOLUMEDOWN);	break;
+				case 0x182: map_key_clear(KEY_BOOKMARKS);	break;
 				case 0x183: map_key_clear(KEY_CONFIG);		break;
 				case 0x184: map_key_clear(KEY_WORDPROCESSOR);	break;
 				case 0x185: map_key_clear(KEY_EDITOR);		break;
@@ -582,9 +587,13 @@
 				case 0x192: map_key_clear(KEY_CALC);		break;
 				case 0x194: map_key_clear(KEY_FILE);		break;
 				case 0x196: map_key_clear(KEY_WWW);		break;
+				case 0x19c: map_key_clear(KEY_COFFEE);		break;
 				case 0x19e: map_key_clear(KEY_COFFEE);		break;
 				case 0x1a6: map_key_clear(KEY_HELP);		break;
 				case 0x1a7: map_key_clear(KEY_DOCUMENTS);	break;
+				case 0x1ab: map_key_clear(KEY_SPELLCHECK);	break;
+				case 0x1b6: map_key_clear(KEY_MEDIA);		break;
+				case 0x1b7: map_key_clear(KEY_SOUND);		break;
 				case 0x1bc: map_key_clear(KEY_MESSENGER);	break;
 				case 0x1bd: map_key_clear(KEY_INFO);		break;
 				case 0x201: map_key_clear(KEY_NEW);		break;
@@ -688,7 +697,55 @@
 			break;
 
 		case HID_UP_MSVENDOR:
-			goto ignore;
+
+			/* Unfortunately, there are multiple devices which
+			 * emit usages from MSVENDOR page that require different
+			 * handling. If this list grows too much in the future,
+			 * more general handling will have to be introduced here
+			 * (i.e. another blacklist).
+			 */
+
+			/* Chicony Chicony KU-0418 tactical pad */
+			if (IS_CHICONY_TACTICAL_PAD(device)) {
+				set_bit(EV_REP, input->evbit);
+				switch(usage->hid & HID_USAGE) {
+					case 0xff01: map_key_clear(BTN_1);		break;
+					case 0xff02: map_key_clear(BTN_2);		break;
+					case 0xff03: map_key_clear(BTN_3);		break;
+					case 0xff04: map_key_clear(BTN_4);		break;
+					case 0xff05: map_key_clear(BTN_5);		break;
+					case 0xff06: map_key_clear(BTN_6);		break;
+					case 0xff07: map_key_clear(BTN_7);		break;
+					case 0xff08: map_key_clear(BTN_8);		break;
+					case 0xff09: map_key_clear(BTN_9);		break;
+					case 0xff0a: map_key_clear(BTN_A);		break;
+					case 0xff0b: map_key_clear(BTN_B);		break;
+					default:    goto ignore;
+				}
+
+			/* Microsoft Natural Ergonomic Keyboard 4000 */
+			} else if (IS_MS_KB(device)) {
+				switch(usage->hid & HID_USAGE) {
+					case 0xfd06:
+						map_key_clear(KEY_CHAT);
+						break;
+					case 0xfd07:
+						map_key_clear(KEY_PHONE);
+						break;
+					case 0xff05:
+						set_bit(EV_REP, input->evbit);
+						map_key_clear(BTN_0);
+						set_bit(BTN_1, input->keybit);
+						set_bit(BTN_2, input->keybit);
+						set_bit(BTN_3, input->keybit);
+						set_bit(BTN_4, input->keybit);
+						set_bit(BTN_5, input->keybit);
+					default:	goto ignore;
+				}
+			} else {
+				goto ignore;
+			}
+			break;
 
 		case HID_UP_CUSTOM: /* Reported on Logitech and Powerbook USB keyboards */
 
@@ -908,6 +965,26 @@
 	if ((usage->type == EV_KEY) && (usage->code == 0)) /* Key 0 is "unassigned", not KEY_UNKNOWN */
 		return;
 
+	/* Handling MS keyboards special buttons */
+	if (IS_MS_KB(hid) && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
+		int key = 0;
+		static int last_key = 0;
+		switch (value) {
+			case 0x01: key = BTN_1; break;
+			case 0x02: key = BTN_2; break;
+			case 0x04: key = BTN_3; break;
+			case 0x08: key = BTN_4; break;
+			case 0x10: key = BTN_5; break;
+			default: break;
+		}
+		if (key) {
+			input_event(input, usage->type, key, 1);
+			last_key = key;
+		} else {
+			input_event(input, usage->type, last_key, 0);
+		}
+	}
+
 	input_event(input, usage->type, usage->code, value);
 
 	if ((field->flags & HID_MAIN_ITEM_RELATIVE) && (usage->type == EV_KEY))
--- include/linux/input.orig	2007-09-22 11:28:10.000000000 +0300
+++ include/linux/input.h	2007-09-22 11:30:03.000000000 +0300
@@ -515,6 +515,7 @@
 #define KEY_ADDRESSBOOK		0x1ad	/* AL Contacts/Address Book */
 #define KEY_MESSENGER		0x1ae	/* AL Instant Messaging */
 #define KEY_DISPLAYTOGGLE	0x1af	/* Turn display (LCD) on and off */
+#define KEY_SPELLCHECK		0x1b0   /* AL Spell Check */
 
 #define KEY_DEL_EOL		0x1c0
 #define KEY_DEL_EOS		0x1c1
from_2.6.23-rc6-mm1_v2.diff (text/x-patch, 2.2 KB)
--- drivers/hid/hid-input.orig	2007-09-20 11:13:14.000000000 +0300
+++ drivers/hid/hid-input.c	2007-09-22 11:41:11.000000000 +0300
@@ -88,7 +88,7 @@
 
 /* hardware needing special handling due to colliding MSVENDOR page usages */
 #define IS_CHICONY_TACTICAL_PAD(x) (x->vendor == 0x04f2 && device->product == 0x0418)
-#define IS_MS_NEK4K(x) (x->vendor == 0x045e && x->product == 0x00db)
+#define IS_MS_KB(x) (x->vendor == 0x045e && (x->product == 0x00db || x->product == 0x00f9))
 
 #ifdef CONFIG_USB_HIDINPUT_POWERBOOK
 
@@ -606,6 +606,7 @@
 				case 0x0f6: map_key_clear(KEY_NEXT);		break;
 				case 0x0fa: map_key_clear(KEY_BACK);		break;
 
+				case 0x182: map_key_clear(KEY_BOOKMARKS);	break;
 				case 0x183: map_key_clear(KEY_CONFIG);		break;
 				case 0x184: map_key_clear(KEY_WORDPROCESSOR);	break;
 				case 0x185: map_key_clear(KEY_EDITOR);		break;
@@ -622,10 +623,13 @@
 				case 0x192: map_key_clear(KEY_CALC);		break;
 				case 0x194: map_key_clear(KEY_FILE);		break;
 				case 0x196: map_key_clear(KEY_WWW);		break;
+				case 0x19c: map_key_clear(KEY_COFFEE);		break;
 				case 0x19e: map_key_clear(KEY_COFFEE);		break;
 				case 0x1a6: map_key_clear(KEY_HELP);		break;
 				case 0x1a7: map_key_clear(KEY_DOCUMENTS);	break;
 				case 0x1ab: map_key_clear(KEY_SPELLCHECK);	break;
+				case 0x1b6: map_key_clear(KEY_MEDIA);		break;
+				case 0x1b7: map_key_clear(KEY_SOUND);		break;
 				case 0x1bc: map_key_clear(KEY_MESSENGER);	break;
 				case 0x1bd: map_key_clear(KEY_INFO);		break;
 				case 0x201: map_key_clear(KEY_NEW);		break;
@@ -758,8 +762,14 @@
 				}
 
 			/* Microsoft Natural Ergonomic Keyboard 4000 */
-			} else if (IS_MS_NEK4K(device)) {
+			} else if (IS_MS_KB(device)) {
 				switch(usage->hid & HID_USAGE) {
+					case 0xfd06:
+						map_key_clear(KEY_CHAT);
+						break;
+					case 0xfd07:
+						map_key_clear(KEY_PHONE);
+						break;
 					case 0xff05:
 						set_bit(EV_REP, input->evbit);
 						map_key_clear(BTN_0);
@@ -1029,8 +1039,8 @@
 		return;
 	}
 
-	/* Handling MS NEK4K special buttons */
-	if (IS_MS_NEK4K(hid) && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
+	/* Handling MS keyboards special buttons */
+	if (IS_MS_KB(hid) && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
 		int key = 0;
 		static int last_key = 0;
 		switch (value) {
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.