Nico Huber 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:
(6 comments)
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?
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@278 PS6, Line 278: #define EON_EN25D16 0x3015 Spurious double entry?
https://review.coreboot.org/c/flashrom/+/33997/6/flashchips.h@504 PS6, Line 504: #define MACRONIX_MX25L12805 0x2018 /* MX25L12805 */ Does it really exist?
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?
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/33997/5/flashchips.h@951 PS5, Line 951: */
Do you have a suggestion for alternate wording? […]
I would prefer to just drop the comment. It's merely a random detail that is already better documented in `flashchips.c`.
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
The downstream names were more specific and therefore seemed more helpful. […]
I think it's common to use the name of the chip that introduced the id. So that would be `GD25LQ128C`? It doesn't matter much, IMHO.
What we do care about, though, is the `.name` presented to the ui. It should mention all supported chips e.g. "GD25LQ128C/D" or "GD25LQ128C/GD25LQ128D". But that's for a future patch.