USB: add arrow key support to usb_kbd

PATCH New Allen Martin 4 participants 13 messages Nov 6, 2012 – Nov 8, 2012

Share & tools

Allen Martin

author

2012-11-06 21:26 UTC #0 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue, but this allows arrow keys to work until this driver is converted.

Signed-off-by Allen Martin <[email protected]>

---
 common/usb_kbd.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

common/usb_kbd.c +13 -0

index 24467ce..4efbcfe 100644
@@ -94,6 +94,15 @@ static const unsigned char usb_kbd_num_keypad[] = {
 };
 
 /*
+ * map arrow keys to ^F/^B ^N/^P, can't really use the proper
+ * ANSI sequence for arrow keys because the queuing code breaks
+ * when a single keypress expands to 3 queue elements
+ */
+static const unsigned char usb_kbd_arrow[] = {
+	0x6, 0x2, 0xe, 0x10
+};
+
+/*
  * NOTE: It's important for the NUM, CAPS, SCROLL-lock bits to be in this
  *       order. See usb_kbd_setled() function!
  */

@@ -224,6 +233,10 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode,
 			keycode = usb_kbd_numkey[scancode - 0x1e];
 	}
 
+	/* Arrow keys */
+	if ((scancode >= 0x4f) && (scancode <= 0x52))
+		keycode = usb_kbd_arrow[scancode - 0x4f];
+
 	/* Numeric keypad */
 	if ((scancode >= 0x54) && (scancode <= 0x67))
 		keycode = usb_kbd_num_keypad[scancode - 0x54];

Signature · 1 line

1.7.10.4

Stephen Warren

2012-11-06 21:51 UTC #1 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Quoted 6 lines · Allen Martin

Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue, but this allows arrow keys to work until this driver is converted.

Tested-by Stephen Warren <[email protected]>

Marek Vasut

2012-11-06 22:49 UTC #2 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

In reply to Allen Martin

Dear Allen Martin,

Quoted 5 lines

Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue

If it's the real fix, then why not go for the real fix right away? :-(

Quoted 5 lines

but this allows arrow keys to
work until this driver is converted.
Signed-off-by Allen Martin <[email protected]>
---

[...]

Best regards,
Marek Vasut

Simon Glass

2012-11-06 22:51 UTC #3 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Hi Marek,

Quoted 9 lines · Marek Vasut

Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue
If it's the real fix, then why not go for the real fix right away? :-(

Because it's a fair chunk of work, and also if we are doing USB we should probably do keyboard.c first. USB would not be the first priority since so much of the logic / keycodes are different.

Regards,
Simon

Quoted 10 lines

Quoted 6 lines
but this allows arrow keys to
work until this driver is converted.
Signed-off-by Allen Martin <[email protected]>
---
[...]
Best regards,
Marek Vasut

Marek Vasut

2012-11-06 22:56 UTC #4 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Dear Simon Glass,

Quoted 11 lines

Hi Marek,
Quoted 8 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue

Quoted 3 lines

Quoted 1 line
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work

Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows ...

Quoted 2 lines

and also if we are doing USB we
should probably do keyboard.c first.

Uh, I don't follow.

Quoted 2 lines

USB would not be the first
priority since so much of the logic / keycodes are different.

Sorry, I'm completely lost.

Quoted 13 lines

Regards,
Simon
Quoted 10 lines
Quoted 5 lines
but this allows arrow keys to
work until this driver is converted.
Signed-off-by Allen Martin <[email protected]>
---
[...]
Best regards,
Marek Vasut

Best regards,
Marek Vasut

Allen Martin

author

2012-11-06 23:06 UTC #5 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Quoted 13 lines · Marek Vasut

Dear Simon Glass,
Quoted 11 lines
Hi Marek,
Quoted 8 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work
Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows ...

I'm definately on board with changing it over to use input, it seems like the right thing to do. I just wasn't signing up to do the work right now. I'm happy to help review patches though if someone else wants to jump in.

-Allen

Signature · 1 line

nvpublic

Marek Vasut

2012-11-07 13:15 UTC #6 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Dear Allen Martin,

Quoted 19 lines · Marek Vasut

Quoted 18 lines
Dear Simon Glass,
Quoted 15 lines
Hi Marek,
Quoted 10 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work

Quoted 8 lines

Quoted 3 lines
Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows
...
I'm definately on board with changing it over to use input, it seems like the right thing to do. I just wasn't signing up to do the work right now. I'm happy to help review patches though if someone else wants to jump in.

Guys ... I have two things in my mind. You want to add features, that's fine with me. But then, the merge window is closed already, so you have crapload of time to work on it. Also, if you won't do it now, noone will ever do it later.

Please, do it now. I don't see it being too much work.

btw. do you remember we've been in such place once already with some other piece of u-boot?

