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: flash: add structure to represent chip wp configuration ......................................................................
Patch Set 22:
(3 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58478/comment/fd05d9a1_d789bbbb PS22, Line 201: Complete `Complete` seems a bit exaggerated. Given the many different implementations, I'm not sure if we can ever have that. Actually, I'm missing one thing already, `WPS`.
https://review.coreboot.org/c/flashrom/+/58478/comment/69db359b_595f3746 PS22, Line 203: srp Please add comments what all these acronyms mean.
File flash.h:
https://review.coreboot.org/c/flashrom/+/58478/comment/b3f30cd6_74048bf8 PS21, Line 201: /* Complete description of a chip's write protection configuration */ : struct flashrom_wp_chip_config { : size_t srp_bit_count; : uint8_t srp[MAX_SRP_BITS]; : : bool cmp_bit_present; : uint8_t cmp; : : bool sec_bit_present; : uint8_t sec; : : bool tb_bit_present; : uint8_t tb; : : size_t bp_bit_count; : uint8_t bp[MAX_BP_BITS]; : };
This is specific to SPI flash chips.
Well, not by nature, probably by coincidence. I don't see why one couldn't make a flash chip with such registers but without SPI interface.
However, I also see that it doesn't seem to belong here. As the commit message suggests, this is the representation used by the writeprotect code (not the API?), it should be private to chip drivers and `writeprotect*.c` I guess.
-
Nico Huber (Code Review)