Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39687 )
Change subject: lspcon_i2c_spi.c: Add support for the i2c-controlled SPI master in PS175/176. ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/flashrom/+/39687/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/39687/3//COMMIT_MSG@7 PS3, Line 7: lspcon_i2c_spi.c: Add support for the i2c-controlled SPI master in PS175/176.
Please remove the dot/period at the end of the commit message summary.
You can move this down into the body and have a shorter summary line:
``` lspcon_i2c_spi.c: Add SPI-master support for PS17{5,6}
This adds support for the Parades lspcon usb-c to dp protocol translator part that is i2c-controlled. The support allows the host to reach the SPI rom that hangs off the part where it stores its firmware. ```
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c File lspcon_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@33 PS3, Line 33: #define FUNCTION_ERR -1 use flashrom enum of native error codes.
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@80 PS3, Line 80: uint8_t data_size; : const uint8_t *data; swap these two for readability?
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@105 PS3, Line 105: if (!flash || !flash->mst || !flash->mst->spi.data) { under what conditions is this true in practice?
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@110 PS3, Line 110: (const struct lspcon_i2c_spi_data *)flash->mst->spi.data case to an intermediate and then extract.
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@418 PS3, Line 418: int fd = ((struct lspcon_i2c_spi_data *)data)->fd; cast it then extract the value.
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@487 PS3, Line 487: struct lspcon_i2c_spi_data data = { fd }; : void *data_ptr = calloc(1, sizeof(data)); just calloc the heap and set the member, avoid the memcpy().
``` struct lspcon_i2c_spi_data * data = calloc(1, sizeof(struct lspcon_i2c_spi_data)); data->fd = fd; ```
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@500 PS3, Line 500: trim \n