Sam McNally 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:
(5 comments)
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c File lspcon_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@270 PS4, Line 270: int ret = lspcon_i2c_spi_disable_all_protection(fd); Are you sure all of this is necessary for every command and that none of this duplicates commands that the other parts of flashrom's SPI infrastructure will generate?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@275 PS4, Line 275: bit byte
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@333 PS4, Line 333: uint8_t command_byte1[] = { ROMADDR_BYTE1, (offset >> 8) & 0xff }; Given ROMADDR_BYTE1 and ROMADDR_BYTE2 are adjacent, can this not be done in a single write? Otherwise, this should use lspcon_i2c_spi_write_register.
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@406 PS4, Line 406: 256 Are you sure about these?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@431 PS4, Line 431: () (void)