[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Robert Winkler]

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(-)

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);
 
 
-- 
1.8.3

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Igor Grinberg]

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);
>  
>  
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Robert Winkler]

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

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[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

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Igor Grinberg]

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.


-- 
Regards,
Igor.

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Robert Winkler]

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

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Igor Grinberg]

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
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Nikita Kiryanov]

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
>


-- 
Regards,
Nikita.