Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33033 )
Change subject: superio/common: Add ssdtgen for generic SuperIOs ......................................................................
Patch Set 27:
(16 comments)
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?
Done
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
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/generic... File src/superio/common/generic.c:
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/generic... PS25, Line 97: FIELDLIST_NAMESTR("ACTR", 8),
would it be useful to have this as 8 individual bits and not as one 8 bit value for enabling/disabli […]
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/generic... PS25, Line 100: FIELDLIST_NAMESTR("IOAL", 8),
I thought those were the names used for IO0H/IO0L. […]
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/generic... PS25, Line 105: FIELDLIST_NAMESTR("INTT", 4),
Ack
Done
https://review.coreboot.org/c/coreboot/+/33033/16/src/superio/common/ssdt.c File src/superio/common/ssdt.c:
https://review.coreboot.org/c/coreboot/+/33033/16/src/superio/common/ssdt.c@... PS16, Line 17: include <superio/common/chip.h> : #include <superio/common/ssdt.h> : : #include <device/device.h> : #include <device/pnp.h> : #include <arch/acpigen.h> : #include <device/pnp_def.h> : #include <console/console.h>
to sort
no
https://review.coreboot.org/c/coreboot/+/33033/16/src/superio/common/ssdt.c@... PS16, Line 25:
missing include <types. […]
Done
https://review.coreboot.org/c/coreboot/+/33033/16/src/superio/common/ssdt.c@... PS16, Line 54: printk(BIOS_ERR, "%s: Missing ACPI path/scope\n", : dev_path(dev));
on one line ?
Done
https://review.coreboot.org/c/coreboot/+/33033/16/src/superio/common/ssdt.c@... PS16, Line 69: struct opregion opreg = OPREGION("IOID", SYSTEMIO, : dev->path.pnp.port, 2);
one line
Done
https://review.coreboot.org/c/coreboot/+/33033/16/src/superio/common/ssdt.c@... PS16, Line 324: printk(BIOS_WARNING, "%s: No HID set in devicetree\n", : dev_path(dev));
one line
Done
https://review.coreboot.org/c/coreboot/+/33033/19/src/superio/common/ssdt.c File src/superio/common/ssdt.c:
https://review.coreboot.org/c/coreboot/+/33033/19/src/superio/common/ssdt.c@... PS19, Line 115: res->base)
Needs to be (1 << res->base) I think.
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c File src/superio/common/ssdt.c:
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 101: while (size > 0) {
it is in bytes
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 103: 0
I guess 1 would be correct
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 110: >
Ack
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 147: snprintf(name, sizeof(name), "VLD%X", vldn);
it makes no assumptions. Will add LDN, too.
Done
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 226: ldn_gen_resources_use(dev);
it just outputs the integers "IOxB"/"IOxS" for every resource in the LDN scope. […]
Done