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 4:
(5 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/chip.h File src/drivers/intel/ish/chip.h:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/chip.h@16 PS4, Line 16: #include <arch/acpi_device.h> This include is not needed here.
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@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?
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@73 PS4, Line 73: .acpi_fill_ssdt_generator = pci_rom_ssdt, This device will never have a rom.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@77 PS4, Line 77: .enable = 0, no need to fill out fields that are 0 value.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@78 PS4, Line 78: .ops_pci = &pci_dev_ops_pci, This will get set during probe.