Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75646?usp=email )
Change subject: Replace all flashchips accesses with get functions ......................................................................
Patch Set 5:
(15 comments)
Patchset:
PS5:
I realise this patch was a intermediate step so the comments are just how it should probably look fo […]
Done
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/31223b9d_eaf0ba69 : PS5, Line 28: * Temporarily, this file is sorted alphabetically by vendor and name to : * assist with merging the Chromium fork of flashrom.
Can we remove this comment by now? (maybe in a separate patch). […]
Done
https://review.coreboot.org/c/flashrom/+/75646/comment/afb19e6f_1f31d463 : PS5, Line 34: internal_
I agree that "internal" almost always means internal programmer. It is flashromspeak :) […]
Done
https://review.coreboot.org/c/flashrom/+/75646/comment/79a33fb2_da183bd0 : PS5, Line 20844: get_flashchips(void)
Where would the caller get the pointer to flashchip db? and why the caller needs to have the pointer […]
Any code that needs to access the supported flashchips would use this function instead of accessing the array directly. This allows to later load the database of supported flashchips dynamically rather than hard coding it in the binary.
https://review.coreboot.org/c/flashrom/+/75646/comment/b58c418a_34e7a568 : PS5, Line 20849: unsigned int
maybe so. it was unsigned int before, so i didn't change it. […]
Done
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/217b37be_b2fa90d2 : PS5, Line 1099: return -1;
Maybe you can add `msg_gerr` before returning. […]
Done
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/cc185715_06991b98 : PS5, Line 1469: const struct flashchip *chip; : : struct flashchip *flashchips=get_flashchips(); : : if (!flashchips) : return NULL;
very clever. and doesn't check the return value. […]
Done
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/ad9a70ce_cf7e3c48 : PS5, Line 95: unsigned int
shouldn't then i downstairs also be size_t? not against this, but it seems that is a larger consiste […]
Done
File print.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/e4138b87_af71dddb : PS5, Line 56: const struct flashchip *chip;
delete
This won't work because we walk the whole array twice and chip is modified the first time.
https://review.coreboot.org/c/flashrom/+/75646/comment/1cf56450_5101611f : PS5, Line 62: flashchips
chip
ditto
https://review.coreboot.org/c/flashrom/+/75646/comment/9a2e313e_0260c79b : PS5, Line 65: chip = flashchips
delete
ditto.
File print_wiki.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/a2bdff40_14e6d3e5 : PS5, Line 294: flashchips
get_flashchips()
Given that get_flashchips() might do more in the future, it doesn't make sense to optimize two lines of code saved by making two function calls in my opinion.
https://review.coreboot.org/c/flashrom/+/75646/comment/c12e89f3_e7f98e63 : PS5, Line 310: flashchips
get_flashchips()
ditto
File tests/selfcheck.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/94c28057_65adc2ea : PS5, Line 80: /* unused */
But it is used two lines below?
Done
https://review.coreboot.org/c/flashrom/+/75646/comment/83be1367_b595402e : PS5, Line 83: flashchips = get_flashchips(); : flashchips_size = get_flashchips_size();
Can this be immediately initialised? (and the same above). This is what non-test code is doing.
Done