Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk 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 7:
(6 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/75646/comment/f0206653_74431d8c : PS7, Line 7: Replace all flashchips accesses with get functions If you are in a good mood, you can add `tree: ` prefix to commit title, which we usually do, so that prefix gives an idea what is changing in commit. This is a big change across the tree.
Patchset:
PS7: Great, thank you! The build error seems legit, and when it's fixed, unit tests will also run. Which is what we want, especially the selfcheck unit test is very relevant. Did you run tests locally?
Also I am thinking whether unit tests have coverage for probing, I need to check. If not, I will try to add a unit test which does probing (with dummy). This would be useful, in addition to selfcheck. But that would be separate patch.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/9f6c3fd5_d8fbead3 : PS7, Line 1936: * They are all defined as externs in this compilation unit so we don't know their sizes which vary : * depending on compiler flags, e.g. the target architecture, and can sometimes be 0. : * For 'flashchips' we export the size explicitly to work around this and to be able to implement the : * checks below. I think this comment needs to be rewritten. `They are all defined as externs in this compilation unit` does not fully apply, and `'flashchips' we export the size explicitly` neither.
I still want some comment, but I want it to be correct. For the rest of constant arrays situation hasn't changed, but for flashchips it had.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/ef0caac1_d476710b : PS7, Line 1472: onst struct flashchip *chip = get_flashchips()[0]; Jenkins complains on this, seems like a legit build error:
ichspi.c:1472:40: error: incompatible types when initializing type 'const struct
flashchip *' using type 'struct flashchip' 1472 | const struct flashchip *chip = get_flashchips()[0];
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/e46469f3_ea194950 : PS7, Line 98: return NULL; Could you please add `msg_gerr` before returning here, too
File tests/selfcheck.c:
https://review.coreboot.org/c/flashrom/+/75646/comment/7554d0b8_1a450006 : PS7, Line 80: flashchips = get_flashchips(); : flashchips_size = get_flashchips_size(); Can this be immediately initialised (like the above?)
-
Anastasia Klimchuk (Code Review)