Attention is currently required from: Stefan Reinauer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54944 )
Change subject: Add support for TerribleFire TF530 SPI controller ......................................................................
Patch Set 1: Code-Review+1
(11 comments)
File tf530_spi.c:
https://review.coreboot.org/c/flashrom/+/54944/comment/8bf26232_729fe20c PS1, Line 17: * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA I don't think we need this paragraph
https://review.coreboot.org/c/flashrom/+/54944/comment/f761131f_87369065 PS1, Line 20: #if CONFIG_TF530_SPI == 1 Is this really necessary? I know other files have these guards, but I don't think they're actually useful.
https://review.coreboot.org/c/flashrom/+/54944/comment/6e02616e_fa9d92d9 PS1, Line 62: static struct ConfigDev *cd = NULL; There's an ongoing effort to remove global state from programmers. See changes to `digilent_spi.c` in CB:54012 and CB:54067 as an example of what should be changed here.
https://review.coreboot.org/c/flashrom/+/54944/comment/3af232c9_df3b790d PS1, Line 64: #define TF53x_CTRL_CS0 1 : #define TF53x_CTRL_CS1 2 : #define TF53x_CTRL_BUSY 4 : #define TF53x_CTRL_CS2 8 Looks like these are bits within a register. Why not rewrite them as shifts?
#define TF53x_CTRL_CS0 (1 << 0) #define TF53x_CTRL_CS1 (1 << 1) #define TF53x_CTRL_BUSY (1 << 2) #define TF53x_CTRL_CS2 (1 << 3)
https://review.coreboot.org/c/flashrom/+/54944/comment/1b4e7db9_bf3e29a2 PS1, Line 71: uint8_t busy = 0; : : while (busy == 0) { : busy = port->ctrl & TF53x_CTRL_BUSY; : } nit: I'd remove the `busy` variable, the name is confusing (it's active-low):
do {} while ((port->ctrl & TF53x_CTRL_BUSY) == 0);
Same below.
https://review.coreboot.org/c/flashrom/+/54944/comment/793764a9_450ff517 PS1, Line 106: return 0; Don't we need to release `struct ConfigDev *cd` or similar?
https://review.coreboot.org/c/flashrom/+/54944/comment/57caca48_0d5a32ba PS1, Line 119: unsigned int Support for for-loop initial declarations was introduced with C99. Some old compilers that do not use C99 mode by default may complain about it. It's not a big deal in this particular case because this programmer is tied to a particular OS, but it's something to keep in mind.
https://review.coreboot.org/c/flashrom/+/54944/comment/4a47284a_29851863 PS1, Line 125: uint8_t volatile nit: `volatile uint8_t`
Also, would this work?
(volatile uint8_t)spi_recv_byte(port);
https://review.coreboot.org/c/flashrom/+/54944/comment/b9a142c6_71e09fa3 PS1, Line 149: : struct Library *ExpansionBase = NULL; : : if ((ExpansionBase = (struct Library *) : OpenLibrary((unsigned char *)"expansion.library", 0L)) == NULL) { Moving the assignment out of the if-block's condition should be easier to read:
struct Library *ExpansionBase = (struct Library *) OpenLibrary((unsigned char *)"expansion.library", 0L);
if ((ExpansionBase == NULL) { return 0; }
Also, are the type casts really necessary? If not, please drop them.
https://review.coreboot.org/c/flashrom/+/54944/comment/afa638da_69ab5f68 PS1, Line 154: 0 Why return 0?
https://review.coreboot.org/c/flashrom/+/54944/comment/dda622a6_9d03416c PS1, Line 169: register_spi_master(&spi_master_tf53x, NULL); This function returns something. Why not propagate the return value?
return register_spi_master(&spi_master_tf53x, NULL);
-
Angel Pons (Code Review)