Attention is currently required from: Nikolai Artemiev, Peter Marheine, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email )
Change subject: tests/erase: record the opcode for each erase ......................................................................
Patch Set 5:
(4 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/82251/comment/700c6b67_72602024 : PS5, Line 407: 5 Can this be a macro in flash.h ? (I mean number 5)
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/82251/comment/352cb842_16fc3990 : PS5, Line 325: TEST_ERASE_INJECTOR Maybe remove this value? I know some existing tests are using it, but they can be adjusted. Right now it looks confusing, especially for new people who might try write a unit test: which value to use? TEST_ERASE_INJECTOR or TEST_ERASE_INJECTOR_1?
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82251/comment/e49ab416_0f1f23df : PS5, Line 198: block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip What if you create 5 functions, block_erase_chip_1, ... , block_erase_chip_5 each of them will call `block_erase_chip` with one extra argument: a value from test injector enum on of TEST_ERASE_INJECTOR_X ? and then you can populate `struct erase_invoke` in full, currently you only fill in 2 members out of 3
Maybe it can even be written in a macro-loop, but I am equally fine with 5 functions written in code.
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/82251/comment/d7978384_2dffa15b : PS5, Line 127: '-DFLASHROM_TEST', Just to test , have you built with this option undefined?
Also I am thinking, maybe live without it? :) There are a bunch of #ifs already in the code
-
Anastasia Klimchuk (Code Review)