Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41779 ) Change subject: lspcon_i2c_spi.c: Add debug message that shows the active block ...................................................................... Patch Set 2: (7 comments) https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c File lspcon_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@476 PS2, Line 476: static void lspcon_i2c_spi_boot_block_info(int fd) move this function after `lspcon_i2c_spi_map_page(int fd, unsigned int offset)` https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@480 PS2, Line 480: ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0xc0); : ret |= lspcon_i2c_spi_write_register(fd, 0xbc, 0x40); The first two magics programmed here are equiv to `lspcon_i2c_spi_reset_mpu_stop(int fd) {}` and so not needed. https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@482 PS2, Line 482: ret |= lspcon_i2c_spi_write_register(fd, 0x8e, 0x00); : ret |= lspcon_i2c_spi_write_register(fd, 0x8f, 0x00); Put a comment above, `/* Set rom addr to beginning. */` and, ``` #define ROMADDR_BYTE1 0x8e #define ROMADDR_BYTE2 0x8f ``` so you can just use `lspcon_i2c_spi_map_page(int fd, unsigned int offset)` https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@484 PS2, Line 484: struct timespec wait_10ms = { 0, (unsigned)1e7 }; I suspect this wait is for the unnecessary MPU reset above, so you can get rid of it here. https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@487 PS2, Line 487: 0x0e Perhaps this should be a #define or have a inline comment /* .. */ that has the name of what the vendor calls this special command magic. https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@490 PS2, Line 490: { {} unnecessary for single lines. https://review.coreboot.org/c/flashrom/+/41779/2/lspcon_i2c_spi.c@491 PS2, Line 491: pdbg("F msg_perr() and use `%s: Failed to..` with __func__. As mentioned before, consider yourself as a user seeing the word "Error/Failed" in a log - what are you going to grep for? -- To view, visit https://review.coreboot.org/c/flashrom/+/41779 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b3022542f98f114097c6f39254ef4c2cf188c90 Gerrit-Change-Number: 41779 Gerrit-PatchSet: 2 Gerrit-Owner: Shiyu Sun <sshiyu@google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@google.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Sam McNally <sammc@google.com> Gerrit-Comment-Date: Fri, 29 May 2020 08:31:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment