Attention is currently required from: Eran Mitrani, Eric Lai, Jingyuan Liang, Kyoung Il Kim, Paul Menzel.
Cliff Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81331?usp=email )
Change subject: drivers/intel/touch: Add driver for Intel Touch Controller and Devices ......................................................................
Patch Set 4:
(18 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81331/comment/4a93609b_8c73e5e2 : PS4, Line 9: controller(THC)
Please add space before the (.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/b4a6f160_18643962 : PS4, Line 14: Meteorlake
Meteor Lake
Done
File src/drivers/intel/touch/chip.h:
https://review.coreboot.org/c/coreboot/+/81331/comment/28c1ad27_fb836536 : PS4, Line 11: MAX_TOUCH_PORTS
MAX_TOUCH_PORTS_PER_CONTROLLER?
The more precise comment should be: maximum number of ports that can be assigned to a THC controller
Actually, I am removing this define since it is not used.
https://review.coreboot.org/c/coreboot/+/81331/comment/7a9ebf19_785bbbd4 : PS4, Line 23: Meteorlake
Meteor Lake
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/793dbae7_011e3297 : PS4, Line 30: /* Touch Host Controller's (THC) 3 protocol modes: : * Switch among IPTS, HID-SPI and HID-I2C. : * 0x0: THC IPTS (not supported at this time) : * 0x1: THC HID SPI : * 0x2: THC HID I2C (for future support) : */
Blank line at the top. […]
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/37219483_4e5edca7 : PS4, Line 45: unsigned int enable_delay_ms;
Is there a better name? `delay_after_enabling_device_ms`?
I'd keep the same naming convention as the GPIO pad and delay naming in rtd3 chip.h:)
https://review.coreboot.org/c/coreboot/+/81331/comment/78852d3d_9d61337d : PS4, Line 57: rpt
I’d use report.
change rpt_ to report_ globally to this driver.
https://review.coreboot.org/c/coreboot/+/81331/comment/a8e29e7b_6ffe759d : PS4, Line 63: /* Touch Host Controller Wake On Touch : Based on this setting vGPIO for given THC will be in native mode, and additional : _CRS for wake will be exposed in ACPI : */
Please use the recommended commenting styles.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/4d7dc163_762744e1 : PS4, Line 70: Spi
SPI
Those comments are copied from FPS headers. I can make changes globally in this driver.
https://review.coreboot.org/c/coreboot/+/81331/comment/1a95d577_a7d21484 : PS4, Line 71: this
This
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/97e38b24_377cb605 : PS4, Line 90: /* Touch Host Controller Hid Over Spi Connection Speed : Hid Over Spi Connection Speed - SPI Frequency : */
Ditto. (Also below. […]
Done
File src/drivers/intel/touch/elan.h:
https://review.coreboot.org/c/coreboot/+/81331/comment/d5c01195_ec3caab4 : PS4, Line 16: // #define ELAN_CONNECTION_SPEED 17000000
Why commented out?
changed to comment.
File src/drivers/intel/touch/touch.c:
https://review.coreboot.org/c/coreboot/+/81331/comment/9a6d1b56_f88a169c : PS4, Line 30: in rpt hdr addr
Spell it out?
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/37da391a_209a116f : PS4, Line 65: "Use TH_SENSOR_GENERIC for parameters from the devicetree!\n");
Indent more?
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/6a3db611_e9115db3 : PS4, Line 106: printk(BIOS_INFO, "use WACOM Write Opcode\n");
Why only the info message for the first case? What should the user do with this information?
also removed the other debug prints for WACOM.
https://review.coreboot.org/c/coreboot/+/81331/comment/abd0499e_001b7993 : PS4, Line 526: static void : touch_generate_acpi_method_on(
Why not one line?
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/23f983ee_6315c612 : PS4, Line 719: .read_resources = pci_dev_read_resources, : .set_resources = pci_dev_set_resources, : .enable_resources = pci_dev_enable_resources, : .init = pci_dev_init, : .scan_bus = &scan_generic_bus, /* Non-default */ : .ops_pci = &pci_dev_ops_pci,
Use tabs for aligning = like below?
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/e84f8d17_ac91357a : PS4, Line 743:
Tab?
Done