Re: [RFC] WIP: add usb keyboard driver

[Peter Mamonov <pmamonov@gmail.com>]

On Thu, 10 Sep 2015 09:38:15 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Excellent! This makes the framebuffer console support complete :)

I'm sorry to dissapoint you, but this implementation of the usb
keyboard driver seems to be wrong. The driver employs the get_report
request to poll the keyboard state via the control endpoint, however
the "Device Class Definition for Human Interface Devices (HID)" states
the following:
"This request is not intended to be used for polling the device state
on a regular basis. [...] The Interrupt In pipe should be used for
recurring Input reports."

I guess this is the source of the problems with some keyboards: 2 of 4
keyboards I've tested changed their state only on key down, but not
on key up.

So, according the standard, we need to poll a keyboard via interrupt
endpoint. However, current implementation of EHCI driver doesn't support
interrupt transactions: submit_int_msg() in ehci-hcd.c is a stub. I'll
try to implement it.

Peter

> 
> I have not much to say, the driver looks ok to me. I'll test it once I
> find time. Some smally comments inline.
> 
> On Wed, Sep 09, 2015 at 07:36:52PM +0300, Peter Mamonov wrote:
> > diff --git a/drivers/input/usb_kbd.c b/drivers/input/usb_kbd.c
> 
> Please add some copyright header and don't forget to put references in
> it where you got the code from.
> 
> > +	if (pressed == 1) {
> > +		if (scancode == NUM_LOCK) {
> > +			data->flags ^= USB_KBD_NUMLOCK;
> > +			return 1;
> > +		}
> > +
> > +		if (scancode == CAPS_LOCK) {
> > +			data->flags ^= USB_KBD_CAPSLOCK;
> > +			return 1;
> > +		}
> > +		if (scancode == SCROLL_LOCK) {
> > +			data->flags ^= USB_KBD_SCROLLLOCK;
> > +			return 1;
> > +		}
> > +	}
> > +
> > +	/* Report keycode if any */
> > +	if (keycode) {
> > +		pr_debug("%s: key pressed: '%c'\n", __FUNCTION__,
> > keycode);
> 
> Please use __func__ rather than __FUNCTION__
> 
> Sascha
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox