| Message ID | 1370280058-15835-1-git-send-email-robert.winkler@boundarydevices.com |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
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. > > Signed-off-by: Robert Winkler <robert.winkler@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? > + > 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... 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). > + > addr = simple_strtoul(s, NULL, 16); > > >
Hi Igor, On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg@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.winkler@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
Adding Anatolij to the CC list. On Tue, Jun 4, 2013 at 8:10 AM, Robert Winkler <robert.winkler@boundarydevices.com> wrote: > Hi Igor, > > On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg@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.winkler@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
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.winkler@boundarydevices.com> wrote: >> Hi Igor, >> >> On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg@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.winkler@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.
Hi All, On Wed, Jun 5, 2013 at 1:31 AM, Igor Grinberg <grinberg@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.winkler@boundarydevices.com> wrote: >>> Hi Igor, >>> >>> On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg@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.winkler@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
On 06/06/13 22:06, Robert Winkler wrote: > Hi All, > > On Wed, Jun 5, 2013 at 1:31 AM, Igor Grinberg <grinberg@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.winkler@boundarydevices.com> wrote: >>>> Hi Igor, >>>> >>>> On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg@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.winkler@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 >
On 06/06/2013 10:06 PM, Robert Winkler wrote: > 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; > I really don't remember anymore, but it was probably a refactoring oversight or something similarly insignificant. The two are equivalent, so feel free to change it to lcd_base to make it more consistent with the rest of the code. >> -- >> Regards, >> Igor. > > > Please advise, > > Robert >
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 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 + 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 + addr = simple_strtoul(s, NULL, 16);
Also change splash_screen_prepare to a weak function. Signed-off-by: Robert Winkler <robert.winkler@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(-)