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 12:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/8c56ca75_5622db86 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.
marking resolved, please feel free to open if you think that the comment is not addressed.
-
Subrata Banik (Code Review)