Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34234 )
Change subject: flashchips: Add Macronix MX25L51245G ......................................................................
Patch Set 2:
(3 comments)
Sorry for being late. There are some rather invisible issues, hard to get right for the first chip:
* We currently try to align the flashchips.c with the Chromium fork, so entries have to stay sorted by .vendor and .name. * Our 4BA support assumes that 4BA erase functions are listed explicitly and before each regular erase function of the same size.
https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c File flashchips.c:
https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7789 PS2, Line 7789: .tested = TEST_OK_PREW,
This tells me that probe, read, erase and write are working (or should be)
What was tested should be mentioned in the commit message.
https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7796 PS2, Line 7796: .block_erase = spi_block_erase_20, There should also be native 4-byte address (4BA) functions: 21/5c/dc. The 4BA should be mentioned first. See MX25L25635F, for instance.
https://review.coreboot.org/c/flashrom/+/34234/2/flashchips.c@7815 PS2, Line 7815: /* 2.35-3.6V for MX25L51245G */ The datasheet says 2.7V, and why is this a comment if the whole entry is for the MX25L51245G?