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:
(4 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 nit: _data is a bit redundant and verbose; It might be cleaner and more direct to just use: `struct it85spi` here, and later, in code `struct it85spi it85spi;`
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@77 PS5, Line 77: unsigned int shm_io_base; : unsigned char *ce_high, *ce_low; : int it85xx_scratch_rom_reenter; : nit: probably want tab indents here in struct definition and for all the code below. I recommend spending some time figuring out how to set up your code editor to follow the style guide for this code base which uses tabs. I'm a bit surprised this isn't picked up by a linter on upload?
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; : } : Perhaps just return NULL on error:
static struct it85spi_data *get_data_from_context(const struct flashctx *flash) { if (!flash || !flash->mst || !flash->mst->spi.data) { msg_perr("Unable to extract data from flash context.\n"); return NULL; } return flash->mst->spi.data; }
And then below:
data = get_data_from_context(flash); if (!data) return SPI_GENERIC_ERROR;
But really, since this is the only place where we extract data from flash, probably no need for this separate function, and just inline the intermediate flash NULL checks.... and further, is it even possible for it85xx_spi_send_command() to be called with a NULL flash or flash->mst? If not, keep the code simple and just let it crash here?
https://review.coreboot.org/c/flashrom/+/47655/5/it85spi.c@321 PS5, Line 321: spi_master_it85xx.data = data; If spi_master_it85xx:it85spi_data is 1:1, and spi_master_it85xx is a static global, then there doesn't seem to be any reason why it85spi_data should be allocated at runtime. Just define it85spi_data a static global as well and skip the calloc / free and associated complexity. Perhaps something like:
static struct it85spi it85spi; static const struct spi_master spi_master_it85xx = { ... .data = &it85spi; };