* [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices [not found] <000101ce35f115206703f61350$%wilczek@samsung.com> @ 2013-10-04 16:53 ` Egbert Eich 2013-10-11 7:13 ` Piotr Wilczek 2013-11-08 22:25 ` [U-Boot] [U-Boot, " Tom Rini 0 siblings, 2 replies; 5+ messages in thread From: Egbert Eich @ 2013-10-04 16:53 UTC (permalink / raw) To: u-boot From: Egbert Eich <eich@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@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@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 @@ -11,7 +11,6 @@ #include <common.h> #include <malloc.h> #include <command.h> -#include <mmc.h> #include <part_efi.h> #include <exports.h> #include <linux/ctype.h> @@ -122,7 +121,7 @@ static int set_gpt_info(block_dev_desc_t *dev_desc, int errno = 0; uint64_t size_ll, start_ll; - debug("%s: MMC lba num: 0x%x %d\n", __func__, + debug("%s: lba num: 0x%x %d\n", __func__, (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba); if (str_part == NULL) @@ -235,25 +234,18 @@ err: return errno; } -static int gpt_mmc_default(int dev, const char *str_part) +static int gpt_default(block_dev_desc_t *blk_dev_desc, const char *str_part) { int ret; char *str_disk_guid; u8 part_count = 0; disk_partition_t *partitions = NULL; - struct mmc *mmc = find_mmc_device(dev); - - if (mmc == NULL) { - printf("%s: mmc dev %d NOT available\n", __func__, dev); - return CMD_RET_FAILURE; - } - if (!str_part) return -1; /* fill partitions */ - ret = set_gpt_info(&mmc->block_dev, str_part, + ret = set_gpt_info(blk_dev_desc, str_part, &str_disk_guid, &partitions, &part_count); if (ret) { if (ret == -1) @@ -266,7 +258,7 @@ static int gpt_mmc_default(int dev, const char *str_part) } /* save partitions layout to disk */ - gpt_restore(&mmc->block_dev, str_disk_guid, partitions, part_count); + gpt_restore(blk_dev_desc, str_disk_guid, partitions, part_count); free(str_disk_guid); free(partitions); @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { int ret = CMD_RET_SUCCESS; int dev = 0; - char *pstr; if (argc < 5) return CMD_RET_USAGE; /* command: 'write' */ if ((strcmp(argv[1], "write") == 0) && (argc == 5)) { - /* 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; + dev = (int)simple_strtoul(argv[3], &ep, 10); + if (!ep || ep[0] != '\0') { + printf("'%s' is not a number\n", argv[3]); + return CMD_RET_USAGE; } + blk_dev_desc = get_dev(argv[2], dev); + if (!blk_dev_desc) { + printf("%s: %s dev %d NOT available\n", + __func__, argv[2], dev); + return CMD_RET_FAILURE; + } + + if (gpt_default(blk_dev_desc, argv[4])) + return CMD_RET_FAILURE; } else { return CMD_RET_USAGE; } -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices 2013-10-04 16:53 ` [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices Egbert Eich @ 2013-10-11 7:13 ` Piotr Wilczek 2013-10-12 23:13 ` Egbert Eich 2013-11-08 22:25 ` [U-Boot] [U-Boot, " Tom Rini 1 sibling, 1 reply; 5+ messages in thread From: Piotr Wilczek @ 2013-10-11 7:13 UTC (permalink / raw) To: u-boot 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@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@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@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 > @@ -11,7 +11,6 @@ > #include <common.h> > #include <malloc.h> > #include <command.h> > -#include <mmc.h> > #include <part_efi.h> > #include <exports.h> > #include <linux/ctype.h> > @@ -122,7 +121,7 @@ static int set_gpt_info(block_dev_desc_t *dev_desc, > int errno = 0; > uint64_t size_ll, start_ll; > > - debug("%s: MMC lba num: 0x%x %d\n", __func__, > + debug("%s: lba num: 0x%x %d\n", __func__, > (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba); > > if (str_part == NULL) > @@ -235,25 +234,18 @@ err: > return errno; > } > > -static int gpt_mmc_default(int dev, const char *str_part) > +static int gpt_default(block_dev_desc_t *blk_dev_desc, const char > +*str_part) > { > int ret; > char *str_disk_guid; > u8 part_count = 0; > disk_partition_t *partitions = NULL; > > - struct mmc *mmc = find_mmc_device(dev); > - > - if (mmc == NULL) { > - printf("%s: mmc dev %d NOT available\n", __func__, dev); > - return CMD_RET_FAILURE; > - } > - > if (!str_part) > return -1; > > /* fill partitions */ > - ret = set_gpt_info(&mmc->block_dev, str_part, > + ret = set_gpt_info(blk_dev_desc, str_part, > &str_disk_guid, &partitions, &part_count); > if (ret) { > if (ret == -1) > @@ -266,7 +258,7 @@ static int gpt_mmc_default(int dev, const char > *str_part) > } > > /* save partitions layout to disk */ > - gpt_restore(&mmc->block_dev, str_disk_guid, partitions, > part_count); > + gpt_restore(blk_dev_desc, str_disk_guid, partitions, part_count); > free(str_disk_guid); > free(partitions); > > @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int > argc, char * const argv[]) { > int ret = CMD_RET_SUCCESS; > int dev = 0; > - char *pstr; > > if (argc < 5) > return CMD_RET_USAGE; > > /* command: 'write' */ > if ((strcmp(argv[1], "write") == 0) && (argc == 5)) { > - /* 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 > + dev = (int)simple_strtoul(argv[3], &ep, 10); > + if (!ep || ep[0] != '\0') { > + printf("'%s' is not a number\n", argv[3]); > + return CMD_RET_USAGE; > } > + 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'. > + return CMD_RET_FAILURE; > + } > + > + if (gpt_default(blk_dev_desc, argv[4])) > + return CMD_RET_FAILURE; > } else { > return CMD_RET_USAGE; > } > -- > 1.8.1.4 Except minor comments this patch looks good to me. I tested it on mmc device (Trats2) and works well. Tested-by: Piotr Wilczek <p.wilczek@samsung.com> Best regards, Piotr Wilczek ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices 2013-10-11 7:13 ` Piotr Wilczek @ 2013-10-12 23:13 ` Egbert Eich 2013-10-14 9:50 ` Piotr Wilczek 0 siblings, 1 reply; 5+ messages in thread From: Egbert Eich @ 2013-10-12 23:13 UTC (permalink / raw) To: u-boot 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@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@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@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@samsung.com> > Thanks a lot for testing! Cheers, Egbert. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices 2013-10-12 23:13 ` Egbert Eich @ 2013-10-14 9:50 ` Piotr Wilczek 0 siblings, 0 replies; 5+ messages in thread From: Piotr Wilczek @ 2013-10-14 9:50 UTC (permalink / raw) To: u-boot > -----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@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@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@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@samsung.com> > > > > Thanks a lot for testing! > > Cheers, > Egbert. Best regards, Piotr Wilczek ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [U-Boot, v3] cmd/gpt: Support gpt command for all devices 2013-10-04 16:53 ` [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices Egbert Eich 2013-10-11 7:13 ` Piotr Wilczek @ 2013-11-08 22:25 ` Tom Rini 1 sibling, 0 replies; 5+ messages in thread From: Tom Rini @ 2013-11-08 22:25 UTC (permalink / raw) To: u-boot On Fri, Oct 04, 2013 at 06:53:04PM +0200, egbert.eich at gmail.com wrote: > From: Egbert Eich <eich@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@suse.com> > Tested-by: Piotr Wilczek <p.wilczek@samsung.com> Applied to u-boot/master with a slight change to the coding style (yes, we want variables declared at the start of the function and __maybe_unused for ones that would be hidden via #ifdef and cause a warning in some cases), thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131108/4de11d76/attachment.pgp> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-08 22:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <000101ce35f115206703f61350$%wilczek@samsung.com>
2013-10-04 16:53 ` [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices Egbert Eich
2013-10-11 7:13 ` Piotr Wilczek
2013-10-12 23:13 ` Egbert Eich
2013-10-14 9:50 ` Piotr Wilczek
2013-11-08 22:25 ` [U-Boot] [U-Boot, " Tom Rini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox