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 7:
(5 comments)
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@105 PS3, Line 105: if (!flash || !flash->mst || !flash->mst->spi.data) {
Most likely this won't be true, but this prevent some cases that `register_lspcon_i2c_spi_master` mi […]
Does it make sense or we want to remove the check?
https://review.coreboot.org/c/flashrom/+/39687/3/lspcon_i2c_spi.c@500 PS3, Line 500:
I am pretty sure there is no new line at the end, is that just a display issue?
Done
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 th […]
Sorry thought I have replied this but somehow it's missing. On the tracking bug I think the vendor do state that we probably need to disable protection before every write attempt, as well as the register protection. As far as I know I didn't see there is any protection disable operation happens with the process. The only thing I noticed was https://github.com/flashrom/flashrom/blob/cd8aeba7f1cee4c2bd1f8598009fc3e6e7... defines a disable block protect function, but that only invoked inside some local function but seems not get called on flash process(?). Not sure if that make sense and Angel can you help verify that? Thanks!
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@364 PS4, Line 364: uint8_t write_buffer[len + 1];
This is quite unsettling, indeed. […]
We can use a fixed size buffer with buf[256 + 1] since that'd be the max size. But does it really helps lot?
https://review.coreboot.org/c/flashrom/+/39687/4/lspcon_i2c_spi.c@365 PS4, Line 365: write_buffer[0] = 0
I think it is the requirement for i2c data transfer to write the start offset before the write data, […]
Do you have any idea if we have to change this or resolve the comment?