Change in coreboot[master]: ACPI: Define acpi_get_preferred_pm_profile()

[Felix Held (Code Review)]

Hello Kyösti Mälkki,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/47666

to review the following change.

Change subject: ACPI: Define acpi_get_preferred_pm_profile()
......................................................................

ACPI: Define acpi_get_preferred_pm_profile()

Change-Id: Id90737cc93261290b7227e9a3249c2b7df79ca3d
Signed-off-by: Kyösti Mälkki 
Signed-off-by: Felix Held 
---
M src/acpi/acpi.c
M src/arch/x86/Makefile.inc
A src/arch/x86/acpi_pm.c
M src/include/acpi/acpi.h
4 files changed, 26 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/47666/1

diff --git a/src/acpi/acpi.c b/src/acpi/acpi.c
index 259813b..f08a6e3 100644
--- a/src/acpi/acpi.c
+++ b/src/acpi/acpi.c
@@ -1251,14 +1251,7 @@
    /* should be 0 ACPI 3.0 */
    fadt->reserved = 0;

-	if (CONFIG(SYSTEM_TYPE_CONVERTIBLE) ||
-	    CONFIG(SYSTEM_TYPE_LAPTOP))
-		fadt->preferred_pm_profile = PM_MOBILE;
-	else if (CONFIG(SYSTEM_TYPE_DETACHABLE) ||
-		 CONFIG(SYSTEM_TYPE_TABLET))
-		fadt->preferred_pm_profile = PM_TABLET;
-	else
-		fadt->preferred_pm_profile = PM_DESKTOP;
+	fadt->preferred_pm_profile = acpi_get_preferred_pm_profile();

arch_fill_fadt(fadt);

diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index a5c3309..8470989 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -240,6 +240,7 @@
 ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32)$(CONFIG_ARCH_RAMSTAGE_X86_64),y)

ramstage-y += acpi.c
+ramstage-y += acpi_pm.c
 ramstage-$(CONFIG_HAVE_ACPI_RESUME) += acpi_s3.c
 ramstage-$(CONFIG_ACPI_BERT) += acpi_bert_storage.c
 ramstage-y += boot.c
diff --git a/src/arch/x86/acpi_pm.c b/src/arch/x86/acpi_pm.c
new file mode 100644
index 0000000..59ea29d
--- /dev/null
+++ b/src/arch/x86/acpi_pm.c
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <acpi/acpi.h>
+#include <smbios.h>
+
+/* Default mapping to ACPI FADT preferred_pm_profile field. */
+uint8_t acpi_get_preferred_pm_profile(void)
+{
+	switch (smbios_mainboard_enclosure_type()) {
+	case SMBIOS_ENCLOSURE_LAPTOP:
+	case SMBIOS_ENCLOSURE_CONVERTIBLE:
+		return PM_MOBILE;
+	case SMBIOS_ENCLOSURE_DETACHABLE:
+	case SMBIOS_ENCLOSURE_TABLET:
+		return PM_TABLET;
+	case SMBIOS_ENCLOSURE_DESKTOP:
+		return PM_DESKTOP;
+	case SMBIOS_ENCLOSURE_UNKNOWN:
+	default:
+		return PM_UNSPECIFIED;
+	}
+}
diff --git a/src/include/acpi/acpi.h b/src/include/acpi/acpi.h
index 78740fb..71dcea8 100644
--- a/src/include/acpi/acpi.h
+++ b/src/include/acpi/acpi.h
@@ -1044,6 +1044,8 @@
 }
 #endif

+uint8_t acpi_get_preferred_pm_profile(void);
+
 /* Returns ACPI_Sx values. */
 int acpi_get_sleep_type(void);

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47666 )

Change subject: ACPI: Define acpi_get_preferred_pm_profile()
......................................................................

Patch Set 1:

Felix, would you like CB:42140 restored instead, with existing comments mostly addressed?

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47666 )

Change subject: ACPI: Define acpi_get_preferred_pm_profile()
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Felix, would you like CB:42140 restored instead, with existing comments mostly addressed?

If you think that that would be a better idea, please do so and I'll abandon this one. Compared to the change I pulled from the review system I did some improvement here https://review.coreboot.org/c/coreboot/+/47666/1/src/arch/x86/acpi_pm.c#9 (avoid casting the enum value to int before the switch case that sued the enum values again)

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47666 )

Change subject: ACPI: Define acpi_get_preferred_pm_profile()
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > Felix, would you like CB:42140 restored instead, with existing comments mostly addressed?
>
> If you think that that would be a better idea, please do so and I'll abandon this one. Compared to the change I pulled from the review system I did some improvement here https://review.coreboot.org/c/coreboot/+/47666/1/src/arch/x86/acpi_pm.c#9 (avoid casting the enum value to int before the switch case that sued the enum values again)

ah, that one wasn't deleted like most of your patches. yeah then it would be better to have your patch restored and rebased instead

[Felix Held (Code Review)]

Felix Held has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47666 )

Change subject: ACPI: Define acpi_get_preferred_pm_profile()
......................................................................

Abandoned

sort-of duplicate of CB:42140 that will get restored instead