Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34496 )
Change subject: Add support for M95M02-A125 ......................................................................
Patch Set 1:
(8 comments)
Nice work, I didn't think it would be that simple :)
Beside coding style, there are some thinks that would probably much easier be done now:
* Probing can be generic (i.e. decide the command bytes based on EEPROM size). * _erase_emulation() can be much simpler if restricted to the page size. * As we don't have to erase before writing FEATURE_NO_ERASE should work (erase emulation is still useful for -E).
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.h@856 PS1, Line 856: 0x00 If we know it's supposed to be 0x00, why not check it?
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.c File flashchips.c:
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.c@14114 PS1, Line 14114: .feature_bits = FEATURE_WRSR_WREN, FEATURE_NO_ERASE and FEATURE_ERASED_ZERO?
https://review.coreboot.org/c/flashrom/+/34496/1/flashchips.c@14121 PS1, Line 14121: .eraseblocks = { { 256 * 1024, 1 } }, Alternatively to handle pages in spi_block_erase_emulation(), we could declare this as { 256, 1 }. Actually, I think that would be much cleaner (you can also add a check there that `blocklen` matches the page size). So you can ignore my comment in `spi25.c`.
https://review.coreboot.org/c/flashrom/+/34496/1/spi25.c File spi25.c:
PS1: (spi)25 != 95, please don't hesitate to start a new file. Or maybe we should rename this one to `spi_jedec.c`?
General coding style is that of the Linux kernel with the exception that we allow up to 112 chars per line (if it helps readability). Use tabs for indentation (they count as 8 chars w.r.t. the line length).
https://review.coreboot.org/c/flashrom/+/34496/1/spi25.c@269 PS1, Line 269: /* ST_M95_RDID_OUTSIZE depends on size of the flash and Please start multi-line comments with /* on a separate line. However this should fit on one line (112 char limit).
Also, why not turn it into code? you can read the size of the EEPROM entry (flash->chip->total_size?), decide the number of bytes to send based on that?
https://review.coreboot.org/c/flashrom/+/34496/1/spi25.c@272 PS1, Line 272: static const unsigned char cmd[ST_M95_RDID_OUTSIZE] = {ST_M95_RDID}; spaces around ST_M95_RDID, please
https://review.coreboot.org/c/flashrom/+/34496/1/spi25.c@282 PS1, Line 282: "L 0x%02x, M 0x%02x, H 0x%02x]\n", Please align output with the existing probing functions.
`id1` and `id2` usually refer to the data read from the chip, afaik.
https://review.coreboot.org/c/flashrom/+/34496/1/spi25.c@635 PS1, Line 635: } Alternatively, allocate `blocklen` bytes and let spi_write_chunked() take care of the pages.