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

[Tomas Melin]

Hi Simon, 

On 11/29/2016 11:40 PM, Simon Glass wrote:
> Hi Tomas,
> 
> On 25 November 2016 at 02:45, Tomas Melin <tomas.melin at vaisala.com> wrote:
>> Enable support for loading a splash image from within a FIT image.
>> The image is assumed to be generated with mkimage -E flag to hold
>> the data external to the FIT.
>>
>> Signed-off-by: Tomas Melin <tomas.melin at vaisala.com>
>> ---
>>  common/image-fit.c     | 49 ++++++++++++++++++++++++++++++++++
>>  common/splash_source.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/image.h        |  4 +++
>>  include/splash.h       |  5 ++--
>>  4 files changed, 127 insertions(+), 2 deletions(-)
> 
> Sorry, a few comments...
> 
> (is there a 2/2 patch - is that the one to sort the includes? If so it
> would be better to put that one first I think, so this one just adds
> the new includes in the right place)
Yes, the patch is here: http://patchwork.ozlabs.org/patch/699136/
I changed the state of that patch to superseded.

> 
> Please can you use patman/checkpatch to check the patch?
> 
> 4 errors, 2 warnings, 0 checks for
> 0001-splash-add-support-for-loading-splash-from-a-FIT-ima.patch:
> error: common/splash_source.c,317: "(foo*)" should be "(foo *)"
> error: common/splash_source.c,321: "(foo*)" should be "(foo *)"
> warning: common/splash_source.c,339: line over 80 characters
> error: common/splash_source.c,350: trailing whitespace
> error: common/splash_source.c,406: code indent should use tabs where possible
> warning: common/splash_source.c,406: please, no spaces at the start of a line
> 
> Also, can you add some docs to README.splashprepare ?

Sorry for not noticing this before posting. Fixing those issues and adding documentation for next round.

>>  }
>> +
> 
> Please drop this blank line.
> 
>> +
>> +       res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
>> +       if (res < 0) {
>> +               printf("Could not find 'data-size' property in FIT\n");
> 
> Should any of these be debug(), or do you really expect these to be
> useful to the user? If we can avoid printf() it does cut the code
> size.

You are right that these are more or less debug level information. Changing to use debug().

> 
>> +               return res;
>> +       }
>> +
>> +       /* Align data offset to 4-byte boundrary */
>> +       fit_size = fdt_totalsize(fit_header);
>> +       fit_size = (fit_size + 3) & ~3;
>> +
>> +       /* Read in the splash data */
>> +       location->offset = (location->offset + fit_size + splash_offset);
>> +       res = splash_storage_read_raw(location, bmp_load_addr , splash_size);
>> +       if (res < 0)
>> +               return res;
>> +
>> +       return 0;
>> +}
> 
> I suppose this is fine. But really I would prefer to use something like:
> 
>    fit_image_load(, ..., &data, &len)
>    memcpy(dst, src_addr, len);
> 
> but that would presumably require quite a few changes, right? At least
> it would be good to know what is missing.

The reason I kept it like this is that fit_image_load() seems to assume 
the FIT image is already loaded to memory. This loads only the splash image data
from storage (not all FIT data). Also, it looks as fit_image_load() doesn't support data
in FIT images with data-offset properties (?).

spl_load_simple_fit() does similar things, but cannot really be used as such for getting
the data needed here.

I'll send out a new version of this patch-series.


BR,
Tomas