Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Patch set 16:Code-Review +1
12 comments:
Patchset:
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:
Patch Set #16, 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.
Patch Set #16, Line 167: return (status_reg == STATUS1 ? SPI_SR_WEL | SPI_SR_WIP : 0);
Nit, no parentheses needed.
This seems very chip specific.
Patch Set #16, 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.
Patch Set #16, 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.
Patch Set #16, Line 339: data->emu_status[0] &= ro_bits;
This also changes behavior, I guess WIP is not accidentally cleared anymore?.
Patch Set #16, Line 340: data->emu_status[0] |= (writearr[1] & ~ro_bits);
Nit, no parentheses needed.
Patch Set #16, Line 347: other chips might differ
Isn't this always the case?
No dangling asterisk, please.
Patch Set #16, Line 364: data->emu_status[1] |= (writearr[1] & ~ro_bits);
Should we add a debug message like above?
Why 32? generally, it seems odd to give an exact number when no exact
number is needed, i.e. `unsigned int` would also do.
To view, visit change 59072. To unsubscribe, or for help writing mail filters, visit settings.