Re: [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices
-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
Re: [U-Boot] [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.
[..]
+ 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
Re: [U-Boot] [Patch v3] cmd/gpt: Support gpt command for all devices
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
@@ -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.wilc...@samsung.com
Best regards,
Piotr Wilczek
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [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
@@ -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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
