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

[Simon Glass]

Hi Hans,

On 19 October 2015 at 02:34, Hans de Goede <hdegoede at 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 <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.
>
>
> 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.

Regards,
Simon