Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29414 )
Change subject: soc/intel/braswell: Remove disabled LPE ACPI code ......................................................................
Patch Set 8:
(1 comment)
One of these patches where already too many people stated their opinion (I'll try anyway). This often happens when the problem description is unclear.
Frans, is this a cosmetic issue? that you just want to get rid of unneeded code? or do you experience any trouble because of the spurious code?
Experience slow system running Linux. Even reporting _STA disabled, Linux remains requesting the _STA() of this device.
So it's a workaround for a problem that we don't understand?
The issue I see here is that this solution adds a lot of redundant code. Usually such code ends up being refactored so it's very likely that such a workaround for an undocumented problem won't survive long.
Please state the reason for this change in the commit message (we should do that always, anyway) and provide enough detail to reproduce the issue. Otherwise, future work on the code can't be checked for regressions.
https://review.coreboot.org/#/c/29414/8/src/soc/intel/braswell/acpi/southclu... File src/soc/intel/braswell/acpi/southcluster.asl:
https://review.coreboot.org/#/c/29414/8/src/soc/intel/braswell/acpi/southclu... PS8, Line 291: Can't we simply move this #include to each mainboard? That would be a -3/+6 patch and we'd have a reasonable place where we could comment why this was done.