Hi Bin,
On 10 May 2015 at 17:15, Bin Meng <bmeng...@gmail.com> wrote:
> Hi Simon,
>
> On Sun, May 10, 2015 at 9:53 PM, Simon Glass <s...@chromium.org> wrote:
>> Hi Bin,
>>
>> On 8 May 2015 at 22:26, Bin Meng <bmeng...@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Sat, May 9, 2015 at 5:44 AM, Simon Glass <s...@chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>> On 6 May 2015 at 03:34, Bin Meng <bmeng...@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...@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.
>
> OK, I will drop this patch in v2.
>
>>>
>>> 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?
>>
>
> Sorry I was not clear. I mean these functions are defined elsewhere
> than the cfb_console.c while 8042 keyboard routines are defined inside
> cfb_console.c. Inconsistent?
>
> #ifdef CONFIG_I8042_KBD
> #include <i8042.h>
>
> #define VIDEO_KBD_INIT_FCT i8042_kbd_init()
> #define VIDEO_TSTC_FCT i8042_tstc
> #define VIDEO_GETC_FCT i8042_getc
> #endif
>
> Seems this rx51 board is the only one that provides keyboard hooks
> into the cfb_console driver.
OK I see. Well yes it is a bit icky. Once someone creates a uclass for
input and video/lcd we can presumably clean all this up. I suspect
what happened is that it was originally assumed that you had a
keyboard when you have a CFB console, then later someone decided that
this was not always true.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
