On 29 October 2015 at 13:09, Simon Glass <s...@chromium.org> wrote: > Hi Hans, > > On 19 October 2015 at 02:34, Hans de Goede <hdego...@redhat.com> wrote: >> >> Hi, >> >> >> On 19-10-15 01:17, Simon Glass wrote: >>> >>> Hi Hans, >>> >>> On 12 September 2015 at 09:15, Hans de Goede <hdego...@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 <s...@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. >> >> >> I think that supporting multiple keyboards the way I've outlined >> above as "1)" should not be that hard. But I do not plan to make time >> for this anytime soon, and as such I can hardly ask you to do this. >> >> So I reluctantly agree to keep this as is (I was hoping the move >> to dm would fix this). >> >>> 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. >> >> >> I'm fine with "usb tree" showing things the wrong way on builds where >> unbinding is disabled. But if I remember the patch-set this thread is >> about correctly, it completely removed unbinding from the usb code. >> >> If you do a new version where unbinding is only skipped when compiled >> out then that is fine with me. > > OK, for now I'm going to apply just this patch from the series, to > unblock the input uclass. > > I'll then redo this series to allow the unbinding as, indeed, that is > what I want to happen too.
Applied to u-boot-dm. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot