diff mbox

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

Message ID 1441732512-727-5-git-send-email-sjg@chromium.org
State Accepted
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass Sept. 8, 2015, 5:15 p.m. UTC
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>
---

 common/cmd_usb.c       | 12 ++++++------
 common/usb_kbd.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
 include/dm/uclass-id.h |  1 +
 3 files changed, 57 insertions(+), 8 deletions(-)

Comments

Marek Vasut Sept. 8, 2015, 6:33 p.m. UTC | #1
On Tuesday, September 08, 2015 at 07:15:11 PM, 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.

Hi,

Why don't you create an UCLASS for generic input device instead ?

But in general, looks pretty standard/OK.

Best regards,
Marek Vasut
Simon Glass Sept. 10, 2015, 2:45 a.m. UTC | #2
Hi Marek,

On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
> On Tuesday, September 08, 2015 at 07:15:11 PM, 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.
>
> Hi,
>
> Why don't you create an UCLASS for generic input device instead ?
>

I sent a series that does that later. My intent with this series is to
get something applied for this release.

> But in general, looks pretty standard/OK.

Regards,
Simon
Marek Vasut Sept. 10, 2015, 11:40 a.m. UTC | #3
On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
> > On Tuesday, September 08, 2015 at 07:15:11 PM, 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.
> > 
> > Hi,
> > 
> > Why don't you create an UCLASS for generic input device instead ?
> 
> I sent a series that does that later. My intent with this series is to
> get something applied for this release.

Hi!

Aren't we pretty much post-RC3 now ?

Best regards,
Marek Vasut
Simon Glass Sept. 11, 2015, 12:43 a.m. UTC | #4
Hi Marek,

On 10 September 2015 at 04:40, Marek Vasut <marex@denx.de> wrote:
> On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
>> > On Tuesday, September 08, 2015 at 07:15:11 PM, 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.
>> >
>> > Hi,
>> >
>> > Why don't you create an UCLASS for generic input device instead ?
>>
>> I sent a series that does that later. My intent with this series is to
>> get something applied for this release.
>
> Hi!
>
> Aren't we pretty much post-RC3 now ?

Yes. It's not critical and I am late - let's see what Hans says.

Regards,
Simon
Hans de Goede Sept. 11, 2015, 8:14 a.m. UTC | #5
Hi,

On 09/11/2015 02:43 AM, Simon Glass wrote:
> Hi Marek,
>
> On 10 September 2015 at 04:40, Marek Vasut <marex@denx.de> wrote:
>> On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
>>>> On Tuesday, September 08, 2015 at 07:15:11 PM, 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.
>>>>
>>>> Hi,
>>>>
>>>> Why don't you create an UCLASS for generic input device instead ?
>>>
>>> I sent a series that does that later. My intent with this series is to
>>> get something applied for this release.
>>
>> Hi!
>>
>> Aren't we pretty much post-RC3 now ?
>
> Yes. It's not critical and I am late - let's see what Hans says.

I have looking into the RFC patchset on my todo, not sure if I will
get around to it this weekend though, and after that I'm travelling
for a week. Even if I get around to testing this I would prefer for
this to be delayed to post v2015.10. I'm fine with the concept of
the set but this needs some careful testing.

Regards,

Hans
Hans de Goede Sept. 12, 2015, 3:15 p.m. UTC | #6
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.

Regards,

Hans



