Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Factor out global state ......................................................................
Patch Set 1:
(13 comments)
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@7 PS1, Line 7: Factor out global state Refactor singleton states into reentrant pattern
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@9 PS1, Line 9: Moves global state into spi_master data. Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver.
https://review.coreboot.org/c/flashrom/+/47655/1//COMMIT_MSG@11 PS1, Line 11: BUGS BUG=b:172876667 TEST=builds
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@a273 PS1, Line 273: static int it85xx_spi_send_command(const struct flashctx *flash, : unsigned int writecnt, unsigned int readcnt, : const unsigned char *writearr, : unsigned char *readarr); this forward declaration can be deleted if in a patch before this one - you will need to move the it85xx_spi_send_command() function before the `struct spi_master spi_master_it85xx` declaration.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@a278 PS1, Line 278: static const struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : }; : keep this here.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@87 PS1, Line 87: static struct spi_master spi_master_it85xx = { : .max_data_read = 64, : .max_data_write = 64, : .command = it85xx_spi_send_command, : .multicommand = default_spi_send_multicommand, : .read = default_spi_read, : .write_256 = default_spi_write_256, : .write_aai = default_spi_write_aai, : }; actually this shouldn't move
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@235 PS1, Line 235: struct it85_data *drv_data = (struct it85_data *)data; calling it85xx_exit_scratch_rom(data) should auto cast the void * so no need for this line.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@235 PS1, Line 235: struct it85_data *drv_data = (struct it85_data *)data; calling it85xx_exit_scratch_rom(data) should auto cast the void * so no need for this line.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@238 PS1, Line 238: _rom(drv_data); after this line you should free data
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@248 PS1, Line 248: if (!data) { : msg_perr("Out of memory!\n"); : return 1; : } ``` if (!data) { msg_perr("Unable to allocate space for extra SPI master data.\n"); return SPI_GENERIC_ERROR; } ```
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@258 PS1, Line 258: if (register_shutdown(it85xx_shutdown, data)) { : free(data); : return 1; : } move to function eulogy.
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@309 PS1, Line 309: msg_pdbg("FWH: %s():%d ret=%d\n", __func__, __LINE__, ret); : if (!ret) { as a patch before this one perhaps remove this debug line and invert the if logic to take the rest of the function out of the if scope and instead return early on failure i.e., `if (ret) return ret;`
and at the end of the function return 0;
https://review.coreboot.org/c/flashrom/+/47655/1/it85spi.c@343 PS1, Line 343: if (!(flash->mst)) { : msg_perr("No data in flash context!\n"); : return 1; : } you do not need this validation as it occurs already in the core. The dispatch site will always provide a valid mst pointer address.