Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Edward O'Callaghan, 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 9:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/996b29b6_dbff3f30 PS8, Line 68: */
However, I think Documentation/ or the commit message is a better place.
Can this comment go into commit message as it is? Appending to existing 2 paragraphs, between "Document" and "BUG"? I know you mentioned potentially shrinking to 2-3 sentences, but maybe we can keep it long? It was useful for me to read long information, it would be harder to understand short version of 2-3 sentences.
Another thing you mentioned:
Also, first calculating 2.065s and then concluding 30s probably raises more questions than are answered.
I found where 30s comes from the last paragraph: it says "defines 5 master sections ... The worst case operation ... might take ~5 seconds" so that's 5*5=25 and taking a bit larger value 30 just in case. I hope my understanding is correct :)
Do you think it's fine to move this comment to commit message? And then, as you said, one line comment in the code?
Thanks all for your suggestion. I have moved the detailed description into the commit section and kept just one liner to define the 30sec timeout.
-
Subrata Banik (Code Review)