[U-Boot] [PATCH V9 1/4] Add cmd_spl command

[Simon Schwarz]

Hi Mike,

2011/12/8 Mike Frysinger <vapier at gentoo.org>:
> On Tuesday 06 December 2011 13:34:35 Simon Schwarz wrote:
>> --- /dev/null
>> +++ b/common/cmd_spl.c
>>
>> +int call_bootm(int argc, char * const argv[], char *subcommand[])
>
> static
done.
>
>> +int spl_export_fdt(int argc, char * const argv[])
>
> static
>
done.
>
>> +#ifdef CONFIG_OF_LIBFDT
>> +     /* Create subcommand string */
>> +     char *subcommand[] = {"start",
>
> that start needs to be on a new line
>
>> +     '\0'};

ok. Will change this to NULL.
>
> if this were NULL (and the call_bootm() checked for that) would be more
> natural to argv[] processing
>
>> +int spl_export_atags(int argc, char * const argv[])
>
> static

done
>
>> +     char *subcommand[] = {"start", "loados",
>> +#ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH
>> +     "ramdisk",
>> +#endif
>> +     "cmdline", "bdt", "prep", '\0'};
>
> char *subcommand[] = {
>        ... some strings ...
>        ... some more strings ...
> };

done
>
>> +int spl_export(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
> static
>
done
>> +{
>> +
>
> delete that newline
>
done
>> +     cmd_tbl_t *c;
>
> i think you can const this ...
>
right should work -> done.
>> +int do_spl(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
> static
>
done.
>> +     cmd_tbl_t *c;
>
> i think you can const this ...
>
done.
>> +             cmd = (int)c->cmd;
>
> you're casting a pointer to an integer ?  wtf is going on ?
>
This is the same as in bootm.c. cmd is overloaded with the state of
the statemachine. It is used as an integer.
>> --- /dev/null
>> +++ b/include/cmd_spl.h
>
> needs #ifdef protection against multiple inclusion
>
right. done.
>> +extern bootm_headers_t images;
>
> i get the feeling this isn't the right place for this and it should be in
> include/image.h instead ...
>

I trust you with this ;)

>> +enum image_type {FDT, ATAGS};
>
> these names are too short, and the "image" namespace is taken by image.h
> already ... but leading on to the following defines ...

Hm - it seems that they are not used anyway. deleted.

>
>> +#define SPL_EXPORT   (0x00000001)
>> +
>> +#define SPL_EXPORT_FDT               (0x00000001)
>> +#define SPL_EXPORT_ATAGS     (0x00000002)
>
> why do these need to be defines ?
> enum spl_export_type {
>        SPL_EXPORT_FDT = 1,
>        SPL_EXPORT_ATAGS = 2,
> };
>
> also, drop the paren here
>

I used bootm.c as reference - it's the same there.

>> --- a/include/configs/devkit8000.h
>> +++ b/include/configs/devkit8000.h
>
> generally board updates should be a sep commit

will do.
> -mike

Thanks!
Simon