[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Hans de Goede]

Hi,

On 08-09-15 19:15, Simon Glass wrote:
> Switch USB keyboards over to use driver model instead of scanning with the
> horrible usb_get_dev_index() function. This involves creating a new uclass
> for keyboards, although so far there is no API.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>

In general I like this patch, so ack for the principle, but the
implementation has issues.

You're now allowing the registration of multiple usb keyb stdio
input devices, but you are still relying on usb_kbd_deregister()
to remove them from the stdio devices list, and when multiple
or used that one will only remove the first one.

This can be fixed by switching to stdio_register_dev, and
store the returned struct stdio_dev pointer for the new dev,
and add a dm remove callback which deregisters that specific
stdio_dev that will also remove the ugliness of looking up
the device by its name to unregister it.

The name which in itself is another, harder to fix issue,
when using iomux, and the stdin string contains usbkbd we
really want to get all usbkbd-s to work, but iomux will
only take the first one.

This can be fixed in 2 ways:

1) in the usbkbd driver by registering a shared stdio_dev
for all usbkbd's and deregistering that only when the
last usbkbd is removed (in the case of dm), this will
require a global list of usbkbd devices, and stdio
callbacks to walk over this list, I believe that this
is likely the best approach

2) Fix this in iomux, and make it look for multiple
devs with the same name and mux them all.

This seems cleaner at a conceptual level, but likely
somewhat hard to implement.

Regards,

Hans



> ---
>
>   common/cmd_usb.c       | 12 ++++++------
>   common/usb_kbd.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
>   include/dm/uclass-id.h |  1 +
>   3 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> index 6874af7..665f8ec 100644
> --- a/common/cmd_usb.c
> +++ b/common/cmd_usb.c
> @@ -526,11 +526,14 @@ static void do_usb_start(void)
>
>   	/* Driver model will probe the devices as they are found */
>   #ifndef CONFIG_DM_USB
> -#ifdef CONFIG_USB_STORAGE
> +# ifdef CONFIG_USB_STORAGE
>   	/* try to recognize storage devices immediately */
>   	usb_stor_curr_dev = usb_stor_scan(1);
> -#endif
> -#endif
> +# endif
> +# ifdef CONFIG_USB_KEYBOARD
> +	drv_usb_kbd_init();
> +# endif
> +#endif /* !CONFIG_DM_USB */
>   #ifdef CONFIG_USB_HOST_ETHER
>   # ifdef CONFIG_DM_ETH
>   #  ifndef CONFIG_DM_USB
> @@ -541,9 +544,6 @@ static void do_usb_start(void)
>   	usb_ether_curr_dev = usb_host_eth_scan(1);
>   # endif
>   #endif
> -#ifdef CONFIG_USB_KEYBOARD
> -	drv_usb_kbd_init();
> -#endif
>   }
>
>   #ifdef CONFIG_DM_USB
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 0227024..8037ebf 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -398,7 +398,7 @@ static int usb_kbd_getc(struct stdio_dev *sdev)
>   }
>
>   /* probes the USB device dev for keyboard type. */
> -static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
> +static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
>   {
>   	struct usb_interface *iface;
>   	struct usb_endpoint_descriptor *ep;
> @@ -495,7 +495,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
>   	int error;
>
>   	/* Try probing the keyboard */
> -	if (usb_kbd_probe(dev, 0) != 1)
> +	if (usb_kbd_probe_dev(dev, 0) != 1)
>   		return -ENOENT;
>
>   	/* Register the keyboard */
> @@ -532,6 +532,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
>   	return 0;
>   }
>
> +#ifndef CONFIG_DM_USB
>   /* Search for keyboard and register it if found. */
>   int drv_usb_kbd_init(void)
>   {
> @@ -590,6 +591,7 @@ int drv_usb_kbd_init(void)
>   	/* No USB Keyboard found */
>   	return -1;
>   }
> +#endif
>
>   /* Deregister the keyboard. */
>   int usb_kbd_deregister(int force)
> @@ -621,3 +623,49 @@ int usb_kbd_deregister(int force)
>   	return 1;
>   #endif
>   }
> +
> +#ifdef CONFIG_DM_USB
> +
> +static int usb_kbd_probe(struct udevice *dev)
> +{
> +	struct usb_device *udev = dev_get_parentdata(dev);
> +	int ret;
> +
> +	ret = probe_usb_keyboard(udev);



> +
> +	return ret;
> +}
> +
> +static const struct udevice_id usb_kbd_ids[] = {
> +	{ .compatible = "usb-keyboard" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(usb_kbd) = {
> +	.name	= "usb_kbd",
> +	.id	= UCLASS_KEYBOARD,
> +	.of_match = usb_kbd_ids,
> +	.probe = usb_kbd_probe,
> +};
> +
> +/* TODO(sjg at chromium.org): Move this into a common location */
> +UCLASS_DRIVER(keyboard) = {
> +	.id		= UCLASS_KEYBOARD,
> +	.name		= "keyboard",
> +};
> +
> +static const struct usb_device_id kbd_id_table[] = {
> +	{
> +		.match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |
> +			USB_DEVICE_ID_MATCH_INT_SUBCLASS |
> +			USB_DEVICE_ID_MATCH_INT_PROTOCOL,
> +		.bInterfaceClass = USB_CLASS_HID,
> +		.bInterfaceSubClass = 1,
> +		.bInterfaceProtocol = 1,
> +	},
> +	{ }		/* Terminating entry */
> +};
> +
> +U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table);
> +
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 1eeec74..bab0025 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -36,6 +36,7 @@ enum uclass_id {
>   	UCLASS_I2C_EEPROM,	/* I2C EEPROM device */
>   	UCLASS_I2C_GENERIC,	/* Generic I2C device */
>   	UCLASS_I2C_MUX,		/* I2C multiplexer */
> +	UCLASS_KEYBOARD,	/* Keyboard input device */
>   	UCLASS_LED,		/* Light-emitting diode (LED) */
>   	UCLASS_LPC,		/* x86 'low pin count' interface */
>   	UCLASS_MASS_STORAGE,	/* Mass storage device */
>