diff mbox

[U-Boot,2/4] sunxi: Avoid any assumption between musb gadget and host but fallback to host

Message ID 1426440467-4525-2-git-send-email-contact@paulk.fr
State Superseded
Delegated to: Hans de Goede
Headers show

Commit Message

Paul Kocialkowski March 15, 2015, 5:27 p.m. UTC
musb might be used in gadget mode on sunxi, so we don't want to assume anything
related to host mode, especially USB keyboard support.
However, in case gadget mode is not explicitly used, fallback to host mode.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 board/sunxi/Kconfig            | 2 +-
 include/configs/sunxi-common.h | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Hans de Goede March 21, 2015, 1:14 p.m. UTC | #1
Hi,

On 15-03-15 18:27, Paul Kocialkowski wrote:
> musb might be used in gadget mode on sunxi, so we don't want to assume anything
> related to host mode, especially USB keyboard support.
> However, in case gadget mode is not explicitly used, fallback to host mode.
>
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>   board/sunxi/Kconfig            | 2 +-
>   include/configs/sunxi-common.h | 5 ++++-
>   2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> index 9d0eb91..0b6a492 100644
> --- a/board/sunxi/Kconfig
> +++ b/board/sunxi/Kconfig
> @@ -436,7 +436,7 @@ config USB_MUSB_SUNXI
>
>   config USB_KEYBOARD
>   	boolean "Enable USB keyboard support"
> -	default y
> +	default n
>   	---help---
>   	Say Y here to add support for using a USB keyboard (typically used
>   	in combination with a graphical console).

You've just disabled the usb keyboard support pretty much everywhere, please
do not do that.

> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 1f7a1cb..ffd9f5c 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -297,13 +297,16 @@ extern int soft_i2c_gpio_scl;
>   #endif
>
>   #ifdef CONFIG_USB_MUSB_SUNXI
> +#ifndef CONFIG_MUSB_GADGET
>   #define CONFIG_MUSB_HOST
> +#endif
>   #define CONFIG_MUSB_PIO_ONLY
>   #endif
>
> -#if defined CONFIG_USB_EHCI || defined CONFIG_USB_MUSB_SUNXI
> +#if defined CONFIG_USB_EHCI || defined CONFIG_MUSB_HOST
>   #define CONFIG_CMD_USB
>   #define CONFIG_USB_STORAGE
> +#define CONFIG_USB_KEYBOARD


And here you are overriding something set by Kconfig which is a big no no,
if you want to disable the keyboard support on boards where you use
the oth port in gadget mode jsut add:
CONFIG_USB_KEYBOARD=n to the (def)config.

Regards,

Hans
Paul Kocialkowski March 22, 2015, 10:54 a.m. UTC | #2
Le samedi 21 mars 2015 à 14:14 +0100, Hans de Goede a écrit :
> Hi,
> 
> On 15-03-15 18:27, Paul Kocialkowski wrote:
> > musb might be used in gadget mode on sunxi, so we don't want to assume anything
> > related to host mode, especially USB keyboard support.
> > However, in case gadget mode is not explicitly used, fallback to host mode.
> >
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >   board/sunxi/Kconfig            | 2 +-
> >   include/configs/sunxi-common.h | 5 ++++-
> >   2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> > index 9d0eb91..0b6a492 100644
> > --- a/board/sunxi/Kconfig
> > +++ b/board/sunxi/Kconfig
> > @@ -436,7 +436,7 @@ config USB_MUSB_SUNXI
> >
> >   config USB_KEYBOARD
> >   	boolean "Enable USB keyboard support"
> > -	default y
> > +	default n
> >   	---help---
> >   	Say Y here to add support for using a USB keyboard (typically used
> >   	in combination with a graphical console).
> 
> You've just disabled the usb keyboard support pretty much everywhere, please
> do not do that.
> 
> > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> > index 1f7a1cb..ffd9f5c 100644
> > --- a/include/configs/sunxi-common.h
> > +++ b/include/configs/sunxi-common.h
> > @@ -297,13 +297,16 @@ extern int soft_i2c_gpio_scl;
> >   #endif
> >
> >   #ifdef CONFIG_USB_MUSB_SUNXI
> > +#ifndef CONFIG_MUSB_GADGET
> >   #define CONFIG_MUSB_HOST
> > +#endif
> >   #define CONFIG_MUSB_PIO_ONLY
> >   #endif
> >
> > -#if defined CONFIG_USB_EHCI || defined CONFIG_USB_MUSB_SUNXI
> > +#if defined CONFIG_USB_EHCI || defined CONFIG_MUSB_HOST
> >   #define CONFIG_CMD_USB
> >   #define CONFIG_USB_STORAGE
> > +#define CONFIG_USB_KEYBOARD
> 
> 
> And here you are overriding something set by Kconfig which is a big no no,
> if you want to disable the keyboard support on boards where you use
> the oth port in gadget mode jsut add:
> CONFIG_USB_KEYBOARD=n to the (def)config.

I understand that the logic is flawed here (disable a Kconfig option in
Kconfig and enable it back in the config header, which is weaker), but I
really don't think it should always be enabled.

IMO the best solution would be to remove USB_KEYBOARD from Kconfig
entirely and enable it in sunxi-common.h as I suggested. This way, we
don't even have to explicitely disable it for devices that don't have
USB support (MSI_Primo81/MSI_Primo73): it will only get enabled when
EHCI or host MUSB support is enabled.

I fail to see the point of using Kconfig here anyway, since nothing
depends on it (in the Kconfig sense) and other platforms have been using
CONFIG_USB_KEYBOARD in the config files without ever needing to have it
in Kconfig. In addition, it doesn't seem very relevant to me to have
that Kconfig option defined in the sunxi part of the code, while
USB_KEYBOARD is also used on other SoCs.

Please let me know what you think here, I'd be glad to make a patch
fixing this behaviour.
diff mbox

Patch

diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 9d0eb91..0b6a492 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -436,7 +436,7 @@  config USB_MUSB_SUNXI
 
 config USB_KEYBOARD
 	boolean "Enable USB keyboard support"
-	default y
+	default n
 	---help---
 	Say Y here to add support for using a USB keyboard (typically used
 	in combination with a graphical console).
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 1f7a1cb..ffd9f5c 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -297,13 +297,16 @@  extern int soft_i2c_gpio_scl;
 #endif
 
 #ifdef CONFIG_USB_MUSB_SUNXI
+#ifndef CONFIG_MUSB_GADGET
 #define CONFIG_MUSB_HOST
+#endif
 #define CONFIG_MUSB_PIO_ONLY
 #endif
 
-#if defined CONFIG_USB_EHCI || defined CONFIG_USB_MUSB_SUNXI
+#if defined CONFIG_USB_EHCI || defined CONFIG_MUSB_HOST
 #define CONFIG_CMD_USB
 #define CONFIG_USB_STORAGE
+#define CONFIG_USB_KEYBOARD
 #endif
 
 #ifdef CONFIG_USB_KEYBOARD