Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 16:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 45: EC_PD_TRY_POWER_ROLE_NONE Just noting: This case makes sense for completeness. But the way it is used currently, this case can never occur.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 131: so malloc() : * and strdup() are used for that. No problem with doing this. But this can also be handled by: static const char usb2_port[] = "usb2-port"; static const char usb3_port[] = "usb3-port";
const char *usb_port_name; If port type is usb2 set usb_port_name to usb2_port, else if it is usb3, set usb_port_name to usb3_port.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 138: if (port->enabled && port->path.type == DEVICE_PATH_USB && Just a nit. If negative conditions are checked and loop is skipped early on, code wouldn't require too much indentation.
if (!port->enabled || port->path.type != DEVICE_PATH_USB continue;
if (port->path.usb.port_type == 2) usb_port_name = usb2_port; else if (port->path.usb.port_type == 3) usb_port_name = usb3_port; else continue;
struct drivers_usb_acpi_config *config = port->chip_info; if (config->type == UPC_TYPE_INTERNAL) // I don't think this is correct. See comment below. continue;
if (config->group.token != (port_number + 1)) continue;
...
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 150: config->type != UPC_TYPE_INTERNAL Shouldn't this check ensure that config->type is one of the following: UPC_TYPE_C_USB2_SS_SWITCH UPC_TYPE_C_USB2_ONLY UPC_TYPE_C_USB2_SS
If a port is not of type UPC_TYPE_INTERNAL, it is not necessary that it will be of type C. Example: https://chromium.git.corp.google.com/chromiumos/third_party/coreboot/+/0c52a...
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 154: 10 sizeof(usb_port)?
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 159: 10 sizeof(usb_port)?
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 185: return If you return here, then device and scope acpigen_pop_len() would be skipped. I think it would be good to move this call to google_chromeec_get_num_pd_ports() to happen before acpigen_write_scope().