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/+/58480 )
Change subject: flashchips,writeprotect_ranges: add range decoding function ......................................................................
Patch Set 23:
(14 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58480/comment/1bea2530_6e62729d PS23, Line 203: range Says less than the type, can be dropped.
https://review.coreboot.org/c/flashrom/+/58480/comment/b92e456e_f036e317 PS23, Line 203: const struct flashrom_wp_chip_config *cfg, uint32_t chip_len It seems this is exactly the input we need for the `w25` implementation. But will it suit all future implementations? Should we pass the full flash definition or even context instead?
https://review.coreboot.org/c/flashrom/+/58480/comment/8ebd4319_a8d1f009 PS23, Line 203: typedef Why have a typedef for it? All the other function pointers in `struct flashchip` don't have/need one?
https://review.coreboot.org/c/flashrom/+/58480/comment/959f09d8_7739a02e PS23, Line 203: uint32_t Please don't hard-code fixed-width types. `unsigned int` should work as well, `size_t` too, and for most flexibility there is `chipsize_t` in `layout.h`. I don't promote the latter much because I'm not sure if it's really necessary. Maybe it would become handy if we'd ever encounter
4GiB chips (doesn't seem that far away anymore ^^).
https://review.coreboot.org/c/flashrom/+/58480/comment/39774cfd_1376078a PS23, Line 203: cfg Says less than the type already does, can be dropped.
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58480/comment/2962edc7_3661c590 PS23, Line 134: }; flashrom_flash_getsize() uses `size_t`? Should this follow?
File writeprotect_ranges.c:
https://review.coreboot.org/c/flashrom/+/58480/comment/0d66a75e_b3d9880c PS23, Line 19: Line break, please.
https://review.coreboot.org/c/flashrom/+/58480/comment/3a440c7f_d7844f7e PS23, Line 21: TODO Not sure if it's worth a TODO. FWIW, this kind of TODO will just stick.
https://review.coreboot.org/c/flashrom/+/58480/comment/9d461a5a_95010486 PS23, Line 27: 32 Why 32?
https://review.coreboot.org/c/flashrom/+/58480/comment/c663f78d_4834b28b PS23, Line 29: r( Missing space.
https://review.coreboot.org/c/flashrom/+/58480/comment/26fd2df0_486e8c71 PS23, Line 34: cfg->bp Isn't this an implicit pointer? Looks like GCC doesn't warn if compared to 0. Why does it work? I guess `coeff` overflows and is 0 ;)
https://review.coreboot.org/c/flashrom/+/58480/comment/a5d85173_90597189 PS23, Line 50: 4 * 1024 4*KiB
https://review.coreboot.org/c/flashrom/+/58480/comment/b28bd977_107e19e3 PS23, Line 51: 64 * 1024 64*KiB
https://review.coreboot.org/c/flashrom/+/58480/comment/ff061593_9ade5f1f PS23, Line 85: && range->len > 0 Does it matter? i.e. where's the start of an empty range? ^^