Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34496 )
Change subject: Add support for M95M02-A125 ......................................................................
Patch Set 2:
(8 comments)
Thanks for the update. It seems nearly ready.
I've looked closer at this "family code" and am sure that it is usually handled as part of what flashrom calls the model id.
https://review.coreboot.org/c/flashrom/+/34496/2/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/34496/2/flashchips.h@858 PS2, Line 858: #define ST_SPI_FAMILY_CODE 0x00 : #define ST_M95M02 0x0012 /* ST M95XXX 2Mbit (256KiB) */ Please use tabs for alignment.
https://review.coreboot.org/c/flashrom/+/34496/2/flashchips.h@859 PS2, Line 859: 00 The high byte 00 here is actually the family code. I don't think we need a separate definition for that. The "model id" for ST_M95M02 should simply include it.
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.c File flashchips.c:
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.c@14121 PS1, Line 14121: .eraseblocks = { { 256 * 1024, 1 } },
Done
I guess, I really meant `{ 256, 1024 }`. The `.count` is handled in walk_eraseblocks(). I don't care much, as running flashrom with -E for an EEPROM is rather theoretical anyway.
https://review.coreboot.org/c/flashrom/+/34496/2/spi25.c File spi25.c:
https://review.coreboot.org/c/flashrom/+/34496/2/spi25.c@443 PS2, Line 443: static int spi_nbyte_program(struct flashctx *flash, unsigned int addr, const uint8_t *bytes, unsigned int len) Moving this function doesn't seem needed anymore?
https://review.coreboot.org/c/flashrom/+/34496/2/spi95.c File spi95.c:
https://review.coreboot.org/c/flashrom/+/34496/2/spi95.c@8 PS2, Line 8: * the Free Software Foundation; version 2 of the License. I would prefer new code to be added under
"either version 2 of the License, or (at your option) any later version."
It's your call. I just want to make sure, you are aware that not all of flashrom agrees to the v2-only choice.
https://review.coreboot.org/c/flashrom/+/34496/2/spi95.c@34 PS2, Line 34: uint32_t id1, id2, id3; As mentioned in `flashchips.h`, the model id (id2) should include the family code. So this should be:
uint8_t id1; uint16_t id2;
https://review.coreboot.org/c/flashrom/+/34496/2/spi95.c@43 PS2, Line 43: id2 = readarr[2]; // model id : id3 = readarr[1]; // SPI Family code, must be 0x00 : Please join these into a single `id2` that is compared against `chip->model_id`.
https://review.coreboot.org/c/flashrom/+/34496/2/spi95.c@61 PS2, Line 61: (uint8_t*) malloc Please place space as follows:
(uint8_t *)malloc