* [PATCH v7 01/11] Documentation: Correction in HID output_report callback description.
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro Jingyuan Liang
` (9 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 01/11] Documentation: Correction in HID output_report callback description Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus Jingyuan Liang
` (8 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 01/11] Documentation: Correction in HID output_report callback description Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 02/11] HID: Add BUS_SPI support and define HID_SPI_DEVICE macro Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 04/11] HID: spi-hid: add spi-hid driver HID layer Jingyuan Liang
` (7 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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 | 158 +++++++++++++++++++++++++++++++++++++
5 files changed, 186 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..4991dc972cae
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -0,0 +1,158 @@
+// 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;
+};
+
+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;
+}
+
+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: %d\n", __func__, error);
+ 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: %d\n", error);
+}
+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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 04/11] HID: spi-hid: add spi-hid driver HID layer
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (2 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
` (6 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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 | 588 +++++++++++++++++++++++++++++++++++++
1 file changed, 588 insertions(+)
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 4991dc972cae..54c278ac8803 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,9 +117,45 @@ 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. */
};
+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)
+{
+ return spi_write(shid->spi, buf, length);
+}
+
static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state)
{
switch (power_state) {
@@ -78,11 +170,489 @@ 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);
+ }
+
+ if (!ret) {
+ dev_err(dev, "Empty report descriptor\n");
+ return -EPROTO;
+ }
+
+ 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,
+};
+
int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
struct spi_hid_conf *conf)
{
@@ -104,6 +674,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
@@ -136,6 +718,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);
@@ -146,6 +732,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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (3 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 04/11] HID: spi-hid: add spi-hid driver HID layer Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 1:13 ` sashiko-bot
2026-10-24 0:51 ` [PATCH v7 06/11] HID: spi-hid: add spi_hid traces Jingyuan Liang
` (5 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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 | 781 +++++++++++++++++++++++++++++++++++--
1 file changed, 746 insertions(+), 35 deletions(-)
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 54c278ac8803..cd4d76384009 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,15 +190,92 @@ 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. */
+
+ /* 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)
@@ -151,6 +287,31 @@ 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);
+ return error;
+ }
+
+ return 0;
+}
+
static int spi_hid_output(struct spi_hid *shid, const void *buf, u16 length)
{
return spi_write(shid->spi, buf, length);
@@ -172,17 +333,96 @@ 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: %d\n", 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);
+
+ /* Drop work requested before this reset. */
+ clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
+ clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
+ clear_bit(SPI_HID_ERROR, &shid->flags);
+
+ 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;
@@ -191,6 +431,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",
@@ -198,6 +446,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)
@@ -208,9 +457,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;
}
@@ -219,7 +476,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,
@@ -228,22 +485,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,
@@ -253,9 +514,170 @@ 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);
+ }
+
+ 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;
+ }
+
+ 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);
+
+ 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)
{
@@ -268,10 +690,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;
}
@@ -315,7 +741,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) {
@@ -324,13 +752,206 @@ 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) {
+ shid->reset_attempts = 0;
+ 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;
+ }
+
+ /* The body must at least hold the body header. */
+ if (header->report_length < sizeof(shid->input->body)) {
+ dev_err(dev, "Input report too small: %u < %zu\n",
+ header->report_length, sizeof(shid->input->body));
+ 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)
{
@@ -355,6 +976,15 @@ 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);
+ /*
+ * -ENODEV: the device is not ready (e.g. a reset is in
+ * progress), so the GET_FEATURE request was not sent.
+ * Don't start another reset on top of the one already running.
+ */
+ if (error != -ENODEV) {
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
+ }
return error;
}
@@ -387,9 +1017,75 @@ 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);
+ 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);
+ 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);
+ 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;
}
@@ -469,6 +1165,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);
@@ -486,6 +1184,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;
}
@@ -494,7 +1196,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);
- shid->reset_attempts = 0;
+ clear_bit(SPI_HID_STARTED, &shid->flags);
}
static int spi_hid_ll_power(struct hid_device *hid, int level)
@@ -525,11 +1227,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) {
@@ -552,6 +1251,8 @@ static int spi_hid_ll_parse(struct hid_device *hid)
(unsigned char const *)shid->response->content,
len);
+ /* Fully initialized: allow a fresh set of reset attempts. */
+ shid->reset_attempts = 0;
set_bit(SPI_HID_READY, &shid->flags);
return 0;
@@ -671,12 +1372,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
@@ -701,7 +1411,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;
@@ -715,13 +1425,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);
@@ -732,6 +1440,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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation
2026-10-24 0:51 ` [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
@ 2026-10-24 1:13 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-10-24 1:13 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 3 potential issue(s) to consider:
- [Medium] HID: spi-hid: spi_hid_reset_work() runs pre-error work before SPI_HID_ERROR
- [Medium] HID: spi-hid: sync requests not aborted or serialized on reset in error path
- [Medium] HID: spi-hid: duplicate reset scheduled on timeout in spi_hid_get_request()
--
commit 921e27cc043191cc71704b14d2cdca05f5da7a1c
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add HID SPI protocol implementation
This commit implements the HID over SPI protocol in spi-hid-core.c,
adding interrupt handling, input report processing, synchronous output
request/response tracking, and error recovery via device reset.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 54c278ac8803b..cd4d763840098 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -172,17 +333,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
[ ... ]
> +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: %d\n", 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);
> +
> + /* Drop work requested before this reset. */
> + clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
> + clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> + clear_bit(SPI_HID_ERROR, &shid->flags);
> +
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> + shid->power_state = HIDSPI_ON;
> +
> + shid->ops->deassert_reset(shid->ops);
> +
> + enable_irq(shid->spi->irq);
> +}
[Severity: Medium]
What happens if a synchronous request is waiting on shid->output_done in
__spi_hid_sync_request() while an interrupt error in spi_hid_dev_irq() ->
spi_hid_irq_error() or a spontaneous RESET_RESPONSE in
spi_hid_process_input_report() triggers a reset?
In __spi_hid_sync_request(), the caller holds shid->output_lock while
waiting in wait_for_completion_timeout(&shid->output_done, ...). However,
spi_hid_error_handler() only acquires shid->power_lock: it does not
serialize with shid->output_lock or shid->io_lock, nor does it clear
shid->expected_response or complete shid->output_done to abort an in-flight
request.
Because of this:
1. A thread in __spi_hid_send_output_report() that already passed the
test_bit(SPI_HID_READY, &shid->flags) check before acquiring
shid->io_lock can perform spi_hid_output() on the SPI bus while
spi_hid_error_handler() has asserted reset and set shid->power_state to
HIDSPI_OFF.
2. When the device finishes resetting and sends RESET_RESPONSE,
spi_hid_reset_response() -> spi_hid_sync_request() blocks on
shid->output_lock until the pre-reset waiter's 1000 ms timeout expires.
3. Once wait_for_completion_timeout() in __spi_hid_sync_request() finally
times out, the pre-reset caller sets SPI_HID_ERROR and schedules
shid->reset_work again, triggering a second hardware reset after the
first recovery completes and incrementing shid->reset_attempts a second
time.
Should the error and reset paths abort any in-flight request (or serialize
with shid->output_lock / shid->io_lock) so that pre-reset waiters do not
stall recovery and schedule a stale reset on timeout?
[ ... ]
> @@ -324,13 +752,206 @@ static int spi_hid_create_device(struct spi_hid *shid)
[ ... ]
> +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);
> + }
[Severity: Medium]
Should spi_hid_reset_work() check SPI_HID_ERROR before
SPI_HID_RESET_RESPONSE and SPI_HID_CREATE_DEVICE?
In spi_hid_error_handler(), the code clears SPI_HID_RESET_RESPONSE and
SPI_HID_CREATE_DEVICE with the comment:
/* Drop work requested before this reset. */
clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
clear_bit(SPI_HID_ERROR, &shid->flags);
However, because spi_hid_error_handler() is only called from the final
else-if branch in spi_hid_reset_work(), it cannot run while
SPI_HID_RESET_RESPONSE or SPI_HID_CREATE_DEVICE is set. If an error sets
SPI_HID_ERROR while either of those bits is pending (for example, an IRQ
error in spi_hid_dev_irq() -> spi_hid_irq_error() after
spi_hid_dev_desc_response() sets SPI_HID_CREATE_DEVICE),
spi_hid_reset_work() will first execute spi_hid_reset_response() or
spi_hid_create_device() / spi_hid_refresh_device(), reschedule itself, and
only then run spi_hid_error_handler() to immediately reset the newly
created or refreshed HID device.
[ ... ]
> @@ -355,6 +976,15 @@ 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);
> + /*
> + * -ENODEV: the device is not ready (e.g. a reset is in
> + * progress), so the GET_FEATURE request was not sent.
> + * Don't start another reset on top of the one already running.
> + */
> + if (error != -ENODEV) {
> + set_bit(SPI_HID_ERROR, &shid->flags);
> + schedule_work(&shid->reset_work);
> + }
> return error;
> }
[Severity: Medium]
Won't a timed-out request in spi_hid_get_request() schedule a duplicate
reset?
When __spi_hid_sync_request() times out waiting on shid->output_done, it
already sets SPI_HID_ERROR and schedules shid->reset_work before returning
-ETIMEDOUT:
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;
Because spi_hid_reset_work() -> spi_hid_error_handler() does not take
shid->output_lock, the worker thread on another CPU can immediately begin
running spi_hid_reset_work() and clear SPI_HID_ERROR while
spi_hid_get_request() (and similarly spi_hid_report_descriptor_request()
and spi_hid_reset_response()) receives -ETIMEDOUT and sets SPI_HID_ERROR
and schedules shid->reset_work a second time. This triggers two
back-to-back hardware resets and increments shid->reset_attempts twice for
a single timeout.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-send-upstream-v7-0-abad13174b43@chromium.org?part=5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v7 06/11] HID: spi-hid: add spi_hid traces
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (4 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 07/11] HID: spi-hid: add ACPI support for HID over SPI Jingyuan Liang
` (4 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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 | 126 ++++++++----------------
drivers/hid/spi-hid/spi-hid-core.h | 103 ++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-trace.h | 184 ++++++++++++++++++++++++++++++++++++
4 files changed, 328 insertions(+), 86 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 cd4d76384009..f30892a4f91f 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,66 +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. */
-
- /* 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,
@@ -303,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);
@@ -350,6 +274,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;
@@ -529,6 +455,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);
@@ -555,6 +483,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) {
@@ -654,6 +584,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) {
@@ -722,6 +654,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) {
@@ -766,6 +700,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);
@@ -855,6 +791,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);
@@ -1033,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");
@@ -1046,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);
@@ -1073,6 +1021,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..e66594b7a8cb
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -0,0 +1,103 @@
+/* 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. */
+
+ /* 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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 07/11] HID: spi-hid: add ACPI support for HID over SPI
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (5 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 06/11] HID: spi-hid: add spi_hid traces Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
` (3 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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 | 260 +++++++++++++++++++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-core.c | 46 +++----
drivers/hid/spi-hid/spi-hid.h | 43 ++++++
5 files changed, 339 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..df459c71e209
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-acpi.c
@@ -0,0 +1,260 @@
+// 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,
+ },
+ .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 f30892a4f91f..4ff93d22715c 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,
@@ -318,8 +294,19 @@ 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);
+ /*
+ * 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);
}
@@ -1377,7 +1364,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..a31956ef24ef
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid.h
@@ -0,0 +1,43 @@
+/* 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);
+
+#endif /* SPI_HID_H */
--
2.56.0.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (6 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 07/11] HID: spi-hid: add ACPI support for HID over SPI Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-27 18:17 ` Conor Dooley
2026-10-27 19:10 ` Rob Herring (Arm)
2026-10-24 0:51 ` [PATCH v7 09/11] HID: spi-hid: add device tree support for HID over SPI Jingyuan Liang
` (2 subsequent siblings)
10 siblings, 2 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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
From: Dmitry Antipov <dmanti@microsoft.com>
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.
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 | 75 ++++++++++++++++++++++
1 file changed, 75 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..bed0746f004c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
@@ -0,0 +1,75 @@
+# 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 <bentiss@kernel.org>
+ - Jiri Kosina <jikos@kernel.org>
+ - 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
+
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - microsoft,g6-touch-digitizer
+
+ 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.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - reset-gpios
+ - vdd-supply
+
+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";
+ 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>;
+ };
+ };
--
2.56.0.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema
2026-10-24 0:51 ` [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
@ 2026-10-27 18:17 ` Conor Dooley
2026-10-27 19:10 ` Rob Herring (Arm)
1 sibling, 0 replies; 17+ messages in thread
From: Conor Dooley @ 2026-10-27 18:17 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: 3891 bytes --]
On Sat, Oct 24, 2026 at 12:51:42AM +0000, Jingyuan Liang wrote:
> From: Dmitry Antipov <dmanti@microsoft.com>
>
> 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.
>
> Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
Personally I prefer how this is done now, but definitely worth those who
approved the previous iteration taking a look at this because it has
changed significantly and no longer contains any of the properties that
cover the timings etc anymore.
Worth noting that this changed since the previous version after I
pointed out that the second hid-over-spi device did not make use of them
at all and needed specific handling in the driver.
> ---
> .../devicetree/bindings/input/hid-over-spi.yaml | 75 ++++++++++++++++++++++
> 1 file changed, 75 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
Typically this would need to match a compatible, but I think this is okay
provided other hid-over-spi devices actually get added here - the series
adding the second hid-over-spi device didn't do so but should be able to
I think?
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> new file mode 100644
> index 000000000000..bed0746f004c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> @@ -0,0 +1,75 @@
> +# 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 <bentiss@kernel.org>
> + - Jiri Kosina <jikos@kernel.org>
> + - 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
> +
> +allOf:
> + - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - microsoft,g6-touch-digitizer
> +
> + 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.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - reset-gpios
> + - vdd-supply
> +
> +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";
> + 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>;
> + };
> + };
>
> --
> 2.56.0.257.gf7497c7d27-goog
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema
2026-10-24 0:51 ` [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
2026-10-27 18:17 ` Conor Dooley
@ 2026-10-27 19:10 ` Rob Herring (Arm)
1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring (Arm) @ 2026-10-27 19:10 UTC (permalink / raw)
To: Jingyuan Liang
Cc: Dmitry Torokhov, Jonathan Corbet, tfiga, Mark Brown, daleyo,
Jiri Kosina, Dmitry Antipov, linux-spi, linux-trace-kernel,
Conor Dooley, Masami Hiramatsu, Jarrett Schultz, linux-doc,
Mathieu Desnoyers, hbarnor, Steven Rostedt, devicetree,
Benjamin Tissoires, Krzysztof Kozlowski, linux-input,
linux-kernel, fqwqf
On Sat, 24 Oct 2026 00:51:42 +0000, Jingyuan Liang wrote:
> From: Dmitry Antipov <dmanti@microsoft.com>
>
> 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.
>
> 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 | 75 ++++++++++++++++++++++
> 1 file changed, 75 insertions(+)
>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v7 09/11] HID: spi-hid: add device tree support for HID over SPI
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (7 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 08/11] dt-bindings: input: Document hid-over-spi DT schema Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
2026-10-24 0:51 ` [PATCH v7 11/11] HID: spi-hid: add panel follower support Jingyuan Liang
10 siblings, 0 replies; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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>
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 | 210 +++++++++++++++++++++++++++++++++++++++
3 files changed, 226 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..d1bbdc17102e
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -0,0 +1,210 @@
+// 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/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+#include "spi-hid.h"
+
+/* Per-compatible data: protocol parameters and timings */
+struct spi_hid_of_data {
+ struct spi_hid_conf conf;
+ u32 post_power_on_delay_ms;
+ u32 minimal_reset_delay_ms;
+};
+
+/* Config structure is filled with match data and Device Tree resources */
+struct spi_hid_of_config {
+ struct spihid_ops ops;
+
+ struct spi_hid_of_data data;
+
+ struct gpio_desc *reset_gpio;
+ struct regulator *supply;
+ bool supply_enabled;
+};
+
+static const struct spi_hid_of_data microsoft_g6_data = {
+ .conf = {
+ .input_report_header_address = 0x1000,
+ .input_report_body_address = 0x1004,
+ .output_report_address = 0x2000,
+ .read_opcode = 0x0b,
+ .write_opcode = 0x02,
+ },
+ .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)
+{
+ 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->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->data.minimal_reset_delay_ms);
+}
+
+static int spi_hid_of_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ const struct spi_hid_of_data *data;
+ 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;
+
+ data = spi_get_device_match_data(spi);
+ if (!data)
+ return dev_err_probe(dev, -ENODEV, "No match data\n");
+
+ config->data = *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->data.conf);
+}
+
+static const struct of_device_id spi_hid_of_match[] = {
+ { .compatible = "microsoft,g6-touch-digitizer", .data = µsoft_g6_data },
+ { }
+};
+MODULE_DEVICE_TABLE(of, spi_hid_of_match);
+
+static const struct spi_device_id spi_hid_of_id_table[] = {
+ { .name = "g6-touch-digitizer", .driver_data = (kernel_ulong_t)µsoft_g6_data },
+ { }
+};
+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,
+ },
+ .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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v7 10/11] HID: spi-hid: add power management implementation
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (8 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 09/11] HID: spi-hid: add device tree support for HID over SPI Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 1:19 ` sashiko-bot
2026-10-24 0:51 ` [PATCH v7 11/11] HID: spi-hid: add panel follower support Jingyuan Liang
10 siblings, 1 reply; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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.
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 | 188 ++++++++++++++++++++++++++++++++++++-
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 | 2 +
5 files changed, 192 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
index df459c71e209..ea291ca269d4 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,
},
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 4ff93d22715c..3b8874ca0e1c 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 {
@@ -231,6 +239,147 @@ 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 int 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 -ETIMEDOUT;
+ }
+
+ set_bit(SPI_HID_READY, &shid->flags);
+ return 0;
+}
+
+static int spi_hid_suspend(struct spi_hid *shid)
+{
+ int error;
+ struct device *dev = &shid->spi->dev;
+
+ /*
+ * Stop reset_work until resume; pending steps keep their flags. The
+ * IRQ stays on so a request in flight gets its response.
+ */
+ disable_work_sync(&shid->reset_work);
+
+ /* 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);
+ enable_work(&shid->reset_work);
+ /* Redo a schedule_work() dropped while disabled. */
+ schedule_work(&shid->reset_work);
+ return error;
+ }
+ }
+ }
+
+ /* disable_irq() waits for a running IRQ thread. */
+ disable_irq(shid->spi->irq);
+
+ 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: %d\n", __func__, error);
+
+ /* Held in reset either way; resume resets it. */
+ 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);
+ enable_work(&shid->reset_work);
+ if (test_bit(SPI_HID_DEAD, &shid->flags)) {
+ /* Stay off; only balance disable_*() 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: %d\n", __func__, 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);
+ error = shid->ops->deassert_reset(shid->ops);
+ if (error)
+ dev_err(dev, "%s: failed to deassert reset: %d\n",
+ __func__, error);
+ else
+ was_reset = true;
+ }
+ }
+
+ enable_irq(shid->spi->irq);
+
+ /*
+ * For devices that may wake up, process work that was pending while
+ * suspend had reset_work disabled. 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)) {
+ /* No reset response: let recovery retry. */
+ set_bit(SPI_HID_ERROR, &shid->flags);
+ schedule_work(&shid->reset_work);
+ return -ETIMEDOUT;
+ }
+ /* HID requests would fail while the device is not ready. */
+ if (!test_bit(SPI_HID_READY, &shid->flags))
+ return 0;
+ 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;
@@ -266,6 +415,7 @@ static void spi_hid_error_handler(struct spi_hid *shid)
if (error)
dev_err(dev, "failed to disable regulator: %d\n", error);
shid->power_state = HIDSPI_OFF;
+ set_bit(SPI_HID_DEAD, &shid->flags);
/* Dead device: leave the IRQ permanently disabled. */
return;
}
@@ -454,8 +604,8 @@ static void spi_hid_reset_response(struct spi_hid *shid)
error = spi_hid_sync_request(shid, &report);
if (error) {
- dev_WARN_ONCE(dev, true,
- "Failed to send device descriptor request: %d\n", error);
+ dev_err(dev, "Failed to send device descriptor request: %d\n",
+ error);
set_bit(SPI_HID_ERROR, &shid->flags);
schedule_work(&shid->reset_work);
}
@@ -546,6 +696,8 @@ static int spi_hid_dev_desc_response(struct spi_hid *shid,
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;
}
@@ -1325,6 +1477,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);
@@ -1358,6 +1511,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: %d\n", __func__, error);
@@ -1398,6 +1563,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 e66594b7a8cb..060272353216 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 d1bbdc17102e..6de9affbebe9 100644
--- a/drivers/hid/spi-hid/spi-hid-of.c
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -195,6 +195,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,
},
diff --git a/drivers/hid/spi-hid/spi-hid.h b/drivers/hid/spi-hid/spi-hid.h
index a31956ef24ef..95acb2845246 100644
--- a/drivers/hid/spi-hid/spi-hid.h
+++ b/drivers/hid/spi-hid/spi-hid.h
@@ -40,4 +40,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 dev_pm_ops spi_hid_core_pm;
+
#endif /* SPI_HID_H */
--
2.56.0.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v7 10/11] HID: spi-hid: add power management implementation
2026-10-24 0:51 ` [PATCH v7 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
@ 2026-10-24 1:19 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-10-24 1:19 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 2 potential issue(s) to consider:
- [High] hid: spi-hid: unhandled deassert_reset() failure in spi_hid_resume()
- [Medium] HID: spi-hid: race and missed hid_driver_reset_resume() in spi_hid_resume()
--
commit 9bd51942caad3c04cffc9fdd14002340e1980586
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add power management implementation
This patch adds system sleep power management callbacks to the HID over SPI
transport driver, handling device power down, reset synchronization, and wake
IRQ setup across suspend and resume.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 4ff93d22715cd..3b8874ca0e1cc 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -231,6 +239,147 @@ 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 int 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 -ETIMEDOUT;
> + }
> +
> + set_bit(SPI_HID_READY, &shid->flags);
> + return 0;
> +}
> +
> +static int spi_hid_suspend(struct spi_hid *shid)
> +{
> + int error;
> + struct device *dev = &shid->spi->dev;
> +
> + /*
> + * Stop reset_work until resume; pending steps keep their flags. The
> + * IRQ stays on so a request in flight gets its response.
> + */
> + disable_work_sync(&shid->reset_work);
> +
> + /* 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);
> + enable_work(&shid->reset_work);
> + /* Redo a schedule_work() dropped while disabled. */
> + schedule_work(&shid->reset_work);
> + return error;
> + }
> + }
> + }
> +
> + /* disable_irq() waits for a running IRQ thread. */
> + disable_irq(shid->spi->irq);
> +
> + 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: %d\n", __func__, error);
> +
> + /* Held in reset either way; resume resets it. */
> + 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);
> + enable_work(&shid->reset_work);
> + if (test_bit(SPI_HID_DEAD, &shid->flags)) {
> + /* Stay off; only balance disable_*() 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: %d\n", __func__, 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);
> + error = shid->ops->deassert_reset(shid->ops);
> + if (error)
> + dev_err(dev, "%s: failed to deassert reset: %d\n",
> + __func__, error);
> + else
> + was_reset = true;
> + }
> + }
[Severity: High]
What happens in spi_hid_resume() if shid->ops->deassert_reset(shid->ops)
fails (or if was_reset is true while shid->hid is still NULL and the reset
times out)?
When deassert_reset() fails in spi_hid_error_handler(), the driver sets
SPI_HID_ERROR and schedules shid->reset_work because a device held in reset
will not assert a reset-response IRQ:
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_error_handler() {
...
error = shid->ops->deassert_reset(shid->ops);
if (error) {
dev_err(dev, "failed to deassert reset: %d\n", error);
set_bit(SPI_HID_ERROR, &shid->flags);
schedule_work(&shid->reset_work);
}
}
In spi_hid_resume(), however, a deassert_reset() failure only logs the error
and leaves was_reset false. Because spi_hid_suspend() cleared SPI_HID_READY,
SPI_HID_RESET_RESPONSE, SPI_HID_CREATE_DEVICE, and SPI_HID_ERROR before
powering down, shid->reset_work is not scheduled, spi_hid_wait_for_reset() is
skipped, and spi_hid_resume() returns 0 when !test_bit(SPI_HID_READY,
&shid->flags).
Similarly, because spi_hid_wait_for_reset() is gated inside if (shid->hid), if
shid->hid was not yet created before suspend and the post-resume reset times
out, nothing sets SPI_HID_ERROR or schedules shid->reset_work.
Could this leave the device permanently held in reset with its regulator
enabled in HIDSPI_ON and no recovery work queued?
> +
> + enable_irq(shid->spi->irq);
> +
> + /*
> + * For devices that may wake up, process work that was pending while
> + * suspend had reset_work disabled. 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)) {
> + /* No reset response: let recovery retry. */
> + set_bit(SPI_HID_ERROR, &shid->flags);
> + schedule_work(&shid->reset_work);
> + return -ETIMEDOUT;
> + }
> + /* HID requests would fail while the device is not ready. */
> + if (!test_bit(SPI_HID_READY, &shid->flags))
> + return 0;
> + 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;
> +}
[Severity: Medium]
On the wakeup path (when device_may_wakeup(dev) is true and was_reset is
false), how does hid_driver_reset_resume(shid->hid) interact with a pending
reset_work?
When spi_hid_irq_error() sets SPI_HID_ERROR or spi_hid_process_input_report()
sets SPI_HID_RESET_RESPONSE in the IRQ thread, SPI_HID_READY is not cleared
immediately. Instead, SPI_HID_READY is cleared later when shid->reset_work
runs spi_hid_error_handler() (which must first acquire shid->power_lock) or
spi_hid_reset_response().
Because spi_hid_resume() holds shid->power_lock across the entire function,
scheduling shid->reset_work and immediately checking test_bit(SPI_HID_READY,
&shid->flags) means:
1. If SPI_HID_ERROR or SPI_HID_RESET_RESPONSE was set while SPI_HID_READY is
still set, spi_hid_resume() sees SPI_HID_READY set and calls
hid_driver_reset_resume(shid->hid) right before shid->reset_work runs and
resets the device.
2. Conversely, if SPI_HID_READY was already cleared (for example, after a sync
request timeout or while SPI_HID_CREATE_DEVICE is pending), spi_hid_resume()
returns 0 early at !test_bit(SPI_HID_READY, &shid->flags). Once
shid->reset_work later completes in spi_hid_refresh_device():
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_refresh_device() {
...
/* Same report descriptor, so no need to create a new hid device. */
if (new_crc32 == shid->report_descriptor_crc32) {
shid->reset_attempts = 0;
set_bit(SPI_HID_READY, &shid->flags);
return;
}
}
it sets SPI_HID_READY and returns without calling
hid_driver_reset_resume(shid->hid), leaving the HID driver in the suspended
state set by spi_hid_suspend().
Should spi_hid_resume() coordinate with pending reset work on the wakeup path
so hid_driver_reset_resume() runs after the reset completes?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-send-upstream-v7-0-abad13174b43@chromium.org?part=10
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v7 11/11] HID: spi-hid: add panel follower support
2026-10-24 0:51 [PATCH v7 00/11] Add spi-hid transport driver Jingyuan Liang
` (9 preceding siblings ...)
2026-10-24 0:51 ` [PATCH v7 10/11] HID: spi-hid: add power management implementation Jingyuan Liang
@ 2026-10-24 0:51 ` Jingyuan Liang
2026-10-24 1:10 ` sashiko-bot
10 siblings, 1 reply; 17+ messages in thread
From: Jingyuan Liang @ 2026-10-24 0:51 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.
Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
---
drivers/hid/spi-hid/Kconfig | 4 +
drivers/hid/spi-hid/spi-hid-core.c | 221 ++++++++++++++++++++++++++++---------
drivers/hid/spi-hid/spi-hid-core.h | 7 ++
3 files changed, 177 insertions(+), 55 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 3b8874ca0e1c..496b33cdf3d4 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -277,10 +277,13 @@ static int spi_hid_suspend(struct spi_hid *shid)
if (error) {
dev_err(dev, "%s failed to suspend hid driver: %d\n",
__func__, error);
- enable_work(&shid->reset_work);
- /* Redo a schedule_work() dropped while disabled. */
- schedule_work(&shid->reset_work);
- return error;
+ /* A follower powers off with the panel anyway. */
+ if (!shid->is_panel_follower) {
+ enable_work(&shid->reset_work);
+ /* Redo a schedule_work() dropped while disabled. */
+ schedule_work(&shid->reset_work);
+ return error;
+ }
}
}
}
@@ -325,28 +328,30 @@ 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: %d\n", __func__, 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);
- error = shid->ops->deassert_reset(shid->ops);
- if (error)
- dev_err(dev, "%s: failed to deassert reset: %d\n",
- __func__, error);
- else
- was_reset = true;
+ error = shid->ops->power_up(shid->ops);
+ if (error) {
+ dev_err(dev, "%s: could not power up: %d\n", __func__, 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);
+ error = shid->ops->deassert_reset(shid->ops);
+ if (error)
+ dev_err(dev, "%s: failed to deassert reset: %d\n",
+ __func__, error);
+ else
+ was_reset = true;
}
enable_irq(shid->spi->irq);
@@ -1447,6 +1452,108 @@ static struct hid_ll_driver spi_hid_ll_driver = {
.raw_request = spi_hid_ll_raw_request,
};
+/*
+ * 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: %d\n", __func__, 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)
{
@@ -1462,10 +1569,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);
@@ -1480,6 +1588,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
@@ -1490,20 +1599,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) {
@@ -1523,21 +1618,19 @@ 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: %d\n", __func__, error);
- 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) {
+ /* Like the IRQ: the first panel_prepared resume enables it. */
+ disable_work(&shid->reset_work);
+ 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));
@@ -1551,15 +1644,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: %d\n", error);
+ 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: %d\n", error);
+ }
}
EXPORT_SYMBOL_GPL(spi_hid_core_remove);
@@ -1567,6 +1672,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);
}
@@ -1574,6 +1682,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 060272353216..28e9880ad464 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.257.gf7497c7d27-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v7 11/11] HID: spi-hid: add panel follower support
2026-10-24 0:51 ` [PATCH v7 11/11] HID: spi-hid: add panel follower support Jingyuan Liang
@ 2026-10-24 1:10 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-10-24 1:10 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 2 potential issue(s) to consider:
- [Medium] HID: spi-hid: unrecovered deassert_reset() failure in spi_hid_resume()
- [Medium] HID: spi-hid: improper wakeup teardown for panel followers in probe
--
commit 35d9a38c1b6ffa8dbb3ab740ec9ce120c5b2e7a1
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add panel follower support
This commit adds DRM panel follower support to spi-hid so that a follower
device powers up and suspends in coordination with its panel rather than
during initial probe and standard system PM callbacks.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 3b8874ca0e1cc..496b33cdf3d46 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -325,28 +328,30 @@ 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: %d\n", __func__, 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);
> - error = shid->ops->deassert_reset(shid->ops);
> - if (error)
> - dev_err(dev, "%s: failed to deassert reset: %d\n",
> - __func__, error);
> - else
> - was_reset = true;
> + error = shid->ops->power_up(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power up: %d\n", __func__, 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);
> + error = shid->ops->deassert_reset(shid->ops);
> + if (error)
> + dev_err(dev, "%s: failed to deassert reset: %d\n",
> + __func__, error);
> + else
> + was_reset = true;
> }
[Severity: Medium]
What happens if shid->ops->deassert_reset(shid->ops) fails in
spi_hid_resume()?
For non-followers, initial power-up goes through spi_hid_dev_init(), which
powers down the device and returns the error when deassert_reset() fails.
For panel followers, spi_hid_core_probe() leaves shid->power_state as
HIDSPI_OFF and shid->hid as NULL, and spi_hid_panel_follower_work() relies
on spi_hid_resume() for the initial power-up as well as subsequent resumes.
When deassert_reset() fails in spi_hid_resume(), was_reset remains false
while shid->power_state is set to HIDSPI_ON and the IRQ is enabled with the
device still held in reset:
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_resume() {
...
if (shid->hid) {
if (was_reset && spi_hid_wait_for_reset(shid)) {
/* No reset response: let recovery retry. */
set_bit(SPI_HID_ERROR, &shid->flags);
schedule_work(&shid->reset_work);
return -ETIMEDOUT;
}
/* HID requests would fail while the device is not ready. */
if (!test_bit(SPI_HID_READY, &shid->flags))
return 0;
...
}
On the first power-up of a panel follower, shid->hid is NULL, so
spi_hid_resume() returns 0 without scheduling shid->reset_work. On subsequent
resumes where shid->hid is non-NULL, was_reset is false and SPI_HID_READY was
cleared during spi_hid_suspend(), so spi_hid_resume() again returns 0 without
setting SPI_HID_ERROR or scheduling shid->reset_work.
Should a deassert_reset() failure in spi_hid_resume() trigger recovery (or
clean up and return the error) so the device is not left powered on in reset
and permanently uninitialized?
> @@ -1447,6 +1452,108 @@ static struct hid_ll_driver spi_hid_ll_driver = {
[ ... ]
> +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);
> +}
[ ... ]
> @@ -1523,21 +1618,19 @@ 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: %d\n", __func__, error);
> - 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) {
> + /* Like the IRQ: the first panel_prepared resume enables it. */
> + disable_work(&shid->reset_work);
> + error = spi_hid_register_panel_follower(shid);
[Severity: Medium]
Does calling device_set_wakeup_capable(dev, false) in
spi_hid_register_panel_follower() after devm_pm_set_wake_irq() in
spi_hid_core_probe() leave the wake IRQ configured and leak
dev->power.wakeup if wakeup was enabled on the device before probe?
In spi_hid_core_probe(), if the firmware node marked the device as wakeup
capable (for example via acpi_bind_one()), device_can_wakeup(dev) is true and
devm_pm_set_wake_irq(dev, spi->irq) attaches a wake IRQ before
shid->is_panel_follower is checked:
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_core_probe() {
...
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;
}
}
if (shid->is_panel_follower) {
/* Like the IRQ: the first panel_prepared resume enables it. */
disable_work(&shid->reset_work);
error = spi_hid_register_panel_follower(shid);
...
}
Furthermore, if a wakeup_source was attached to dev->power.wakeup (for
example via sysfs power/wakeup before async or deferred probe completes),
calling device_set_wakeup_capable(dev, false) directly sets
dev->power.can_wakeup = false without detaching the wakeup source first.
Subsequent calls to device_wakeup_disable(dev), including from
device_pm_remove() on unbind, check dev->power.can_wakeup and return early:
drivers/base/power/wakeup.c:device_wakeup_disable() {
struct wakeup_source *ws;
if (!dev || !dev->power.can_wakeup)
return;
ws = device_wakeup_detach(dev);
wakeup_source_unregister(ws);
}
Should spi_hid_register_panel_follower() use device_init_wakeup(dev, false)
(which calls device_wakeup_disable(dev) before clearing wakeup capability),
and should spi_hid_core_probe() skip devm_pm_set_wake_irq() when
shid->is_panel_follower is true?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-send-upstream-v7-0-abad13174b43@chromium.org?part=11
^ permalink raw reply [flat|nested] 17+ messages in thread