Change in coreboot[master]: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support

[Josie Nordrum (Code Review)]

Josie Nordrum has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46429 )

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

acpi/acpigen_dsm: fix I2C HID DSM to report correct function support

Fix DSM function 0 (query function) to correctly report function support
for its revision.  Revision 1 should return 0x3, all higher revisions
should return 0.

BUG=b:170862147
BRANCH=Zork
TEST=check for dmesg error

Signed-off-by: Josie Nordrum 
Change-Id: Iee082ef5cf44c4cf7ab304345af56f3b5173ca56
---
M src/acpi/acpigen_dsm.c
1 file changed, 3 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/46429/1

diff --git a/src/acpi/acpigen_dsm.c b/src/acpi/acpigen_dsm.c
index 2336537..6a89c73 100644
--- a/src/acpi/acpigen_dsm.c
+++ b/src/acpi/acpigen_dsm.c
@@ -12,23 +12,15 @@
    /* ToInteger (Arg1, Local2) */
    acpigen_write_to_integer(ARG1_OP, LOCAL2_OP);
    /* If (LEqual (Local2, 0x0)) */
-	acpigen_write_if_lequal_op_int(LOCAL2_OP, 0x0);
-	/*   Return (Buffer (One) { 0x1f }) */
-	acpigen_write_return_singleton_buffer(0x1f);
+	acpigen_write_if_lequal_op_int(LOCAL2_OP, 0x1);
+	/*   Return (Buffer (One) { 0x3 }) */
+	acpigen_write_return_singleton_buffer(0x3);
    acpigen_pop_len();	/* Pop : If */
    /* Else */
    acpigen_write_else();
-	/*   If (LEqual (Local2, 0x1)) */
-	acpigen_write_if_lequal_op_int(LOCAL2_OP, 0x1);
-	/*     Return (Buffer (One) { 0x3f }) */
-	acpigen_write_return_singleton_buffer(0x3f);
-	acpigen_pop_len();	/* Pop : If */
-	/*   Else */
-	acpigen_write_else();
    /*     Return (Buffer (One) { 0x0 }) */
    acpigen_write_return_singleton_buffer(0x0);
    acpigen_pop_len();	/* Pop : Else */
-	acpigen_pop_len();	/* Pop : Else */
 }

static void i2c_hid_func1_cb(void *arg)

[Furquan Shaikh (Code Review)]

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

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/46429/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46429/1//COMMIT_MSG@11 
PS1, Line 11: should return 0.
It would be good to add a line saying 0x3 is because I2C HID Revision 1 supports only 1 additional function.

https://review.coreboot.org/c/coreboot/+/46429/1//COMMIT_MSG@15 
PS1, Line 15: check for dmesg error
Ensure no dmesg errors. Also, you disassembled SSDT to verify it is as expected. Can you please add that information as well?

https://review.coreboot.org/c/coreboot/+/46429/1/src/acpi/acpigen_dsm.c 
File src/acpi/acpigen_dsm.c:

https://review.coreboot.org/c/coreboot/+/46429/1/src/acpi/acpigen_dsm.c@9 
PS1, Line 9: 
Can you please add a comment here capturing the details that:
1. I2C HID currently supports revision 1 only. For revision 1, only 1 additional function is supported. Thus, query function should return 0x3 (bit 0 = additional function supported, bit 1 = function with index 1 supported)
2. All other revisions do not support any additional functions, hence return 0x0.

https://review.coreboot.org/c/coreboot/+/46429/1/src/acpi/acpigen_dsm.c@14 
PS1, Line 14: 0x0
This should be 0x1 as well.

[Josie Nordrum (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

acpi/acpigen_dsm: fix I2C HID DSM to report correct function support

Fix DSM function 0 (query function) to correctly report function support
for its revision.  Revision 1 should return 0x3 because I2C HID supports
only 1 additional function. All other revisions should return 0.

BUG=b:170862147
BRANCH=Zork
TEST=ensure no dmesg errors; disassemble and verify SSDT

Signed-off-by: Josie Nordrum 
Change-Id: Iee082ef5cf44c4cf7ab304345af56f3b5173ca56
---
M src/acpi/acpigen_dsm.c
1 file changed, 11 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/46429/2

[Josie Nordrum (Code Review)]

Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46429 )

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/46429/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46429/1//COMMIT_MSG@11 
PS1, Line 11: should return 0.
> It would be good to add a line saying 0x3 is because I2C HID Revision 1 supports only 1 additional f […]

Done

https://review.coreboot.org/c/coreboot/+/46429/1//COMMIT_MSG@15 
PS1, Line 15: check for dmesg error
> Ensure no dmesg errors. Also, you disassembled SSDT to verify it is as expected. […]

Done

https://review.coreboot.org/c/coreboot/+/46429/1/src/acpi/acpigen_dsm.c 
File src/acpi/acpigen_dsm.c:

https://review.coreboot.org/c/coreboot/+/46429/1/src/acpi/acpigen_dsm.c@9 
PS1, Line 9:
> Can you please add a comment here capturing the details that: […]

Done

https://review.coreboot.org/c/coreboot/+/46429/1/src/acpi/acpigen_dsm.c@14 
PS1, Line 14: 0x0
> This should be 0x1 as well.

Done

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46429 )

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c 
File src/acpi/acpigen_dsm.c:

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c@12 
PS2, Line 12:  * 	bit 0 = additional function supported 
trailing whitespace

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c@12 
PS2, Line 12:  * 	bit 0 = additional function supported 
please, no space before tabs

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c@13 
PS2, Line 13:  * 	bit 1 = function with index 1 supported
please, no space before tabs

