Re: [U-Boot] [PATCH 13/13] lcd: split splash code into its own function

Hi NIkita,

On 1 February 2015 at 06:59, Nikita Kiryanov nikita@compulab.co.il wrote:

Hi Simon,

On 01/31/2015 02:25 AM, Simon Glass wrote:

...

Hi Nikita,

On 29 January 2015 at 04:21, Nikita Kiryanov nikita@compulab.co.il wrote:

...

lcd_logo() currently performs tasks well beyond just displaying the logo. It has code which displays splash image, it has logic which determines when the different display features are displayed, and it is coupled with the lcd console because it holds the responsibility of returning the lcd console base address.

Make lcd_logo() just about the logo by:

• Moving splash image display code into a dedicated function
• Moving the logic regarding when various features are displayed to lcd_clear() (which is arguably not the correct name for housing such code either, but it is currently the most fitting location code wise)
• Move the responsibility of setting the console base address to lcd_clear() too.

Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Cc: Bo Shen voice.shen@atmel.com Cc: Simon Glass sjg@chromium.org Cc: Anatolij Gustschin agust@denx.de

---

common/lcd.c | 52 ++++++++++++++++++++++------------------------------ common/splash.c | 16 ++++++++++++++++ include/splash.h | 11 ++++++++++- 3 files changed, 48 insertions(+), 31 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c index d0c0aff..f33942c 100644 --- a/common/lcd.c +++ b/common/lcd.c @@ -46,7 +46,7 @@ DECLARE_GLOBAL_DATA_PTR;

static int lcd_init(void *lcdbase); -static void *lcd_logo(void); +static void lcd_logo(void); static void lcd_setfgcolor(int color); static void lcd_setbgcolor(int color);

@@ -169,6 +169,9 @@ void lcd_clear(void) { short console_rows, console_cols; int bg_color;

•   char *s;
  

•   ulong addr;
  

•   static int do_splash = 1;
  

  #if LCD_BPP == LCD_COLOR8 /* Setting the palette */ lcd_setcolreg(CONSOLE_COLOR_BLACK, 0, 0, 0);

@@ -218,7 +221,23 @@ void lcd_clear(void) #endif console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH; lcd_init_console(lcd_base, console_rows, console_cols);

•   lcd_init_console(lcd_logo(), console_rows, console_cols);
  

•   if (do_splash) {
  

•           s = getenv("splashimage");
  

•           if (s) {
  

•                   do_splash = 0;
  

•                   addr = simple_strtoul(s, NULL, 16);
  

•                   if (lcd_splash(addr) == 0) {
  

•                           lcd_sync();
  

•                           return;
  

•                   }
  

•           }
  

•   }
  

•   lcd_logo();
  

+#if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)

•   addr = (ulong)lcd_base + BMP_LOGO_HEIGHT * lcd_line_length;
  

•   lcd_init_console((void *)addr, console_rows, console_cols);
  

I'm just a bit unsure about this - before this function was always called. I have some idea but can you explain why it is now in an #ifidef?

The only time when the base address of the console is defined as anything other than lcd_base, is when we get to the very end of the original lcd_logo() and the above #defines apply. In all other cases, the address of lcd_base is used, and that is set as default at the start of lcd_clear().

OK I see, thanks.

Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon