Attention is currently required from: Michał Żygowski, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 )
Change subject: acpi_ec: Implement basic ACPI embedded controller API ......................................................................
Patch Set 6: Code-Review+1
(4 comments)
File acpi_ec.h:
https://review.coreboot.org/c/flashrom/+/55714/comment/5c457c98_20b63019 PS6, Line 20: #if defined(__i386__) || defined(__x86_64__) I know it's still a common pattern, but this shouldn't be here. Such things are up to the build system and shouldn't clutter the code. Same for the `.c` file, it should simply not be compiled if incompatible.
https://review.coreboot.org/c/flashrom/+/55714/comment/c04b3d73_fa7632e6 PS6, Line 39: uint8_t address, uint8_t *data I guess it's ok because even a C compiler notices the different types (by now), but we usually keep the destination as the first argument (like in standard C functions, memcpy() etc.). That keeps the order like it would be in an assignment statement.
File acpi_ec.c:
https://review.coreboot.org/c/flashrom/+/55714/comment/f23ef51c_a753eb27 PS6, Line 43: EC_MAX_STATUS_CHECKS If this is the default, why is it also part of the API?
https://review.coreboot.org/c/flashrom/+/55714/comment/ffa67e91_30276549 PS6, Line 48: } Would you mind exchanging the conditions? It seems odd not to check the index in the for condition. e.g.
for (i = 0; i < max_checks; ++i) { if ((INB(EC_STS_CMD) & EC_STS_IBF) == 0) return true; }
return false;