Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/25082 )
Change subject: [WIP]src/drivers/spi: Read Winbond's flash protection bits ......................................................................
Patch Set 1:
(10 comments)
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/spi_flash.c@69 PS1, Line 69: printk(BIOS_WARNING, "SF: Failed to send command %02x: %d\n", : cmd[0], ret); Now that the command can contain more than one byte do you want to do something like this: if (ret) { printk(BIOS_WARNING, "SF: Failed to send command ret: %d, cmd:", ret); for (i = 0; i < cmd_len; i++) printk(BIOS_WARNING, "%02x ", cmd[i]); printk(BIOS_WARNING, "\n"); }
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/spi_flash.c@438 PS1, Line 438: ) || !region
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c:
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@199 PS1, Line 199: port part
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@204 PS1, Line 204: i size_t i
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@219 PS1, Line 219: 1 use a macro for this?
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@233 PS1, Line 233: port part
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@238 PS1, Line 238: block size_t?
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@266 PS1, Line 266: KiB Do you really need to divide by KiB?
block = reg.s.sec ? (4 * KiB) : max(64 * KiB, flash->size / 64);
https://review.coreboot.org/#/c/25082/1/src/drivers/spi/winbond.c@285 PS1, Line 285: 4 magic value. Do you want to add a macro for this?
https://review.coreboot.org/#/c/25082/1/src/include/spi_flash.h File src/include/spi_flash.h:
https://review.coreboot.org/#/c/25082/1/src/include/spi_flash.h@107 PS1, Line 107: 0 if the device doesn't support block protection : * 0 if the device doesn't enable block protection : * 0 Do you think it would help if we returned different return codes for these caes?
-
Furquan Shaikh (Code Review)