[U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices

[Egbert Eich]

On Fri, Oct 11, 2013 at 09:13:22AM +0200, Piotr Wilczek wrote:
> Dear Egbert Eich,
> 
> > -----Original Message-----
> > From: Egbert Eich [mailto:egbert.eich at gmail.com]
> > Sent: Friday, October 04, 2013 6:53 PM
> > To: u-boot at lists.denx.de
> > Cc: Piotr Wilczek; Tom Rini; Egbert Eich; Egbert Eich
> > Subject: [Patch v3] cmd/gpt: Support gpt command for all devices
> > 
> > From: Egbert Eich <eich at suse.com>
> > 
> > The gpt command was only implemented for mmc devices. There is no
> > reason why this command should not be generalized and be applied all
> > other storage device classes.
> > This change both simplifies the implementation and eliminates a build
> > failure for systems that don't support mmcs.
> > 
> > Signed-off-by: Egbert Eich <eich at suse.com>
> > ---
> > Changes for v2:
> >    - Coding style cleanup.
> > Changes for v3:
> >    - Removed wrong '&'
> >    - Removed unused variable
> >    - Fixed argument checking
> >    Spotted by Piotr Wilczek <p.wilczek at samsung.com>
> > 
> >  common/cmd_gpt.c | 45 +++++++++++++++++++--------------------------
> >  1 file changed, 19 insertions(+), 26 deletions(-)
> > 
> > diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index a46f5cc..17b1180
> > 100644
> > --- a/common/cmd_gpt.c
> > +++ b/common/cmd_gpt.c

[..]

> > 
> > @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int

[..]

> > -		/* device: 'mmc' */
> > -		if (strcmp(argv[2], "mmc") == 0) {
> > -			/* check if 'dev' is a number */
> > -			for (pstr = argv[3]; *pstr != '\0'; pstr++)
> > -				if (!isdigit(*pstr)) {
> > -					printf("'%s' is not a number\n",
> > -						argv[3]);
> > -					return CMD_RET_USAGE;
> > -				}
> > -			dev = (int)simple_strtoul(argv[3], NULL, 10);
> > -			/* write to mmc */
> > -			if (gpt_mmc_default(dev, argv[4]))
> > -				return CMD_RET_FAILURE;
> > +		char *ep;
> > +		block_dev_desc_t *blk_dev_desc;
> This probably should be at the beginning of the function

I personally prefer to keep symbols as local as possible (ie. declare them
in the block they are used in if they are just used within a single block) 
for the following rasons:
1. It makes the code more readable ie. the definition is closeby to
   the location where it is used and doesn't require scrolling to 
   the beginning of a function and the scope of the variable is is 
   much more obvious.
2. The compiler can optimize much better as it knows that a variable 
   can be discarded at the end of the block also by reusing stack 
   slots stack sapce can be used much more efficiently by the compiler.

I agree that in the case at hand the second argument is not too
relevant, it is more a coding style issue. If there is a coding
style requirement to have those definitions at the beginning of
the function I will create a new patch.


[..]

> > +		blk_dev_desc = get_dev(argv[2], dev);
> > +		if (!blk_dev_desc) {
> > +			printf("%s: %s dev %d NOT available\n",
> > +			       __func__, argv[2], dev);
> I think it is not necessary since the mmc subsystem prints 'MMC Device not
> found'.

I've done a quick look over the code - of all subsystems MMC seems to be 
the only one which prints a message when its get_dev() method is called 
but no device is found. Therefore I'd prefer to leave this there.

> 
> Except minor comments this patch looks good to me.
> I tested it on mmc device (Trats2) and works well.

Ok, thanks!

> 
> Tested-by: Piotr Wilczek <p.wilczek at samsung.com>
> 

Thanks a lot for testing!

Cheers,
	Egbert.