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! > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot