Hi Mike,
2011/12/8 Mike Frysinger <vap...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
