Nico Huber has posted comments on this change. ( https://review.coreboot.org/26949 )
Change subject: flashchips: Add Macronix MX25U51245G ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/26949/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/26949/2//COMMIT_MSG@14 PS2, Line 14: the later means I : have tested 4-byte addressing
Agree, and its why I was fairly fastidious in documenting exactly what I did test. […]
As I said, we don't have clear rules. You documented what you did so the current commit message should be ok. And OK flags are fine too. (In the long run, we can't say that all OK flags are really true anyway, because don't test every chip after every change, don't know what chip works with what programmer etc...)
https://review.coreboot.org/#/c/26949/2/flashchips.c File flashchips.c:
https://review.coreboot.org/#/c/26949/2/flashchips.c@8572 PS2, Line 8572: 512B
8kbit isn't it? Or are bit units implicit? […]
b is for bits, B for bytes. But writing "bit" is probably less ambiguous. Yes, new text looks good. (Actually there is so much information in the datasheets, I don't know why people picked this to comment everywhere. Would be much better to put everything in code and don't comment, IMHO.)
-
Nico Huber (Code Review)