[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Andreas Pretzsch]

The sspi command writes the given data out on SPI and prints the data it
reads to the console. For write-only slaves (i.e. a SPI-connected latch
used as output expander), this is pointless and clutters the console.
When called as "sspi.w", this output is omitted.

The flag is optional and backwards compatible, previous sspi revisions
would simply ignore the flag (checked back to 2011.03).

Signed-off-by: Andreas Pretzsch <apr@cn-eng.de>
---
Data size (number of bits) are passed as separate parameter to sspi,
hence .w is "free" and not used as data size anyway.
---
 common/cmd_spi.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/common/cmd_spi.c b/common/cmd_spi.c
index 8c623c9..c56c191 100644
--- a/common/cmd_spi.c
+++ b/common/cmd_spi.c
@@ -72,12 +72,17 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	uchar tmp;
 	int   j;
 	int   rcode = 0;
+	int   write_only = 0;
 
 	/*
 	 * We use the last specified parameters, unless new ones are
 	 * entered.
 	 */
 
+	j = strlen(argv[0]);
+	if (j > 2 && argv[0][j-2] == '.' && argv[0][j-1] == 'w')
+		write_only = 1;
+
 	if ((flag & CMD_FLAG_REPEAT) == 0)
 	{
 		if (argc >= 2) {
@@ -131,10 +136,12 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		printf("Error during SPI transaction\n");
 		rcode = 1;
 	} else {
-		for(j = 0; j < ((bitlen + 7) / 8); j++) {
-			printf("%02X", din[j]);
+		if (!write_only) {
+			/* dump read values to console */
+			for (j = 0; j < ((bitlen + 7) / 8); j++)
+				printf("%02X", din[j]);
+			printf("\n");
 		}
-		printf("\n");
 	}
 	spi_release_bus(slave);
 	spi_free_slave(slave);
@@ -147,7 +154,8 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 U_BOOT_CMD(
 	sspi,	5,	1,	do_spi,
 	"SPI utility command",
-	"[<bus>:]<cs>[.<mode>] <bit_len> <dout> - Send and receive bits\n"
+	"[.w] [<bus>:]<cs>[.<mode>] <bit_len> <dout> - Send and receive bits\n"
+	".w        - write only => don't print read result\n"
 	"<bus>     - Identifies the SPI bus\n"
 	"<cs>      - Identifies the chip select\n"
 	"<mode>    - Identifies the SPI mode to use\n"
-- 
1.7.5.4

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Mike Frysinger]

On Sat, Jul 16, 2011 at 13:32, Andreas Pretzsch wrote:
> The sspi command writes the given data out on SPI and prints the data it
> reads to the console. For write-only slaves (i.e. a SPI-connected latch
> used as output expander), this is pointless and clutters the console.
> When called as "sspi.w", this output is omitted.
>
> The flag is optional and backwards compatible, previous sspi revisions
> would simply ignore the flag (checked back to 2011.03).

i think the flag is misleading.  "sspi.w" makes it sound like it'd
call the SPI layer with a NULL read buffer and not simply omit the
output.  what you describe is more like a "quiet" flag.

along these lines, doesnt the general shell provide basic output
redirection to support "silencing" all commands rather than having to
add a "quiet" flag to them all ?  then your script could simply do
"sspi ... >/dev/null" (or however u-boot does it).
-mike

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Andreas Pretzsch]

Am Montag, den 18.07.2011, 13:49 -0400 schrieb Mike Frysinger:
> On Sat, Jul 16, 2011 at 13:32, Andreas Pretzsch wrote:
> > The sspi command writes the given data out on SPI and prints the data it
> > reads to the console. For write-only slaves (i.e. a SPI-connected latch
> > used as output expander), this is pointless and clutters the console.
> > When called as "sspi.w", this output is omitted.
> >
> > The flag is optional and backwards compatible, previous sspi revisions
> > would simply ignore the flag (checked back to 2011.03).
> 
> i think the flag is misleading.  "sspi.w" makes it sound like it'd
> call the SPI layer with a NULL read buffer and not simply omit the
> output.  what you describe is more like a "quiet" flag.

Correct, strictly speaking.
I chose it from a usage perspective, hence the "write", but ".q" for
quiet or similar would also have been possible. "write" just felt more
natural to me.

I also thought about passing NULL for the read buffer, but a quick
browse through the code showed that most, but not all SPI drivers are
prepared for that. And as there is already a static rx buffer in the spi
command code, I preferred to keep a few needless byte copies over fixing
all the spi drivers...

On the long run, the sspi command should be reworked anyway, as today
the read data is not usable in a script, it's just dumped to the
console.
Following the common U-Boot way to do this, I'd suggest
	sspi [<bus>:]<cs>[.<mode>] <bit_len> <dout> [din_env_var]
and either never print the read result to the console (my favorite) or
only if no env variable to store is passed. To be defined, comments
welcome.

The issue here is that the current sspi command would barf due to excess
argument, hence "new" sspi usage in an env script won't work on older
U-Boot revisions, whereas this is fine with the ".w" approach.
Not really that of a problem, admittedly. Maybe I was a bit too deep in
the "don't break it if not really necessary" mode...


> along these lines, doesnt the general shell provide basic output
> redirection to support "silencing" all commands rather than having to
> add a "quiet" flag to them all ?  then your script could simply do
> "sspi ... >/dev/null" (or however u-boot does it).

Not that I know of. Just tried on hush parser, no effect. Also looks
like the whole redirect code is disabled by a #ifndef __U_BOOT__.
Maybe one could change env stdout before and after, might work, but I'd
call that grotesque...

-- 

carpe noctem engineering
Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch
Dipl.-Ing. (FH) Andreas Pretzsch          Tel. +49-(0)731-5521572
Hahnengasse 3                             Fax: +49-(0)731-5521573
89073 Ulm, Germany                        email: apr at cn-eng.de

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Mike Frysinger]

