Attention is currently required from: Nikolai Artemiev, Peter Marheine, Thomas Heijligen.
7 comments:
Patchset:
Sorry for delay in review! This looks great, I only have few small comments. This is very useful addition to unit tests!
File include/flash.h:
Patch Set #6, Line 334: /* special cases must come last. */
I think this comment better go under FLASHROM_TEST, otherwise it is orphaned in the non-test code. Don't know if anyone ever looks into generated code after pre-processor stage (I did few times only), but still.
#ifndef FLASHROM_TEST
};
#else
TEST_ERASE_INJECTOR_1,
TEST_ERASE_INJECTOR_2,
TEST_ERASE_INJECTOR_3,
TEST_ERASE_INJECTOR_4,
TEST_ERASE_INJECTOR_5,
};
#define NUM_TEST_ERASE_INJECTORS 5
extern erasefunc_t *g_test_erase_injector[NUM_TEST_ERASE_INJECTORS];
#endif
This style is different from the others (read and write), ifndef vs ifdef and I know it saves few lines, but I think maybe in the case consistency would be good?
File tests/erase_func_algo.c:
Patch Set #5, Line 198: block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip
I had avoided doing that just because it requires updating all of the existing test cases, but done […]
Thank you for doing this. I think it's valuable to test, if we have test running we prevent it from being broken in future!
File tests/erase_func_algo.c:
block_erase_chip_1,
block_erase_chip_2,
block_erase_chip_3,
block_erase_chip_4,
block_erase_chip_5,
I think it can help readers, to add per-line comment, since the suffixes (1, 2, 3, 4, 5) do not correspond to block-length/opcodes (1b, 2b, 4b, 8b, 16b).
I know this can be deducted by going through all the code, but a comment here can be a good time-saver.
Either, each line N comment `TEST_ERASE_INJECTOR_N`
or, `erasefn for opcode with corresponds to block size N`
or maybe you prefer some other comment
{0xf, 0x1, TEST_ERASE_INJECTOR_1},
{0x8, 0x1, TEST_ERASE_INJECTOR_1},
{0x9, 0x1, TEST_ERASE_INJECTOR_1},
{0xa, 0x1, TEST_ERASE_INJECTOR_1},
{0xb, 0x1, TEST_ERASE_INJECTOR_1},
{0xc, 0x1, TEST_ERASE_INJECTOR_1},
{0xd, 0x1, TEST_ERASE_INJECTOR_1},
{0xe, 0x1, TEST_ERASE_INJECTOR_1},
{0x3, 0x1, TEST_ERASE_INJECTOR_1},
{0x4, 0x1, TEST_ERASE_INJECTOR_1},
{0x5, 0x1, TEST_ERASE_INJECTOR_1},
{0x6, 0x1, TEST_ERASE_INJECTOR_1},
{0x7, 0x1, TEST_ERASE_INJECTOR_1},
{0x0, 0x1, TEST_ERASE_INJECTOR_1},
{0x1, 0x1, TEST_ERASE_INJECTOR_1},
These lines have trailing whitespace (shows as bright pink in gerrit :))
File tests/meson.build:
Patch Set #5, Line 127: '-DFLASHROM_TEST',
Yes, building the regular flashrom binary still works. […]
After thinking more, I agree with you.
To view, visit change 82251. To unsubscribe, or for help writing mail filters, visit settings.