Change in coreboot[master]: mb/lenovo/x201: Add support for undock button on X200 ultrabase

[Maccraft123 (Code Review)]

Maccraft123 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

mb/lenovo/x201: Add support for undock button on X200 ultrabase

Change-Id: Iaaecad031bb1f39dd1a778d0c8eaea6bce9e0f57
Signed-off-by: Maciej Matuszczyk 
---
M src/mainboard/lenovo/x201/acpi/dock.asl
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/38446/1

diff --git a/src/mainboard/lenovo/x201/acpi/dock.asl b/src/mainboard/lenovo/x201/acpi/dock.asl
index 2bba821..b3381a7 100644
--- a/src/mainboard/lenovo/x201/acpi/dock.asl
+++ b/src/mainboard/lenovo/x201/acpi/dock.asl
@@ -60,6 +60,11 @@
    	Notify(_SB.DOCK, 3)
    }

+	Method(_Q50, 0, NotSerialized)
+	{
+		Notify(_SB.DOCK, 3)
+	}
+
    Method(_Q58, 0, NotSerialized)
    {
    	Notify(_SB.DOCK, 0)

[Maccraft123 (Code Review)]

Hello Alexander Couzens, Patrick Rudolph,

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

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

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

mb/lenovo/x201: Add support for undock button on X200 ultrabase

Only ACPI code had to be changed,
smihandler.c already supported it.

Change-Id: Iaaecad031bb1f39dd1a778d0c8eaea6bce9e0f57
Signed-off-by: Maciej Matuszczyk 
---
M src/mainboard/lenovo/x201/acpi/dock.asl
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/38446/2

[Mimoja (Code Review)]

Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 2: Code-Review+1

I would love a bit more verbose commit message on how it all comes together, but thats just me. Looks fine to me :)

[Peter Lemenkov (Code Review)]

Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 2:

Should the same code be added to t410/t410s as well? They use the same dock stations so I guess should use the same code.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 2: Code-Review+2

[Maccraft123 (Code Review)]

Maccraft123 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Should the same code be added to t410/t410s as well? They use the same dock stations so I guess should use the same code.

It can be added, but it needs testing. X201 and T410 use different docking stations, but EC is the same.

[Paul Menzel (Code Review)]

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@7 
PS2, Line 7: mb/lenovo/x201: Add support for undock button on X200 ultrabase
> mb/lenovo/x201: Support undock button on X200 ultrabase

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@10 
PS2, Line 10: smihandler.c already supported it.
One line please. Maybe:
> Only the ACPI code needs to be extended, as smihandler.c already
> supported it. _Q50 is taken from the vendor DSDT.

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@11 
PS2, Line 11: 
Please add the problem description. I have no idea, but something like:
> Currently pressing the undock button in Linux 4.… nothing happens,
> and the device cannot be safely undocked. Now, pressing the
> button, the LED lights up, and the device can be removed.

