Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33033 )
Change subject: superio/common: Add ssdtgen for generic SuperIOs ......................................................................
Patch Set 19:
(4 comments)
haven't really reviewed the actual acpi code generation yet, but i already have some comments for the rest
https://review.coreboot.org/c/coreboot/+/33033/19/Documentation/superio/comm... File Documentation/superio/common/ssdt.md:
https://review.coreboot.org/c/coreboot/+/33033/19/Documentation/superio/comm... PS19, Line 3: This page describes the common SSDT ACPI generator that can be found in maybe add here that it is for SIOs?
https://review.coreboot.org/c/coreboot/+/33033/19/Documentation/superio/comm... PS19, Line 29: mixed spaces and tabs here; please use one of those consistently
https://review.coreboot.org/c/coreboot/+/33033/19/src/superio/common/generic... File src/superio/common/generic.c:
https://review.coreboot.org/c/coreboot/+/33033/19/src/superio/common/generic... PS19, Line 29: res->flags |= IORESOURCE_STORED; the resource gets marked as stored, but isn't stored here. is this to this device being a dummy device or does this interfere with the actual SIO config space index device?
https://review.coreboot.org/c/coreboot/+/33033/19/src/superio/common/generic... PS19, Line 167: * coreboot's generic allocator doesn't expect them behind PnP devices. is it a good idea to make this device a pnp device and not a generic device that has to contain pnp devices? Just had a look and the chip_operations that would be for the whole SIO chip doesn't provide acpi table generation hook functionality, so I saw the reason for this workaround, but I still dislike this workaround. Would this be the only case where it might be useful to generate acpi code for chips additionally to devices?