[U-Boot] [PATCH 5/7] dfu:cmd: Support for DFU u-boot command

[Lukasz Majewski <l.majewski@samsung.com>]

Dear Mike,

> On Tuesday 03 July 2012 05:38:09 Lukasz Majewski wrote:
> > --- /dev/null
> > +++ b/common/cmd_dfu.c
> >
> > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > argv[])
> 
> static

It can be static (static int do_dfu). On the other hand the U_BOOT_CMD
macro defines:

int	(*cmd)(struct cmd_tbl_s *, int, int, char * const []); at
struct cmd_tbl_s, which is int.

> 
> > +{
> > +	char *str_env = NULL, *env_bkp = NULL;
> 
> no need to assign NULL here.  str_env should be const.
Yes, correct.
> 
> > +	static char *s = "dfu";
> 
> no need to declare this static
Why not? The s ptr is further passed to g_dnl_init(s), so my intend was
to declare string in the data segment of this translation unit. In this
way the data under this ptr will not vanish. 
> 
> > +	int ret = 0;
> 
> no need to init to 0

Ok.
> 
> > +	env_bkp = strdup(str_env);
> > +	ret = dfu_config_entities(env_bkp, argv[1],
> > +			    (int)simple_strtoul(argv[2], NULL,
> > 10));
> > +	if (ret)
> > +		return CMD_RET_FAILURE;
> > +
> > +	if (strcmp(argv[3], "list") == 0) {
> > +		dfu_show_entities();
> > +		dfu_free_entities();
> > +		free(env_bkp);
> > +		return CMD_RET_SUCCESS;
> 
> for these last three statements, you could just do "goto done" and
> put a done label below ...

Yes, you are correct. Thanks for tip.

> 
> > +exit:
> > +	g_dnl_cleanup();
> 
> done:
> 
> > +	dfu_free_entities();
> > +	free(env_bkp);
> > +
> > +	return CMD_RET_SUCCESS;
> 
> -mike



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group