Change in coreboot[master]: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks

[Wim Vervoorn (Code Review)]

Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks

Prevent iasl remarks about unused parameters.

BUG=N/A
TEST=build

Change-Id: I54fa4712e618038fdd5a96c2012c2ec64ca34706
Signed-off-by: Wim Vervoorn 
---
M src/soc/intel/skylake/acpi/dptf/thermal.asl
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38428/1

diff --git a/src/soc/intel/skylake/acpi/dptf/thermal.asl b/src/soc/intel/skylake/acpi/dptf/thermal.asl
index 742b092..a63bd8a 100644
--- a/src/soc/intel/skylake/acpi/dptf/thermal.asl
+++ b/src/soc/intel/skylake/acpi/dptf/thermal.asl
@@ -91,6 +91,11 @@
    	Return (CTOK (Arg0))
    } Else {
 #endif
+#ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
+		/* Prevent iasl remarks about unused parameters */
+		Store( Arg0, Local0)
+		Store( Local0, Arg0)
+#endif
    	Return (CTOK (Arg1))
 #ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES
    }

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 2:

I wonder if there is a solution that would allow using less preprocessor.

[Wim Vervoorn (Code Review)]

Hello Patrick Rudolph, Frans Hendriks, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks

Prevent iasl remarks about unused parameters.

BUG=N/A
TEST=build

Change-Id: I54fa4712e618038fdd5a96c2012c2ec64ca34706
Signed-off-by: Wim Vervoorn 
---
M src/soc/intel/skylake/acpi/dptf/thermal.asl
1 file changed, 7 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38428/3

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 3:
> Patch Set 2:
>
> I wonder if there is a solution that would allow using less preprocessor.

I had a look at it. You are right. I updated the code to make it easier to read. Unfortunately, I am not aware of a better way to prevent the remarks. I am not aware of a pragma indicating an argument is optional or not handled on purpose.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38428/1/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/dptf/thermal.asl:

https://review.coreboot.org/c/coreboot/+/38428/1/src/soc/intel/skylake/acpi/... 
PS1, Line 96: 		Store( Arg0, Local0)
Remove the space after (?

[Wim Vervoorn (Code Review)]

Hello Patrick Rudolph, Frans Hendriks, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#4).

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks

Prevent iasl remarks about unused parameters.

BUG=N/A
TEST=build

Change-Id: I54fa4712e618038fdd5a96c2012c2ec64ca34706
Signed-off-by: Wim Vervoorn 
---
M src/soc/intel/skylake/acpi/dptf/thermal.asl
1 file changed, 7 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38428/4

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38428/1/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/dptf/thermal.asl:

https://review.coreboot.org/c/coreboot/+/38428/1/src/soc/intel/skylake/acpi/... 
PS1, Line 96: 		Store( Arg0, Local0)
> Remove the space after (?

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 4: Code-Review+1

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 4: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38428/4/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/dptf/thermal.asl:

https://review.coreboot.org/c/coreboot/+/38428/4/src/soc/intel/skylake/acpi/... 
PS4, Line 89: #ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
            : 	/* Prevent iasl remarks about unused parameters */
            : 	Store(Arg0, Local0)
            : #endif
            : 	Store(Arg1, Local0)
Would this work:

Change src/ec/google/chromeec/acpi/ec.asl so that Method RDCP is defined only if EC_ENABLE_MULTIPLE_DPTF_PROFILES is defined and then the check here can be simplified:

Method (DTRP, 2, Serialized)
{
    If (CondRefOf (_SB.PCI0.LPCB.EC0.RCDP)) {
         If (_SB.PCI0.LPCB.EC0.RCDP == One)
             Return (CTOK (Arg0))
    }

Return (CTOK (Arg1))
}

Just avoids the multiple stores to Local0 which seemed a little confusing.

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/skylake/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38428/4/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/dptf/thermal.asl:

https://review.coreboot.org/c/coreboot/+/38428/4/src/soc/intel/skylake/acpi/... 
PS4, Line 89: #ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
            : 	/* Prevent iasl remarks about unused parameters */
            : 	Store(Arg0, Local0)
            : #endif
            : 	Store(Arg1, Local0)
> Would this work: […]

If I add this as well, it works. I will create an updated patch.

#ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
External (_SB.PCI0.LPCB.EC0.RCDP, MethodObj)
#endif

Change in coreboot[master]: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks

[Wim Vervoorn (Code Review)]

Hello Patrick Rudolph, Frans Hendriks, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#5).

Change subject: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks

Prevent iasl remarks about unused parameters.

BUG=N/A
TEST=build

Change-Id: I54fa4712e618038fdd5a96c2012c2ec64ca34706
Signed-off-by: Wim Vervoorn 
---
M src/ec/google/chromeec/acpi/ec.asl
M src/soc/intel/common/acpi/dptf/thermal.asl
M src/soc/intel/skylake/acpi/dptf/thermal.asl
3 files changed, 21 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38428/5

Change in coreboot[master]: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38428/1/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/dptf/thermal.asl:

https://review.coreboot.org/c/coreboot/+/38428/1/src/soc/intel/skylake/acpi/... 
PS1, Line 96: 		Store( Arg0, Local0)
> Done

Done

https://review.coreboot.org/c/coreboot/+/38428/4/src/soc/intel/skylake/acpi/... 
File src/soc/intel/skylake/acpi/dptf/thermal.asl:

https://review.coreboot.org/c/coreboot/+/38428/4/src/soc/intel/skylake/acpi/... 
PS4, Line 89: #ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
            : 	/* Prevent iasl remarks about unused parameters */
            : 	Store(Arg0, Local0)
            : #endif
            : 	Store(Arg1, Local0)
> If I add this as well, it works. I will create an updated patch. […]

Done

Change in coreboot[master]: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38428 )

Change subject: soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks
......................................................................

soc/intel/{skylake,common}/acpi/dptf/thermal.asl: Prevent iasl remarks

Prevent iasl remarks about unused parameters.

BUG=N/A
TEST=build

Change-Id: I54fa4712e618038fdd5a96c2012c2ec64ca34706
Signed-off-by: Wim Vervoorn 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38428
Tested-by: build bot (Jenkins) 
Reviewed-by: Frans Hendriks 
---
M src/ec/google/chromeec/acpi/ec.asl
M src/soc/intel/common/acpi/dptf/thermal.asl
M src/soc/intel/skylake/acpi/dptf/thermal.asl
3 files changed, 21 insertions(+), 17 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Frans Hendriks: Looks good to me, approved

diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl
index 962988e..bf792d3 100644
--- a/src/ec/google/chromeec/acpi/ec.asl
+++ b/src/ec/google/chromeec/acpi/ec.asl
@@ -510,6 +510,7 @@
    	Return (^TBMD)
    }

+#ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES
    /* Read current Device DPTF Profile Number */
    Method (RCDP, 0, NotSerialized)
    {
@@ -524,7 +525,7 @@
    		Return (Local0)
    	}
    }
-
+#endif
 #if CONFIG(EC_GOOGLE_CHROMEEC_ACPI_USB_PORT_POWER)
    /*
     * Enable USB Port Power
diff --git a/src/soc/intel/common/acpi/dptf/thermal.asl b/src/soc/intel/common/acpi/dptf/thermal.asl
index 7058b27..6e361dc 100644
--- a/src/soc/intel/common/acpi/dptf/thermal.asl
+++ b/src/soc/intel/common/acpi/dptf/thermal.asl
@@ -79,6 +79,10 @@
 #endif
 }

+#ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
+External (_SB.PCI0.LPCB.EC0.RCDP, MethodObj)
+#endif
+
 /*
  * Method to return trip temperature value depending upon the device mode.
  * Arg0 --> Value to return when device is in tablet mode
@@ -86,15 +90,12 @@
  */
 Method (DTRP, 2, Serialized)
 {
-#ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES
-	If (LEqual (_SB.PCI0.LPCB.EC0.RCDP, One)) {
-		Return (CTOK (Arg0))
-	} Else {
-#endif
-		Return (CTOK (Arg1))
-#ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES
+	If (CondRefOf (_SB.PCI0.LPCB.EC0.RCDP)) {
+		If (LEqual (_SB.PCI0.LPCB.EC0.RCDP, One)) {
+			Return (CTOK (Arg0))
+		}
    }
-#endif
+	Return (CTOK (Arg1))
 }

#ifdef DPTF_TSR0_SENSOR_ID
diff --git a/src/soc/intel/skylake/acpi/dptf/thermal.asl b/src/soc/intel/skylake/acpi/dptf/thermal.asl
index 742b092..f73d366 100644
--- a/src/soc/intel/skylake/acpi/dptf/thermal.asl
+++ b/src/soc/intel/skylake/acpi/dptf/thermal.asl
@@ -79,6 +79,11 @@
 }

#if defined(DPTF_TSR0_SENSOR_ID) || defined(DPTF_TSR1_SENSOR_ID) || defined(DPTF_TSR2_SENSOR_ID)
+
+#ifndef EC_ENABLE_MULTIPLE_DPTF_PROFILES
+External (_SB.PCI0.LPCB.EC0.RCDP, MethodObj)
+#endif
+
 /*
  * Method to return trip temperature value depending upon the device mode.
  * Arg0 --> Value to return when device is in tablet mode
@@ -86,15 +91,12 @@
  */
 Method (DTRP, 2, Serialized)
 {
-#ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES
-	If (LEqual (_SB.PCI0.LPCB.EC0.RCDP, One)) {
-		Return (CTOK (Arg0))
-	} Else {
-#endif
-		Return (CTOK (Arg1))
-#ifdef EC_ENABLE_MULTIPLE_DPTF_PROFILES
+	If (CondRefOf (_SB.PCI0.LPCB.EC0.RCDP)) {
+		If (LEqual (_SB.PCI0.LPCB.EC0.RCDP, One)) {
+			Return (CTOK (Arg0))
+		}
    }
-#endif
+	Return (CTOK (Arg1))
 }
 #endif