[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