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