[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Reinhard Meyer]

To facilitate better testing of SPI peripherals, add
the possibility to specify bus and SPI mode to the
"sspi" command

Signed-off-by: Reinhard Meyer <u-boot@emk-elektronik.de>
---
 common/cmd_spi.c |   33 +++++++++++++++++++++++----------
 1 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/common/cmd_spi.c b/common/cmd_spi.c
index bafa217..fd1b1f5 100644
--- a/common/cmd_spi.c
+++ b/common/cmd_spi.c
@@ -47,7 +47,9 @@
 /*
  * Values from last command.
  */
-static unsigned int	device;
+static unsigned int	bus;
+static unsigned int	cs;
+static unsigned int	mode;
 static int   		bitlen;
 static uchar 		dout[MAX_SPI_BYTES];
 static uchar 		din[MAX_SPI_BYTES];
@@ -78,8 +80,18 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	if ((flag & CMD_FLAG_REPEAT) == 0)
 	{
-		if (argc >= 2)
-			device = simple_strtoul(argv[1], NULL, 10);
+		if (argc >= 2) {
+			mode = CONFIG_DEFAULT_SPI_MODE;
+			bus = simple_strtoul(argv[1], &cp, 10);
+			if (*cp == ':') {
+				cs = simple_strtoul(cp+1, &cp, 10);
+				if (*cp == '.');
+					mode = simple_strtoul(cp+1, NULL, 10);
+			} else {
+				cs = bus;
+				bus = CONFIG_DEFAULT_SPI_BUS;
+			}
+		}
 		if (argc >= 3)
 			bitlen = simple_strtoul(argv[2], NULL, 10);
 		if (argc >= 4) {
@@ -107,15 +119,13 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return 1;
 	}
 
-	/* FIXME: Make these parameters run-time configurable */
-	slave = spi_setup_slave(CONFIG_DEFAULT_SPI_BUS, device, 1000000,
-			CONFIG_DEFAULT_SPI_MODE);
+	slave = spi_setup_slave(bus, cs, 1000000, mode);
 	if (!slave) {
-		printf("Invalid device %d, giving up.\n", device);
+		printf("Invalid bus %d cs %d, giving up.\n", bus, cs);
 		return 1;
 	}
 
-	debug ("spi chipsel = %08X\n", device);
+	debug ("spi bus=%02x cs=%02x\n", bus, cs);
 
 	spi_claim_bus(slave);
 	if(spi_xfer(slave, bitlen, dout, din,
@@ -139,8 +149,11 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 U_BOOT_CMD(
 	sspi,	5,	1,	do_spi,
 	"SPI utility commands",
-	"<device> <bit_len> <dout> - Send <bit_len> bits from <dout> out the SPI\n"
-	"<device>  - Identifies the chip select of the device\n"
+	"<cs> <bit_len> <dout> - Send <bit_len> bits from <dout> out the SPI\n"
+	"<bus>:<cs>[.<mode>] - extended form\n"
+	"<bus>     - Identifies the SPI bus of the device\n"
+	"<cs>      - Identifies the chip select of the device\n"
+	"<mode>    - Identifies the SPI mode to use for the transfer\n"
 	"<bit_len> - Number of bits to send (base 10)\n"
 	"<dout>    - Hexadecimal string that gets sent"
 );
-- 
1.5.6.5

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Mike Frysinger]

On Mon, Aug 9, 2010 at 12:21 PM, Reinhard Meyer wrote:
> @@ -107,15 +119,13 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> ? ? ? ? ? ? ? ?return 1;
> ? ? ? ?}
>
> - ? ? ? /* FIXME: Make these parameters run-time configurable */
> - ? ? ? slave = spi_setup_slave(CONFIG_DEFAULT_SPI_BUS, device, 1000000,
> - ? ? ? ? ? ? ? ? ? ? ? CONFIG_DEFAULT_SPI_MODE);
> + ? ? ? slave = spi_setup_slave(bus, cs, 1000000, mode);
> ? ? ? ?if (!slave) {
> - ? ? ? ? ? ? ? printf("Invalid device %d, giving up.\n", device);
> + ? ? ? ? ? ? ? printf("Invalid bus %d cs %d, giving up.\n", bus, cs);

i would use the simple naming convention like so:
    printf("Invalid device %d:%d\n", bus, cs);

> ?U_BOOT_CMD(
> ? ? ? ?sspi, ? 5, ? ? ?1, ? ? ?do_spi,
> ? ? ? ?"SPI utility commands",
> - ? ? ? "<device> <bit_len> <dout> - Send <bit_len> bits from <dout> out the SPI\n"
> - ? ? ? "<device> ?- Identifies the chip select of the device\n"
> + ? ? ? "<cs> <bit_len> <dout> - Send <bit_len> bits from <dout> out the SPI\n"
> + ? ? ? "<bus>:<cs>[.<mode>] - extended form\n"
> + ? ? ? "<bus> ? ? - Identifies the SPI bus of the device\n"
> + ? ? ? "<cs> ? ? ?- Identifies the chip select of the device\n"
> + ? ? ? "<mode> ? ?- Identifies the SPI mode to use for the transfer\n"
> ? ? ? ?"<bit_len> - Number of bits to send (base 10)\n"
> ? ? ? ?"<dout> ? ?- Hexadecimal string that gets sent"

this usage string no longer makes sense.  how about:
"[<bus>:]<cs>[.<mode>] <bit_len> <dout> - Send <bit_len> bits from
<dout> out the SPI\n"
-mike

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Mike Frysinger]

were you going to send an updated version ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100823/df41fc38/attachment.pgp 

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Reinhard Meyer]

Dear Mike Frysinger,
> were you going to send an updated version ?
Soon. Currently I have other issues and a rather
"dirty" working tree :)

