Hi Hans,
On 12 September 2015 at 09:15, Hans de Goede hdegoede@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@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:
- 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
- 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