Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG@9 PS2, Line 9: The include chain can define one of the macros (BIT, MIN, MAX are all
in some scenarios, this h file will be included (not directly) with others (e.g. ec_commands. […]
No, I disagree. Macros like these are APIs just like functions, and they should be uniquely defined with a single implementation across this project. Header ordering should never have an effect on how the code works, but by doing this you risk exactly that, because which of the two definitions you're pulling in depends only on ordering (and you won't notice if they diverge). Macro definitions should not be conditionalized like this except for special circumstances.
ec_commands.h is special because it gets pulled in from another project with different definitions for these macros. That's why ec_commands.h itself has #ifndefs like this. The appropriate solution there is to make sure that ec_commands.h is always included after libpayload.h, which you can cleanly achieve by creating a wrapper header that includes both files and making sure all other depthcharge files always include that wrapper header and never ec_commands.h directly. This issue was already discussed in CL:2126525, I don't know why nothing came out of it in the end.
If this is your only problem, please solve it by including "src/drivers/ec/cros/commands.h" from your depthcharge code (instead of <ec_commands.h> directly) and adding an #include <libpayload.h> to the top of that header.