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

Thu Jun 6 21:06:47 CEST 2013

Hi All,

On Wed, Jun 5, 2013 at 1:31 AM, Igor Grinberg <grinberg at 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 at boundarydevices.com> wrote:
>>> Hi Igor,
>>>
>>> On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg at 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 at 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