Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@29 PS1, Line 29: if (!dev->enabled || !config || !config->firmware_variant) Shouldn't we check if root->enabled is true as well? Or does the device walking already take that into account? looks like it doesn't such notions:
-------{ ------->-------struct device *dev; ------->-------for (dev = all_devices; dev; dev = dev->next) ------->------->-------if (dev->ops && dev->ops->acpi_fill_ssdt_generator) ------->------->------->-------dev->ops->acpi_fill_ssdt_generator(dev); ------->-------current = (unsigned long) acpigen_get_current(); -------}
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@71 PS1, Line 71: static struct device_operations pci_ish_device_ops; Why aren't you working on the intel_ish_ops directly? The path below NULLs out acpi_fill_ssdt_generator and then at enable_dev time you're switching ops out again. This is really bizarre. The ops needs to be a composition of the default pci device ops with only updating .scan_bus field.
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@90 PS1, Line 90: ish_intel_driver.devices = pci_device_ids; Why is this being initialized again? Just put those in the static initializer.