Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 )
Change subject: [RFC] writeprotect: add functions to read and write wp_chip_state ......................................................................
Patch Set 6:
(4 comments)
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58479/comment/f55f298e_5cd01830 PS6, Line 26: print_wp_chip_state You can move print_wp_chip_state here on the top, and avoid forward declaration.
https://review.coreboot.org/c/flashrom/+/58479/comment/46fa91f2_1764679d PS6, Line 28: static int read_reg_bit( : const struct flashctx *flash, : const struct reg_bit_info bit, : uint8_t *value, : bool *present) Just to check: does this not fit into 112 chars? It really helps to grep when all parameters are on one line. 112 chars is a lot, most of the code does not need that much. But for the list of function parameters, it makes grep more informative, and that's very useful. You are declaring lots of new functions in this patch series, the comment applies to all of them.
https://review.coreboot.org/c/flashrom/+/58479/comment/359208e4_436b8f9a PS6, Line 73: void *suppress_unused_warning_for_read_wp_chip_state = read_wp_chip_state; Maybe I am missing something, but I don't see this is being used in the patch? If you use it later in the chain, better to introduce it when it's used. And anyway I will ask you: does it have to be a global state?
https://review.coreboot.org/c/flashrom/+/58479/comment/2cd04490_927c1b0c PS6, Line 141: void *suppress_unused_warning_for_write_wp_chip_state = write_wp_chip_state; Same comment as previous