* [PATCH v6 00/11] Add spi-hid transport driver
@ 2026-10-19 19:01 Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 01/11] Documentation: Correction in HID output_report callback description Jingyuan Liang
` (10 more replies)
0 siblings, 11 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Jarrett Schultz, Dmitry Antipov, Angela Czubak
This series picks up the spi-hid driver work originally started by
Microsoft. The patch breakdown has been modified and the implementation
has been refactored to address upstream feedback and testing issues. We
are submitting this as a new series while keeping the original sign-off
chain to reflect the history.
Same as the original series, there is a change to HID documentation, some
HID core changes to support a SPI device, the SPI HID transport driver,
and HID over SPI Device Tree binding. We have added the HID over SPI ACPI
support, power management, panel follower, and quirks for Ilitek touch
controllers.
Original authors: Jarrett Schultz <jaschultz@microsoft.com>,
Dmitry Antipov <dmanti@microsoft.com>
Link: https://lore.kernel.org/r/86b63b7b-afda-d7f4-7bfa-175085d5a8ef@gmail.com
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
Changes in v6:
- Take output_lock while reallocating buffers in ll_start()
- Don't drop input reports while a refreshed device probes
- OF: drop mod_devicetable.h, use named initializers, fix alignment
- Move the dt-bindings patch before the OF driver patch
- Drop Tested-by from the documentation and DT binding patches
- Link to v5: https://lore.kernel.org/r/20261009-send-upstream-v5-0-384af01da3ee@chromium.org
Changes in v5:
- Fix request/response races; match GET_FEATURE responses by report ID
- Reset the device on response timeout; wait uninterruptibly
- Validate device descriptor and report lengths; fix buffer sizing
- Honor rtype in raw_request(); fix GET_REPORT off-by-one
- Use hid_safe_input_report(); forward input reports only while open
- Disable IRQ around hid_device teardown; fix a sparse warning
- Use disable_work_sync() for reset_work in remove()
- Cap trace copies at 64 bytes; cacheline-align read approval buffers
- ACPI: select RESET_CONTROLLER, use device_reset_optional()
- OF & DT: read opcodes as u8, use dev_err_probe(), require reg
- PM: cancel reset_work on suspend, wait for reset before
reset_resume(), fix IRQ imbalance on resume failure
- Panel follower: fix regulator/IRQ imbalance; depend on DRM || !DRM
- Link to v4: https://lore.kernel.org/r/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org
Changes in v4:
- Extended io_lock scope to protect the shid->hid pointer lifecycle
against races with the IRQ handler
- Cacheline-aligned DMA buffers, enforced 4-byte alignment
- Added error rollback for failed suspend/resume transitions
- Moved IRQ request to probe with IRQF_NO_AUTOEN (disabled by default)
- DT Bindings & OF Driver:
- Required a device-specific compatible string in the schema.
- Added description to explain why opcodes and addresses properties
need to be defined in the schema
- Added `spi-peripheral-props.yaml` reference and switched to
`unevaluatedProperties: false`.
- Added fallback to default timing parameters in OF driver if match
data is missing.
- Link to v3: https://lore.kernel.org/r/20260402-send-upstream-v3-0-6091c458d357@chromium.org
Changes in v3:
- Add io_lock init
- Relocate tracepoints to drivers/hid/spi-hid/ and fix tracepoint macros
- Add tracepoints for sync, error handling, reset, and report processing
- Clean up internal includes and fix Makefile CFLAGS
- Add more details in v2 changelog
- Link to v2: https://lore.kernel.org/r/20260324-send-upstream-v2-0-521ce8afff86@chromium.org
Changes in v2:
- Clean up DT bindings: fix formatting and remove timing and flags properties
- Update DT binding example: use a device-specific compatible and drop
reset_assert
- Simplify ACPI/OF match tables by removing ACPI_PTR/of_match_ptr
- Refactor OF driver to use match data for timing parameters instead
of DT properties
- Switch to fsleep() for delays in ACPI and OF drivers
- Drop patch 12 as it is vendor specific
- Add a lock to fix input/output concurrency race
- Link to v1: https://lore.kernel.org/r/20260303-send-upstream-v1-0-1515ba218f3d@chromium.org
---
Angela Czubak (2):
HID: spi-hid: add transport driver skeleton for HID over SPI bus
HID: spi-hid: add ACPI support for HID over SPI
Jarrett Schultz (3):
Documentation: Correction in HID output_report callback description.
HID: Add BUS_SPI support and define HID_SPI_DEVICE macro
HID: spi-hid: add device tree support for HID over SPI
Jingyuan Liang (6):
HID: spi-hid: add spi-hid driver HID layer
HID: spi-hid: add HID SPI protocol implementation
HID: spi-hid: add spi_hid traces
dt-bindings: input: Document hid-over-spi DT schema
HID: spi-hid: add power management implementation
HID: spi-hid: add panel follower support
.../devicetree/bindings/input/hid-over-spi.yaml | 130 ++
Documentation/hid/hid-transport.rst | 4 +-
drivers/hid/Kconfig | 2 +
drivers/hid/Makefile | 2 +
drivers/hid/hid-core.c | 3 +
drivers/hid/spi-hid/Kconfig | 50 +
drivers/hid/spi-hid/Makefile | 12 +
drivers/hid/spi-hid/spi-hid-acpi.c | 262 +++
drivers/hid/spi-hid/spi-hid-core.c | 1757 ++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-core.h | 118 ++
drivers/hid/spi-hid/spi-hid-of.c | 242 +++
drivers/hid/spi-hid/spi-hid-trace.h | 184 ++
drivers/hid/spi-hid/spi-hid.h | 46 +
include/linux/hid.h | 2 +
14 files changed, 2812 insertions(+), 2 deletions(-)
---
base-commit: ee9c669f9bf5fd2c24206746ded9382fe810df89
change-id: 20260212-send-upstream-75f6fd9ed92e
Best regards,
--
Jingyuan Liang <jingyliang@chromium.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 01/11] Documentation: Correction in HID output_report callback description.
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro Jingyuan Liang
` (9 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Jarrett Schultz, Dmitry Antipov
From: Jarrett Schultz <jaschultz@microsoft.com>
Originally output_report callback was described as must-be asynchronous,
but that is not the case in some implementations, namely i2c-hid.
Correct the documentation to say that it may be asynchronous.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
Documentation/hid/hid-transport.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/hid/hid-transport.rst b/Documentation/hid/hid-transport.rst
index 6f1692da296c..2008cf432af1 100644
--- a/Documentation/hid/hid-transport.rst
+++ b/Documentation/hid/hid-transport.rst
@@ -327,8 +327,8 @@ The available HID callbacks are:
Send raw output report via intr channel. Used by some HID device drivers
which require high throughput for outgoing requests on the intr channel. This
- must not cause SET_REPORT calls! This must be implemented as asynchronous
- output report on the intr channel!
+ must not cause SET_REPORT calls! This call might be asynchronous, so the
+ caller should not expect an immediate response!
::
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 01/11] Documentation: Correction in HID output_report callback description Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus Jingyuan Liang
` (8 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Jarrett Schultz, Dmitry Antipov
From: Jarrett Schultz <jaschultz@microsoft.com>
If connecting a hid_device with bus field indicating BUS_SPI print out
"SPI" in the debug print.
Macro sets the bus field to BUS_SPI and uses arguments to set vendor
product fields.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/hid-core.c | 3 +++
include/linux/hid.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a3ff0514f9cd..1b9dbb7515ad 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2373,6 +2373,9 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
case BUS_I2C:
bus = "I2C";
break;
+ case BUS_SPI:
+ bus = "SPI";
+ break;
case BUS_SDW:
bus = "SOUNDWIRE";
break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 8d17b741638c..45f713fe9982 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -821,6 +821,8 @@ struct hid_descriptor {
.bus = BUS_BLUETOOTH, .vendor = (ven), .product = (prod)
#define HID_I2C_DEVICE(ven, prod) \
.bus = BUS_I2C, .vendor = (ven), .product = (prod)
+#define HID_SPI_DEVICE(ven, prod) \
+ .bus = BUS_SPI, .vendor = (ven), .product = (prod)
#define HID_REPORT_ID(rep) \
.report_type = (rep)
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 01/11] Documentation: Correction in HID output_report callback description Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:11 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer Jingyuan Liang
` (7 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Angela Czubak, Dmitry Antipov
From: Angela Czubak <acz@semihalf.com>
Create spi-hid folder and add Kconfig and Makefile for spi-hid driver.
Add basic device structure, definitions, and probe/remove functions.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/Kconfig | 2 +
drivers/hid/Makefile | 2 +
drivers/hid/spi-hid/Kconfig | 15 +++
drivers/hid/spi-hid/Makefile | 9 ++
drivers/hid/spi-hid/spi-hid-core.c | 213 +++++++++++++++++++++++++++++++++++++
5 files changed, 241 insertions(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index c43e824428f0..17a42301875c 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1527,6 +1527,8 @@ source "drivers/hid/bpf/Kconfig"
source "drivers/hid/i2c-hid/Kconfig"
+source "drivers/hid/spi-hid/Kconfig"
+
source "drivers/hid/intel-ish-hid/Kconfig"
source "drivers/hid/amd-sfh-hid/Kconfig"
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 48a863b245ee..b5a7ef7f208f 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -177,6 +177,8 @@ obj-$(CONFIG_USB_KBD) += usbhid/
obj-$(CONFIG_I2C_HID_CORE) += i2c-hid/
+obj-$(CONFIG_SPI_HID_CORE) += spi-hid/
+
obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
obj-$(CONFIG_AMD_SFH_HID) += amd-sfh-hid/
diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
new file mode 100644
index 000000000000..836fdefe8345
--- /dev/null
+++ b/drivers/hid/spi-hid/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Copyright (c) 2021 Microsoft Corporation
+#
+
+menuconfig SPI_HID
+ tristate "SPI HID support"
+ default y
+ depends on SPI
+
+if SPI_HID
+
+config SPI_HID_CORE
+ tristate
+endif
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
new file mode 100644
index 000000000000..92e24cddbfc2
--- /dev/null
+++ b/drivers/hid/spi-hid/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the SPI HID input drivers
+#
+# Copyright (c) 2021 Microsoft Corporation
+#
+
+obj-$(CONFIG_SPI_HID_CORE) += spi-hid.o
+spi-hid-objs = spi-hid-core.o
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
new file mode 100644
index 000000000000..02a7608c4b88
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol implementation
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ *
+ * This code is partly based on "HID over I2C protocol implementation:
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * which in turn is partly based on "USB HID support for Linux":
+ *
+ * Copyright (c) 1999 Andreas Gal
+ * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ * Copyright (c) 2007-2008 Oliver Neukum
+ * Copyright (c) 2006-2010 Jiri Kosina
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/hid-over-spi.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+/* struct spi_hid_conf - Conf provided to the core */
+struct spi_hid_conf {
+ u32 input_report_header_address;
+ u32 input_report_body_address;
+ u32 output_report_address;
+ u8 read_opcode;
+ u8 write_opcode;
+};
+
+/**
+ * struct spihid_ops - Ops provided to the core
+ * @power_up: do sequencing to power up the device
+ * @power_down: do sequencing to power down the device
+ * @assert_reset: do sequencing to assert the reset line
+ * @deassert_reset: do sequencing to deassert the reset line
+ * @sleep_minimal_reset_delay: minimal sleep delay during reset
+ */
+struct spihid_ops {
+ int (*power_up)(struct spihid_ops *ops);
+ int (*power_down)(struct spihid_ops *ops);
+ int (*assert_reset)(struct spihid_ops *ops);
+ int (*deassert_reset)(struct spihid_ops *ops);
+ void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
+};
+
+/* Driver context */
+struct spi_hid {
+ struct spi_device *spi; /* spi device. */
+ struct hid_device *hid; /* pointer to corresponding HID dev. */
+
+ struct spihid_ops *ops;
+ struct spi_hid_conf *conf;
+
+ enum hidspi_power_state power_state;
+
+ u32 regulator_error_count;
+ int regulator_last_error;
+ u32 bus_error_count;
+ int bus_last_error;
+ u32 dir_count; /* device initiated reset count. */
+};
+
+static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state)
+{
+ switch (power_state) {
+ case HIDSPI_ON:
+ return "d0";
+ case HIDSPI_SLEEP:
+ return "d2";
+ case HIDSPI_OFF:
+ return "d3";
+ default:
+ return "unknown";
+ }
+}
+
+static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
+{
+ return IRQ_HANDLED;
+}
+
+static ssize_t bus_error_count_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%u (%d)\n",
+ shid->bus_error_count, shid->bus_last_error);
+}
+static DEVICE_ATTR_RO(bus_error_count);
+
+static ssize_t regulator_error_count_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%u (%d)\n",
+ shid->regulator_error_count,
+ shid->regulator_last_error);
+}
+static DEVICE_ATTR_RO(regulator_error_count);
+
+static ssize_t device_initiated_reset_count_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%u\n", shid->dir_count);
+}
+static DEVICE_ATTR_RO(device_initiated_reset_count);
+
+static struct attribute *spi_hid_attrs[] = {
+ &dev_attr_bus_error_count.attr,
+ &dev_attr_regulator_error_count.attr,
+ &dev_attr_device_initiated_reset_count.attr,
+ NULL /* Terminator */
+};
+
+static const struct attribute_group spi_hid_group = {
+ .attrs = spi_hid_attrs,
+};
+
+const struct attribute_group *spi_hid_groups[] = {
+ &spi_hid_group,
+ NULL
+};
+EXPORT_SYMBOL_GPL(spi_hid_groups);
+
+int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
+ struct spi_hid_conf *conf)
+{
+ struct device *dev = &spi->dev;
+ struct spi_hid *shid;
+ int error;
+
+ if (spi->irq <= 0)
+ return dev_err_probe(dev, spi->irq ?: -EINVAL, "Missing IRQ\n");
+
+ shid = devm_kzalloc(dev, sizeof(*shid), GFP_KERNEL);
+ if (!shid)
+ return -ENOMEM;
+
+ shid->spi = spi;
+ shid->power_state = HIDSPI_ON;
+ shid->ops = ops;
+ shid->conf = conf;
+
+ spi_set_drvdata(spi, shid);
+
+ /*
+ * At the end of probe we initialize the device:
+ * 0) assert reset, bias the interrupt line
+ * 1) sleep minimal reset delay
+ * 2) request IRQ
+ * 3) power up the device
+ * 4) deassert reset (high)
+ * After this we expect an IRQ with a reset response.
+ */
+
+ shid->ops->assert_reset(shid->ops);
+
+ shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+ error = devm_request_threaded_irq(dev, spi->irq, NULL, spi_hid_dev_irq,
+ IRQF_ONESHOT, dev_name(&spi->dev), shid);
+ if (error) {
+ dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
+ return error;
+ }
+
+ error = shid->ops->power_up(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: could not power up\n", __func__);
+ return error;
+ }
+
+ shid->ops->deassert_reset(shid->ops);
+
+ dev_dbg(dev, "%s: d3 -> %s\n", __func__,
+ spi_hid_power_mode_string(shid->power_state));
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spi_hid_core_probe);
+
+void spi_hid_core_remove(struct spi_device *spi)
+{
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ int error;
+
+ shid->ops->assert_reset(shid->ops);
+ error = shid->ops->power_down(shid->ops);
+ if (error)
+ dev_err(dev, "failed to disable regulator\n");
+}
+EXPORT_SYMBOL_GPL(spi_hid_core_remove);
+
+MODULE_DESCRIPTION("HID over SPI transport driver");
+MODULE_AUTHOR("Dmitry Antipov <dmanti@microsoft.com>");
+MODULE_LICENSE("GPL");
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (2 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:26 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
` (6 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Dmitry Antipov, Angela Czubak
Add HID low level driver callbacks to register SPI as a HID driver, and
an external touch device as a HID device.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/spi-hid-core.c | 592 +++++++++++++++++++++++++++++++++++++
1 file changed, 592 insertions(+)
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 02a7608c4b88..fca7a44eeb9f 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -20,13 +20,69 @@
* Copyright (c) 2006-2010 Jiri Kosina
*/
+#include <linux/completion.h>
+#include <linux/crc32.h>
#include <linux/device.h>
+#include <linux/err.h>
#include <linux/hid.h>
#include <linux/hid-over-spi.h>
#include <linux/interrupt.h>
+#include <linux/jiffies.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/unaligned.h>
+
+#define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST 0x00
+
+#define SPI_HID_RESP_TIMEOUT 1000
+
+/* Protocol message size constants */
+#define SPI_HID_OUTPUT_HEADER_LEN 8
+
+/* flags */
+/*
+ * ready flag indicates that the FW is ready to accept commands and
+ * requests. The FW becomes ready after sending the report descriptor.
+ */
+#define SPI_HID_READY 0
+
+/* Raw input buffer with data from the bus */
+struct spi_hid_input_buf {
+ u8 header[HIDSPI_INPUT_HEADER_SIZE];
+ u8 body[HIDSPI_INPUT_BODY_HEADER_SIZE];
+ u8 content[];
+};
+
+/* Raw output report buffer to be put on the bus */
+struct spi_hid_output_buf {
+ u8 header[SPI_HID_OUTPUT_HEADER_LEN];
+ u8 content[];
+};
+
+/* Data necessary to send an output report */
+struct spi_hid_output_report {
+ u8 report_type;
+ u16 content_length;
+ u8 content_id;
+ u8 *content;
+};
+
+/* Processed data from a device descriptor */
+struct spi_hid_device_descriptor {
+ u16 hid_version;
+ u16 report_descriptor_length;
+ u16 max_input_length;
+ u16 max_output_length;
+ u16 max_fragment_length;
+ u16 vendor_id;
+ u16 product_id;
+ u16 version_id;
+ u8 no_output_report_ack;
+};
/* struct spi_hid_conf - Conf provided to the core */
struct spi_hid_conf {
@@ -61,8 +117,26 @@ struct spi_hid {
struct spihid_ops *ops;
struct spi_hid_conf *conf;
+ struct spi_hid_device_descriptor desc; /* HID device descriptor. */
+ struct spi_hid_output_buf *output; /* Output buffer. */
+ struct spi_hid_input_buf *input; /* Input buffer. */
+ struct spi_hid_input_buf *response; /* Response buffer. */
+
+ u16 response_length;
+ u16 bufsize;
+
enum hidspi_power_state power_state;
+ u8 reset_attempts; /* The number of reset attempts. */
+
+ unsigned long flags; /* device flags. */
+
+ /* Control lock to make sure one output transaction at a time. */
+ struct mutex output_lock;
+ struct completion output_done;
+
+ u32 report_descriptor_crc32; /* HID report descriptor crc32 checksum. */
+
u32 regulator_error_count;
int regulator_last_error;
u32 bus_error_count;
@@ -70,6 +144,33 @@ struct spi_hid {
u32 dir_count; /* device initiated reset count. */
};
+static struct hid_ll_driver spi_hid_ll_driver;
+
+static void spi_hid_populate_output_header(u8 *buf,
+ const struct spi_hid_conf *conf,
+ const struct spi_hid_output_report *report)
+{
+ buf[0] = conf->write_opcode;
+ put_unaligned_be24(conf->output_report_address, &buf[1]);
+ buf[4] = report->report_type;
+ put_unaligned_le16(report->content_length, &buf[5]);
+ buf[7] = report->content_id;
+}
+
+static int spi_hid_output(struct spi_hid *shid, const void *buf, u16 length)
+{
+ int error;
+
+ error = spi_write(shid->spi, buf, length);
+
+ if (error) {
+ shid->bus_error_count++;
+ shid->bus_last_error = error;
+ }
+
+ return error;
+}
+
static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state)
{
switch (power_state) {
@@ -84,11 +185,484 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
}
}
+static void spi_hid_stop_hid(struct spi_hid *shid)
+{
+ struct hid_device *hid = shid->hid;
+
+ shid->hid = NULL;
+ clear_bit(SPI_HID_READY, &shid->flags);
+
+ if (hid)
+ hid_destroy_device(hid);
+}
+
+static int __spi_hid_send_output_report(struct spi_hid *shid,
+ struct spi_hid_output_report *report)
+{
+ struct spi_hid_output_buf *buf = shid->output;
+ struct device *dev = &shid->spi->dev;
+ u16 report_length;
+ u16 padded_length;
+ u8 padding;
+ int error;
+
+ if (report->content_length > shid->desc.max_output_length ||
+ report->content_length > shid->bufsize) {
+ dev_err(dev, "Output report too big, content_length 0x%x\n",
+ report->content_length);
+ return -E2BIG;
+ }
+
+ spi_hid_populate_output_header(buf->header, shid->conf, report);
+
+ if (report->content_length)
+ memcpy(&buf->content, report->content, report->content_length);
+
+ report_length = sizeof(buf->header) + report->content_length;
+ padded_length = round_up(report_length, 4);
+ padding = padded_length - report_length;
+ memset(&buf->content[report->content_length], 0, padding);
+
+ error = spi_hid_output(shid, buf, padded_length);
+ if (error)
+ dev_err(dev, "Failed output transfer: %d\n", error);
+
+ return error;
+}
+
+static int spi_hid_send_output_report(struct spi_hid *shid,
+ struct spi_hid_output_report *report)
+{
+ guard(mutex)(&shid->output_lock);
+ return __spi_hid_send_output_report(shid, report);
+}
+
+static int __spi_hid_sync_request(struct spi_hid *shid,
+ struct spi_hid_output_report *report)
+{
+ struct device *dev = &shid->spi->dev;
+ int error;
+
+ reinit_completion(&shid->output_done);
+
+ error = __spi_hid_send_output_report(shid, report);
+ if (error)
+ return error;
+
+ error = wait_for_completion_interruptible_timeout(&shid->output_done,
+ msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
+ if (error == 0) {
+ dev_err(dev, "Response timed out\n");
+ return -ETIMEDOUT;
+ }
+ if (error < 0)
+ return error;
+
+ return 0;
+}
+
+static int spi_hid_sync_request(struct spi_hid *shid,
+ struct spi_hid_output_report *report)
+{
+ guard(mutex)(&shid->output_lock);
+ return __spi_hid_sync_request(shid, report);
+}
+
+/*
+ * This function returns the length of the report descriptor, or a negative
+ * error code if something went wrong.
+ */
+static int spi_hid_report_descriptor_request(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_output_report report = {
+ .report_type = REPORT_DESCRIPTOR,
+ .content_length = 0,
+ .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
+ .content = NULL,
+ };
+ int ret;
+
+ ret = spi_hid_sync_request(shid, &report);
+ if (ret) {
+ dev_err(dev,
+ "Expected report descriptor not received: %d\n", ret);
+ return ret;
+ }
+
+ ret = shid->response_length;
+ if (ret != shid->desc.report_descriptor_length) {
+ ret = min_t(unsigned int, ret, shid->desc.report_descriptor_length);
+ dev_err(dev, "Received report descriptor length doesn't match device descriptor field, using min of the two: %d\n",
+ ret);
+ }
+
+ return ret;
+}
+
+static int spi_hid_create_device(struct spi_hid *shid)
+{
+ struct hid_device *hid;
+ struct device *dev = &shid->spi->dev;
+ int error;
+
+ hid = hid_allocate_device();
+ error = PTR_ERR_OR_ZERO(hid);
+ if (error) {
+ dev_err(dev, "Failed to allocate hid device: %d\n", error);
+ return error;
+ }
+
+ hid->driver_data = shid->spi;
+ hid->ll_driver = &spi_hid_ll_driver;
+ hid->dev.parent = &shid->spi->dev;
+ hid->bus = BUS_SPI;
+ hid->version = shid->desc.hid_version;
+ hid->vendor = shid->desc.vendor_id;
+ hid->product = shid->desc.product_id;
+
+ snprintf(hid->name, sizeof(hid->name), "spi %04X:%04X",
+ hid->vendor, hid->product);
+ strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
+
+ shid->hid = hid;
+
+ error = hid_add_device(hid);
+ if (error) {
+ dev_err(dev, "Failed to add hid device: %d\n", error);
+ /*
+ * We likely got here because report descriptor request timed
+ * out. Let's disconnect and destroy the hid_device structure.
+ */
+ spi_hid_stop_hid(shid);
+ return error;
+ }
+
+ return 0;
+}
+
+static int spi_hid_get_request(struct spi_hid *shid, u8 content_id,
+ u8 *buf, size_t len)
+{
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_output_report report = {
+ .report_type = GET_FEATURE,
+ .content_length = 0,
+ .content_id = content_id,
+ .content = NULL,
+ };
+ u16 content_len;
+ int error, ret;
+
+ /* buf[0] holds the report ID, so there must be room for at least that. */
+ if (!len)
+ return -EINVAL;
+
+ guard(mutex)(&shid->output_lock);
+
+ error = __spi_hid_sync_request(shid, &report);
+ if (error) {
+ dev_err(dev,
+ "Expected get request response not received! Error %d\n",
+ error);
+ return error;
+ }
+
+ /* Content Length counts only the payload, not the Content ID. */
+ content_len = get_unaligned_le16(&shid->response->body[1]);
+ if (content_len > shid->bufsize) {
+ dev_err(dev, "Get report response too big: %u > %u\n",
+ content_len, shid->bufsize);
+ return -EPROTO;
+ }
+
+ /* buf[0] holds the report ID, the payload follows. */
+ ret = min_t(size_t, len - 1, content_len);
+ buf[0] = shid->response->body[3];
+ memcpy(&buf[1], shid->response->content, ret);
+
+ return ret + 1;
+}
+
+static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
+ u8 content_id)
+{
+ struct spi_hid_output_report report = {
+ .report_type = SET_FEATURE,
+ .content_length = arg_len,
+ .content_id = content_id,
+ .content = arg_buf,
+ };
+
+ return spi_hid_sync_request(shid, &report);
+}
+
+/* This is a placeholder. Will be implemented in the next patch. */
static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
{
return IRQ_HANDLED;
}
+static int spi_hid_alloc_buffers(struct spi_hid *shid, size_t report_size)
+{
+ struct device *dev = &shid->spi->dev;
+ int inbufsize = round_up(sizeof(shid->input->header) +
+ sizeof(shid->input->body) + report_size, 4);
+ int outbufsize = round_up(sizeof(shid->output->header) + report_size, 4);
+ void *tmp;
+
+ tmp = devm_krealloc(dev, shid->output, outbufsize, GFP_KERNEL | __GFP_ZERO);
+ if (!tmp)
+ return -ENOMEM;
+ shid->output = tmp;
+
+ tmp = devm_krealloc(dev, shid->input, inbufsize, GFP_KERNEL | __GFP_ZERO);
+ if (!tmp)
+ return -ENOMEM;
+ shid->input = tmp;
+
+ tmp = devm_krealloc(dev, shid->response, inbufsize, GFP_KERNEL | __GFP_ZERO);
+ if (!tmp)
+ return -ENOMEM;
+ shid->response = tmp;
+
+ if (!shid->output || !shid->input || !shid->response)
+ return -ENOMEM;
+
+ shid->bufsize = report_size;
+
+ return 0;
+}
+
+static int spi_hid_get_report_length(struct hid_report *report)
+{
+ return DIV_ROUND_UP(report->size, 8) +
+ report->device->report_enum[report->type].numbered + 2;
+}
+
+/*
+ * Traverse the supplied list of reports and find the longest
+ */
+static void spi_hid_find_max_report(struct hid_device *hid, u32 type,
+ u16 *max)
+{
+ struct hid_report *report;
+ u16 size;
+
+ /*
+ * We should not rely on wMaxInputLength, as some devices may set it to
+ * a wrong length.
+ */
+ list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
+ size = spi_hid_get_report_length(report);
+ if (*max < size)
+ *max = size;
+ }
+}
+
+/* hid_ll_driver interface functions */
+
+static int spi_hid_ll_start(struct hid_device *hid)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ int error = 0;
+ /*
+ * HID_MIN_BUFFER_SIZE is the minimum transport buffer size, not a
+ * requirement on the device's report sizes. Use it as a floor so
+ * devices with small reports are still supported.
+ */
+ u16 bufsize = HID_MIN_BUFFER_SIZE;
+
+ spi_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
+ spi_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
+ spi_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
+
+ if (bufsize > shid->bufsize) {
+ guard(disable_irq)(&shid->spi->irq);
+
+ error = spi_hid_alloc_buffers(shid, bufsize);
+ if (error)
+ return error;
+ }
+
+ return 0;
+}
+
+static void spi_hid_ll_stop(struct hid_device *hid)
+{
+ hid->claimed = 0;
+}
+
+static int spi_hid_ll_open(struct hid_device *hid)
+{
+ return 0;
+}
+
+static void spi_hid_ll_close(struct hid_device *hid)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+
+ shid->reset_attempts = 0;
+}
+
+static int spi_hid_ll_power(struct hid_device *hid, int level)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ int error = 0;
+
+ guard(mutex)(&shid->output_lock);
+ if (!shid->hid)
+ error = -ENODEV;
+
+ return error;
+}
+
+static int spi_hid_ll_parse(struct hid_device *hid)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ unsigned int rsize = shid->desc.report_descriptor_length;
+ int error, len;
+
+ if (rsize > HID_MAX_DESCRIPTOR_SIZE) {
+ dev_err(dev,
+ "Report descriptor size %d is greater than HID_MAX_DESCRIPTOR_SIZE %d\n",
+ rsize, HID_MAX_DESCRIPTOR_SIZE);
+ return -EINVAL;
+ }
+
+ if (rsize > shid->bufsize) {
+ error = spi_hid_alloc_buffers(shid, rsize);
+ if (error)
+ return error;
+ }
+
+ len = spi_hid_report_descriptor_request(shid);
+ if (len < 0) {
+ dev_err(dev, "Report descriptor request failed, %d\n", len);
+ return len;
+ }
+
+ /*
+ * FIXME: below call returning 0 doesn't mean that the report descriptor
+ * is good. We might be caching a crc32 of a corrupted r. d. or who
+ * knows what the FW sent. Need to have a feedback loop about r. d.
+ * being ok and only then cache it.
+ */
+ error = hid_parse_report(hid, (u8 *)shid->response->content, len);
+ if (error) {
+ dev_err(dev, "failed parsing report: %d\n", error);
+ return error;
+ }
+ shid->report_descriptor_crc32 = crc32_le(0,
+ (unsigned char const *)shid->response->content,
+ len);
+
+ set_bit(SPI_HID_READY, &shid->flags);
+
+ return 0;
+}
+
+static int spi_hid_ll_output_report(struct hid_device *hid, __u8 *buf,
+ size_t len)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ struct spi_hid_output_report report = {
+ .report_type = OUTPUT_REPORT,
+ .content_length = len - 1,
+ .content_id = buf[0],
+ .content = &buf[1],
+ };
+ int error;
+
+ if (!test_bit(SPI_HID_READY, &shid->flags)) {
+ dev_err(dev, "%s called in unready state\n", __func__);
+ return -ENODEV;
+ }
+
+ if (shid->desc.no_output_report_ack)
+ error = spi_hid_send_output_report(shid, &report);
+ else
+ error = spi_hid_sync_request(shid, &report);
+
+ if (error) {
+ dev_err(dev, "failed to send output report\n");
+ return error;
+ }
+
+ return len;
+}
+
+static int spi_hid_ll_raw_request(struct hid_device *hid,
+ unsigned char reportnum, __u8 *buf,
+ size_t len, unsigned char rtype, int reqtype)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ int ret;
+
+ switch (reqtype) {
+ case HID_REQ_SET_REPORT:
+ if (buf[0] != reportnum) {
+ dev_err(dev, "report id mismatch\n");
+ return -EINVAL;
+ }
+
+ if (rtype == HID_OUTPUT_REPORT)
+ return spi_hid_ll_output_report(hid, buf, len);
+
+ if (rtype != HID_FEATURE_REPORT)
+ return -EINVAL;
+
+ ret = spi_hid_set_request(shid, &buf[1], len - 1,
+ reportnum);
+ if (ret) {
+ dev_err(dev, "failed to set report\n");
+ return ret;
+ }
+
+ ret = len;
+ break;
+ case HID_REQ_GET_REPORT:
+ /*
+ * Only feature reports for now: GET_INPUT_REPORT is not
+ * supported.
+ */
+ if (rtype != HID_FEATURE_REPORT)
+ return -EOPNOTSUPP;
+
+ ret = spi_hid_get_request(shid, reportnum, buf, len);
+ if (ret < 0) {
+ dev_err(dev, "failed to get report\n");
+ return ret;
+ }
+ break;
+ default:
+ dev_err(dev, "invalid request type\n");
+ return -EIO;
+ }
+
+ return ret;
+}
+
+static struct hid_ll_driver spi_hid_ll_driver = {
+ .start = spi_hid_ll_start,
+ .stop = spi_hid_ll_stop,
+ .open = spi_hid_ll_open,
+ .close = spi_hid_ll_close,
+ .power = spi_hid_ll_power,
+ .parse = spi_hid_ll_parse,
+ .output_report = spi_hid_ll_output_report,
+ .raw_request = spi_hid_ll_raw_request,
+};
+
static ssize_t bus_error_count_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -159,6 +733,18 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
spi_set_drvdata(spi, shid);
+ mutex_init(&shid->output_lock);
+ init_completion(&shid->output_done);
+
+ /*
+ * we need to allocate the buffer without knowing the maximum
+ * size of the reports. Let's use HID_MAX_DESCRIPTOR_SIZE, then we do the
+ * real computation later.
+ */
+ error = spi_hid_alloc_buffers(shid, HID_MAX_DESCRIPTOR_SIZE);
+ if (error)
+ return error;
+
/*
* At the end of probe we initialize the device:
* 0) assert reset, bias the interrupt line
@@ -191,6 +777,10 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
dev_dbg(dev, "%s: d3 -> %s\n", __func__,
spi_hid_power_mode_string(shid->power_state));
+ error = spi_hid_create_device(shid);
+ if (error)
+ return error;
+
return 0;
}
EXPORT_SYMBOL_GPL(spi_hid_core_probe);
@@ -201,6 +791,8 @@ void spi_hid_core_remove(struct spi_device *spi)
struct device *dev = &spi->dev;
int error;
+ spi_hid_stop_hid(shid);
+
shid->ops->assert_reset(shid->ops);
error = shid->ops->power_down(shid->ops);
if (error)
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (3 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:22 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 06/11] HID: spi-hid: add spi_hid traces Jingyuan Liang
` (5 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Dmitry Antipov, Angela Czubak
This driver follows HID Over SPI Protocol Specification 1.0 available at
https://www.microsoft.com/en-us/download/details.aspx?id=103325. The
initial version of the driver does not support: 1) multi-fragment input
reports, 2) sending GET_INPUT and COMMAND output report types and
processing their respective acknowledge input reports, and 3) device
sleep power state.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/spi-hid-core.c | 780 +++++++++++++++++++++++++++++++++++--
1 file changed, 746 insertions(+), 34 deletions(-)
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index fca7a44eeb9f..90b2c2fea650 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -20,14 +20,20 @@
* Copyright (c) 2006-2010 Jiri Kosina
*/
+#include <linux/cache.h>
#include <linux/completion.h>
#include <linux/crc32.h>
#include <linux/device.h>
+#include <linux/dma-mapping.h>
#include <linux/err.h>
#include <linux/hid.h>
#include <linux/hid-over-spi.h>
+#include <linux/input.h>
#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/slab.h>
@@ -35,12 +41,22 @@
#include <linux/string.h>
#include <linux/sysfs.h>
#include <linux/unaligned.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+/* Protocol constants */
+#define SPI_HID_READ_APPROVAL_CONSTANT 0xff
+#define SPI_HID_INPUT_HEADER_SYNC_BYTE 0x5a
+#define SPI_HID_INPUT_HEADER_VERSION 0x03
+#define SPI_HID_SUPPORTED_VERSION 0x0300
#define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST 0x00
-#define SPI_HID_RESP_TIMEOUT 1000
+#define SPI_HID_MAX_RESET_ATTEMPTS 3
+#define SPI_HID_RESP_TIMEOUT 1000
/* Protocol message size constants */
+#define SPI_HID_READ_APPROVAL_LEN 5
#define SPI_HID_OUTPUT_HEADER_LEN 8
/* flags */
@@ -49,6 +65,26 @@
* requests. The FW becomes ready after sending the report descriptor.
*/
#define SPI_HID_READY 0
+/*
+ * refresh_in_progress is set to true while the refresh_device worker
+ * thread is destroying the old hid device. When this flag is set to
+ * true, input reports are dropped.
+ */
+#define SPI_HID_REFRESH_IN_PROGRESS 1
+/*
+ * reset_pending indicates that the device is being reset. When this flag
+ * is set to true, garbage interrupts triggered during reset will be
+ * dropped and will not cause error handling.
+ */
+#define SPI_HID_RESET_PENDING 2
+#define SPI_HID_RESET_RESPONSE 3
+#define SPI_HID_CREATE_DEVICE 4
+#define SPI_HID_ERROR 5
+/*
+ * started is set while the HID device is open, i.e. between ll_open and
+ * ll_close. Input reports are only forwarded while this flag is set.
+ */
+#define SPI_HID_STARTED 6
/* Raw input buffer with data from the bus */
struct spi_hid_input_buf {
@@ -57,6 +93,22 @@ struct spi_hid_input_buf {
u8 content[];
};
+/* Processed data from input report header */
+struct spi_hid_input_header {
+ u8 version;
+ u16 report_length;
+ u8 last_fragment_flag;
+ u8 sync_const;
+};
+
+/* Processed data from an input report */
+struct spi_hid_input_report {
+ u8 report_type;
+ u16 content_length;
+ u8 content_id;
+ u8 *content;
+};
+
/* Raw output report buffer to be put on the bus */
struct spi_hid_output_buf {
u8 header[SPI_HID_OUTPUT_HEADER_LEN];
@@ -114,6 +166,9 @@ struct spi_hid {
struct spi_device *spi; /* spi device. */
struct hid_device *hid; /* pointer to corresponding HID dev. */
+ struct spi_transfer input_transfer[2]; /* Transfer buffer for read and write. */
+ struct spi_message input_message; /* used to execute a sequence of spi transfers. */
+
struct spihid_ops *ops;
struct spi_hid_conf *conf;
@@ -124,6 +179,10 @@ struct spi_hid {
u16 response_length;
u16 bufsize;
+ /* Response type awaited by a sync request, 0 if none. Protected by io_lock. */
+ u8 expected_response;
+ /* Content ID of that request. Protected by io_lock. */
+ u8 expected_content_id;
enum hidspi_power_state power_state;
@@ -131,8 +190,28 @@ struct spi_hid {
unsigned long flags; /* device flags. */
- /* Control lock to make sure one output transaction at a time. */
+ struct work_struct reset_work;
+
+ /*
+ * Serializes request/response transactions: held from sending a
+ * request until its response in shid->response has been consumed.
+ * Taken before io_lock.
+ */
struct mutex output_lock;
+ /* Power lock to make sure one power state change at a time. */
+ struct mutex power_lock;
+ /*
+ * Serializes SPI bus transfers (output writes vs. IRQ-thread reads) and
+ * protects shid->expected_response, updates to shid->desc and
+ * publication of shid->hid. Held only briefly and never while waiting
+ * for a response, since the IRQ thread needs it to deliver that
+ * response. Nests inside output_lock.
+ *
+ * It does not keep the hid_device alive: the IRQ must be quiesced with
+ * disable_irq() before the hid_device is destroyed.
+ */
+ struct mutex io_lock;
+
struct completion output_done;
u32 report_descriptor_crc32; /* HID report descriptor crc32 checksum. */
@@ -142,10 +221,67 @@ struct spi_hid {
u32 bus_error_count;
int bus_last_error;
u32 dir_count; /* device initiated reset count. */
+
+ /* DMA-safe transfer buffers */
+ u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN] ____cacheline_aligned;
+ u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
};
static struct hid_ll_driver spi_hid_ll_driver;
+static void spi_hid_populate_read_approvals(const struct spi_hid_conf *conf,
+ u8 *header_buf, u8 *body_buf)
+{
+ header_buf[0] = conf->read_opcode;
+ put_unaligned_be24(conf->input_report_header_address, &header_buf[1]);
+ header_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
+
+ body_buf[0] = conf->read_opcode;
+ put_unaligned_be24(conf->input_report_body_address, &body_buf[1]);
+ body_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
+}
+
+static void spi_hid_parse_dev_desc(const struct hidspi_dev_descriptor *raw,
+ struct spi_hid_device_descriptor *desc)
+{
+ desc->hid_version = le16_to_cpu(raw->bcd_ver);
+ desc->report_descriptor_length = le16_to_cpu(raw->rep_desc_len);
+ desc->max_input_length = le16_to_cpu(raw->max_input_len);
+ desc->max_output_length = le16_to_cpu(raw->max_output_len);
+
+ /* FIXME: multi-fragment not supported, field below not used */
+ desc->max_fragment_length = le16_to_cpu(raw->max_frag_len);
+
+ desc->vendor_id = le16_to_cpu(raw->vendor_id);
+ desc->product_id = le16_to_cpu(raw->product_id);
+ desc->version_id = le16_to_cpu(raw->version_id);
+ desc->no_output_report_ack = le16_to_cpu(raw->flags) & BIT(0);
+}
+
+static void spi_hid_populate_input_header(const u8 *buf,
+ struct spi_hid_input_header *header)
+{
+ header->version = buf[0] & 0xf;
+ header->report_length = (get_unaligned_le16(&buf[1]) & 0x3fff) * 4;
+ header->last_fragment_flag = (buf[2] & 0x40) >> 6;
+ header->sync_const = buf[3];
+}
+
+static void spi_hid_populate_input_body(const u8 *buf,
+ struct spi_hid_input_report *body)
+{
+ body->report_type = buf[0];
+ body->content_length = get_unaligned_le16(&buf[1]);
+ body->content_id = buf[3];
+}
+
+static void spi_hid_input_report_prepare(struct spi_hid_input_buf *buf,
+ struct spi_hid_input_report *report)
+{
+ spi_hid_populate_input_body(buf->body, report);
+ report->content = buf->content;
+}
+
static void spi_hid_populate_output_header(u8 *buf,
const struct spi_hid_conf *conf,
const struct spi_hid_output_report *report)
@@ -157,6 +293,33 @@ static void spi_hid_populate_output_header(u8 *buf,
buf[7] = report->content_id;
}
+static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
+ bool is_header)
+{
+ int error;
+
+ shid->input_transfer[0].tx_buf = is_header ?
+ shid->read_approval_header :
+ shid->read_approval_body;
+ shid->input_transfer[0].len = SPI_HID_READ_APPROVAL_LEN;
+
+ shid->input_transfer[1].rx_buf = buf;
+ shid->input_transfer[1].len = length;
+
+ spi_message_init_with_transfers(&shid->input_message,
+ shid->input_transfer, 2);
+
+ error = spi_sync(shid->spi, &shid->input_message);
+ if (error) {
+ dev_err(&shid->spi->dev, "Error starting sync transfer: %d\n", error);
+ shid->bus_error_count++;
+ shid->bus_last_error = error;
+ return error;
+ }
+
+ return 0;
+}
+
static int spi_hid_output(struct spi_hid *shid, const void *buf, u16 length)
{
int error;
@@ -187,17 +350,94 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
static void spi_hid_stop_hid(struct spi_hid *shid)
{
- struct hid_device *hid = shid->hid;
+ struct hid_device *hid;
- shid->hid = NULL;
- clear_bit(SPI_HID_READY, &shid->flags);
+ scoped_guard(mutex, &shid->io_lock) {
+ hid = shid->hid;
+ shid->hid = NULL;
+ clear_bit(SPI_HID_READY, &shid->flags);
+ }
if (hid)
hid_destroy_device(hid);
}
+static void spi_hid_error_handler(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+ int error;
+
+ guard(mutex)(&shid->power_lock);
+ if (shid->power_state == HIDSPI_OFF)
+ return;
+
+ disable_irq(shid->spi->irq);
+
+ if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
+ dev_err(dev, "unresponsive device, aborting\n");
+ spi_hid_stop_hid(shid);
+ shid->ops->assert_reset(shid->ops);
+ error = shid->ops->power_down(shid->ops);
+ if (error) {
+ dev_err(dev, "failed to disable regulator\n");
+ shid->regulator_error_count++;
+ shid->regulator_last_error = error;
+ }
+ shid->power_state = HIDSPI_OFF;
+ /* Dead device: leave the IRQ permanently disabled. */
+ return;
+ }
+
+ clear_bit(SPI_HID_READY, &shid->flags);
+ set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+
+ shid->ops->assert_reset(shid->ops);
+
+ shid->power_state = HIDSPI_OFF;
+
+ /*
+ * We want to cancel pending reset work as the device is being reset
+ * to recover from an error. cancel_work_sync will put us in a deadlock
+ * because this function is scheduled in 'reset_work' and we should
+ * avoid waiting for itself.
+ */
+ cancel_work(&shid->reset_work);
+
+ shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+ shid->power_state = HIDSPI_ON;
+
+ shid->ops->deassert_reset(shid->ops);
+
+ enable_irq(shid->spi->irq);
+}
+
+/* Map an output report type to the response type the device sends back. */
+static u8 spi_hid_response_type(u8 report_type)
+{
+ switch (report_type) {
+ case DEVICE_DESCRIPTOR:
+ return DEVICE_DESCRIPTOR_RESPONSE;
+ case REPORT_DESCRIPTOR:
+ return REPORT_DESCRIPTOR_RESPONSE;
+ case SET_FEATURE:
+ return SET_FEATURE_RESPONSE;
+ case GET_FEATURE:
+ return GET_FEATURE_RESPONSE;
+ case OUTPUT_REPORT:
+ return OUTPUT_REPORT_RESPONSE;
+ default:
+ return 0;
+ }
+}
+
+/*
+ * If expected_response is non-zero, arm the response tracking under io_lock
+ * before the write so that the IRQ path only accepts a response of that type.
+ */
static int __spi_hid_send_output_report(struct spi_hid *shid,
- struct spi_hid_output_report *report)
+ struct spi_hid_output_report *report,
+ u8 expected_response)
{
struct spi_hid_output_buf *buf = shid->output;
struct device *dev = &shid->spi->dev;
@@ -206,6 +446,14 @@ static int __spi_hid_send_output_report(struct spi_hid *shid,
u8 padding;
int error;
+ lockdep_assert_held(&shid->output_lock);
+
+ /* While not READY, only (re)init descriptor requests may be sent. */
+ if (report->report_type != DEVICE_DESCRIPTOR &&
+ report->report_type != REPORT_DESCRIPTOR &&
+ !test_bit(SPI_HID_READY, &shid->flags))
+ return -ENODEV;
+
if (report->content_length > shid->desc.max_output_length ||
report->content_length > shid->bufsize) {
dev_err(dev, "Output report too big, content_length 0x%x\n",
@@ -213,6 +461,7 @@ static int __spi_hid_send_output_report(struct spi_hid *shid,
return -E2BIG;
}
+ guard(mutex)(&shid->io_lock);
spi_hid_populate_output_header(buf->header, shid->conf, report);
if (report->content_length)
@@ -223,9 +472,17 @@ static int __spi_hid_send_output_report(struct spi_hid *shid,
padding = padded_length - report_length;
memset(&buf->content[report->content_length], 0, padding);
+ if (expected_response) {
+ reinit_completion(&shid->output_done);
+ shid->expected_response = expected_response;
+ shid->expected_content_id = report->content_id;
+ }
+
error = spi_hid_output(shid, buf, padded_length);
- if (error)
+ if (error) {
dev_err(dev, "Failed output transfer: %d\n", error);
+ shid->expected_response = 0;
+ }
return error;
}
@@ -234,7 +491,7 @@ static int spi_hid_send_output_report(struct spi_hid *shid,
struct spi_hid_output_report *report)
{
guard(mutex)(&shid->output_lock);
- return __spi_hid_send_output_report(shid, report);
+ return __spi_hid_send_output_report(shid, report, 0);
}
static int __spi_hid_sync_request(struct spi_hid *shid,
@@ -243,22 +500,26 @@ static int __spi_hid_sync_request(struct spi_hid *shid,
struct device *dev = &shid->spi->dev;
int error;
- reinit_completion(&shid->output_done);
-
- error = __spi_hid_send_output_report(shid, report);
+ error = __spi_hid_send_output_report(shid, report,
+ spi_hid_response_type(report->report_type));
if (error)
return error;
- error = wait_for_completion_interruptible_timeout(&shid->output_done,
- msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
- if (error == 0) {
- dev_err(dev, "Response timed out\n");
- return -ETIMEDOUT;
+ if (wait_for_completion_timeout(&shid->output_done,
+ msecs_to_jiffies(SPI_HID_RESP_TIMEOUT)))
+ return 0;
+
+ /* Drop late responses and block new HID requests until the reset. */
+ scoped_guard(mutex, &shid->io_lock) {
+ shid->expected_response = 0;
+ clear_bit(SPI_HID_READY, &shid->flags);
}
- if (error < 0)
- return error;
- return 0;
+ dev_err(dev, "Response timed out\n");
+ /* The device may still answer later: resync it with a reset. */
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
+ return -ETIMEDOUT;
}
static int spi_hid_sync_request(struct spi_hid *shid,
@@ -268,9 +529,174 @@ static int spi_hid_sync_request(struct spi_hid *shid,
return __spi_hid_sync_request(shid, report);
}
+/*
+ * Handle the reset response from the FW by sending a request for the device
+ * descriptor.
+ */
+static void spi_hid_reset_response(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_output_report report = {
+ .report_type = DEVICE_DESCRIPTOR,
+ .content_length = 0x0,
+ .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
+ .content = NULL,
+ };
+ int error;
+
+ if (test_bit(SPI_HID_READY, &shid->flags)) {
+ dev_err(dev, "Spontaneous FW reset!\n");
+ clear_bit(SPI_HID_READY, &shid->flags);
+ shid->dir_count++;
+ }
+
+ if (shid->power_state == HIDSPI_OFF)
+ return;
+
+ error = spi_hid_sync_request(shid, &report);
+ if (error) {
+ dev_WARN_ONCE(dev, true,
+ "Failed to send device descriptor request: %d\n", error);
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
+ }
+}
+
+static int spi_hid_input_report_handler(struct spi_hid *shid,
+ struct spi_hid_input_buf *buf)
+{
+ struct device *dev = &shid->spi->dev;
+ struct hid_device *hid;
+ struct spi_hid_input_report r;
+ int error = 0;
+
+ scoped_guard(mutex, &shid->io_lock) {
+ if (!test_bit(SPI_HID_READY, &shid->flags) ||
+ !test_bit(SPI_HID_STARTED, &shid->flags) ||
+ test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
+ dev_dbg(dev, "HID not ready (flags 0x%lx), dropping input report\n",
+ shid->flags);
+ return 0;
+ }
+
+ hid = shid->hid;
+ spi_hid_input_report_prepare(buf, &r);
+ }
+
+ /*
+ * Safe after dropping io_lock: hid is only freed, and bufsize only
+ * changed, after disable_irq(), which waits for this handler. The
+ * buffer is the content ID byte at r.content - 1 plus bufsize bytes.
+ */
+ error = hid_safe_input_report(hid, HID_INPUT_REPORT, r.content - 1,
+ shid->bufsize + 1,
+ r.content_length + 1, 1);
+
+ if (error == -ENODEV || error == -EBUSY) {
+ dev_err(dev, "ignoring report --> %d\n", error);
+ return 0;
+ } else if (error) {
+ dev_err(dev, "Bad input report: %d\n", error);
+ }
+
+ return error;
+}
+
+/*
+ * Validate a device descriptor response and, if valid, publish it to
+ * shid->desc and request device creation.
+ */
+static int spi_hid_dev_desc_response(struct spi_hid *shid,
+ struct spi_hid_input_report *body)
+{
+ struct hidspi_dev_descriptor *raw =
+ (struct hidspi_dev_descriptor *)shid->input->content;
+ struct device *dev = &shid->spi->dev;
+
+ /* Validate device descriptor length before parsing */
+ if (body->content_length != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
+ dev_err(dev, "Invalid content length %d, expected %zu\n",
+ body->content_length, HIDSPI_DEVICE_DESCRIPTOR_SIZE);
+ return -EPROTO;
+ }
+
+ if (le16_to_cpu(raw->dev_desc_len) != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
+ dev_err(dev, "Invalid wDeviceDescLength %d, expected %zu\n",
+ le16_to_cpu(raw->dev_desc_len),
+ HIDSPI_DEVICE_DESCRIPTOR_SIZE);
+ return -EPROTO;
+ }
+
+ if (le16_to_cpu(raw->bcd_ver) != SPI_HID_SUPPORTED_VERSION) {
+ dev_err(dev, "Unsupported device descriptor version %4x\n",
+ le16_to_cpu(raw->bcd_ver));
+ return -EPROTONOSUPPORT;
+ }
+
+ /* Fail before reset_attempts is cleared so resets stay bounded. */
+ if (!le16_to_cpu(raw->rep_desc_len) ||
+ le16_to_cpu(raw->rep_desc_len) > HID_MAX_DESCRIPTOR_SIZE) {
+ dev_err(dev, "Invalid wReportDescLength %d, max %d\n",
+ le16_to_cpu(raw->rep_desc_len),
+ HID_MAX_DESCRIPTOR_SIZE);
+ return -EPROTO;
+ }
+
+ spi_hid_parse_dev_desc(raw, &shid->desc);
+
+ /* Reset attempts at every device descriptor fetch */
+ shid->reset_attempts = 0;
+ set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
+ schedule_work(&shid->reset_work);
+
+ return 0;
+}
+
+static int spi_hid_response_handler(struct spi_hid *shid,
+ struct spi_hid_input_report *body)
+{
+ int error = 0;
+
+ guard(mutex)(&shid->io_lock);
+
+ /*
+ * Drop late or unsolicited responses. With no request IDs, also match
+ * the report ID for GET_FEATURE so it can't get another report's data.
+ */
+ if (!shid->expected_response ||
+ body->report_type != shid->expected_response ||
+ (body->report_type == GET_FEATURE_RESPONSE &&
+ body->content_id != shid->expected_content_id)) {
+ dev_err(&shid->spi->dev, "Unexpected response report 0x%x\n",
+ body->report_type);
+ return 0;
+ }
+ shid->expected_response = 0;
+
+ shid->response_length = body->content_length;
+ if (body->report_type == REPORT_DESCRIPTOR_RESPONSE ||
+ body->report_type == GET_FEATURE_RESPONSE) {
+ memcpy(shid->response->body, shid->input->body,
+ sizeof(shid->input->body));
+ memcpy(shid->response->content, shid->input->content,
+ body->content_length);
+ } else if (body->report_type == DEVICE_DESCRIPTOR_RESPONSE) {
+ /*
+ * Publish the descriptor before complete() so reset_work never
+ * sees a partially written shid->desc.
+ */
+ error = spi_hid_dev_desc_response(shid, body);
+ }
+
+ /* Wake the waiter even on error so it doesn't wait for the timeout. */
+ complete(&shid->output_done);
+
+ return error;
+}
+
/*
* This function returns the length of the report descriptor, or a negative
- * error code if something went wrong.
+ * error code if something went wrong. Caller must hold output_lock.
*/
static int spi_hid_report_descriptor_request(struct spi_hid *shid)
{
@@ -283,10 +709,14 @@ static int spi_hid_report_descriptor_request(struct spi_hid *shid)
};
int ret;
- ret = spi_hid_sync_request(shid, &report);
+ lockdep_assert_held(&shid->output_lock);
+
+ ret = __spi_hid_sync_request(shid, &report);
if (ret) {
dev_err(dev,
"Expected report descriptor not received: %d\n", ret);
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
return ret;
}
@@ -325,7 +755,9 @@ static int spi_hid_create_device(struct spi_hid *shid)
hid->vendor, hid->product);
strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
- shid->hid = hid;
+ scoped_guard(mutex, &shid->io_lock) {
+ shid->hid = hid;
+ }
error = hid_add_device(hid);
if (error) {
@@ -334,13 +766,205 @@ static int spi_hid_create_device(struct spi_hid *shid)
* We likely got here because report descriptor request timed
* out. Let's disconnect and destroy the hid_device structure.
*/
- spi_hid_stop_hid(shid);
+ scoped_guard(disable_irq, &shid->spi->irq)
+ spi_hid_stop_hid(shid);
return error;
}
return 0;
}
+static void spi_hid_refresh_device(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+ u32 new_crc32 = 0;
+ int error = 0;
+
+ /* Keep shid->response stable until the CRC is computed. */
+ scoped_guard(mutex, &shid->output_lock) {
+ error = spi_hid_report_descriptor_request(shid);
+ if (error < 0) {
+ dev_err(dev,
+ "%s: failed report descriptor request: %d\n",
+ __func__, error);
+ return;
+ }
+ new_crc32 = crc32_le(0, (unsigned char const *)shid->response->content,
+ (size_t)error);
+ }
+
+ /* Same report descriptor, so no need to create a new hid device. */
+ if (new_crc32 == shid->report_descriptor_crc32) {
+ set_bit(SPI_HID_READY, &shid->flags);
+ return;
+ }
+
+ shid->report_descriptor_crc32 = new_crc32;
+
+ set_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags);
+
+ /*
+ * Disable the IRQ only for tear-down: creation needs it to receive
+ * the report descriptor.
+ */
+ scoped_guard(disable_irq, &shid->spi->irq) {
+ spi_hid_stop_hid(shid);
+ }
+
+ /* Allow input while the new device probes. */
+ clear_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags);
+
+ error = spi_hid_create_device(shid);
+ if (error)
+ dev_err(dev, "%s: Failed to create hid device: %d\n", __func__, error);
+}
+
+static void spi_hid_reset_work(struct work_struct *work)
+{
+ struct spi_hid *shid =
+ container_of(work, struct spi_hid, reset_work);
+ struct device *dev = &shid->spi->dev;
+ int error = 0;
+ bool resched = false;
+
+ if (test_and_clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags)) {
+ spi_hid_reset_response(shid);
+ resched = true;
+ } else if (test_and_clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags)) {
+ guard(mutex)(&shid->power_lock);
+ if (shid->power_state != HIDSPI_OFF) {
+ if (!shid->hid) {
+ error = spi_hid_create_device(shid);
+ if (error) {
+ dev_err(dev, "%s: Failed to create hid device: %d\n",
+ __func__, error);
+ }
+ } else {
+ spi_hid_refresh_device(shid);
+ }
+ } else {
+ dev_err(dev, "%s: Powered off, returning\n", __func__);
+ }
+ resched = true;
+ } else if (test_and_clear_bit(SPI_HID_ERROR, &shid->flags)) {
+ spi_hid_error_handler(shid);
+ }
+
+ /*
+ * If other flags are still pending, safely reschedule ourselves
+ * to process them in the next workqueue cycle.
+ */
+ if (resched && (shid->flags & (BIT(SPI_HID_RESET_RESPONSE) |
+ BIT(SPI_HID_CREATE_DEVICE) |
+ BIT(SPI_HID_ERROR)))) {
+ schedule_work(&shid->reset_work);
+ }
+}
+
+static int spi_hid_process_input_report(struct spi_hid *shid,
+ struct spi_hid_input_buf *buf)
+{
+ struct spi_hid_input_header header;
+ struct spi_hid_input_report body;
+ struct device *dev = &shid->spi->dev;
+
+ spi_hid_populate_input_header(buf->header, &header);
+ spi_hid_input_report_prepare(buf, &body);
+
+ if (HIDSPI_INPUT_BODY_SIZE(body.content_length) > header.report_length) {
+ dev_err(dev, "Bad body length %zu > %u\n",
+ HIDSPI_INPUT_BODY_SIZE(body.content_length),
+ header.report_length);
+ return -EPROTO;
+ }
+
+ switch (body.report_type) {
+ case DATA:
+ return spi_hid_input_report_handler(shid, buf);
+ case RESET_RESPONSE:
+ clear_bit(SPI_HID_RESET_PENDING, &shid->flags);
+ set_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
+ schedule_work(&shid->reset_work);
+ break;
+ case DEVICE_DESCRIPTOR_RESPONSE:
+ return spi_hid_response_handler(shid, &body);
+ case OUTPUT_REPORT_RESPONSE:
+ if (shid->desc.no_output_report_ack) {
+ dev_err(dev, "Unexpected output report response\n");
+ break;
+ }
+ fallthrough;
+ case GET_FEATURE_RESPONSE:
+ case SET_FEATURE_RESPONSE:
+ case REPORT_DESCRIPTOR_RESPONSE:
+ spi_hid_response_handler(shid, &body);
+ break;
+ /*
+ * FIXME: sending GET_INPUT and COMMAND reports not supported, thus
+ * throw away responses to those, they should never come.
+ */
+ case GET_INPUT_REPORT_RESPONSE:
+ case COMMAND_RESPONSE:
+ dev_err(dev, "Not a supported report type: 0x%x\n",
+ body.report_type);
+ break;
+ default:
+ dev_err(dev, "Unknown input report: 0x%x\n", body.report_type);
+ return -EPROTO;
+ }
+
+ return 0;
+}
+
+static int spi_hid_bus_validate_header(struct spi_hid *shid,
+ struct spi_hid_input_header *header)
+{
+ struct device *dev = &shid->spi->dev;
+ /*
+ * Body capacity of shid->input, matching spi_hid_alloc_buffers().
+ * shid->bufsize only changes with the IRQ disabled, so it is stable here.
+ */
+ u32 max_body = round_up(sizeof(shid->input->header) +
+ sizeof(shid->input->body) + shid->bufsize, 4) -
+ sizeof(shid->input->header);
+
+ if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
+ dev_err(dev, "Unknown input report version (v 0x%x)\n",
+ header->version);
+ return -EINVAL;
+ }
+
+ /*
+ * report_length is device-provided and is used as the size of the body
+ * transfer, so it must never exceed the input buffer.
+ */
+ if (header->report_length > max_body) {
+ dev_err(dev, "Input report too big: %u > %u\n",
+ header->report_length, max_body);
+ return -EMSGSIZE;
+ }
+
+ if (shid->desc.max_input_length != 0 &&
+ header->report_length > shid->desc.max_input_length) {
+ dev_err(dev, "Input report body size %u > max expected of %u\n",
+ header->report_length, shid->desc.max_input_length);
+ return -EMSGSIZE;
+ }
+
+ if (header->last_fragment_flag != 1) {
+ dev_err(dev, "Multi-fragment reports not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (header->sync_const != SPI_HID_INPUT_HEADER_SYNC_BYTE) {
+ dev_err(dev, "Invalid input report sync constant (0x%x)\n",
+ header->sync_const);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int spi_hid_get_request(struct spi_hid *shid, u8 content_id,
u8 *buf, size_t len)
{
@@ -365,6 +989,8 @@ static int spi_hid_get_request(struct spi_hid *shid, u8 content_id,
dev_err(dev,
"Expected get request response not received! Error %d\n",
error);
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
return error;
}
@@ -397,9 +1023,81 @@ static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
return spi_hid_sync_request(shid, &report);
}
-/* This is a placeholder. Will be implemented in the next patch. */
+/* Schedule a reset to recover from an error in the IRQ handler. */
+static irqreturn_t spi_hid_irq_error(struct spi_hid *shid)
+{
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
+
+ return IRQ_HANDLED;
+}
+
static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
{
+ struct spi_hid *shid = _shid;
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_input_header header;
+ int error = 0;
+
+ scoped_guard(mutex, &shid->io_lock) {
+ if (shid->power_state == HIDSPI_OFF) {
+ dev_warn(dev, "Device is off, ignoring interrupt\n");
+ return IRQ_NONE;
+ }
+
+ error = spi_hid_input_sync(shid, shid->input->header,
+ sizeof(shid->input->header), true);
+ if (error) {
+ dev_err(dev, "Failed to transfer header: %d\n", error);
+ return spi_hid_irq_error(shid);
+ }
+
+ if (shid->input_message.status < 0) {
+ dev_warn(dev, "Error reading header: %d\n",
+ shid->input_message.status);
+ shid->bus_error_count++;
+ shid->bus_last_error = shid->input_message.status;
+ return spi_hid_irq_error(shid);
+ }
+
+ spi_hid_populate_input_header(shid->input->header, &header);
+
+ error = spi_hid_bus_validate_header(shid, &header);
+ if (error) {
+ if (!test_bit(SPI_HID_RESET_PENDING, &shid->flags)) {
+ dev_err(dev, "Failed to validate header: %d\n", error);
+ print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
+ DUMP_PREFIX_NONE, 16, 1, shid->input->header,
+ sizeof(shid->input->header), false);
+ shid->bus_error_count++;
+ shid->bus_last_error = error;
+ return spi_hid_irq_error(shid);
+ }
+ return IRQ_HANDLED;
+ }
+
+ error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
+ false);
+ if (error) {
+ dev_err(dev, "Failed to transfer body: %d\n", error);
+ return spi_hid_irq_error(shid);
+ }
+
+ if (shid->input_message.status < 0) {
+ dev_warn(dev, "Error reading body: %d\n",
+ shid->input_message.status);
+ shid->bus_error_count++;
+ shid->bus_last_error = shid->input_message.status;
+ return spi_hid_irq_error(shid);
+ }
+ }
+
+ error = spi_hid_process_input_report(shid, shid->input);
+ if (error) {
+ dev_err(dev, "Failed to process input report: %d\n", error);
+ return spi_hid_irq_error(shid);
+ }
+
return IRQ_HANDLED;
}
@@ -479,6 +1177,8 @@ static int spi_hid_ll_start(struct hid_device *hid)
spi_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
if (bufsize > shid->bufsize) {
+ /* Serialize with requests in flight, e.g. from reset_work. */
+ guard(mutex)(&shid->output_lock);
guard(disable_irq)(&shid->spi->irq);
error = spi_hid_alloc_buffers(shid, bufsize);
@@ -496,6 +1196,10 @@ static void spi_hid_ll_stop(struct hid_device *hid)
static int spi_hid_ll_open(struct hid_device *hid)
{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+
+ set_bit(SPI_HID_STARTED, &shid->flags);
return 0;
}
@@ -504,6 +1208,7 @@ static void spi_hid_ll_close(struct hid_device *hid)
struct spi_device *spi = hid->driver_data;
struct spi_hid *shid = spi_get_drvdata(spi);
+ clear_bit(SPI_HID_STARTED, &shid->flags);
shid->reset_attempts = 0;
}
@@ -535,11 +1240,8 @@ static int spi_hid_ll_parse(struct hid_device *hid)
return -EINVAL;
}
- if (rsize > shid->bufsize) {
- error = spi_hid_alloc_buffers(shid, rsize);
- if (error)
- return error;
- }
+ /* Keep shid->response stable until done. */
+ guard(mutex)(&shid->output_lock);
len = spi_hid_report_descriptor_request(shid);
if (len < 0) {
@@ -730,12 +1432,21 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
shid->power_state = HIDSPI_ON;
shid->ops = ops;
shid->conf = conf;
+ set_bit(SPI_HID_RESET_PENDING, &shid->flags);
spi_set_drvdata(spi, shid);
+ /* Using now populated conf let's pre-calculate the read approvals */
+ spi_hid_populate_read_approvals(shid->conf, shid->read_approval_header,
+ shid->read_approval_body);
+
mutex_init(&shid->output_lock);
+ mutex_init(&shid->power_lock);
+ mutex_init(&shid->io_lock);
init_completion(&shid->output_done);
+ INIT_WORK(&shid->reset_work, spi_hid_reset_work);
+
/*
* we need to allocate the buffer without knowing the maximum
* size of the reports. Let's use HID_MAX_DESCRIPTOR_SIZE, then we do the
@@ -760,7 +1471,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
shid->ops->sleep_minimal_reset_delay(shid->ops);
error = devm_request_threaded_irq(dev, spi->irq, NULL, spi_hid_dev_irq,
- IRQF_ONESHOT, dev_name(&spi->dev), shid);
+ IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(&spi->dev), shid);
if (error) {
dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
return error;
@@ -774,13 +1485,11 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
shid->ops->deassert_reset(shid->ops);
+ enable_irq(spi->irq);
+
dev_dbg(dev, "%s: d3 -> %s\n", __func__,
spi_hid_power_mode_string(shid->power_state));
- error = spi_hid_create_device(shid);
- if (error)
- return error;
-
return 0;
}
EXPORT_SYMBOL_GPL(spi_hid_core_probe);
@@ -791,6 +1500,9 @@ void spi_hid_core_remove(struct spi_device *spi)
struct device *dev = &spi->dev;
int error;
+ disable_irq(spi->irq);
+ disable_work_sync(&shid->reset_work);
+
spi_hid_stop_hid(shid);
shid->ops->assert_reset(shid->ops);
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 06/11] HID: spi-hid: add spi_hid traces
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (4 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 07/11] HID: spi-hid: add ACPI support for HID over SPI Jingyuan Liang
` (4 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Dmitry Antipov, Angela Czubak
Add traces for the purpose of debugging spi_hid driver.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Angela Czubak <acz@semihalf.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/Makefile | 1 +
drivers/hid/spi-hid/spi-hid-core.c | 132 ++++++++------------------
drivers/hid/spi-hid/spi-hid-core.h | 109 +++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-trace.h | 184 ++++++++++++++++++++++++++++++++++++
4 files changed, 334 insertions(+), 92 deletions(-)
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
index 92e24cddbfc2..733e006df56e 100644
--- a/drivers/hid/spi-hid/Makefile
+++ b/drivers/hid/spi-hid/Makefile
@@ -7,3 +7,4 @@
obj-$(CONFIG_SPI_HID_CORE) += spi-hid.o
spi-hid-objs = spi-hid-core.o
+CFLAGS_spi-hid-core.o := -I$(src)
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 90b2c2fea650..e428fae762ac 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -44,6 +44,11 @@
#include <linux/wait.h>
#include <linux/workqueue.h>
+#include "spi-hid-core.h"
+
+#define CREATE_TRACE_POINTS
+#include "spi-hid-trace.h"
+
/* Protocol constants */
#define SPI_HID_READ_APPROVAL_CONSTANT 0xff
#define SPI_HID_INPUT_HEADER_SYNC_BYTE 0x5a
@@ -86,13 +91,6 @@
*/
#define SPI_HID_STARTED 6
-/* Raw input buffer with data from the bus */
-struct spi_hid_input_buf {
- u8 header[HIDSPI_INPUT_HEADER_SIZE];
- u8 body[HIDSPI_INPUT_BODY_HEADER_SIZE];
- u8 content[];
-};
-
/* Processed data from input report header */
struct spi_hid_input_header {
u8 version;
@@ -109,12 +107,6 @@ struct spi_hid_input_report {
u8 *content;
};
-/* Raw output report buffer to be put on the bus */
-struct spi_hid_output_buf {
- u8 header[SPI_HID_OUTPUT_HEADER_LEN];
- u8 content[];
-};
-
/* Data necessary to send an output report */
struct spi_hid_output_report {
u8 report_type;
@@ -123,19 +115,6 @@ struct spi_hid_output_report {
u8 *content;
};
-/* Processed data from a device descriptor */
-struct spi_hid_device_descriptor {
- u16 hid_version;
- u16 report_descriptor_length;
- u16 max_input_length;
- u16 max_output_length;
- u16 max_fragment_length;
- u16 vendor_id;
- u16 product_id;
- u16 version_id;
- u8 no_output_report_ack;
-};
-
/* struct spi_hid_conf - Conf provided to the core */
struct spi_hid_conf {
u32 input_report_header_address;
@@ -161,72 +140,6 @@ struct spihid_ops {
void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
};
-/* Driver context */
-struct spi_hid {
- struct spi_device *spi; /* spi device. */
- struct hid_device *hid; /* pointer to corresponding HID dev. */
-
- struct spi_transfer input_transfer[2]; /* Transfer buffer for read and write. */
- struct spi_message input_message; /* used to execute a sequence of spi transfers. */
-
- struct spihid_ops *ops;
- struct spi_hid_conf *conf;
-
- struct spi_hid_device_descriptor desc; /* HID device descriptor. */
- struct spi_hid_output_buf *output; /* Output buffer. */
- struct spi_hid_input_buf *input; /* Input buffer. */
- struct spi_hid_input_buf *response; /* Response buffer. */
-
- u16 response_length;
- u16 bufsize;
- /* Response type awaited by a sync request, 0 if none. Protected by io_lock. */
- u8 expected_response;
- /* Content ID of that request. Protected by io_lock. */
- u8 expected_content_id;
-
- enum hidspi_power_state power_state;
-
- u8 reset_attempts; /* The number of reset attempts. */
-
- unsigned long flags; /* device flags. */
-
- struct work_struct reset_work;
-
- /*
- * Serializes request/response transactions: held from sending a
- * request until its response in shid->response has been consumed.
- * Taken before io_lock.
- */
- struct mutex output_lock;
- /* Power lock to make sure one power state change at a time. */
- struct mutex power_lock;
- /*
- * Serializes SPI bus transfers (output writes vs. IRQ-thread reads) and
- * protects shid->expected_response, updates to shid->desc and
- * publication of shid->hid. Held only briefly and never while waiting
- * for a response, since the IRQ thread needs it to deliver that
- * response. Nests inside output_lock.
- *
- * It does not keep the hid_device alive: the IRQ must be quiesced with
- * disable_irq() before the hid_device is destroyed.
- */
- struct mutex io_lock;
-
- struct completion output_done;
-
- u32 report_descriptor_crc32; /* HID report descriptor crc32 checksum. */
-
- u32 regulator_error_count;
- int regulator_last_error;
- u32 bus_error_count;
- int bus_last_error;
- u32 dir_count; /* device initiated reset count. */
-
- /* DMA-safe transfer buffers */
- u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN] ____cacheline_aligned;
- u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
-};
-
static struct hid_ll_driver spi_hid_ll_driver;
static void spi_hid_populate_read_approvals(const struct spi_hid_conf *conf,
@@ -309,6 +222,11 @@ static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
spi_message_init_with_transfers(&shid->input_message,
shid->input_transfer, 2);
+ /* rx_buf is not filled yet; received data is traced by *_complete. */
+ trace_spi_hid_input_sync(shid, shid->input_transfer[0].tx_buf,
+ shid->input_transfer[0].len,
+ shid->input_transfer[1].rx_buf, 0, 0);
+
error = spi_sync(shid->spi, &shid->input_message);
if (error) {
dev_err(&shid->spi->dev, "Error starting sync transfer: %d\n", error);
@@ -367,6 +285,8 @@ static void spi_hid_error_handler(struct spi_hid *shid)
struct device *dev = &shid->spi->dev;
int error;
+ trace_spi_hid_error_handler(shid);
+
guard(mutex)(&shid->power_lock);
if (shid->power_state == HIDSPI_OFF)
return;
@@ -544,6 +464,8 @@ static void spi_hid_reset_response(struct spi_hid *shid)
};
int error;
+ trace_spi_hid_reset_response(shid);
+
if (test_bit(SPI_HID_READY, &shid->flags)) {
dev_err(dev, "Spontaneous FW reset!\n");
clear_bit(SPI_HID_READY, &shid->flags);
@@ -571,6 +493,8 @@ static int spi_hid_input_report_handler(struct spi_hid *shid,
int error = 0;
scoped_guard(mutex, &shid->io_lock) {
+ trace_spi_hid_input_report_handler(shid);
+
if (!test_bit(SPI_HID_READY, &shid->flags) ||
!test_bit(SPI_HID_STARTED, &shid->flags) ||
test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
@@ -673,6 +597,8 @@ static int spi_hid_response_handler(struct spi_hid *shid,
}
shid->expected_response = 0;
+ trace_spi_hid_response_handler(shid);
+
shid->response_length = body->content_length;
if (body->report_type == REPORT_DESCRIPTOR_RESPONSE ||
body->report_type == GET_FEATURE_RESPONSE) {
@@ -736,6 +662,8 @@ static int spi_hid_create_device(struct spi_hid *shid)
struct device *dev = &shid->spi->dev;
int error;
+ trace_spi_hid_create_device(shid);
+
hid = hid_allocate_device();
error = PTR_ERR_OR_ZERO(hid);
if (error) {
@@ -780,6 +708,8 @@ static void spi_hid_refresh_device(struct spi_hid *shid)
u32 new_crc32 = 0;
int error = 0;
+ trace_spi_hid_refresh_device(shid);
+
/* Keep shid->response stable until the CRC is computed. */
scoped_guard(mutex, &shid->output_lock) {
error = spi_hid_report_descriptor_request(shid);
@@ -868,6 +798,8 @@ static int spi_hid_process_input_report(struct spi_hid *shid,
struct spi_hid_input_report body;
struct device *dev = &shid->spi->dev;
+ trace_spi_hid_process_input_report(shid);
+
spi_hid_populate_input_header(buf->header, &header);
spi_hid_input_report_prepare(buf, &body);
@@ -1039,6 +971,9 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
struct spi_hid_input_header header;
int error = 0;
+ trace_spi_hid_dev_irq(shid, irq);
+ trace_spi_hid_header_transfer(shid);
+
scoped_guard(mutex, &shid->io_lock) {
if (shid->power_state == HIDSPI_OFF) {
dev_warn(dev, "Device is off, ignoring interrupt\n");
@@ -1052,6 +987,13 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
return spi_hid_irq_error(shid);
}
+ trace_spi_hid_input_header_complete(shid,
+ shid->input_transfer[0].tx_buf,
+ shid->input_transfer[0].len,
+ shid->input_transfer[1].rx_buf,
+ shid->input_transfer[1].len,
+ shid->input_message.status);
+
if (shid->input_message.status < 0) {
dev_warn(dev, "Error reading header: %d\n",
shid->input_message.status);
@@ -1083,6 +1025,12 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
return spi_hid_irq_error(shid);
}
+ trace_spi_hid_input_body_complete(shid, shid->input_transfer[0].tx_buf,
+ shid->input_transfer[0].len,
+ shid->input_transfer[1].rx_buf,
+ shid->input_transfer[1].len,
+ shid->input_message.status);
+
if (shid->input_message.status < 0) {
dev_warn(dev, "Error reading body: %d\n",
shid->input_message.status);
diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
new file mode 100644
index 000000000000..1d8356f787f3
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -0,0 +1,109 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ */
+
+#ifndef SPI_HID_CORE_H
+#define SPI_HID_CORE_H
+
+#include <linux/hid-over-spi.h>
+#include <linux/spi/spi.h>
+
+/* Protocol message size constants */
+#define SPI_HID_READ_APPROVAL_LEN 5
+#define SPI_HID_OUTPUT_HEADER_LEN 8
+
+/* Raw input buffer with data from the bus */
+struct spi_hid_input_buf {
+ u8 header[HIDSPI_INPUT_HEADER_SIZE];
+ u8 body[HIDSPI_INPUT_BODY_HEADER_SIZE];
+ u8 content[];
+};
+
+/* Raw output report buffer to be put on the bus */
+struct spi_hid_output_buf {
+ u8 header[SPI_HID_OUTPUT_HEADER_LEN];
+ u8 content[];
+};
+
+/* Processed data from a device descriptor */
+struct spi_hid_device_descriptor {
+ u16 hid_version;
+ u16 report_descriptor_length;
+ u16 max_input_length;
+ u16 max_output_length;
+ u16 max_fragment_length;
+ u16 vendor_id;
+ u16 product_id;
+ u16 version_id;
+ u8 no_output_report_ack;
+};
+
+/* Driver context */
+struct spi_hid {
+ struct spi_device *spi; /* spi device. */
+ struct hid_device *hid; /* pointer to corresponding HID dev. */
+
+ struct spi_transfer input_transfer[2]; /* Transfer buffer for read and write. */
+ struct spi_message input_message; /* used to execute a sequence of spi transfers. */
+
+ struct spihid_ops *ops;
+ struct spi_hid_conf *conf;
+
+ struct spi_hid_device_descriptor desc; /* HID device descriptor. */
+ struct spi_hid_output_buf *output; /* Output buffer. */
+ struct spi_hid_input_buf *input; /* Input buffer. */
+ struct spi_hid_input_buf *response; /* Response buffer. */
+
+ u16 response_length;
+ u16 bufsize;
+ /* Response type awaited by a sync request, 0 if none. Protected by io_lock. */
+ u8 expected_response;
+ /* Content ID of that request. Protected by io_lock. */
+ u8 expected_content_id;
+
+ enum hidspi_power_state power_state;
+
+ u8 reset_attempts; /* The number of reset attempts. */
+
+ unsigned long flags; /* device flags. */
+
+ struct work_struct reset_work;
+
+ /*
+ * Serializes request/response transactions: held from sending a
+ * request until its response in shid->response has been consumed.
+ * Taken before io_lock.
+ */
+ struct mutex output_lock;
+ /* Power lock to make sure one power state change at a time. */
+ struct mutex power_lock;
+ /*
+ * Serializes SPI bus transfers (output writes vs. IRQ-thread reads) and
+ * protects shid->expected_response, updates to shid->desc and
+ * publication of shid->hid. Held only briefly and never while waiting
+ * for a response, since the IRQ thread needs it to deliver that
+ * response. Nests inside output_lock.
+ *
+ * It does not keep the hid_device alive: the IRQ must be quiesced with
+ * disable_irq() before the hid_device is destroyed.
+ */
+ struct mutex io_lock;
+
+ struct completion output_done;
+
+ u32 report_descriptor_crc32; /* HID report descriptor crc32 checksum. */
+
+ u32 regulator_error_count;
+ int regulator_last_error;
+ u32 bus_error_count;
+ int bus_last_error;
+ u32 dir_count; /* device initiated reset count. */
+
+ /* DMA-safe transfer buffers */
+ u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN] ____cacheline_aligned;
+ u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
+};
+
+#endif /* SPI_HID_CORE_H */
diff --git a/drivers/hid/spi-hid/spi-hid-trace.h b/drivers/hid/spi-hid/spi-hid-trace.h
new file mode 100644
index 000000000000..a36e07b95001
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-trace.h
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Microsoft Corporation
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM spi_hid
+
+#if !defined(_SPI_HID_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _SPI_HID_TRACE_H
+
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+#include "spi-hid-core.h"
+
+/*
+ * Maximum number of tx/rx bytes copied into the ring buffer per event. This
+ * matches the most %*ph will print, keeps events small, and ensures the copy
+ * never depends on the caller-supplied length fitting inside the buffer.
+ */
+#define SPI_HID_TRACE_BUF_MAX 64
+
+DECLARE_EVENT_CLASS(spi_hid_transfer,
+ TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+ const void *rx_buf, u16 rx_len, int ret),
+
+ TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret),
+
+ TP_STRUCT__entry(
+ __field(int, bus_num)
+ __field(int, chip_select)
+ __field(int, ret)
+ __field(int, tx_len)
+ __field(u16, rx_len)
+ __dynamic_array(u8, rx_buf,
+ min_t(u16, rx_len, SPI_HID_TRACE_BUF_MAX))
+ __dynamic_array(u8, tx_buf,
+ min_t(int, tx_len, SPI_HID_TRACE_BUF_MAX))
+ ),
+
+ TP_fast_assign(
+ __entry->bus_num = shid->spi->controller->bus_num;
+ __entry->chip_select = spi_get_chipselect(shid->spi, 0);
+ __entry->ret = ret;
+ __entry->tx_len = tx_len;
+ __entry->rx_len = rx_len;
+
+ memcpy(__get_dynamic_array(tx_buf), tx_buf,
+ __get_dynamic_array_len(tx_buf));
+ memcpy(__get_dynamic_array(rx_buf), rx_buf,
+ __get_dynamic_array_len(rx_buf));
+ ),
+
+ TP_printk("spi%d.%d: len=%d tx=[%*phD] rx=[%*phD] --> %d",
+ __entry->bus_num, __entry->chip_select,
+ __entry->tx_len + __entry->rx_len,
+ __get_dynamic_array_len(tx_buf), __get_dynamic_array(tx_buf),
+ __get_dynamic_array_len(rx_buf), __get_dynamic_array(rx_buf),
+ __entry->ret)
+);
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_input_sync,
+ TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+ const void *rx_buf, u16 rx_len, int ret),
+ TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret));
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_input_header_complete,
+ TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+ const void *rx_buf, u16 rx_len, int ret),
+ TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret));
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_input_body_complete,
+ TP_PROTO(struct spi_hid *shid, const void *tx_buf, int tx_len,
+ const void *rx_buf, u16 rx_len, int ret),
+ TP_ARGS(shid, tx_buf, tx_len, rx_buf, rx_len, ret));
+
+DECLARE_EVENT_CLASS(spi_hid_irq,
+ TP_PROTO(struct spi_hid *shid, int irq),
+
+ TP_ARGS(shid, irq),
+
+ TP_STRUCT__entry(
+ __field(int, bus_num)
+ __field(int, chip_select)
+ __field(int, irq)
+ ),
+
+ TP_fast_assign(
+ __entry->bus_num = shid->spi->controller->bus_num;
+ __entry->chip_select = spi_get_chipselect(shid->spi, 0);
+ __entry->irq = irq;
+ ),
+
+ TP_printk("spi%d.%d: IRQ %d",
+ __entry->bus_num, __entry->chip_select, __entry->irq)
+);
+
+DEFINE_EVENT(spi_hid_irq, spi_hid_dev_irq,
+ TP_PROTO(struct spi_hid *shid, int irq), TP_ARGS(shid, irq));
+
+DECLARE_EVENT_CLASS(spi_hid,
+ TP_PROTO(struct spi_hid *shid),
+
+ TP_ARGS(shid),
+
+ TP_STRUCT__entry(
+ __field(int, bus_num)
+ __field(int, chip_select)
+ __field(int, power_state)
+ __field(u32, flags)
+
+ __field(int, vendor_id)
+ __field(int, product_id)
+ __field(int, max_input_length)
+ __field(int, max_output_length)
+ __field(u16, hid_version)
+ __field(u16, report_descriptor_length)
+ __field(u16, version_id)
+ ),
+
+ TP_fast_assign(
+ __entry->bus_num = shid->spi->controller->bus_num;
+ __entry->chip_select = spi_get_chipselect(shid->spi, 0);
+ __entry->power_state = shid->power_state;
+ __entry->flags = shid->flags;
+
+ __entry->vendor_id = shid->desc.vendor_id;
+ __entry->product_id = shid->desc.product_id;
+ __entry->max_input_length = shid->desc.max_input_length;
+ __entry->max_output_length = shid->desc.max_output_length;
+ __entry->hid_version = shid->desc.hid_version;
+ __entry->report_descriptor_length =
+ shid->desc.report_descriptor_length;
+ __entry->version_id = shid->desc.version_id;
+ ),
+
+ TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state p:%d len i:%d o:%d r:%d flags 0x%08x",
+ __entry->bus_num, __entry->chip_select,
+ __entry->vendor_id, __entry->product_id, __entry->version_id,
+ __entry->hid_version >> 8, __entry->hid_version & 0xff,
+ __entry->power_state, __entry->max_input_length,
+ __entry->max_output_length, __entry->report_descriptor_length,
+ __entry->flags)
+);
+
+DEFINE_EVENT(spi_hid, spi_hid_header_transfer, TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_process_input_report,
+ TP_PROTO(struct spi_hid *shid), TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_input_report_handler,
+ TP_PROTO(struct spi_hid *shid), TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_reset_response, TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_create_device, TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_refresh_device, TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_response_handler, TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid));
+
+DEFINE_EVENT(spi_hid, spi_hid_error_handler, TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid));
+
+#endif /* _SPI_HID_TRACE_H */
+
+/*
+ * The following must be outside the protection of the above #if block.
+ */
+#undef TRACE_INCLUDE_PATH
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_PATH .
+
+/*
+ * It is required that the TRACE_INCLUDE_FILE be the same
+ * as this file without the ".h".
+ */
+#define TRACE_INCLUDE_FILE spi-hid-trace
+#include <trace/define_trace.h>
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 07/11] HID: spi-hid: add ACPI support for HID over SPI
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (5 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 06/11] HID: spi-hid: add spi_hid traces Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
` (3 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Angela Czubak
From: Angela Czubak <acz@semihalf.com>
Detect SPI HID devices described in ACPI.
Signed-off-by: Angela Czubak <acz@semihalf.com>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/Kconfig | 16 +++
drivers/hid/spi-hid/Makefile | 1 +
drivers/hid/spi-hid/spi-hid-acpi.c | 261 +++++++++++++++++++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-core.c | 48 +++----
drivers/hid/spi-hid/spi-hid.h | 45 +++++++
5 files changed, 344 insertions(+), 27 deletions(-)
diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
index 836fdefe8345..303ada03a1bf 100644
--- a/drivers/hid/spi-hid/Kconfig
+++ b/drivers/hid/spi-hid/Kconfig
@@ -10,6 +10,22 @@ menuconfig SPI_HID
if SPI_HID
+config SPI_HID_ACPI
+ tristate "HID over SPI transport layer ACPI driver"
+ depends on ACPI
+ select RESET_CONTROLLER
+ select SPI_HID_CORE
+ help
+ Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+ other HID based devices which are connected to your computer via SPI.
+ This driver supports ACPI-based systems.
+
+ If unsure, say N.
+
+ This support is also available as a module. If so, the module
+ will be called spi-hid-acpi. It will also build/depend on the
+ module spi-hid.
+
config SPI_HID_CORE
tristate
endif
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
index 733e006df56e..3ca326602643 100644
--- a/drivers/hid/spi-hid/Makefile
+++ b/drivers/hid/spi-hid/Makefile
@@ -8,3 +8,4 @@
obj-$(CONFIG_SPI_HID_CORE) += spi-hid.o
spi-hid-objs = spi-hid-core.o
CFLAGS_spi-hid-core.o := -I$(src)
+obj-$(CONFIG_SPI_HID_ACPI) += spi-hid-acpi.o
diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
new file mode 100644
index 000000000000..31ca3fa336cd
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-acpi.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol, ACPI related code
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ *
+ * This code was forked out of the HID over SPI core code, which is partially
+ * based on "HID over I2C protocol implementation:
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * which in turn is partially based on "USB HID support for Linux":
+ *
+ * Copyright (c) 1999 Andreas Gal
+ * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ * Copyright (c) 2007-2008 Oliver Neukum
+ * Copyright (c) 2006-2010 Jiri Kosina
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/reset.h>
+#include <linux/uuid.h>
+
+#include "spi-hid.h"
+
+/* Config structure is filled with data from ACPI */
+struct spi_hid_acpi_config {
+ struct spihid_ops ops;
+
+ struct spi_hid_conf property_conf;
+ u32 post_power_on_delay_ms;
+ u32 minimal_reset_delay_ms;
+ struct acpi_device *adev;
+ struct device *dev;
+};
+
+/* HID SPI Device: 6e2ac436-0fcf41af-a265-b32a220dcfab */
+static guid_t spi_hid_guid =
+ GUID_INIT(0x6E2AC436, 0x0FCF, 0x41AF,
+ 0xA2, 0x65, 0xB3, 0x2A, 0x22, 0x0D, 0xCF, 0xAB);
+
+static int spi_hid_acpi_populate_config(struct spi_hid_acpi_config *conf,
+ struct acpi_device *adev)
+{
+ acpi_handle handle = acpi_device_handle(adev);
+ union acpi_object *obj;
+
+ conf->adev = adev;
+
+ /* Revision 3 for HID over SPI V1, see specification. */
+ obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 1, NULL,
+ ACPI_TYPE_INTEGER);
+ if (!obj) {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID input report header address failed\n");
+ return -ENODEV;
+ }
+ conf->property_conf.input_report_header_address = obj->integer.value;
+ ACPI_FREE(obj);
+
+ obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 2, NULL,
+ ACPI_TYPE_INTEGER);
+ if (!obj) {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID input report body address failed\n");
+ return -ENODEV;
+ }
+ conf->property_conf.input_report_body_address = obj->integer.value;
+ ACPI_FREE(obj);
+
+ obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 3, NULL,
+ ACPI_TYPE_INTEGER);
+ if (!obj) {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID output report header address failed\n");
+ return -ENODEV;
+ }
+ conf->property_conf.output_report_address = obj->integer.value;
+ ACPI_FREE(obj);
+
+ obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 4, NULL,
+ ACPI_TYPE_BUFFER);
+ if (!obj) {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID read opcode failed\n");
+ return -ENODEV;
+ }
+ if (obj->buffer.length == 1) {
+ conf->property_conf.read_opcode = obj->buffer.pointer[0];
+ } else {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID read opcode, too long buffer\n");
+ ACPI_FREE(obj);
+ return -ENODEV;
+ }
+ ACPI_FREE(obj);
+
+ obj = acpi_evaluate_dsm_typed(handle, &spi_hid_guid, 3, 5, NULL,
+ ACPI_TYPE_BUFFER);
+ if (!obj) {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID write opcode failed\n");
+ return -ENODEV;
+ }
+ if (obj->buffer.length == 1) {
+ conf->property_conf.write_opcode = obj->buffer.pointer[0];
+ } else {
+ acpi_handle_err(handle,
+ "Error _DSM call to get HID write opcode, too long buffer\n");
+ ACPI_FREE(obj);
+ return -ENODEV;
+ }
+ ACPI_FREE(obj);
+
+ /* Value not provided in ACPI,*/
+ conf->post_power_on_delay_ms = 5;
+ conf->minimal_reset_delay_ms = 150;
+
+ if (!acpi_has_method(handle, "_RST")) {
+ acpi_handle_err(handle, "No reset method for acpi handle\n");
+ return -EINVAL;
+ }
+
+ /* FIXME: not reading hid-over-spi-flags, multi-SPI not supported */
+
+ return 0;
+}
+
+static int spi_hid_acpi_power_none(struct spihid_ops *ops)
+{
+ return 0;
+}
+
+static int spi_hid_acpi_power_down(struct spihid_ops *ops)
+{
+ struct spi_hid_acpi_config *conf = container_of(ops,
+ struct spi_hid_acpi_config,
+ ops);
+
+ return acpi_device_set_power(conf->adev, ACPI_STATE_D3);
+}
+
+static int spi_hid_acpi_power_up(struct spihid_ops *ops)
+{
+ struct spi_hid_acpi_config *conf = container_of(ops,
+ struct spi_hid_acpi_config,
+ ops);
+ int error;
+
+ error = acpi_device_set_power(conf->adev, ACPI_STATE_D0);
+ if (error) {
+ dev_err(&conf->adev->dev, "Error could not power up ACPI device: %d\n", error);
+ return error;
+ }
+
+ if (conf->post_power_on_delay_ms)
+ msleep(conf->post_power_on_delay_ms);
+
+ return 0;
+}
+
+static int spi_hid_acpi_assert_reset(struct spihid_ops *ops)
+{
+ return 0;
+}
+
+static int spi_hid_acpi_deassert_reset(struct spihid_ops *ops)
+{
+ struct spi_hid_acpi_config *conf = container_of(ops,
+ struct spi_hid_acpi_config,
+ ops);
+
+ /*
+ * Probe guarantees an ACPI reset method exists. Use the optional
+ * variant so that the absence of an additional reset controller
+ * binding is not reported as an error after a successful reset.
+ */
+ return device_reset_optional(conf->dev);
+}
+
+static void spi_hid_acpi_sleep_minimal_reset_delay(struct spihid_ops *ops)
+{
+ struct spi_hid_acpi_config *conf = container_of(ops,
+ struct spi_hid_acpi_config,
+ ops);
+ fsleep(1000 * conf->minimal_reset_delay_ms);
+}
+
+static int spi_hid_acpi_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct acpi_device *adev;
+ struct spi_hid_acpi_config *config;
+ int error;
+
+ adev = ACPI_COMPANION(dev);
+ if (!adev) {
+ dev_err(dev, "Error could not get ACPI device\n");
+ return -ENODEV;
+ }
+
+ config = devm_kzalloc(dev, sizeof(struct spi_hid_acpi_config),
+ GFP_KERNEL);
+ if (!config)
+ return -ENOMEM;
+
+ config->dev = dev;
+
+ if (acpi_device_power_manageable(adev)) {
+ config->ops.power_up = spi_hid_acpi_power_up;
+ config->ops.power_down = spi_hid_acpi_power_down;
+ } else {
+ config->ops.power_up = spi_hid_acpi_power_none;
+ config->ops.power_down = spi_hid_acpi_power_none;
+ }
+ config->ops.assert_reset = spi_hid_acpi_assert_reset;
+ config->ops.deassert_reset = spi_hid_acpi_deassert_reset;
+ config->ops.sleep_minimal_reset_delay =
+ spi_hid_acpi_sleep_minimal_reset_delay;
+
+ error = spi_hid_acpi_populate_config(config, adev);
+ if (error) {
+ dev_err(dev, "%s: unable to populate config data\n", __func__);
+ return error;
+ }
+
+ return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
+}
+
+static const struct acpi_device_id spi_hid_acpi_match[] = {
+ { "ACPI0C51", 0 },
+ { "PNP0C51", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, spi_hid_acpi_match);
+
+static struct spi_driver spi_hid_acpi_driver = {
+ .driver = {
+ .name = "spi_hid_acpi",
+ .owner = THIS_MODULE,
+ .acpi_match_table = spi_hid_acpi_match,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ .dev_groups = spi_hid_groups,
+ },
+ .probe = spi_hid_acpi_probe,
+ .remove = spi_hid_core_remove,
+};
+
+module_spi_driver(spi_hid_acpi_driver);
+
+MODULE_DESCRIPTION("HID over SPI ACPI transport driver");
+MODULE_AUTHOR("Angela Czubak <aczubak@google.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index e428fae762ac..96805c10bfe3 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -44,6 +44,7 @@
#include <linux/wait.h>
#include <linux/workqueue.h>
+#include "spi-hid.h"
#include "spi-hid-core.h"
#define CREATE_TRACE_POINTS
@@ -115,31 +116,6 @@ struct spi_hid_output_report {
u8 *content;
};
-/* struct spi_hid_conf - Conf provided to the core */
-struct spi_hid_conf {
- u32 input_report_header_address;
- u32 input_report_body_address;
- u32 output_report_address;
- u8 read_opcode;
- u8 write_opcode;
-};
-
-/**
- * struct spihid_ops - Ops provided to the core
- * @power_up: do sequencing to power up the device
- * @power_down: do sequencing to power down the device
- * @assert_reset: do sequencing to assert the reset line
- * @deassert_reset: do sequencing to deassert the reset line
- * @sleep_minimal_reset_delay: minimal sleep delay during reset
- */
-struct spihid_ops {
- int (*power_up)(struct spihid_ops *ops);
- int (*power_down)(struct spihid_ops *ops);
- int (*assert_reset)(struct spihid_ops *ops);
- int (*deassert_reset)(struct spihid_ops *ops);
- void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
-};
-
static struct hid_ll_driver spi_hid_ll_driver;
static void spi_hid_populate_read_approvals(const struct spi_hid_conf *conf,
@@ -327,8 +303,21 @@ static void spi_hid_error_handler(struct spi_hid *shid)
shid->power_state = HIDSPI_ON;
- shid->ops->deassert_reset(shid->ops);
+ error = shid->ops->deassert_reset(shid->ops);
+ if (error) {
+ dev_err(dev, "failed to deassert reset: %d\n", error);
+ shid->bus_error_count++;
+ shid->bus_last_error = error;
+ /*
+ * Without a successful reset the device will not send a reset
+ * response, so no IRQ would trigger another recovery attempt.
+ * Retry from here; reset_attempts bounds the number of tries.
+ */
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
+ }
+ /* Balance disable_irq() above; a retry disables it again. */
enable_irq(shid->spi->irq);
}
@@ -1431,7 +1420,12 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
return error;
}
- shid->ops->deassert_reset(shid->ops);
+ error = shid->ops->deassert_reset(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: failed to deassert reset: %d\n", __func__, error);
+ shid->ops->power_down(shid->ops);
+ return error;
+ }
enable_irq(spi->irq);
diff --git a/drivers/hid/spi-hid/spi-hid.h b/drivers/hid/spi-hid/spi-hid.h
new file mode 100644
index 000000000000..f5a5f4d54beb
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Microsoft Corporation
+ * Copyright (c) 2026 Google LLC
+ */
+
+#ifndef SPI_HID_H
+#define SPI_HID_H
+
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+
+/* struct spi_hid_conf - Conf provided to the core */
+struct spi_hid_conf {
+ u32 input_report_header_address;
+ u32 input_report_body_address;
+ u32 output_report_address;
+ u8 read_opcode;
+ u8 write_opcode;
+};
+
+/**
+ * struct spihid_ops - Ops provided to the core
+ * @power_up: do sequencing to power up the device
+ * @power_down: do sequencing to power down the device
+ * @assert_reset: do sequencing to assert the reset line
+ * @deassert_reset: do sequencing to deassert the reset line
+ * @sleep_minimal_reset_delay: minimal sleep delay during reset
+ */
+struct spihid_ops {
+ int (*power_up)(struct spihid_ops *ops);
+ int (*power_down)(struct spihid_ops *ops);
+ int (*assert_reset)(struct spihid_ops *ops);
+ int (*deassert_reset)(struct spihid_ops *ops);
+ void (*sleep_minimal_reset_delay)(struct spihid_ops *ops);
+};
+
+int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
+ struct spi_hid_conf *conf);
+
+void spi_hid_core_remove(struct spi_device *spi);
+
+extern const struct attribute_group *spi_hid_groups[];
+
+#endif /* SPI_HID_H */
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (6 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 07/11] HID: spi-hid: add ACPI support for HID over SPI Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-20 16:34 ` Rob Herring (Arm)
2026-10-20 17:57 ` Conor Dooley
2026-10-19 19:01 ` [PATCH v6 09/11] HID: spi-hid: add device tree support for HID over SPI Jingyuan Liang
` (2 subsequent siblings)
10 siblings, 2 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Dmitry Antipov, Jarrett Schultz
Documentation describes the required and optional properties for
implementing Device Tree for a Microsoft G6 Touch Digitizer that
supports HID over SPI Protocol 1.0 specification.
The properties are common to HID over SPI.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
.../devicetree/bindings/input/hid-over-spi.yaml | 130 +++++++++++++++++++++
1 file changed, 130 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
new file mode 100644
index 000000000000..2a5fa0868a09
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/hid-over-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HID over SPI Devices
+
+maintainers:
+ - Benjamin Tissoires <benjamin.tissoires@redhat.com>
+ - Jiri Kosina <jkosina@suse.cz>
+ - Jingyuan Liang <jingyliang@chromium.org>
+
+description: |+
+ HID over SPI provides support for various Human Interface Devices over the
+ SPI bus. These devices can be for example touchpads, keyboards, touch screens
+ or sensors.
+
+ The specification has been written by Microsoft and is currently available
+ here: https://www.microsoft.com/en-us/download/details.aspx?id=103325
+
+ The HID over SPI specification does not define fixed values for the
+ read/write opcodes and the input/output report addresses. The host
+ retrieves them from platform firmware (ACPI _DSM), and they differ
+ between devices. These properties provide the same values on Device
+ Tree systems, so devices that fully comply with the specification work
+ with the "hid-over-spi" fallback compatible.
+
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - microsoft,g6-touch-digitizer
+ - const: hid-over-spi
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+ description:
+ GPIO specifier for the device's reset line. The HID over SPI
+ specification requires a dedicated, host-driven, active-low reset
+ line. The line must be flagged with GPIO_ACTIVE_LOW.
+
+ vdd-supply:
+ description:
+ Regulator for the VDD supply voltage.
+
+ input-report-header-address:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 0xffffff
+ description:
+ A value to be included in the Read Approval packet, listing an address of
+ the input report header to be put on the SPI bus. This address has 24
+ bits.
+
+ input-report-body-address:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 0xffffff
+ description:
+ A value to be included in the Read Approval packet, listing an address of
+ the input report body to be put on the SPI bus. This address has 24 bits.
+
+ output-report-address:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 0xffffff
+ description:
+ A value to be included in the Output Report sent by the host, listing an
+ address where the output report on the SPI bus is to be written to. This
+ address has 24 bits.
+
+ read-opcode:
+ $ref: /schemas/types.yaml#/definitions/uint8
+ description:
+ Value to be used in Read Approval packets. 1 byte.
+
+ write-opcode:
+ $ref: /schemas/types.yaml#/definitions/uint8
+ description:
+ Value to be used in Output Report packets. 1 byte.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - reset-gpios
+ - vdd-supply
+ - input-report-header-address
+ - input-report-body-address
+ - output-report-address
+ - read-opcode
+ - write-opcode
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ hid@0 {
+ compatible = "microsoft,g6-touch-digitizer", "hid-over-spi";
+ reg = <0x0>;
+ interrupts-extended = <&gpio 42 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+ vdd-supply = <&pm8350c_l3>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&ts_d6_int_bias>;
+ input-report-header-address = <0x1000>;
+ input-report-body-address = <0x1004>;
+ output-report-address = <0x2000>;
+ read-opcode = /bits/ 8 <0x0b>;
+ write-opcode = /bits/ 8 <0x02>;
+ };
+ };
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 09/11] HID: spi-hid: add device tree support for HID over SPI
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (7 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 11/11] HID: spi-hid: add panel follower support Jingyuan Liang
10 siblings, 0 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang, Jarrett Schultz, Dmitry Antipov
From: Jarrett Schultz <jaschultz@microsoft.com>
Detect SPI HID devices described in Device Tree.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/Kconfig | 15 +++
drivers/hid/spi-hid/Makefile | 1 +
drivers/hid/spi-hid/spi-hid-of.c | 241 +++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
index 303ada03a1bf..63f202975ac3 100644
--- a/drivers/hid/spi-hid/Kconfig
+++ b/drivers/hid/spi-hid/Kconfig
@@ -26,6 +26,21 @@ config SPI_HID_ACPI
will be called spi-hid-acpi. It will also build/depend on the
module spi-hid.
+config SPI_HID_OF
+ tristate "HID over SPI transport layer Open Firmware driver"
+ depends on OF
+ select SPI_HID_CORE
+ help
+ Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+ other HID based devices which are connected to your computer via SPI.
+ This driver supports Open Firmware (Device Tree)-based systems.
+
+ If unsure, say N.
+
+ This support is also available as a module. If so, the module
+ will be called spi-hid-of. It will also build/depend on the
+ module spi-hid.
+
config SPI_HID_CORE
tristate
endif
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
index 3ca326602643..31192e71edae 100644
--- a/drivers/hid/spi-hid/Makefile
+++ b/drivers/hid/spi-hid/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_SPI_HID_CORE) += spi-hid.o
spi-hid-objs = spi-hid-core.o
CFLAGS_spi-hid-core.o := -I$(src)
obj-$(CONFIG_SPI_HID_ACPI) += spi-hid-acpi.o
+obj-$(CONFIG_SPI_HID_OF) += spi-hid-of.o
diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
new file mode 100644
index 000000000000..24215ffaaafd
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol, Open Firmware related code
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ *
+ * This code was forked out of the HID over SPI core code, which is partially
+ * based on "HID over I2C protocol implementation:
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * which in turn is partially based on "USB HID support for Linux":
+ *
+ * Copyright (c) 1999 Andreas Gal
+ * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ * Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ * Copyright (c) 2007-2008 Oliver Neukum
+ * Copyright (c) 2006-2010 Jiri Kosina
+ */
+
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+#include "spi-hid.h"
+
+struct spi_hid_timing_data {
+ u32 post_power_on_delay_ms;
+ u32 minimal_reset_delay_ms;
+};
+
+/* Config structure is filled with data from Device Tree */
+struct spi_hid_of_config {
+ struct spihid_ops ops;
+
+ struct spi_hid_conf property_conf;
+ const struct spi_hid_timing_data *timing_data;
+
+ struct gpio_desc *reset_gpio;
+ struct regulator *supply;
+ bool supply_enabled;
+};
+
+static const struct spi_hid_timing_data timing_data = {
+ .post_power_on_delay_ms = 10,
+ .minimal_reset_delay_ms = 100,
+};
+
+static int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
+ struct device *dev)
+{
+ int error;
+ u32 val;
+
+ error = device_property_read_u32(dev, "input-report-header-address",
+ &val);
+ if (error) {
+ dev_err(dev, "Input report header address not provided\n");
+ return -ENODEV;
+ }
+ conf->property_conf.input_report_header_address = val;
+
+ error = device_property_read_u32(dev, "input-report-body-address", &val);
+ if (error) {
+ dev_err(dev, "Input report body address not provided\n");
+ return -ENODEV;
+ }
+ conf->property_conf.input_report_body_address = val;
+
+ error = device_property_read_u32(dev, "output-report-address", &val);
+ if (error) {
+ dev_err(dev, "Output report address not provided\n");
+ return -ENODEV;
+ }
+ conf->property_conf.output_report_address = val;
+
+ error = device_property_read_u8(dev, "read-opcode",
+ &conf->property_conf.read_opcode);
+ if (error) {
+ dev_err(dev, "Read opcode not provided\n");
+ return -ENODEV;
+ }
+
+ error = device_property_read_u8(dev, "write-opcode",
+ &conf->property_conf.write_opcode);
+ if (error) {
+ dev_err(dev, "Write opcode not provided\n");
+ return -ENODEV;
+ }
+
+ conf->supply = devm_regulator_get(dev, "vdd");
+ if (IS_ERR(conf->supply))
+ return dev_err_probe(dev, PTR_ERR(conf->supply),
+ "Failed to get regulator\n");
+ conf->supply_enabled = false;
+
+ conf->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(conf->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(conf->reset_gpio),
+ "Failed to get reset GPIO\n");
+
+ return 0;
+}
+
+static int spi_hid_of_power_down(struct spihid_ops *ops)
+{
+ struct spi_hid_of_config *conf = container_of(ops,
+ struct spi_hid_of_config,
+ ops);
+ int error;
+
+ if (!conf->supply_enabled)
+ return 0;
+
+ error = regulator_disable(conf->supply);
+ if (error == 0)
+ conf->supply_enabled = false;
+
+ return error;
+}
+
+static int spi_hid_of_power_up(struct spihid_ops *ops)
+{
+ struct spi_hid_of_config *conf = container_of(ops,
+ struct spi_hid_of_config,
+ ops);
+ int error;
+
+ if (conf->supply_enabled)
+ return 0;
+
+ error = regulator_enable(conf->supply);
+
+ if (error == 0) {
+ conf->supply_enabled = true;
+ fsleep(1000 * conf->timing_data->post_power_on_delay_ms);
+ }
+
+ return error;
+}
+
+static int spi_hid_of_assert_reset(struct spihid_ops *ops)
+{
+ struct spi_hid_of_config *conf = container_of(ops,
+ struct spi_hid_of_config,
+ ops);
+
+ return gpiod_set_value_cansleep(conf->reset_gpio, 1);
+}
+
+static int spi_hid_of_deassert_reset(struct spihid_ops *ops)
+{
+ struct spi_hid_of_config *conf = container_of(ops,
+ struct spi_hid_of_config,
+ ops);
+
+ return gpiod_set_value_cansleep(conf->reset_gpio, 0);
+}
+
+static void spi_hid_of_sleep_minimal_reset_delay(struct spihid_ops *ops)
+{
+ struct spi_hid_of_config *conf = container_of(ops,
+ struct spi_hid_of_config,
+ ops);
+ fsleep(1000 * conf->timing_data->minimal_reset_delay_ms);
+}
+
+static int spi_hid_of_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct spi_hid_of_config *config;
+ int error;
+
+ config = devm_kzalloc(dev, sizeof(struct spi_hid_of_config),
+ GFP_KERNEL);
+ if (!config)
+ return -ENOMEM;
+
+ config->ops.power_up = spi_hid_of_power_up;
+ config->ops.power_down = spi_hid_of_power_down;
+ config->ops.assert_reset = spi_hid_of_assert_reset;
+ config->ops.deassert_reset = spi_hid_of_deassert_reset;
+ config->ops.sleep_minimal_reset_delay =
+ spi_hid_of_sleep_minimal_reset_delay;
+
+ config->timing_data = device_get_match_data(dev);
+ if (!config->timing_data)
+ config->timing_data = &timing_data;
+
+ /*
+ * FIXME: multi-SPI not supported. Once it is, derive the
+ * HID-over-SPI flags from spi->mode.
+ */
+
+ error = spi_hid_of_populate_config(config, dev);
+ if (error)
+ return dev_err_probe(dev, error, "Unable to populate config data\n");
+
+ return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
+}
+
+static const struct of_device_id spi_hid_of_match[] = {
+ { .compatible = "hid-over-spi", .data = &timing_data },
+ { }
+};
+MODULE_DEVICE_TABLE(of, spi_hid_of_match);
+
+static const struct spi_device_id spi_hid_of_id_table[] = {
+ { .name = "hid" },
+ { .name = "hid-over-spi" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, spi_hid_of_id_table);
+
+static struct spi_driver spi_hid_of_driver = {
+ .driver = {
+ .name = "spi_hid_of",
+ .owner = THIS_MODULE,
+ .of_match_table = spi_hid_of_match,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ .dev_groups = spi_hid_groups,
+ },
+ .probe = spi_hid_of_probe,
+ .remove = spi_hid_core_remove,
+ .id_table = spi_hid_of_id_table,
+};
+
+module_spi_driver(spi_hid_of_driver);
+
+MODULE_DESCRIPTION("HID over SPI OF transport driver");
+MODULE_AUTHOR("Dmitry Antipov <dmanti@microsoft.com>");
+MODULE_LICENSE("GPL");
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 10/11] HID: spi-hid: add power management implementation
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (8 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 09/11] HID: spi-hid: add device tree support for HID over SPI Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
2026-10-19 19:36 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 11/11] HID: spi-hid: add panel follower support Jingyuan Liang
10 siblings, 1 reply; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang
Implement HID over SPI driver power management callbacks.
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/spi-hid-acpi.c | 1 +
drivers/hid/spi-hid/spi-hid-core.c | 185 +++++++++++++++++++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-core.h | 2 +
drivers/hid/spi-hid/spi-hid-of.c | 1 +
drivers/hid/spi-hid/spi-hid.h | 1 +
5 files changed, 190 insertions(+)
diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
index 31ca3fa336cd..a52cafe86c05 100644
--- a/drivers/hid/spi-hid/spi-hid-acpi.c
+++ b/drivers/hid/spi-hid/spi-hid-acpi.c
@@ -246,6 +246,7 @@ static struct spi_driver spi_hid_acpi_driver = {
.driver = {
.name = "spi_hid_acpi",
.owner = THIS_MODULE,
+ .pm = &spi_hid_core_pm,
.acpi_match_table = spi_hid_acpi_match,
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
.dev_groups = spi_hid_groups,
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 96805c10bfe3..c2232d5959b8 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -36,6 +36,8 @@
#include <linux/list.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_wakeirq.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <linux/string.h>
@@ -60,6 +62,7 @@
#define SPI_HID_MAX_RESET_ATTEMPTS 3
#define SPI_HID_RESP_TIMEOUT 1000
+#define SPI_HID_RESET_TIMEOUT 1000
/* Protocol message size constants */
#define SPI_HID_READ_APPROVAL_LEN 5
@@ -91,6 +94,11 @@
* ll_close. Input reports are only forwarded while this flag is set.
*/
#define SPI_HID_STARTED 6
+/*
+ * dead indicates that the device was powered off after too many failed
+ * resets. It stays off until the driver is rebound, even across resume.
+ */
+#define SPI_HID_DEAD 7
/* Processed data from input report header */
struct spi_hid_input_header {
@@ -242,6 +250,148 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
}
}
+/*
+ * Wait for the device descriptor response after reset; the device then takes
+ * requests. Set READY here: the report descriptor check that normally sets it
+ * needs power_lock, which callers hold.
+ */
+static void spi_hid_wait_for_reset(struct spi_hid *shid)
+{
+ unsigned long timeout = msecs_to_jiffies(SPI_HID_RESET_TIMEOUT +
+ SPI_HID_RESP_TIMEOUT);
+
+ if (!wait_for_completion_timeout(&shid->reset_done, timeout)) {
+ dev_warn(&shid->spi->dev,
+ "Device descriptor not received after reset\n");
+ return;
+ }
+
+ set_bit(SPI_HID_READY, &shid->flags);
+}
+
+static int spi_hid_suspend(struct spi_hid *shid)
+{
+ int error;
+ struct device *dev = &shid->spi->dev;
+
+ /* HID driver suspend may talk to the device; responses need the IRQ. */
+ scoped_guard(mutex, &shid->power_lock) {
+ if (shid->power_state != HIDSPI_OFF && shid->hid) {
+ error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
+ if (error) {
+ dev_err(dev, "%s failed to suspend hid driver: %d\n",
+ __func__, error);
+ return error;
+ }
+ }
+ }
+
+ /*
+ * disable_irq() waits for a running IRQ thread, so no new reset_work
+ * can be queued after it returns. Then wait for any queued or running
+ * instance. Must not hold power_lock here: reset_work takes it.
+ */
+ disable_irq(shid->spi->irq);
+ cancel_work_sync(&shid->reset_work);
+
+ guard(mutex)(&shid->power_lock);
+ if (!device_may_wakeup(dev)) {
+ if (shid->power_state != HIDSPI_OFF) {
+ /* Device is fully reset on resume; drop stale state. */
+ clear_bit(SPI_HID_READY, &shid->flags);
+ clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
+ clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
+ clear_bit(SPI_HID_ERROR, &shid->flags);
+ set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+
+ shid->ops->assert_reset(shid->ops);
+
+ error = shid->ops->power_down(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: could not power down\n", __func__);
+ shid->regulator_error_count++;
+ shid->regulator_last_error = error;
+ /*
+ * Undo partial suspend before returning error.
+ * Complete the reset; RESET_PENDING stays set
+ * until the device sends its reset response.
+ */
+ shid->ops->sleep_minimal_reset_delay(shid->ops);
+ reinit_completion(&shid->reset_done);
+ shid->ops->deassert_reset(shid->ops);
+ enable_irq(shid->spi->irq);
+ if (shid->hid) {
+ spi_hid_wait_for_reset(shid);
+ hid_driver_reset_resume(shid->hid);
+ }
+ return error;
+ }
+
+ shid->power_state = HIDSPI_OFF;
+ }
+ }
+ return 0;
+}
+
+static int spi_hid_resume(struct spi_hid *shid)
+{
+ int error;
+ struct device *dev = &shid->spi->dev;
+ bool was_reset = false;
+
+ guard(mutex)(&shid->power_lock);
+ if (test_bit(SPI_HID_DEAD, &shid->flags)) {
+ /* Stay off; only balance disable_irq() in suspend. */
+ enable_irq(shid->spi->irq);
+ return 0;
+ }
+
+ if (!device_may_wakeup(dev)) {
+ if (shid->power_state == HIDSPI_OFF) {
+ shid->ops->assert_reset(shid->ops);
+
+ shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+ error = shid->ops->power_up(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: could not power up\n", __func__);
+ shid->regulator_error_count++;
+ shid->regulator_last_error = error;
+ /* Suspend runs next even if resume fails. */
+ enable_irq(shid->spi->irq);
+ return error;
+ }
+ shid->power_state = HIDSPI_ON;
+ reinit_completion(&shid->reset_done);
+ shid->ops->deassert_reset(shid->ops);
+ was_reset = true;
+ }
+ }
+
+ enable_irq(shid->spi->irq);
+
+ /*
+ * For devices that may wake up, process work that was pending when
+ * suspend cancelled reset_work. Otherwise suspend already dropped it.
+ */
+ if (test_bit(SPI_HID_RESET_RESPONSE, &shid->flags) ||
+ test_bit(SPI_HID_CREATE_DEVICE, &shid->flags) ||
+ test_bit(SPI_HID_ERROR, &shid->flags))
+ schedule_work(&shid->reset_work);
+
+ if (shid->hid) {
+ if (was_reset)
+ spi_hid_wait_for_reset(shid);
+ error = hid_driver_reset_resume(shid->hid);
+ if (error) {
+ dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
+ __func__, error);
+ return error;
+ }
+ }
+ return 0;
+}
+
static void spi_hid_stop_hid(struct spi_hid *shid)
{
struct hid_device *hid;
@@ -280,6 +430,7 @@ static void spi_hid_error_handler(struct spi_hid *shid)
shid->regulator_last_error = error;
}
shid->power_state = HIDSPI_OFF;
+ set_bit(SPI_HID_DEAD, &shid->flags);
/* Dead device: leave the IRQ permanently disabled. */
return;
}
@@ -561,6 +712,8 @@ static int spi_hid_dev_desc_response(struct spi_hid *shid,
shid->reset_attempts = 0;
set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
schedule_work(&shid->reset_work);
+ /* The device can take requests now; see spi_hid_wait_for_reset(). */
+ complete(&shid->reset_done);
return 0;
}
@@ -1381,6 +1534,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
mutex_init(&shid->power_lock);
mutex_init(&shid->io_lock);
init_completion(&shid->output_done);
+ init_completion(&shid->reset_done);
INIT_WORK(&shid->reset_work, spi_hid_reset_work);
@@ -1414,6 +1568,18 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
return error;
}
+ /*
+ * Use device_can_wakeup(), not device_may_wakeup(): power/wakeup can
+ * change after probe, and the PM core checks it at suspend time.
+ */
+ if (device_can_wakeup(dev)) {
+ error = devm_pm_set_wake_irq(dev, spi->irq);
+ if (error) {
+ dev_err(dev, "%s: failed to set wake IRQ\n", __func__);
+ return error;
+ }
+ }
+
error = shid->ops->power_up(shid->ops);
if (error) {
dev_err(dev, "%s: could not power up\n", __func__);
@@ -1454,6 +1620,25 @@ void spi_hid_core_remove(struct spi_device *spi)
}
EXPORT_SYMBOL_GPL(spi_hid_core_remove);
+static int spi_hid_core_pm_suspend(struct device *dev)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return spi_hid_suspend(shid);
+}
+
+static int spi_hid_core_pm_resume(struct device *dev)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return spi_hid_resume(shid);
+}
+
+const struct dev_pm_ops spi_hid_core_pm = {
+ SYSTEM_SLEEP_PM_OPS(spi_hid_core_pm_suspend, spi_hid_core_pm_resume)
+};
+EXPORT_SYMBOL_GPL(spi_hid_core_pm);
+
MODULE_DESCRIPTION("HID over SPI transport driver");
MODULE_AUTHOR("Dmitry Antipov <dmanti@microsoft.com>");
MODULE_LICENSE("GPL");
diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
index 1d8356f787f3..0b3248aedfce 100644
--- a/drivers/hid/spi-hid/spi-hid-core.h
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -92,6 +92,8 @@ struct spi_hid {
struct mutex io_lock;
struct completion output_done;
+ /* Completed once the device descriptor is received after a reset. */
+ struct completion reset_done;
u32 report_descriptor_crc32; /* HID report descriptor crc32 checksum. */
diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
index 24215ffaaafd..71b230c1000a 100644
--- a/drivers/hid/spi-hid/spi-hid-of.c
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -225,6 +225,7 @@ static struct spi_driver spi_hid_of_driver = {
.driver = {
.name = "spi_hid_of",
.owner = THIS_MODULE,
+ .pm = &spi_hid_core_pm,
.of_match_table = spi_hid_of_match,
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
.dev_groups = spi_hid_groups,
diff --git a/drivers/hid/spi-hid/spi-hid.h b/drivers/hid/spi-hid/spi-hid.h
index f5a5f4d54beb..17b2fdf192ed 100644
--- a/drivers/hid/spi-hid/spi-hid.h
+++ b/drivers/hid/spi-hid/spi-hid.h
@@ -41,5 +41,6 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
void spi_hid_core_remove(struct spi_device *spi);
extern const struct attribute_group *spi_hid_groups[];
+extern const struct dev_pm_ops spi_hid_core_pm;
#endif /* SPI_HID_H */
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 11/11] HID: spi-hid: add panel follower support
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
` (9 preceding siblings ...)
2026-10-19 19:01 ` [PATCH v6 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
@ 2026-10-19 19:01 ` Jingyuan Liang
10 siblings, 0 replies; 18+ messages in thread
From: Jingyuan Liang @ 2026-10-19 19:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Jingyuan Liang
Add support to spi-hid to be a panel follower.
Tested-by: Dale Whinham <daleyo@gmail.com>
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/Kconfig | 4 +
drivers/hid/spi-hid/spi-hid-core.c | 213 ++++++++++++++++++++++++++++---------
drivers/hid/spi-hid/spi-hid-core.h | 7 ++
3 files changed, 174 insertions(+), 50 deletions(-)
diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
index 63f202975ac3..656dbda1c5d8 100644
--- a/drivers/hid/spi-hid/Kconfig
+++ b/drivers/hid/spi-hid/Kconfig
@@ -13,6 +13,7 @@ if SPI_HID
config SPI_HID_ACPI
tristate "HID over SPI transport layer ACPI driver"
depends on ACPI
+ depends on DRM || !DRM
select RESET_CONTROLLER
select SPI_HID_CORE
help
@@ -29,6 +30,7 @@ config SPI_HID_ACPI
config SPI_HID_OF
tristate "HID over SPI transport layer Open Firmware driver"
depends on OF
+ depends on DRM || !DRM
select SPI_HID_CORE
help
Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
@@ -43,4 +45,6 @@ config SPI_HID_OF
config SPI_HID_CORE
tristate
+ # We need to call into panel code so if DRM=m, this can't be 'y'
+ depends on DRM || !DRM
endif
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index c2232d5959b8..8b2e21be5107 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -281,7 +281,9 @@ static int spi_hid_suspend(struct spi_hid *shid)
if (error) {
dev_err(dev, "%s failed to suspend hid driver: %d\n",
__func__, error);
- return error;
+ /* A follower powers off with the panel anyway. */
+ if (!shid->is_panel_follower)
+ return error;
}
}
}
@@ -311,6 +313,9 @@ static int spi_hid_suspend(struct spi_hid *shid)
dev_err(dev, "%s: could not power down\n", __func__);
shid->regulator_error_count++;
shid->regulator_last_error = error;
+ }
+ /* A follower powers off with the panel anyway. */
+ if (error && !shid->is_panel_follower) {
/*
* Undo partial suspend before returning error.
* Complete the reset; RESET_PENDING stays set
@@ -346,26 +351,28 @@ static int spi_hid_resume(struct spi_hid *shid)
return 0;
}
- if (!device_may_wakeup(dev)) {
- if (shid->power_state == HIDSPI_OFF) {
- shid->ops->assert_reset(shid->ops);
+ /*
+ * Check power_state, not device_may_wakeup(): power/wakeup can change
+ * while the device is off.
+ */
+ if (shid->power_state == HIDSPI_OFF) {
+ shid->ops->assert_reset(shid->ops);
- shid->ops->sleep_minimal_reset_delay(shid->ops);
+ shid->ops->sleep_minimal_reset_delay(shid->ops);
- error = shid->ops->power_up(shid->ops);
- if (error) {
- dev_err(dev, "%s: could not power up\n", __func__);
- shid->regulator_error_count++;
- shid->regulator_last_error = error;
- /* Suspend runs next even if resume fails. */
- enable_irq(shid->spi->irq);
- return error;
- }
- shid->power_state = HIDSPI_ON;
- reinit_completion(&shid->reset_done);
- shid->ops->deassert_reset(shid->ops);
- was_reset = true;
+ error = shid->ops->power_up(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: could not power up\n", __func__);
+ shid->regulator_error_count++;
+ shid->regulator_last_error = error;
+ /* Suspend runs next even if resume fails. */
+ enable_irq(shid->spi->irq);
+ return error;
}
+ shid->power_state = HIDSPI_ON;
+ reinit_completion(&shid->reset_done);
+ shid->ops->deassert_reset(shid->ops);
+ was_reset = true;
}
enable_irq(shid->spi->irq);
@@ -1504,6 +1511,110 @@ const struct attribute_group *spi_hid_groups[] = {
};
EXPORT_SYMBOL_GPL(spi_hid_groups);
+/*
+ * At the end of probe we initialize the device:
+ * 0) assert reset, bias the interrupt line
+ * 1) sleep minimal reset delay
+ * 2) power up the device
+ * 3) deassert reset (high)
+ * After this we expect an IRQ with a reset response.
+ */
+static int spi_hid_dev_init(struct spi_hid *shid)
+{
+ struct spi_device *spi = shid->spi;
+ struct device *dev = &spi->dev;
+ int error;
+
+ guard(mutex)(&shid->power_lock);
+
+ shid->ops->assert_reset(shid->ops);
+
+ shid->ops->sleep_minimal_reset_delay(shid->ops);
+
+ error = shid->ops->power_up(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: could not power up\n", __func__);
+ shid->regulator_error_count++;
+ shid->regulator_last_error = error;
+ return error;
+ }
+
+ shid->power_state = HIDSPI_ON;
+ error = shid->ops->deassert_reset(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: failed to deassert reset: %d\n", __func__, error);
+ shid->ops->power_down(shid->ops);
+ shid->power_state = HIDSPI_OFF;
+ return error;
+ }
+
+ enable_irq(spi->irq);
+
+ return 0;
+}
+
+static void spi_hid_panel_follower_work(struct work_struct *work)
+{
+ struct spi_hid *shid = container_of(work, struct spi_hid,
+ panel_follower_work);
+ int error;
+
+ /* Also does the first power up: probe leaves the device off. */
+ error = spi_hid_resume(shid);
+ if (error)
+ dev_warn(&shid->spi->dev, "Power on failed: %d\n", error);
+ /* Even a failed resume enables the IRQ; let suspend undo it. */
+ WRITE_ONCE(shid->panel_follower_work_finished, true);
+}
+
+static int spi_hid_panel_follower_resume(struct drm_panel_follower *follower)
+{
+ struct spi_hid *shid = container_of(follower, struct spi_hid, panel_follower);
+
+ /* Powering on can be slow; don't block the panel's power up. */
+ WRITE_ONCE(shid->panel_follower_work_finished, false);
+ schedule_work(&shid->panel_follower_work);
+
+ return 0;
+}
+
+static int spi_hid_panel_follower_suspend(struct drm_panel_follower *follower)
+{
+ struct spi_hid *shid = container_of(follower, struct spi_hid, panel_follower);
+
+ cancel_work_sync(&shid->panel_follower_work);
+
+ if (!READ_ONCE(shid->panel_follower_work_finished))
+ return 0;
+
+ return spi_hid_suspend(shid);
+}
+
+static const struct drm_panel_follower_funcs
+ spi_hid_panel_follower_prepare_funcs = {
+ .panel_prepared = spi_hid_panel_follower_resume,
+ .panel_unpreparing = spi_hid_panel_follower_suspend,
+};
+
+static int spi_hid_register_panel_follower(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+
+ shid->panel_follower.funcs = &spi_hid_panel_follower_prepare_funcs;
+
+ /*
+ * If we're not in control of our own power up/power down then we can't
+ * do the logic to manage wakeups. Give a warning if a user thought
+ * that was possible then force the capability off.
+ */
+ if (device_can_wakeup(dev)) {
+ dev_warn(dev, "Can't wakeup if following panel\n");
+ device_set_wakeup_capable(dev, false);
+ }
+
+ return drm_panel_add_follower(dev, &shid->panel_follower);
+}
+
int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
struct spi_hid_conf *conf)
{
@@ -1519,10 +1630,11 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
return -ENOMEM;
shid->spi = spi;
- shid->power_state = HIDSPI_ON;
+ shid->power_state = HIDSPI_OFF;
shid->ops = ops;
shid->conf = conf;
set_bit(SPI_HID_RESET_PENDING, &shid->flags);
+ shid->is_panel_follower = drm_is_panel_follower(&spi->dev);
spi_set_drvdata(spi, shid);
@@ -1537,6 +1649,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
init_completion(&shid->reset_done);
INIT_WORK(&shid->reset_work, spi_hid_reset_work);
+ INIT_WORK(&shid->panel_follower_work, spi_hid_panel_follower_work);
/*
* we need to allocate the buffer without knowing the maximum
@@ -1547,20 +1660,6 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
if (error)
return error;
- /*
- * At the end of probe we initialize the device:
- * 0) assert reset, bias the interrupt line
- * 1) sleep minimal reset delay
- * 2) request IRQ
- * 3) power up the device
- * 4) deassert reset (high)
- * After this we expect an IRQ with a reset response.
- */
-
- shid->ops->assert_reset(shid->ops);
-
- shid->ops->sleep_minimal_reset_delay(shid->ops);
-
error = devm_request_threaded_irq(dev, spi->irq, NULL, spi_hid_dev_irq,
IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(&spi->dev), shid);
if (error) {
@@ -1580,21 +1679,17 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
}
}
- error = shid->ops->power_up(shid->ops);
- if (error) {
- dev_err(dev, "%s: could not power up\n", __func__);
- return error;
- }
-
- error = shid->ops->deassert_reset(shid->ops);
- if (error) {
- dev_err(dev, "%s: failed to deassert reset: %d\n", __func__, error);
- shid->ops->power_down(shid->ops);
- return error;
+ if (shid->is_panel_follower) {
+ error = spi_hid_register_panel_follower(shid);
+ if (error)
+ return dev_err_probe(dev, error,
+ "Failed to register panel follower\n");
+ } else {
+ error = spi_hid_dev_init(shid);
+ if (error)
+ return error;
}
- enable_irq(spi->irq);
-
dev_dbg(dev, "%s: d3 -> %s\n", __func__,
spi_hid_power_mode_string(shid->power_state));
@@ -1608,15 +1703,27 @@ void spi_hid_core_remove(struct spi_device *spi)
struct device *dev = &spi->dev;
int error;
+ if (shid->is_panel_follower) {
+ drm_panel_remove_follower(&shid->panel_follower);
+ /* Already cancelled by panel_unpreparing; be explicit. */
+ cancel_work_sync(&shid->panel_follower_work);
+ }
+
+ /*
+ * Follower suspend above can be skipped. Disabling again is
+ * fine: nothing re-enables the IRQ before devres frees it.
+ */
disable_irq(spi->irq);
disable_work_sync(&shid->reset_work);
spi_hid_stop_hid(shid);
- shid->ops->assert_reset(shid->ops);
- error = shid->ops->power_down(shid->ops);
- if (error)
- dev_err(dev, "failed to disable regulator\n");
+ if (shid->power_state != HIDSPI_OFF) {
+ shid->ops->assert_reset(shid->ops);
+ error = shid->ops->power_down(shid->ops);
+ if (error)
+ dev_err(dev, "failed to disable regulator\n");
+ }
}
EXPORT_SYMBOL_GPL(spi_hid_core_remove);
@@ -1624,6 +1731,9 @@ static int spi_hid_core_pm_suspend(struct device *dev)
{
struct spi_hid *shid = dev_get_drvdata(dev);
+ if (shid->is_panel_follower)
+ return 0;
+
return spi_hid_suspend(shid);
}
@@ -1631,6 +1741,9 @@ static int spi_hid_core_pm_resume(struct device *dev)
{
struct spi_hid *shid = dev_get_drvdata(dev);
+ if (shid->is_panel_follower)
+ return 0;
+
return spi_hid_resume(shid);
}
diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
index 0b3248aedfce..631897dd9e8a 100644
--- a/drivers/hid/spi-hid/spi-hid-core.h
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -10,6 +10,8 @@
#include <linux/hid-over-spi.h>
#include <linux/spi/spi.h>
+#include <drm/drm_panel.h>
+
/* Protocol message size constants */
#define SPI_HID_READ_APPROVAL_LEN 5
#define SPI_HID_OUTPUT_HEADER_LEN 8
@@ -56,6 +58,10 @@ struct spi_hid {
struct spi_hid_input_buf *input; /* Input buffer. */
struct spi_hid_input_buf *response; /* Response buffer. */
+ struct drm_panel_follower panel_follower;
+ bool is_panel_follower;
+ bool panel_follower_work_finished;
+
u16 response_length;
u16 bufsize;
/* Response type awaited by a sync request, 0 if none. Protected by io_lock. */
@@ -70,6 +76,7 @@ struct spi_hid {
unsigned long flags; /* device flags. */
struct work_struct reset_work;
+ struct work_struct panel_follower_work;
/*
* Serializes request/response transactions: held from sending a
--
2.56.0.212.g129ede20d9-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus
2026-10-19 19:01 ` [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus Jingyuan Liang
@ 2026-10-19 19:11 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-10-19 19:11 UTC (permalink / raw)
To: Jingyuan Liang
Cc: linux-input, linux-trace-kernel, devicetree, robh, conor+dt,
dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] HID: spi-hid: multi-value sysfs attributes and missing ABI documentation
--
commit 4c66f3d16c395b79d0997b29357fb80b5a6f2708
Author: Angela Czubak <acz@semihalf.com>
HID: spi-hid: add transport driver skeleton for HID over SPI bus
This commit creates the spi-hid directory with its Kconfig and Makefile, and
adds the initial core driver skeleton including basic device structures,
sysfs attributes, and probe/remove functions.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> new file mode 100644
> index 0000000000000..02a7608c4b882
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -0,0 +1,213 @@
[ ... ]
> +static ssize_t bus_error_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u (%d)\n",
> + shid->bus_error_count, shid->bus_last_error);
> +}
> +static DEVICE_ATTR_RO(bus_error_count);
> +
> +static ssize_t regulator_error_count_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u (%d)\n",
> + shid->regulator_error_count,
> + shid->regulator_last_error);
> +}
> +static DEVICE_ATTR_RO(regulator_error_count);
> +
> +static ssize_t device_initiated_reset_count_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u\n", shid->dir_count);
> +}
> +static DEVICE_ATTR_RO(device_initiated_reset_count);
> +
> +static struct attribute *spi_hid_attrs[] = {
> + &dev_attr_bus_error_count.attr,
> + &dev_attr_regulator_error_count.attr,
> + &dev_attr_device_initiated_reset_count.attr,
> + NULL /* Terminator */
> +};
> +
> +static const struct attribute_group spi_hid_group = {
> + .attrs = spi_hid_attrs,
> +};
> +
> +const struct attribute_group *spi_hid_groups[] = {
> + &spi_hid_group,
> + NULL
> +};
> +EXPORT_SYMBOL_GPL(spi_hid_groups);
[Severity: Low]
In drivers/hid/spi-hid/spi-hid-core.c, bus_error_count_show() and
regulator_error_count_show() format both an unsigned error count and a signed
last error code in parentheses ("%u (%d)\n") into a single sysfs file when
userspace reads bus_error_count or regulator_error_count via spi_hid_groups
(which is attached to .dev_groups in the later ACPI and Device Tree support
commits in this series).
Per Documentation/filesystems/sysfs.rst, sysfs attributes should export one
value per file without custom formatting or mixing types, and new sysfs
attributes should be documented in Documentation/ABI/.
Should bus_last_error and regulator_last_error be split into separate sysfs
attributes (or moved to debugfs), and should Documentation/ABI/ entries be
added for the attributes in spi_hid_attrs?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-send-upstream-v6-0-cb9ba4b3d6fa@chromium.org?part=3
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation
2026-10-19 19:01 ` [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
@ 2026-10-19 19:22 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-10-19 19:22 UTC (permalink / raw)
To: Jingyuan Liang
Cc: linux-input, linux-trace-kernel, devicetree, robh, conor+dt,
dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] hid: spi-hid: spi_hid_bus_validate_header() rejects descriptors > max_input
- [High] hid: spi-hid: spi_hid_dev_desc_response() clears reset_attempts too early
- [Medium] hid: spi-hid: spi_hid_bus_validate_header() allows 0-length report body
- [Medium] hid: spi-hid: stale flags and duplicate SPI_HID_ERROR cause spurious resets
- [Medium] hid: spi-hid: unaborted sync request in __spi_hid_sync_request() on reset
--
commit 68d1db2b55c0e004ec2c06854ae93db414465298
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add HID SPI protocol implementation
This commit implements the HID Over SPI Protocol Specification 1.0 in
drivers/hid/spi-hid/spi-hid-core.c, handling SPI input/output transfers,
descriptor parsing, interrupt processing, and error recovery resets.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index fca7a44eeb9f9..90b2c2fea6501 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -187,17 +350,94 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
>
> static void spi_hid_stop_hid(struct spi_hid *shid)
> {
> - struct hid_device *hid = shid->hid;
> + struct hid_device *hid;
>
> - shid->hid = NULL;
> - clear_bit(SPI_HID_READY, &shid->flags);
> + scoped_guard(mutex, &shid->io_lock) {
> + hid = shid->hid;
> + shid->hid = NULL;
> + clear_bit(SPI_HID_READY, &shid->flags);
> + }
>
> if (hid)
> hid_destroy_device(hid);
> }
>
> +static void spi_hid_error_handler(struct spi_hid *shid)
> +{
> + struct device *dev = &shid->spi->dev;
> + int error;
> +
> + guard(mutex)(&shid->power_lock);
> + if (shid->power_state == HIDSPI_OFF)
> + return;
> +
> + disable_irq(shid->spi->irq);
> +
> + if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> + dev_err(dev, "unresponsive device, aborting\n");
> + spi_hid_stop_hid(shid);
> + shid->ops->assert_reset(shid->ops);
> + error = shid->ops->power_down(shid->ops);
> + if (error) {
> + dev_err(dev, "failed to disable regulator\n");
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + }
> + shid->power_state = HIDSPI_OFF;
> + /* Dead device: leave the IRQ permanently disabled. */
> + return;
> + }
> +
> + clear_bit(SPI_HID_READY, &shid->flags);
> + set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> + shid->ops->assert_reset(shid->ops);
> +
> + shid->power_state = HIDSPI_OFF;
> +
> + /*
> + * We want to cancel pending reset work as the device is being reset
> + * to recover from an error. cancel_work_sync will put us in a deadlock
> + * because this function is scheduled in 'reset_work' and we should
> + * avoid waiting for itself.
> + */
> + cancel_work(&shid->reset_work);
[Severity: Medium]
Should spi_hid_error_handler() also clear pending SPI_HID_RESET_RESPONSE,
SPI_HID_CREATE_DEVICE, and SPI_HID_ERROR bits in shid->flags here?
Furthermore, in __spi_hid_sync_request(), a timeout already sets
SPI_HID_ERROR and schedules shid->reset_work, but its callers
(spi_hid_reset_response(), spi_hid_report_descriptor_request(), and
spi_hid_get_request()) unconditionally set SPI_HID_ERROR and schedule
shid->reset_work a second time on error:
spi_hid_get_request()
__spi_hid_sync_request()
set_bit(SPI_HID_ERROR, &shid->flags);
schedule_work(&shid->reset_work);
return -ETIMEDOUT;
set_bit(SPI_HID_ERROR, &shid->flags);
schedule_work(&shid->reset_work);
If shid->reset_work starts on another CPU after the first schedule_work()
and clears SPI_HID_ERROR via test_and_clear_bit(), the caller can set
SPI_HID_ERROR and queue shid->reset_work again. If cancel_work() cancels
the work item without clearing the flag bits, the stale SPI_HID_ERROR bit
remains set in shid->flags and will trigger a second reset when
spi_hid_reset_work() later checks pending flags to reschedule itself. And
if the second schedule_work() runs after cancel_work(), reset_work will run
a second time immediately.
Also, if spi_hid_get_request() is called while SPI_HID_READY is cleared,
__spi_hid_send_output_report() returns -ENODEV, which spi_hid_get_request()
treats as a fatal error by setting SPI_HID_ERROR and scheduling a hardware
reset.
Finally, in spi_hid_reset_work(), SPI_HID_RESET_RESPONSE and
SPI_HID_CREATE_DEVICE are checked before SPI_HID_ERROR in the if/else-if
chain, so if SPI_HID_ERROR is set while one of those bits is pending, the
worker will attempt normal initialization before running
spi_hid_error_handler().
> +
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> + shid->power_state = HIDSPI_ON;
> +
> + shid->ops->deassert_reset(shid->ops);
> +
> + enable_irq(shid->spi->irq);
> +}
[ ... ]
> @@ -234,39 +491,208 @@ static int spi_hid_send_output_report(struct spi_hid *shid,
> struct spi_hid_output_report *report)
> {
> guard(mutex)(&shid->output_lock);
> - return __spi_hid_send_output_report(shid, report);
> + return __spi_hid_send_output_report(shid, report, 0);
> }
>
> static int __spi_hid_sync_request(struct spi_hid *shid,
> struct spi_hid_output_report *report)
> {
> struct device *dev = &shid->spi->dev;
> int error;
>
> - reinit_completion(&shid->output_done);
> -
> - error = __spi_hid_send_output_report(shid, report);
> + error = __spi_hid_send_output_report(shid, report,
> + spi_hid_response_type(report->report_type));
> if (error)
> return error;
>
> - error = wait_for_completion_interruptible_timeout(&shid->output_done,
> - msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
> - if (error == 0) {
> - dev_err(dev, "Response timed out\n");
> - return -ETIMEDOUT;
> + if (wait_for_completion_timeout(&shid->output_done,
> + msecs_to_jiffies(SPI_HID_RESP_TIMEOUT)))
> + return 0;
> +
> + /* Drop late responses and block new HID requests until the reset. */
> + scoped_guard(mutex, &shid->io_lock) {
> + shid->expected_response = 0;
> + clear_bit(SPI_HID_READY, &shid->flags);
> }
> - if (error < 0)
> - return error;
>
> - return 0;
> + dev_err(dev, "Response timed out\n");
> + /* The device may still answer later: resync it with a reset. */
> + set_bit(SPI_HID_ERROR, &shid->flags);
> + schedule_work(&shid->reset_work);
> + return -ETIMEDOUT;
> }
[Severity: Medium]
What happens if a spontaneous firmware reset (RESET_RESPONSE in
spi_hid_process_input_report()) or an IRQ error (spi_hid_error_handler())
occurs while a synchronous request is waiting on shid->output_done in
__spi_hid_sync_request() with shid->output_lock held?
Because neither the RESET_RESPONSE path nor spi_hid_error_handler() aborts
the in-flight request or wakes shid->output_done, the waiter continues
holding shid->output_lock for the full SPI_HID_RESP_TIMEOUT (blocking
spi_hid_reset_response() in reset_work) and then sets SPI_HID_ERROR and
schedules reset_work when the timeout expires, resetting the device a
second time after recovery.
[ ... ]
> +static int spi_hid_dev_desc_response(struct spi_hid *shid,
> + struct spi_hid_input_report *body)
> +{
> + struct hidspi_dev_descriptor *raw =
> + (struct hidspi_dev_descriptor *)shid->input->content;
> + struct device *dev = &shid->spi->dev;
> +
> + /* Validate device descriptor length before parsing */
> + if (body->content_length != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
> + dev_err(dev, "Invalid content length %d, expected %zu\n",
> + body->content_length, HIDSPI_DEVICE_DESCRIPTOR_SIZE);
> + return -EPROTO;
> + }
> +
> + if (le16_to_cpu(raw->dev_desc_len) != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
> + dev_err(dev, "Invalid wDeviceDescLength %d, expected %zu\n",
> + le16_to_cpu(raw->dev_desc_len),
> + HIDSPI_DEVICE_DESCRIPTOR_SIZE);
> + return -EPROTO;
> + }
> +
> + if (le16_to_cpu(raw->bcd_ver) != SPI_HID_SUPPORTED_VERSION) {
> + dev_err(dev, "Unsupported device descriptor version %4x\n",
> + le16_to_cpu(raw->bcd_ver));
> + return -EPROTONOSUPPORT;
> + }
> +
> + /* Fail before reset_attempts is cleared so resets stay bounded. */
> + if (!le16_to_cpu(raw->rep_desc_len) ||
> + le16_to_cpu(raw->rep_desc_len) > HID_MAX_DESCRIPTOR_SIZE) {
> + dev_err(dev, "Invalid wReportDescLength %d, max %d\n",
> + le16_to_cpu(raw->rep_desc_len),
> + HID_MAX_DESCRIPTOR_SIZE);
> + return -EPROTO;
> + }
> +
> + spi_hid_parse_dev_desc(raw, &shid->desc);
> +
> + /* Reset attempts at every device descriptor fetch */
> + shid->reset_attempts = 0;
> + set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> + schedule_work(&shid->reset_work);
> +
> + return 0;
> +}
[Severity: High]
Does clearing shid->reset_attempts = 0 here in spi_hid_dev_desc_response()
cause an unbounded reset loop if fetching or validating the report
descriptor subsequently fails?
When SPI_HID_CREATE_DEVICE runs spi_hid_create_device() -> hid_add_device()
-> spi_hid_ll_parse() (or spi_hid_refresh_device()), it calls
spi_hid_report_descriptor_request(). If that request times out or fails
validation in spi_hid_dev_irq(), SPI_HID_ERROR is set and
spi_hid_error_handler() increments shid->reset_attempts from 0 to 1 and
resets the device.
After the reset, if the device responds to DEVICE_DESCRIPTOR again,
spi_hid_dev_desc_response() resets shid->reset_attempts back to 0 before
REPORT_DESCRIPTOR is retried, so shid->reset_attempts oscillates between 0
and 1 and never reaches SPI_HID_MAX_RESET_ATTEMPTS. Should
shid->reset_attempts only be cleared once the report descriptor is fetched
and SPI_HID_READY is set?
[ ... ]
> @@ -334,13 +766,205 @@ static int spi_hid_create_device(struct spi_hid *shid)
[ ... ]
> +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> + struct spi_hid_input_header *header)
> +{
> + struct device *dev = &shid->spi->dev;
> + /*
> + * Body capacity of shid->input, matching spi_hid_alloc_buffers().
> + * shid->bufsize only changes with the IRQ disabled, so it is stable here.
> + */
> + u32 max_body = round_up(sizeof(shid->input->header) +
> + sizeof(shid->input->body) + shid->bufsize, 4) -
> + sizeof(shid->input->header);
> +
> + if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> + dev_err(dev, "Unknown input report version (v 0x%x)\n",
> + header->version);
> + return -EINVAL;
> + }
> +
> + /*
> + * report_length is device-provided and is used as the size of the body
> + * transfer, so it must never exceed the input buffer.
> + */
> + if (header->report_length > max_body) {
> + dev_err(dev, "Input report too big: %u > %u\n",
> + header->report_length, max_body);
> + return -EMSGSIZE;
> + }
[Severity: Medium]
Can header->report_length be 0 (or smaller than sizeof(shid->input->body))
here?
In spi_hid_populate_input_header(), header->report_length is computed as
(get_unaligned_le16(&buf[1]) & 0x3fff) * 4. If a malformed header or bus
glitch during reset produces a length of 0, spi_hid_bus_validate_header()
returns 0 because it only checks the upper bound.
In spi_hid_dev_irq(), that bypasses the SPI_HID_RESET_PENDING check,
performs a 0-byte body read via spi_hid_input_sync(), and then parses stale
bytes from shid->input->body in spi_hid_process_input_report(), triggering
spi_hid_irq_error(). Should spi_hid_bus_validate_header() reject
header->report_length < sizeof(shid->input->body)?
> +
> + if (shid->desc.max_input_length != 0 &&
> + header->report_length > shid->desc.max_input_length) {
> + dev_err(dev, "Input report body size %u > max expected of %u\n",
> + header->report_length, shid->desc.max_input_length);
> + return -EMSGSIZE;
> + }
[Severity: High]
Won't this check in spi_hid_bus_validate_header() reject
REPORT_DESCRIPTOR_RESPONSE (and post-reset DEVICE_DESCRIPTOR_RESPONSE)
whenever the descriptor packet is larger than shid->desc.max_input_length?
During initialization:
spi_hid_dev_desc_response()
spi_hid_parse_dev_desc(raw, &shid->desc)
desc->max_input_length = le16_to_cpu(raw->max_input_len);
set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
schedule_work(&shid->reset_work);
When spi_hid_reset_work() then calls spi_hid_create_device() ->
hid_add_device() -> spi_hid_ll_parse() ->
spi_hid_report_descriptor_request(), shid->desc.max_input_length is already
populated with wMaxInputLength.
When the device replies with REPORT_DESCRIPTOR_RESPONSE, spi_hid_dev_irq()
calls spi_hid_bus_validate_header() before reading the body or knowing the
report type. Since wMaxInputLength is the maximum length of input reports
while wReportDescLength (up to HID_MAX_DESCRIPTOR_SIZE = 4096) is typically
much larger, header->report_length > shid->desc.max_input_length will fail
with -EMSGSIZE and trigger spi_hid_irq_error() on every attempt until the
device is aborted. (Note also the comment in spi_hid_find_max_report() that
some devices set wMaxInputLength to an inaccurate length.)
> +
> + if (header->last_fragment_flag != 1) {
> + dev_err(dev, "Multi-fragment reports not supported\n");
> + return -EOPNOTSUPP;
> + }
[ ... ]
> @@ -365,6 +989,8 @@ static int spi_hid_get_request(struct spi_hid *shid, u8 content_id,
> dev_err(dev,
> "Expected get request response not received! Error %d\n",
> error);
> + set_bit(SPI_HID_ERROR, &shid->flags);
> + schedule_work(&shid->reset_work);
> return error;
> }
[ ... ]
> @@ -397,9 +1023,81 @@ static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
> return spi_hid_sync_request(shid, &report);
> }
>
> -/* This is a placeholder. Will be implemented in the next patch. */
> +/* Schedule a reset to recover from an error in the IRQ handler. */
> +static irqreturn_t spi_hid_irq_error(struct spi_hid *shid)
> +{
> + set_bit(SPI_HID_ERROR, &shid->flags);
> + schedule_work(&shid->reset_work);
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> {
> + struct spi_hid *shid = _shid;
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_input_header header;
> + int error = 0;
> +
> + scoped_guard(mutex, &shid->io_lock) {
> + if (shid->power_state == HIDSPI_OFF) {
> + dev_warn(dev, "Device is off, ignoring interrupt\n");
> + return IRQ_NONE;
> + }
> +
> + error = spi_hid_input_sync(shid, shid->input->header,
> + sizeof(shid->input->header), true);
> + if (error) {
> + dev_err(dev, "Failed to transfer header: %d\n", error);
> + return spi_hid_irq_error(shid);
> + }
> +
> + if (shid->input_message.status < 0) {
> + dev_warn(dev, "Error reading header: %d\n",
> + shid->input_message.status);
> + shid->bus_error_count++;
> + shid->bus_last_error = shid->input_message.status;
> + return spi_hid_irq_error(shid);
> + }
> +
> + spi_hid_populate_input_header(shid->input->header, &header);
> +
> + error = spi_hid_bus_validate_header(shid, &header);
> + if (error) {
> + if (!test_bit(SPI_HID_RESET_PENDING, &shid->flags)) {
> + dev_err(dev, "Failed to validate header: %d\n", error);
> + print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
> + DUMP_PREFIX_NONE, 16, 1, shid->input->header,
> + sizeof(shid->input->header), false);
> + shid->bus_error_count++;
> + shid->bus_last_error = error;
> + return spi_hid_irq_error(shid);
> + }
> + return IRQ_HANDLED;
> + }
> +
> + error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
> + false);
[ ... ]
> @@ -479,6 +1177,8 @@ static int spi_hid_ll_start(struct hid_device *hid)
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-send-upstream-v6-0-cb9ba4b3d6fa@chromium.org?part=5
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer
2026-10-19 19:01 ` [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer Jingyuan Liang
@ 2026-10-19 19:26 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-10-19 19:26 UTC (permalink / raw)
To: Jingyuan Liang
Cc: linux-input, linux-trace-kernel, devicetree, robh, conor+dt,
dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: spi-hid: zero-length report descriptor accepted in spi_hid_ll_parse()
--
commit 9328ec8c48a0b55cf1019d845ec2c37e89f4cdbb
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add spi-hid driver HID layer
This commit adds the HID low-level driver callbacks and device creation logic
for spi-hid, handling report descriptor fetching, buffer allocation, and
output/feature report requests.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 02a7608c4b882..fca7a44eeb9f9 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -84,11 +185,484 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
[ ... ]
> +/*
> + * This function returns the length of the report descriptor, or a negative
> + * error code if something went wrong.
> + */
> +static int spi_hid_report_descriptor_request(struct spi_hid *shid)
> +{
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_output_report report = {
> + .report_type = REPORT_DESCRIPTOR,
> + .content_length = 0,
> + .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
> + .content = NULL,
> + };
> + int ret;
> +
> + ret = spi_hid_sync_request(shid, &report);
> + if (ret) {
> + dev_err(dev,
> + "Expected report descriptor not received: %d\n", ret);
> + return ret;
> + }
> +
> + ret = shid->response_length;
> + if (ret != shid->desc.report_descriptor_length) {
> + ret = min_t(unsigned int, ret, shid->desc.report_descriptor_length);
> + dev_err(dev, "Received report descriptor length doesn't match device descriptor field, using min of the two: %d\n",
> + ret);
> + }
> +
> + return ret;
> +}
[ ... ]
> +static int spi_hid_ll_parse(struct hid_device *hid)
> +{
> + struct spi_device *spi = hid->driver_data;
> + struct spi_hid *shid = spi_get_drvdata(spi);
> + struct device *dev = &spi->dev;
> + unsigned int rsize = shid->desc.report_descriptor_length;
> + int error, len;
> +
> + if (rsize > HID_MAX_DESCRIPTOR_SIZE) {
> + dev_err(dev,
> + "Report descriptor size %d is greater than HID_MAX_DESCRIPTOR_SIZE %d\n",
> + rsize, HID_MAX_DESCRIPTOR_SIZE);
> + return -EINVAL;
> + }
> +
> + if (rsize > shid->bufsize) {
> + error = spi_hid_alloc_buffers(shid, rsize);
> + if (error)
> + return error;
> + }
> +
> + len = spi_hid_report_descriptor_request(shid);
> + if (len < 0) {
> + dev_err(dev, "Report descriptor request failed, %d\n", len);
> + return len;
> + }
> +
> + /*
> + * FIXME: below call returning 0 doesn't mean that the report descriptor
> + * is good. We might be caching a crc32 of a corrupted r. d. or who
> + * knows what the FW sent. Need to have a feedback loop about r. d.
> + * being ok and only then cache it.
> + */
> + error = hid_parse_report(hid, (u8 *)shid->response->content, len);
> + if (error) {
> + dev_err(dev, "failed parsing report: %d\n", error);
> + return error;
> + }
> + shid->report_descriptor_crc32 = crc32_le(0,
> + (unsigned char const *)shid->response->content,
> + len);
> +
> + set_bit(SPI_HID_READY, &shid->flags);
> +
> + return 0;
> +}
[Severity: Medium]
What happens in spi_hid_ll_parse() if the report descriptor length is 0?
In spi_hid_ll_parse(), rsize (shid->desc.report_descriptor_length) is checked
against HID_MAX_DESCRIPTOR_SIZE, but not for zero. Later in the series, the
commit "HID: spi-hid: add HID SPI protocol implementation" validates
raw->rep_desc_len != 0 in spi_hid_dev_desc_response(), but if a device returns
a REPORT_DESCRIPTOR_RESPONSE with content_length == 0 (shid->response_length ==
0), spi_hid_report_descriptor_request() computes min_t(unsigned int, 0,
shid->desc.report_descriptor_length) and returns 0.
Both spi_hid_ll_parse() here and spi_hid_refresh_device() in the protocol patch
only check if (len < 0), treating len == 0 as success.
When len == 0 is passed to hid_parse_report():
drivers/hid/hid-core.c:hid_parse_report() {
hid->dev_rdesc = kmemdup(start, size, GFP_KERNEL);
if (!hid->dev_rdesc)
return -ENOMEM;
hid->dev_rsize = size;
return 0;
}
kmemdup() returns ZERO_SIZE_PTR (non-NULL), so hid_parse_report() returns 0.
spi_hid_ll_parse() then caches a CRC32 of 0 and sets SPI_HID_READY, and
hid_add_device() sees a non-NULL hdev->dev_rdesc and calls device_add(),
registering an unusable hid_device while bypassing SPI_HID_ERROR reset
recovery.
Should spi_hid_report_descriptor_request() or its callers reject a 0-length
report descriptor (for example, checking !rsize in spi_hid_ll_parse() and
returning -EINVAL when shid->response_length == 0 or len <= 0)?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-send-upstream-v6-0-cb9ba4b3d6fa@chromium.org?part=4
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 10/11] HID: spi-hid: add power management implementation
2026-10-19 19:01 ` [PATCH v6 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
@ 2026-10-19 19:36 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-10-19 19:36 UTC (permalink / raw)
To: Jingyuan Liang
Cc: linux-input, linux-trace-kernel, devicetree, robh, conor+dt,
dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] hid: spi-hid: disable_irq() before cancel_work_sync() in spi_hid_suspend()
- [High] hid: spi-hid: unchecked deassert_reset() and reset timeout in resume/suspend
- [High] hid: spi-hid: holding power_lock in spi_hid_wait_for_reset() blocks recovery
- [Medium] hid: spi-hid: spi_hid_resume() runs reset_resume before pending reset_work
- [Medium] HID: spi-hid: skipped power-up in spi_hid_resume() when wakeup is enabled
--
commit 943f2d7ec87adee993c65a27b7655a39b4d0ef5c
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add power management implementation
This commit implements system sleep power management callbacks for the
HID over SPI transport driver, handling device suspend, resume, reset
synchronization, and wake IRQ setup.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 96805c10bfe36..c2232d5959b88 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -242,6 +250,148 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
> }
> }
>
> +/*
> + * Wait for the device descriptor response after reset; the device then takes
> + * requests. Set READY here: the report descriptor check that normally sets it
> + * needs power_lock, which callers hold.
> + */
> +static void spi_hid_wait_for_reset(struct spi_hid *shid)
> +{
> + unsigned long timeout = msecs_to_jiffies(SPI_HID_RESET_TIMEOUT +
> + SPI_HID_RESP_TIMEOUT);
> +
> + if (!wait_for_completion_timeout(&shid->reset_done, timeout)) {
> + dev_warn(&shid->spi->dev,
> + "Device descriptor not received after reset\n");
> + return;
> + }
> +
> + set_bit(SPI_HID_READY, &shid->flags);
> +}
[Severity: High]
What happens if an error occurs during the post-reset handshake while a
caller is waiting in spi_hid_wait_for_reset()?
Both spi_hid_resume() and the power_down() rollback path in
spi_hid_suspend() hold shid->power_lock across spi_hid_wait_for_reset()
and hid_driver_reset_resume().
If a transient bus error, header validation error, or descriptor error
occurs in spi_hid_dev_irq(), spi_hid_reset_response(), or
spi_hid_dev_desc_response(), the driver sets SPI_HID_ERROR and schedules
shid->reset_work to run spi_hid_error_handler():
spi_hid_reset_work()
spi_hid_error_handler()
guard(mutex)(&shid->power_lock);
Because spi_hid_wait_for_reset() is sleeping with shid->power_lock held,
spi_hid_error_handler() blocks on shid->power_lock and cannot perform any
of its SPI_HID_MAX_RESET_ATTEMPTS recovery resets until
spi_hid_wait_for_reset() times out after 2000 ms and spi_hid_resume() or
spi_hid_suspend() returns. That leaves SPI_HID_READY cleared when
hid_driver_reset_resume() runs, and delays the error reset until after
resume finishes.
> +
> +static int spi_hid_suspend(struct spi_hid *shid)
> +{
> + int error;
> + struct device *dev = &shid->spi->dev;
> +
> + /* HID driver suspend may talk to the device; responses need the IRQ. */
> + scoped_guard(mutex, &shid->power_lock) {
> + if (shid->power_state != HIDSPI_OFF && shid->hid) {
> + error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
> + if (error) {
> + dev_err(dev, "%s failed to suspend hid driver: %d\n",
> + __func__, error);
> + return error;
> + }
> + }
> + }
> +
> + /*
> + * disable_irq() waits for a running IRQ thread, so no new reset_work
> + * can be queued after it returns. Then wait for any queued or running
> + * instance. Must not hold power_lock here: reset_work takes it.
> + */
> + disable_irq(shid->spi->irq);
> + cancel_work_sync(&shid->reset_work);
[Severity: High]
Can calling disable_irq(shid->spi->irq) before
cancel_work_sync(&shid->reset_work) starve an in-flight synchronous
request in spi_hid_reset_work()?
If shid->reset_work is already running when spi_hid_suspend() disables the
IRQ, it may be executing a synchronous request in:
spi_hid_reset_work()
spi_hid_reset_response() / spi_hid_create_device() / spi_hid_refresh_device()
spi_hid_sync_request()
__spi_hid_sync_request()
wait_for_completion_timeout(&shid->output_done, ...)
Since shid->output_done is completed by spi_hid_response_handler() from
the threaded IRQ handler spi_hid_dev_irq(), disabling the IRQ first
prevents the response interrupt from being serviced while
cancel_work_sync() waits for reset_work.
This stalls cancel_work_sync() for the full SPI_HID_RESP_TIMEOUT, fails
the request with -ETIMEDOUT, clears SPI_HID_READY, sets SPI_HID_ERROR,
and triggers dev_WARN_ONCE() in spi_hid_reset_response():
error = spi_hid_sync_request(shid, &report);
if (error) {
dev_WARN_ONCE(dev, true,
"Failed to send device descriptor request: %d\n", error);
set_bit(SPI_HID_ERROR, &shid->flags);
schedule_work(&shid->reset_work);
}
For wakeup-enabled devices, this also leaves the device suspended with
SPI_HID_READY cleared (causing spi_hid_input_report_handler() to drop
wakeup input reports) and SPI_HID_ERROR pending. In addition, because
shid->power_lock is released after hid_driver_suspend() and before
disable_irq(), spi_hid_create_device() or spi_hid_refresh_device() in
spi_hid_reset_work() can register a new shid->hid in that window that
never has hid_driver_suspend() called on it.
> +
> + guard(mutex)(&shid->power_lock);
> + if (!device_may_wakeup(dev)) {
> + if (shid->power_state != HIDSPI_OFF) {
> + /* Device is fully reset on resume; drop stale state. */
> + clear_bit(SPI_HID_READY, &shid->flags);
> + clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
> + clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> + clear_bit(SPI_HID_ERROR, &shid->flags);
> + set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> + shid->ops->assert_reset(shid->ops);
> +
> + error = shid->ops->power_down(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power down\n", __func__);
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + /*
> + * Undo partial suspend before returning error.
> + * Complete the reset; RESET_PENDING stays set
> + * until the device sends its reset response.
> + */
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
> + reinit_completion(&shid->reset_done);
> + shid->ops->deassert_reset(shid->ops);
> + enable_irq(shid->spi->irq);
> + if (shid->hid) {
> + spi_hid_wait_for_reset(shid);
> + hid_driver_reset_resume(shid->hid);
> + }
> + return error;
> + }
> +
> + shid->power_state = HIDSPI_OFF;
> + }
> + }
> + return 0;
> +}
> +
> +static int spi_hid_resume(struct spi_hid *shid)
> +{
> + int error;
> + struct device *dev = &shid->spi->dev;
> + bool was_reset = false;
> +
> + guard(mutex)(&shid->power_lock);
> + if (test_bit(SPI_HID_DEAD, &shid->flags)) {
> + /* Stay off; only balance disable_irq() in suspend. */
> + enable_irq(shid->spi->irq);
> + return 0;
> + }
> +
> + if (!device_may_wakeup(dev)) {
> + if (shid->power_state == HIDSPI_OFF) {
[Severity: Medium]
Can gating the power-up path on !device_may_wakeup(dev) and
shid->power_state == HIDSPI_OFF leave the device unpowered across resume
or hibernation restore?
In this commit, if shid->power_state is HIDSPI_OFF while
device_may_wakeup(dev) is true (for example, if power/wakeup is enabled
while suspended or after a failed power_up()), spi_hid_resume() skips
powering up and resetting the device. A later commit in the series ("HID:
spi-hid: add panel follower support") removes the outer
!device_may_wakeup(dev) check in spi_hid_resume(), but hibernation still
has a related problem when wakeup is enabled.
Because spi_hid_core_pm uses SYSTEM_SLEEP_PM_OPS(spi_hid_core_pm_suspend,
spi_hid_core_pm_resume), .freeze, .thaw, .poweroff, and .restore all call
spi_hid_suspend() and spi_hid_resume(). When device_may_wakeup(dev) is
true during hibernation:
1. During .freeze, spi_hid_suspend() sees device_may_wakeup(dev) and skips
clearing SPI_HID_READY, asserting reset, calling ops->power_down(), and
setting shid->power_state = HIDSPI_OFF. The hibernation image therefore
saves shid->power_state as HIDSPI_ON (and conf->supply_enabled as true
in spi-hid-of).
2. During .poweroff, spi_hid_suspend() again skips powering down the
device, and platform power is then removed by S4 or by the boot
kernel's .freeze before image restoration.
3. During .restore, spi_hid_resume() sees shid->power_state == HIDSPI_ON
from the restored image, skips assert_reset(), power_up(),
deassert_reset(), and spi_hid_wait_for_reset(), and calls
hid_driver_reset_resume(shid->hid) while the hardware is unpowered or
held in reset.
> + shid->ops->assert_reset(shid->ops);
> +
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> + error = shid->ops->power_up(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power up\n", __func__);
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + /* Suspend runs next even if resume fails. */
> + enable_irq(shid->spi->irq);
> + return error;
> + }
> + shid->power_state = HIDSPI_ON;
> + reinit_completion(&shid->reset_done);
> + shid->ops->deassert_reset(shid->ops);
> + was_reset = true;
> + }
> + }
> +
> + enable_irq(shid->spi->irq);
> +
> + /*
> + * For devices that may wake up, process work that was pending when
> + * suspend cancelled reset_work. Otherwise suspend already dropped it.
> + */
> + if (test_bit(SPI_HID_RESET_RESPONSE, &shid->flags) ||
> + test_bit(SPI_HID_CREATE_DEVICE, &shid->flags) ||
> + test_bit(SPI_HID_ERROR, &shid->flags))
> + schedule_work(&shid->reset_work);
> +
> + if (shid->hid) {
> + if (was_reset)
> + spi_hid_wait_for_reset(shid);
> + error = hid_driver_reset_resume(shid->hid);
[Severity: High]
Should spi_hid_resume() and the power_down() error rollback in
spi_hid_suspend() check the return value of shid->ops->deassert_reset()
and handle timeouts from spi_hid_wait_for_reset()?
Unlike spi_hid_core_probe() and spi_hid_error_handler(), both sites ignore
the return value of shid->ops->deassert_reset(shid->ops) (which evaluates
the ACPI _RST method in spi_hid_acpi_deassert_reset() or drives the reset
GPIO in spi_hid_of_deassert_reset()).
If deassert_reset() fails, or if the device fails to respond before
spi_hid_wait_for_reset() times out, spi_hid_wait_for_reset() only logs a
warning and returns without setting SPI_HID_READY, setting SPI_HID_ERROR,
or scheduling shid->reset_work (and if shid->hid is NULL,
spi_hid_wait_for_reset() is not called at all).
Both functions then unconditionally call hid_driver_reset_resume(shid->hid)
while SPI_HID_READY is still 0, causing any synchronous HID requests
during reset_resume to fail with -ENODEV in __spi_hid_send_output_report(),
and leaving the device stuck in SPI_HID_RESET_PENDING with no recovery
scheduled.
[Severity: Medium]
Also in spi_hid_resume(), what happens when a wakeup-enabled device
(was_reset == false) resumes while reset or recovery work is pending?
When SPI_HID_RESET_RESPONSE, SPI_HID_CREATE_DEVICE, or SPI_HID_ERROR was
pending when spi_hid_suspend() cancelled shid->reset_work, SPI_HID_READY
is 0.
In spi_hid_resume(), schedule_work(&shid->reset_work) queues the worker
asynchronously while shid->power_lock is still held, spi_hid_wait_for_reset()
is skipped because was_reset is false, and hid_driver_reset_resume(shid->hid)
is called immediately before shid->reset_work can complete the reset and
set SPI_HID_READY.
As a result, __spi_hid_send_output_report() rejects any feature or output
reports sent by hid_driver_reset_resume() with -ENODEV:
/* While not READY, only (re)init descriptor requests may be sent. */
if (report->report_type != DEVICE_DESCRIPTOR &&
report->report_type != REPORT_DESCRIPTOR &&
!test_bit(SPI_HID_READY, &shid->flags))
return -ENODEV;
> + if (error) {
> + dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
> + __func__, error);
> + return error;
> + }
> + }
> + return 0;
> +}
[ ... ]
> @@ -1454,6 +1620,25 @@ void spi_hid_core_remove(struct spi_device *spi)
> }
> EXPORT_SYMBOL_GPL(spi_hid_core_remove);
>
> +static int spi_hid_core_pm_suspend(struct device *dev)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return spi_hid_suspend(shid);
> +}
> +
> +static int spi_hid_core_pm_resume(struct device *dev)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return spi_hid_resume(shid);
> +}
> +
> +const struct dev_pm_ops spi_hid_core_pm = {
> + SYSTEM_SLEEP_PM_OPS(spi_hid_core_pm_suspend, spi_hid_core_pm_resume)
> +};
> +EXPORT_SYMBOL_GPL(spi_hid_core_pm);
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-send-upstream-v6-0-cb9ba4b3d6fa@chromium.org?part=10
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema
2026-10-19 19:01 ` [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
@ 2026-10-20 16:34 ` Rob Herring (Arm)
2026-10-20 17:57 ` Conor Dooley
1 sibling, 0 replies; 18+ messages in thread
From: Rob Herring (Arm) @ 2026-10-20 16:34 UTC (permalink / raw)
To: Jingyuan Liang
Cc: Jiri Kosina, daleyo, hbarnor, Steven Rostedt, fqwqf,
Masami Hiramatsu, Jonathan Corbet, linux-spi, linux-input,
Mark Brown, Dmitry Antipov, Jarrett Schultz, linux-kernel,
Dmitry Torokhov, tfiga, Mathieu Desnoyers, devicetree,
Benjamin Tissoires, Conor Dooley, Krzysztof Kozlowski,
linux-trace-kernel, linux-doc
On Mon, 19 Oct 2026 19:01:20 +0000, Jingyuan Liang wrote:
> Documentation describes the required and optional properties for
> implementing Device Tree for a Microsoft G6 Touch Digitizer that
> supports HID over SPI Protocol 1.0 specification.
>
> The properties are common to HID over SPI.
>
> Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
> ---
> .../devicetree/bindings/input/hid-over-spi.yaml | 130 +++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema
2026-10-19 19:01 ` [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
2026-10-20 16:34 ` Rob Herring (Arm)
@ 2026-10-20 17:57 ` Conor Dooley
1 sibling, 0 replies; 18+ messages in thread
From: Conor Dooley @ 2026-10-20 17:57 UTC (permalink / raw)
To: Jingyuan Liang
Cc: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-input, linux-doc, linux-kernel, linux-spi,
linux-trace-kernel, devicetree, hbarnor, tfiga, fqwqf, daleyo,
Dmitry Antipov, Jarrett Schultz
[-- Attachment #1: Type: text/plain, Size: 6063 bytes --]
On Mon, Oct 19, 2026 at 07:01:20PM +0000, Jingyuan Liang wrote:
> Documentation describes the required and optional properties for
> implementing Device Tree for a Microsoft G6 Touch Digitizer that
> supports HID over SPI Protocol 1.0 specification.
>
> The properties are common to HID over SPI.
>
> Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
Signoff chain is not right, since the nominal patch author is the last
signoff.
> ---
> .../devicetree/bindings/input/hid-over-spi.yaml | 130 +++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> new file mode 100644
> index 000000000000..2a5fa0868a09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> @@ -0,0 +1,130 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/hid-over-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HID over SPI Devices
> +
> +maintainers:
> + - Benjamin Tissoires <benjamin.tissoires@redhat.com>
> + - Jiri Kosina <jkosina@suse.cz>
> + - Jingyuan Liang <jingyliang@chromium.org>
> +
> +description: |+
> + HID over SPI provides support for various Human Interface Devices over the
> + SPI bus. These devices can be for example touchpads, keyboards, touch screens
> + or sensors.
> +
> + The specification has been written by Microsoft and is currently available
> + here: https://www.microsoft.com/en-us/download/details.aspx?id=103325
> +
> + The HID over SPI specification does not define fixed values for the
> + read/write opcodes and the input/output report addresses. The host
> + retrieves them from platform firmware (ACPI _DSM), and they differ
> + between devices. These properties provide the same values on Device
> + Tree systems, so devices that fully comply with the specification work
> + with the "hid-over-spi" fallback compatible.
> +
> +allOf:
> + - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - microsoft,g6-touch-digitizer
> + - const: hid-over-spi
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + reset-gpios:
> + maxItems: 1
> + description:
> + GPIO specifier for the device's reset line. The HID over SPI
> + specification requires a dedicated, host-driven, active-low reset
> + line. The line must be flagged with GPIO_ACTIVE_LOW.
> +
> + vdd-supply:
> + description:
> + Regulator for the VDD supply voltage.
> +
> + input-report-header-address:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 0xffffff
> + description:
> + A value to be included in the Read Approval packet, listing an address of
> + the input report header to be put on the SPI bus. This address has 24
> + bits.
> +
> + input-report-body-address:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 0xffffff
> + description:
> + A value to be included in the Read Approval packet, listing an address of
> + the input report body to be put on the SPI bus. This address has 24 bits.
> +
> + output-report-address:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 0xffffff
> + description:
> + A value to be included in the Output Report sent by the host, listing an
> + address where the output report on the SPI bus is to be written to. This
> + address has 24 bits.
> +
> + read-opcode:
> + $ref: /schemas/types.yaml#/definitions/uint8
> + description:
> + Value to be used in Read Approval packets. 1 byte.
> +
> + write-opcode:
> + $ref: /schemas/types.yaml#/definitions/uint8
> + description:
> + Value to be used in Output Report packets. 1 byte.
I still dunno why any of these properties exist, especially when there's
a patch in my queue for another microsoft touch device on top of
this series that does not use any of them.
The commit message really needs to explain why these exist and why
compatibles cannot be used to determine these things - as is done for
the microsoft,romulus13-touchpad. If the very next device that appears
seems to require a specific compatible and cannot use these properties
to get across what's needed to the kernel it calls into question why
the binding is designed like this.
pw-bot: changes-requested
Thanks,
Conor.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - reset-gpios
> + - vdd-supply
> + - input-report-header-address
> + - input-report-body-address
> + - output-report-address
> + - read-opcode
> + - write-opcode
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/gpio/gpio.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hid@0 {
> + compatible = "microsoft,g6-touch-digitizer", "hid-over-spi";
> + reg = <0x0>;
> + interrupts-extended = <&gpio 42 IRQ_TYPE_EDGE_FALLING>;
> + reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
> + vdd-supply = <&pm8350c_l3>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&ts_d6_int_bias>;
> + input-report-header-address = <0x1000>;
> + input-report-body-address = <0x1004>;
> + output-report-address = <0x2000>;
> + read-opcode = /bits/ 8 <0x0b>;
> + write-opcode = /bits/ 8 <0x02>;
> + };
> + };
>
> --
> 2.56.0.212.g129ede20d9-goog
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-10-20 17:57 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-19 19:01 [PATCH v6 00/11] Add spi-hid transport driver Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 01/11] Documentation: Correction in HID output_report callback description Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus Jingyuan Liang
2026-10-19 19:11 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer Jingyuan Liang
2026-10-19 19:26 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
2026-10-19 19:22 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 06/11] HID: spi-hid: add spi_hid traces Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 07/11] HID: spi-hid: add ACPI support for HID over SPI Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
2026-10-20 16:34 ` Rob Herring (Arm)
2026-10-20 17:57 ` Conor Dooley
2026-10-19 19:01 ` [PATCH v6 09/11] HID: spi-hid: add device tree support for HID over SPI Jingyuan Liang
2026-10-19 19:01 ` [PATCH v6 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
2026-10-19 19:36 ` sashiko-bot
2026-10-19 19:01 ` [PATCH v6 11/11] HID: spi-hid: add panel follower support Jingyuan Liang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox