Hello Iru Cai,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38650
to review the following change.
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
lenovo/t440p: fix keyboard backlight
Change-Id: Ib4c2b1591d26e2bb33f9549e3933efe9a6e0b043 Signed-off-by: Iru Cai mytbk920423@gmail.com --- M src/mainboard/lenovo/t440p/cmos.layout M src/mainboard/lenovo/t440p/devicetree.cb 2 files changed, 5 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38650/1
diff --git a/src/mainboard/lenovo/t440p/cmos.layout b/src/mainboard/lenovo/t440p/cmos.layout index 8c5fb0b..e61886b 100644 --- a/src/mainboard/lenovo/t440p/cmos.layout +++ b/src/mainboard/lenovo/t440p/cmos.layout @@ -103,9 +103,10 @@ 8 1 Primary 9 0 AHCI 9 1 Compatible -10 0 Both -10 1 Keyboard only -10 2 Thinklight only +# Haswell ThinkPads has no Thinklight +#10 0 Both +10 1 Keyboard +#10 2 Thinklight only 10 3 None #12 0 Integrated Only #12 1 Discrete Only diff --git a/src/mainboard/lenovo/t440p/devicetree.cb b/src/mainboard/lenovo/t440p/devicetree.cb index 7e5e616..b63767e 100644 --- a/src/mainboard/lenovo/t440p/devicetree.cb +++ b/src/mainboard/lenovo/t440p/devicetree.cb @@ -83,6 +83,7 @@ register "config1" = "0x0d" register "config2" = "0xa8" register "config3" = "0xc4" + register "has_keyboard_backlight" = "1" register "event2_enable" = "0xff" register "event3_enable" = "0xff" register "event4_enable" = "0xd0"
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38650/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38650/1//COMMIT_MSG@8 PS1, Line 8: Please elaborate in the commit message body. What is broken, and how do you fix it.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... File src/mainboard/lenovo/t440p/cmos.layout:
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... PS1, Line 106: has have
Dennis Witzig has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
When changing it to "Keyboard" manually, it builds successful and the backlight works again on my T440p. Deactivating it with nvramtool also works.
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... File src/mainboard/lenovo/t440p/cmos.layout:
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... PS1, Line 107: Both cmos.default is still set to "Both" which gives an error while building because it now doesn't exist anymore
Hello Alexander Couzens, Dennis Witzig, Patrick Rudolph, Iru Cai, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38650
to look at the new patch set (#2).
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
lenovo/t440p: fix keyboard backlight
It is found that keyboard backlight in T440p is enabled by clearing bit 3 of EC RAM 0x01. This patch sets has_keyboard_backlight in devicetree.cb and also corrects the CMOS configuration.
Change-Id: Ib4c2b1591d26e2bb33f9549e3933efe9a6e0b043 Signed-off-by: Iru Cai mytbk920423@gmail.com --- M src/mainboard/lenovo/t440p/cmos.default M src/mainboard/lenovo/t440p/cmos.layout M src/mainboard/lenovo/t440p/devicetree.cb 3 files changed, 6 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38650/2
Iru Cai (vimacs) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38650/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38650/1//COMMIT_MSG@8 PS1, Line 8:
Please elaborate in the commit message body. What is broken, and how do you fix it.
Done
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... File src/mainboard/lenovo/t440p/cmos.layout:
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... PS1, Line 106: has
have
Done
https://review.coreboot.org/c/coreboot/+/38650/1/src/mainboard/lenovo/t440p/... PS1, Line 107: Both
cmos. […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2: Code-Review+2
Dennis Witzig has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2: Code-Review+1
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2: Code-Review+2
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2: Code-Review-1
cmos default still says it's defaulting to "Both".
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
cmos default still says it's defaulting to "Both".
What do you mean?
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-1
cmos default still says it's defaulting to "Both".
What do you mean?
The file src/mainboard/lenovo/t440p/cmos.default contains "backlight=Both"
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-1
cmos default still says it's defaulting to "Both".
What do you mean?
The file src/mainboard/lenovo/t440p/cmos.default contains "backlight=Both"
Does it?
https://review.coreboot.org/c/coreboot/+/38650/2/src/mainboard/lenovo/t440p/...
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2: -Code-Review
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-1
cmos default still says it's defaulting to "Both".
What do you mean?
The file src/mainboard/lenovo/t440p/cmos.default contains "backlight=Both"
Does it?
https://review.coreboot.org/c/coreboot/+/38650/2/src/mainboard/lenovo/t440p/...
Sorry, maybe I downloaded by accident the v1 of the change via git review.
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 2: Code-Review+2
Alexander Couzens has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
lenovo/t440p: fix keyboard backlight
It is found that keyboard backlight in T440p is enabled by clearing bit 3 of EC RAM 0x01. This patch sets has_keyboard_backlight in devicetree.cb and also corrects the CMOS configuration.
Change-Id: Ib4c2b1591d26e2bb33f9549e3933efe9a6e0b043 Signed-off-by: Iru Cai mytbk920423@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38650 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Dennis Witzig dennis@wtzg.de Reviewed-by: Alexander Couzens lynxis@fe80.eu --- M src/mainboard/lenovo/t440p/cmos.default M src/mainboard/lenovo/t440p/cmos.layout M src/mainboard/lenovo/t440p/devicetree.cb 3 files changed, 6 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Alexander Couzens: Looks good to me, approved Angel Pons: Looks good to me, approved Dennis Witzig: Looks good to me, but someone else must approve
diff --git a/src/mainboard/lenovo/t440p/cmos.default b/src/mainboard/lenovo/t440p/cmos.default index b104ae1..0949e7b 100644 --- a/src/mainboard/lenovo/t440p/cmos.default +++ b/src/mainboard/lenovo/t440p/cmos.default @@ -9,5 +9,5 @@ f1_to_f12_as_primary=Enable sticky_fn=Disable trackpoint=Enable -backlight=Both +backlight=Keyboard usb_always_on=Disable diff --git a/src/mainboard/lenovo/t440p/cmos.layout b/src/mainboard/lenovo/t440p/cmos.layout index 8c5fb0b..f65933a 100644 --- a/src/mainboard/lenovo/t440p/cmos.layout +++ b/src/mainboard/lenovo/t440p/cmos.layout @@ -103,9 +103,10 @@ 8 1 Primary 9 0 AHCI 9 1 Compatible -10 0 Both -10 1 Keyboard only -10 2 Thinklight only +# Haswell ThinkPads have no Thinklight +#10 0 Both +10 1 Keyboard +#10 2 Thinklight only 10 3 None #12 0 Integrated Only #12 1 Discrete Only diff --git a/src/mainboard/lenovo/t440p/devicetree.cb b/src/mainboard/lenovo/t440p/devicetree.cb index 7e5e616..b63767e 100644 --- a/src/mainboard/lenovo/t440p/devicetree.cb +++ b/src/mainboard/lenovo/t440p/devicetree.cb @@ -83,6 +83,7 @@ register "config1" = "0x0d" register "config2" = "0xa8" register "config3" = "0xc4" + register "has_keyboard_backlight" = "1" register "event2_enable" = "0xff" register "event3_enable" = "0xff" register "event4_enable" = "0xd0"
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38650 )
Change subject: lenovo/t440p: fix keyboard backlight ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/402 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/401 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/400
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Alexander Couzens (Code Review)
-
Angel Pons (Code Review)
-
Dennis Witzig (Code Review)
-
Iru Cai (vimacs) (Code Review)
-
Paul Menzel (Code Review)