> ---
>
>   common/cmd_usb.c       | 12 ++++++------
>   common/usb_kbd.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
>   include/dm/uclass-id.h |  1 +
>   3 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> index 6874af7..665f8ec 100644
> --- a/common/cmd_usb.c
> +++ b/common/cmd_usb.c
> @@ -526,11 +526,14 @@ static void do_usb_start(void)
>
>   	/* Driver model will probe the devices as they are found */
>   #ifndef CONFIG_DM_USB
> -#ifdef CONFIG_USB_STORAGE
> +# ifdef CONFIG_USB_STORAGE
>   	/* try to recognize storage devices immediately */
>   	usb_stor_curr_dev = usb_stor_scan(1);
> -#endif
> -#endif
> +# endif
> +# ifdef CONFIG_USB_KEYBOARD
> +	drv_usb_kbd_init();
> +# endif
> +#endif /* !CONFIG_DM_USB */
>   #ifdef CONFIG_USB_HOST_ETHER
>   # ifdef CONFIG_DM_ETH
>   #  ifndef CONFIG_DM_USB
> @@ -541,9 +544,6 @@ static void do_usb_start(void)
>   	usb_ether_curr_dev = usb_host_eth_scan(1);
>   # endif
>   #endif
> -#ifdef CONFIG_USB_KEYBOARD
> -	drv_usb_kbd_init();
> -#endif
>   }
>
>   #ifdef CONFIG_DM_USB
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 0227024..8037ebf 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -398,7 +398,7 @@ static int usb_kbd_getc(struct stdio_dev *sdev)
>   }
>
>   /* probes the USB device dev for keyboard type. */
> -static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
> +static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
>   {
>   	struct usb_interface *iface;
>   	struct usb_endpoint_descriptor *ep;
> @@ -495,7 +495,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
>   	int error;
>
>   	/* Try probing the keyboard */
> -	if (usb_kbd_probe(dev, 0) != 1)
> +	if (usb_kbd_probe_dev(dev, 0) != 1)
>   		return -ENOENT;
>
>   	/* Register the keyboard */
> @@ -532,6 +532,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
>   	return 0;
>   }
>
> +#ifndef CONFIG_DM_USB
>   /* Search for keyboard and register it if found. */
>   int drv_usb_kbd_init(void)
>   {
> @@ -590,6 +591,7 @@ int drv_usb_kbd_init(void)
>   	/* No USB Keyboard found */
>   	return -1;
>   }
> +#endif
>
>   /* Deregister the keyboard. */
>   int usb_kbd_deregister(int force)
> @@ -621,3 +623,49 @@ int usb_kbd_deregister(int force)
>   	return 1;
>   #endif
>   }
> +
> +#ifdef CONFIG_DM_USB
> +
> +static int usb_kbd_probe(struct udevice *dev)
> +{
> +	struct usb_device *udev = dev_get_parentdata(dev);
> +	int ret;
> +
> +	ret = probe_usb_keyboard(udev);



> +
> +	return ret;
> +}
> +
> +static const struct udevice_id usb_kbd_ids[] = {
> +	{ .compatible = "usb-keyboard" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(usb_kbd) = {
> +	.name	= "usb_kbd",
> +	.id	= UCLASS_KEYBOARD,
> +	.of_match = usb_kbd_ids,
> +	.probe = usb_kbd_probe,
> +};
> +
> +/* TODO(sjg@chromium.org): Move this into a common location */
> +UCLASS_DRIVER(keyboard) = {
> +	.id		= UCLASS_KEYBOARD,
> +	.name		= "keyboard",
> +};
> +
> +static const struct usb_device_id kbd_id_table[] = {
> +	{
> +		.match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |
> +			USB_DEVICE_ID_MATCH_INT_SUBCLASS |
> +			USB_DEVICE_ID_MATCH_INT_PROTOCOL,
> +		.bInterfaceClass = USB_CLASS_HID,
> +		.bInterfaceSubClass = 1,
> +		.bInterfaceProtocol = 1,
> +	},
> +	{ }		/* Terminating entry */
> +};
> +
> +U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table);
> +
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 1eeec74..bab0025 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -36,6 +36,7 @@ enum uclass_id {
>   	UCLASS_I2C_EEPROM,	/* I2C EEPROM device */
>   	UCLASS_I2C_GENERIC,	/* Generic I2C device */
>   	UCLASS_I2C_MUX,		/* I2C multiplexer */
> +	UCLASS_KEYBOARD,	/* Keyboard input device */
>   	UCLASS_LED,		/* Light-emitting diode (LED) */
>   	UCLASS_LPC,		/* x86 'low pin count' interface */
>   	UCLASS_MASS_STORAGE,	/* Mass storage device */
>
Simon Glass Oct. 18, 2015, 11:17 p.m. UTC | #7
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.

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.

[snip]

Regards,
Simon
Hans de Goede Oct. 19, 2015, 8:34 a.m. UTC | #8
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
Simon Glass Oct. 29, 2015, 7:09 p.m. UTC | #9
Hi Hans,

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

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
Simon Glass Oct. 30, 2015, 8:24 p.m. UTC | #10
On 29 October 2015 at 13:09, Simon Glass <sjg@chromium.org> wrote:
> Hi Hans,
>
> On 19 October 2015 at 02:34, Hans de Goede <hdegoede@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@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.
>
> 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.
diff mbox

Patch

diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index 6874af7..665f8ec 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -526,11 +526,14 @@  static void do_usb_start(void)
 
 	/* Driver model will probe the devices as they are found */
 #ifndef CONFIG_DM_USB
-#ifdef CONFIG_USB_STORAGE
+# ifdef CONFIG_USB_STORAGE
 	/* try to recognize storage devices immediately */
 	usb_stor_curr_dev = usb_stor_scan(1);
-#endif
-#endif
+# endif
+# ifdef CONFIG_USB_KEYBOARD
+	drv_usb_kbd_init();
+# endif
+#endif /* !CONFIG_DM_USB */
 #ifdef CONFIG_USB_HOST_ETHER
 # ifdef CONFIG_DM_ETH
 #  ifndef CONFIG_DM_USB
@@ -541,9 +544,6 @@  static void do_usb_start(void)
 	usb_ether_curr_dev = usb_host_eth_scan(1);
 # endif
 #endif
-#ifdef CONFIG_USB_KEYBOARD
-	drv_usb_kbd_init();
-#endif
 }
 
 #ifdef CONFIG_DM_USB
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 0227024..8037ebf 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -398,7 +398,7 @@  static int usb_kbd_getc(struct stdio_dev *sdev)
 }
 
 /* probes the USB device dev for keyboard type. */
