Angel Pons 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:
(6 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@39 PS4, Line 39: #define SWSPI_WDATA_CLEAR_STATUS How about adding an extra space between `#define` and the macro name? #define SWSPI_WDATA 0x90 #define SWSPI_WDATA_CLEAR_STATUS 0x00
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@314 PS4, Line 314: 100000000
`(unsigned)1e8` is more readable.
Or (100 * 1000 * 1000)
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@364 PS4, Line 364: uint8_t write_buffer[len + 1];
I find these VLA usages and clever ptr arithmetic to idx after the few elem a little unsettling. […]
This is quite unsettling, indeed. Is there any reason to not use a fixed-size buffer?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@365 PS4, Line 365: write_buffer[0] = 0 Why the special treatment for position zero?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@492 PS4, Line 492: spi SPI
https://review.coreboot.org/c/flashrom/+/39687/4/programmer.h File programmer.h:
https://review.coreboot.org/c/flashrom/+/39687/4/programmer.h@818 PS4, Line 818: Shouldn't this be guarded like on line 579?