[flashrom] [PATCH] Add Linux SPI support

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Mar 2 23:56:43 CET 2011


Auf 25.02.2011 23:31, Sven Schnelle schrieb:
> Hi List,
>
> this patch adds support for the Linux SPI subsystem. See 
> http://www.kernel.org/doc/Documentation/spi/spidev for a 
> short introduction.
>
> Usage is as follows:
>
> flashrom -p linux_spi:dev=/dev/spidevX.Y
>
> where X is the bus number, and Y device. It accepts an optional
> parameter 'speed' which allows to set the SPI CLK speed in KHz.
>
> I'm using a AVR32 Board
> (http://www.atmel.com/dyn/products/tools_card.asp?tool_id=4102) to
> program my ThinkPad X60, but it should work on every Linux system.
>
> Signed-off-by: Sven Schnelle <svens at stackframe.org>

Thanks for your patch! Review follows.

Cosmetic:
linux_spi_init() has trailing whitespace in some lines, and spaces
instead of tabs in some lines.
Having a man page entry for this would be awesome, but that can be
postponed until the main patch is in.


> Index: Makefile
> ===================================================================
> --- Makefile	(revision 1261)
> +++ Makefile	(working copy)
> @@ -152,6 +152,8 @@
>  # Always enable Bus Pirate SPI for now.
>  CONFIG_BUSPIRATE_SPI ?= yes
>  
> +CONFIG_LINUX_SPI ?= yes

Either set it to no by default, or make the yes conditional on detecting
a Linux target (not host, because DOS binaries are generated on a Linux
host). I'd postpone the Linux detection to a later patch.
Adding a short comment why it defaults to no or a comment under which
circumstances it defaults to yes would be appreciated.


> +
>  # Disable Dediprog SF100 until support is complete and tested.
>  CONFIG_DEDIPROG ?= no
>  
> Index: linux_spi.c
> ===================================================================
> --- linux_spi.c	(revision 0)
> +++ linux_spi.c	(revision 0)
> @@ -0,0 +1,111 @@
> +int linux_spi_init(void)
> +{
> +	char *p, *endp, *dev;
> +	int speed = 0;
>   

int speed, but later you use strtoul which fills an unsigned long.


> +
> +        dev = extract_programmer_param("dev");
> +        if (!dev || !strlen(dev)) {
>   

strlen(dev) can happen if the user specified no device name:
-p linux_spi:dev=
Please handle that case separately (error message, abort).


> +                msg_perr("No spi device given. Use flashrom -p "
> +			 "linux_spi:dev=/dev/spidevX.Y\n");
> +                return 1;
> +        }   
>   

Maybe add a msg_dbg or msg_spew about the device name specified by the user.


> +
> +        p = extract_programmer_param("speed");
> +        if (p && strlen(p)) {
>   


Please handle the same problem for speed=


> +		speed = strtoul(p, &endp, 10) * 1024;
> +		if (p == endp) {
> +			msg_perr("%s: invalid clock: %s\n", __func__, p);
>   

Maybe "%s: invalid clock: %s kHz\n" instead?

> +			return 1;
> +		}
>   

Print the chosen clock frequency here? Check if the supplied clock
frequency is nonzero and nonnegative?


> +        }   
> +
> +	if ((fd = open(dev, O_RDWR)) == -1) {
> +		msg_perr("%s: failed to open %s: %s\n", __func__,
> +			 dev, strerror(errno));
> +		return 1;
> +	}
> +
> +	if (speed > 0 && ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed) == -1) {
>   

Are you 100% sure that this ioctl wants a signed int?
http://www.kernel.org/doc/Documentation/spi/spidev says something about
an u32.


> +		msg_perr("%s: failed to set speed %dHz: %s\n",
> +			 __func__, speed, strerror(errno));
> +		close(fd);
> +		return 1;
> +	}
>   

Not sure if Linux SPI masters use the right CPOL/CPHA and bit ordering
and bits per word by default, so setting SPI_IOC_WR_MODE and
SPI_IOC_WR_LSB_FIRST and SPI_IOC_WR_BITS_PER_WORD would probably be a
very good idea.


> +
> +	buses_supported = CHIP_BUSTYPE_SPI;
> +	spi_controller = SPI_CONTROLLER_LINUX;
> +	return 0;
> +}
> +
> +int linux_spi_shutdown(void)
> +{
> +	if (fd != -1) {
> +		close(fd);
> +		fd = -1;
> +	}
> +	return 0;
> +}
> +
> +int linux_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> +		const unsigned char *txbuf, unsigned char *rxbuf)
> +{
> +	struct spi_ioc_transfer msg[2] = {
> +		{ .tx_buf = (unsigned long)txbuf, .len = writecnt },
> +		{ .rx_buf = (unsigned long)rxbuf, .len = readcnt }
>   

Really unsigned long casts for txbuf and rxbuf?


> +	};
> +
> +	if (fd == -1)
> +		return -1;
> +
> +	if (ioctl(fd, SPI_IOC_MESSAGE(2), msg) == -1) {
> +		msg_cerr("%s: ioctl: %s\n", __func__, strerror(errno));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +int linux_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len)
> +{
> +	return spi_read_chunked(flash, buf, start, len, 12);
>   

Should not be 12, but rather getpagesize() and a comment that max
transfer size is one page.


> +}
> +
> +int linux_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
> +{
> +	return spi_write_chunked(flash, buf, start, len, 12);
>   

Should not be 12, but rather getpagesize()-4 with a similar comment as
above. We lose 4 bytes at the beginning because we have to write
command+addr to the SPI line.


> +}
>   

I think the next iteration will definitely be ackable.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list