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

[Simon Glass]

On 29 October 2015 at 13:09, Simon Glass <sjg at chromium.org> wrote:
> 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.

Applied to u-boot-dm.