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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 19-10-15 01:17, Simon Glass wrote:
> 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:
>>
>> 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