[flashrom] [PATCH] linux_spi.c: set SPI mode, word justification and bits per word on init.

[Stefan Tauner]

On Fri, 02 Mar 2012 22:43:44 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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 at 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).
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner