Furquan Shaikh 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 4:
(3 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@33 PS4, Line 33: acpigen_write_scope(acpi_device_scope(root)); : acpigen_write_device(acpi_device_name(root)); This will end up adding ISHB device again to the ACPI tree -- probably resulting in parsing errors in kernel and also associating the _DSD with incorrect device. This should just emit scope here:
acpigen_write_scope(acpi_device_path(root));
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@40 PS4, Line 40: acpigen_write_name_dword("_ADR", address);
Why are we adding in an _ADR field if it's already in the static asl file?
Yes, this should not be required.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@46 PS4, Line 46: acpigen_pop_len(); /* Device */ Not required.