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

[Igor Grinberg]

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.

Have a great holidays everyone!

-- 
Regards,
Igor.