Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two ......................................................................
Patch Set 3: Code-Review+1
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/58735/comment/714ccfec_6abd0111 PS3, Line 20: TEST=1) probe-read-verify-erase section-write-reboot : on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E : 2) probe and read on Panther Point (7 series PCH)
Personally, I don't really like these "tags" (here TEST or in other commits sometimes TESTED), becau […]
If you still need a `TEST=` tag for some reason, you could do something like this:
TEST=Check that the following scenarios still behave properly: 1) Probe-read-verify-erase section-write-reboot on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E 2) Probe and read on Panther Point (7 series PCH)
Note that the text following `TEST=` provides information about how the test results were interpreted. Without this part, it might be that both tests failed spectacularly, but the commit message wouldn't be lying: the change *was* tested, but without success! 😄
And yes, manually adding Tested-by tags is perfectly fine. I only use them when someone else has tested my changes. I feel a Tested-by tag is redundant when I've tested my own changes.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/10efe72b_6a8e8ae0 PS2, Line 1822: arg = extract_programmer_param("ich_spi_mode");
Wow! this is great idea, I might use it in future, thank you!! I would leave this commit as is, hope it's fine.
Sure, no problem!
https://review.coreboot.org/c/flashrom/+/58735/comment/f33a90f0_5a50853e PS2, Line 1769:
Yes this was accidental, I removed it.
Thanks!
-
Angel Pons (Code Review)