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

[Wolfgang Denk]

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.

> +/* 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[];
> +
> +/* 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.

> +static int ustrtoul(const char *cp, char **endp, unsigned int base)

Maybe we should add this to some global place, like lib_generic ?

> +        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.

> +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.

> +	tbuf = vmalloc(tbuf_size);

Why do we need new alloc() stuff?

> +
> +	vfree(tbuf);

Ditto?

> +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.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A stone was placed at a ford in a river with the inscription:
"When this stone is covered it is dangerous to ford here."