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:
(13 comments)
https://review.coreboot.org/c/flashrom/+/39687/5/Makefile File Makefile:
https://review.coreboot.org/c/flashrom/+/39687/5/Makefile@675 PS5, Line 675: CONFIG_LSPCON_I2C_SPI ?= yes Sorry forgot to make this as false. Will update on next iteration.
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? […]
Done
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@275 PS4, Line 275: bit
byte
Done
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@314 PS4, Line 314: wait_100_ms
wait_100ms
Done
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@314 PS4, Line 314: 100000000
`(unsigned)1e8` is more readable.
Done
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? Otherwis […]
Yes this can be done in a single write, I just want to separate them to make this clear. I will update this to use write_register if that's fine.
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?
I think it is the requirement for i2c data transfer to write the start offset before the write data, this is useful if the write has to be done slowly like 32 byte a time. Also make two separate write that send 0 first and followed by write data does not work. I would assume we will need to keep this, does it make sense?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@406 PS4, Line 406: 256
Are you sure about these?
Actually no, I have go through the code and looks like that applied to the send_command method eventually and thus this max value should be 16. While for the max write, I notice the value get passed in eventually as out_len here: https://github.com/flashrom/flashrom/blob/ef78de4a21323b8c459337356289218211..., that means the limit should be 16 - 4 is that correct?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@415 PS4, Line 415: stop
stopped
Done
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@431 PS4, Line 431: ()
(void)
Done
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@492 PS4, Line 492: spi
SPI
Done
https://review.coreboot.org/c/flashrom/+/39687/4/meson_options.txt File meson_options.txt:
https://review.coreboot.org/c/flashrom/+/39687/4/meson_options.txt@38 PS4, Line 38: true
Do we perhaps want to default to false as the i2c helper only currently supports Linux and thus woul […]
Done
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?
Done