On 01/25/2012 08:10 AM, Matthias Fuchs wrote:
Hi Eric,
please see my comments below.
On 24.01.2012 17:18, Eric Nelson wrote:
This patch allows a board configuration file to provide a default chip-select for serial flash so that first argument to the 'sf' command is optional.
On boards that use the mxc_spi driver and a GPIO for chip select, this allows a much simpler command line: U-Boot> sf probe instead of U-Boot> sf probe 0x5300
Signed-off-by: Eric Nelsoneric.nelson@boundarydevices.com Acked-by: Dirk Behmedirk.behme@de.bosch.com Acked-by: Stefano Babicsbabic@denx.de
common/cmd_sf.c | 34 +++++++++++++++++++++++----------- 1 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 7225656..4b32171 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -70,20 +70,28 @@ static int do_spi_flash_probe(int argc, char * const argv[]) char *endp; struct spi_flash *new;
- if (argc< 2)
return -1;- cs = simple_strtoul(argv[1],&endp, 0);
- if (*argv[1] == 0 || (*endp != 0&& *endp != ':'))
+#ifndef CONFIG_SPI_FLASH_CS
- if (argc< 2) {
printf("%s: missing arguments\n", __func__);I think this format for the error message is a little bit untypical for u-boot. We do not show up the internal C function name. Better would be to show the command usage, right?
Looking at this area of the code in more detail, there are quite a few cases where improper usage silently return -1.
I'm inclined to either follow that lead or toss them together with a "goto usage" as done in do_spi_flash().
Any preferences?
return -1;
- if (*endp == ':') {
if (endp[1] == 0)return -1;
- }
+#else
- cs = CONFIG_SPI_FLASH_CS ;
Other options for the spi flash subsystem are called CONFIG_SF_DEFAULT_MODE|SPEED. It think it make sense to call this CONFIG_SF_DEFAULT_CS and CONFIG_SF_DEFAULT_BUS (see below).
See include/configs/efikamx.h (reference to unused symbol CONFIG_SPI_FLASH_CS).
+#endif
bus = cs;cs = simple_strtoul(endp + 1,&endp, 0);if (*endp != 0)
if (argc>= 2) {
cs = simple_strtoul(argv[1],&endp, 0);if (*argv[1] == 0 || (*endp != 0&& *endp != ':')) return -1;if (*endp == ':') {if (endp[1] == 0)return -1;bus = cs;cs = simple_strtoul(endp + 1,&endp, 0);if (*endp != 0)return -1;}}
if (argc>= 3) {
@@ -299,7 +307,11 @@ usage: U_BOOT_CMD( sf, 5, 1, do_spi_flash, "SPI flash sub-system", +#ifndef CONFIG_SPI_FLASH_CS "probe [bus:]cs [hz] [mode] - init flash device on given SPI bus\n" +#else
- "probe [[bus:]cs] [hz] [mode] - init flash device on given SPI bus\n"
+#endif " and chip select\n" "sf read addr offset len - read `len' bytes starting at\n" " `offset' to memory at `addr'\n"
Can you also add a config option for the SPI bus number? I think these two need to handled in the same patch.
So you could add this stuff:
diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 9e97c8e..fa4312a 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -63,7 +63,11 @@ static int sf_parse_len_arg(char *arg, ulong *len)
static int do_spi_flash_probe(int argc, char * const argv[]) { +#ifdef CONFIG_SF_DEFAULT_BUS
unsigned int bus = CONFIG_SF_DEFAULT_BUS;+#else unsigned int bus = 0; +#endif unsigned int cs; unsigned int speed = CONFIG_SF_DEFAULT_SPEED; unsigned int mode = CONFIG_SF_DEFAULT_MODE;
Can do.