Hello SH Kim, Seunghwan Kim,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39602
to review the following change.
Change subject: mb/google/nightfury: Update overridetree.cb ......................................................................
mb/google/nightfury: Update overridetree.cb
Updating devicetree to enable ELAN touchpad and ELAN touchscreen on nightfury
BUG=none BRANCH=firmware-hatch-12672.B TEST=built and verified touchpad and touchscreen worked
Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Change-Id: Ieba6558ce3897ce2f95f51ed667465d84b4ab189 --- M src/mainboard/google/hatch/variants/nightfury/overridetree.cb 1 file changed, 21 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/39602/1
diff --git a/src/mainboard/google/hatch/variants/nightfury/overridetree.cb b/src/mainboard/google/hatch/variants/nightfury/overridetree.cb index 0cf18e7..6dc3d9f 100644 --- a/src/mainboard/google/hatch/variants/nightfury/overridetree.cb +++ b/src/mainboard/google/hatch/variants/nightfury/overridetree.cb @@ -199,47 +199,30 @@ end
device pci 15.0 on - chip drivers/i2c/hid - register "generic.hid" = ""PNP0C50"" - register "generic.desc" = ""Synaptics Touchpad"" - register "generic.irq" = "ACPI_IRQ_WAKE_EDGE_LOW(GPP_A21_IRQ)" - register "generic.probed" = "1" - register "generic.wake" = "GPE0_DW0_21" - register "hid_desc_reg_offset" = "0x20" - device i2c 0x20 on end - end + chip drivers/i2c/generic + register "hid" = ""ELAN0000"" + register "desc" = ""ELAN Touchpad"" + register "irq" = "ACPI_IRQ_WAKE_EDGE_LOW(GPP_A21_IRQ)" + register "probed" = "1" + register "wake" = "GPE0_DW0_21" + device i2c 0x15 on end + end end # I2C 0
device pci 15.1 on - chip drivers/i2c/generic - register "hid" = "ACPI_DT_NAMESPACE_HID" - register "compat_string" = ""atmel,maxtouch"" - register "desc" = ""Atmel Touchscreen"" - register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_D16_IRQ)" - register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" - register "reset_delay_ms" = "91" # 90.5 ms - register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C12)" - register "enable_delay_ms" = "1" # 90 ns - register "has_power_resource" = "1" - register "disable_gpio_export_in_crs" = "1" - register "probed" = "1" - device i2c 4b on end - end - - chip drivers/i2c/generic - register "hid" = ""ELAN0001"" - register "desc" = ""ELAN Touchscreen"" - register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_D16_IRQ)" - register "probed" = "1" - register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C12)" - register "enable_delay_ms" = "10" - register "enable_off_delay_ms" = "100" - register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" - register "reset_delay_ms" = "20" - register "reset_off_delay_ms" = "2" - register "has_power_resource" = "1" - device i2c 10 on end - end + chip drivers/i2c/hid + register "generic.hid" = ""ELAN902C"" + register "generic.desc" = ""ELAN Touchscreen"" + register "generic.irq" = "ACPI_IRQ_EDGE_LOW(GPP_D16_IRQ)" + register "generic.probed" = "1" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C12)" + register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" + register "generic.reset_delay_ms" = "20" + register "generic.has_power_resource" = "1" + register "generic.disable_gpio_export_in_crs" = "1" + register "hid_desc_reg_offset" = "0x01" + device i2c 0x10 on end + end end # I2C #1
device pci 15.2 off end # I2C #2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39602 )
Change subject: mb/google/nightfury: Update overridetree.cb ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39602/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/nightfury/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/39602/1/src/mainboard/google/hatch/... PS1, Line 214: ELAN902C I don't see this HID used in the 4.19 kernel anywhere... ELAN0001 is the only touchscreen HID that I see.
shkim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39602 )
Change subject: mb/google/nightfury: Update overridetree.cb ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39602/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/nightfury/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/39602/1/src/mainboard/google/hatch/... PS1, Line 214: ELAN902C
I don't see this HID used in the 4.19 kernel anywhere... […]
Following comment from ELAN is the reason why I set the HID to "ELAN902C". Should we use ELAN0001?
======================================================================= Terra project is using Elan-I2C driver for finger only. Terra-Q project is using standard HIDI2C driver for finger and pen functions. But Terra-Q is using Elan-I2C driver now, so please help to modify coreboot to HIDI2C interface. Below CL is other project for your reference, but need to modify gereric.hid from ELAN90FC(No Vendor) to ELAN902C(Samsung specific). CL link: https://review.coreboot.org/c/coreboot/+/38823/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39602 )
Change subject: mb/google/nightfury: Update overridetree.cb ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39602/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/nightfury/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/39602/1/src/mainboard/google/hatch/... PS1, Line 214: ELAN902C
Following comment from ELAN is the reason why I set the HID to "ELAN902C". […]
Ah, I understand, that driver uses _CID to bind instead of _HID. _HID is used for vendor-specific quirks.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39602 )
Change subject: mb/google/nightfury: Update overridetree.cb ......................................................................
mb/google/nightfury: Update overridetree.cb
Updating devicetree to enable ELAN touchpad and ELAN touchscreen on nightfury
BUG=none BRANCH=firmware-hatch-12672.B TEST=built and verified touchpad and touchscreen worked
Signed-off-by: Seunghwan Kim sh_.kim@samsung.com Change-Id: Ieba6558ce3897ce2f95f51ed667465d84b4ab189 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39602 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/hatch/variants/nightfury/overridetree.cb 1 file changed, 21 insertions(+), 38 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/nightfury/overridetree.cb b/src/mainboard/google/hatch/variants/nightfury/overridetree.cb index 0cf18e7..6dc3d9f 100644 --- a/src/mainboard/google/hatch/variants/nightfury/overridetree.cb +++ b/src/mainboard/google/hatch/variants/nightfury/overridetree.cb @@ -199,47 +199,30 @@ end
device pci 15.0 on - chip drivers/i2c/hid - register "generic.hid" = ""PNP0C50"" - register "generic.desc" = ""Synaptics Touchpad"" - register "generic.irq" = "ACPI_IRQ_WAKE_EDGE_LOW(GPP_A21_IRQ)" - register "generic.probed" = "1" - register "generic.wake" = "GPE0_DW0_21" - register "hid_desc_reg_offset" = "0x20" - device i2c 0x20 on end - end + chip drivers/i2c/generic + register "hid" = ""ELAN0000"" + register "desc" = ""ELAN Touchpad"" + register "irq" = "ACPI_IRQ_WAKE_EDGE_LOW(GPP_A21_IRQ)" + register "probed" = "1" + register "wake" = "GPE0_DW0_21" + device i2c 0x15 on end + end end # I2C 0
device pci 15.1 on - chip drivers/i2c/generic - register "hid" = "ACPI_DT_NAMESPACE_HID" - register "compat_string" = ""atmel,maxtouch"" - register "desc" = ""Atmel Touchscreen"" - register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_D16_IRQ)" - register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" - register "reset_delay_ms" = "91" # 90.5 ms - register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C12)" - register "enable_delay_ms" = "1" # 90 ns - register "has_power_resource" = "1" - register "disable_gpio_export_in_crs" = "1" - register "probed" = "1" - device i2c 4b on end - end - - chip drivers/i2c/generic - register "hid" = ""ELAN0001"" - register "desc" = ""ELAN Touchscreen"" - register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_D16_IRQ)" - register "probed" = "1" - register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C12)" - register "enable_delay_ms" = "10" - register "enable_off_delay_ms" = "100" - register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" - register "reset_delay_ms" = "20" - register "reset_off_delay_ms" = "2" - register "has_power_resource" = "1" - device i2c 10 on end - end + chip drivers/i2c/hid + register "generic.hid" = ""ELAN902C"" + register "generic.desc" = ""ELAN Touchscreen"" + register "generic.irq" = "ACPI_IRQ_EDGE_LOW(GPP_D16_IRQ)" + register "generic.probed" = "1" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C12)" + register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D15)" + register "generic.reset_delay_ms" = "20" + register "generic.has_power_resource" = "1" + register "generic.disable_gpio_export_in_crs" = "1" + register "hid_desc_reg_offset" = "0x01" + device i2c 0x10 on end + end end # I2C #1
device pci 15.2 off end # I2C #2
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39602 )
Change subject: mb/google/nightfury: Update overridetree.cb ......................................................................
Patch Set 2:
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/1474 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1473 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1472
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Patrick Georgi (Code Review)
-
shkim (Code Review)
-
Tim Wawrzynczak (Code Review)