On Wednesday, July 20, 2011 13:04:47 Andreas Pretzsch wrote:
> I also thought about passing NULL for the read buffer, but a quick
> browse through the code showed that most, but not all SPI drivers are
> prepared for that. And as there is already a static rx buffer in the spi
> command code, I preferred to keep a few needless byte copies over fixing
> all the spi drivers...

those drivers are broken.  the SPI API requires them to handle NULL rx or tx 
buffers.  do not cater to broken code by avoiding fixing common code because 
of bugs in drivers.

> Following the common U-Boot way to do this, I'd suggest
> 	sspi [<bus>:]<cs>[.<mode>] <bit_len> <dout> [din_env_var]
> and either never print the read result to the console (my favorite) or
> only if no env variable to store is passed. To be defined, comments
> welcome.

is this the common u-boot approach ?  seems like extending every random func 
to take a supplementary env var is the wrong way.  if the hush shell simply 
supported syntax like:
	foo="`sspi ......`"
then every command would get this for free

> > along these lines, doesnt the general shell provide basic output
> > redirection to support "silencing" all commands rather than having to
> > add a "quiet" flag to them all ?  then your script could simply do
> > "sspi ... >/dev/null" (or however u-boot does it).
> 
> Not that I know of. Just tried on hush parser, no effect. Also looks
> like the whole redirect code is disabled by a #ifndef __U_BOOT__.
> Maybe one could change env stdout before and after, might work, but I'd
> call that grotesque...

yes, playing with stdout would be awful, but it would work today ;)
-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/20110720/6b17f8cb/attachment.pgp 

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Detlev Zundel]

Hi Mike,

[...]

>> Following the common U-Boot way to do this, I'd suggest
>> 	sspi [<bus>:]<cs>[.<mode>] <bit_len> <dout> [din_env_var]
>> and either never print the read result to the console (my favorite) or
>> only if no env variable to store is passed. To be defined, comments
>> welcome.
>
> is this the common u-boot approach ?  seems like extending every random func 
> to take a supplementary env var is the wrong way.  if the hush shell simply 
> supported syntax like:
> 	foo="`sspi ......`"
> then every command would get this for free

Indeed - this would be the best solution and I would greatly appreciate
such a feature as it would be a big step in what we can script
(elegantly) in U-Boot.

Cheers
  Detlev

-- 
More than any other time in history, mankind faces a crossroads.  One
path leads  to despair  and utter  hopelessness.   The other to total
extinction.  Let us pray, we have the wisdom to choose correctly.
                                        -- Woody Allen
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Andreas Pretzsch]

Am Donnerstag, den 21.07.2011, 11:01 +0200 schrieb Detlev Zundel:
> Hi Mike,
> 
> [...]
> 
> >> Following the common U-Boot way to do this, I'd suggest
> >> 	sspi [<bus>:]<cs>[.<mode>] <bit_len> <dout> [din_env_var]
> >> and either never print the read result to the console (my favorite) or
> >> only if no env variable to store is passed. To be defined, comments
> >> welcome.
> >
> > is this the common u-boot approach ?  seems like extending every random func 
> > to take a supplementary env var is the wrong way.  if the hush shell simply 
> > supported syntax like:
> > 	foo="`sspi ......`"
> > then every command would get this for free
> 
> Indeed - this would be the best solution and I would greatly appreciate
> such a feature as it would be a big step in what we can script
> (elegantly) in U-Boot.

Full ACK that. I'd love to have a (nearly) full-scale hush shell in
U-Boot. Esp. as IMHO we left the "small scale bootloader" area for quite
some time now. And adjusting to different hardware variants purely by
script instead of special code is a valuable target IMHO. A bit
inconvenient with U-Boot hush shell, but doable.

But I doubt that this will be there in the immediate future (sorry,
currently no time on my side for this). And it won't help with the old
parser.

So how to proceed with the sspi command ?
  - keep it as it is, ignoring the "read not scriptable" status
  - strip down the printf output to only the value (required for ``)
  - add the additional parameter

Personally, I don't care that much, as I don't need a SPI read in any of
my devices here and could live with the noisy SPI write. But as I opened
up this issue, I'd like to finalize it.

Wolfgang, Detlev, Mike, please decide.

In this context, I suppose the original "spi.w" patch is NAK ?

Also, given any rework of the sspi command, cmd_spi.c should probably be
checkpatch-massaged (9 errors, 11 warnings, incl. kernel-stuff) before.
The usual whitespace and line length issues.
I suppose this would go in despite merge window closure, but any other
change won't, so submit it as separate patch instead of a series ?

-- 

carpe noctem engineering
Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch
Dipl.-Ing. (FH) Andreas Pretzsch          Tel. +49-(0)731-5521572
Hahnengasse 3                             Fax: +49-(0)731-5521573
89073 Ulm, Germany                        email: apr at cn-eng.de

[U-Boot] [PATCH] command "sspi": add write-only flag '.w' (discard read data)

[Mike Frysinger]

On Fri, Jul 22, 2011 at 14:02, Andreas Pretzsch wrote:
> So how to proceed with the sspi command ?

i think any proposals pending for the sspi command could be handled
with a better hush implementation, so i'd prefer to go that route as
the "bigger picture"

> Also, given any rework of the sspi command, cmd_spi.c should probably be
> checkpatch-massaged (9 errors, 11 warnings, incl. kernel-stuff) before.
> The usual whitespace and line length issues.
> I suppose this would go in despite merge window closure, but any other
> change won't, so submit it as separate patch instead of a series ?

a single patch to address all the checkpatch warnings is fine
-mike