Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37639 )
Change subject: superio/common/conf_mode: Add op to write SSDT ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37639/2/src/include/device/pnp.h File src/include/device/pnp.h:
https://review.coreboot.org/c/coreboot/+/37639/2/src/include/device/pnp.h@73 PS2, Line 73: void (*ssdt_enter_conf_mode)(struct device *dev, const char *idx, const char *data); : void (*ssdt_exit_conf_mode)(struct device *dev, const char *idx, const char *data); should this be guarded by #if CONFIG(HAVE_ACPI_TABLES) to be consistent with device_operations in device.h? without having looked into the details i wonder though, why some members of other structs are only present when HAVE_ACPI_TABLES is set.
https://review.coreboot.org/c/coreboot/+/37639/2/src/include/device/pnp.h@78 PS2, Line 78: const char *idx, const char *data maybe add a comment that those two parameters correspond to the index and data register of the sio config register pair; wasn't obvious to me before having a look at the actual implementation
https://review.coreboot.org/c/coreboot/+/37639/2/src/superio/common/conf_mod... File src/superio/common/conf_mode.c:
https://review.coreboot.org/c/coreboot/+/37639/2/src/superio/common/conf_mod... PS2, Line 181: 2 0x02 would be more consistent here; same thing a few lines below