Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48840 )
Change subject: realtek_mst_i2c_spi.c: Update PAGE_SIZE and fix write ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/48840/2/realtek_mst_i2c_spi.c@387 PS2, Line 387: uint8_t block_idx = (start + i) >> 16; : uint8_t page_idx = (start + i) >> 8; : uint8_t byte_idx = start + i;
I don't like that this code relies on overflow behavior. […]
Generally agree with the first point but I think it's out of scope of this change and rather have this merged first to fix the immediate problem of brokenness then follow up with style clean ups.
Same for the second point, although I see what you are saying about consolidating the pattern it is also nice to have realtek_mst_i2c_spi_map_page() with its current signature? The pattern being on both R/W primitives was speculative before as we worked our way though understanding the MCU. Therefore I rather we keep things flexible until everything definitely fully works on all hw variants. Like you said before, we should avoid bolting on too many patches into the same change.