* [PATCH v4 1/5] HID: intel-thc-hid: intel-ipts: bring up the port
2026-11-14 21:40 [PATCH v4 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports Emfox Zhou
@ 2026-11-14 21:40 ` Emfox Zhou
2026-11-14 21:40 ` [PATCH v4 2/5] HID: intel-thc-hid: intel-ipts: receive reports Emfox Zhou
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Emfox Zhou @ 2026-11-14 21:40 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
Ports on Tiger Lake, Lakefield, Alder Lake and Raptor Lake can run in
IPTS mode: instead of HIDSPI the port drives the touch controller over
plain SPI, and the controller answers with a 64 byte configuration
block at address zero that carries the vendor and product IDs, the
buffer sizes and the SPI timing it expects.
Add a port driver that does that much: select the SPI port, release the
controller from reset, read the configuration block over the existing
PIO path, apply the timing the controller asked for and enable the
touch engine. What the controller reports is printed, and removal puts
it back into reset.
Three things are not obvious from the register names:
- The bulk data window is at address 0x1000, not at zero. It is set
here because the DMA engines write there; left unset they write into
the configuration block and the controller drops the frames without
reporting an error.
- Quiescing gates the whole data path, not only the interrupt. The
port stays quiesced while the configuration block is read and the
timing is applied, and is only released once that is done.
- The edge detect bit belongs to the interrupt setup, but is set
here: the first configuration read is unreliable without it.
Meteor Lake and newer IDs are deliberately absent: those ports are
already claimed by the HIDSPI driver.
Signed-off-by: Emfox Zhou <emfox@debian.org>
---
drivers/hid/intel-thc-hid/Kconfig | 14 ++
drivers/hid/intel-thc-hid/Makefile | 4 +
drivers/hid/intel-thc-hid/intel-ipts/ipts.c | 237 ++++++++++++++++++
drivers/hid/intel-thc-hid/intel-ipts/ipts.h | 132 ++++++++++
.../hid/intel-thc-hid/intel-ipts/pci-ipts.c | 125 +++++++++
5 files changed, 512 insertions(+)
create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/ipts.c
create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/ipts.h
create mode 100644 drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
diff --git a/drivers/hid/intel-thc-hid/Kconfig b/drivers/hid/intel-thc-hid/Kconfig
index 9d74e53b..3df645f3 100644
--- a/drivers/hid/intel-thc-hid/Kconfig
+++ b/drivers/hid/intel-thc-hid/Kconfig
@@ -40,4 +40,18 @@ config INTEL_QUICKI2C
Say Y/M here if you want to support Intel QuickI2C. If unsure, say N.
+config INTEL_IPTS
+ tristate "Intel Touch Host Controller IPTS mode"
+ depends on INTEL_THC_HID
+ help
+ Support for THC ports in IPTS mode. Instead of HIDSPI these ports
+ drive the touch controller over plain SPI and carry its reports in a
+ private framing. This driver brings such a port up, reads the
+ controller's configuration block and exposes the reports as a HID
+ device, so that a decoder in user space can turn them into input
+ events.
+
+ Say Y/M here if you have an Intel touch controller that runs in IPTS
+ mode. If unsure, say N.
+
endmenu
diff --git a/drivers/hid/intel-thc-hid/Makefile b/drivers/hid/intel-thc-hid/Makefile
index f1182253..04900959 100644
--- a/drivers/hid/intel-thc-hid/Makefile
+++ b/drivers/hid/intel-thc-hid/Makefile
@@ -20,4 +20,8 @@ intel-quicki2c-objs += intel-quicki2c/pci-quicki2c.o
intel-quicki2c-objs += intel-quicki2c/quicki2c-hid.o
intel-quicki2c-objs += intel-quicki2c/quicki2c-protocol.o
+obj-$(CONFIG_INTEL_IPTS) += intel-ipts.o
+intel-ipts-objs += intel-ipts/pci-ipts.o
+intel-ipts-objs += intel-ipts/ipts.o
+
ccflags-y += -I $(src)/intel-thc
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
new file mode 100644
index 00000000..205dc51b
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -0,0 +1,237 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Transport for THC ports in IPTS mode: the controller is driven over plain
+ * SPI and carries its HID reports in a private framing.
+ */
+
+#include <linux/delay.h>
+#include <linux/math.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include "intel-thc-hw.h"
+#include "ipts.h"
+
+/* Bulk transfers have no address phase, so the SPI opcode is zero. */
+#define THC_IPTS_SPI_OPCODE 0
+
+/* Slowest clock the core can program, used until the controller is read. */
+#define THC_IPTS_BOOT_SPI_HZ THC_IPTS_SPI_MIN_HZ
+
+/* Time the controller needs after reset before it answers on the bus. */
+#define THC_IPTS_BOOT_MS 100
+
+static int thc_ipts_read_cfg(struct thc_ipts *idev)
+{
+ int ret, try;
+
+ for (try = 0; try < THC_IPTS_CFG_TRIES; try++) {
+ u32 got = 0;
+
+ memset(&idev->cfg, 0, sizeof(idev->cfg));
+ ret = thc_tic_pio_read(idev->thc, 0, THC_IPTS_CFG_SIZE, &got,
+ (u32 *)&idev->cfg);
+ if (!ret && got == THC_IPTS_CFG_SIZE &&
+ le32_to_cpu(idev->cfg.device_id) == THC_IPTS_TIC_ID)
+ return 0;
+
+ if (try + 1 < THC_IPTS_CFG_TRIES)
+ msleep(THC_IPTS_CFG_RETRY_MS);
+ }
+
+ dev_err(idev->dev,
+ "controller did not answer, last id 0x%08x, error %d\n",
+ le32_to_cpu(idev->cfg.device_id), ret ?: -EIO);
+
+ return ret ?: -EIO;
+}
+
+/*
+ * The core warns when the port is already in the requested state, so sample
+ * the bit first; it is a plain register bit and safe to read here.
+ */
+static int thc_ipts_quiesce(struct thc_ipts *idev, bool quiesce)
+{
+ u32 ctrl;
+ int ret;
+
+ ret = regmap_read(idev->thc->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
+ &ctrl);
+ if (ret)
+ return ret;
+
+ if (!!(ctrl & THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_EN) == quiesce)
+ return 0;
+
+ return thc_interrupt_quiesce(idev->thc, quiesce);
+}
+
+static int thc_ipts_spi_set(struct thc_ipts *idev, u32 hz, u32 mode)
+{
+ int ret;
+
+ /* No multi-packet transfers in this mode, so both sizes stay zero. */
+ ret = thc_spi_read_config(idev->thc, hz, mode, THC_IPTS_SPI_OPCODE, 0);
+ if (ret)
+ return ret;
+
+ return thc_spi_write_config(idev->thc, hz, mode, THC_IPTS_SPI_OPCODE,
+ 0, 0);
+}
+
+/*
+ * The core picks the fastest gear that does not exceed the request, so the
+ * clock is rounded down; a request below the slowest gear is raised to it,
+ * because running faster than asked is the one direction that breaks it.
+ */
+static void thc_ipts_spi_from_cfg(struct thc_ipts *idev, u32 *hz, u32 *mode)
+{
+ u32 cfg = le32_to_cpu(idev->cfg.spi_cfg);
+ u32 div = THC_IPTS_SPI_CLKDIV(cfg);
+
+ if (!div) {
+ /* Nothing valid asks for this; use the slowest gear. */
+ dev_warn_once(idev->dev, "controller asks for divisor 0\n");
+ *hz = THC_IPTS_SPI_MIN_HZ;
+ } else {
+ /*
+ * The core compares against gears rounded to 100 Hz, so round
+ * the same way: divisor 3 would otherwise land 34 Hz below the
+ * 41 MHz gear and select the 35 MHz one.
+ */
+ *hz = DIV_ROUND_CLOSEST(THC_IPTS_SPI_BASE_HZ / div, 100) * 100;
+ if (cfg & THC_IPTS_SPI_CLKDIV8)
+ *hz /= THC_IPTS_SPI_CLKDIV8_DIV;
+ }
+
+ if (*hz < THC_IPTS_SPI_MIN_HZ) {
+ dev_warn_once(idev->dev,
+ "controller asks for %u Hz, slowest is %u Hz\n",
+ *hz, THC_IPTS_SPI_MIN_HZ);
+ *hz = THC_IPTS_SPI_MIN_HZ;
+ }
+
+ if (cfg & THC_IPTS_SPI_QUAD)
+ *mode = THC_QUAD_IO;
+ else if (cfg & THC_IPTS_SPI_DUAL)
+ *mode = THC_DUAL_IO;
+ else
+ *mode = THC_SINGLE_IO;
+}
+
+static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
+{
+ u32 touch = le32_to_cpu(idev->cfg.touch_cfg);
+
+ touch &= ~(THC_IPTS_TOUCH_POWER_MASK | THC_IPTS_TOUCH_ENABLE |
+ THC_IPTS_TOUCH_HID_REPORT);
+ touch |= THC_IPTS_TOUCH_POWER(enable ? THC_IPTS_TOUCH_POWER_ON : 0);
+ if (enable)
+ touch |= THC_IPTS_TOUCH_ENABLE | THC_IPTS_TOUCH_HID_REPORT;
+
+ return thc_tic_pio_write(idev->thc, THC_IPTS_CFG_TOUCH, sizeof(touch),
+ &touch);
+}
+
+/**
+ * thc_ipts_port_start - bring one port up
+ * @idev: port to start
+ */
+int thc_ipts_port_start(struct thc_ipts *idev)
+{
+ u32 ctrl, hz, mode;
+ int ret;
+
+ ret = thc_ipts_quiesce(idev, true);
+ if (ret)
+ return ret;
+
+ ret = thc_port_select(idev->thc, THC_PORT_TYPE_SPI);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(idev->thc->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
+ &ctrl);
+ if (ret)
+ return ret;
+ if (!(ctrl & THC_M_PRT_CONTROL_PORT_SUPPORTED))
+ return -ENODEV;
+
+ /* Without this the engines write into the configuration block. */
+ thc_spi_input_output_address_config(idev->thc, 0, THC_IPTS_BULK_ADDR,
+ THC_IPTS_BULK_ADDR);
+
+ ret = thc_ipts_spi_set(idev, THC_IPTS_BOOT_SPI_HZ, THC_SINGLE_IO);
+ if (ret)
+ return ret;
+
+ /* The first configuration read is unreliable without edge detect. */
+ thc_int_trigger_type_select(idev->thc, true);
+
+ /* Release the controller from reset; it answers only after this. */
+ ret = regmap_set_bits(idev->thc->thc_regmap,
+ THC_M_PRT_CONTROL_OFFSET,
+ THC_M_PRT_CONTROL_DEVRST);
+ if (ret)
+ return ret;
+
+ msleep(THC_IPTS_BOOT_MS);
+
+ ret = thc_ipts_read_cfg(idev);
+ if (ret)
+ return ret;
+
+ idev->vendor = le16_to_cpu(idev->cfg.vendor);
+ idev->product = le16_to_cpu(idev->cfg.product);
+ idev->max_rx = THC_IPTS_RX_BUF_SIZE(le32_to_cpu(idev->cfg.buf_sizes));
+ idev->max_tx = THC_IPTS_TX_BUF_SIZE(le32_to_cpu(idev->cfg.buf_sizes));
+ if (idev->max_rx <= THC_IPTS_RX_HDR_SIZE ||
+ idev->max_tx <= THC_IPTS_TX_HDR_SIZE) {
+ dev_err(idev->dev, "buffer sizes %zu/%zu are too small\n",
+ idev->max_rx, idev->max_tx);
+ return -ENODEV;
+ }
+ thc_ipts_spi_from_cfg(idev, &hz, &mode);
+ ret = thc_ipts_spi_set(idev, hz, mode);
+ if (ret) {
+ dev_warn(idev->dev,
+ "cannot follow the SPI timing, going slow\n");
+ ret = thc_ipts_spi_set(idev, THC_IPTS_BOOT_SPI_HZ,
+ THC_SINGLE_IO);
+ if (ret)
+ return ret;
+ } else {
+ dev_dbg(idev->dev, "SPI clock asked for %u Hz, rounded down\n",
+ hz);
+ }
+
+ ret = thc_ipts_quiesce(idev, false);
+ if (ret)
+ return ret;
+
+ ret = thc_ipts_touch_set(idev, true);
+ if (ret)
+ return ret;
+ idev->running = true;
+
+ return 0;
+}
+
+/**
+ * thc_ipts_port_stop - tear one port down
+ * @idev: port to stop
+ */
+void thc_ipts_port_stop(struct thc_ipts *idev)
+{
+ if (!idev->thc)
+ return;
+
+ if (idev->running)
+ thc_ipts_touch_set(idev, false);
+
+ idev->running = false;
+ regmap_clear_bits(idev->thc->thc_regmap,
+ THC_M_PRT_CONTROL_OFFSET,
+ THC_M_PRT_CONTROL_DEVRST);
+ thc_ipts_quiesce(idev, true);
+}
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
new file mode 100644
index 00000000..f0493dca
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -0,0 +1,132 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _THC_IPTS_H_
+#define _THC_IPTS_H_
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/build_bug.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/types.h>
+
+#include "intel-thc-dev.h"
+
+/* Configuration block the controller keeps at address zero. */
+#define THC_IPTS_CFG_SIZE 64
+#define THC_IPTS_CFG_TOUCH 0x0c
+#define THC_IPTS_TIC_ID 0x43495424 /* "$TIC" */
+
+/* Bulk data window. The DMA engines have to be pointed here, not at zero. */
+#define THC_IPTS_BULK_ADDR 0x1000
+
+/* Ports in IPTS mode, one pair per generation. */
+#define PCI_DEVICE_ID_INTEL_THC_LKF_PORT1 0x98d0
+#define PCI_DEVICE_ID_INTEL_THC_LKF_PORT2 0x98d1
+#define PCI_DEVICE_ID_INTEL_THC_TGL_LP_PORT1 0xa0d0
+#define PCI_DEVICE_ID_INTEL_THC_TGL_LP_PORT2 0xa0d1
+#define PCI_DEVICE_ID_INTEL_THC_TGL_H_PORT1 0x43d0
+#define PCI_DEVICE_ID_INTEL_THC_TGL_H_PORT2 0x43d1
+#define PCI_DEVICE_ID_INTEL_THC_ADL_S_PORT1 0x7ad8
+#define PCI_DEVICE_ID_INTEL_THC_ADL_S_PORT2 0x7ad9
+#define PCI_DEVICE_ID_INTEL_THC_ADL_P_PORT1 0x51d0
+#define PCI_DEVICE_ID_INTEL_THC_ADL_P_PORT2 0x51d1
+#define PCI_DEVICE_ID_INTEL_THC_ADL_M_PORT1 0x54d0
+#define PCI_DEVICE_ID_INTEL_THC_ADL_M_PORT2 0x54d1
+#define PCI_DEVICE_ID_INTEL_THC_RPL_S_PORT1 0x7a58
+#define PCI_DEVICE_ID_INTEL_THC_RPL_S_PORT2 0x7a59
+
+/*
+ * The block is partly filled while the controller is still starting, so a
+ * missing $TIC tag is retried rather than treated as an error.
+ */
+#define THC_IPTS_CFG_TRIES 10
+#define THC_IPTS_CFG_RETRY_MS 100
+
+/* Bytes of the configuration block this driver interprets. */
+#define THC_IPTS_CFG_KNOWN 40
+
+/*
+ * Layout of the configuration block, as far as this driver reads it. Only the
+ * fields marked as used are read; the others are here to keep the offsets of
+ * the block, which the driver hands to the core as a whole.
+ */
+struct thc_ipts_cfg {
+ __le32 irq_cause; /* pending interrupt causes */
+ __le32 error; /* protocol error flags, not used */
+ __le32 buf_sizes; /* Rx/Tx sizes, see *_BUF_SIZE() */
+ __le32 touch_cfg; /* touch engine control, writable */
+ __le32 touch_state; /* touch engine state, not used */
+ __le32 device_id; /* THC_IPTS_TIC_ID after a good read */
+ __le32 spi_cfg; /* SPI timing the controller asks for */
+ __le16 vendor; /* vendor ID reported to HID */
+ __le16 product; /* product ID reported to HID */
+ __le32 revision; /* product revision */
+ __le32 fw_version; /* firmware version word */
+ u8 rest[THC_IPTS_CFG_SIZE - THC_IPTS_CFG_KNOWN];
+};
+
+static_assert(sizeof(struct thc_ipts_cfg) == THC_IPTS_CFG_SIZE);
+
+/* The core is handed the struct itself, so the offsets are part of the deal. */
+static_assert(offsetof(struct thc_ipts_cfg, buf_sizes) == 0x08);
+static_assert(offsetof(struct thc_ipts_cfg, touch_cfg) == 0x0c);
+static_assert(offsetof(struct thc_ipts_cfg, spi_cfg) == 0x18);
+static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
+
+/* Buffer sizes are stored as (size / 64) - 1, Rx in bits 13:0, Tx in 23:14. */
+#define THC_IPTS_DMA_UNIT 64
+#define THC_IPTS_RX_BUF_SIZE(val) \
+ ((size_t)(FIELD_GET(GENMASK(13, 0), (val)) + 1) * THC_IPTS_DMA_UNIT)
+#define THC_IPTS_TX_BUF_SIZE(val) \
+ ((size_t)(FIELD_GET(GENMASK(23, 14), (val)) + 1) * THC_IPTS_DMA_UNIT)
+
+#define THC_IPTS_TOUCH_ENABLE BIT(0)
+#define THC_IPTS_TOUCH_HID_REPORT BIT(2)
+#define THC_IPTS_TOUCH_POWER_ON 3
+#define THC_IPTS_TOUCH_POWER_MASK GENMASK(5, 3)
+#define THC_IPTS_TOUCH_POWER(val) \
+ FIELD_PREP(THC_IPTS_TOUCH_POWER_MASK, (val))
+
+#define THC_IPTS_SPI_CLKDIV(val) FIELD_GET(GENMASK(3, 1), (val))
+#define THC_IPTS_SPI_CLKDIV8 BIT(4)
+#define THC_IPTS_SPI_CLKDIV8_DIV 8
+#define THC_IPTS_SPI_DUAL BIT(6)
+#define THC_IPTS_SPI_QUAD BIT(7)
+#define THC_IPTS_SPI_BASE_HZ 125000000
+/* Slowest clock the core can program. */
+#define THC_IPTS_SPI_MIN_HZ 7812500
+
+/*
+ * Every frame carries a code and a length in front of the payload. Frames
+ * from the controller have a long header, frames to it a short one, and only
+ * the second one is padded to a four byte boundary.
+ */
+#define THC_IPTS_RX_HDR_SIZE 64
+#define THC_IPTS_TX_HDR_SIZE 8
+
+/**
+ * struct thc_ipts - state of one THC port in IPTS mode
+ * @dev: device used for resources and logging
+ * @thc: shared THC core context
+ * @cfg: configuration block last read from the controller
+ * @vendor: vendor ID the controller reports for its HID device
+ * @product: product ID the controller reports for its HID device
+ * @max_rx: largest frame the controller may send, in bytes
+ * @max_tx: largest frame the driver may send, in bytes
+ * @running: the controller answered and is out of reset
+ */
+struct thc_ipts {
+ struct device *dev;
+ struct thc_device *thc;
+ struct thc_ipts_cfg cfg;
+ u16 vendor;
+ u16 product;
+ size_t max_rx;
+ size_t max_tx;
+ bool running;
+};
+
+int thc_ipts_port_start(struct thc_ipts *idev);
+void thc_ipts_port_stop(struct thc_ipts *idev);
+
+#endif /* _THC_IPTS_H_ */
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
new file mode 100644
index 00000000..48046cd1
--- /dev/null
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI glue for the Intel Touch Host Controller in IPTS mode. Ports that
+ * the HIDSPI driver already claims are deliberately left out of the ID table.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/workqueue.h>
+
+#include "ipts.h"
+
+static const struct pci_device_id thc_ipts_pci_ids[] = {
+ /* Lakefield */
+ {PCI_DEVICE_DATA(INTEL, THC_LKF_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_LKF_PORT2, NULL)},
+ /* Tiger Lake */
+ {PCI_DEVICE_DATA(INTEL, THC_TGL_LP_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_TGL_LP_PORT2, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_TGL_H_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_TGL_H_PORT2, NULL)},
+ /* Alder Lake */
+ {PCI_DEVICE_DATA(INTEL, THC_ADL_S_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_ADL_S_PORT2, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_ADL_P_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_ADL_P_PORT2, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_ADL_M_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_ADL_M_PORT2, NULL)},
+ /* Raptor Lake */
+ {PCI_DEVICE_DATA(INTEL, THC_RPL_S_PORT1, NULL)},
+ {PCI_DEVICE_DATA(INTEL, THC_RPL_S_PORT2, NULL)},
+ {}
+};
+MODULE_DEVICE_TABLE(pci, thc_ipts_pci_ids);
+
+static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+ struct thc_ipts *idev;
+ void __iomem *mem;
+ int ret;
+
+ idev = devm_kzalloc_obj(&pci->dev, *idev);
+ if (!idev)
+ return -ENOMEM;
+
+ idev->dev = &pci->dev;
+ pci_set_drvdata(pci, idev);
+
+ ret = pcim_enable_device(pci);
+ if (ret) {
+ dev_err(idev->dev,
+ "cannot enable the PCI function, error %d\n", ret);
+ return ret;
+ }
+
+ pci_set_master(pci);
+
+ mem = pcim_iomap_region(pci, 0, KBUILD_MODNAME);
+ ret = PTR_ERR_OR_ZERO(mem);
+ if (ret) {
+ dev_err(idev->dev, "cannot map the register space, error %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = dma_set_mask_and_coherent(idev->dev, DMA_BIT_MASK(64));
+ if (ret)
+ ret = dma_set_mask_and_coherent(idev->dev, DMA_BIT_MASK(32));
+ if (ret) {
+ dev_err(idev->dev, "no usable DMA configuration, error %d\n",
+ ret);
+ return ret;
+ }
+
+ idev->thc = thc_dev_init(idev->dev, mem);
+ if (IS_ERR(idev->thc))
+ return PTR_ERR(idev->thc);
+
+ ret = thc_ipts_port_start(idev);
+ if (ret) {
+ thc_ipts_port_stop(idev);
+ return ret;
+ }
+
+ dev_info(idev->dev, "controller %04x:%04x rev %u, firmware 0x%08x\n",
+ idev->vendor, idev->product, le32_to_cpu(idev->cfg.revision),
+ le32_to_cpu(idev->cfg.fw_version));
+ dev_info(idev->dev, "spi cfg 0x%08x, rx %zu tx %zu bytes\n",
+ le32_to_cpu(idev->cfg.spi_cfg), idev->max_rx, idev->max_tx);
+
+ return 0;
+}
+
+static void thc_ipts_remove(struct pci_dev *pci)
+{
+ struct thc_ipts *idev = pci_get_drvdata(pci);
+
+ thc_ipts_port_stop(idev);
+ pci_clear_master(pci);
+}
+
+static void thc_ipts_shutdown(struct pci_dev *pci)
+{
+ struct thc_ipts *idev = pci_get_drvdata(pci);
+
+ if (idev)
+ thc_ipts_port_stop(idev);
+}
+
+static struct pci_driver thc_ipts_driver = {
+ .name = "intel-ipts",
+ .id_table = thc_ipts_pci_ids,
+ .probe = thc_ipts_probe,
+ .remove = thc_ipts_remove,
+ .shutdown = thc_ipts_shutdown,
+ /* Bring-up waits for the controller, up to a few seconds. */
+ .driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+};
+module_pci_driver(thc_ipts_driver);
+
+MODULE_AUTHOR("Emfox Zhou <emfox@debian.org>");
+MODULE_DESCRIPTION("IPTS transport for the Intel Touch Host Controller");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("INTEL_THC");
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v4 2/5] HID: intel-thc-hid: intel-ipts: receive reports
2026-11-14 21:40 [PATCH v4 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports Emfox Zhou
2026-11-14 21:40 ` [PATCH v4 1/5] HID: intel-thc-hid: intel-ipts: bring up the port Emfox Zhou
@ 2026-11-14 21:40 ` Emfox Zhou
2026-11-14 22:00 ` sashiko-bot
2026-11-14 21:40 ` [PATCH v4 3/5] HID: intel-thc-hid: intel-ipts: register the HID device Emfox Zhou
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Emfox Zhou @ 2026-11-14 21:40 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
Take the port one step further: configure the two DMA engines, one for
the frames the controller sends and one for the frames the host sends,
and register the interrupt that reports them.
Interrupt remapping rejects the MSI requests these ports send: the
requester ID in them is 01:05.0, neither the port's own function nor a
valid DMA alias for it, so only the legacy interrupt pin works once
remapping is on. MSI is the fallback for a platform that does not route
that pin. The pin is shared with other functions on the PCH, and such a
line only takes handlers that agree on oneshot, so the request asks for
it only if a sharer uses it. The hardirq masks the port itself, so a
foreign interrupt costs at most a few register reads in the thread, and
is reported as not handled.
Transfer errors are recovered from a work item and not from the threaded
handler: thc_rxdma_reset() takes the bus lock, and a pending
thc_dma_write() holds that lock while it waits for the very completion the
handler has to deliver. The port stays masked while that item runs and the
handler ignores the line, so the reset cannot race with a frame being read:
masking alone would not help, as another function on the shared pin can
still wake the handler.
Every frame carries a code and a length in front of the payload. The
receive side decodes that header and logs each frame.
Three details are worth spelling out:
- The interrupt configuration has to be applied before the DMA
engines are started: it programs the device interrupt type field to
the invalid value, and starting the receive engine then replaces that
field with the type it expects. The other order would leave the
invalid value in place.
- The line is masked while the threaded handler runs, and an event
that arrives in that window leaves nothing behind but its status
bit. The handler therefore drains the status registers instead of
trusting a single delivery: a write completion taken for granted
this way makes the next transfer wait for a completion that never
comes, and the port stays unusable until it is reset.
- Bulk transfers have no address phase, so the SPI opcode is zero.
The opcodes the HIDSPI path uses make this controller ignore the
frames altogether.
Signed-off-by: Emfox Zhou <emfox@debian.org>
---
drivers/hid/intel-thc-hid/intel-ipts/ipts.c | 249 ++++++++++++++++++
drivers/hid/intel-thc-hid/intel-ipts/ipts.h | 17 ++
.../hid/intel-thc-hid/intel-ipts/pci-ipts.c | 44 +++-
3 files changed, 309 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
index 205dc51b..b4819944 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -6,8 +6,13 @@
#include <linux/delay.h>
#include <linux/math.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
#include <linux/regmap.h>
+#include <linux/sizes.h>
#include <linux/string.h>
+#include <linux/unaligned.h>
+#include <linux/workqueue.h>
#include "intel-thc-hw.h"
#include "ipts.h"
@@ -21,6 +26,13 @@
/* Time the controller needs after reset before it answers on the bus. */
#define THC_IPTS_BOOT_MS 100
+/*
+ * thc_interrupt_handler() handles one cause per call, so a burst
+ * needs one call per cause plus the one that reports none. The
+ * bound only stops a controller that never falls quiet.
+ */
+#define THC_IPTS_IRQ_PASSES (THC_UNKNOWN_INT + 1)
+
static int thc_ipts_read_cfg(struct thc_ipts *idev)
{
int ret, try;
@@ -133,6 +145,212 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
&touch);
}
+static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
+{
+ size_t capacity = idev->max_rx - THC_IPTS_RX_HDR_SIZE;
+ u32 code, size;
+
+ if (len < THC_IPTS_RX_HDR_SIZE)
+ return;
+
+ code = get_unaligned_le32(idev->rx_frame);
+ /* The header may claim more than what actually arrived. */
+ size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4),
+ capacity);
+ size = min_t(u32, size, len - THC_IPTS_RX_HDR_SIZE);
+
+ dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
+}
+
+static void thc_ipts_dma_stop(struct thc_ipts *idev)
+{
+ if (!idev->dma_ready)
+ return;
+
+ WRITE_ONCE(idev->dma_ready, false);
+ thc_dma_unconfigure(idev->thc);
+ thc_dma_release(idev->thc);
+}
+
+static int thc_ipts_dma_start(struct thc_ipts *idev)
+{
+ int ret;
+
+ /*
+ * Allocated once, so each buffer is retried on its own: a failed
+ * first start must not leave a NULL frame buffer for a resume.
+ */
+ if (!idev->rx_frame) {
+ /*
+ * The core rounds the packet size up to a page when it sets
+ * up the engines, so the buffer has to be that large.
+ */
+ idev->rx_frame = devm_kzalloc(idev->dev,
+ round_up(idev->max_rx, SZ_4K),
+ GFP_KERNEL);
+ if (!idev->rx_frame)
+ return -ENOMEM;
+ }
+
+ ret = thc_dma_set_max_packet_sizes(idev->thc, 0, idev->max_rx,
+ idev->max_tx, 0);
+ if (ret)
+ return ret;
+
+ ret = thc_dma_allocate(idev->thc);
+ if (ret)
+ return ret;
+
+ ret = thc_dma_configure(idev->thc);
+ if (ret) {
+ thc_dma_release(idev->thc);
+ return ret;
+ }
+
+ WRITE_ONCE(idev->dma_ready, true);
+
+ return 0;
+}
+
+irqreturn_t thc_ipts_hardirq(int irq, void *data)
+{
+ struct thc_ipts *idev = data;
+
+ /*
+ * The pin is shared and level triggered: mask first, or it stays
+ * asserted on the way out. The cause itself can only be read in the
+ * thread.
+ */
+ thc_interrupt_enable(idev->thc, false);
+
+ /* A reset in flight: the port is masked, so this is not ours. */
+ if (READ_ONCE(idev->recovering))
+ return IRQ_NONE;
+
+ /*
+ * Nothing of ours is pending in these two states and the port is
+ * masked: the interrupt belongs to another function on the line.
+ */
+ if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
+ return IRQ_NONE;
+
+ return IRQ_WAKE_THREAD;
+}
+
+static int thc_ipts_read_frames(struct thc_ipts *idev)
+{
+ for (;;) {
+ int finished = 0;
+ size_t len = 0;
+
+ if (thc_rxdma_read(idev->thc, THC_RXDMA2, idev->rx_frame,
+ &len, &finished)) {
+ dev_warn_once(idev->dev,
+ "cannot read a frame from the engine\n");
+ return -EIO;
+ }
+
+ /* Nothing was read: the ring was empty and len is still 0. */
+ if (!len)
+ break;
+
+ thc_ipts_handle_frame(idev, len);
+ if (finished)
+ break;
+ }
+
+ return 0;
+}
+
+/*
+ * Run from a work item: thc_rxdma_reset() takes the bus lock, which
+ * thc_dma_write() holds while it waits for a completion this thread has to
+ * deliver, so resetting inline would stall until the write times out.
+ */
+void thc_ipts_recover(struct work_struct *work)
+{
+ struct thc_ipts *idev = container_of(work, struct thc_ipts,
+ recover_work);
+
+ /* A handler may have unmasked after the error; keep it masked. */
+ thc_interrupt_enable(idev->thc, false);
+
+ if (thc_rxdma_reset(idev->thc)) {
+ /* Nothing left: keep the port masked until it restarts. */
+ dev_err(idev->dev, "cannot reset the receive engine\n");
+ return;
+ }
+
+ WRITE_ONCE(idev->recovering, false);
+ if (!READ_ONCE(idev->stopping))
+ thc_interrupt_enable(idev->thc, true);
+}
+
+irqreturn_t thc_ipts_thread(int irq, void *data)
+{
+ struct thc_ipts *idev = data;
+ irqreturn_t ret = IRQ_NONE; /* no cause of ours yet */
+ bool need_recover = false;
+ int pass;
+
+ /* A reset is in flight; the engine state is not ours to read. */
+ if (READ_ONCE(idev->recovering))
+ return IRQ_NONE;
+
+ /*
+ * The port stays masked until the end of this handler, so a cause that
+ * arrives in between leaves only its status bit: keep reading until
+ * the core reports none.
+ */
+ for (pass = 0; pass < THC_IPTS_IRQ_PASSES; pass++) {
+ int mask = thc_interrupt_handler(idev->thc);
+
+ /* The core reports this one when no cause is pending. */
+ if (mask == BIT(THC_UNKNOWN_INT))
+ break;
+
+ ret = IRQ_HANDLED;
+ if (mask & (BIT(THC_FATAL_ERR_INT) | BIT(THC_TXN_ERR_INT))) {
+ dev_err(idev->dev,
+ "transfer error, resetting the engine\n");
+ need_recover = true;
+ /*
+ * Keep reading: the core stops at the first cause, so
+ * a Tx completion queued behind the error would wait
+ * out its timeout under the bus lock.
+ */
+ continue;
+ }
+
+ if (need_recover)
+ continue;
+
+ /*
+ * A non-DMA cause needs no action: the core acknowledges it,
+ * and this port moves report data through the DMA engines.
+ */
+ if (mask & BIT(THC_RXDMA2_INT) && thc_ipts_read_frames(idev))
+ need_recover = true;
+ }
+
+ /*
+ * The work item resets the engine and unmasks the port again; a frame
+ * read while it rewrites the ring would corrupt the descriptors, so
+ * ->recovering keeps the handler off the line until it is done.
+ */
+ if (need_recover) {
+ WRITE_ONCE(idev->recovering, true);
+ schedule_work(&idev->recover_work);
+ return IRQ_HANDLED;
+ }
+
+ if (!READ_ONCE(idev->stopping))
+ thc_interrupt_enable(idev->thc, true);
+
+ /* An interrupt with no cause of ours is not ours to claim. */
+ return ret;
+}
+
/**
* thc_ipts_port_start - bring one port up
* @idev: port to start
@@ -142,6 +360,9 @@ int thc_ipts_port_start(struct thc_ipts *idev)
u32 ctrl, hz, mode;
int ret;
+ WRITE_ONCE(idev->stopping, false);
+ WRITE_ONCE(idev->recovering, false);
+
ret = thc_ipts_quiesce(idev, true);
if (ret)
return ret;
@@ -214,6 +435,19 @@ int thc_ipts_port_start(struct thc_ipts *idev)
return ret;
idev->running = true;
+ /*
+ * thc_interrupt_config() programs the interrupt type field to the
+ * invalid value; starting RXDMA2 then sets it to the type the engine
+ * expects, so the configuration has to happen first.
+ */
+ thc_interrupt_config(idev->thc);
+
+ ret = thc_ipts_dma_start(idev);
+ if (ret)
+ return ret;
+
+ thc_interrupt_enable(idev->thc, true);
+
return 0;
}
@@ -226,6 +460,19 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
if (!idev->thc)
return;
+ /*
+ * Mask the line, then wait for a handler that may be holding a
+ * frame for the HID device which is about to go away.
+ */
+ WRITE_ONCE(idev->stopping, true);
+ thc_interrupt_enable(idev->thc, false);
+ synchronize_irq(idev->irq);
+ cancel_work_sync(&idev->recover_work);
+
+ /* A handler that read ->stopping early may have unmasked. */
+ thc_interrupt_enable(idev->thc, false);
+
+ /* Stop the controller before the buffers it writes into go away. */
if (idev->running)
thc_ipts_touch_set(idev, false);
@@ -234,4 +481,6 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
THC_M_PRT_CONTROL_OFFSET,
THC_M_PRT_CONTROL_DEVRST);
thc_ipts_quiesce(idev, true);
+
+ thc_ipts_dma_stop(idev);
}
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
index f0493dca..f233d9c8 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/interrupt.h>
#include <linux/types.h>
+#include <linux/wait.h>
#include "intel-thc-dev.h"
@@ -108,24 +109,40 @@ static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
* struct thc_ipts - state of one THC port in IPTS mode
* @dev: device used for resources and logging
* @thc: shared THC core context
+ * @irq: interrupt line of the port
* @cfg: configuration block last read from the controller
* @vendor: vendor ID the controller reports for its HID device
* @product: product ID the controller reports for its HID device
* @max_rx: largest frame the controller may send, in bytes
* @max_tx: largest frame the driver may send, in bytes
+ * @rx_frame: buffer for one received frame, including its header
+ * @recover_work: resets the receive engine after a transfer error
* @running: the controller answered and is out of reset
+ * @dma_ready: DMA engines are configured and may be used
+ * @recovering: a reset is queued or running, ignore the interrupt
+ * @stopping: the port is being torn down, ignore the interrupt
*/
struct thc_ipts {
struct device *dev;
struct thc_device *thc;
+ int irq;
struct thc_ipts_cfg cfg;
u16 vendor;
u16 product;
size_t max_rx;
size_t max_tx;
+ u8 *rx_frame;
+ struct work_struct recover_work;
bool running;
+ bool dma_ready;
+ bool recovering;
+ bool stopping;
};
+irqreturn_t thc_ipts_hardirq(int irq, void *data);
+irqreturn_t thc_ipts_thread(int irq, void *data);
+void thc_ipts_recover(struct work_struct *work);
+
int thc_ipts_port_start(struct thc_ipts *idev);
void thc_ipts_port_stop(struct thc_ipts *idev);
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
index 48046cd1..161bee9f 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -46,6 +46,7 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
idev->dev = &pci->dev;
pci_set_drvdata(pci, idev);
+ INIT_WORK(&idev->recover_work, thc_ipts_recover);
ret = pcim_enable_device(pci);
if (ret) {
@@ -77,10 +78,42 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
if (IS_ERR(idev->thc))
return PTR_ERR(idev->thc);
+ /*
+ * Interrupt remapping rejects the MSI requests these ports send, so
+ * take the legacy pin (shared with other functions on the PCH, hence
+ * IRQF_SHARED); MSI is only for a platform that routes no pin.
+ */
+ ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_INTX);
+ if (ret < 0) {
+ dev_warn(idev->dev, "no interrupt pin, falling back to MSI\n");
+ ret = pci_alloc_irq_vectors(pci, 1, 1,
+ PCI_IRQ_MSI | PCI_IRQ_MSIX);
+ }
+ if (ret < 0) {
+ dev_err(idev->dev, "no interrupt vector, error %d\n", ret);
+ return ret;
+ }
+
+ idev->irq = pci_irq_vector(pci, 0);
+
+ /*
+ * A shared line only takes handlers that agree on oneshot, and the
+ * hardirq masks the port itself, so ask for it only if a sharer has.
+ */
+ ret = devm_request_threaded_irq(idev->dev, idev->irq, thc_ipts_hardirq,
+ thc_ipts_thread,
+ IRQF_SHARED | IRQF_COND_ONESHOT,
+ KBUILD_MODNAME, idev);
+ if (ret) {
+ dev_err(idev->dev, "cannot get the interrupt, error %d\n", ret);
+ goto err_vectors;
+ }
+
ret = thc_ipts_port_start(idev);
if (ret) {
thc_ipts_port_stop(idev);
- return ret;
+ devm_free_irq(idev->dev, idev->irq, idev);
+ goto err_vectors;
}
dev_info(idev->dev, "controller %04x:%04x rev %u, firmware 0x%08x\n",
@@ -90,6 +123,11 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
le32_to_cpu(idev->cfg.spi_cfg), idev->max_rx, idev->max_tx);
return 0;
+
+err_vectors:
+ pci_free_irq_vectors(pci);
+
+ return ret;
}
static void thc_ipts_remove(struct pci_dev *pci)
@@ -97,6 +135,10 @@ static void thc_ipts_remove(struct pci_dev *pci)
struct thc_ipts *idev = pci_get_drvdata(pci);
thc_ipts_port_stop(idev);
+
+ /* The handler has to go before the vector it is bound to is freed. */
+ devm_free_irq(idev->dev, idev->irq, idev);
+ pci_free_irq_vectors(pci);
pci_clear_master(pci);
}
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 2/5] HID: intel-thc-hid: intel-ipts: receive reports
2026-11-14 21:40 ` [PATCH v4 2/5] HID: intel-thc-hid: intel-ipts: receive reports Emfox Zhou
@ 2026-11-14 22:00 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-11-14 22:00 UTC (permalink / raw)
To: Emfox Zhou; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: shared IRQ race unmasks
thc_ipts_thread()
- [Medium] HID: intel-thc-hid: PRD table descriptors not reset in
thc_ipts_recover()
--
commit 45ecfedbdb695899d182e98734d3a4bf861c6155
Author: Emfox Zhou <emfox@debian.org>
HID: intel-thc-hid: intel-ipts: receive reports
This commit configures the Rx and Tx DMA engines for IPTS ports, registers
a threaded interrupt handler, and adds frame reception and error recovery
handling.
> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index 205dc51b86cda..b48199448dbec 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> @@ -133,6 +145,212 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
[ ... ]
> +irqreturn_t thc_ipts_hardirq(int irq, void *data)
> +{
> + struct thc_ipts *idev = data;
> +
> + /*
> + * The pin is shared and level triggered: mask first, or it stays
> + * asserted on the way out. The cause itself can only be read in the
> + * thread.
> + */
> + thc_interrupt_enable(idev->thc, false);
> +
> + /* A reset in flight: the port is masked, so this is not ours. */
> + if (READ_ONCE(idev->recovering))
> + return IRQ_NONE;
> +
> + /*
> + * Nothing of ours is pending in these two states and the port is
> + * masked: the interrupt belongs to another function on the line.
> + */
> + if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
> + return IRQ_NONE;
> +
> + return IRQ_WAKE_THREAD;
> +}
[ ... ]
> +void thc_ipts_recover(struct work_struct *work)
> +{
> + struct thc_ipts *idev = container_of(work, struct thc_ipts,
> + recover_work);
> +
> + /* A handler may have unmasked after the error; keep it masked. */
> + thc_interrupt_enable(idev->thc, false);
> +
> + if (thc_rxdma_reset(idev->thc)) {
> + /* Nothing left: keep the port masked until it restarts. */
> + dev_err(idev->dev, "cannot reset the receive engine\n");
> + return;
> + }
[Severity: Medium]
Does thc_rxdma_reset() leave hardware-modified PRD table descriptors in
place after an error?
When THC_RXDMA2 writes a frame into a PRD table, the hardware updates each
entry's len to the received byte count and sets end_of_prd on the last
entry used. Normal reads restore those entries to SZ_4K via
thc_copy_one_sgl_to_prd() in thc_dma_read(), but when need_recover is set
in thc_ipts_thread() (or when read_dma_buffer() fails), pending frames are
left unconsumed. thc_rxdma_reset() only calls thc_dma_unconfigure() and
thc_dma_configure(), resetting TPCRP and TPCWP without calling
thc_copy_sgls_to_prd(), so later frames that exceed the stale entry
lengths will hit PRD buffer overrun errors.
> +
> + WRITE_ONCE(idev->recovering, false);
> + if (!READ_ONCE(idev->stopping))
> + thc_interrupt_enable(idev->thc, true);
> +}
> +
> +irqreturn_t thc_ipts_thread(int irq, void *data)
> +{
> + struct thc_ipts *idev = data;
> + irqreturn_t ret = IRQ_NONE; /* no cause of ours yet */
> + bool need_recover = false;
> + int pass;
> +
> + /* A reset is in flight; the engine state is not ours to read. */
> + if (READ_ONCE(idev->recovering))
> + return IRQ_NONE;
> +
> + /*
> + * The port stays masked until the end of this handler, so a cause that
> + * arrives in between leaves only its status bit: keep reading until
> + * the core reports none.
> + */
> + for (pass = 0; pass < THC_IPTS_IRQ_PASSES; pass++) {
> + int mask = thc_interrupt_handler(idev->thc);
> +
> + /* The core reports this one when no cause is pending. */
> + if (mask == BIT(THC_UNKNOWN_INT))
> + break;
> +
> + ret = IRQ_HANDLED;
> + if (mask & (BIT(THC_FATAL_ERR_INT) | BIT(THC_TXN_ERR_INT))) {
> + dev_err(idev->dev,
> + "transfer error, resetting the engine\n");
> + need_recover = true;
> + /*
> + * Keep reading: the core stops at the first cause, so
> + * a Tx completion queued behind the error would wait
> + * out its timeout under the bus lock.
> + */
> + continue;
> + }
> +
> + if (need_recover)
> + continue;
> +
> + /*
> + * A non-DMA cause needs no action: the core acknowledges it,
> + * and this port moves report data through the DMA engines.
> + */
> + if (mask & BIT(THC_RXDMA2_INT) && thc_ipts_read_frames(idev))
> + need_recover = true;
> + }
> +
> + /*
> + * The work item resets the engine and unmasks the port again; a frame
> + * read while it rewrites the ring would corrupt the descriptors, so
> + * ->recovering keeps the handler off the line until it is done.
> + */
> + if (need_recover) {
> + WRITE_ONCE(idev->recovering, true);
> + schedule_work(&idev->recover_work);
> + return IRQ_HANDLED;
> + }
> +
> + if (!READ_ONCE(idev->stopping))
> + thc_interrupt_enable(idev->thc, true);
> +
> + /* An interrupt with no cause of ours is not ours to claim. */
> + return ret;
> +}
[Severity: Medium]
Can a shared interrupt cause thc_ipts_thread() to run while the port is
unmasked in hardware?
thc_ipts_probe() requests the interrupt with IRQF_SHARED |
IRQF_COND_ONESHOT, which leaves IRQS_ONESHOT unset when no earlier handler
on the line requested IRQF_ONESHOT. If another device on the shared INTx
line fires while thc_ipts_thread() is running, thc_ipts_hardirq() sets
IRQTF_RUNTHREAD again before the first pass of thc_ipts_thread() unmasks
the port:
CPU0 (irq_thread) CPU1 (<hardirq>)
thc_ipts_thread()
for (pass = 0; ...)
...
thc_ipts_hardirq()
thc_interrupt_enable(..., false)
return IRQ_WAKE_THREAD
/* sets IRQTF_RUNTHREAD */
thc_interrupt_enable(..., true)
thc_ipts_thread() <-- runs with port unmasked
The same race also exists in thc_ipts_recover() and thc_ipts_port_start(),
which publish idev->recovering = false and idev->dma_ready = true before
calling thc_interrupt_enable(idev->thc, true): a shared interrupt in that
window wakes thc_ipts_thread() and then gets unmasked underneath it. If
that unmasked run hits an error, the need_recover path returns without
calling thc_interrupt_enable(idev->thc, false), leaving the port unmasked
until recover_work runs.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261114214026.2809740-1-emfox@debian.org?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 3/5] HID: intel-thc-hid: intel-ipts: register the HID device
2026-11-14 21:40 [PATCH v4 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports Emfox Zhou
2026-11-14 21:40 ` [PATCH v4 1/5] HID: intel-thc-hid: intel-ipts: bring up the port Emfox Zhou
2026-11-14 21:40 ` [PATCH v4 2/5] HID: intel-thc-hid: intel-ipts: receive reports Emfox Zhou
@ 2026-11-14 21:40 ` Emfox Zhou
2026-11-14 21:54 ` sashiko-bot
2026-11-14 21:40 ` [PATCH v4 4/5] HID: intel-thc-hid: intel-ipts: handle system sleep Emfox Zhou
2026-11-14 21:40 ` [PATCH v4 5/5] HID: intel-thc-hid: intel-ipts: document the IPTS mode Emfox Zhou
4 siblings, 1 reply; 8+ messages in thread
From: Emfox Zhou @ 2026-11-14 21:40 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
Register a HID device for the port and let it own the handshake: the
report descriptor is not read once at probe and kept, it is asked for
over the transport whenever the HID core parses the device, and feature
reports the core asks for are fetched the same way.
A feature answer arrives as a frame of its own while the request waits, so
it can land after that request timed out and another one started. The
answer carries the report ID back, and every request also gets a serial
number: asking for the same report twice would otherwise leave the ID
unable to tell the two answers apart. The flags, the buffer and the
serial number are updated together under a lock, so matching an answer to
a request and consuming it are one step.
Requests are refused while a recovery is in flight: the port is masked,
so a write would wait out its one second timeout, holding the bus lock
that the reset needs.
Teardown takes the request lock before it masks the line: a request already
inside thc_dma_write needs the interrupt that completes it, and with the
line masked first it would wait out its timeout holding the bus lock.
Input reports go straight to the input layer, and the raw report stream
is available to user space through hidraw. That is where decoding
belongs: the reports carry a heatmap, and turning it into contacts
needs floating point arithmetic the kernel cannot do.
The controller starts streaming once user space asks for it, through a
set feature request on report 5. The driver does not do that on its
own, the daemon that opens hidraw sends it.
Signed-off-by: Emfox Zhou <emfox@debian.org>
---
drivers/hid/intel-thc-hid/intel-ipts/ipts.c | 371 ++++++++++++++++--
drivers/hid/intel-thc-hid/intel-ipts/ipts.h | 51 +++
.../hid/intel-thc-hid/intel-ipts/pci-ipts.c | 8 +-
3 files changed, 400 insertions(+), 30 deletions(-)
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
index b4819944..80973944 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
@@ -4,8 +4,10 @@
* SPI and carries its HID reports in a private framing.
*/
+#include <linux/cleanup.h>
#include <linux/delay.h>
#include <linux/math.h>
+#include <linux/hid.h>
#include <linux/interrupt.h>
#include <linux/minmax.h>
#include <linux/regmap.h>
@@ -145,8 +147,83 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
&touch);
}
+/*
+ * One frame to the controller: code, length, then the payload padded to four
+ * bytes. The frame is built in @idev->tx_frame, so callers serialise with
+ * @idev->req_lock.
+ */
+static int thc_ipts_send(struct thc_ipts *idev, u32 code, const void *data,
+ size_t len)
+{
+ size_t padded = round_up(len, 4);
+ u8 *frame = idev->tx_frame;
+
+ if (READ_ONCE(idev->stopping))
+ return -ESHUTDOWN;
+ /* The port is masked: the write would wait out its timeout. */
+ if (READ_ONCE(idev->recovering))
+ return -EAGAIN;
+ if (!READ_ONCE(idev->dma_ready))
+ return -EAGAIN;
+ if (THC_IPTS_TX_HDR_SIZE + padded > idev->max_tx)
+ return -EMSGSIZE;
+
+ put_unaligned_le32(code, frame);
+ put_unaligned_le32((u32)len, frame + 4);
+ memset(frame + THC_IPTS_TX_HDR_SIZE, 0, padded);
+ if (len && data)
+ memcpy(frame + THC_IPTS_TX_HDR_SIZE, data, len);
+
+ return thc_dma_write(idev->thc, frame, THC_IPTS_TX_HDR_SIZE + padded);
+}
+
+static void thc_ipts_descriptor(struct thc_ipts *idev, const u8 *data,
+ size_t len)
+{
+ /* One answer per request: a late reply must not overwrite it. */
+ if (len <= THC_IPTS_DESC_PREAMBLE || READ_ONCE(idev->have_desc))
+ return;
+
+ len = min(len - THC_IPTS_DESC_PREAMBLE, idev->max_rx);
+ memcpy(idev->desc, data + THC_IPTS_DESC_PREAMBLE, len);
+ idev->desc_len = len;
+ WRITE_ONCE(idev->have_desc, true);
+ wake_up(&idev->wq);
+}
+
+/* The report is collected in a private buffer; the caller copies it out. */
+static void thc_ipts_feature(struct thc_ipts *idev, const u8 *data,
+ size_t len)
+{
+ if (!len)
+ return;
+
+ /*
+ * A request that timed out is dropped by the sender, which may be
+ * asking for another report by now: check and copy in one step. The
+ * serial number tells an earlier answer from the one asked for.
+ */
+ spin_lock_irq(&idev->feature_lock);
+ if (!idev->feature_pending ||
+ (idev->feature_id && data[0] != idev->feature_id)) {
+ spin_unlock_irq(&idev->feature_lock);
+ return;
+ }
+
+ len = min(len, idev->max_rx);
+ memcpy(idev->feature, data, len);
+ idev->feature_len = len;
+ idev->feature_pending = false;
+ idev->feature_ack = idev->feature_seq;
+ WRITE_ONCE(idev->feature_done, true);
+ spin_unlock_irq(&idev->feature_lock);
+
+ wake_up(&idev->wq);
+}
+
static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
{
+ const u8 *payload = idev->rx_frame + THC_IPTS_RX_HDR_SIZE;
size_t capacity = idev->max_rx - THC_IPTS_RX_HDR_SIZE;
u32 code, size;
@@ -155,11 +232,31 @@ static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
code = get_unaligned_le32(idev->rx_frame);
/* The header may claim more than what actually arrived. */
- size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4),
- capacity);
+ size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4), capacity);
size = min_t(u32, size, len - THC_IPTS_RX_HDR_SIZE);
- dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
+ switch (code) {
+ case THC_IPTS_RX_INPUT: {
+ struct hid_device *hdev = READ_ONCE(idev->hid);
+
+ if (hdev)
+ hid_safe_input_report(hdev, HID_INPUT_REPORT,
+ (u8 *)payload, capacity, size, 1);
+ break;
+ }
+ case THC_IPTS_RX_DESC:
+ thc_ipts_descriptor(idev, payload, size);
+ break;
+ case THC_IPTS_RX_FEATURE:
+ thc_ipts_feature(idev, payload, size);
+ break;
+ case THC_IPTS_RX_RESET:
+ dev_warn(idev->dev, "controller asked for a reset\n");
+ break;
+ default:
+ dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
+ break;
+ }
}
static void thc_ipts_dma_stop(struct thc_ipts *idev)
@@ -188,10 +285,24 @@ static int thc_ipts_dma_start(struct thc_ipts *idev)
idev->rx_frame = devm_kzalloc(idev->dev,
round_up(idev->max_rx, SZ_4K),
GFP_KERNEL);
- if (!idev->rx_frame)
- return -ENOMEM;
}
+ if (!idev->tx_frame)
+ idev->tx_frame = devm_kzalloc(idev->dev,
+ round_up(idev->max_tx, 4),
+ GFP_KERNEL);
+ if (!idev->desc)
+ idev->desc = devm_kzalloc(idev->dev, idev->max_rx, GFP_KERNEL);
+ if (!idev->feature)
+ idev->feature = devm_kzalloc(idev->dev, idev->max_rx,
+ GFP_KERNEL);
+ if (!idev->rx_frame || !idev->tx_frame || !idev->desc || !idev->feature)
+ return -ENOMEM;
+ /*
+ * The sizes the controller advertises are the largest frame it sends
+ * and the largest it accepts, which is what the engines need; the core
+ * rounds them up to a page and derives the PRD table sizes from them.
+ */
ret = thc_dma_set_max_packet_sizes(idev->thc, 0, idev->max_rx,
idev->max_tx, 0);
if (ret)
@@ -351,6 +462,209 @@ irqreturn_t thc_ipts_thread(int irq, void *data)
return ret;
}
+static int thc_ipts_hid_parse(struct hid_device *hdev)
+{
+ struct thc_ipts *idev = hdev->driver_data;
+ unsigned long timeout = msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS);
+ u64 zero = 0;
+ int ret;
+ int try;
+
+ guard(mutex)(&idev->req_lock);
+
+ for (try = 0; try < THC_IPTS_REQ_TRIES; try++) {
+ WRITE_ONCE(idev->have_desc, false);
+
+ /* The controller ignores the eight byte argument. */
+ ret = thc_ipts_send(idev, THC_IPTS_TX_GET_DESC, &zero,
+ sizeof(zero));
+ if (ret)
+ return ret;
+
+ if (wait_event_timeout(idev->wq, READ_ONCE(idev->have_desc),
+ timeout) &&
+ idev->desc_len > 0)
+ return hid_parse_report(hdev, idev->desc,
+ idev->desc_len);
+ }
+
+ dev_err(idev->dev,
+ "controller did not return a report descriptor in %d tries\n",
+ try);
+
+ return -ETIMEDOUT;
+}
+
+/*
+ * A feature report is not pushed by the controller: it is asked for and comes
+ * back as a frame of its own, so the request waits for the handler.
+ */
+static int thc_ipts_get_feature(struct thc_ipts *idev, unsigned char reportnum,
+ u8 *buf, size_t len)
+{
+ int ret = -ETIMEDOUT;
+ unsigned long timeout = msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS);
+ unsigned int seq;
+ int try;
+
+ guard(mutex)(&idev->req_lock);
+
+ /*
+ * The same report may be asked for twice in a row and the answer to
+ * the first request can still be in flight: the ID alone cannot tell
+ * the two apart, so every request carries a serial number.
+ */
+ spin_lock_irq(&idev->feature_lock);
+ WRITE_ONCE(idev->feature_done, false);
+ idev->feature_id = reportnum;
+ idev->feature_pending = true;
+ seq = ++idev->feature_seq;
+ spin_unlock_irq(&idev->feature_lock);
+
+ for (try = 0; try < THC_IPTS_REQ_TRIES; try++) {
+ ret = thc_ipts_send(idev, THC_IPTS_TX_GET_FEATURE, &reportnum,
+ 1);
+ if (ret)
+ break;
+
+ if (wait_event_timeout(idev->wq,
+ READ_ONCE(idev->feature_done) &&
+ READ_ONCE(idev->feature_ack) == seq,
+ timeout)) {
+ ret = 0;
+ break;
+ }
+
+ ret = -ETIMEDOUT;
+ }
+
+ /* Give up on the answer: a reply still in flight is dropped. */
+ spin_lock_irq(&idev->feature_lock);
+ idev->feature_pending = false;
+ spin_unlock_irq(&idev->feature_lock);
+
+ if (ret) {
+ dev_err(idev->dev, "no feature report %u after %d tries\n",
+ reportnum, try);
+ return ret;
+ }
+
+ /* The payload is the report as user space writes it: ID first. */
+ spin_lock_irq(&idev->feature_lock);
+ len = min(len, idev->feature_len);
+ if (len)
+ memcpy(buf, idev->feature, len);
+ spin_unlock_irq(&idev->feature_lock);
+
+ if (!len)
+ return -EPROTO;
+
+ return len;
+}
+
+static int thc_ipts_hid_raw_request(struct hid_device *hdev,
+ unsigned char reportnum, __u8 *buf,
+ size_t len, unsigned char rtype,
+ int reqtype)
+{
+ struct thc_ipts *idev = hdev->driver_data;
+ u32 code;
+ int ret;
+
+ if (!buf || !len)
+ return -EINVAL;
+
+ if (reqtype == HID_REQ_GET_REPORT && rtype == HID_FEATURE_REPORT)
+ return thc_ipts_get_feature(idev, reportnum, buf, len);
+
+ if (reqtype != HID_REQ_SET_REPORT ||
+ (rtype != HID_OUTPUT_REPORT && rtype != HID_FEATURE_REPORT))
+ return -EOPNOTSUPP;
+
+ code = rtype == HID_OUTPUT_REPORT ? THC_IPTS_TX_OUTPUT :
+ THC_IPTS_TX_SET_FEATURE;
+
+ guard(mutex)(&idev->req_lock);
+
+ ret = thc_ipts_send(idev, code, buf, len);
+ if (ret)
+ return ret;
+
+ /* raw_request returns the byte count, which hidraw forwards. */
+ return (int)len;
+}
+
+/*
+ * The HID core calls all four of these unconditionally, and this transport
+ * has nothing to do when a user opens or closes the report stream.
+ */
+static int thc_ipts_hid_start(struct hid_device *hdev)
+{
+ return 0;
+}
+
+static void thc_ipts_hid_stop(struct hid_device *hdev)
+{
+}
+
+static int thc_ipts_hid_open(struct hid_device *hdev)
+{
+ return 0;
+}
+
+static void thc_ipts_hid_close(struct hid_device *hdev)
+{
+}
+
+static const struct hid_ll_driver thc_ipts_ll_driver = {
+ .start = thc_ipts_hid_start,
+ .stop = thc_ipts_hid_stop,
+ .open = thc_ipts_hid_open,
+ .close = thc_ipts_hid_close,
+ .parse = thc_ipts_hid_parse,
+ .raw_request = thc_ipts_hid_raw_request,
+};
+
+static void thc_ipts_hid_remove(struct thc_ipts *idev)
+{
+ if (!READ_ONCE(idev->hid))
+ return;
+
+ hid_destroy_device(idev->hid);
+ WRITE_ONCE(idev->hid, NULL);
+}
+
+static int thc_ipts_hid_add(struct thc_ipts *idev)
+{
+ struct hid_device *hdev;
+ int ret;
+
+ hdev = hid_allocate_device();
+ if (IS_ERR(hdev))
+ return PTR_ERR(hdev);
+
+ hdev->ll_driver = &thc_ipts_ll_driver;
+ hdev->bus = BUS_PCI;
+ hdev->vendor = idev->vendor;
+ hdev->product = idev->product;
+ hdev->version = le32_to_cpu(idev->cfg.revision) & 0xffff;
+ hdev->dev.parent = idev->dev;
+ hdev->driver_data = idev;
+ snprintf(hdev->name, sizeof(hdev->name), "intel-ipts %04X:%04X",
+ idev->vendor, idev->product);
+ strscpy(hdev->phys, dev_name(idev->dev), sizeof(hdev->phys));
+
+ ret = hid_add_device(hdev);
+ if (ret) {
+ hid_destroy_device(hdev);
+ return ret;
+ }
+
+ WRITE_ONCE(idev->hid, hdev);
+
+ return 0;
+}
+
/**
* thc_ipts_port_start - bring one port up
* @idev: port to start
@@ -448,7 +762,7 @@ int thc_ipts_port_start(struct thc_ipts *idev)
thc_interrupt_enable(idev->thc, true);
- return 0;
+ return thc_ipts_hid_add(idev);
}
/**
@@ -461,26 +775,31 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
return;
/*
- * Mask the line, then wait for a handler that may be holding a
- * frame for the HID device which is about to go away.
+ * Requests first: one inside thc_dma_write needs the interrupt that
+ * completes it, so it finishes before the line is masked. A new one
+ * sees ->stopping under the same lock and backs out.
*/
- WRITE_ONCE(idev->stopping, true);
- thc_interrupt_enable(idev->thc, false);
- synchronize_irq(idev->irq);
- cancel_work_sync(&idev->recover_work);
-
- /* A handler that read ->stopping early may have unmasked. */
- thc_interrupt_enable(idev->thc, false);
-
- /* Stop the controller before the buffers it writes into go away. */
- if (idev->running)
- thc_ipts_touch_set(idev, false);
-
- idev->running = false;
- regmap_clear_bits(idev->thc->thc_regmap,
- THC_M_PRT_CONTROL_OFFSET,
- THC_M_PRT_CONTROL_DEVRST);
- thc_ipts_quiesce(idev, true);
+ scoped_guard(mutex, &idev->req_lock) {
+ WRITE_ONCE(idev->stopping, true);
+ thc_interrupt_enable(idev->thc, false);
+ synchronize_irq(idev->irq);
+ cancel_work_sync(&idev->recover_work);
+
+ /* A handler that read ->stopping early may have unmasked. */
+ thc_interrupt_enable(idev->thc, false);
+
+ /* Stop the controller before its buffers go away. */
+ if (idev->running)
+ thc_ipts_touch_set(idev, false);
+
+ idev->running = false;
+ regmap_clear_bits(idev->thc->thc_regmap,
+ THC_M_PRT_CONTROL_OFFSET,
+ THC_M_PRT_CONTROL_DEVRST);
+ thc_ipts_quiesce(idev, true);
+
+ thc_ipts_dma_stop(idev);
+ }
- thc_ipts_dma_stop(idev);
+ thc_ipts_hid_remove(idev);
}
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
index f233d9c8..7d274242 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
+++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.h
@@ -6,7 +6,10 @@
#include <linux/bits.h>
#include <linux/build_bug.h>
#include <linux/device.h>
+#include <linux/hid.h>
#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/wait.h>
@@ -43,6 +46,13 @@
#define THC_IPTS_CFG_TRIES 10
#define THC_IPTS_CFG_RETRY_MS 100
+/* A report descriptor arrives with eight bytes of preamble in front of it. */
+#define THC_IPTS_DESC_PREAMBLE 8
+
+/* Retry budget for the two handshakes user space can trigger. */
+#define THC_IPTS_REQ_TRIES 6
+#define THC_IPTS_REQ_TIMEOUT_MS 300
+
/* Bytes of the configuration block this driver interprets. */
#define THC_IPTS_CFG_KNOWN 40
@@ -105,6 +115,17 @@ static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
#define THC_IPTS_RX_HDR_SIZE 64
#define THC_IPTS_TX_HDR_SIZE 8
+/* Frame codes. The same numbers mean different things per direction. */
+#define THC_IPTS_RX_INPUT 3
+#define THC_IPTS_RX_FEATURE 4
+#define THC_IPTS_RX_DESC 5
+#define THC_IPTS_RX_RESET 7
+
+#define THC_IPTS_TX_SET_FEATURE 3
+#define THC_IPTS_TX_GET_FEATURE 4
+#define THC_IPTS_TX_OUTPUT 5
+#define THC_IPTS_TX_GET_DESC 7
+
/**
* struct thc_ipts - state of one THC port in IPTS mode
* @dev: device used for resources and logging
@@ -116,6 +137,21 @@ static_assert(offsetof(struct thc_ipts_cfg, vendor) == 0x1c);
* @max_rx: largest frame the controller may send, in bytes
* @max_tx: largest frame the driver may send, in bytes
* @rx_frame: buffer for one received frame, including its header
+ * @tx_frame: buffer for one frame to send, including its header
+ * @desc: copy of the report descriptor, past its preamble
+ * @desc_len: length of @desc
+ * @have_desc: a descriptor arrived and has not been consumed yet
+ * @feature: buffer a feature report is collected in
+ * @feature_len: length of the report in @feature
+ * @feature_id: report ID that was asked for, checked against the answer
+ * @feature_seq: serial number of the request in flight
+ * @feature_ack: serial number the accepted answer belongs to
+ * @feature_pending: a feature report was asked for and has not arrived yet
+ * @feature_done: a feature report arrived and has not been consumed yet
+ * @feature_lock: protects the feature report buffer and its flags
+ * @wq: waited on for a report descriptor or a feature report
+ * @req_lock: serialises request frames and the feature handshake
+ * @hid: HID device the reports are handed to, once it has been registered
* @recover_work: resets the receive engine after a transfer error
* @running: the controller answered and is out of reset
* @dma_ready: DMA engines are configured and may be used
@@ -132,6 +168,21 @@ struct thc_ipts {
size_t max_rx;
size_t max_tx;
u8 *rx_frame;
+ u8 *tx_frame;
+ u8 *desc;
+ size_t desc_len;
+ bool have_desc;
+ u8 *feature;
+ size_t feature_len;
+ u8 feature_id;
+ unsigned int feature_seq;
+ unsigned int feature_ack;
+ bool feature_pending;
+ bool feature_done;
+ spinlock_t feature_lock; /* feature report handshake */
+ wait_queue_head_t wq;
+ struct mutex req_lock; /* request frames */
+ struct hid_device *hid;
struct work_struct recover_work;
bool running;
bool dma_ready;
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
index 161bee9f..8f8db21d 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -45,15 +45,15 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
return -ENOMEM;
idev->dev = &pci->dev;
+ mutex_init(&idev->req_lock);
+ spin_lock_init(&idev->feature_lock);
+ init_waitqueue_head(&idev->wq);
pci_set_drvdata(pci, idev);
INIT_WORK(&idev->recover_work, thc_ipts_recover);
ret = pcim_enable_device(pci);
- if (ret) {
- dev_err(idev->dev,
- "cannot enable the PCI function, error %d\n", ret);
+ if (ret)
return ret;
- }
pci_set_master(pci);
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 3/5] HID: intel-thc-hid: intel-ipts: register the HID device
2026-11-14 21:40 ` [PATCH v4 3/5] HID: intel-thc-hid: intel-ipts: register the HID device Emfox Zhou
@ 2026-11-14 21:54 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-11-14 21:54 UTC (permalink / raw)
To: Emfox Zhou; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: broken feature_seq check in
thc_ipts_get_feature()
--
commit 933c7f8eb6c2821b64a9386be774666dc0883345
Author: Emfox Zhou <emfox@debian.org>
HID: intel-thc-hid: intel-ipts: register the HID device
This commit registers a HID device for the IPTS port and implements report
descriptor parsing, input report forwarding, and raw feature/output report
requests over DMA.
> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index b48199448dbec..80973944a8994 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> +/* The report is collected in a private buffer; the caller copies it out. */
> +static void thc_ipts_feature(struct thc_ipts *idev, const u8 *data,
> + size_t len)
> +{
> + if (!len)
> + return;
> +
> + /*
> + * A request that timed out is dropped by the sender, which may be
> + * asking for another report by now: check and copy in one step. The
> + * serial number tells an earlier answer from the one asked for.
> + */
> + spin_lock_irq(&idev->feature_lock);
> + if (!idev->feature_pending ||
> + (idev->feature_id && data[0] != idev->feature_id)) {
> + spin_unlock_irq(&idev->feature_lock);
> + return;
> + }
> +
> + len = min(len, idev->max_rx);
> + memcpy(idev->feature, data, len);
> + idev->feature_len = len;
> + idev->feature_pending = false;
> + idev->feature_ack = idev->feature_seq;
> + WRITE_ONCE(idev->feature_done, true);
> + spin_unlock_irq(&idev->feature_lock);
> +
> + wake_up(&idev->wq);
> +}
[ ... ]
> +/*
> + * A feature report is not pushed by the controller: it is asked for and comes
> + * back as a frame of its own, so the request waits for the handler.
> + */
> +static int thc_ipts_get_feature(struct thc_ipts *idev, unsigned char reportnum,
> + u8 *buf, size_t len)
> +{
> + int ret = -ETIMEDOUT;
> + unsigned long timeout = msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS);
> + unsigned int seq;
> + int try;
> +
> + guard(mutex)(&idev->req_lock);
> +
> + /*
> + * The same report may be asked for twice in a row and the answer to
> + * the first request can still be in flight: the ID alone cannot tell
> + * the two apart, so every request carries a serial number.
> + */
> + spin_lock_irq(&idev->feature_lock);
> + WRITE_ONCE(idev->feature_done, false);
> + idev->feature_id = reportnum;
> + idev->feature_pending = true;
> + seq = ++idev->feature_seq;
> + spin_unlock_irq(&idev->feature_lock);
> +
> + for (try = 0; try < THC_IPTS_REQ_TRIES; try++) {
> + ret = thc_ipts_send(idev, THC_IPTS_TX_GET_FEATURE, &reportnum,
> + 1);
> + if (ret)
> + break;
> +
> + if (wait_event_timeout(idev->wq,
> + READ_ONCE(idev->feature_done) &&
> + READ_ONCE(idev->feature_ack) == seq,
> + timeout)) {
> + ret = 0;
> + break;
> + }
> +
> + ret = -ETIMEDOUT;
> + }
[Severity: Medium]
Can idev->feature_seq distinguish a delayed reply to an earlier request from
the current request?
thc_ipts_get_feature() only transmits reportnum on the wire, and
thc_ipts_feature() unconditionally sets idev->feature_ack = idev->feature_seq
whenever a matching report ID arrives while idev->feature_pending is true.
Because idev->feature_seq is only updated under idev->req_lock at the start
of thc_ipts_get_feature(), a late reply from a timed-out request for the
same report ID will be stamped with the new request's sequence number and
consumed as the new answer.
In addition, idev->feature_done and idev->feature_pending are initialized
once before the retry loop rather than on each attempt. If
wait_event_timeout() expires on try 0 and the delayed reply arrives just as
try 1 begins, thc_ipts_feature() sets feature_done = true and
feature_pending = false before try 1 calls thc_ipts_send(). Try 1 then
sends a duplicate THC_IPTS_TX_GET_FEATURE frame before checking
feature_done, leaving an extra reply in flight (or failing the request if
thc_ipts_send() returns an error even though the reply already arrived).
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261114214026.2809740-1-emfox@debian.org?part=3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 4/5] HID: intel-thc-hid: intel-ipts: handle system sleep
2026-11-14 21:40 [PATCH v4 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports Emfox Zhou
` (2 preceding siblings ...)
2026-11-14 21:40 ` [PATCH v4 3/5] HID: intel-thc-hid: intel-ipts: register the HID device Emfox Zhou
@ 2026-11-14 21:40 ` Emfox Zhou
2026-11-14 21:40 ` [PATCH v4 5/5] HID: intel-thc-hid: intel-ipts: document the IPTS mode Emfox Zhou
4 siblings, 0 replies; 8+ messages in thread
From: Emfox Zhou @ 2026-11-14 21:40 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
System sleep takes the port down and builds it up again rather than
keeping the HID device across it: the controller forgets the streaming
mode user space asked it for, and hidraw offers no way to tell user
space to ask again. The device goes away and comes back, so the daemon
watching it restarts and repeats the request.
Signed-off-by: Emfox Zhou <emfox@debian.org>
---
.../hid/intel-thc-hid/intel-ipts/pci-ipts.c | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
index 8f8db21d..81a46149 100644
--- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
+++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
@@ -142,6 +142,41 @@ static void thc_ipts_remove(struct pci_dev *pci)
pci_clear_master(pci);
}
+/*
+ * The controller forgets the streaming mode and hidraw cannot ask user space
+ * to repeat itself, so the port is taken down and rebuilt: the HID device
+ * comes back and the daemon restarts with it.
+ */
+static int thc_ipts_suspend(struct device *dev)
+{
+ struct thc_ipts *idev = dev_get_drvdata(dev);
+
+ if (!idev)
+ return -ENODEV;
+
+ thc_ipts_port_stop(idev);
+
+ return 0;
+}
+
+static int thc_ipts_resume(struct device *dev)
+{
+ struct thc_ipts *idev = dev_get_drvdata(dev);
+ int ret;
+
+ if (!idev)
+ return -ENODEV;
+
+ ret = thc_ipts_port_start(idev);
+ if (ret)
+ thc_ipts_port_stop(idev);
+
+ return ret;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(thc_ipts_pm_ops, thc_ipts_suspend,
+ thc_ipts_resume);
+
static void thc_ipts_shutdown(struct pci_dev *pci)
{
struct thc_ipts *idev = pci_get_drvdata(pci);
@@ -156,6 +191,7 @@ static struct pci_driver thc_ipts_driver = {
.probe = thc_ipts_probe,
.remove = thc_ipts_remove,
.shutdown = thc_ipts_shutdown,
+ .driver.pm = pm_ptr(&thc_ipts_pm_ops),
/* Bring-up waits for the controller, up to a few seconds. */
.driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
};
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v4 5/5] HID: intel-thc-hid: intel-ipts: document the IPTS mode
2026-11-14 21:40 [PATCH v4 0/5] HID: intel-thc-hid: intel-ipts: support THC IPTS ports Emfox Zhou
` (3 preceding siblings ...)
2026-11-14 21:40 ` [PATCH v4 4/5] HID: intel-thc-hid: intel-ipts: handle system sleep Emfox Zhou
@ 2026-11-14 21:40 ` Emfox Zhou
4 siblings, 0 replies; 8+ messages in thread
From: Emfox Zhou @ 2026-11-14 21:40 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
The THC documentation only covers HIDSPI and HIDI2C. Ports from before
Meteor Lake run in a third mode, and the intel-ipts driver is
written against behaviour that is not described anywhere in tree.
Record what the driver depends on: the layout of the configuration
block, the frame codes and header sizes, the bulk data window, and the
order in which quiesce, interrupt configuration and the DMA engines
have to be set up.
Signed-off-by: Emfox Zhou <emfox@debian.org>
---
Documentation/hid/intel-thc-hid.rst | 95 ++++++++++++++++++++++++++++-
1 file changed, 94 insertions(+), 1 deletion(-)
diff --git a/Documentation/hid/intel-thc-hid.rst b/Documentation/hid/intel-thc-hid.rst
index 8b378c57..0f6328aa 100644
--- a/Documentation/hid/intel-thc-hid.rst
+++ b/Documentation/hid/intel-thc-hid.rst
@@ -590,7 +590,100 @@ To debug THC, event tracing mechanism is used. To enable debug logs::
echo 1 > /sys/kernel/debug/tracing/events/intel_thc/enable
cat /sys/kernel/debug/tracing/trace
-7. Reference
+7. IPTS Mode
+============
+
+THC also supports IPTS mode, the private Intel Precise Touch and Stylus
+protocol. The port drives the touch controller over plain SPI and moves the
+data through the same DMA engines, but the framing is private and the
+parameters come from the controller itself instead of ACPI. The
+``intel-ipts`` module handles ports in that mode, which is what Tiger Lake,
+Lakefield, Alder Lake and Raptor Lake hardware use.
+
+This section describes how the driver uses those ports. It is not a
+specification: everything below comes from observing the controller and is
+limited to what this driver relies on.
+
+7.1 Configuration Block
+-----------------------
+
+At SPI address 0 the controller exposes a 64 byte block. The driver reads it
+over the PIO path after releasing the controller from reset, and retries
+until the tag ``$TIC`` (``0x43495424``) appears, because the block is only
+partially filled while the controller is still starting.
+
+Fields the driver uses:
+
+====== ==========================================================
+Offset Meaning
+====== ==========================================================
+0x00 pending interrupt causes
+0x08 buffer sizes, ``(size / 64) - 1``; Rx bits 13:0, Tx 23:14
+0x0c touch engine control, writable
+0x14 controller tag, ``$TIC``
+0x18 SPI timing the controller asks for
+0x1c vendor and product ID reported to HID
+0x20 product revision
+0x24 firmware version word
+====== ==========================================================
+
+The touch engine control word carries an enable bit, a bit asking for HID
+reports instead of raw data, and a three bit power state. The driver enables
+it with reports and full power, and clears it again when the port is shut
+down.
+
+7.2 Frames
+----------
+
+Both directions carry a code and a length in front of the payload. Frames
+from the controller use a 64 byte header, frames to it an 8 byte header, and
+only the latter has the payload padded to a four byte boundary.
+
+==== ========================== =======================
+Code Received from controller Sent to controller
+==== ========================== =======================
+3 input report set feature
+4 feature report get feature
+5 report descriptor output report
+7 reset request get report descriptor
+==== ========================== =======================
+
+A report descriptor comes back with eight bytes of preamble that the driver
+skips.
+
+7.3 Data Window
+---------------
+
+Bulk transfers go through address ``0x1000`` of the controller, not through
+address 0. The DMA engines have to be pointed there explicitly, otherwise
+they write into the configuration block and the controller drops the frames
+without reporting an error. The input header address is zero: that is where
+the controller writes the interrupt cause word, at the start of the
+configuration block.
+
+7.4 Enabling and Flow Control
+-----------------------------
+
+The port has to be released from quiesce before the controller will send
+anything: quiescing gates the whole data path, not just the interrupt. The
+interrupt configuration programs the interrupt type field in the device to
+the invalid value, and starting the receive engine then replaces that field
+with the type it expects, so the two have to happen in that order.
+
+The controller starts streaming once user space asks for it, through a set
+feature request on report 5. The driver does not do that on its own; the
+daemon that opens hidraw sends it.
+
+7.5 What Is Left to User Space
+------------------------------
+
+The reports carry heatmap data, and turning that into contacts is a
+substantial image-processing job that already exists in user space. The
+driver therefore stops at the HID layer: the report descriptor comes from
+the controller and is not written by the driver, the report stream is
+available through hidraw, and ``iptsd`` decodes it into input events.
+
+8. Reference
============
- HIDSPI: https://download.microsoft.com/download/c/a/0/ca07aef3-3e10-4022-b1e9-c98cea99465d/HidSpiProtocolSpec.pdf
- HIDI2C: https://download.microsoft.com/download/7/d/d/7dd44bb7-2a7a-4505-ac1c-7227d3d96d5b/hid-over-i2c-protocol-spec-v1-0.docx
^ permalink raw reply related [flat|nested] 8+ messages in thread