Hello Seunghwan Kim,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34581
to review the following change.
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen detection event.
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen detection event
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 23 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/1
diff --git a/src/mainboard/google/hatch/variants/kohaku/gpio.c b/src/mainboard/google/hatch/variants/kohaku/gpio.c index 88c1d7b..a5ac068 100644 --- a/src/mainboard/google/hatch/variants/kohaku/gpio.c +++ b/src/mainboard/google/hatch/variants/kohaku/gpio.c @@ -24,7 +24,7 @@ /* A6 : SERIRQ ==> NC */ PAD_NC(GPP_A6, NONE), /* A10 : PEN_RESET_ODL */ - PAD_CFG_GPO(GPP_A10, 0, DEEP), + PAD_CFG_GPO(GPP_A10, 1, DEEP), /* A17 : PIRQA# ==> NC */ PAD_NC(GPP_A17, NONE), /* A18 : ISH_GP0 ==> NC */ @@ -39,15 +39,9 @@ PAD_NC(GPP_B8, NONE), /* C1 : SMBDATA: NC */ PAD_NC(GPP_C1, NONE), - /* - * C12 : EMR_GARAGE_INT - * The same signal is routed to both A8 and C12. Currently C12 - * is the interrupt source, and A8 is the wake source. - * Hoping that GPP_A8 can be used for both interrupt (SCI) and wake - * (GPIO). Keeping as GPI for now. - */ - PAD_CFG_GPI_SCI(GPP_C12, NONE, DEEP, EDGE_SINGLE, INVERT), - /* C15 : EN_PP3300_TSP_DIG_DX */ + /* C7 : PEN_IRQ_OD_L */ + PAD_CFG_GPI_APIC(GPP_C7, NONE, PLTRST, LEVEL, NONE), + /* C15 : EN_PP3300_DIG_DX */ PAD_CFG_GPO(GPP_C15, 0, DEEP), /* C23 : UART2_CTS# ==> NC */ PAD_NC(GPP_C23, NONE), @@ -69,6 +63,10 @@ PAD_NC(GPP_G5, NONE), /* G6 : GPP_G6 ==> NC */ PAD_NC(GPP_G6, NONE), + /* H4 : PCH_I2C_PEN_SDA */ + PAD_CFG_NF(GPP_H4, NONE, DEEP, NF1), + /* H5 : PCH_I2C_PEN_SCL */ + PAD_CFG_NF(GPP_H5, NONE, DEEP, NF1), };
const struct pad_config *override_gpio_table(size_t *num) diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb index e463b8b..f3055a1 100644 --- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb +++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb @@ -77,12 +77,25 @@ register "generic.hid" = ""WCOM50C1"" register "generic.desc" = ""WCOM Digitizer"" register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_C7_IRQ)" - register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A10)" - register "generic.reset_delay_ms" = "1" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C15)" + # TODO: Check if reset_gpio is needed for power sequence of pen device + #register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A10)" + #register "generic.reset_delay_ms" = "1" register "generic.has_power_resource" = "1" register "hid_desc_reg_offset" = "0x1" device i2c 0x09 on end end + chip drivers/generic/gpio_keys + register "name" = ""PENH"" + register "gpio" = "ACPI_GPIO_IRQ_LEVEL_LOW(GPP_A8)" + register "key.wake" = "GPE0_DW0_08" + register "key.wakeup_event_action" = "EV_ACT_DEASSERTED" + register "key.dev_name" = ""EJCT"" + register "key.linux_code" = "SW_PEN_INSERTED" + register "key.linux_input_type" = "EV_SW" + register "key.label" = ""pen_eject"" + device generic 0 on end + end end # I2C #2 device pci 19.0 on chip drivers/i2c/da7219
Hello Seunghwan Kim, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34581
to look at the new patch set (#2).
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event.
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 23 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... PS2, Line 27: 1 I would keep this as zero and let the kernel do the power sequencing.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... PS2, Line 27: 1
I would keep this as zero and let the kernel do the power sequencing.
On testing that, keeping this as 0 and enabling the reset_gpio doesn't seem to work...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@9 PS1, Line 9: Enabling stylus pen device and pen detection event. Please describe the changes you are marking as part of this CL.
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@10 PS1, Line 10: What about pen eject? You added gpio_keys node, but no mention about the change.
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@13 PS1, Line 13: TEST=Verified pen input operation and pen detection event Did pen eject work to: 1. Show the stylus tools on eject 2. Hide the stylus tools on insert 3. Wake from pen eject from S0ix and S3 (Did the eventlog have right information?) 4. No wake by pen insert from S0ix and S3.
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 49: Why was this removed? Is it not required anymore?
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 27: 1 Note: This should be set to 0 if you figure out that the reset GPIO needs to be part of power sequencing in ACPI power resource.
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 43: NONE INVERT
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... PS2, Line 27: 1
On testing that, keeping this as 0 and enabling the reset_gpio doesn't seem to work...
Yeah, it is being tracked at b/137326841 to understand what the real power-on sequencing requirement is w.r.t. reset GPIO.
SH Kim has uploaded a new patch set (#3) to the change originally created by shkim. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event.
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 20 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/3
SH Kim has uploaded a new patch set (#4) to the change originally created by shkim. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event. - Adding enable_gpio for power sequencing - Configuring GPP_H4 and GPP_H5 as native function - Adding PENH device node for pen ejection event
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event (pop-up and wake from s0ix on pen ejection)
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 20 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/4
SH Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@13 PS1, Line 13: TEST=Verified pen input operation and pen detection event
Did pen eject work to: […]
1. Show the stylus tools on eject ; Yes 2. Hide the stylus tools on insert ; Yes 3. Wake from pen eject from S0ix and S3 (Did the eventlog have right information?) ; Yes. (Tested on s0ix) But there is no wake source in eventlog. Should we have the wake source in event log? 4. No wake by pen insert from S0ix and S3. ; Yes. (tested on s0ix)
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 49:
Why was this removed? Is it not required anymore?
It was routed to EMR_GARAGE_INT before, but not used. GPP_C12 would be routed to EN_PP3300_TSP_DX in latest schematics, we need to configure it in another CL for enabling TSP device.
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 27: 1
Note: This should be set to 0 if you figure out that the reset GPIO needs to be part of power sequen […]
As our device charger checked with wacom, they said we don't need triggering reset for power sequencing of the IC. When the IC powered on, reset would be triggered internally. So I think we can keep this signal as high or route to always on power.
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 43: NONE
INVERT
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@13 PS1, Line 13: TEST=Verified pen input operation and pen detection event
- Show the stylus tools on eject […]
Re#3, the wake source does not currently show up, this is a known issue.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW Shouldn't this be ACPI_GPIO_INPUT_ACTIVE_LOW?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
Shouldn't this be ACPI_GPIO_INPUT_ACTIVE_LOW?
ACPI_GPIO_IRQ_EDGE_BOTH? Don't we need an IRQ on both edges for stylus toolbar show/hide?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 66: /* H4 : PCH_I2C_PEN_SDA */ : PAD_CFG_NF(GPP_H4, NONE, DEEP, NF1), : /* H5 : PCH_I2C_PEN_SCL */ : PAD_CFG_NF(GPP_H5, NONE, DEEP, NF1), : This is fine, just a note that I'm pretty sure FSP is doing this same thing; these are set as PAD_NC in the hatch board, but we're still able to communicate with the digitizer IC.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 66: /* H4 : PCH_I2C_PEN_SDA */ : PAD_CFG_NF(GPP_H4, NONE, DEEP, NF1), : /* H5 : PCH_I2C_PEN_SCL */ : PAD_CFG_NF(GPP_H5, NONE, DEEP, NF1), :
This is fine, just a note that I'm pretty sure FSP is doing this same thing; these are set as PAD_NC […]
Yeah, I believe it is based on the SerialIoDevConfig.
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
ACPI_GPIO_IRQ_EDGE_BOTH? Don't we need an IRQ on both edges for stylus toolbar show/hide?
Based on the name of the member, it looks like we are just sharing information about the GpioIO() in the _CRS and gpio_keys kernel driver uses that to request the right IRQ. It would be good to check what exactly the expectation is.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
Based on the name of the member, it looks like we are just sharing information about the GpioIO() in […]
You're right, of course, but consistency between the different overrides is helpful. Either leave this one as is and push a change for hatch/overridetree as well, or change this one to match the other.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
You're right, of course, but consistency between the different overrides is helpful. […]
If the expectation is GpioIO() to be provided, then we should change this and the hatch/overridetree as well.
SH Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 66: /* H4 : PCH_I2C_PEN_SDA */ : PAD_CFG_NF(GPP_H4, NONE, DEEP, NF1), : /* H5 : PCH_I2C_PEN_SCL */ : PAD_CFG_NF(GPP_H5, NONE, DEEP, NF1), :
Yeah, I believe it is based on the SerialIoDevConfig.
Thanks for the explanation.
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
If the expectation is GpioIO() to be provided, then we should change this and the hatch/overridetree […]
ACPI_GPIO_IRQ_EDGE_BOTH makes reverse pen ejection event action. Insert - Display pen pop-up window and cause wake from suspend Eject - Hide pop-up window
Both ACPI_GPIO_IRQ_LEVEL_LOW and ACPI_GPIO_INPUT_ACTIVE_LOW are okay for the pen ejection event - pop-up window and wake operation. Can you check what we should use?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
ACPI_GPIO_IRQ_EDGE_BOTH makes reverse pen ejection event action. […]
ACPI_GPIO_IRQ_EDGE_BOTH makes reverse pen ejection event action. Insert - Display pen pop-up window and cause wake from suspend Eject - Hide pop-up window
That seems to be the opposite of the behavior we want. We should wake only when the pen is eject from the garage. Also the Stylus options should pop-up only when the pen is eject. ACPI_GPIO_INPUT_ACTIVE_LOW should meet the requirements.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
ACPI_GPIO_IRQ_EDGE_BOTH makes reverse pen ejection event action. […]
The kernel driver is written more from the perspective of a devicetree implementation, and doesn't seem to care if it's a GpioIo or GpioInterrupt. Since the kernel will grab the IRQ from the gpio_chip device, I would say let the kernel do it. ACPI_GPIO_INPUT_ACTIVE_LOW would be fine. Let's go ahead with this one, and I'll push a change to the other boards to switch to that.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 87: ACPI_GPIO_IRQ_LEVEL_LOW
The kernel driver is written more from the perspective of a devicetree implementation, and doesn't s […]
Just couple of comments: 1. As long as the kernel is capable of handling GpioIO and GpioInterrupt, I think its fine to choose one and go with that. But this should be correctly documented (probably in gpio_keys.h) 2. If we decide to go with GPIO_IRQ, I think it would be better to update gpio_keys driver to rename gpio to gpio_irq and handle it appropriately in gpio_keys.c
These can be done as follow-up changes.
Tim Wawrzynczak has uploaded a new patch set (#5) to the change originally created by shkim. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event. - Adding enable_gpio for power sequencing - Configuring GPP_H4 and GPP_H5 as native function - Adding PENH device node for pen ejection event
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event (pop-up and wake from s0ix on pen ejection)
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 19 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/5
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 5: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/5/src/mainboard/google/hatch/... PS5, Line 89: reset_gpio If you are planning to add reset gpio to power resource, then it does not need to be set to 1 in gpio.c
Hello Karthik Ramasubramanian, Seunghwan Kim, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34581
to look at the new patch set (#6).
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event. - Adding enable_gpio for power sequencing - Configuring GPP_H4 and GPP_H5 as native function - Adding PENH device node for pen ejection event
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event (pop-up and wake from s0ix on pen ejection)
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 24 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/6
shkim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/5/src/mainboard/google/hatch/... PS5, Line 89: reset_gpio
If you are planning to add reset gpio to power resource, then it does not need to be set to 1 in gpi […]
We cannot use GPP_A10 as reset_gpio (b/137326841#comment86) so need further discussion. How about remove reset_gpio right now? Or we can hold this CL until we get decision about this.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/6/src/mainboard/google/hatch/... PS6, Line 91: # need to reassign it or remove it. trailing whitespace
Hello Karthik Ramasubramanian, Seunghwan Kim, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34581
to look at the new patch set (#7).
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event. - Adding enable_gpio for power sequencing - Configuring GPP_H4 and GPP_H5 as native function - Adding PENH device node for pen ejection event
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event (pop-up and wake from s0ix on pen ejection)
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 24 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/7/src/mainboard/google/hatch/... PS7, Line 91: # we need to reassign it or remove it. trailing whitespace
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/7/src/mainboard/google/hatch/... PS7, Line 91: # we need to reassign it or remove it.
trailing whitespace
You need to get rid of the trailing whitespace.
SH Kim has uploaded a new patch set (#8) to the change originally created by shkim. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event. - Adding enable_gpio for power sequencing - Configuring GPP_H4 and GPP_H5 as native function - Adding PENH device node for pen ejection event
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event (pop-up and wake from s0ix on pen ejection)
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 24 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34581/8
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 8: Code-Review+2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 8: Code-Review+2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 8:
(7 comments)
Resolving all the comments.
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@9 PS1, Line 9: Enabling stylus pen device and pen detection event.
Please describe the changes you are marking as part of this CL.
Done
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@10 PS1, Line 10:
What about pen eject? You added gpio_keys node, but no mention about the change.
Done
https://review.coreboot.org/c/coreboot/+/34581/1//COMMIT_MSG@13 PS1, Line 13: TEST=Verified pen input operation and pen detection event
Re#3, the wake source does not currently show up, this is a known issue.
Done
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 49:
It was routed to EMR_GARAGE_INT before, but not used. […]
Done
https://review.coreboot.org/c/coreboot/+/34581/1/src/mainboard/google/hatch/... PS1, Line 27: 1
As our device charger checked with wacom, they said we don't need triggering reset for power sequenc […]
Done
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/2/src/mainboard/google/hatch/... PS2, Line 27: 1
Yeah, it is being tracked at b/137326841 to understand what the real power-on sequencing requirement […]
Done
https://review.coreboot.org/c/coreboot/+/34581/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/5/src/mainboard/google/hatch/... PS5, Line 89: reset_gpio
We cannot use GPP_A10 as reset_gpio (b/137326841#comment86) so need further discussion. […]
Done
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/34581/7/src/mainboard/google/hatch/... PS7, Line 91: # we need to reassign it or remove it.
You need to get rid of the trailing whitespace.
Done
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/34581/4/src/mainboard/google/hatch/... PS4, Line 66: /* H4 : PCH_I2C_PEN_SDA */ : PAD_CFG_NF(GPP_H4, NONE, DEEP, NF1), : /* H5 : PCH_I2C_PEN_SCL */ : PAD_CFG_NF(GPP_H5, NONE, DEEP, NF1), :
Thanks for the explanation.
Done
Shelley Chen has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34581 )
Change subject: mb/google/kohaku: Enable stylus pen device ......................................................................
mb/google/kohaku: Enable stylus pen device
Enabling stylus pen device and pen_eject event. - Adding enable_gpio for power sequencing - Configuring GPP_H4 and GPP_H5 as native function - Adding PENH device node for pen ejection event
BUG=b:137326841 BRANCH=none TEST=Verified pen input operation and pen_eject event (pop-up and wake from s0ix on pen ejection)
Change-Id: Ic252a1f90c0fc6cb9b1e426d75a8b503824681f3 Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/34581 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Paul Fagerburg pfagerburg@chromium.org --- M src/mainboard/google/hatch/variants/kohaku/gpio.c M src/mainboard/google/hatch/variants/kohaku/overridetree.cb 2 files changed, 24 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Paul Fagerburg: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/kohaku/gpio.c b/src/mainboard/google/hatch/variants/kohaku/gpio.c index 88c1d7b..d22de04 100644 --- a/src/mainboard/google/hatch/variants/kohaku/gpio.c +++ b/src/mainboard/google/hatch/variants/kohaku/gpio.c @@ -24,7 +24,7 @@ /* A6 : SERIRQ ==> NC */ PAD_NC(GPP_A6, NONE), /* A10 : PEN_RESET_ODL */ - PAD_CFG_GPO(GPP_A10, 0, DEEP), + PAD_CFG_GPO(GPP_A10, 1, DEEP), /* A17 : PIRQA# ==> NC */ PAD_NC(GPP_A17, NONE), /* A18 : ISH_GP0 ==> NC */ @@ -39,15 +39,9 @@ PAD_NC(GPP_B8, NONE), /* C1 : SMBDATA: NC */ PAD_NC(GPP_C1, NONE), - /* - * C12 : EMR_GARAGE_INT - * The same signal is routed to both A8 and C12. Currently C12 - * is the interrupt source, and A8 is the wake source. - * Hoping that GPP_A8 can be used for both interrupt (SCI) and wake - * (GPIO). Keeping as GPI for now. - */ - PAD_CFG_GPI_SCI(GPP_C12, NONE, DEEP, EDGE_SINGLE, INVERT), - /* C15 : EN_PP3300_TSP_DIG_DX */ + /* C7 : PEN_IRQ_OD_L */ + PAD_CFG_GPI_APIC(GPP_C7, NONE, PLTRST, LEVEL, INVERT), + /* C15 : EN_PP3300_DIG_DX */ PAD_CFG_GPO(GPP_C15, 0, DEEP), /* C23 : UART2_CTS# ==> NC */ PAD_NC(GPP_C23, NONE), @@ -69,6 +63,10 @@ PAD_NC(GPP_G5, NONE), /* G6 : GPP_G6 ==> NC */ PAD_NC(GPP_G6, NONE), + /* H4 : PCH_I2C_PEN_SDA */ + PAD_CFG_NF(GPP_H4, NONE, DEEP, NF1), + /* H5 : PCH_I2C_PEN_SCL */ + PAD_CFG_NF(GPP_H5, NONE, DEEP, NF1), };
const struct pad_config *override_gpio_table(size_t *num) diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb index b3ae1bc..8a1de84 100644 --- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb +++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb @@ -86,12 +86,26 @@ register "generic.hid" = ""WCOM50C1"" register "generic.desc" = ""WCOM Digitizer"" register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_C7_IRQ)" - register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A10)" - register "generic.reset_delay_ms" = "1" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C15)" + # TODO: We can't use GPP_A10 as reset_gpio due to its voltage level, + # so we need to reassign it or remove it. + #register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A10)" + #register "generic.reset_delay_ms" = "1" register "generic.has_power_resource" = "1" register "hid_desc_reg_offset" = "0x1" device i2c 0x09 on end end + chip drivers/generic/gpio_keys + register "name" = ""PENH"" + register "gpio" = "ACPI_GPIO_INPUT_ACTIVE_LOW(GPP_A8)" + register "key.wake" = "GPE0_DW0_08" + register "key.wakeup_event_action" = "EV_ACT_DEASSERTED" + register "key.dev_name" = ""EJCT"" + register "key.linux_code" = "SW_PEN_INSERTED" + register "key.linux_input_type" = "EV_SW" + register "key.label" = ""pen_eject"" + device generic 0 on end + end end # I2C #2 device pci 19.0 on chip drivers/i2c/da7219
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Paul Fagerburg (Code Review)
-
SH Kim (Code Review)
-
Shelley Chen (Code Review)
-
shkim (Code Review)
-
Tim Wawrzynczak (Code Review)