[U-Boot] [PATCH 1/4] video: cfb_console: Remove the unnecessary CONFIG_VGA_AS_SINGLE_DEVICE wraps

[Simon Glass]

Hi Bin,

On 8 May 2015 at 22:26, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Sat, May 9, 2015 at 5:44 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 6 May 2015 at 03:34, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> There are two places in the cfb_console driver that test whether
>>> CONFIG_VGA_AS_SINGLE_DEVICE is defined or not, but actually it is
>>> unnecessary, hence clean it up.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>> ---
>>>
>>>  drivers/video/cfb_console.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
>>> index f4231b8..fcaaa7f 100644
>>> --- a/drivers/video/cfb_console.c
>>> +++ b/drivers/video/cfb_console.c
>>> @@ -2274,9 +2274,7 @@ int drv_video_init(void)
>>>  #endif
>>>         if (have_keyboard) {
>>>                 debug("KBD: Keyboard init ...\n");
>>> -#if !defined(CONFIG_VGA_AS_SINGLE_DEVICE)
>>>                 skip_dev_init |= (VIDEO_KBD_INIT_FCT == -1);
>>> -#endif
>>>         }
>>>         if (skip_dev_init)
>>>                 return 0;
>>> @@ -2289,14 +2287,12 @@ int drv_video_init(void)
>>>         console_dev.putc = video_putc;  /* 'putc' function */
>>>         console_dev.puts = video_puts;  /* 'puts' function */
>>>
>>> -#if !defined(CONFIG_VGA_AS_SINGLE_DEVICE)
>>>         if (have_keyboard) {
>>>                 /* Also init console device */
>>>                 console_dev.flags |= DEV_FLAGS_INPUT;
>>>                 console_dev.tstc = VIDEO_TSTC_FCT;      /* 'tstc' function */
>>>                 console_dev.getc = VIDEO_GETC_FCT;      /* 'getc' function */
>>>         }
>>> -#endif
>>>
>>>         if (stdio_register(&console_dev) != 0)
>>>                 return 0;
>>> --
>>> 1.8.2.1
>>>
>>
>> This is needed by some boards, e.g. (just a sample):
>>
>> 02: video: cfb_console: Remove the unnecessary CONFIG_VGA_AS_SINGLE_DEVICE wraps
>>        arm:  +   wandboard_quad wandboard_dl wandboard_solo cm_fx6
>> +../drivers/video/cfb_console.c: In function ‘drv_video_init’:
>> +../drivers/video/cfb_console.c:2277:21: error: ‘VIDEO_KBD_INIT_FCT’
>> undeclared (first use in this function)
>> +../drivers/video/cfb_console.c:2277:21: note: each undeclared
>> identifier is reported only once for each function it appears in
>> +../drivers/video/cfb_console.c:2293:22: error: ‘VIDEO_TSTC_FCT’
>> undeclared (first use in this function)
>> +../drivers/video/cfb_console.c:2294:22: error: ‘VIDEO_GETC_FCT’
>> undeclared (first use in this function)
>> +make[3]: *** [drivers/video/cfb_console.o] Error 1
>> +make[2]: *** [drivers/video] Error 2
>> +make[1]: *** [drivers] Error 2
>> +make: *** [sub-make] Error 2
>>
>
> Sorry, I failed to check this CONFIG_VGA_AS_SINGLE_DEVICE is actually
> defined by so many boards. But I still see the logic is not straight
> forward, due to the variable 'have_keyboard' is equal to
> '!defined(CONFIG_VGA_AS_SINGLE_DEVICE)'. Can we do it like this:

Well the presence of a keyboard is also controlled by device tree. If
this moved to Kconfig then we could use if() instead of #ifdef which
would be better.

>
> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> index f4231b8..640f0a8 100644
> --- a/drivers/video/cfb_console.c
> +++ b/drivers/video/cfb_console.c
> @@ -2252,7 +2252,6 @@ int drv_video_init(void)
>  {
>         int skip_dev_init;
>         struct stdio_dev console_dev;
> -       bool have_keyboard;
>
>         /* Check if video initialization should be skipped */
>         if (board_video_skip())
> @@ -2264,20 +2263,10 @@ int drv_video_init(void)
>         if (board_cfb_skip())
>                 return 0;
>
> -#if defined(CONFIG_VGA_AS_SINGLE_DEVICE)
> -       have_keyboard = false;
> -#elif defined(CONFIG_OF_CONTROL)
> -       have_keyboard = !fdtdec_get_config_bool(gd->fdt_blob,
> -                                               "u-boot,no-keyboard");
> -#else
> -       have_keyboard = true;
> -#endif
> -       if (have_keyboard) {
> -               debug("KBD: Keyboard init ...\n");
>  #if !defined(CONFIG_VGA_AS_SINGLE_DEVICE)
> -               skip_dev_init |= (VIDEO_KBD_INIT_FCT == -1);
> +       debug("KBD: Keyboard init ...\n");
> +       skip_dev_init |= (VIDEO_KBD_INIT_FCT == -1);
>  #endif
> -       }
>         if (skip_dev_init)
>                 return 0;
>
> @@ -2290,12 +2279,10 @@ int drv_video_init(void)
>         console_dev.puts = video_puts;  /* 'puts' function */
>
>  #if !defined(CONFIG_VGA_AS_SINGLE_DEVICE)
> -       if (have_keyboard) {
> -               /* Also init console device */
> -               console_dev.flags |= DEV_FLAGS_INPUT;
> -               console_dev.tstc = VIDEO_TSTC_FCT;      /* 'tstc' function */
> -               console_dev.getc = VIDEO_GETC_FCT;      /* 'getc' function */
> -       }
> +       /* Also init console device */
> +       console_dev.flags |= DEV_FLAGS_INPUT;
> +       console_dev.tstc = VIDEO_TSTC_FCT;      /* 'tstc' function */
> +       console_dev.getc = VIDEO_GETC_FCT;      /* 'getc' function */
>  #endif
>
>         if (stdio_register(&console_dev) != 0)
>
>
> Also I checked include/configs/nokia_rx51.h, it has the following 3
> defined but not actually hooked into the cfb_console.c
>
> #define VIDEO_KBD_INIT_FCT              rx51_kp_init()
> #define VIDEO_TSTC_FCT                  rx51_kp_tstc
> #define VIDEO_GETC_FCT                  rx51_kp_getc
>
> I believe this is broken?

i don't have this board but it does seem to build OK and these
functions are in the image. What is broken?

Regards,
Simon