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:
(16 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81331/comment/a877ebc6_654d9e1f : PS2, Line 9: Support WACOM, ELAN, and generic touch sensor devices.
Please write an introduction what that new device it, and what driver is needed for it.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/56df2466_03a7183f : PS2, Line 10: This driver generates the following ACPI objects:
Please add a blank line between paragraphs.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/8d58126a_905eabae : PS2, Line 11: _DSM : _CRS : Power resource with _STA, _ON, and _OFF : _RST
Please format this as a list.
Done
Patchset:
PS2:
Separate the ELAN and WACOM support to another patch?
This driver is intended to support these two devices to reduce the needs of introducing all necessary parameters in the devicetree. These two headers are included in its touch.c and it makes sense to stay in single patchset:)
File src/drivers/intel/touch/chip.h:
PS2:
Mention the datasheet name somewhere in the file?
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/cd518487_5709fc94 : PS2, Line 29: Touch Host Controller Mode
THC's 3 protocol modes.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/b2713598_1f5259c6 : PS2, Line 30: Switch between Intel THC protocol and Industry standard HID Over SPI protocol
Switch among IPTS, HID-SPI and HID-I2C.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/5e5e61bc_782e9871 : PS2, Line 31: supporded
supported
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/cabf167d_593265d7 : PS2, Line 31: Thc
THC?
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/ad46e634_a98e70e3 : PS2, Line 31: :
Please add a space after the colon.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/e659abb1_1ce4167a : PS2, Line 33: supporded
supported
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/9422e825_86834af3 : PS2, Line 33: ;
THC HID I2C is for future support.
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/0f5f205d_0692affa : PS2, Line 29: /* Touch Host Controller Mode : Switch between Intel THC protocol and Industry standard HID Over SPI protocol. : 0x0:Thc IPTS (not supporded at this time) : 0x1:Thc HID SPI : 0x2 Thc HID I2C; (not supporded at this time) : */
I’d use […]
Done
https://review.coreboot.org/c/coreboot/+/81331/comment/f05946f9_0ab1a715 : PS2, Line 41: /* GPIO used to enable device. */
Redundant as the variable name says the same?
These parameters follow the GPIO ones in./src/soc/intel/common/block/pcie/rtd3/chip.h:
/* GPIO used to enable device. */ struct acpi_gpio enable_gpio; /* Delay to be inserted after device is enabled. */ unsigned int enable_delay_ms; /* Delay to be inserted after device is disabled. */ unsigned int enable_off_delay_ms;
/* GPIO used to take device out of reset or to put it into reset. */ struct acpi_gpio reset_gpio; /* Delay to be inserted after device is taken out of reset. */ unsigned int reset_delay_ms; /* Delay to be inserted after device is put into reset. */ unsigned int reset_off_delay_ms;
Other chip.h files also follow the same way.
https://review.coreboot.org/c/coreboot/+/81331/comment/8184a608_cf0aa052 : PS2, Line 46: enable_off
Why not disable?
same as the above, #41.
File src/drivers/intel/touch/elan.h:
https://review.coreboot.org/c/coreboot/+/81331/comment/50816769_de325758 : PS2, Line 9: #define ELAN_RST_SEQ_DLY 50
Is that time? Put the unit into the name?
Done