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. [..] > > + 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.wilc...@samsung.com> > Thanks a lot for testing! Cheers, Egbert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot