Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33997 )
Change subject: flashchips.h: merge definitions from Chromium fork ......................................................................
Patch Set 6: Code-Review+1
(7 comments)
Looks good, save for a few nits
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@40 PS6, Line 40: #define VARIABLE_SIZE_DEVICE_ID 0x10af
What are these for?
If unused, please drop.
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@278 PS6, Line 278: #define EON_EN25D16 0x3015
Spurious double entry?
Definitely. I'd keep the bottom one, and optionally rename it to Q16 for consistency (doesn't need to be in this patch)
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@504 PS6, Line 504: #define MACRONIX_MX25L12805 0x2018 /* MX25L12805 */
Does it really exist?
Looks like it's the same as the line below, but following the style of the lines above, so it seems to be duplicated.
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@962 PS6, Line 962: #define WINBOND_NEX_W25Q128_W 0x6018 /* Same as W25Q128FV (QPI mode), W25R128FV */
Spurious, double entry?
Looks like it, please keep the "W25Q128_W" entry
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@949 PS5, Line 949: W25Q128
I'm unfamiliar with these parts. […]
I don't think this comment needs to be updated (see the other comment thread), but I wanted to note that this is not specific to the W25Q128.
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@951 PS5, Line 951: */
I would prefer to just drop the comment. It's merely a random detail […]
Agreed.
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c File flashchips.c:
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.c@5892 PS5, Line 5892: CD
I think it's common to use the name of the chip that introduced […]
Agreed. I wouldn't change the model_id define, but rather the string name (on a separate patch).