[flashrom-gerrit] Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API

[Angel Pons (Code Review)]

Attention is currently required from: Michał Żygowski.
Angel Pons 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 1:

(5 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/55714/comment/24cdc698_9db2d472 
PS1, Line 7: ACPI
What part of the code is about ACPI? Sure, the EC data/control I/O register pair is described in ACPI, but the interface per se has nothing to do with ACPI, as far as I know.

https://review.coreboot.org/c/flashrom/+/55714/comment/42f420a4_6c296a1e 
PS1, Line 11: e
typo: c

File acpi_ec.h:

https://review.coreboot.org/c/flashrom/+/55714/comment/36a9f392_6ef81d64 
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?

File acpi_ec.c:

https://review.coreboot.org/c/flashrom/+/55714/comment/a0c88db5_85de96b7 
PS1, Line 84: ec_wait_for_ibuf
obuf?

https://review.coreboot.org/c/flashrom/+/55714/comment/2bc72922_7a12ff9e 
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?

bool ec_read_reg(uint8_t address, uint8_t *data)
 {
    if (!ec_write_cmd(EC_CONTROL, EC_CMD_READ_REG))
    	return false;

if (!ec_write_byte(EC_CONTROL, EC_DATA, address))
    	return false;

return ec_read_byte(EC_CONTROL, EC_DATA, data);
 }

bool ec_write_reg(uint8_t address, uint8_t data)
 {
    if (!ec_write_cmd(EC_CONTROL, EC_CMD_READ_REG))
    	return false;

if (!ec_write_byte(EC_CONTROL, EC_DATA, address))
    	return false;

return ec_write_byte(EC_CONTROL, EC_DATA, data);
 }