Hi All, On Wed, Jun 5, 2013 at 1:31 AM, Igor Grinberg <grinb...@compulab.co.il> wrote: > Hi Robert, > > On 06/04/13 21:11, Robert Winkler wrote: >> 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 > > Ok. > The major benefit of the construct (which Wolfgang wants to remove) is > clear code with no #ifdefs inside functions. > I don't have any special feelings for that construct, but I'd like to > keep #ifdefs out of any functions (the benefit). > >>> >>> 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. > > ... > some thoughts below... > >>> >>> >>>>> >>>>> 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? > > While I was writing the above, I meant to keep the #ifdef ... #else ... #endif > construct. > > So currently, after reading the link you've provided, > I can see two reasonable solutions: > > 1) Keep the #ifdef construct as it was, but move it to a common location, > so it can be called from both the lcd.c and cfb_console.c with no code > duplication. This also keeps the CONFIG option still meaningful. > > 2) Remove the construct and make the splash_screen_prepare() function weak. > Move the weak definition to a common location, so it can be called from > both > the lcd.c and cfb_console.c with no code duplication. > Remove the CONFIG option as it turns meaningless, but move its > documentation > (with needed adjustments) to some splash screen doc place. This will keep > the > functions clear of the #ifdefs. > > I would go for 1) and the patch for it is shorter. > For 2) it still should be two patches: > * Making the splash_screen_prepare() function weak and removing CONFIG option. > * Fixing the bug with cfb_console.c by moving the weak definition > to a common location and adding a call to it in cfb_console.c. >
What common location is there? In either case (making it weak or not) I'm not sure where to put it and it looks like common.h, video_font.h and video_fond_data.h are the only headers they share so common.h is probably where I'd put the prototype if I didn't create a new header. Either way it sees stupid to make it's own C file unless we were also taking the time to pull out a lot more of the duplication between lcd.c and cfb_console.c at the same time. There is a lot of duplication and very similar code. There have been some similar extractions the past: c270730f580e85ddab82e981abf8a518f78ae803 d3983ee85325d2be730830ebcf82585ee7cd2ecb One other unrelated question for Nikita from his splash prepare patch. Why did you use gd->fb_base when the global lcd_base is used elsewhere in the function and lcd.c in general. It seems inconsistent. 1094 if (splash_screen_prepare()) 1095 return (void *)gd->fb_base; > -- > Regards, > Igor. Please advise, Robert _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
