Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@76 PS5, Line 76: it85spi_data
This is consistent with the rest of the tree, please keep things consistent and we can argue about t […]
sgtm. Consistency is best.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@227 PS5, Line 227: static int get_data_from_context(const struct flashctx *flash, struct it85spi_data **data) : { : if (!flash || !flash->mst || !flash->mst->spi.data) { : msg_perr("Unable to extract data from flash context.\n"); : return SPI_GENERIC_ERROR; : } : *data = (struct it85spi_data *) flash->mst->spi.data; : : return 0; : } :
While true that this could be inlined it would be inconsistent with other drivers. […]
Consistency is good. Better would be to implement the unit tests in this CL that show the value in adding this extra complexity. Otherwise I still recommend the simpler and direct implementation for now, and then adding this complexity when there is value in doing so.
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@321 PS5, Line 321: spi_master_it85xx.data = data;
We explicitly do not want static globals of any kind though, that would be counter to the entire pat […]
Removing "static globals of any kind" is not a goal in and of itself. They are quite useful and efficient. There has to be some motivation to do so, such as when runtime allocation provides some value.
The value of this patch is that it collects scattered device specific file-global static state variables into a single context structure, and then makes the functions pure by having them operate on a passed-in context struct rather than directly accessing global state.
I don't see any additional value, at this point, of allocating the context struct on the heap rather than as a static global. AFAICT, it just introduces extra runtime complexity.
Perhaps the value is realized by some other patches that perform additional refactoring and/or add unit tests?