Reinhard

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Reinhard Meyer]

Dear Mike Frysinger,
> this usage string no longer makes sense.  how about:
> "[<bus>:]<cs>[.<mode>] <bit_len> <dout> - Send <bit_len> bits from
> <dout> out the SPI\n"


The problem is, that without blowing up the parser even more, only
2 forms of the command are valid:

the classic form

<cs> <bit_len> <dout>

or

<bus>:<cs>[.<mode>] <bit_len> <dout>

Since this command is mainly used for testing/debugging hardware,
I saw no need to allow the form

<cs>.<mode> <bit_len> <dout>

If thats ok, I can make a patch with the help fixed.

Best Regards
Reinhard

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Mike Frysinger]

On Tuesday, August 24, 2010 04:05:19 Reinhard Meyer wrote:
> Dear Mike Frysinger,
> > were you going to send an updated version ?
> 
> Soon. Currently I have other issues and a rather
> "dirty" working tree :)

dont sweat it.  just wanted to make sure it wasnt lost/forgotten/ignored.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100824/1a45a508/attachment.pgp 

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Mike Frysinger]

On Tuesday, August 24, 2010 06:53:49 Reinhard Meyer wrote:
> > this usage string no longer makes sense.  how about:
> > "[<bus>:]<cs>[.<mode>] <bit_len> <dout> - Send <bit_len> bits from
> > <dout> out the SPI\n"
> 
> The problem is, that without blowing up the parser even more, only
> 2 forms of the command are valid:
> 
> the classic form
> 
> <cs> <bit_len> <dout>
> 
> or
> 
> <bus>:<cs>[.<mode>] <bit_len> <dout>

how so ?  the bus:cs.mode field is always going to be one argv, and your patch 
has it so that argv[1] is always parsed against this form.  i dont see where 
this limitation is that you're seeing ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100825/0c095ac0/attachment.pgp 

[U-Boot] [PATCH] SPI: cmd_spi.c: add option to specify bus and mode

[Reinhard Meyer]

Dear Mike Frysinger,
>> <bus>:<cs>[.<mode>]<bit_len>  <dout>
>
> how so ?  the bus:cs.mode field is always going to be one argv, and your patch
> has it so that argv[1] is always parsed against this form.  i dont see where
> this limitation is that you're seeing ...
> -mike
Currently, without a ":", the "." part won't be handled. But I see that a simple
rearrangement would solve this. I'll fix and test that.

Reinhard