Re: [flashrom] [PATCH] Add Linux SPI support

[Carl-Daniel Hailfinger]

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

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