[U-Boot] [PATCH V3 4/6] sf command: allow default chip select through CONFIG_SPI_FLASH_CS
Eric Nelson
eric.nelson at boundarydevices.com
Fri Jan 27 02:36:22 CET 2012
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 at boundarydevices.com>
>> Acked-by: Dirk Behme<dirk.behme at de.bosch.com>
>> Acked-by: Stefano Babic<sbabic at 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.
More information about the U-Boot
mailing list