Attention is currently required from: Cliff Huang, Eran Mitrani, Eric Lai, Jingyuan Liang, Kyoung Il Kim.
Paul Menzel 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/21b7fd22_d07dd5a5 : PS4, Line 9: controller(THC) Please add space before the (.
https://review.coreboot.org/c/coreboot/+/81331/comment/2e07ac77_0a81f556 : PS4, Line 14: Meteorlake Meteor Lake
File src/drivers/intel/touch/chip.h:
https://review.coreboot.org/c/coreboot/+/81331/comment/6079db90_272f9425 : PS4, Line 11: MAX_TOUCH_PORTS MAX_TOUCH_PORTS_PER_CONTROLLER?
https://review.coreboot.org/c/coreboot/+/81331/comment/e2fad248_8d42d2e5 : PS4, Line 23: Meteorlake Meteor Lake
https://review.coreboot.org/c/coreboot/+/81331/comment/2829eceb_c5b5cfd3 : 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.
/* * […] * /
https://review.coreboot.org/c/coreboot/+/81331/comment/01bfc2aa_f6bddc00 : PS4, Line 45: unsigned int enable_delay_ms; Is there a better name? `delay_after_enabling_device_ms`?
https://review.coreboot.org/c/coreboot/+/81331/comment/2f5595f1_7ce9040b : PS4, Line 57: rpt I’d use report.
https://review.coreboot.org/c/coreboot/+/81331/comment/1e5a691a_d05ef1fb : 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.
https://review.coreboot.org/c/coreboot/+/81331/comment/d2af3da3_44bb5423 : PS4, Line 70: Spi SPI
https://review.coreboot.org/c/coreboot/+/81331/comment/5063f253_fe7876c8 : PS4, Line 71: this This
https://review.coreboot.org/c/coreboot/+/81331/comment/c4ddea38_85023cbc : PS4, Line 90: /* Touch Host Controller Hid Over Spi Connection Speed : Hid Over Spi Connection Speed - SPI Frequency : */ Ditto. (Also below.)
File src/drivers/intel/touch/elan.h:
https://review.coreboot.org/c/coreboot/+/81331/comment/0daab7da_af039e1e : PS4, Line 16: // #define ELAN_CONNECTION_SPEED 17000000 Why commented out?
File src/drivers/intel/touch/touch.c:
https://review.coreboot.org/c/coreboot/+/81331/comment/d20b2927_17518b75 : PS4, Line 30: in rpt hdr addr Spell it out?
https://review.coreboot.org/c/coreboot/+/81331/comment/6fb5515f_8cdc2f2a : PS4, Line 65: "Use TH_SENSOR_GENERIC for parameters from the devicetree!\n"); Indent more?
https://review.coreboot.org/c/coreboot/+/81331/comment/57643706_98188594 : 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?
https://review.coreboot.org/c/coreboot/+/81331/comment/e3b5ab55_7251018d : PS4, Line 526: static void : touch_generate_acpi_method_on( Why not one line?
https://review.coreboot.org/c/coreboot/+/81331/comment/c1c41ed5_9de8e41b : 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?
https://review.coreboot.org/c/coreboot/+/81331/comment/9e9501e0_0800e0a9 : PS4, Line 743: Tab?