Best regards,
Marek Vasut

Stephen Warren

2012-11-06 23:09 UTC #7 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

In reply to Marek Vasut

Quoted 13 lines · Marek Vasut

Dear Simon Glass,
Quoted 11 lines
Hi Marek,
Quoted 8 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work
Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows ...

Marek, I find this attitude a little ridiculous. If everything was fixed completely the first time around, there would be no work left to do; we'd just stop developing U-Boot. Equally, if this small addition to the USB keyboard code is so bad it can't be allowed since the whole driver must be re-written instead, why was the existing code allowed into U-Boot in the first place?

Incremental small patches are good; they allow small simple things to be implemented without causing massive disruption. That's great for locating any regressions.

Is there anything actually technically wrong with this specific patch? I would say no; it's very simple, non-invasive, low-risk, doesn't appear to introduce any long-term maintenance burden, doesn't completely prevent or remotely hinder reworking the USB keyboard support in the future, etc.

Marek Vasut

2012-11-07 13:18 UTC #8 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Dear Stephen Warren,

Quoted 22 lines · Marek Vasut

Quoted 13 lines
Dear Simon Glass,
Quoted 11 lines
Hi Marek,
Quoted 8 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work
Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows
...

Quoted 6 lines

Marek, I find this attitude a little ridiculous. If everything was fixed completely the first time around, there would be no work left to do; we'd just stop developing U-Boot. Equally, if this small addition to the USB keyboard code is so bad it can't be allowed since the whole driver must be re-written instead, why was the existing code allowed into U-Boot in the first place?

Because evil b*tch (=me) wasn't around in the first place ;-) Anyway ... I'll apply it (not because of your whining and stuff ... but because it's beneficial). Though, I'd be really glad if you invested time to rework the driver. Otherwise, it'll be me who'll end up doing the work and I'd prefer to delegate it to someone who brough the issue up sooner ;-)

Quoted 9 lines

Incremental small patches are good; they allow small simple things to be implemented without causing massive disruption. That's great for locating any regressions.
Is there anything actually technically wrong with this specific patch? I would say no; it's very simple, non-invasive, low-risk, doesn't appear to introduce any long-term maintenance burden, doesn't completely prevent or remotely hinder reworking the USB keyboard support in the future, etc.

Best regards,
Marek Vasut

Allen Martin

author

2012-11-07 19:02 UTC #9 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Quoted 31 lines · Marek Vasut

Dear Stephen Warren,
Quoted 22 lines · Marek Vasut
Quoted 13 lines
Dear Simon Glass,
Quoted 11 lines
Hi Marek,
Quoted 8 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work
Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows
...
Quoted 6 lines
Marek, I find this attitude a little ridiculous. If everything was fixed completely the first time around, there would be no work left to do; we'd just stop developing U-Boot. Equally, if this small addition to the USB keyboard code is so bad it can't be allowed since the whole driver must be re-written instead, why was the existing code allowed into U-Boot in the first place?

Quoted 5 lines

Because evil b*tch (=me) wasn't around in the first place ;-) Anyway ... I'll apply it (not because of your whining and stuff ... but because it's beneficial). Though, I'd be really glad if you invested time to rework the driver. Otherwise, it'll be me who'll end up doing the work and I'd prefer to delegate it to someone who brough the issue up sooner ;-)

Thanks Marek, I'd be happy to rework this driver if no one else wants to do it. I just can't sign up to do it right now as there are some tegra specific things (like USB gadget support, and enabling thumb) that are more important to me and my employer to do first, and I only work on u-boot on the side so I have limited bandwidth.

It sounds like we're all in violent agreement that moving the driver to the input subsystem is the right thing to do, and if someone is eager to work on it before I have a chance to I'm happy to review patches.

Quoted 5 lines

Quoted 5 lines
Incremental small patches are good; they allow small simple things to be implemented without causing massive disruption. That's great for locating any regressions.

Quoted 8 lines

Quoted 5 lines
Is there anything actually technically wrong with this specific patch? I would say no; it's very simple, non-invasive, low-risk, doesn't appear to introduce any long-term maintenance burden, doesn't completely prevent or remotely hinder reworking the USB keyboard support in the future, etc.
Best regards,
Marek Vasut

-Allen

Signature · 1 line

nvpublic

Marek Vasut

2012-11-08 01:19 UTC #10 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

Dear Allen Martin,

Quoted 40 lines · Marek Vasut

Quoted 32 lines
Dear Stephen Warren,
Quoted 23 lines · Marek Vasut
Quoted 13 lines
Dear Simon Glass,
Quoted 11 lines
Hi Marek,
Quoted 8 lines · Marek Vasut
Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
If it's the real fix, then why not go for the real fix right away? :-(
Because it's a fair chunk of work
Let's either do it properly or not at all ... if I let you do these semi- complete fixes, we'll end up with a stinking pile of crap like windows ...
Quoted 6 lines
Marek, I find this attitude a little ridiculous. If everything was fixed completely the first time around, there would be no work left to do; we'd just stop developing U-Boot. Equally, if this small addition to the USB keyboard code is so bad it can't be allowed since the whole driver must be re-written instead, why was the existing code allowed into U-Boot in the first place?
Quoted 6 lines
Because evil b*tch (=me) wasn't around in the first place ;-) Anyway ... I'll apply it (not because of your whining and stuff ... but because it's beneficial). Though, I'd be really glad if you invested time to rework the driver. Otherwise, it'll be me who'll end up doing the work and I'd prefer to delegate it to someone who brough the issue up sooner ;-)

Quoted 5 lines

Thanks Marek, I'd be happy to rework this driver if no one else wants to do it. I just can't sign up to do it right now as there are some tegra specific things (like USB gadget support, and enabling thumb) that are more important to me and my employer to do first, and I only work on u-boot on the side so I have limited bandwidth.

Honestly ... I think you should talk to your employer :-(

Quoted 4 lines

It sounds like we're all in violent agreement that moving the driver to the input subsystem is the right thing to do, and if someone is eager to work on it before I have a chance to I'm happy to review patches.

I suspect your employer needs to understand that it's imperative to get properly involved in project and not just throw scraps over the fence. Please understand that every open-source project quality is only as high as the quality of all the contributors ... so if you only throw scraps at u-boot, it will be a pile of scraps (and it'll be really crappy) ... on the other hand, if you contribute properly and cherish the code, it'll be really a good and thriving project. I'm pretty sure your employer already understands that you're getting a lot of code for free and contributing back would be a really nice way of kind-of payback.

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
Incremental small patches are good; they allow small simple things to be implemented without causing massive disruption. That's great for locating any regressions.
Is there anything actually technically wrong with this specific patch? I would say no; it's very simple, non-invasive, low-risk, doesn't appear to introduce any long-term maintenance burden, doesn't completely prevent or remotely hinder reworking the USB keyboard support in the future, etc.

Quoted 4 lines

Quoted 2 lines
Best regards,
Marek Vasut
-Allen

Best regards,
Marek Vasut

Allen Martin

author

2012-11-06 22:55 UTC #11 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

In reply to Marek Vasut

Quoted 10 lines · Marek Vasut

Dear Allen Martin,
Quoted 5 lines
Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue
If it's the real fix, then why not go for the real fix right away? :-(

It would be a lot more work to do that. This patch just adds support for 4 new keys without rocking the boat too much. I wasn't quite ready to signup to porting the whole driver over as there's a bunch things I want to work on inside tegra first.

-Allen

Signature · 1 line

nvpublic

Simon Glass

2012-11-06 22:52 UTC #12 lore raw

PATCH 1/1 USB: add arrow key support to usb_kbd

In reply to Allen Martin

Quoted 8 lines · Allen Martin

Check for scancodes for arrow keys and map them to ^F/^B, ^N/^P. Control characters are used instead of ANSI sequence because the queueing code in usb_kbd doesn't handle the data increase when one keypress generates 3 keycodes. The real fix is to convert this driver to use the input subsystem and queue, but this allows arrow keys to work until this driver is converted.
Signed-off-by Allen Martin <[email protected]>

Tested-by Simon Glass <[email protected]>

Acked-by Simon Glass <[email protected]>

Quoted 38 lines

---
 common/usb_kbd.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

common/usb_kbd.c +13 -1

index 24467ce..4efbcfe 100644
@@ -94,6 +94,15 @@ static const unsigned char usb_kbd_num_keypad[] = {
 };
 
 /*
+ * map arrow keys to ^F/^B ^N/^P, can't really use the proper
+ * ANSI sequence for arrow keys because the queuing code breaks
+ * when a single keypress expands to 3 queue elements
+ */
+static const unsigned char usb_kbd_arrow[] = {
+       0x6, 0x2, 0xe, 0x10
+};
+
+/*
  * NOTE: It's important for the NUM, CAPS, SCROLL-lock bits to be in this
  *       order. See usb_kbd_setled() function!
  */

@@ -224,6 +233,10 @@ static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode,
                        keycode = usb_kbd_numkey[scancode - 0x1e];
        }
 
+       /* Arrow keys */
+       if ((scancode >= 0x4f) && (scancode <= 0x52))
+               keycode = usb_kbd_arrow[scancode - 0x4f];
+
        /* Numeric keypad */
        if ((scancode >= 0x54) && (scancode <= 0x67))
                keycode = usb_kbd_num_keypad[scancode - 0x54];
--

1.7.10.4