Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39687 )
Change subject: lspcon_i2c_spi.c: Add SPI-master support for PS17{5,6} ......................................................................
Patch Set 4:
(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.
You can move this down into the body and have a shorter summary line: […]
Done
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.
Done
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?
Done
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?
Most likely this won't be true, but this prevent some cases that `register_lspcon_i2c_spi_master` might not register customized master with proper data.
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.
Done
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.
Done
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(). […]
Done
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@500 PS3, Line 500:
trim \n
I am pretty sure there is no new line at the end, is that just a display issue?