[U-Boot] [PATCH 6/6] sunxi: Add usb keyboard Kconfig option

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

Hi,

On 11/16/2014 02:37 PM, Ian Campbell wrote:
> On Sun, 2014-11-16 at 14:28 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/16/2014 12:55 PM, Ian Campbell wrote:
>>> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>>>> For use together with the hdmi console.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  board/sunxi/Kconfig            |  7 +++++++
>>>>  configs/Ippo_q8h_v5_defconfig  |  1 +
>>>>  include/configs/sunxi-common.h | 25 +++++++++++++++++++++----
>>>>  3 files changed, 29 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
>>>> index 422033a..246cd9a 100644
>>>> --- a/board/sunxi/Kconfig
>>>> +++ b/board/sunxi/Kconfig
>>>> @@ -223,4 +223,11 @@ config VIDEO
>>>>  	Say Y here to add support for using a cfb console on the HDMI output
>>>>  	found on most sunxi devices.
>>>>  
>>>> +config USB_KEYBOARD
>>>
>>> This seems like it ought to be under drivers/ somewhere, either
>>> drivers/usb/Kconfig or drivers/input/Kconfig perhaps.
>>
>> You're right the problem with that is, that what we really need is
>> to Kconfig-ify the entire USB stuff (eg also CONFIG_USB_EHCI), but that
>> is a bit more then I was planning on working on atm, esp. since that
>> touches many many boards. So this seems best for now.
> 
> OK, is the USB custodian OK with this plan?

Good question, I've just dropped Marek a mail on this with you in the CC.

>>>> +	boolean "Enable USB keyboard support"
>>>> +	default y
>>>> +	---help---
>>>> +	Say Y here to add support for using a USB keyboard (typically used
>>>> +	in combination with a graphical console on HDMI).
>>>> +
>>>>  endif
>>>> diff --git a/configs/Ippo_q8h_v5_defconfig b/configs/Ippo_q8h_v5_defconfig
>>>> index 53df213..50c2f93 100644
>>>> --- a/configs/Ippo_q8h_v5_defconfig
>>>> +++ b/configs/Ippo_q8h_v5_defconfig
>>>> @@ -5,3 +5,4 @@ CONFIG_MACH_SUN8I=y
>>>>  CONFIG_TARGET_IPPO_Q8H_V5=y
>>>>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-a23-ippo-q8h-v5.dtb"
>>>>  CONFIG_VIDEO=n
>>>> +CONFIG_USB_KEYBOARD=n
>>>
>>> Is this the only platform with video+usb which you have? What about e.g.
>>> cubie*, bananapi etc?
>>
>> You mean without video + usb, since the default is y, and it is being
>> forced to n here.
> 
> Sorry, I read this backwards somehow!
> 
>> No this is not the only board we support without HDMI,
>> I has forgotten about the A13 boards (fixed locally now), this is the
>> only board without a host usb connector (it is a tablet), it does support
>> otg, but we do not support that yet.
>>
>> I've also made a local change to disable CONFIG_USB_KEYBOARD by default
>> in the A13 boards since although it does work there it makes little
>> sense to have it when their is no video out support.
> 
> Sounds good.

Ok.

>>>> @@ -298,17 +304,28 @@
>>>>  
>>>>  #include <config_distro_bootcmd.h>
>>>>  
>>>> +#ifdef CONFIG_USB_KEYBOARD
>>>> +#define CONSOLE_IN_SETTINGS \
>>>> +	"preboot=usb start\0" \
>>>> +	"stdin=serial,usbkbd\0"
>>>> +#else
>>>> +#define CONSOLE_IN_SETTINGS \
>>>> +	"stdin=serial\0"
>>>> +#endif
>>>> +
>>>>  #ifdef CONFIG_VIDEO
>>>> -#define CONSOLE_ENV_SETTINGS \
>>>> -	"stdin=serial\0" \
>>>> +#define CONSOLE_OUT_SETTINGS \
>>>>  	"stdout=serial,vga\0" \
>>>>  	"stderr=serial,vga\0"
>>>>  #else
>>>> -#define CONSOLE_ENV_SETTINGS
>>>> +#define CONSOLE_OUT_SETTINGS \
>>>> +	"stdout=serial\0" \
>>>> +	"stderr=serial\0"
>>>
>>> Ah, here are the settings I asked about earlier. Are these in the wrong
>>> patch or did something change in this patch which makes them needed only
>>> now?
>>
>> These are only necessary when CONFIG_SYS_CONSOLE_IS_IN_ENV is set, which
>> sofar was only happening when CONFIG_VIDEO is set. In this patch I'm setting
>> them always to also support just CONFIG_USB_KEYBOARD without CONFIG_VIDEO,
>> which is a bit weird, but I do not want to disallow it.
>>
>> This does mean that we should also unconditionally enable
>> CONFIG_SYS_CONSOLE_IS_IN_ENV with this patch (moving it out of the
>> #ifdef CONFIG_VIDEO) block, which I've not done, I'll fix this locally.
> 
> OK. Not sure what you were planning but it may makes sense to fold some
> portion of that change into the earlier patch.

Yeah, I'll go move things around a bit then post a v2 of the last 4 patches
of this set.

Regards,

Hans