Hi Igor, On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinb...@compulab.co.il> wrote: > Hi Robert, > > On 06/03/13 20:20, Robert Winkler wrote: >> Also change splash_screen_prepare to a weak function. > > You should be able to make a commit message a bit better. > Also, personally, I see here two functional changes and it would be better > to separate them into two patches: > 1) Make the splash_screen_prepare() weak (I don't like this approach, > explanation below) > 2) Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO > > We have considered making the splash_screen_prepare() function weak > in the past, but decided to make it a config option instead. > This way it is documented in the README and is selectable in the > board config. > Once you change it to be weak, there is no point in the config option > anymore... Personally, I don't like this approach. > Wolfgang said as long as I was fixing the bug of SPLASH_SCREEN_PREPARE not working for CONFIG_VIDEO I should change it to a weak function so that's what I did.
See the email here: http://lists.denx.de/pipermail/u-boot/2013-May/155478.html I agree with you about the pointlessness of the CONFIG option and I too like having it documented in the README. That's the only reason I left the #ifdefs in at all because I didn't want to/didn't think I should remove the CONFIG altogether. I'm not sure what the solution is. >> >> Signed-off-by: Robert Winkler <robert.wink...@boundarydevices.com> >> --- >> board/compulab/cm_t35/cm_t35.c | 2 +- >> common/lcd.c | 10 ++++------ >> drivers/video/cfb_console.c | 14 ++++++++++++++ >> 3 files changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/board/compulab/cm_t35/cm_t35.c b/board/compulab/cm_t35/cm_t35.c >> index b0b80e5..95098af 100644 >> --- a/board/compulab/cm_t35/cm_t35.c >> +++ b/board/compulab/cm_t35/cm_t35.c >> @@ -120,7 +120,7 @@ static inline int splash_load_from_nand(void) >> } >> #endif /* CONFIG_CMD_NAND */ >> >> -int board_splash_screen_prepare(void) >> +int splash_screen_prepare(void) >> { >> char *env_splashimage_value; >> u32 bmp_load_addr; >> diff --git a/common/lcd.c b/common/lcd.c >> index edae835..90f1143 100644 >> --- a/common/lcd.c >> +++ b/common/lcd.c >> @@ -1069,15 +1069,13 @@ 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) >> +int __splash_screen_prepare(void) >> { >> return 0; >> } >> + >> +int splash_screen_prepare(void) >> + __attribute__ ((weak, alias("__splash_screen_prepare"))); >> #endif > > You remove the #else statement, apparently you break the compilation for > !CONFIG_SPLASH_SCREEN_PREPARE, as the below lcd_logo() function references > the splash_screen_prepare() function. > Also, this means you force the code to have the ugly #ifdefs inside > functions - that is not really nice. > >> >> static void *lcd_logo(void) >> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c >> index 0793f07..9180998 100644 >> --- a/drivers/video/cfb_console.c >> +++ b/drivers/video/cfb_console.c >> @@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int >> width, int x, int y, int black) >> #endif >> } >> >> +#ifdef CONFIG_SPLASH_SCREEN_PREPARE >> +int __splash_screen_prepare(void) >> +{ >> + return 0; >> +} >> + >> +int splash_screen_prepare(void) >> + __attribute__ ((weak, alias("__splash_screen_prepare"))); >> +#endif > > Why duplicate the above? > Probably, just place it in a common location? I asked Wolfgang about this in the original thread but haven't heard back so I just submitted it like this with the thought that CONFIG_VIDEO and CONFIG_LCD are never used simultaneously and are designed not to be (I could easily be wrong about that). > >> + >> static void *video_logo(void) >> { >> char info[128]; >> @@ -1996,6 +2006,10 @@ static void *video_logo(void) >> s = getenv("splashimage"); >> if (s != NULL) { >> >> +#ifdef CONFIG_SPLASH_SCREEN_PREPARE >> + splash_screen_prepare(); >> +#endif > > These are the ugly #ifdefs, I was talking about... Agreed > > I would recommend to just move the splash_screen_prepare() function definition > to a common location and add the above function call (with no #ifdef around). And keep the meaningless CONFIG? > >> + >> addr = simple_strtoul(s, NULL, 16); >> >> >> > > -- > Regards, > Igor. Robert Winkler _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
