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

[Simon Glass]

Hi Hans,

On 12 September 2015 at 09:15, Hans de Goede <hdegoede at redhat.com> wrote:
> 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.
>

I've had another look at this and here are my comments so far:

1. The existing driver does not support multiple keyboards. It
implements this limitation in multiple ways which would be a real pain
to fix while keeping the old code. I think it makes much more sense to
remove this limitation when we have either a) moved all uses of USB
keyboard to driver model, or perhaps b) moved stdio to driver model.
For now the driver model approach provides the same functionality as
before so I think it is fine.

2. The point about out-of-order devices in the 'usb tree'
display....well if you disable unbinding that is what you get. I'm
sure we could fix it by sorting the devices before displaying them,
but it does not seem that important to me. It is more likely that the
unbind support will be enabled in U-Boot proper, and perhaps disabled
in SPL, which doesn't have commands anyway.

[snip]

Regards,
Simon