[flashrom] [PATCH] Add Linux SPI support

[Sven Schnelle]

Hi Uwe,

Uwe Hermann <uwe at hermann-uwe.de> writes:

>> > +int linux_spi_init(void)
>> > +{
>> > +	char *p, *endp, *dev;
>> > +	int speed = 0;
>> >   
>> 
>> int speed, but later you use strtoul which fills an unsigned long.
>
> Fixed in the attached patch.
>  
>  
>> > +        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).
>

How can that happen? The NULL pointer and zero length cases are handled
correctly. With the current trunk i get:

# ./flashrom -r test.bin -p linux_spi:dev=
flashrom v0.9.4-r1427 on Linux 2.6.38+ (avr32), built with GCC
4.2.4-atmel.1.1.3.avr32linux.1, big endian
flashrom is free software, get the source code at
http://www.flashrom.org

Calibrating delay loop... OK.
No SPI device given. Use flashrom -p linux_spi:dev=/dev/spidevX.Y
Error: Programmer initialization failed.

Which looks ok for me.

>> > +                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.
>
> Done.
>  
>  
>> > +        p = extract_programmer_param("speed");
>> > +        if (p && strlen(p)) {
>> >   
>> 
>> 
>> Please handle the same problem for speed=
>  
> Same question here :)
>  
>> > +        }   
>> > +
>> > +	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.
>  
> Fixed.
>
>  
>> 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.

The spi_board_info structure in the board specific linux kernel code
specify the default values. Such a structure has to exist for every
single SPI device that sits on the Board, so we have correct default
values set in the Kernel. As a feature we might still make that
configurable later.

Signed-off-by: Sven Schnelle <svens at stackframe.org>

Index: linux_spi.c
===================================================================
--- linux_spi.c	(revision 1427)
+++ linux_spi.c	(working copy)
@@ -54,7 +54,7 @@
 int linux_spi_init(void)
 {
 	char *p, *endp, *dev;
-	int speed = 0;
+	uint32_t speed = 0;
 
 	dev = extract_programmer_param("dev");
 	if (!dev || !strlen(dev)) {
@@ -65,31 +65,35 @@
 
 	p = extract_programmer_param("speed");
 	if (p && strlen(p)) {
-		speed = strtoul(p, &endp, 10) * 1024;
+		speed = (uint32_t)strtoul(p, &endp, 10) * 1024;
 		if (p == endp) {
 			msg_perr("%s: invalid clock: %s kHz\n", __func__, p);
 			return 1;
 		}
 	}
 
+	msg_pdbg("Using device %s\n", dev);
 	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) {
-		msg_perr("%s: failed to set speed %dHz: %s\n",
-			 __func__, speed, strerror(errno));
-		close(fd);
-		return 1;
-	}
+	if (speed > 0) {
+		if (ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed) == -1) {
+			msg_perr("%s: failed to set speed %dHz: %s\n",
+				 __func__, speed, strerror(errno));
+			close(fd);
+			return 1;
+		}
 
+		msg_pdbg("Using %d KHz clock\n", speed);
+        }
+
 	if (register_shutdown(linux_spi_shutdown, NULL))
 		return 1;
 
 	register_spi_programmer(&spi_programmer_linux);
-
 	return 0;
 }