[U-Boot] [PATCH 3/6] video: cfb_console: logo can be positioned via the splashpos variable

[Bastian.Ruppert at sewerin.de <Bastian.Ruppert@sewerin.de>]

> Hi Bastian,

Hello Anatolij,

>
> there is a number of issues with this patch, please see comments
> below.
>
> On Fri, 10 Aug 2012 09:26:43 +0200
> Bastian Ruppert <Bastian.Ruppert@Sewerin.de> wrote:
>
> > Signed-off-by: Bastian Ruppert <Bastian.Ruppert@Sewerin.de>
> > CC: Anatolij Gustschin <agust@denx.de>
> > CC: Tom Rini <trini@ti.com>
> > CC: Stefano Babic <sbabic@denx.de>
> > ---
> >  drivers/video/cfb_console.c |   61 +++++++++++++++++++++++++++
> +---------------
> >  1 files changed, 40 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> > index 19d061f..21b52bd 100644
> > --- a/drivers/video/cfb_console.c
> > +++ b/drivers/video/cfb_console.c
> > @@ -66,7 +66,11 @@
> >   * CONFIG_CONSOLE_TIME         - display time/date in upper right
> >   *            corner, needs CONFIG_CMD_DATE and
> >   *            CONFIG_CONSOLE_CURSOR
> > - * CONFIG_VIDEO_LOGO         - display Linux Logo in upper left corner
> > + * CONFIG_VIDEO_LOGO         - display Linux Logo in upper left
corner.
> > + *            Use CONFIG_SPLASH_SCREEN_ALIGN with
> > + *            environment variable "splashpos" to place
> > + *            the logo on other position. In this case
> > + *            no CONSOLE_EXTRA_INFO is possible.
> >   * CONFIG_VIDEO_BMP_LOGO      - use bmp_logo instead of linux_logo
> >   * CONFIG_CONSOLE_EXTRA_INFO  - display additional board information
> >   *            strings that normaly goes to serial
> > @@ -369,6 +373,8 @@ static void *video_fb_address;   /* frame
> buffer address */
> >  static void *video_console_address;   /* console buffer start address
*/
> >
> >  static int video_logo_height = VIDEO_LOGO_HEIGHT;
> > +static int video_logo_xpos;
> > +static int video_logo_ypos;
> >
> >  static int __maybe_unused cursor_state;
> >  static int __maybe_unused old_col;
> > @@ -1594,40 +1600,53 @@ static void *video_logo(void)
> >     __maybe_unused int y_off = 0;
> >
> >  #ifdef CONFIG_SPLASH_SCREEN
> > -   char *s;
> >     ulong addr;
> > -
> > -   s = getenv("splashimage");
> > +#endif
> > +#if defined(CONFIG_SPLASH_SCREEN) || defined
(CONFIG_SPLASH_SCREEN_ALIGN)
> > +   char *s;
> > +#endif
>
> these ifdefs should be better reduced, I think we can use __maybe_unused
> here, like for y_off above.

OK.

> > +#ifdef CONFIG_SPLASH_SCREEN_ALIGN
> > +   s = getenv("splashpos");
> >     if (s != NULL) {
> > -      int x = 0, y = 0;
> > +      if (s[0] == 'm')
> > +         video_logo_xpos = BMP_ALIGN_CENTER;
>
> The 'm' case will work with splashscreen, but not with the video logo.
> There is no proper offset calculation in logo_plot() if xpos or ypos
> are set to BMP_ALIGN_CENTER. As a result the logo offset will be wrong
> and an access to wrong offset can even brick the board (on boards with
> small frame buffers).
>
> ...
> > +
> > +      if (video_display_bitmap(addr,         \
> > +               video_logo_xpos,   \
>
> no need to use "\" here.
>
> ...
> > +   logo_plot(video_fb_address,      \
> > +      VIDEO_COLS,         \
> > +      video_logo_xpos,      \
>
> ditto.
>
> ...

OK.

> > +#ifdef CONFIG_SPLASH_SCREEN_ALIGN
> > +   /* when using splashpos for video_logo, no console output */
> > +   return (video_fb_address + video_logo_height * VIDEO_LINE_LEN);
>
> The returned address is used as text console offset, so if the logo is
> moved down, the video_logo_height should be increased by video_logo_ypos.
> Otherwise the text and cursor positions will be wrong in the video
> console.
>
> I've fixed these issues and submitted a patch v2 3/6. Please test.
>
> Thanks,
>
> Anatolij


Thank you for your effort,

Bastian.