| Message ID | 1352237163-26765-1-git-send-email-amartin@nvidia.com |
|---|---|
| State | Accepted |
| Delegated to: | Marek Vasut |
| Headers | show |
On 11/06/2012 02:26 PM, Allen Martin wrote: > 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 <swarren@nvidia.com>
Dear 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 If it's the real fix, then why not go for the real fix right away? :-( > but this allows arrow keys to > work until this driver is converted. > > Signed-off-by: Allen Martin <amartin@nvidia.com> > --- [...] Best regards, Marek Vasut
Hi Marek, On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > Dear 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 > > 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 > >> but this allows arrow keys to >> work until this driver is converted. >> >> Signed-off-by: Allen Martin <amartin@nvidia.com> >> --- > [...] > > Best regards, > Marek Vasut
On Tue, Nov 6, 2012 at 1:26 PM, Allen Martin <amartin@nvidia.com> wrote: > 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 <amartin@nvidia.com> Tested-by: Simon Glass <sjg@chromium.org> Acked-by: Simon Glass <sjg@chromium.org> > --- > common/usb_kbd.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > index 24467ce..4efbcfe 100644 > --- a/common/usb_kbd.c > +++ b/common/usb_kbd.c > @@ -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 >
On Tue, Nov 06, 2012 at 02:49:29PM -0800, Marek Vasut wrote: > Dear 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 > > 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
Dear Simon Glass, > Hi Marek, > > On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > > Dear 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 > > > > 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 ... > and also if we are doing USB we > should probably do keyboard.c first. Uh, I don't follow. > USB would not be the first > priority since so much of the logic / keycodes are different. Sorry, I'm completely lost. > Regards, > Simon > > >> but this allows arrow keys to > >> work until this driver is converted. > >> > >> Signed-off-by: Allen Martin <amartin@nvidia.com> > >> --- > > > > [...] > > > > Best regards, > > Marek Vasut Best regards, Marek Vasut
On Tue, Nov 06, 2012 at 02:56:37PM -0800, Marek Vasut wrote: > Dear Simon Glass, > > > Hi Marek, > > > > On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > > > Dear 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 > > > > > > 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
On 11/06/2012 03:56 PM, Marek Vasut wrote: > Dear Simon Glass, > >> Hi Marek, >> >> On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: >>> Dear 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 >>> >>> 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.
Dear Allen Martin, > On Tue, Nov 06, 2012 at 02:56:37PM -0800, Marek Vasut wrote: > > Dear Simon Glass, > > > > > Hi Marek, > > > > > > On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > > > > Dear 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 > > > > > > > > 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. 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
Dear Stephen Warren, > On 11/06/2012 03:56 PM, Marek Vasut wrote: > > Dear Simon Glass, > > > >> Hi Marek, > >> > >> On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > >>> Dear 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 > >>> > >>> 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? 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 ;-) > 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
On Wed, Nov 07, 2012 at 05:18:27AM -0800, Marek Vasut wrote: > Dear Stephen Warren, > > > On 11/06/2012 03:56 PM, Marek Vasut wrote: > > > Dear Simon Glass, > > > > > >> Hi Marek, > > >> > > >> On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > > >>> Dear 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 > > >>> > > >>> 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? > > 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. > > > 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
Dear Allen Martin, > On Wed, Nov 07, 2012 at 05:18:27AM -0800, Marek Vasut wrote: > > Dear Stephen Warren, > > > > > On 11/06/2012 03:56 PM, Marek Vasut wrote: > > > > Dear Simon Glass, > > > > > > > >> Hi Marek, > > > >> > > > >> On Tue, Nov 6, 2012 at 2:49 PM, Marek Vasut <marex@denx.de> wrote: > > > >>> Dear 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 > > > >>> > > > >>> 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? > > > > 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. Honestly ... I think you should talk to your employer :-( > 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. > > > 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 Best regards, Marek Vasut
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 24467ce..4efbcfe 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -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];
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 <amartin@nvidia.com> --- common/usb_kbd.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)