Peter Lemenkov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Lid is open by default ......................................................................
mb/lenovo/x201/acpi_tables: Lid is open by default
It's really hard to power up this laptop with the lid closed so let's make it open by default (the same way like in a similar laptops).
Change-Id: I5bb2f716865c2bb569a4735f135842526043713c Signed-off-by: Peter Lemenkov lemenkov@gmail.com --- M src/mainboard/lenovo/x201/acpi_tables.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37985/1
diff --git a/src/mainboard/lenovo/x201/acpi_tables.c b/src/mainboard/lenovo/x201/acpi_tables.c index 6a29ba0..2a8d935 100644 --- a/src/mainboard/lenovo/x201/acpi_tables.c +++ b/src/mainboard/lenovo/x201/acpi_tables.c @@ -22,4 +22,7 @@ { gnvs->tcrt = CRITICAL_TEMPERATURE; gnvs->tpsv = PASSIVE_TEMPERATURE; + + /* the lid is open by default. */ + gnvs->lids = 1; }
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Lid is open by default ......................................................................
Patch Set 1: Code-Review+2
it's possible via the dock :)
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Lid is open by default ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37985/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37985/1//COMMIT_MSG@7 PS1, Line 7: mb/lenovo/x201/acpi_tables: Lid is open by default Please use imperative mood:
Default to lid open
Hello Alexander Couzens, Patrick Rudolph, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37985
to look at the new patch set (#2).
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
mb/lenovo/x201/acpi_tables: Default to lid open
It's really hard to power up this laptop with the lid closed so let's make it open by default (the same way like in a similar laptops).
Change-Id: I5bb2f716865c2bb569a4735f135842526043713c Signed-off-by: Peter Lemenkov lemenkov@gmail.com --- M src/mainboard/lenovo/x201/acpi_tables.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37985/2
Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37985/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37985/1//COMMIT_MSG@7 PS1, Line 7: mb/lenovo/x201/acpi_tables: Lid is open by default
Please use imperative mood: […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Patch Set 1: Code-Review+2
it's possible via the dock :)
Well, if going that route, one doesn't want the laptop to get suspended due to lid closed anyway
https://review.coreboot.org/c/coreboot/+/37985/3/src/mainboard/lenovo/x201/a... File src/mainboard/lenovo/x201/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/37985/3/src/mainboard/lenovo/x201/a... PS3, Line 27: gnvs->lids = 1; I know T410 has this order, but pretty much every other Lenovo device sets lids above the temperature limits. Would be nice to be consistent.
Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37985/3/src/mainboard/lenovo/x201/a... File src/mainboard/lenovo/x201/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/37985/3/src/mainboard/lenovo/x201/a... PS3, Line 27: gnvs->lids = 1;
I know T410 has this order, but pretty much every other Lenovo device sets lids above the temperatur […]
Done
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37985
to look at the new patch set (#4).
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
mb/lenovo/x201/acpi_tables: Default to lid open
It's really hard to power up this laptop with the lid closed so let's make it open by default (the same way like in a similar laptops).
Change-Id: I5bb2f716865c2bb569a4735f135842526043713c Signed-off-by: Peter Lemenkov lemenkov@gmail.com --- M src/mainboard/lenovo/x201/acpi_tables.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37985/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37985/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37985/4//COMMIT_MSG@10 PS4, Line 10: by default (the same way like in a similar laptops). I think this would sound more natural:
.. by default, as done on many other laptops.
Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37985/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37985/4//COMMIT_MSG@10 PS4, Line 10: by default (the same way like in a similar laptops).
I think this would sound more natural: […]
Done
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37985
to look at the new patch set (#5).
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
mb/lenovo/x201/acpi_tables: Default to lid open
It's really hard to power up this laptop with the lid closed so let's make it open by default, as done on many other laptops.
Change-Id: I5bb2f716865c2bb569a4735f135842526043713c Signed-off-by: Peter Lemenkov lemenkov@gmail.com --- M src/mainboard/lenovo/x201/acpi_tables.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37985/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 5:
It's pretty easy if you use a docking station. I'd like to see a sane default, by reading the current lid state from EC.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 5: Code-Review+2
Patch Set 5:
It's pretty easy if you use a docking station. I'd like to see a sane default, by reading the current lid state from EC.
I don't think it's a good idea, what if the OS decides to suspend?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 5:
Patch Set 5: Code-Review+2
Patch Set 5:
It's pretty easy if you use a docking station. I'd like to see a sane default, by reading the current lid state from EC.
I don't think it's a good idea, what if the OS decides to suspend?
That sentiment would also (and even more so) apply to devices with a power button outside the clam shell, and in fact that's what I suspect is what's going wrong with some of my (non-corebooted) laptops.
I'll get this in, but once somebody collects and presents the data to show that a precise reading is no regression (or even improves the situation), I'm more than happy to revert this.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
mb/lenovo/x201/acpi_tables: Default to lid open
It's really hard to power up this laptop with the lid closed so let's make it open by default, as done on many other laptops.
Change-Id: I5bb2f716865c2bb569a4735f135842526043713c Signed-off-by: Peter Lemenkov lemenkov@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37985 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/lenovo/x201/acpi_tables.c 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/lenovo/x201/acpi_tables.c b/src/mainboard/lenovo/x201/acpi_tables.c index 6a29ba0..5065648 100644 --- a/src/mainboard/lenovo/x201/acpi_tables.c +++ b/src/mainboard/lenovo/x201/acpi_tables.c @@ -20,6 +20,9 @@
void acpi_create_gnvs(global_nvs_t *gnvs) { + /* the lid is open by default. */ + gnvs->lids = 1; + gnvs->tcrt = CRITICAL_TEMPERATURE; gnvs->tpsv = PASSIVE_TEMPERATURE; }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37985 )
Change subject: mb/lenovo/x201/acpi_tables: Default to lid open ......................................................................
Patch Set 6:
I'll get this in, but once somebody collects and presents the data to show that a precise reading is no regression (or even improves the situation), I'm more than happy to revert this.
That's weirdest reasoning. We have just tested the `lid = 0` case for about 6 years. IMHO, this change fixes nothing. It merely exchanges one sometimes wrong value with another sometimes wrong value.
-
Alexander Couzens (Code Review) -
Angel Pons (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Peter Lemenkov (Code Review)