Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 )
Change subject: libflashrom,writeprotect: add support for reading/writing WP configs ......................................................................
Patch Set 10:
(6 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58479/comment/48957021_d01c6a76 PS9, Line 131: int flashrom_wp_read_chip_config(const struct flashrom_flashctx *flash, struct flashrom_wp_chip_config *cfg); : int flashrom_wp_write_chip_config(struct flashrom_flashctx *flash, const struct flashrom_wp_chip_config *cfg); : int flashrom_wp_compare_chip_configs(const struct flashrom_wp_chip_config *cfga, const struct flashrom_wp_chip_config *cfgb);
You can omit parameter names if they are self-explanatory from parameter type, which all of these se […]
Done
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58479/comment/a519d6bf_e0eb314f PS9, Line 27: #define wp_chip_config flashrom_wp_chip_config : #define wp_range flashrom_wp_range : #define wp_mode flashrom_wp_mode
Oh, so why this prefix was added from the very beginning? ;)
Yep, I'm not sure how much I like it, but it does save a lot of unnecessary in the code.
https://review.coreboot.org/c/flashrom/+/58479/comment/e3f83446_1f7619cc PS9, Line 39: if (spi_read_register(flash, bit.reg, value)) : return 1;
Let's propagate return value, […]
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/7756b38b_e700a1e6 PS9, Line 128: return ret;
This should be on its own line (and same below).
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/4fe1823d_abfffc67 PS9, Line 158: if (ord == 0) ord = cfga->srp[i] - cfgb->srp[i];
How does it work? If ord becomes non-0 in the first loop cycle, then the rest of (ord == 0) are skip […]
If you unroll the loop it looks like this:
if (ord == 0) ord = cfga->srp[1] - cfgb->srp[1]; if (ord == 0) ord = cfga->srp[0] - cfgb->srp[0];
The earlier comparisons are more important so we should only change the value of ord if all previous comparisons gave 0.
https://review.coreboot.org/c/flashrom/+/58479/comment/2e198c07_28f0121b PS9, Line 161: ord = cfga->cmp - cfgb->cmp;
Please make it on its own line, and the same for other conditionals below (including ones inside for […]
Done