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.nel...@boundarydevices.com>
> Acked-by: Dirk Behme <dirk.be...@de.bosch.com> 
> Acked-by: Stefano Babic <sba...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to