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