On Fri, 02 Mar 2012 22:43:44 +0100 Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> > > --- > > 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). -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
