Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27276 )
Change subject: soc/amd/common: Remove redundant ACPI S3 test ......................................................................
Patch Set 4: -Code-Review
(3 comments)
https://review.coreboot.org/#/c/27276/4/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/27276/4/src/soc/amd/common/block/pi/agesawra... PS4, Line 425: : :
You do not even reach do_agesawrapper() lines calling these functions, with HAVE_ACPI_RESUME=n. […]
You do not even reach do_agesawrapper() lines calling these functions, with HAVE_ACPI_RESUME=n. You are correct.
https://review.coreboot.org/#/c/27276/4/src/soc/amd/stoneyridge/Kconfig File src/soc/amd/stoneyridge/Kconfig:
https://review.coreboot.org/#/c/27276/4/src/soc/amd/stoneyridge/Kconfig@a53 PS4, Line 53: Why remove? True, if no S3 enabled than the linker will not add these files. But it's more clear this way, plus compile time will be a little less (the files won't be compiled at all against only not being linked).
https://review.coreboot.org/#/c/27276/4/src/soc/amd/stoneyridge/chip.c File src/soc/amd/stoneyridge/chip.c:
https://review.coreboot.org/#/c/27276/4/src/soc/amd/stoneyridge/chip.c@158 PS4, Line 158: acpi_s3_resume_allowed() Not sure why this more complicated test. If it's on a resume path, then certainly S3 is allowed.