Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Angel Pons, Anastasia Klimchuk. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 )
Change subject: ichspi: Unify timeouts across all SPI operations to 30s ......................................................................
Patch Set 8:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/0c2222e6_96ca7802 PS8, Line 68: */
This comment is much too long, starts by describing the past state of the code and references a specific flash chip which it shouldn't. Also, first calculating 2.065s and then concluding 30s probably raises more questions than are answered.
It should be possible to boil it down to 2 or 3 sentences, preferably in the commit message and not as a code comment. If Intel would update the public datasheets, we could also reference those. If not, we could also add it to Documentation/.
Intel has updated the ADL SPI programming guide which I believe is not in public domain, so we need to have the explanation behind the increased timeout logic.
I agreed with you that, this is too verbose and can be optimised.
Are you recommending to remove the comments from code block and only keep in the commit section alone ?
-
Subrata Banik (Code Review)