Attention is currently required from: Simon Buhrow, Thomas Heijligen, Aarya.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67354 )
Change subject: spi.c: Add erasefn and opcodes for AT45 and S25F ......................................................................
Patch Set 2:
(1 comment)
File spi.c:
https://review.coreboot.org/c/flashrom/+/67354/comment/2ae48d33_e70d875b PS2, Line 50: &spi_erase_at45cs_sector, 0x50},
Maybe we can make a separate probe_opcode function for this chip?
That could work too. Given that the chip has only a single erasefunc, that function would be simpler. But I'm not sure if it wouldn't be more code overall and we'd also have yet another pointer in struct flashchip.
What I would do: Keep spi_get_erasefn_from_opcode() in `spi25.c`. After all that is the scope it's supposed to support. Implement spi_get_opcode_from_erasefn() here, let it return a list of codes `const uint8_t *`. Its code doesn't even have to change (beside returning NULL instead of 0x00). The struct for function_opcode_list[] would get an `uint8_t opcodes[3]`. Fields omitted are 0, so we'd only have to add some more braces and get a 0-termination for free. The caller would get a simple loop that stops when it encounters a 0.
That would leave us with two very similar lists for both functions. But I don't think it's too bad, after all they serve different purposes.