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

[Tomas Melin]

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

>>>>
>>>> So long as the error is reported (even if it is not a very specific
>>>> error), people can add DEBUG and track it down.
>>>
>>> That depends who 'people' are? Devs? Users?
>>
>> Well in this case the user will never see the problem, unless someone
>> has screwed up the splash screen image. Mostly I'm talking about devs.
>>
>> Better would be to have an expanded debug() system which lets you turn
>> debugging on globally when hunting for a problem. That would be a nice
>> project for someone...
> 
> Yes, indeed that sounds like a nice project.
> It would be great to be able to specify the debug verbosity on per build
> basis (e.g. Kconfig).
> 

Indeed, that would be a great feature.

Regards,
Tomas