[U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Tomas Melin]

Hi Igor, Simon,

On 12/26/2016 09:24 AM, Igor Grinberg wrote:
> Hi Tomas,
> 
> Sorry for the late response...
> 
> On 12/20/16 07:29, Tomas Melin wrote:
>> Hi Igor, Simon,
>>
>> On 12/15/2016 11:08 AM, Igor Grinberg wrote:
>>> Hi Tomas,
>>>
>>> On 12/14/16 16:23, Tomas Melin wrote:
>>>> Hi Simon, Igor,
>>>>
>>>> On 12/14/2016 02:53 PM, Igor Grinberg wrote:
>>>>> On 12/13/16 22:29, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> I think two above debug() are very legitimate - no need to shout if no FIT image
>>>>>>>>> or no splash in it...
>>>>>>>>>
>>>>>>>>>> +     res = fit_image_get_data_offset(fit_header, node_offset,
>>>>>>>>>> +                                     &splash_offset);
>>>>>>>>>> +     if (res < 0) {
>>>>>>>>>> +             debug("Could not find 'data-offset' property in FIT\n");
>>>>>>>>>> +             return res;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
>>>>>>>>>> +     if (res < 0) {
>>>>>>>>>> +             debug("Could not find 'data-size' property in FIT\n");
>>>>>>>>>> +             return res;
>>>>>>>>>> +     }
>>>>>>>>>
>>>>>>>>> Now regarding these two, I'm not sure.
>>>>>>>>> Since we have found a valid FIT and also a node with a correct splash name,
>>>>>>>>> probably the intent is that we show the splash, right?
>>>>>>>>> But in the two above checks, we find inconsistencies that do not allow us to
>>>>>>>>> show the splash - meaning the FIT is not actually good (am I right here?).
>>>>>>>>> So may be we should report it to the 'user' and allow correcting the FIT?
>>>>>>>>> Otherwise, it is impossible to debug the image w/o a debug version of U-Boot...
>>>>>>>>> Do I make sense, or do I miss something?
>>>>>>>>
>>>>>>>> Yes that makes some sense, but the problem is that then you are
>>>>>>>> including error messages always which would never happen in a working
>>>>>>>> system (i.e. it just bloats the code).
>>>>>>>
>>>>>>> Unless, there a kind of corruption or a user mistake and then that same
>>>>>>> user can't even understand what happened because we do not help him with
>>>>>>> an error message.
>>>>>>> You cannot know that these error messages will never happen...
>>>>>>> This is a generic code which can be used by a wide variety of platforms -
>>>>>>> I don't think you can foresee all the possible use cases.
>>>>>>>
>>>>>>> We are talking about several tens of bytes vs. usability.
>>>>>>> If there is an error, it should be stated as such. It should not just
>>>>>>> exit silently...
>>>>>>
>>>>>> I agree with that, there should definitely be an error printed. It
>>>>>> should say something like 'Failed to load splash screen (err=-4)' or
>>>>>> something like that. The error number should provide some clues and
>>>>>> people can dig in.
>>>>>
>>>>> Great idea!
>>>>
>>>> splash_load_fit() currently fails silently but still reports the error in
>>>> the return value. And this function is used so that board.c calls 
>>>> splash_source_load()->splash_load_fit().
>>>> The board function call will get notified of the error value as provided
>>>> by the return value for splash_load_fit(). In our board implementation that is 
>>>> actually exactly how it is done, the board function call checks the return
>>>> value and prints ("Failed to load splash screen image, error: %d\n", ret)
>>>> in case there was and error.
>>>>
>>>> IMHO this is quite good behaviour as is, leaving it up to the implementation
>>>> in the board.c if there should be a error message or not (and if it should 
>>>> bloat the code with another printf or not).
>>>
>>> Well, yes this makes sense if you care to do the work in the board code.
>>> Although, I would expect that sometime this code will be called from
>>> a generic board independent place (e.g. init array, etc.).
>>
>> I don't have a strong opinion, even if I do prefer the current version.
>> Please let me know if patch this still needs changes. 
> 
> Well, I can see two courses this patch can take:
> 1) We merge it as-is and any additional board uses the splash_load_fit() will
>    copy/paste the error handling from your board, or generalizes it and patches
>    both boards.
> 2) We make a more generic approach (e.g. print error messages in the common code)
>    and any new user (board or common call) will not need to handle the errors,
>    but just use the call.
> 
> Either one is good with me.

I'll update the patch to use printf:s for the latter statements (as discussed above.), I will send out a new version shortly.

Thanks,
Tomas 

> 
> Have a great holidays everyone!
>