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 25:
(7 comments)
finally i got around to have a closer look at the patch...
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 100: FIELDLIST_NAMESTR("IOAL", 8), IOH1, IOL1 missing?
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/generic... PS25, Line 105: FIELDLIST_NAMESTR("INTT", 4), second irq missing
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) { hmm, for a 256 byte big resource this will generate a 255 and a 1 byte acpi io resource. the resource size for the acpi io region is in bytes and not in bytes-1, right?
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 103: 0 AddressAlignment == 0 means fixed, right?
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 110: > shouldn't this be a >= here?
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 147: snprintf(name, sizeof(name), "VLD%X", vldn); for the vldn the base ldn also needs to be known; only the enable bit position (the virtual part of the ldn) isn't sufficient. or is the assumtion that vldn 0 always exists when other ldns exist as well?
https://review.coreboot.org/c/coreboot/+/33033/25/src/superio/common/ssdt.c@... PS25, Line 226: ldn_gen_resources_use(dev); this doesn't need a name, header and footer?