Dear Wolfgang, On Tue, Oct 21, 2008 at 7:24 PM, Wolfgang Denk <[EMAIL PROTECTED]> wrote: > Dear Kyungmin Park, > > In message <[EMAIL PROTECTED]> you wrote: >> UBI command support >> >> Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> > ... >> +++ b/common/cmd_ubi.c >> @@ -0,0 +1,535 @@ >> +/* >> + * Unsorted Block Image commands >> + */ >> + >> +#include <common.h> >> +#include <command.h> > > GPL header missing.
Okay I added. > >> +/* board/\*\/ubi.c */ > > What's the '\'s there? > >> +extern int ubi_board_scan(void); > > Please move the prototypes to some header file. > >> +/* drivers/mtd/ubi/build.c */ >> +extern struct ubi_device *ubi_devices[]; Moved to proper header files. >> + >> +/* Private own data */ >> +static struct ubi_device *ubi; >> +static int ubi_initialized; >> + >> +static void ubi_dump_vol_info(const struct ubi_volume *vol) >> +{ >> + dbg_msg("volume information dump:"); >> + dbg_msg("vol_id %d", vol->vol_id); >> + dbg_msg("reserved_pebs %d", vol->reserved_pebs); >> + dbg_msg("alignment %d", vol->alignment); >> + dbg_msg("data_pad %d", vol->data_pad); >> + dbg_msg("vol_type %d", vol->vol_type); >> + dbg_msg("name_len %d", vol->name_len); >> + dbg_msg("usable_leb_size %d", vol->usable_leb_size); >> + dbg_msg("used_ebs %d", vol->used_ebs); >> + dbg_msg("used_bytes %lld", vol->used_bytes); >> + dbg_msg("last_eb_bytes %d", vol->last_eb_bytes); >> + dbg_msg("corrupted %d", vol->corrupted); >> + dbg_msg("upd_marker %d", vol->upd_marker); > > Please use the standard debug() macro. It's not debug message, it's ubi msg. I fixed. > >> +static int ustrtoul(const char *cp, char **endp, unsigned int base) > > Maybe we should add this to some global place, like lib_generic ? Move to lib_generic/vsprintf.c > >> + unsigned long result = simple_strtoul(cp, endp, base); >> + switch (**endp) { >> + case 'G' : >> + result *= 1024; > > Please add a > /* fall through */ > comment to make clkear that it is intentional not to have a "break;" > here. > >> + case 'M': >> + result *= 1024; > > Ditto. Did > >> +static int verify_mkvol_req(const struct ubi_device *ubi, >> + const struct ubi_mkvol_req *req) >> +{ > ... >> +bad: >> + printf("bad volume creation request"); >> +// ubi_dbg_dump_mkvol_req(req); >> + return err; > > No C++ comments, please. And please don;t add dead code either. Yes I remove it > >> + tbuf = vmalloc(tbuf_size); > > Why do we need new alloc() stuff? > >> + >> + vfree(tbuf); > > Ditto? Use malloc & free. > >> +static int do_ubi(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) >> +{ >> + size_t size = 0; >> + ulong addr = 0; >> + int err = 0; >> + >> + if (!ubi_initialized) { >> + err = ubi_board_scan(); >> + if (err) { >> + printf("ubi initialize error %d\n", err); > > "UBI init error" -- maybe we can decode the error number into some > string? > >> + /* E.g., create volume size type */ >> + if (argc == 5) { >> + if (strncmp(argv[4], "s", 1) == 0) >> + dynamic = 0; >> + else if (strncmp(argv[4], "d", 1) != 0) { >> + printf("You give wrong type\n"); > > "Incorrect type". It would also be helpful if the incorrect parameter > gets printed, so the user sees what he passed. > >> + return 1; >> + } >> + argc--; >> + } >> + /* E.g., create volume size */ >> + if (argc == 4) { >> + err = parse_num(&size, argv[3]); >> + if (err) { >> + printf("You give correct size\n"); > > "Incorrect size". Please also print incorrect argument value. > >> + if (strncmp(argv[1], "write", 5) == 0) { >> + if (argc < 5) { >> + printf("You give wrong parameters\n"); > > Print usage message instead. > >> + } >> + >> + addr = simple_strtoul(argv[2], NULL, 16); >> + err = parse_num(&size, argv[4]); >> + if (err) { >> + printf("You give wrong size\n"); > > See above. > >> + if (err) { >> + printf("You give wrong size\n"); > > Ditto. > >> + printf("Unknown UBI command or invalid number of arguments\n"); > > Print usage message instead. > How to display usage? Thank you, Kyungmin Park _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot