From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Nelson <eric.nelson@boundarydevices.com>
Date: Thu, 26 Jan 2012 18:36:22 -0700
Subject: [U-Boot] [PATCH V3 4/6] sf command: allow default chip select
 through CONFIG_SPI_FLASH_CS
In-Reply-To: <4F201B5A.4010500@esd.eu>
References: <1327421904-21487-1-git-send-email-eric.nelson@boundarydevices.com>
	<1327421904-21487-5-git-send-email-eric.nelson@boundarydevices.com>
	<4F201B5A.4010500@esd.eu>
Message-ID: <4F21FF96.60907@boundarydevices.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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 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?
>

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.