Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 )
Change subject: tests: Do not run a test if its driver is not built ......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55295/comment/d99e4585_b1bd3ef7 PS5, Line 10: however config options can be disabled and in that case test should : not be run.
I'd even say it's better to return back to this later. Why switch to another scheme before we have seen how this one turns out? I grasp from the name `init_shutdown.c` that the idea was to have individual files per tested path. Once we try to add another file, we'll probably much easier see what the advantages and disadvan- tages are.
As stated earlier, missing dependencies might be a reason to favor per programmer files. For instance if we need some library's header file to compile the test code. But that's a problem that I'd try to solve only once it's clear to exist :)
I fully agree. Predicting all potential advantages and disadvantages of some approach is hard (at least for me), and I find it easier to wait until the issues manifest themselves.
As for weak functions. I'd rather clutter the code with
if (CONFIG(MEC1308))than with weak declarations. In the only project I know that makes heavily use of weak symbols (coreboot) they often turn bad.
I concur. Most uses of weak symbols in coreboot cause more trouble than benefits, and this is why I try to get rid of weak symbols where possible.
For instance, CB:55812 could have added a weak declaration for the `mb_get_lpddr3_dq_dqs_map()` function instead of a new Kconfig option. However, mainboards with LPDDR3 *must* provide the DQ/DQS map, or else raminit will always fail. If one forgets to define the `mb_get_lpddr3_dq_dqs_map()` function, the current Kconfig approach will result in a build error (linker complains about undefined reference). Using a weak symbol would result in a runtime failure, and finding out what went wrong wouldn't be trivial (raminit is done by the MRC.bin blob, which doesn't log nor return useful error information).