> 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.rupp...@sewerin.de> wrote:
>
> > Signed-off-by: Bastian Ruppert <bastian.rupp...@sewerin.de>
> > CC: Anatolij Gustschin <ag...@denx.de>
> > CC: Tom Rini <tr...@ti.com>
> > CC: Stefano Babic <sba...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
