[L] Change in flashrom[main]: spidriver: Add support for the Excamera Labs SPIDriver programmer

[Simon Arlott (Code Review)]

Attention is currently required from: Anastasia Klimchuk, Peter Marheine.

Simon Arlott has posted comments on this change by Simon Arlott. ( https://review.coreboot.org/c/flashrom/+/86411?usp=email )

Change subject: spidriver: Add support for the Excamera Labs SPIDriver programmer
......................................................................

Patch Set 2:

(10 comments)

Patchset:

PS2:
> Simon, nice to meet you, thank you for the patch! This is really cool :)  […]

I've added an entry to the release notes.

I don't think I can maintain support for this programmer, it's quite slow to use and I'm looking at other USB SPI options but I only do occasional flash chip reads.

Commit Message:

https://review.coreboot.org/c/flashrom/+/86411/comment/96dd7a81_4cd3ec70?usp... :
PS2, Line 9: This is a SPI hardware interface with a display (https://spidriver.com/),
           : connected as an FT230X USB serial device at a fixed baud rate of 460800.
           : 
           : Firmware: https://github.com/jamesbowman/spidriver
           : Protocol: https://github.com/jamesbowman/spidriver/blob/master/protocol.md
> These lines would be very good to add in the beginning of manpage entry that you added. […]

Done

File include/programmer.h:

https://review.coreboot.org/c/flashrom/+/86411/comment/0fc60cf6_0d74222f?usp... :
PS2, Line 101: extern const struct programmer_entry programmer_spidriver;
> You need to swap lines 100 and 101 to be in nice alphabetical order. […]

Done

File spidriver.c:

https://review.coreboot.org/c/flashrom/+/86411/comment/d2a61cf3_05d95c35?usp... :
PS2, Line 19:  */
> Maybe add link to firmware here as well? (from commit message)

Done

https://review.coreboot.org/c/flashrom/+/86411/comment/98830cf0_7f6dfbe2?usp... :
PS2, Line 81: {
> Why do you need this bracket (and closing on line 101)? Is there a reason for this block?

It reduces the scope of "buf" so that it's not being shadowed later and can't accidentally be used.

https://review.coreboot.org/c/flashrom/+/86411/comment/6aa405e7_d98656fd?usp... :
PS2, Line 136: /* Read and write */
> I assume that's just because of how SPI works, since the periperhal always shifts bits in from the h […]

There's no read API, because something must always be written on the SPI bus in order to read from it. Typically this would be 0x00 for SPI and the preceding command would prepare for a read operation.

https://github.com/jamesbowman/spidriver/blob/master/protocol.md

https://review.coreboot.org/c/flashrom/+/86411/comment/76b2bb98_1ebd23b4?usp... :
PS2, Line 179: 	.max_data_read	= MAX_DATA_READ_UNLIMITED,
             : 	.max_data_write	= MAX_DATA_WRITE_UNLIMITED,
> The indentation here needs to be aligned with the rest of `=`

Done

https://review.coreboot.org/c/flashrom/+/86411/comment/afb40d7a_c3d152c4?usp... :
PS2, Line 211: Invalid SPI mode %ld
> Lets add the second part to the error message: "Valid values are <... […]

Done

https://review.coreboot.org/c/flashrom/+/86411/comment/94e0943c_69c2b049?usp... :
PS2, Line 212: return 1;
> So invalid `mode` param returns an error, but invalid values for `a` and `b` below continue running  […]

Done

https://review.coreboot.org/c/flashrom/+/86411/comment/0edfc7b2_9e1797f0?usp... :
PS2, Line 309: version 1
> Is it useful to print `fw_version` here instead, as an in addition to ?

Done