[U-Boot] [PATCH V3 4/6] sf command: allow default chip select through CONFIG_SPI_FLASH_CS

[Matthias Fuchs <matthias.fuchs@esd.eu>]

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 Nelson <eric.nelson@boundarydevices.com>
> Acked-by: Dirk Behme <dirk.behme@de.bosch.com> 
> Acked-by: Stefano Babic <sbabic@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?

>  		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).

> +#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;

Matthias