Change in coreboot[master]: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH
Hello Alex Levin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to review the following change.
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb 3 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/1
diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index 16834ae..8d5faef 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -1,6 +1,7 @@ config BOARD_GOOGLE_BASEBOARD_VOLTEER def_bool n select BOARD_ROMSIZE_KB_32768 + select DRIVERS_GENERIC_GPIO_KEYS select DRIVERS_GENERIC_MAX98357A select DRIVERS_I2C_GENERIC select DRIVERS_I2C_HID diff --git a/src/mainboard/google/volteer/variants/volteer/gpio.c b/src/mainboard/google/volteer/variants/volteer/gpio.c index 2b99e52..775bd68 100644 --- a/src/mainboard/google/volteer/variants/volteer/gpio.c +++ b/src/mainboard/google/volteer/variants/volteer/gpio.c @@ -30,7 +30,7 @@ PAD_CFG_NF(GPP_A23, NONE, DEEP, NF1),
/* B3 : CPU_GP2 ==> PEN_DET_ODL */ - PAD_CFG_GPI(GPP_B3, NONE, DEEP), + PAD_CFG_GPI_GPIO_DRIVER(GPP_B3, NONE, PLTRST), /* B5 : ISH_I2C0_CVF_SDA */ PAD_CFG_NF(GPP_B5, NONE, DEEP, NF1), /* B6 : ISH_I2C0_CVF_SCL */ @@ -98,7 +98,7 @@ PAD_CFG_GPO(GPP_D18, 1, DEEP),
/* E1 : SPI1_IO2 ==> PEN_DET_ODL */ - PAD_CFG_GPI_SCI_LOW(GPP_E1, NONE, DEEP, EDGE_SINGLE), + PAD_CFG_GPI_SCI(GPP_E1, NONE, DEEP, EDGE_SINGLE, NONE), /* E2 : SPI1_IO3 ==> WLAN_PCIE_WAKE_ODL */ PAD_CFG_GPI(GPP_E2, NONE, DEEP), /* E3 : CPU_GP0 ==> USI_REPORT_EN */ diff --git a/src/mainboard/google/volteer/variants/volteer/overridetree.cb b/src/mainboard/google/volteer/variants/volteer/overridetree.cb index 0944765..76a2b6d 100644 --- a/src/mainboard/google/volteer/variants/volteer/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer/overridetree.cb @@ -68,6 +68,18 @@ register "hid_desc_reg_offset" = "0x01" device i2c 10 on end end + chip drivers/generic/gpio_keys + register "name" = ""PENH"" + # GPP_B3 is the IRQ source, and GPP_E1 is the wake source + register "gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_B3)" + register "key.wake" = "GPE0_DW2_01" + 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 # I2C1 0xA0E9 device pci 15.2 on chip drivers/i2c/sx9310
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43764/1/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/gpio.c:
https://review.coreboot.org/c/coreboot/+/43764/1/src/mainboard/google/voltee... PS1, Line 33: PAD_CFG_GPI_GPIO_DRIVER(GPP_B3, NONE, PLTRST), code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43764/1/src/mainboard/google/voltee... PS1, Line 33: PAD_CFG_GPI_GPIO_DRIVER(GPP_B3, NONE, PLTRST), please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43764/1/src/mainboard/google/voltee... PS1, Line 101: PAD_CFG_GPI_SCI(GPP_E1, NONE, DEEP, EDGE_SINGLE, NONE), code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43764/1/src/mainboard/google/voltee... PS1, Line 101: PAD_CFG_GPI_SCI(GPP_E1, NONE, DEEP, EDGE_SINGLE, NONE), please, no spaces at the start of a line
Hello Furquan Shaikh, Duncan Laurie, Nick Vaccaro, Alex Levin, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to look at the new patch set (#2).
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb 3 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/2
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Nick Vaccaro, Alex Levin, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to look at the new patch set (#3).
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb 3 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/gpio.c:
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 33: PAD_CFG_GPI_GPIO_DRIVER I've usually used _GPI_APIC for this before, but I guess the kernel must know how to handle this? Do you get the stylus tools to pop up in the UI when you eject the pen?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 75: wake this was renamed to `wake_gpe`
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 76: register "key.wakeup_event_action" = "EV_ACT_DEASSERTED" Also since this signal is dual-routed, you'll want to set `register "key.wakeup_route" = "WAKEUP_ROUTE_SCI"` to indicate the SCI is the source for wakeups, so the ACPI data is exported correctly (it is the default, but it's nice to be explicit here).
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 71: chip drivers/generic/gpio_keys : register "name" = ""PENH"" : # GPP_B3 is the IRQ source, and GPP_E1 is the wake source : register "gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_B3)" : register "key.wake" = "GPE0_DW2_01" : 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 also tabs please 😄
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/gpio.c:
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 33: PAD_CFG_GPI_GPIO_DRIVER
I've usually used _GPI_APIC for this before, but I guess the kernel must know how to handle this? Do […]
well, verified interrupts are incrementing via /proc/interrupts
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 75: wake
this was renamed to `wake_gpe`
thanks. residue of working on older repo...
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 76: register "key.wakeup_event_action" = "EV_ACT_DEASSERTED"
Also since this signal is dual-routed, you'll want to set `register "key. […]
Ack
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 71: chip drivers/generic/gpio_keys : register "name" = ""PENH"" : # GPP_B3 is the IRQ source, and GPP_E1 is the wake source : register "gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_B3)" : register "key.wake" = "GPE0_DW2_01" : 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
also tabs please 😄
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/gpio.c:
https://review.coreboot.org/c/coreboot/+/43764/3/src/mainboard/google/voltee... PS3, Line 33: PAD_CFG_GPI_GPIO_DRIVER
well, verified interrupts are incrementing via /proc/interrupts
Oops my bad, I was remembering the pin wrong.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Nick Vaccaro, Alex Levin, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to look at the new patch set (#4).
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 4 files changed, 29 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 4:
You need DRIVERS_GENERIC_GPIO_KEYS for volteer2 as well
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 4:
Patch Set 4:
You need DRIVERS_GENERIC_GPIO_KEYS for volteer2 as well
right. thanks.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Nick Vaccaro, Alex Levin, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to look at the new patch set (#5).
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 4 files changed, 30 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH fun fact: linux ignores whatever you set this to and does what it wants (irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING) 😋
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG@7 PS5, Line 7: volteer mention volteer2 as well?
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
fun fact: linux ignores whatever you set this to and does what it wants (irqflags = IRQF_TRIGGER_RIS […]
lol, than we have this for "alternative" OSes?
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG@7 PS5, Line 7: volteer
mention volteer2 as well?
should I submit it in a separate patch for volteer2 to keep it clean?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
lol, than we have this for "alternative" OSes?
Well, the gpio is used to get notification of both eject and insert at runtime.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG@7 PS5, Line 7: volteer
should I submit it in a separate patch for volteer2 to keep it clean?
eh it's fine, they're the same thing. maybe: mb/google/volteer: Add gpio-keys ACPI node for pen eject
and then mention you added it to volteer & volteer2 here
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5: Code-Review+2
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43764/5//COMMIT_MSG@7 PS5, Line 7: volteer
eh it's fine, they're the same thing. […]
Ack
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
Well, the gpio is used to get notification of both eject and insert at runtime.
it does. but Tim says Linux ignores this setting.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer/variants/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
Well, the gpio is used to get notification of both eject and insert at runtime.
yeah, it's just confusing here when we have to select which trigger type it is. maybe there should be a DONTCARE macro too...
I think Aaron means linux does that because it assumes you want to detect both ejection & insertion at runtime.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Duncan Laurie, Nick Vaccaro, Alex Levin, Tim Wawrzynczak, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to look at the new patch set (#6).
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
Adding for both Volteer and Volteer2 variants.
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 4 files changed, 30 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/6/src/mainboard/google/voltee... File src/mainboard/google/volteer/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/43764/6/src/mainboard/google/voltee... PS6, Line 29: DRIVERS_GENERIC_GPIO_KEYS nit: It is also fine to just put this selection in Kconfig under BOARD_GOOGLE_BASEBOARD_VOLTEER as the gpio_keys driver will be optimized out by the linker if it is unused.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
yeah, it's just confusing here when we have to select which trigger type it is. […]
That's exactly what I meant. That said, I think the important bit is noting that falling or rising edge cooresponds to key.linux_code. And the wake through SCI pin corresponds to signal polarity for eject. Maybe we should have some encapsulating driver that for garaged stylus that conveys all the things we want to provide. Not for this change, but that way alleviate the confusion.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
That's exactly what I meant. […]
IIRC the ACPI GPIO layer uses this to do the filtering for the trigger so it does matter, just not at the raw irq level.
The pinctrl driver can also use it to set the pad up (if we didn't do this already).
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
IIRC the ACPI GPIO layer uses this to do the filtering for the trigger so it does matter, just not a […]
Aaron, that's a good idea. The garaged stylus keeps coming up, I might look into a little driver to smooth over gpio-keys for that. Duncan, where does the OSPM do that? I thought it just passed IRQ info to the device drivers to take care of
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
Aaron, that's a good idea. […]
Looks like I was out of date, gpiolib seems to use this field directly to setup the irqflags:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
Looks like I was out of date, gpiolib seems to use this field directly to setup the irqflags: […]
Thanks for the pointer. I don't think we would ever make it that far in that function though, there is no _Exx or _Lxx method nor an _EVT method for the GPIO controller, so it should bail on line 290 in that file instead? There is also no `_AEI` method indicating that OSPM is supposed to handle this event (via `_Exx` or `_Lxx`), so doesn't all of this info just trickle down to the device driver? Or am I misunderstanding section 5.6 of the ACPI spec?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
Thanks for the pointer. […]
So many layers can't keep it all straight, I think I was mixing another platform (apollolake?) where we had to do some funky routing of GPIO interrupts through _GPE.
I forgot that since this change also configures the pad as GPIO not ACPI and routes with GpioInt it should end up going through the pinmux layer to configure the pad to interrupt the GPIO controller on both edges: (which we are already doing in coreboot anyway) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43764/5/src/mainboard/google/voltee... PS5, Line 74: EDGE_BOTH
So many layers can't keep it all straight, I think I was mixing another platform (apollolake?) where […]
Now I'm going to have to instrument the kernel because I am not convinced either :)
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Tim Wawrzynczak, Nick Vaccaro, Alex Levin, Tim Wawrzynczak, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43764
to look at the new patch set (#7).
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
Adding for both Volteer and Volteer2 variants.
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org --- M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 4 files changed, 29 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/43764/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 7: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43764 )
Change subject: mb/google/volteer: Add gpio-keys ACPI node for PENH ......................................................................
mb/google/volteer: Add gpio-keys ACPI node for PENH
Use gpio_keys driver to add ACPI node for pen eject event. Also setting gpio wake pin for wake events.
Removal and insertion (both edges) triggers IRQ and only removal is a wake event (rising edge).
Adding for both Volteer and Volteer2 variants.
BUG=b:146083964 BRANCH=None TEST=tested on a Volteer
Change-Id: Ida3217a5b156320856ce3302c2623eba2230f28d Signed-off-by: Alex Levin levinale@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/43764 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/variants/volteer/gpio.c M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 4 files changed, 29 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index 16834ae..8d5faef 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -1,6 +1,7 @@ config BOARD_GOOGLE_BASEBOARD_VOLTEER def_bool n select BOARD_ROMSIZE_KB_32768 + select DRIVERS_GENERIC_GPIO_KEYS select DRIVERS_GENERIC_MAX98357A select DRIVERS_I2C_GENERIC select DRIVERS_I2C_HID diff --git a/src/mainboard/google/volteer/variants/volteer/gpio.c b/src/mainboard/google/volteer/variants/volteer/gpio.c index 2b99e52..9c36138 100644 --- a/src/mainboard/google/volteer/variants/volteer/gpio.c +++ b/src/mainboard/google/volteer/variants/volteer/gpio.c @@ -30,7 +30,7 @@ PAD_CFG_NF(GPP_A23, NONE, DEEP, NF1),
/* B3 : CPU_GP2 ==> PEN_DET_ODL */ - PAD_CFG_GPI(GPP_B3, NONE, DEEP), + PAD_CFG_GPI_GPIO_DRIVER(GPP_B3, NONE, PLTRST), /* B5 : ISH_I2C0_CVF_SDA */ PAD_CFG_NF(GPP_B5, NONE, DEEP, NF1), /* B6 : ISH_I2C0_CVF_SCL */ @@ -98,7 +98,7 @@ PAD_CFG_GPO(GPP_D18, 1, DEEP),
/* E1 : SPI1_IO2 ==> PEN_DET_ODL */ - PAD_CFG_GPI_SCI_LOW(GPP_E1, NONE, DEEP, EDGE_SINGLE), + PAD_CFG_GPI_SCI(GPP_E1, NONE, DEEP, EDGE_SINGLE, NONE), /* E2 : SPI1_IO3 ==> WLAN_PCIE_WAKE_ODL */ PAD_CFG_GPI(GPP_E2, NONE, DEEP), /* E3 : CPU_GP0 ==> USI_REPORT_EN */ diff --git a/src/mainboard/google/volteer/variants/volteer/overridetree.cb b/src/mainboard/google/volteer/variants/volteer/overridetree.cb index 0944765..54566f7 100644 --- a/src/mainboard/google/volteer/variants/volteer/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer/overridetree.cb @@ -68,6 +68,19 @@ register "hid_desc_reg_offset" = "0x01" device i2c 10 on end end + chip drivers/generic/gpio_keys + register "name" = ""PENH"" + # GPP_B3 is the IRQ source, and GPP_E1 is the wake source + register "gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_B3)" + register "key.wake_gpe" = "GPE0_DW2_01" + register "key.wakeup_route" = "WAKEUP_ROUTE_SCI" + 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 # I2C1 0xA0E9 device pci 15.2 on chip drivers/i2c/sx9310 diff --git a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb index c32b80e..9c78b94 100644 --- a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb @@ -67,6 +67,19 @@ register "hid_desc_reg_offset" = "0x01" device i2c 10 on end end + chip drivers/generic/gpio_keys + register "name" = ""PENH"" + # GPP_B3 is the IRQ source, and GPP_E1 is the wake source + register "gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_B3)" + register "key.wake_gpe" = "GPE0_DW2_01" + register "key.wakeup_route" = "WAKEUP_ROUTE_SCI" + 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 # I2C1 0xA0E9 device pci 15.2 on chip drivers/i2c/sx9310
-
Aaron Durbin (Code Review) -
Alex Levin (Code Review)
-
build bot (Jenkins) (Code Review)
-
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)