Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40325 )
Change subject: Add writeprotect support ......................................................................
Patch Set 7: Code-Review+1
(5 comments)
I'm not sure if this could be split up a bit.
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.h File writeprotect.h:
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.h@44 PS7, Line 44: wp_wpce775x Do we have this programmer upstream?
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@873 PS7, Line 873: uint8_t mx25l_read_config_register(const struct flashctx *flash);//XXX Hmm?
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@1132 PS7, Line 1132: //case ATMEL_AT25SF128A: Hm?
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@1473 PS7, Line 1473: /* FIXME: this is NOT endian-free copy. */ How about providing a union instead of a struct? Here's an example: https://github.com/coreboot/coreboot/blob/dc0c08100124278efc9ed91952378b0190...
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@2236 PS7, Line 2236: //case SPANSION_ID: : // switch (flash->chip->model_id) { : // case SPANSION_S25FS128S_L: : // case SPANSION_S25FS128S_S: { : // *wp = &s25fs128s_wp; : // (*wp)->descrs = s25fs128s_ranges; : // *num_entries = ARRAY_SIZE(s25fs128s_ranges); : // break; : // } : // case SPANSION_S25FL256S_UL: : // case SPANSION_S25FL256S_US: { : // *wp = &s25fl256s_wp; : // (*wp)->descrs = s25fl256s_ranges; : // *num_entries = ARRAY_SIZE(s25fl256s_ranges); : // break; : // } : // default: : // msg_cerr("%s():%d Spansion flash chip mismatch (0x%04x)" : // ", aborting\n", __func__, __LINE__, : // flash->chip->model_id); : // return -1; : // } : // break; Um?