Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58483 )
Change subject: libflashrom,writeprotect: add flashrom_wp_{get,set}_mode() ......................................................................
Patch Set 25:
(7 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58483/comment/a1f22da1_22bcc989 PS25, Line 148: const In a forward declaration, `const` as a qualifier of a parameter (i.e. not a qualifier for something a parameter points to) is meaningless. Arguments are always passed by value and it doesn't matter to the caller if the callee will change its own copy.
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58483/comment/07996cb6_a828b109 PS25, Line 374: Spurious line break?
https://review.coreboot.org/c/flashrom/+/58483/comment/913a7999_7dbb0a33 PS25, Line 406: const This is correct usage of `const`. It tells the compiler and the reader that the intention is not to change the value.
https://review.coreboot.org/c/flashrom/+/58483/comment/e9c794e6_2c3ffd39 PS25, Line 410: return 1; Wouldn't it be a success if `mode == WP_MODE_DISABLE`?
https://review.coreboot.org/c/flashrom/+/58483/comment/5b83f455_3256f89f PS25, Line 412: f( Missing space. Could be a `switch/case` though.
https://review.coreboot.org/c/flashrom/+/58483/comment/f08b91a4_0beccf53 PS25, Line 420: f( Missing space.
https://review.coreboot.org/c/flashrom/+/58483/comment/7ae43c15_9a5c026a PS25, Line 437: } I wonder if it's a good idea to have SRP bits in an array. The intention is unclear. Is `srp_bit_count` only used to decide if SRP1 is exists? or does it prepare for more than 2 SRP bits? It seems the code would overall be more clear if we had separate bit definitions named by what we expect them to do. For instance, we expect SRP0 to always control the hardware write protection, so why not call it `hwwp` or something like that?