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