Adding Anatolij to the CC list.
On Tue, Jun 4, 2013 at 8:10 AM, Robert Winkler <robert.wink...@boundarydevices.com> wrote: > 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
