[U-Boot] [PATCH] [UBI] UBI command support

[Kyungmin Park]

Dear Wolfgang,

On Tue, Oct 21, 2008 at 7:24 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Kyungmin Park,
>
> In message <20081021091859.GA29306 at july> you wrote:
>> UBI command support
>>
>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> ...
>> +++ 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