> -----Original Message----- > From: Egbert Eich [mailto:egbert.e...@gmail.com] > Sent: Sunday, October 13, 2013 1:14 AM > To: Piotr Wilczek > Cc: 'Egbert Eich'; u-boot@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.e...@gmail.com] > > > Sent: Friday, October 04, 2013 6:53 PM > > > To: u-boot@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 <e...@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 <e...@suse.com> > > > --- > > > Changes for v2: > > > - Coding style cleanup. > > > Changes for v3: > > > - Removed wrong '&' > > > - Removed unused variable > > > - Fixed argument checking > > > Spotted by Piotr Wilczek <p.wilc...@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.wilc...@samsung.com> > > > > Thanks a lot for testing! > > Cheers, > Egbert. Best regards, Piotr Wilczek _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot