Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45751 )
Change subject: src/{include/acpi,mainboard}: Introduce CURRENT_DSDT_REV macro ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45751/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45751/4//COMMIT_MSG@9 PS4, Line 9: Define DSDT revision according to ACPI specification 6.3 - January 2019 : page #150 Table 5-41 Differentiated System Description Table Fields (DSDT)": : DSDT rivison: : "2 This field also sets the global integer width for the AML : interpreter. Values less than two will cause the interpreter to : use 32-bit integers and math. Values of two and greater will : cause the interpreter to use full 64-bit integers and math." Please describe the change instead. The spec doesn't matter as the commit doesn't seem to change the number. AFAICS, it's only a cosmetic change, i.e. replacing 2 with a macro. This should be mentioned at least so reviewers know what to look for.
Also mention the incentive. What is better after the change? (e.g. we can then easier adapt all boards at once, which might be nice if we ever want to)
https://review.coreboot.org/c/coreboot/+/45751/4/src/include/acpi/acpi.h File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/45751/4/src/include/acpi/acpi.h@44 PS4, Line 44: CURRENT_DSDT_REV The "current" in the name seems to imply that we might update this in the future and if we do we'd do it for all boards? As we don't know what a possible future `3` would imply (at least I'm not an oracle), how can we predict if this would make sense?
The name should at least reflect the number. Otherwise, this change makes 161 files less readable, i.e. one has to look up then what CURRENT_DSDT_REV is when reading one of these files.