Change in coreboot[master]: ec/lenovo/h8: Add an option to set F1-F12 as primary function

[Iru Cai (vimacs) (Code Review)]

Hello Iru Cai,

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

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

to review the following change.

Change subject: ec/lenovo/h8: Add an option to set F1-F12 as primary function
......................................................................

ec/lenovo/h8: Add an option to set F1-F12 as primary function

Tested on Lenovo ThinkPad T440p.

Change-Id: I83dc2c19341475abeeacd374a1b6cf152ec9b497
Signed-off-by: Iru Cai 
---
M src/ec/lenovo/h8/h8.c
1 file changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34639/1

diff --git a/src/ec/lenovo/h8/h8.c b/src/ec/lenovo/h8/h8.c
index 2b959ce..46b83b6 100644
--- a/src/ec/lenovo/h8/h8.c
+++ b/src/ec/lenovo/h8/h8.c
@@ -86,6 +86,14 @@
    	ec_clr_bit(0x0, 3);
 }

+static void f1_to_f12_as_primary(int on)
+{
+	if (on)
+		ec_set_bit(0x3b, 3);
+	else
+		ec_clr_bit(0x3b, 3);
+}
+
 static void h8_log_ec_version(void)
 {
    char ecfw[17];
@@ -334,6 +342,9 @@
    	val = 0;
    h8_sticky_fn(val);

+	if (get_option(&val, "f1_to_f12_as_primary") == CB_SUCCESS)
+		f1_to_f12_as_primary(val);
+
    if (get_option(&val, "first_battery") != CB_SUCCESS)
    	val = PRIMARY_BATTERY;
    h8_charge_priority(val);

[Paul Menzel (Code Review)]

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

Change subject: ec/lenovo/h8: Add an option to set F1-F12 as primary function
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/34639/1//COMMIT_MSG@7 
PS1, Line 7: an
The article could be removed.

https://review.coreboot.org/c/coreboot/+/34639/1//COMMIT_MSG@8 
PS1, Line 8: 
How did you find this bit?

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34639 )

Change subject: ec/lenovo/h8: Add an option to set F1-F12 as primary function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34639/1/src/ec/lenovo/h8/h8.c 
File src/ec/lenovo/h8/h8.c:

https://review.coreboot.org/c/coreboot/+/34639/1/src/ec/lenovo/h8/h8.c@346 
PS1, Line 346: 		f1_to_f12_as_primary(val);
always set this bit when a specific EC version is present?

Change in coreboot[master]: ec/lenovo/h8: Add option to set F1-F12 as primary function

[Iru Cai (vimacs) (Code Review)]

Hello Alexander Couzens, Iru Cai, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: ec/lenovo/h8: Add option to set F1-F12 as primary function
......................................................................

ec/lenovo/h8: Add option to set F1-F12 as primary function

Tested on Lenovo ThinkPad T440p.

Change-Id: I83dc2c19341475abeeacd374a1b6cf152ec9b497
Signed-off-by: Iru Cai 
---
M src/ec/lenovo/h8/Kconfig
M src/ec/lenovo/h8/h8.c
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34639/2

Change in coreboot[master]: ec/lenovo/h8: Add option to set F1-F12 as primary function

[Iru Cai (vimacs) (Code Review)]

Iru Cai (vimacs) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34639 )

Change subject: ec/lenovo/h8: Add option to set F1-F12 as primary function
......................................................................

Patch Set 2:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/34639/1//COMMIT_MSG@7 
PS1, Line 7: an
> The article could be removed.

Done

https://review.coreboot.org/c/coreboot/+/34639/1//COMMIT_MSG@8 
PS1, Line 8:
> How did you find this bit?

I found this in EcIoDxe in the OEM UEFI firmware image, and verified it with ectool.

https://review.coreboot.org/c/coreboot/+/34639/1/src/ec/lenovo/h8/h8.c 
File src/ec/lenovo/h8/h8.c:

https://review.coreboot.org/c/coreboot/+/34639/1/src/ec/lenovo/h8/h8.c@346 
PS1, Line 346: 		f1_to_f12_as_primary(val);
> always set this bit when a specific EC version is present?

I think it's better to add a Kconfig option as patch set 2 does.

Change in coreboot[master]: ec/lenovo/h8: Add option to set F1-F12 as primary function

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34639 )

Change subject: ec/lenovo/h8: Add option to set F1-F12 as primary function
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: ec/lenovo/h8: Add option to set F1-F12 as primary function

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34639 )

Change subject: ec/lenovo/h8: Add option to set F1-F12 as primary function
......................................................................

ec/lenovo/h8: Add option to set F1-F12 as primary function

Tested on Lenovo ThinkPad T440p.

Change-Id: I83dc2c19341475abeeacd374a1b6cf152ec9b497
Signed-off-by: Iru Cai 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34639
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/Kconfig
M src/ec/lenovo/h8/h8.c
2 files changed, 18 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/ec/lenovo/h8/Kconfig b/src/ec/lenovo/h8/Kconfig
index d874975..f3df56a 100644
--- a/src/ec/lenovo/h8/Kconfig
+++ b/src/ec/lenovo/h8/Kconfig
@@ -40,6 +40,10 @@
      If VBOOT is enabled, press Fn-Key at power on to force a recovery mode
      boot instead of regular FW_MAIN_x boot.

+config H8_HAS_PRIMARY_FN_KEYS
+	bool
+	default n
+
 endif

config H8_DOCK_EARLY_INIT
diff --git a/src/ec/lenovo/h8/h8.c b/src/ec/lenovo/h8/h8.c
index 2b959ce..3a99b52 100644
--- a/src/ec/lenovo/h8/h8.c
+++ b/src/ec/lenovo/h8/h8.c
@@ -86,6 +86,14 @@
    	ec_clr_bit(0x0, 3);
 }

+static void f1_to_f12_as_primary(int on)
+{
+	if (on)
+		ec_set_bit(0x3b, 3);
+	else
+		ec_clr_bit(0x3b, 3);
+}
+
 static void h8_log_ec_version(void)
 {
    char ecfw[17];
@@ -334,6 +342,12 @@
    	val = 0;
    h8_sticky_fn(val);

+	if (CONFIG(H8_HAS_PRIMARY_FN_KEYS)) {
+		if (get_option(&val, "f1_to_f12_as_primary") != CB_SUCCESS)
+			val = 1;
+		f1_to_f12_as_primary(val);
+	}
+
    if (get_option(&val, "first_battery") != CB_SUCCESS)
    	val = PRIMARY_BATTERY;
    h8_charge_priority(val);