Attention is currently required from: Subrata Banik, Paul Menzel, Angel Pons, Anastasia Klimchuk. Nico Huber 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/4b0b24bf_37f109a0 PS8, Line 68: */
This comment is much too long, starts by describing the past state of the […]
I would prefer at most one line as a code comment. We often have timeout constants, they all have their reasons, and we shouldn't write one page of text per constant. Where to place it depends on how much you want to say. If you keep it to the point (we may have to wait for other masters, that can take up to (n - 1) * max), that's something for the commit message. If you want to say more, please put it in Documentation/.
Also, I noticed now that the constant is only used in a single function, it would be better to move it there into the C code, e.g.
static int ich_hwseq_wait_for_cycle_complete(unsigned int len, enum ich_chipset ich_gen) { /* The bus may be busy due to other masters, hence the long timeout of 30s. */ const unsigned int timeout_us = 30*1000*1000;