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

[Piotr Wilczek]

> -----Original Message-----
> From: Egbert Eich [mailto:egbert.eich at gmail.com]
> Sent: Sunday, October 13, 2013 1:14 AM
> To: Piotr Wilczek
> Cc: 'Egbert Eich'; u-boot at lists.denx.de; 'Tom Rini'; 'Egbert Eich'
> Subject: Re: [Patch v3] cmd/gpt: Support gpt command for all devices
> 
> 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.
> 
Agreed, I think it is a coding style rule in U-boot. Let the maintainer to
decide.

> 
> [..]
> 
> > > +		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.
> 
Ok.

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

Best regards,
Piotr Wilczek