Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46140 )
Change subject: WIP: s25f.c: import file from cros flashrom ......................................................................
Patch Set 2:
(10 comments)
https://review.coreboot.org/c/flashrom/+/46140/2/chipdrivers.h File chipdrivers.h:
https://review.coreboot.org/c/flashrom/+/46140/2/chipdrivers.h@24 PS2, Line 24: #include "writeprotect.h" /* for modifier_bits */ I don't think this is needed in this commit for adding the chip initial support.
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c File s25f.c:
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@34 PS2, Line 34: * s25f.c - Helper functions for Spansion S25FL and S25FS SPI flash chips. : * Uses 24 bit addressing for the FS chips and 32 bit addressing for the FL : * chips (which is required by the overlayed sector size devices). : * TODO: Implement fancy hybrid sector architecture helpers. separate out comment from license header.
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@43 PS2, Line 43: #include "hwaccess.h" is this needed?
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@45 PS2, Line 45: #include "writeprotect.h" ditto.
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@192 PS2, Line 192: unsigned char read_cr_cmd[] = { : CMD_RDAR, : (addr >> 16) & 0xff, : (addr >> 8) & 0xff, : (addr & 0xff), : 0x00, 0x00, 0x00, 0x00, : 0x00, 0x00, 0x00, 0x00, : }; looks like these various cmd blocks though the file are badly formatted. Please make it consistent with the rest of flashrom such as in spi25.c as an example I guess.
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@489 PS2, Line 489: et; = 0;
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@494 PS2, Line 494: if (!ret) { invert the ret check logic to avoid such a big block under a if branch.
``` if (ret) return ret; ```
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@531 PS2, Line 531: return ret =
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@531 PS2, Line 531: return ret =
https://review.coreboot.org/c/flashrom/+/46140/2/s25f.c@534 PS2, Line 534: 0; ret