Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59529 )
Change subject: spi25_statusreg: delete read_status_register() ......................................................................
Patch Set 63: Code-Review+1
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59529/comment/ffc37119_7c178cd2 PS63, Line 7: spi25_statusreg: delete read_status_register() Apart from deleting a function, this patch does one more good thing: it propagates error codes (which previously were ignored). This is very worth mentioning in commit message. Something like "As a side effect of using spi_read_register() instead of old function, error codes are propagated and not ignored". And this probably goes after the comment below.
https://review.coreboot.org/c/flashrom/+/59529/comment/973b04dc_6c65082e PS63, Line 7: read_status_register Looking at the code, it is `spi_read_status_register()` function which is deleted :)
https://review.coreboot.org/c/flashrom/+/59529/comment/1f979a12_b2120d89 PS63, Line 8: Also, it is worth adding one sentence explaining why the function is deleted, something like "The function is no longer needed, since it can be fully replaced by generic spi_read_register()".
https://review.coreboot.org/c/flashrom/+/59529/comment/8497c1d7_b743fbe3 PS63, Line 11: flashrom -{r,w,E} Same as for previous patch: maybe a bit more info about chips / or environment where this was tested?
File spi25.c:
https://review.coreboot.org/c/flashrom/+/59529/comment/4c326302_73c967e1 PS63, Line 307: 500 Maybe I forgot and it was discussed before, but where 500 comes from ? Don't know, but maybe it is worth a comment?
-
Anastasia Klimchuk (Code Review)