video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Also change splash_screen_prepare to a weak function.
--- 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(-)
board/compulab/cm_t35/cm_t35.c +1 -1
@@ -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;
common/lcd.c +4 -6
@@ -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 )
drivers/video/cfb_console.c +14 -0
@@ -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 );
Signature · 1 line
1.8.3
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Hi Robert,
Quoted 1 line · Robert Winkler
Also change
splash_screen_prepareto 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.
Quoted 33 lines
Signed-off-by Robert Winkler <[email protected]>--- 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(-)board/compulab/cm_t35/cm_t35.c +1
-1@@ -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;common/lcd.c +0
-4@@ -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 (); -}
common/lcd.c int lcd_display_bitmap(ulong bmp_image, int x, int y) · line 1079
Quoted 10 lines
common/lcd.c +4
-2-#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.
Quoted 9 lines
common/lcd.c +0
-0static void *lcd_logo (void )drivers/video/cfb_console.c +0
-0@@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)#endif }
drivers/video/cfb_console.c static void plot_logo_or_black(void *screen, int width, int x, int y, int black) · line 1977
Quoted 10 lines
drivers/video/cfb_console.c +9
-0+#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?
drivers/video/cfb_console.c static void *video_logo(void) · line 2011
Quoted 11 lines
drivers/video/cfb_console.c +4
-0+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).
Quoted 5 lines
drivers/video/cfb_console.c +1
-0+ addr =simple_strtoul (s,NULL ,16 );
Signature · 2 lines
Regards, Igor.
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Hi Igor,
Quoted 12 lines · Igor Grinberg
Hi Robert,
Quoted 1 line · Robert Winkler
Also change
splash_screen_prepareto 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 thesplash_screen_prepare()weak (I don't like this approach, explanation below)
2) AddCONFIG_SPLASH_SCREEN_PREPAREsupport toCONFIG_VIDEO
Quoted 7 lines
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.
Quoted 60 lines
Quoted 33 lines
Signed-off-by Robert Winkler <[email protected]>--- 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(-)board/compulab/cm_t35/cm_t35.c +1
-1@@ -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;common/lcd.c +0
-4@@ -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 (); -}common/lcd.c int lcd_display_bitmap(ulong bmp_image, int x, int y) · line 1079
Quoted 10 lines
common/lcd.c +4
-2-#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 thesplash_screen_prepare()function.
Also, this means you force the code to have the ugly #ifdefs inside functions - that is not really nice.Quoted 10 lines
common/lcd.c +0
-0static void *lcd_logo (void )drivers/video/cfb_console.c +0
-0@@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)#endif }
Quoted 12 lines
drivers/video/cfb_console.c static void plot_logo_or_black(void *screen, int width, int x, int y, int black) · line 1977
Quoted 9 lines
drivers/video/cfb_console.c +9
-0+#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).
Quoted 9 lines
Quoted 9 lines
drivers/video/cfb_console.c +1
-0+static void *video_logo (void ) {char info[128 ];@@ -1996,6 +2006,10 @@ static void *video_logo(void) s =getenv ("splashimage" );if (s !=NULL ) {
Quoted 5 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2011
Quoted 3 lines
drivers/video/cfb_console.c +3
-0+#ifdef CONFIG_SPLASH_SCREEN_PREPARE +splash_screen_prepare (); +#endif These are the ugly #ifdefs, I was talking about...
Agreed
Quoted 3 lines
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?
Quoted 10 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2015
Quoted 6 lines
drivers/video/cfb_console.c +1
-0+ addr =simple_strtoul (s,NULL ,16 );--
Regards,
Igor.
Robert Winkler
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Adding Anatolij to the CC list.
Quoted 149 lines · Robert Winkler
Hi Igor,
Quoted 12 lines · Igor Grinberg
Hi Robert,
Quoted 1 line · Robert Winkler
Also change
splash_screen_prepareto 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 thesplash_screen_prepare()weak (I don't like this approach, explanation below)
2) AddCONFIG_SPLASH_SCREEN_PREPAREsupport toCONFIG_VIDEOQuoted 7 lines
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_PREPAREnot working
forCONFIG_VIDEOI 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.htmlI 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.
Quoted 60 lines
Quoted 33 lines
Signed-off-by Robert Winkler <[email protected]>--- 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(-)board/compulab/cm_t35/cm_t35.c +1
-1@@ -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;common/lcd.c +0
-4@@ -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 (); -}common/lcd.c int lcd_display_bitmap(ulong bmp_image, int x, int y) · line 1079
Quoted 10 lines
common/lcd.c +4
-2-#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 thesplash_screen_prepare()function.
Also, this means you force the code to have the ugly #ifdefs inside functions - that is not really nice.Quoted 10 lines
common/lcd.c +0
-0static void *lcd_logo (void )drivers/video/cfb_console.c +0
-0@@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)#endif }Quoted 12 lines
drivers/video/cfb_console.c static void plot_logo_or_black(void *screen, int width, int x, int y, int black) · line 1977
Quoted 9 lines
drivers/video/cfb_console.c +9
-0+#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 thatCONFIG_VIDEOandCONFIG_LCDare never used simultaneously and are
designed not to be (I could easily be wrong about that).Quoted 9 lines
Quoted 9 lines
drivers/video/cfb_console.c +1
-0+static void *video_logo (void ) {char info[128 ];@@ -1996,6 +2006,10 @@ static void *video_logo(void) s =getenv ("splashimage" );if (s !=NULL ) {Quoted 5 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2011
Quoted 3 lines
drivers/video/cfb_console.c +3
-0+#ifdef CONFIG_SPLASH_SCREEN_PREPARE +splash_screen_prepare (); +#endif These are the ugly #ifdefs, I was talking about...
Agreed
Quoted 3 lines
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?
Quoted 10 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2015
Quoted 6 lines
drivers/video/cfb_console.c +1
-0+ addr =simple_strtoul (s,NULL ,16 );--
Regards,
Igor.Robert Winkler
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Hi Robert,
Quoted 32 lines · Robert Winkler
Adding Anatolij to the CC list.
Quoted 28 lines · Robert Winkler
Hi Igor,
Quoted 12 lines · Igor Grinberg
Hi Robert,
Quoted 1 line · Robert Winkler
Also change
splash_screen_prepareto 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 thesplash_screen_prepare()weak (I don't like this approach, explanation below)
2) AddCONFIG_SPLASH_SCREEN_PREPAREsupport toCONFIG_VIDEOQuoted 7 lines
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_PREPAREnot working
forCONFIG_VIDEOI 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).
Quoted 8 lines
Quoted 8 lines
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...
Quoted 94 lines
Quoted 94 lines
Quoted 62 lines
Quoted 35 lines
Signed-off-by Robert Winkler <[email protected]>--- 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(-)board/compulab/cm_t35/cm_t35.c +1
-1@@ -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;common/lcd.c +0
-4@@ -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 (); -}common/lcd.c int lcd_display_bitmap(ulong bmp_image, int x, int y) · line 1079
Quoted 10 lines
common/lcd.c +4
-2-#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 thesplash_screen_prepare()function.
Also, this means you force the code to have the ugly #ifdefs inside functions - that is not really nice.Quoted 10 lines
common/lcd.c +0
-0static void *lcd_logo (void )drivers/video/cfb_console.c +0
-0@@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)#endif }Quoted 12 lines
drivers/video/cfb_console.c static void plot_logo_or_black(void *screen, int width, int x, int y, int black) · line 1977
Quoted 9 lines
drivers/video/cfb_console.c +9
-0+#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 thatCONFIG_VIDEOandCONFIG_LCDare never used simultaneously and are
designed not to be (I could easily be wrong about that).Quoted 14 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2011
Quoted 12 lines
drivers/video/cfb_console.c +4
-0+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...
Quoted 6 lines
Quoted 6 lines
Agreed
Quoted 3 lines
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.
Signature · 2 lines
Regards, Igor.
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Hi All,
Quoted 172 lines · Igor Grinberg
Hi Robert,
Quoted 32 lines · Robert Winkler
Adding Anatolij to the CC list.
Quoted 28 lines · Robert Winkler
Hi Igor,
Quoted 12 lines · Igor Grinberg
Hi Robert,
Quoted 1 line · Robert Winkler
Also change
splash_screen_prepareto 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 thesplash_screen_prepare()weak (I don't like this approach, explanation below)
2) AddCONFIG_SPLASH_SCREEN_PREPAREsupport toCONFIG_VIDEOQuoted 7 lines
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_PREPAREnot working
forCONFIG_VIDEOI 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.htmlOk.
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).Quoted 8 lines
Quoted 8 lines
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...Quoted 94 lines
Quoted 94 lines
Quoted 62 lines
Quoted 35 lines
Signed-off-by Robert Winkler <[email protected]>--- 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(-)board/compulab/cm_t35/cm_t35.c +1
-1@@ -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;common/lcd.c +0
-4@@ -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 (); -}common/lcd.c int lcd_display_bitmap(ulong bmp_image, int x, int y) · line 1079
Quoted 10 lines
common/lcd.c +4
-2-#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 thesplash_screen_prepare()function.
Also, this means you force the code to have the ugly #ifdefs inside functions - that is not really nice.Quoted 10 lines
common/lcd.c +0
-0static void *lcd_logo (void )drivers/video/cfb_console.c +0
-0@@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)#endif }Quoted 12 lines
drivers/video/cfb_console.c static void plot_logo_or_black(void *screen, int width, int x, int y, int black) · line 1977
Quoted 9 lines
drivers/video/cfb_console.c +9
-0+#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 thatCONFIG_VIDEOandCONFIG_LCDare never used simultaneously and are
designed not to be (I could easily be wrong about that).Quoted 14 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2011
Quoted 12 lines
drivers/video/cfb_console.c +4
-0+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...
Quoted 6 lines
Quoted 6 lines
Agreed
Quoted 3 lines
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.candcfb_console.cwith 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 thelcd.candcfb_console.cwith 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.
Quoted 6 lines
I would go for 1) and the patch for it is shorter.
For 2) it still should be two patches:
* Making thesplash_screen_prepare()function weak and removing CONFIG option.
* Fixing the bug withcfb_console.cby moving the weak definition to a common location and adding a call to it incfb_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 likecommon.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;
Quoted 3 lines
--
Regards,
Igor.
Please advise,
Robert
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Quoted 182 lines · Robert Winkler
Hi All,
Quoted 172 lines · Igor Grinberg
Hi Robert,
Quoted 32 lines · Robert Winkler
Adding Anatolij to the CC list.
Quoted 28 lines · Robert Winkler
Hi Igor,
Quoted 12 lines · Igor Grinberg
Hi Robert,
Quoted 1 line · Robert Winkler
Also change
splash_screen_prepareto 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 thesplash_screen_prepare()weak (I don't like this approach, explanation below)
2) AddCONFIG_SPLASH_SCREEN_PREPAREsupport toCONFIG_VIDEOQuoted 7 lines
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_PREPAREnot working
forCONFIG_VIDEOI 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.htmlOk.
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).Quoted 8 lines
Quoted 8 lines
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...Quoted 94 lines
Quoted 94 lines
Quoted 62 lines
Quoted 35 lines
Signed-off-by Robert Winkler <[email protected]>--- 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(-)board/compulab/cm_t35/cm_t35.c +1
-1@@ -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;common/lcd.c +0
-4@@ -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 (); -}common/lcd.c int lcd_display_bitmap(ulong bmp_image, int x, int y) · line 1079
Quoted 10 lines
common/lcd.c +4
-2-#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 thesplash_screen_prepare()function.
Also, this means you force the code to have the ugly #ifdefs inside functions - that is not really nice.Quoted 10 lines
common/lcd.c +0
-0static void *lcd_logo (void )drivers/video/cfb_console.c +0
-0@@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)#endif }Quoted 12 lines
drivers/video/cfb_console.c static void plot_logo_or_black(void *screen, int width, int x, int y, int black) · line 1977
Quoted 9 lines
drivers/video/cfb_console.c +9
-0+#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 thatCONFIG_VIDEOandCONFIG_LCDare never used simultaneously and are
designed not to be (I could easily be wrong about that).Quoted 14 lines
drivers/video/cfb_console.c static void *video_logo(void) · line 2011
Quoted 12 lines
drivers/video/cfb_console.c +4
-0+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...
Quoted 6 lines
Quoted 6 lines
Agreed
Quoted 3 lines
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.candcfb_console.cwith 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 thelcd.candcfb_console.cwith 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.Quoted 6 lines
I would go for 1) and the patch for it is shorter.
For 2) it still should be two patches:
* Making thesplash_screen_prepare()function weak and removing CONFIG option.
* Fixing the bug withcfb_console.cby moving the weak definition to a common location and adding a call to it incfb_console.c.
Quoted 5 lines
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.handvideo_fond_data.hare the only headers they share socommon.his 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?
Quoted 4 lines
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.candcfb_console.cat 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.
Quoted 22 lines
There is a lot of duplication and very similar code. There have been some similar extractions the past:
c270730f580e85ddab82e981abf8a518f78ae803d3983ee85325d2be730830ebcf82585ee7cd2ecbOne other unrelated question for Nikita from his splash prepare patch.
Why did you usegd->fb_basewhen the globallcd_baseis used
elsewhere
in the function andlcd.cin general. It seems inconsistent.1094 if (
splash_screen_prepare())
1095 return (void *)gd->fb_base;Quoted 3 lines
--
Regards,
Igor.Please advise,
Robert
Signature · 2 lines
Regards, Igor.
PATCH 1/1 video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
In reply to Robert WinklerQuoted 8 lines · Robert Winkler
One other unrelated question for Nikita from his splash prepare patch.
Why did you usegd->fb_basewhen the globallcd_baseis used
elsewhere
in the function andlcd.cin 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.
Quoted 9 lines
Quoted 3 lines
--
Regards,
Igor.Please advise,
Robert
Signature · 2 lines
Regards, Nikita.