[Maccraft123 (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Mimoja, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

mb/lenovo/x201: Add support for undock button on X200 ultrabase

Only the ACPI code needs to be extended, as smihandler.c already
supported it. The _Q50 Method is just _Q18 with changed name.

Currently, when pressing the undock button in Linux nothing happens,
and the device cannot be safely undocked. The only method to safely
undock device is to press key combination Fn+F9. Now, pressing the
button, the green LED lights up, and the device can be removed.

Change-Id: Iaaecad031bb1f39dd1a778d0c8eaea6bce9e0f57
Signed-off-by: Maciej Matuszczyk 
---
M src/mainboard/lenovo/x201/acpi/dock.asl
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/38446/3

[Angel Pons (Code Review)]

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 3: Code-Review+1

(4 comments)

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@7 
PS2, Line 7: mb/lenovo/x201: Add support for undock button on X200 ultrabase
> > mb/lenovo/x201: Support undock button on X200 ultrabase

Why? The current commit summary is using a correct tense. Is it that the summary is too long?

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@10 
PS2, Line 10: smihandler.c already supported it.
> One line please. Maybe: […]

Done

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@11 
PS2, Line 11:
> Please add the problem description. I have no idea, but something like: […]

Done, with a few remarks

https://review.coreboot.org/c/coreboot/+/38446/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38446/3//COMMIT_MSG@12 
PS3, Line 12: Currently, when pressing the undock button in Linux nothing happens,
            : and the device cannot be safely undocked. The only method to safely
            : undock device is to press key combination Fn+F9. Now, pressing the
            : button, the green LED lights up, and the device can be removed.
There's a few missing words:
> undock *the* device
> press *the* key combination
> Now, *when* pressing the

I guess this is because they aren't used in your native language? In any case, adding them would make the lines a biiit too long. My OCD decided to reword this paragraph so that it fits in three lines:
> On Linux, pressing the undock button does nothing, so the only safe way to undock is to press Fn+F9. With this patch, when the undock button is pressed, the green LED lights up, and undocking is safe.

[Angel Pons (Code Review)]

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 3:

Suggestion: Document the ACPI EC query methods. At least have a comment to indicate what they do. Of course, this would be done on a separate patch.

[Maccraft123 (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Mimoja, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

mb/lenovo/x201: Add support for undock button on X200 ultrabase

Only the ACPI code needs to be extended, as smihandler.c already
supported it. The _Q50 Method is just _Q18 with changed name.

On Linux, pressing the undock button does nothing, so the only safe
way to undock is to press Fn+F9. With this patch, when the undock
button is pressed, the green LED lights up, and undocking is safe.

Change-Id: Iaaecad031bb1f39dd1a778d0c8eaea6bce9e0f57
Signed-off-by: Maciej Matuszczyk 
---
M src/mainboard/lenovo/x201/acpi/dock.asl
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/38446/4

[Paul Menzel (Code Review)]

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

Change subject: mb/lenovo/x201: Add support for undock button on X200 ultrabase
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@7 
PS2, Line 7: mb/lenovo/x201: Add support for undock button on X200 ultrabase
> Why? The current commit summary is using a correct tense. […]

Yes, is correct, but a little long. Also I prefer to use *Support sth.* instead of *Add support for sth.*.

Change in coreboot[master]: mb/lenovo/x201: Support undock button on X200 ultrabase

[Maccraft123 (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Mimoja, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: mb/lenovo/x201: Support undock button on X200 ultrabase
......................................................................

mb/lenovo/x201: Support undock button on X200 ultrabase

Only the ACPI code needs to be extended, as smihandler.c already
supported it. The _Q50 Method is just _Q18 with changed name.

On Linux, pressing the undock button does nothing, so the only safe
way to undock is to press Fn+F9. With this patch, when the undock
button is pressed, the green LED lights up, and undocking is safe.

Change-Id: Iaaecad031bb1f39dd1a778d0c8eaea6bce9e0f57
Signed-off-by: Maciej Matuszczyk 
---
M src/mainboard/lenovo/x201/acpi/dock.asl
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/38446/5

Change in coreboot[master]: mb/lenovo/x201: Support undock button on X200 ultrabase

[Maccraft123 (Code Review)]

Maccraft123 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Support undock button on X200 ultrabase
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38446/2//COMMIT_MSG@7 
PS2, Line 7: mb/lenovo/x201: Add support for undock button on X200 ultrabase
> Yes, is correct, but a little long. Also I prefer to use *Support sth. […]

Done

Change in coreboot[master]: mb/lenovo/x201: Support undock button on X200 ultrabase

[Angel Pons (Code Review)]

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

Change subject: mb/lenovo/x201: Support undock button on X200 ultrabase
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38446/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38446/3//COMMIT_MSG@12 
PS3, Line 12: Currently, when pressing the undock button in Linux nothing happens,
            : and the device cannot be safely undocked. The only method to safely
            : undock device is to press key combination Fn+F9. Now, pressing the
            : button, the green LED lights up, and the device can be removed.
> There's a few missing words: […]

Done

Change in coreboot[master]: mb/lenovo/x201: Support undock button on X200 ultrabase

[Angel Pons (Code Review)]

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

Change subject: mb/lenovo/x201: Support undock button on X200 ultrabase
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: mb/lenovo/x201: Support undock button on X200 ultrabase

[Patrick Georgi (Code Review)]

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

Change subject: mb/lenovo/x201: Support undock button on X200 ultrabase
......................................................................

mb/lenovo/x201: Support undock button on X200 ultrabase

Only the ACPI code needs to be extended, as smihandler.c already
supported it. The _Q50 Method is just _Q18 with changed name.

On Linux, pressing the undock button does nothing, so the only safe
way to undock is to press Fn+F9. With this patch, when the undock
button is pressed, the green LED lights up, and undocking is safe.

Change-Id: Iaaecad031bb1f39dd1a778d0c8eaea6bce9e0f57
Signed-off-by: Maciej Matuszczyk 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38446
Reviewed-by: Angel Pons 
Reviewed-by: Mimoja 
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/lenovo/x201/acpi/dock.asl
1 file changed, 5 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Mimoja: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/mainboard/lenovo/x201/acpi/dock.asl b/src/mainboard/lenovo/x201/acpi/dock.asl
index 2bba821..b3381a7 100644
--- a/src/mainboard/lenovo/x201/acpi/dock.asl
+++ b/src/mainboard/lenovo/x201/acpi/dock.asl
@@ -60,6 +60,11 @@
    	Notify(_SB.DOCK, 3)
    }

+	Method(_Q50, 0, NotSerialized)
+	{
+		Notify(_SB.DOCK, 3)
+	}
+
    Method(_Q58, 0, NotSerialized)
    {
    	Notify(_SB.DOCK, 0)

Change in coreboot[master]: mb/lenovo/x201: Support undock button on X200 ultrabase

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38446 )

Change subject: mb/lenovo/x201: Support undock button on X200 ultrabase
......................................................................

Patch Set 6:

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/190
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/189
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/188

Please note: This test is under development and might not be accurate at all!