| Message ID | 1310837526-3394-1-git-send-email-apr@cn-eng.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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...
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
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
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 ?
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
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"
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(-)