-static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
+static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
 {
 	struct usb_interface *iface;
 	struct usb_endpoint_descriptor *ep;
@@ -495,7 +495,7 @@  static int probe_usb_keyboard(struct usb_device *dev)
 	int error;
 
 	/* Try probing the keyboard */
-	if (usb_kbd_probe(dev, 0) != 1)
+	if (usb_kbd_probe_dev(dev, 0) != 1)
 		return -ENOENT;
 
 	/* Register the keyboard */
@@ -532,6 +532,7 @@  static int probe_usb_keyboard(struct usb_device *dev)
 	return 0;
 }
 
+#ifndef CONFIG_DM_USB
 /* Search for keyboard and register it if found. */
 int drv_usb_kbd_init(void)
 {
@@ -590,6 +591,7 @@  int drv_usb_kbd_init(void)
 	/* No USB Keyboard found */
 	return -1;
 }
+#endif
 
 /* Deregister the keyboard. */
 int usb_kbd_deregister(int force)
@@ -621,3 +623,49 @@  int usb_kbd_deregister(int force)
 	return 1;
 #endif
 }
+
+#ifdef CONFIG_DM_USB
+
+static int usb_kbd_probe(struct udevice *dev)
+{
+	struct usb_device *udev = dev_get_parentdata(dev);
+	int ret;
+
+	ret = probe_usb_keyboard(udev);
+
+	return ret;
+}
+
+static const struct udevice_id usb_kbd_ids[] = {
+	{ .compatible = "usb-keyboard" },
+	{ }
+};
+
+U_BOOT_DRIVER(usb_kbd) = {
+	.name	= "usb_kbd",
+	.id	= UCLASS_KEYBOARD,
+	.of_match = usb_kbd_ids,
+	.probe = usb_kbd_probe,
+};
+
+/* TODO(sjg@chromium.org): Move this into a common location */
+UCLASS_DRIVER(keyboard) = {
+	.id		= UCLASS_KEYBOARD,
+	.name		= "keyboard",
+};
+
+static const struct usb_device_id kbd_id_table[] = {
+	{
+		.match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |
+			USB_DEVICE_ID_MATCH_INT_SUBCLASS |
+			USB_DEVICE_ID_MATCH_INT_PROTOCOL,
+		.bInterfaceClass = USB_CLASS_HID,
+		.bInterfaceSubClass = 1,
+		.bInterfaceProtocol = 1,
+	},
+	{ }		/* Terminating entry */
+};
+
+U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table);
+
+#endif
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 1eeec74..bab0025 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -36,6 +36,7 @@  enum uclass_id {
 	UCLASS_I2C_EEPROM,	/* I2C EEPROM device */
 	UCLASS_I2C_GENERIC,	/* Generic I2C device */
 	UCLASS_I2C_MUX,		/* I2C multiplexer */
+	UCLASS_KEYBOARD,	/* Keyboard input device */
 	UCLASS_LED,		/* Light-emitting diode (LED) */
 	UCLASS_LPC,		/* x86 'low pin count' interface */
 	UCLASS_MASS_STORAGE,	/* Mass storage device */