Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30989 )
Change subject: intelblocks/systemagent: Add ACPI table generation hook ......................................................................
Patch Set 2:
(5 comments)
not quite there yet :)
https://review.coreboot.org/#/c/30989/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30989/2//COMMIT_MSG@9 PS2, Line 9: SoC-scope just `SoC scope` I guess, same for `weak function` and `SoC-code` cause they aren't compound adjectives ;)
https://review.coreboot.org/#/c/30989/2//COMMIT_MSG@9 PS2, Line 9: I am not a native speaker, but I think that there should be a comma here.
https://review.coreboot.org/#/c/30989/2//COMMIT_MSG@13 PS2, Line 13: make `do no harm` should be the right one
https://review.coreboot.org/#/c/30989/2/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/#/c/30989/2/src/soc/intel/common/block/include/i... PS2, Line 79: const Don't declare constness for function-local variables, same for the `rsdp` parameter. If you want `dev` to point to constant data, then `const` must be moved _before_ the `*`, typically written as `const struct device* dev`.
https://review.coreboot.org/#/c/30989/2/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/#/c/30989/2/src/soc/intel/common/block/systemage... PS2, Line 23: #include <intelblocks/acpi.h> Is there a specific reason to not sort the includes? If so, consider documenting with a comment.