| Message ID | 20230520190856.34720-5-boerge.struempfel@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: add SPI_MOSI_IDLE_LOW mode bit | expand |
On Sat, May 20, 2023 at 10:09 PM Boerge Struempfel <boerge.struempfel@gmail.com> wrote: > > Added the three missing spi mode bits SPI_3WIRE_HIZ, SPI_RX_CPHA_FLIP, > and SPI_MOSI_IDLE_LOW. Due to the length of the new options, the > indentation of the options in the help message was also adjusted for all > other options. Actually since you are touching all of them in the user-visible output, you may also reshuffle them to be grouped logically. I'm not sure if the switch-case ordering would be nice to have shuffled as well. If so, in this case it might be better to have it as a preparatory patch before you adding new options (and hence take care of indentation in the first patch). That said, just think about it, I'm not insisting.
Am So., 21. Mai 2023 um 11:00 Uhr schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Sat, May 20, 2023 at 10:09 PM Boerge Struempfel > <boerge.struempfel@gmail.com> wrote: > > > > Added the three missing spi mode bits SPI_3WIRE_HIZ, SPI_RX_CPHA_FLIP, > > and SPI_MOSI_IDLE_LOW. Due to the length of the new options, the > > indentation of the options in the help message was also adjusted for all > > other options. > > Actually since you are touching all of them in the user-visible > output, you may also reshuffle them to be grouped logically. I'm not > sure if the switch-case ordering would be nice to have shuffled as > well. If so, in this case it might be better to have it as a > preparatory patch before you adding new options (and hence take care > of indentation in the first patch). That said, just think about it, > I'm not insisting. > Thanks for the suggestion. I tried coming up with a logical way of ordering, but I am having some difficulties deciding. What do you think of the following order? general device settings " -D --device device to use (default /dev/spidev1.1)\n" " -s --speed max speed (Hz)\n" " -d --delay delay (usec)\n" " -l --loop loopback\n" spi mode " -H --cpha clock phase\n" " -O --cpol clock polarity\n" " -F --rx-cpha-flip flip CPHA on Rx only xfer\n" number of wires for transmission " -2 --dual dual transfer\n" " -4 --quad quad transfer\n" " -8 --octal octal transfer\n" " -3 --3wire SI/SO signals shared\n" " -Z --3wire-hiz high impedance turnaround\n" additional parameters " -b --bpw bits per word\n" " -L --lsb least significant bit first\n" " -C --cs-high chip select active high\n" " -N --no-cs no chip select\n" " -R --ready slave pulls low to pause\n" " -M --mosi-idle-low leave mosi line low when idle\n" data " -i --input input data from a file (e.g. \"test.bin\")\n" " -o --output output data to a file (e.g. \"results.bin\")\n" " -p Send data (e.g. \"1234\\xde\\xad\")\n" " -S --size transfer size\n" " -I --iter iterations\n"); misc " -v --verbose Verbose (show tx buffer)\n" -- Kind regards, Börge Strümpfel
On Sun, May 21, 2023 at 2:35 PM Börge Strümpfel <boerge.struempfel@gmail.com> wrote: > > Am So., 21. Mai 2023 um 11:00 Uhr schrieb Andy Shevchenko > <andy.shevchenko@gmail.com>: > > > > On Sat, May 20, 2023 at 10:09 PM Boerge Struempfel > > <boerge.struempfel@gmail.com> wrote: > > > > > > Added the three missing spi mode bits SPI_3WIRE_HIZ, SPI_RX_CPHA_FLIP, > > > and SPI_MOSI_IDLE_LOW. Due to the length of the new options, the > > > indentation of the options in the help message was also adjusted for all > > > other options. > > > > Actually since you are touching all of them in the user-visible > > output, you may also reshuffle them to be grouped logically. I'm not > > sure if the switch-case ordering would be nice to have shuffled as > > well. If so, in this case it might be better to have it as a > > preparatory patch before you adding new options (and hence take care > > of indentation in the first patch). That said, just think about it, > > I'm not insisting. > > > > Thanks for the suggestion. I tried coming up with a logical way of > ordering, but I am having some difficulties deciding. What do you > think of the following order? > > general device settings > " -D --device device to use (default /dev/spidev1.1)\n" > " -s --speed max speed (Hz)\n" > " -d --delay delay (usec)\n" > " -l --loop loopback\n" > > spi mode > " -H --cpha clock phase\n" > " -O --cpol clock polarity\n" > " -F --rx-cpha-flip flip CPHA on Rx only xfer\n" > > number of wires for transmission > " -2 --dual dual transfer\n" > " -4 --quad quad transfer\n" > " -8 --octal octal transfer\n" > " -3 --3wire SI/SO signals shared\n" > " -Z --3wire-hiz high impedance turnaround\n" > > additional parameters > " -b --bpw bits per word\n" > " -L --lsb least significant bit first\n" > " -C --cs-high chip select active high\n" > " -N --no-cs no chip select\n" > " -R --ready slave pulls low to pause\n" > " -M --mosi-idle-low leave mosi line low when idle\n" > > data > " -i --input input data from a file (e.g. \"test.bin\")\n" > " -o --output output data to a file (e.g. \"results.bin\")\n" > " -p Send data (e.g. \"1234\\xde\\xad\")\n" > " -S --size transfer size\n" > " -I --iter iterations\n"); > > misc > " -v --verbose Verbose (show tx buffer)\n" Looks great to me, thank you for doing that!
Am So., 21. Mai 2023 um 21:26 Uhr schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Sun, May 21, 2023 at 2:35 PM Börge Strümpfel > <boerge.struempfel@gmail.com> wrote: > > > > Am So., 21. Mai 2023 um 11:00 Uhr schrieb Andy Shevchenko > > <andy.shevchenko@gmail.com>: > > > > > > On Sat, May 20, 2023 at 10:09 PM Boerge Struempfel > > > <boerge.struempfel@gmail.com> wrote: > > > > > > > > Added the three missing spi mode bits SPI_3WIRE_HIZ, SPI_RX_CPHA_FLIP, > > > > and SPI_MOSI_IDLE_LOW. Due to the length of the new options, the > > > > indentation of the options in the help message was also adjusted for all > > > > other options. > > > > > > Actually since you are touching all of them in the user-visible > > > output, you may also reshuffle them to be grouped logically. I'm not > > > sure if the switch-case ordering would be nice to have shuffled as > > > well. If so, in this case it might be better to have it as a > > > preparatory patch before you adding new options (and hence take care > > > of indentation in the first patch). That said, just think about it, > > > I'm not insisting. > > > > > > > Thanks for the suggestion. I tried coming up with a logical way of > > ordering, but I am having some difficulties deciding. What do you > > think of the following order? > > > > general device settings > > " -D --device device to use (default /dev/spidev1.1)\n" > > " -s --speed max speed (Hz)\n" > > " -d --delay delay (usec)\n" > > " -l --loop loopback\n" > > > > spi mode > > " -H --cpha clock phase\n" > > " -O --cpol clock polarity\n" > > " -F --rx-cpha-flip flip CPHA on Rx only xfer\n" > > > > number of wires for transmission > > " -2 --dual dual transfer\n" > > " -4 --quad quad transfer\n" > > " -8 --octal octal transfer\n" > > " -3 --3wire SI/SO signals shared\n" > > " -Z --3wire-hiz high impedance turnaround\n" > > > > additional parameters > > " -b --bpw bits per word\n" > > " -L --lsb least significant bit first\n" > > " -C --cs-high chip select active high\n" > > " -N --no-cs no chip select\n" > > " -R --ready slave pulls low to pause\n" > > " -M --mosi-idle-low leave mosi line low when idle\n" > > > > data > > " -i --input input data from a file (e.g. \"test.bin\")\n" > > " -o --output output data to a file (e.g. \"results.bin\")\n" > > " -p Send data (e.g. \"1234\\xde\\xad\")\n" > > " -S --size transfer size\n" > > " -I --iter iterations\n"); > > > > misc > > " -v --verbose Verbose (show tx buffer)\n" > > Looks great to me, thank you for doing that! > You are welcome. Should I only reorder the flags, or actually introduce the "group"-headers to visibly distinguish the options? -- With best regards, Boerge Struempfel
On Mon, May 22, 2023 at 10:23 AM Börge Strümpfel <boerge.struempfel@gmail.com> wrote: > Am So., 21. Mai 2023 um 21:26 Uhr schrieb Andy Shevchenko > <andy.shevchenko@gmail.com>: > > On Sun, May 21, 2023 at 2:35 PM Börge Strümpfel > > <boerge.struempfel@gmail.com> wrote: > > > Am So., 21. Mai 2023 um 11:00 Uhr schrieb Andy Shevchenko > > > <andy.shevchenko@gmail.com>: ... > > > Thanks for the suggestion. I tried coming up with a logical way of > > > ordering, but I am having some difficulties deciding. What do you > > > think of the following order? > > > > > > general device settings > > > " -D --device device to use (default /dev/spidev1.1)\n" > > > " -s --speed max speed (Hz)\n" > > > " -d --delay delay (usec)\n" > > > " -l --loop loopback\n" > > > > > > spi mode > > > " -H --cpha clock phase\n" > > > " -O --cpol clock polarity\n" > > > " -F --rx-cpha-flip flip CPHA on Rx only xfer\n" > > > > > > number of wires for transmission > > > " -2 --dual dual transfer\n" > > > " -4 --quad quad transfer\n" > > > " -8 --octal octal transfer\n" > > > " -3 --3wire SI/SO signals shared\n" > > > " -Z --3wire-hiz high impedance turnaround\n" > > > > > > additional parameters > > > " -b --bpw bits per word\n" > > > " -L --lsb least significant bit first\n" > > > " -C --cs-high chip select active high\n" > > > " -N --no-cs no chip select\n" > > > " -R --ready slave pulls low to pause\n" > > > " -M --mosi-idle-low leave mosi line low when idle\n" > > > > > > data > > > " -i --input input data from a file (e.g. \"test.bin\")\n" > > > " -o --output output data to a file (e.g. \"results.bin\")\n" > > > " -p Send data (e.g. \"1234\\xde\\xad\")\n" > > > " -S --size transfer size\n" > > > " -I --iter iterations\n"); > > > > > > misc > > > " -v --verbose Verbose (show tx buffer)\n" > > > > Looks great to me, thank you for doing that! > > > You are welcome. > Should I only reorder the flags, or actually introduce the > "group"-headers to visibly distinguish the options? Up to you. If you think it increases the usability I'm all for it.
diff --git a/tools/spi/spidev_test.c b/tools/spi/spidev_test.c index b0ca44c70e83..66bfe90c541e 100644 --- a/tools/spi/spidev_test.c +++ b/tools/spi/spidev_test.c @@ -172,28 +172,31 @@ static void print_usage(const char *prog) { - printf("Usage: %s [-DsbdlHOLC3vpNR24SI]\n", prog); - puts(" -D --device device to use (default /dev/spidev1.1)\n" - " -s --speed max speed (Hz)\n" - " -d --delay delay (usec)\n" - " -b --bpw bits per word\n" - " -i --input input data from a file (e.g. \"test.bin\")\n" - " -o --output output data to a file (e.g. \"results.bin\")\n" - " -l --loop loopback\n" - " -H --cpha clock phase\n" - " -O --cpol clock polarity\n" - " -L --lsb least significant bit first\n" - " -C --cs-high chip select active high\n" - " -3 --3wire SI/SO signals shared\n" - " -v --verbose Verbose (show tx buffer)\n" - " -p Send data (e.g. \"1234\\xde\\xad\")\n" - " -N --no-cs no chip select\n" - " -R --ready slave pulls low to pause\n" - " -2 --dual dual transfer\n" - " -4 --quad quad transfer\n" - " -8 --octal octal transfer\n" - " -S --size transfer size\n" - " -I --iter iterations\n"); + printf("Usage: %s [-DsbdlHOLC3ZFMvpNR24SI]\n", prog); + puts(" -D --device device to use (default /dev/spidev1.1)\n" + " -s --speed max speed (Hz)\n" + " -d --delay delay (usec)\n" + " -b --bpw bits per word\n" + " -i --input input data from a file (e.g. \"test.bin\")\n" + " -o --output output data to a file (e.g. \"results.bin\")\n" + " -l --loop loopback\n" + " -H --cpha clock phase\n" + " -O --cpol clock polarity\n" + " -L --lsb least significant bit first\n" + " -C --cs-high chip select active high\n" + " -3 --3wire SI/SO signals shared\n" + " -Z --3wire-hiz high impedance turnaround\n" + " -F --rx-cpha-flip flip CPHA on Rx only xfer\n" + " -M --mosi-idle-low leave mosi line low when idle\n" + " -v --verbose Verbose (show tx buffer)\n" + " -p Send data (e.g. \"1234\\xde\\xad\")\n" + " -N --no-cs no chip select\n" + " -R --ready slave pulls low to pause\n" + " -2 --dual dual transfer\n" + " -4 --quad quad transfer\n" + " -8 --octal octal transfer\n" + " -S --size transfer size\n" + " -I --iter iterations\n"); exit(1); } @@ -201,31 +204,34 @@ static void parse_opts(int argc, char *argv[]) { while (1) { static const struct option lopts[] = { - { "device", 1, 0, 'D' }, - { "speed", 1, 0, 's' }, - { "delay", 1, 0, 'd' }, - { "bpw", 1, 0, 'b' }, - { "input", 1, 0, 'i' }, - { "output", 1, 0, 'o' }, - { "loop", 0, 0, 'l' }, - { "cpha", 0, 0, 'H' }, - { "cpol", 0, 0, 'O' }, - { "lsb", 0, 0, 'L' }, - { "cs-high", 0, 0, 'C' }, - { "3wire", 0, 0, '3' }, - { "no-cs", 0, 0, 'N' }, - { "ready", 0, 0, 'R' }, - { "dual", 0, 0, '2' }, - { "verbose", 0, 0, 'v' }, - { "quad", 0, 0, '4' }, - { "octal", 0, 0, '8' }, - { "size", 1, 0, 'S' }, - { "iter", 1, 0, 'I' }, + { "device", 1, 0, 'D' }, + { "speed", 1, 0, 's' }, + { "delay", 1, 0, 'd' }, + { "bpw", 1, 0, 'b' }, + { "input", 1, 0, 'i' }, + { "output", 1, 0, 'o' }, + { "loop", 0, 0, 'l' }, + { "cpha", 0, 0, 'H' }, + { "cpol", 0, 0, 'O' }, + { "lsb", 0, 0, 'L' }, + { "cs-high", 0, 0, 'C' }, + { "3wire", 0, 0, '3' }, + { "3wire-hiz", 0, 0, 'Z' }, + { "rx-cpha-flip", 0, 0, 'F' }, + { "mosi-idle-low", 0, 0, 'M' }, + { "no-cs", 0, 0, 'N' }, + { "ready", 0, 0, 'R' }, + { "dual", 0, 0, '2' }, + { "verbose", 0, 0, 'v' }, + { "quad", 0, 0, '4' }, + { "octal", 0, 0, '8' }, + { "size", 1, 0, 'S' }, + { "iter", 1, 0, 'I' }, { NULL, 0, 0, 0 }, }; int c; - c = getopt_long(argc, argv, "D:s:d:b:i:o:lHOLC3NR248p:vS:I:", + c = getopt_long(argc, argv, "D:s:d:b:i:o:lHOLC3ZFMNR248p:vS:I:", lopts, NULL); if (c == -1) @@ -268,6 +274,15 @@ static void parse_opts(int argc, char *argv[]) case '3': mode |= SPI_3WIRE; break; + case 'Z': + mode |= SPI_3WIRE_HIZ; + break; + case 'F': + mode |= SPI_RX_CPHA_FLIP; + break; + case 'M': + mode |= SPI_MOSI_IDLE_LOW; + break; case 'N': mode |= SPI_NO_CS; break;
Added the three missing spi mode bits SPI_3WIRE_HIZ, SPI_RX_CPHA_FLIP, and SPI_MOSI_IDLE_LOW. Due to the length of the new options, the indentation of the options in the help message was also adjusted for all other options. Signed-off-by: Boerge Struempfel <boerge.struempfel@gmail.com> --- tools/spi/spidev_test.c | 101 +++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 43 deletions(-)