Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58478 )
Change subject: writeprotect.h: add structure to represent chip wp configuration bits ......................................................................
Patch Set 26: Code-Review+1
(2 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58478/comment/73759344_e228f8dc PS22, Line 201: Complete
The intention is that it will be kept complete, i.e. […]
Your intentions don't change that the comment is misleading. If you need to explain a comment what's that? a meta comment? And even according to what you describe ("as necessary to support new chips") implies that we'd support all WP features of a chip, which we don't even for the first added chips AFAICS. You could say it's complete wrt. the block protection configuration and then hope the comment stays true for added chips.
Generally, code comments are dangerous. People often trust comments instead of reading the code. If a comment is wrong, misleading or even just outdated, that's very bad for maintenance. What makes things worse is that developers and reviewers often ignore existing comments when the code changes. Especially when comments state something that was obvious to them in the first place. Better not write comments that are prone to misinterpretation or to get outdated.
Actually, I think you already found better words yourself :)
It allows most WP code to store and manipulate a chip's configuration without knowing the exact layout of bits in the chip's status registers.
That's something that explain the idea of the struct, I'd simply leave out the completeness claim and add the above:
/* * Description of a chip's write protection configuration * * It allows most WP code to store and manipulate a chip's configuration * without knowing the exact layout of bits in the chip's status registers. */
(NB. Please don't mark threads as resolved unless you are 100% sure a consent has been found.)
https://review.coreboot.org/c/flashrom/+/58478/comment/d47bba1b_d4eb302d PS22, Line 202: chip
I've changed it to `wp_bits`. […]
Thanks!