Attention is currently required from: Angel Pons. Michał Żygowski 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 2:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55714/comment/9bdc1e09_9fdbec2c PS1, Line 11: e
typo: c
Done
File acpi_ec.h:
https://review.coreboot.org/c/flashrom/+/55714/comment/6bc4df46_8be57f0b PS1, Line 41: bool ec_wait_for_ibuf(uint8_t control_port); : bool ec_wait_for_obuf(uint8_t control_port, unsigned int max_checks);
Any reason why only one of these has the `max_checks` parameter?
The erase command required longer timeout on output buffer flag. The input buffer did not require any non-default timeouts so only one had the additional parameters. Added the max_checks to `ec_wait_for_ibuf` too fro consistency.
File acpi_ec.c:
https://review.coreboot.org/c/flashrom/+/55714/comment/a0be26f6_b6586c2c PS1, Line 84: ec_wait_for_ibuf
obuf?
Yes, it should be obuf. The reference implementation had ibuf which is probably a bug and we missed it.
https://review.coreboot.org/c/flashrom/+/55714/comment/f949ba5d_3deb2a49 PS1, Line 72: : : bool ec_read_reg(uint8_t address, uint8_t *data) : { : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(EC_CMD_READ_REG, EC_CONTROL); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(address, EC_DATA); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : *data = INB(EC_DATA); : : return true; : } : : bool ec_write_reg(uint8_t address, uint8_t data) : { : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(EC_CMD_WRITE_REG, EC_CONTROL); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(address, EC_DATA); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(data, EC_DATA); : : return true; : }
Why not use the functions you defined above? […]
No idea why we didn't do that ;) Applied the suggestion.