Attention is currently required from: Bora Guvendik, Cliff Huang, Intel coreboot Reviewers, Kyoung Il Kim, Paul Menzel, Subrata Banik.
Jérémy Compostella has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/85198?usp=email )
Change subject: drivers/intel/touch: Add Intel Touch Controller driver ......................................................................
Patch Set 14:
(2 comments)
Patchset:
PS14: I found several little commit message improvements:
``` THC is a hardware component that interfaces between a touch sensor and the system's SPI or I2C bus. This driver publishes data into the Secondary System Descriptor Table (SSDT).
This driver generates the following ACPI objects: - Device Specific Method (_DSM) - Current Resource Settings (_CRS) - Power Resource with Status (_STA), _ON, and _OFF methods - Device Specific Data (_DSD) for THC-I2C - Device Reset (_RST) for THC-SPI
Template device configuration for the following supported devices: - Wacom: THC-SPI touchscreen only - Elan: both THC-SPI and THC-I2C touchscreens - Hynitron: THC-I2C touchpad only
The configuration is split into device, SoC, and MB-specific categories. - SoC-specific: Implement soc_get_thc_hidi2c_info and soc_get_thc_hidspi_info functions for SoC-specific configurations. These can be placed in the SoC's chip.c file. - Device-specific: This driver provides the device-specific configuration for the supported devices; otherwise, it requires information via the device tree for unsupported/generic devices. - MB-specific: MB-specific details, such as the LTR value, need to be provided in the device tree.
The corrected version of the text would be:
BUG=none TEST=Select the DRIVERS_INTEL_TOUCH option on a motherboard with the necessary configurations, and verify that the THC ACPI tables are generated in the SSDT. ```
File src/drivers/intel/touch/elan.h:
https://review.coreboot.org/c/coreboot/+/85198/comment/2a4fae7c_9214cc9e?usp... : PS14, Line 33: .sensor_dev_name = ELAN_DEV_NAME, Could you remove the defines and move the values directly in the static constant ? I don't see any value adding such an indirection.