From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Mon, 19 Oct 2015 10:34:25 +0200 Subject: [U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model In-Reply-To: References: <1441732512-727-1-git-send-email-sjg@chromium.org> <1441732512-727-5-git-send-email-sjg@chromium.org> <55F441A1.7060905@redhat.com> Message-ID: <5624AB11.20302@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 19-10-15 01:17, Simon Glass wrote: > Hi Hans, > > On 12 September 2015 at 09:15, Hans de Goede 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 >> >> >> 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. Regards, Hans