Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 )
Change subject: dummyflasher: add SR2 emulation harness ......................................................................
Patch Set 16: Code-Review+1
(12 comments)
Patchset:
PS16: Many thanks for extending the dummy programmer!
There is one thing that your patch actually seems to fix. Please always push such fixes in separate commits (ahead or even in a separate queue so they can be merged easily). Looks quite good otherwise.
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59072/comment/a42040d7_56bdc91b PS16, Line 42: int emu_features; /* combination of FEATURE_* constants */ It seems a little odd to have this while most of the code doesn't make use of it. If you are going to check only one (or a few) bits, why not just add individual `bool` flags? That would even be more readable, I guess.
https://review.coreboot.org/c/flashrom/+/59072/comment/487baa09_78633c0e PS16, Line 167: return (status_reg == STATUS1 ? SPI_SR_WEL | SPI_SR_WIP : 0); Nit, no parentheses needed.
https://review.coreboot.org/c/flashrom/+/59072/comment/05ba2f37_4b3f2d50 PS16, Line 168: } This seems very chip specific.
https://review.coreboot.org/c/flashrom/+/59072/comment/5ee23b72_8eeae367 PS16, Line 335: wrsr_ext = (writecnt == 3 && (data->emu_features & FEATURE_WRSR_EXT)); In this case, I think the additional parentheses make it actually harder to read the line. For every open parenthesis, one has to keep track where it's closed again.
https://review.coreboot.org/c/flashrom/+/59072/comment/dccc2f50_a1ed8d1e PS16, Line 338: ro_bits = get_status_ro_bit_mask(STATUS1); Looks like this changes behavior, i.e. fixes that the WEL could be written before? Please put such changes into separate commits.
https://review.coreboot.org/c/flashrom/+/59072/comment/a1022284_86574c25 PS16, Line 339: data->emu_status[0] &= ro_bits; This also changes behavior, I guess WIP is not accidentally cleared anymore?.
https://review.coreboot.org/c/flashrom/+/59072/comment/56cb96c8_62856af0 PS16, Line 340: data->emu_status[0] |= (writearr[1] & ~ro_bits); Nit, no parentheses needed.
https://review.coreboot.org/c/flashrom/+/59072/comment/2f689e6a_370a7491 PS16, Line 347: other chips might differ Isn't this always the case?
https://review.coreboot.org/c/flashrom/+/59072/comment/a9f606e8_9727776c PS16, Line 347: * No dangling asterisk, please.
https://review.coreboot.org/c/flashrom/+/59072/comment/9337ac17_a245db49 PS16, Line 364: data->emu_status[1] |= (writearr[1] & ~ro_bits); Should we add a debug message like above?
https://review.coreboot.org/c/flashrom/+/59072/comment/a2e58c01_78041f10 PS16, Line 970: 32 Why 32? generally, it seems odd to give an exact number when no exact number is needed, i.e. `unsigned int` would also do.