[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[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 <amartin@nvidia.com>
---
 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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Stephen Warren]

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>

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Marek Vasut]

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Marek Vasut]

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

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

-Allen
-- 
nvpublic

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Marek Vasut]

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

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

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.

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

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

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Marek Vasut]

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Allen Martin]

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

[U-Boot] [PATCH] USB: add arrow key support to usb_kbd

[Simon Glass]

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
>