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

login
register
mail settings
Submitter Robert Winkler
Date June 14, 2013, 5 p.m.
Message ID <1371229203-10840-2-git-send-email-robert.winkler@boundarydevices.com>
Download mbox | patch
Permalink /patch/251471/
State Superseded
Delegated to: Anatolij Gustschin
Headers show

Comments

Robert Winkler - June 14, 2013, 5 p.m.
Create splash.c/h to put the function and any future common
splash screen code in.

Signed-off-by: Robert Winkler <robert.winkler@boundarydevices.com>
---
 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
Igor Grinberg - June 17, 2013, 5:34 a.m.
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@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.

>  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.

> +#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@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...

> + */
> +
> +#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@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
>
Robert Winkler - June 17, 2013, 5:08 p.m.
Hi Igor,

On Sun, Jun 16, 2013 at 10:34 PM, Igor Grinberg <grinberg@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@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@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@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.
Igor Grinberg - June 18, 2013, 6:20 a.m.
On 06/17/13 20:08, Robert Winkler wrote:
> Hi Igor,
> 
> On Sun, Jun 16, 2013 at 10:34 PM, Igor Grinberg <grinberg@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@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.

Well, what I meant is, once you make it a separate file (named splash.c),
it is rather strange that it should be always compiled.
It is fine for me now, but once more splash screen specific code moves
to this file, it surely should depend on CONFIG_SPLASH_SCREEN.

In general, I think the construct of
1) having the .c file compiled conditionally
   (meaning depend on CONFIG_... option)
2) having the .h file to define the interface to the .c file
   and provide a default implementation
   (when the above CONFIG_... is not set),

like in many cases Linux does, brings the benefit of clear yet robust
(in regard to the CONFIG_... defined or not) code has proven itself.

> 
>>
>>>  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@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@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.
>
Robert Winkler - June 18, 2013, 4:50 p.m.
On Mon, Jun 17, 2013 at 11:20 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>
>
> On 06/17/13 20:08, Robert Winkler wrote:
>> Hi Igor,
>>
>> On Sun, Jun 16, 2013 at 10:34 PM, Igor Grinberg <grinberg@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@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.
>
> Well, what I meant is, once you make it a separate file (named splash.c),
> it is rather strange that it should be always compiled.
> It is fine for me now, but once more splash screen specific code moves
> to this file, it surely should depend on CONFIG_SPLASH_SCREEN.
>
> In general, I think the construct of
> 1) having the .c file compiled conditionally
>    (meaning depend on CONFIG_... option)
> 2) having the .h file to define the interface to the .c file
>    and provide a default implementation
>    (when the above CONFIG_... is not set),
>
> like in many cases Linux does, brings the benefit of clear yet robust
> (in regard to the CONFIG_... defined or not) code has proven itself.
>
Ah, I'm sorry, I misread that as SPLASH_SCREEN_PREPARE, but that makes
more sense.  However I haven't noticed
any function definitions in h files in U-Boot and as it stands it has
to be this way.  Since a lot of the common code in lcd.c
and cfb_console is unrelated to (or at least, not only used) for
splash screens, maybe splash.c/h should be called
something else and thus always included.  Not sure what to call it,
but something to do with logos/bmp probably.

The overlap between
lcd_init -> lcd_clear -> lcd_logo -> bmp_display
and
video_init -> video_logo -> video_display_bitmap

is as much related to general bmp/logo display as splash screen
functionality and it would probably be more sensible
to pull it all out that than try to snip out only the splash screen
stuff for splash.c

Just an idea.

>>
>>>
>>>>  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@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@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.
>>
>
> --
> Regards,
> Igor.

Patch

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
 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.
+ */
+#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@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
+ */
+
+#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.
+ */
+#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@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
+ */
+
+#ifndef _SPLASH_H_
+#define _SPLASH_H_
+
+
+int splash_screen_prepare(void);
+
+
+#endif