[Furquan Shaikh (Code Review)]

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

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c 
File src/acpi/acpigen_dsm.c:

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c@12 
PS2, Line 12:  * 	bit 0 = additional function supported
> trailing whitespace

This is causing Jenkins to report failure. You will have to get rid of the whitespace.

[Josie Nordrum (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

acpi/acpigen_dsm: fix I2C HID DSM to report correct function support

Fix DSM function 0 (query function) to correctly report function support
for its revision.  Revision 1 should return 0x3 because I2C HID supports
only 1 additional function. All other revisions should return 0.

BUG=b:170862147
BRANCH=Zork
TEST=ensure no dmesg errors; disassemble and verify SSDT

Signed-off-by: Josie Nordrum 
Change-Id: Iee082ef5cf44c4cf7ab304345af56f3b5173ca56
---
M src/acpi/acpigen_dsm.c
1 file changed, 11 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/46429/3

[Josie Nordrum (Code Review)]

Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46429 )

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c 
File src/acpi/acpigen_dsm.c:

https://review.coreboot.org/c/coreboot/+/46429/2/src/acpi/acpigen_dsm.c@12 
PS2, Line 12:  * 	bit 0 = additional function supported
> This is causing Jenkins to report failure. You will have to get rid of the whitespace.

Done

[Furquan Shaikh (Code Review)]

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

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

Patch Set 3: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46429 )

Change subject: acpi/acpigen_dsm: fix I2C HID DSM to report correct function support
......................................................................

acpi/acpigen_dsm: fix I2C HID DSM to report correct function support

Fix DSM function 0 (query function) to correctly report function support
for its revision.  Revision 1 should return 0x3 because I2C HID supports
only 1 additional function. All other revisions should return 0.

BUG=b:170862147
BRANCH=Zork
TEST=ensure no dmesg errors; disassemble and verify SSDT

Signed-off-by: Josie Nordrum 
Change-Id: Iee082ef5cf44c4cf7ab304345af56f3b5173ca56
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46429
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/acpi/acpigen_dsm.c
1 file changed, 11 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/acpi/acpigen_dsm.c b/src/acpi/acpigen_dsm.c
index 2336537..fc53ddf 100644
--- a/src/acpi/acpigen_dsm.c
+++ b/src/acpi/acpigen_dsm.c
@@ -7,28 +7,27 @@

#define ACPI_DSM_I2C_HID_UUID		"3CDFF6F7-4267-4555-AD05-B30A3D8938DE"

+/* I2C HID currently supports revision 1 only, for which, only 1 additional
+ * function is supported. Thus, the query function should return 0x3:
+ *	bit 0 = additional function supported
+ *	bit 1 = function with index 1 supported
+ * All other revisions do not support additional functions and hence return 0
+*/
+
 static void i2c_hid_func0_cb(void *arg)
 {
    /* ToInteger (Arg1, Local2) */
    acpigen_write_to_integer(ARG1_OP, LOCAL2_OP);
-	/* If (LEqual (Local2, 0x0)) */
-	acpigen_write_if_lequal_op_int(LOCAL2_OP, 0x0);
-	/*   Return (Buffer (One) { 0x1f }) */
-	acpigen_write_return_singleton_buffer(0x1f);
+	/* If (LEqual (Local2, 0x1)) */
+	acpigen_write_if_lequal_op_int(LOCAL2_OP, 0x1);
+	/*   Return (Buffer (One) { 0x3 }) */
+	acpigen_write_return_singleton_buffer(0x3);
    acpigen_pop_len();	/* Pop : If */
    /* Else */
    acpigen_write_else();
-	/*   If (LEqual (Local2, 0x1)) */
-	acpigen_write_if_lequal_op_int(LOCAL2_OP, 0x1);
-	/*     Return (Buffer (One) { 0x3f }) */
-	acpigen_write_return_singleton_buffer(0x3f);
-	acpigen_pop_len();	/* Pop : If */
-	/*   Else */
-	acpigen_write_else();
    /*     Return (Buffer (One) { 0x0 }) */
    acpigen_write_return_singleton_buffer(0x0);
    acpigen_pop_len();	/* Pop : Else */
-	acpigen_pop_len();	/* Pop : Else */
 }

static void i2c_hid_func1_cb(void *arg)