Re: [flashrom] [PATCH] linux_spi.c: set SPI mode, word justification and bits per word on init.
Am 02.03.2012 00:43 schrieb Stefan Tauner:
Previously we relied on a correctly set up state.
untested.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
linux_spi.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/linux_spi.c b/linux_spi.c index d994389..d29c59a 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -57,6 +57,8 @@ int linux_spi_init(void) { char *p, *endp, *dev; uint32_t speed = 0;
- /* FIXME: make the following configurable by CLI options. */
- uint8_t mode = SPI_MODE_0, lsb = 0, bits = 0; /* mode 0, msb first, 8 bits */
Can you move that comment above the variable definitions? Where should we note that SPI_MODE_0 also implies CS# active low?
dev = extract_programmer_param("dev"); if (!dev || !strlen(dev)) { @@ -92,6 +94,27 @@ int linux_spi_init(void) msg_pdbg("Using %d kHz clock\n", speed); }
- if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) {
msg_perr("%s: failed to set SPI mode to %u: %s\n",__func__, mode, strerror(errno));close(fd);return 1;- }
- if (ioctl(fd, SPI_IOC_WR_LSB_FIRST, &lsb) == -1) {
msg_perr("%s: failed to set SPI justification to %u: %s\n",__func__, lsb, strerror(errno));
This message would benefit from an explanation what SPI justification is. Suggestion: msg_perr("%s: failed to set SPI bit order to %s first: %s\n", __func_, lsb ? "LSB" : "MSB", strerror(errno));
close(fd);return 1;- }
- if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) {
msg_perr("%s: failed to set the number of bits in an SPI word to %u: %s\n",__func__, bits, strerror(errno));
bits is 0. The error message would suggest that we tried to set the number of bits to 0. Does 0 also mean 8 bits, or would we have to set 8 bits with bits=8?
close(fd);return 1;- }
- if (register_shutdown(linux_spi_shutdown, NULL)) return 1;
As an alternative, we could avoid the whole close(fd) dance by calling register_shutdown() first, and then letting it do the work for us automatically after we return 1.
Regards, Carl-Daniel
On Fri, 02 Mar 2012 22:43:44 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 02.03.2012 00:43 schrieb Stefan Tauner:
Previously we relied on a correctly set up state.
untested.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
linux_spi.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/linux_spi.c b/linux_spi.c index d994389..d29c59a 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -57,6 +57,8 @@ int linux_spi_init(void) { char *p, *endp, *dev; uint32_t speed = 0;
- /* FIXME: make the following configurable by CLI options. */
- uint8_t mode = SPI_MODE_0, lsb = 0, bits = 0; /* mode 0, msb first, 8 bits */
Can you move that comment above the variable definitions?
done, reads now: /* SPI mode 0, msb first, 8 bits (i.e. value=0) */
Where should we note that SPI_MODE_0 also implies CS# active low?
it does not. the test program seems to be outdated, the actual code masks the CPOL/CPHA bits.
dev = extract_programmer_param("dev"); if (!dev || !strlen(dev)) { @@ -92,6 +94,27 @@ int linux_spi_init(void) msg_pdbg("Using %d kHz clock\n", speed); }
- if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) {
msg_perr("%s: failed to set SPI mode to %u: %s\n",__func__, mode, strerror(errno));close(fd);return 1;- }
- if (ioctl(fd, SPI_IOC_WR_LSB_FIRST, &lsb) == -1) {
msg_perr("%s: failed to set SPI justification to %u: %s\n",__func__, lsb, strerror(errno));This message would benefit from an explanation what SPI justification is. Suggestion: msg_perr("%s: failed to set SPI bit order to %s first: %s\n", __func_, lsb ? "LSB" : "MSB", strerror(errno));
right, was too lazy to think about a better term/solution at the time; fixed.
close(fd);return 1;- }
- if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) {
msg_perr("%s: failed to set the number of bits in an SPI word to %u: %s\n",__func__, bits, strerror(errno));bits is 0. The error message would suggest that we tried to set the number of bits to 0. Does 0 also mean 8 bits, or would we have to set 8 bits with bits=8?
bits = 0 is the only defined value in the documentation and is actually the only one implemented in the code and means 8 bits per word. i have changed the message to this: msg_perr("%s: failed to set the number of bits per SPI word to %s: %s\n", __func__, bits == 0 ? "8" : "<undef>", strerror(errno));
close(fd);return 1;- }
- if (register_shutdown(linux_spi_shutdown, NULL)) return 1;
As an alternative, we could avoid the whole close(fd) dance by calling register_shutdown() first, and then letting it do the work for us automatically after we return 1.
how do we do it in other programmers? we should probably define and document a single suggested way so that we dont have to discuss this every time. :)
in this particular case i think it makes sense. in general relying on the shutdown function only may be a bit hard to grasp/implement for complicated init functions that allocate/manipulate lots of stuff (e.g. serprog).
Am 02.03.2012 23:51 schrieb Stefan Tauner:
On Fri, 02 Mar 2012 22:43:44 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 02.03.2012 00:43 schrieb Stefan Tauner:
Previously we relied on a correctly set up state.
untested.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
linux_spi.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/linux_spi.c b/linux_spi.c index d994389..d29c59a 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -57,6 +57,8 @@ int linux_spi_init(void) { char *p, *endp, *dev; uint32_t speed = 0;
- /* FIXME: make the following configurable by CLI options. */
- uint8_t mode = SPI_MODE_0, lsb = 0, bits = 0; /* mode 0, msb first, 8 bits */
Can you move that comment above the variable definitions?
done, reads now: /* SPI mode 0, msb first, 8 bits (i.e. value=0) */
Where should we note that SPI_MODE_0 also implies CS# active low?
it does not. the test program seems to be outdated, the actual code masks the CPOL/CPHA bits.
Let me rephrase that: Setting the SPI mode handles CPOL/CPHA/CS/LSB/... all in one. The bits which are not set have their default value (0). http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/drivers/spi/spidev.c#L72 #define SPI_MODE_MASK (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP | SPI_NO_CS | SPI_READY)
dev = extract_programmer_param("dev"); if (!dev || !strlen(dev)) { @@ -92,6 +94,27 @@ int linux_spi_init(void) msg_pdbg("Using %d kHz clock\n", speed); }
- if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) {
msg_perr("%s: failed to set SPI mode to %u: %s\n",__func__, mode, strerror(errno));close(fd);return 1;- }
- if (ioctl(fd, SPI_IOC_WR_LSB_FIRST, &lsb) == -1) {
msg_perr("%s: failed to set SPI justification to %u: %s\n",__func__, lsb, strerror(errno));This message would benefit from an explanation what SPI justification is. Suggestion: msg_perr("%s: failed to set SPI bit order to %s first: %s\n", __func_, lsb ? "LSB" : "MSB", strerror(errno));
right, was too lazy to think about a better term/solution at the time; fixed.
close(fd);return 1;- }
- if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) {
msg_perr("%s: failed to set the number of bits in an SPI word to %u: %s\n",__func__, bits, strerror(errno));bits is 0. The error message would suggest that we tried to set the number of bits to 0. Does 0 also mean 8 bits, or would we have to set 8 bits with bits=8?
bits = 0 is the only defined value in the documentation and is actually the only one implemented in the code and means 8 bits per word.
The documentation is explicit that bits is the number of bits, unless bits=0, in which case it is treated as 8 bits. I also have checked the code in the Linux kernel and it agrees with me.
Note that retval in ioctl handling is _not_ the value entered by the user, but an error/success code for accessing user data. http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/drivers/spi/spidev.c#L408 http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/drivers/spi/spi.c#L727
i have changed the message to this: msg_perr("%s: failed to set the number of bits per SPI word to %s: %s\n", __func__, bits == 0 ? "8" : "<undef>", strerror(errno));
close(fd);return 1;- }
- if (register_shutdown(linux_spi_shutdown, NULL)) return 1;
As an alternative, we could avoid the whole close(fd) dance by calling register_shutdown() first, and then letting it do the work for us automatically after we return 1.
how do we do it in other programmers? we should probably define and document a single suggested way so that we dont have to discuss this every time. :)
A single suggested way (or maybe two ways, depending on the hardware interface design we're using) is a good idea as long as it's not a hard unbreakable rule afterwards. The following two ways seem to make sense for linux_spi IMHO: ret = 1; goto out_shutdown; or setting everything after register_shutdown.
If we want to restore old settings on shutdown, it gets a bit hackier. One possible way would be: int restore_settings; init (){ open fd else return 1; restore_settings=0; register_shutdown() else return 1; get all settings and store them in file-level variables or a file-level struct else return 1; restore_settings=1; set all settings else return 1; return 0; } shutdown() { if (restore_settings){ restore settings; } close(fd); }
in this particular case i think it makes sense. in general relying on the shutdown function only may be a bit hard to grasp/implement for complicated init functions that allocate/manipulate lots of stuff (e.g. serprog).
Agreed.
Side note: How should we handle a failed register_shutdown()? Call the shutdown function manually and return 1? Or is there any other good way to clean up?
Regards, Carl-Daniel
Previously we relied on a correctly set up state.
Also, we start to rely on the shutdown function for cleanup after registering it, i.e. we no longer explicitly call close(fd) after register_shutdown().
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- linux_spi.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/linux_spi.c b/linux_spi.c index d994389..2ae681f 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -57,6 +57,10 @@ int linux_spi_init(void) { char *p, *endp, *dev; uint32_t speed = 0; + /* FIXME: make the following configurable by CLI options. */ + /* SPI mode 0 (beware this also includes: MSB first, CS active low and others */ + const uint8_t mode = SPI_MODE_0; + const uint8_t bits = 8;
dev = extract_programmer_param("dev"); if (!dev || !strlen(dev)) { @@ -81,19 +85,31 @@ int linux_spi_init(void) return 1; }
+ if (register_shutdown(linux_spi_shutdown, NULL)) + return 1; + /* We rely on the shutdown function for cleanup from here on. */ + if (speed > 0) { if (ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed) == -1) { - msg_perr("%s: failed to set speed %dHz: %s\n", + msg_perr("%s: failed to set speed to %d Hz: %s\n", __func__, speed, strerror(errno)); - close(fd); return 1; }
msg_pdbg("Using %d kHz clock\n", speed); }
- if (register_shutdown(linux_spi_shutdown, NULL)) + if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) { + msg_perr("%s: failed to set SPI mode to 0x%02x: %s\n", + __func__, mode, strerror(errno)); return 1; + } + + if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) { + msg_perr("%s: failed to set the number of bits per SPI word to %u: %s\n", + __func__, bits == 0 ? 8 : bits, strerror(errno)); + return 1; + }
register_spi_programmer(&spi_programmer_linux);
-
Carl-Daniel Hailfinger -
Stefan Tauner