[U-Boot] [PATCH 3/6] sunxi: video: Add cfb console driver for sunxi

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

Hi,

Thanks for the reviews!


On 11/16/2014 12:35 PM, Ian Campbell wrote:
> On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
>> +/* this is needed so the above will actually do something */
>> +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
>> [...] 
>> +#ifdef CONFIG_VIDEO
>> +#define CONSOLE_ENV_SETTINGS \
>> +	"stdin=serial\0" \
>> +	"stdout=serial,vga\0" \
>> +	"stderr=serial,vga\0"
>> +#else
>> +#define CONSOLE_ENV_SETTINGS
> 
> Now that CONFIG_SYS_CONSOLE_IS_IN_ENV is set do we need to either say
> something here and/or provide the overwrite_console() hook?

No, if we have a need for a overwrite_console() hook, we need to also
define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE, see common/console.c:

#ifdef CONFIG_SYS_CONSOLE_IS_IN_ENV
/*
 * if overwrite_console returns 1, the stdin, stderr and stdout
 * are switched to the serial port, else the settings in the
 * environment are used
 */
#ifdef CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE
extern int overwrite_console(void);
#define OVERWRITE_CONSOLE overwrite_console()
#else
#define OVERWRITE_CONSOLE 0
#endif /* CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE */

#endif /* CONFIG_SYS_CONSOLE_IS_IN_ENV */

> Perhaps something along the lines of:
>         #ifdef CONFIG_VIDEO
>         #define CONSOLE_ENV_VGA_SETTINGS ",vga"
>         #endif
>         
>         #define CONSOLE_ENV_SETTINGS \
>         	"stdin=serial\0" \
>         	"stdout=serial" CONSOLE_ENV_VGA_SETTINGS "\0" \
>         	"stderr=serial" CONSOLE_ENV_VGA_SETTINGS "\0"

There is no need to set these when CONFIG_VIDEO is not defined
since CONFIG_SYS_CONSOLE_IS_IN_ENV only gets set when
CONFIG_VIDEO is set, at least in this point in the patch set.

Once we add usb-keyboard support I want to support usb-keyb. support,
without having it depend on CONFIG_VIDEO (even though it makes the
most sense with CONFIG_VIDEO).

But even then I prefer this style:

#ifdef CONFIG_VIDEO
#define CONSOLE_OUT_SETTINGS \
        "stdout=serial,vga\0" \
        "stderr=serial,vga\0"
#else
#define CONSOLE_OUT_SETTINGS \
        "stdout=serial\0" \
        "stderr=serial\0"
#endif

Over the pre-processor magic you're suggesting, as it is more
readable IMHO, also it matches what we're do for
#ifdef CONFIG_MMC, #ifdef CONFIG_AHCI, etc.

> Has this been tested on a serial-only board?

Erm, good question.

So the A13 olinuxino boards do not have hdmi, which means I should
add CONFIG_VIDEO=n to their defconfigs, fixed in my local tree.

After that I've just ran some tests on an A13 board, which work fine
(as expected, but you're right this ought to be tested).

Regards,

Hans