Attention is currently required from: Paul Menzel, Angel Pons. Stefan Reinauer 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:
(7 comments)
Patchset:
PS1: Thanks for the review!
File tf530_spi.c:
https://review.coreboot.org/c/flashrom/+/54944/comment/4a524533_56212800 PS1, Line 4: * Copyright (C) 2019 Steven J Leary
This differs from the commit author data.
Steven did the original port and published it on his github.
https://review.coreboot.org/c/flashrom/+/54944/comment/044e3d14_3b06c2cd 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 u […]
It is modeled after the LINUX_SPI driver. I did not try to create an improvement over the other drivers but rather keep the consistency with what's there. Is this not recommended?
https://review.coreboot.org/c/flashrom/+/54944/comment/b9ed6081_6ed885c2 PS1, Line 62: static struct ConfigDev *cd = NULL;
There's an ongoing effort to remove global state from programmers. See changes to `digilent_spi. […]
I'll have a look at this again. I noticed, but since this programmer is on the CPU board itself, there is no chance that this code will ever be called reentrantly.
This is also a global (system wide) handle, e.g. the same value for all tasks in the system.
https://review.coreboot.org/c/flashrom/+/54944/comment/57bb84c4_30602279 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: […]
Yeah they're necessary. :( I'll reorg.
https://review.coreboot.org/c/flashrom/+/54944/comment/13d2311c_e24544ce PS1, Line 154: 0
Why return 0?
Good catch
https://review.coreboot.org/c/flashrom/+/54944/comment/4dd773ac_47f8ae7a PS1, Line 169: register_spi_master(&spi_master_tf53x, NULL);
This function returns something. Why not propagate the return value? […]
Ack