Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59073 )
Change subject: [RFC][WPTST] dummyflasher: SR2 for W25Q128FV ......................................................................
Patch Set 10:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59073/comment/4a6f2b8a_d2d91567 PS10, Line 8: : By the way, W25Q128FV has three status registers, but no plans to use : the third one yet. If you remove "by the way", it is a very good second paragraph for this commit message!
The first paragraph needs to describe more precisely what this commit is doing. A wider question is about this and previous patch, what exactly each of them is doing. Maybe the work between them can be re-distributed (for example, previous patch has branch with emu_status_len == 2 , but the value set to 2 only here). I am saying "maybe" because it is a question, let's look closely.
After the work is distributed between two patches, then commit description and commit message for each of them need to reflect that. At the moment commit descriptions are: "support emulation of SR2" "SR2 for W25Q128FV"
Imagine looking at commit history a year after. It would not be easy to guess what each patch is doing.
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59073/comment/70006fff_8952b246 PS10, Line 922: data->emu_status_len = 2; I added a comment to previous patch about emu_status_len always 1, I see that it becomes 2 in this patch. However, in the previous patch it is always 1. Maybe some of the parts of previous patch should be here?
-
Anastasia Klimchuk (Code Review)