[U-Boot] [PATCH 1/2] video: lcd: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Robert Winkler]

Hi Igor,

On Sun, Jun 16, 2013 at 10:34 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Robert,
>
> On 06/14/13 20:00, Robert Winkler wrote:
>> Create splash.c/h to put the function and any future common
>> splash screen code in.
>>
>> Signed-off-by: Robert Winkler <robert.winkler at boundarydevices.com>
>
> Thanks for the effort!
> Several comments below...
>
>> ---
>>  common/Makefile             |  1 +
>>  common/lcd.c                | 19 ++++++-------------
>>  common/splash.c             | 37 +++++++++++++++++++++++++++++++++++++
>>  drivers/video/cfb_console.c |  8 ++++++--
>>  include/lcd.h               |  1 -
>>  include/splash.h            | 30 ++++++++++++++++++++++++++++++
>>  6 files changed, 80 insertions(+), 16 deletions(-)
>>  create mode 100644 common/splash.c
>>  create mode 100644 include/splash.h
>>
>> diff --git a/common/Makefile b/common/Makefile
>> index 0e0fff1..1d70584 100644
>> --- a/common/Makefile
>> +++ b/common/Makefile
>> @@ -203,6 +203,7 @@ COBJS-y += flash.o
>>  COBJS-$(CONFIG_CMD_KGDB) += kgdb.o kgdb_stubs.o
>>  COBJS-$(CONFIG_I2C_EDID) += edid.o
>>  COBJS-$(CONFIG_KALLSYMS) += kallsyms.o
>> +COBJS-y += splash.o
>
> I think this should depend on CONFIG_SPLASH_SCREEN.
No it shouldn't.  The function is always called so it always needs to
be defined.  It's the
same behavior we had before, it was always compiled into lcd.h.
Whether or not the CONFIG
was defined just changed whether it actually called
board_splash_screen_prepare or just returned 0.

This is of course true for when it's a weak function as well.

>
>>  COBJS-$(CONFIG_LCD) += lcd.o
>>  COBJS-$(CONFIG_LYNXKDI) += lynxkdi.o
>>  COBJS-$(CONFIG_MENU) += menu.o
>> diff --git a/common/lcd.c b/common/lcd.c
>> index 3a60484..4a85ebb 100644
>> --- a/common/lcd.c
>> +++ b/common/lcd.c
>> @@ -43,6 +43,11 @@
>>  #include <lcd.h>
>>  #include <watchdog.h>
>>
>> +/*
>> + * Include splash.h for splash_screen_prepare() etc.
>> + */
>
> I think this comment is meaningless, the below include is self explanatory.
Agreed.  I was just trying to match the other superfluous comments.
>
>> +#include <splash.h>
>> +
>>  #if defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \
>>       defined(CONFIG_CPU_MONAHANS)
>>  #define CONFIG_CPU_PXA
>> @@ -1072,18 +1077,6 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
>>  }
>>  #endif
>>
>> -#ifdef CONFIG_SPLASH_SCREEN_PREPARE
>> -static inline int splash_screen_prepare(void)
>> -{
>> -     return board_splash_screen_prepare();
>> -}
>> -#else
>> -static inline int splash_screen_prepare(void)
>> -{
>> -     return 0;
>> -}
>> -#endif
>> -
>>  static void *lcd_logo(void)
>>  {
>>  #ifdef CONFIG_SPLASH_SCREEN
>> @@ -1096,7 +1089,7 @@ static void *lcd_logo(void)
>>               do_splash = 0;
>>
>>               if (splash_screen_prepare())
>> -                     return (void *)gd->fb_base;
>> +                     return (void *)lcd_base;
>>
>>               addr = simple_strtoul (s, NULL, 16);
>>  #ifdef CONFIG_SPLASH_SCREEN_ALIGN
>> diff --git a/common/splash.c b/common/splash.c
>> new file mode 100644
>> index 0000000..fe13c69
>> --- /dev/null
>> +++ b/common/splash.c
>> @@ -0,0 +1,37 @@
>> +/*
>> + * Copyright (C) 2013, Boundary Devices <info at boundarydevices.com>
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.       See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>
> I would drop the postal address, as it changed in the past and
> probably will change in the future and then we will have a bunch of
> files with wrong address...
Good point, will do.
>
>> + */
>> +
>> +#include <splash.h>
>> +#include <config.h>
>> +
>> +#ifdef CONFIG_SPLASH_SCREEN_PREPARE
>> +int splash_screen_prepare(void)
>> +{
>> +     return board_splash_screen_prepare();
>> +}
>> +#else
>> +int splash_screen_prepare(void)
>> +{
>> +     printf("SPLASH_SCREEN_PREPARE not defined\n");
>> +     return 0;
>> +}
>> +#endif
>> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
>> index b769222..4543730 100644
>> --- a/drivers/video/cfb_console.c
>> +++ b/drivers/video/cfb_console.c
>> @@ -178,6 +178,11 @@
>>  #include <video_fb.h>
>>
>>  /*
>> + * Include splash.h for splash_screen_prepare() etc.
>> + */
>
> Same here, the below include does not need any comment.
>
>> +#include <splash.h>
>> +
>> +/*
>>   * some Macros
>>   */
>>  #define VIDEO_VISIBLE_COLS   (pGD->winSizeX)
>> @@ -1991,10 +1996,9 @@ static void *video_logo(void)
>>  #ifdef CONFIG_SPLASH_SCREEN
>>       s = getenv("splashimage");
>>       if (s != NULL) {
>> -
>> +             splash_screen_prepare();
>>               addr = simple_strtoul(s, NULL, 16);
>>
>> -
>>               if (video_display_bitmap(addr,
>>                                       video_logo_xpos,
>>                                       video_logo_ypos) == 0) {
>> diff --git a/include/lcd.h b/include/lcd.h
>> index 30225ed..79bf13e 100644
>> --- a/include/lcd.h
>> +++ b/include/lcd.h
>> @@ -37,7 +37,6 @@ extern struct vidinfo panel_info;
>>
>>  void lcd_ctrl_init(void *lcdbase);
>>  void lcd_enable(void);
>> -int board_splash_screen_prepare(void);
>>
>>  /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */
>>  void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue);
>> diff --git a/include/splash.h b/include/splash.h
>> new file mode 100644
>> index 0000000..478f864
>> --- /dev/null
>> +++ b/include/splash.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * Copyright (C) 2013, Boundary Devices <info at boundarydevices.com>
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.       See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>
> If you decide to drop the postal address, then please do it also here.
>
>> + */
>> +
>> +#ifndef _SPLASH_H_
>> +#define _SPLASH_H_
>> +
>> +
>> +int splash_screen_prepare(void);
>> +
>> +
>> +#endif
>>
>
> --
> Regards,
> Igor.