On 06/06/13 22:06, Robert Winkler wrote: > 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.
Well, this is a splash screen specific code, may it is time for splash.h? This might be a question for Anatolij? > > 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. Well, its own C file might be a good approach. I don't think you have to take the time right now to pull out all the duplications, but you can start this and then we encourage people to continue your work. > 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 > -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
