* [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus
@ 2022-01-15 2:31 Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 1/5] HID: Add BUS_SPI support when printing out device info in hid_connect() Dmitry Antipov
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-01-15 2:31 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, Felipe Balbi
Cc: linux-input, devicetree, linux-spi, Dmitry Antipov
Surface Duo devices use a touch digitizer that communicates to the main
SoC via SPI and presents itself as a HID device. This patch's goal is to
add the spi-hid transport driver to drivers/hid. The driver follows the
publically available HID Over SPI Protocol Specification version 1.0.
The specification is available at
https://www.microsoft.com/en-us/download/details.aspx?id=103325.
In the initial commits there are some HID core changes to support a SPI
device, a change to HID documentation, HID over SPI Device Tree
bindings, and finally the SPI HID transport driver.
Dmitry Antipov (5):
HID: Add BUS_SPI support when printing out device info in
hid_connect()
HID: define HID_SPI_DEVICE macro in hid.h
Documentation: DT bindings for HID over SPI.
Documentation: Correction in HID output_report callback description.
HID: add spi-hid, transport driver for HID over SPI bus
.../bindings/input/hid-over-spi.txt | 43 +
Documentation/hid/hid-transport.rst | 4 +-
arch/arm64/configs/defconfig | 1 +
drivers/hid/Kconfig | 2 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-core.c | 3 +
drivers/hid/spi-hid/Kconfig | 25 +
drivers/hid/spi-hid/Makefile | 12 +
drivers/hid/spi-hid/spi-hid-core.c | 1328 +++++++++++++++++
drivers/hid/spi-hid/spi-hid-core.h | 188 +++
drivers/hid/spi-hid/spi-hid-of.c | 148 ++
drivers/hid/spi-hid/spi-hid-of.h | 34 +
drivers/hid/spi-hid/spi-hid_trace.h | 198 +++
drivers/hid/spi-hid/trace.c | 11 +
include/linux/hid.h | 2 +
15 files changed, 1998 insertions(+), 2 deletions(-)
create mode 100755 Documentation/devicetree/bindings/input/hid-over-spi.txt
create mode 100644 drivers/hid/spi-hid/Kconfig
create mode 100644 drivers/hid/spi-hid/Makefile
create mode 100644 drivers/hid/spi-hid/spi-hid-core.c
create mode 100644 drivers/hid/spi-hid/spi-hid-core.h
create mode 100755 drivers/hid/spi-hid/spi-hid-of.c
create mode 100755 drivers/hid/spi-hid/spi-hid-of.h
create mode 100644 drivers/hid/spi-hid/spi-hid_trace.h
create mode 100644 drivers/hid/spi-hid/trace.c
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/5] HID: Add BUS_SPI support when printing out device info in hid_connect()
2022-01-15 2:31 [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus Dmitry Antipov
@ 2022-01-15 2:31 ` Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 2/5] HID: define HID_SPI_DEVICE macro in hid.h Dmitry Antipov
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-01-15 2:31 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, Felipe Balbi
Cc: linux-input, devicetree, linux-spi, Dmitry Antipov
From: Dmitry Antipov <dmanti@microsoft.com>
If connecting a hid_device with bus field indicating BUS_SPI print out
"SPI" in the debug print.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
---
drivers/hid/hid-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index dbed2524fd47..65350ad985fe 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2005,6 +2005,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_VIRTUAL:
bus = "VIRTUAL";
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/5] HID: define HID_SPI_DEVICE macro in hid.h
2022-01-15 2:31 [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 1/5] HID: Add BUS_SPI support when printing out device info in hid_connect() Dmitry Antipov
@ 2022-01-15 2:31 ` Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 3/5] Documentation: DT bindings for HID over SPI Dmitry Antipov
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-01-15 2:31 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, Felipe Balbi
Cc: linux-input, devicetree, linux-spi, Dmitry Antipov
From: Dmitry Antipov <dmanti@microsoft.com>
Macro sets the bus field to BUS_SPI and uses arguments to set vendor
product fields.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
---
include/linux/hid.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f453be385bd4..1f134c8f8972 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -684,6 +684,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.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 3/5] Documentation: DT bindings for HID over SPI.
2022-01-15 2:31 [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 1/5] HID: Add BUS_SPI support when printing out device info in hid_connect() Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 2/5] HID: define HID_SPI_DEVICE macro in hid.h Dmitry Antipov
@ 2022-01-15 2:31 ` Dmitry Antipov
2022-01-20 20:06 ` Rob Herring
2022-01-24 10:32 ` Felipe Balbi
2022-01-15 2:31 ` [PATCH v3 4/5] Documentation: Correction in HID output_report callback description Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus Dmitry Antipov
4 siblings, 2 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-01-15 2:31 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, Felipe Balbi
Cc: linux-input, devicetree, linux-spi, Dmitry Antipov
From: Dmitry Antipov <dmanti@microsoft.com>
Added documentation describes the required properties for implementing
Device Tree for a device supporting HID over SPI and also provides an
example.
Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
---
.../bindings/input/hid-over-spi.txt | 43 +++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100755 Documentation/devicetree/bindings/input/hid-over-spi.txt
diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.txt b/Documentation/devicetree/bindings/input/hid-over-spi.txt
new file mode 100755
index 000000000000..5eba95b5724e
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/hid-over-spi.txt
@@ -0,0 +1,43 @@
+* HID over SPI Device-Tree bindings
+
+HID over SPI provides support for Human Interface Devices over the SPI bus. HID
+Over SPI Protocol Specification 1.0 was written by Microsoft and is available at
+https://www.microsoft.com/en-us/download/details.aspx?id=103325.
+
+If this binding is used, the kernel module spi-hid will handle the communication
+with the device and the generic hid core layer will handle the protocol.
+
+Required properties:
+- compatible: must be "hid-over-spi"
+- interrupts: interrupt line
+- vdd-supply: phandle of the regulator that provides the supply voltage
+- reset_gpio-gpios: gpio wired to the device reset line
+- post-power-on-delay-ms: time required by the device after enabling its
+regulators or powering it on, before it is ready for communication
+- minimal-reset-delay-ms: minimum amount of time that device needs to be in
+reset state for the reset to take effect
+- input-report-header-address: this property and the rest are described in HID
+Over SPI Protocol Spec 1.0
+- input-report-body-address
+- output-report-address
+- read-opcode
+- write-opcode
+- flags
+
+Example:
+ spi-hid-dev0 {
+ compatible = "hid-over-spi";
+ reg = <0>;
+ interrupts-extended = <&tlmm 42 IRQ_TYPE_EDGE_FALLING>;
+ vdd-supply = <&pm8350c_l3>;
+ input-report-header-address = <0x1000>;
+ input-report-body-address = <0x1004>;
+ output-report-address = <0x2000>;
+ read-opcode = <0x0b>;
+ write-opcode = <0x02>;
+ flags = <0x00>;
+ reset_gpio-gpios = <&tlmm 25 GPIO_ACTIVE_LOW>;
+ post-power-on-delay-ms = <5>;
+ minimal-reset-delay-ms = <5>;
+
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 4/5] Documentation: Correction in HID output_report callback description.
2022-01-15 2:31 [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus Dmitry Antipov
` (2 preceding siblings ...)
2022-01-15 2:31 ` [PATCH v3 3/5] Documentation: DT bindings for HID over SPI Dmitry Antipov
@ 2022-01-15 2:31 ` Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus Dmitry Antipov
4 siblings, 0 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-01-15 2:31 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, Felipe Balbi
Cc: linux-input, devicetree, linux-spi, Dmitry Antipov
From: Dmitry Antipov <dmanti@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>
---
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.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus
2022-01-15 2:31 [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus Dmitry Antipov
` (3 preceding siblings ...)
2022-01-15 2:31 ` [PATCH v3 4/5] Documentation: Correction in HID output_report callback description Dmitry Antipov
@ 2022-01-15 2:31 ` Dmitry Antipov
2022-01-15 4:32 ` Dmitry Torokhov
2022-01-15 6:10 ` kernel test robot
4 siblings, 2 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-01-15 2:31 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, Felipe Balbi
Cc: linux-input, devicetree, linux-spi, Dmitry Antipov
From: Dmitry Antipov <dmanti@microsoft.com>
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>
---
arch/arm64/configs/defconfig | 1 +
drivers/hid/Kconfig | 2 +
drivers/hid/Makefile | 1 +
drivers/hid/spi-hid/Kconfig | 25 +
drivers/hid/spi-hid/Makefile | 12 +
drivers/hid/spi-hid/spi-hid-core.c | 1328 +++++++++++++++++++++++++++
drivers/hid/spi-hid/spi-hid-core.h | 188 ++++
drivers/hid/spi-hid/spi-hid-of.c | 148 +++
drivers/hid/spi-hid/spi-hid-of.h | 34 +
drivers/hid/spi-hid/spi-hid_trace.h | 198 ++++
drivers/hid/spi-hid/trace.c | 11 +
11 files changed, 1948 insertions(+)
create mode 100644 drivers/hid/spi-hid/Kconfig
create mode 100644 drivers/hid/spi-hid/Makefile
create mode 100644 drivers/hid/spi-hid/spi-hid-core.c
create mode 100644 drivers/hid/spi-hid/spi-hid-core.h
create mode 100755 drivers/hid/spi-hid/spi-hid-of.c
create mode 100755 drivers/hid/spi-hid/spi-hid-of.h
create mode 100644 drivers/hid/spi-hid/spi-hid_trace.h
create mode 100644 drivers/hid/spi-hid/trace.c
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index f2e2b9bdd702..25249a4b0c8a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -805,6 +805,7 @@ CONFIG_SND_AUDIO_GRAPH_CARD=m
CONFIG_HID_MULTITOUCH=m
CONFIG_I2C_HID_ACPI=m
CONFIG_I2C_HID_OF=m
+CONFIG_SPI_HID=m
CONFIG_USB_CONN_GPIO=m
CONFIG_USB=y
CONFIG_USB_OTG=y
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index a7c78ac96270..cd2c10703fcf 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1262,6 +1262,8 @@ source "drivers/hid/usbhid/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 55a6fa3eca5a..caf418dda343 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -144,6 +144,7 @@ obj-$(CONFIG_USB_MOUSE) += usbhid/
obj-$(CONFIG_USB_KBD) += usbhid/
obj-$(CONFIG_I2C_HID_CORE) += i2c-hid/
+obj-$(CONFIG_SPI_HID) += spi-hid/
obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/
diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
new file mode 100644
index 000000000000..37302d658162
--- /dev/null
+++ b/drivers/hid/spi-hid/Kconfig
@@ -0,0 +1,25 @@
+#
+# Copyright (c) 2021 Microsoft Corporation
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 2 as published by
+# the Free Software Foundation.
+#
+menu "SPI HID support"
+ depends on SPI
+
+config SPI_HID
+ tristate "HID over SPI transport layer"
+ default n
+ depends on SPI && INPUT && OF
+ select HID
+ help
+ Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+ other HID based devices which is connected to your computer via SPI.
+
+ If unsure, say N.
+
+ This support is also available as a module. If so, the module
+ will be called spi-hid.
+
+endmenu
diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
new file mode 100644
index 000000000000..0d34a04b5dc7
--- /dev/null
+++ b/drivers/hid/spi-hid/Makefile
@@ -0,0 +1,12 @@
+#
+# Copyright (c) 2021 Microsoft Corporation
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 2 as published by
+# the Free Software Foundation.
+#
+# Makefile for the SPI input drivers
+#
+CFLAGS_trace.o = -I$(src)
+obj-$(CONFIG_SPI_HID) += spi-hid.o
+spi-hid-objs := spi-hid-core.o spi-hid-of.o trace.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..4d5a24613beb
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.c
@@ -0,0 +1,1328 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol implementation
+ * spi-hid-core.c
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ *
+ * 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/crc32.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/hid.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/of.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/string.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include "spi-hid-core.h"
+#include "spi-hid_trace.h"
+#include "spi-hid-of.h"
+#include "../hid-ids.h"
+
+#define SPI_HID_MAX_RESET_ATTEMPTS 3
+
+static struct hid_ll_driver spi_hid_ll_driver;
+
+static void spi_hid_populate_read_approvals(struct spi_hid_of_config *conf,
+ __u8 *header_buf, __u8 *body_buf)
+{
+ header_buf[0] = conf->read_opcode;
+ header_buf[1] = (conf->input_report_header_address >> 16) & 0xff;
+ header_buf[2] = (conf->input_report_header_address >> 8) & 0xff;
+ header_buf[3] = (conf->input_report_header_address >> 0) & 0xff;
+ header_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
+
+ body_buf[0] = conf->read_opcode;
+ body_buf[1] = (conf->input_report_body_address >> 16) & 0xff;
+ body_buf[2] = (conf->input_report_body_address >> 8) & 0xff;
+ body_buf[3] = (conf->input_report_body_address >> 0) & 0xff;
+ body_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
+}
+
+static void spi_hid_parse_dev_desc(struct spi_hid_device_desc_raw *raw,
+ struct spi_hid_device_descriptor *desc)
+{
+ desc->hid_version = le16_to_cpu(raw->bcdVersion);
+ desc->report_descriptor_length = le16_to_cpu(raw->wReportDescLength);
+ desc->max_input_length = le16_to_cpu(raw->wMaxInputLength);
+ desc->max_output_length = le16_to_cpu(raw->wMaxOutputLength);
+
+ /* FIXME: multi-fragment not supported, field below not used */
+ desc->max_fragment_length = le16_to_cpu(raw->wMaxFragmentLength);
+
+ desc->vendor_id = le16_to_cpu(raw->wVendorID);
+ desc->product_id = le16_to_cpu(raw->wProductID);
+ desc->version_id = le16_to_cpu(raw->wVersionID);
+ desc->no_output_report_ack = le16_to_cpu(raw->wFlags) & BIT(0);
+}
+
+static void spi_hid_populate_input_header(__u8 *buf,
+ struct spi_hid_input_header *header)
+{
+ header->version = buf[0] & 0xf;
+ header->report_length = (buf[1] | ((buf[2] & 0x3f) << 8)) * 4;
+ header->last_fragment_flag = (buf[2] & 0x40) >> 6;
+ header->sync_const = buf[3];
+}
+
+static void spi_hid_populate_input_body(__u8 *buf,
+ struct spi_hid_input_body *body)
+{
+ body->report_type = buf[0];
+ body->content_length = buf[1] | (buf[2] << 8);
+ body->content_id = buf[3];
+}
+
+static void spi_hid_input_report_prepare(struct spi_hid_input_buf *buf,
+ struct spi_hid_input_report *report)
+{
+ struct spi_hid_input_header header;
+ struct spi_hid_input_body body;
+
+ spi_hid_populate_input_header(buf->header, &header);
+ spi_hid_populate_input_body(buf->body, &body);
+ report->report_type = body.report_type;
+ report->content_length = body.content_length;
+ report->content_id = body.content_id;
+ report->content = buf->content;
+}
+
+static void spi_hid_populate_output_header(__u8 *buf,
+ struct spi_hid_of_config *conf,
+ struct spi_hid_output_report *report)
+{
+ buf[0] = conf->write_opcode;
+ buf[1] = (conf->output_report_address >> 16) & 0xff;
+ buf[2] = (conf->output_report_address >> 8) & 0xff;
+ buf[3] = (conf->output_report_address >> 0) & 0xff;
+ buf[4] = report->report_type;
+ buf[5] = report->content_length & 0xff;
+ buf[6] = (report->content_length >> 8) & 0xff;
+ buf[7] = report->content_id;
+}
+
+static int spi_hid_input_async(struct spi_hid *shid, void *buf, u16 length,
+ void (*complete)(void *), bool is_header)
+{
+ int ret;
+ struct device *dev = &shid->spi->dev;
+
+ 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);
+
+ shid->input_message.complete = complete;
+ shid->input_message.context = shid;
+
+ trace_spi_hid_input_async(shid,
+ shid->input_transfer[0].tx_buf,
+ shid->input_transfer[0].len,
+ shid->input_transfer[1].rx_buf,
+ shid->input_transfer[1].len, 0);
+
+ ret = spi_async(shid->spi, &shid->input_message);
+ if (ret) {
+ dev_err(dev, "Error starting async transfer: %d, resetting\n",
+ ret);
+ shid->bus_error_count++;
+ shid->bus_last_error = ret;
+ schedule_work(&shid->error_work);
+ }
+
+ return ret;
+}
+
+static int spi_hid_output(struct spi_hid *shid, void *buf, u16 length)
+{
+ struct spi_transfer transfer;
+ struct spi_message message;
+ int ret;
+
+ memset(&transfer, 0, sizeof(transfer));
+
+ transfer.tx_buf = buf;
+ transfer.len = length;
+
+ spi_message_init_with_transfers(&message, &transfer, 1);
+
+ /*
+ * REVISIT: Should output be asynchronous?
+ *
+ * According to Documentation/hid/hid-transport.rst, ->output_report()
+ * must be implemented as an asynchronous operation.
+ */
+ trace_spi_hid_output_begin(shid, transfer.tx_buf,
+ transfer.len, NULL, 0, 0);
+
+ ret = spi_sync(shid->spi, &message);
+
+ trace_spi_hid_output_end(shid, transfer.tx_buf,
+ transfer.len, NULL, 0, ret);
+
+ if (ret) {
+ shid->bus_error_count++;
+ shid->bus_last_error = ret;
+ }
+
+ return ret;
+}
+
+static const char *const spi_hid_power_mode_string(u8 power_state)
+{
+ switch (power_state) {
+ case SPI_HID_POWER_MODE_ON:
+ return "d0";
+ case SPI_HID_POWER_MODE_SLEEP:
+ return "d2";
+ case SPI_HID_POWER_MODE_OFF:
+ return "d3";
+ case SPI_HID_POWER_MODE_WAKING_SLEEP:
+ return "d3*";
+ default:
+ return "unknown";
+ }
+}
+
+static void spi_hid_suspend(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+
+ if (shid->power_state == SPI_HID_POWER_MODE_OFF)
+ return;
+
+ disable_irq(shid->spi->irq);
+ shid->ready = false;
+ sysfs_notify(&dev->kobj, NULL, "ready");
+
+ spi_hid_of_assert_reset(&shid->conf);
+
+ shid->power_state = SPI_HID_POWER_MODE_OFF;
+}
+
+static void spi_hid_resume(struct spi_hid *shid)
+{
+ if (shid->power_state == SPI_HID_POWER_MODE_ON)
+ return;
+
+ shid->power_state = SPI_HID_POWER_MODE_ON;
+ enable_irq(shid->spi->irq);
+ shid->input_transfer_pending = 0;
+
+ spi_hid_of_deassert_reset(&shid->conf);
+}
+
+static struct hid_device *spi_hid_disconnect_hid(struct spi_hid *shid)
+{
+ struct hid_device *hid = shid->hid;
+
+ shid->hid = NULL;
+
+ return hid;
+}
+
+static void spi_hid_stop_hid(struct spi_hid *shid)
+{
+ struct hid_device *hid;
+
+ hid = spi_hid_disconnect_hid(shid);
+ if (hid) {
+ cancel_work_sync(&shid->create_device_work);
+ cancel_work_sync(&shid->refresh_device_work);
+ hid_destroy_device(hid);
+ }
+}
+
+static void spi_hid_error_handler(struct spi_hid *shid)
+{
+ struct device *dev = &shid->spi->dev;
+ int ret;
+
+ if (shid->power_state == SPI_HID_POWER_MODE_OFF)
+ return;
+
+ if (shid->attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
+ dev_err(dev, "unresponsive device, aborting.\n");
+ spi_hid_stop_hid(shid);
+ spi_hid_of_assert_reset(&shid->conf);
+ ret = spi_hid_of_power_down(&shid->conf);
+ if (ret) {
+ dev_err(dev, "failed to disable regulator\n");
+ shid->regulator_error_count++;
+ shid->regulator_last_error = ret;
+ }
+ return;
+ }
+
+ trace_spi_hid_error_handler(shid);
+
+ shid->ready = false;
+ sysfs_notify(&dev->kobj, NULL, "ready");
+
+ spi_hid_of_assert_reset(&shid->conf);
+
+ shid->power_state = SPI_HID_POWER_MODE_OFF;
+ shid->input_transfer_pending = 0;
+ cancel_work_sync(&shid->reset_work);
+
+ spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
+
+ shid->power_state = SPI_HID_POWER_MODE_ON;
+
+ spi_hid_of_deassert_reset(&shid->conf);
+}
+
+static void spi_hid_error_work(struct work_struct *work)
+{
+ struct spi_hid *shid = container_of(work, struct spi_hid, error_work);
+
+ spi_hid_error_handler(shid);
+}
+
+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 ret;
+
+ if (report->content_length > shid->desc.max_output_length) {
+ dev_err(dev, "Output report too big, content_length 0x%x\n",
+ report->content_length);
+ ret = -E2BIG;
+ goto out;
+ }
+
+ 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);
+
+ ret = spi_hid_output(shid, buf, padded_length);
+ if (ret) {
+ dev_err(dev, "Failed output transfer\n");
+ goto out;
+ }
+
+ return 0;
+
+out:
+ return ret;
+}
+
+static int spi_hid_sync_request(struct spi_hid *shid,
+ struct spi_hid_output_report *report)
+{
+ struct device *dev = &shid->spi->dev;
+ int ret = 0;
+
+ ret = spi_hid_send_output_report(shid, report);
+ if (ret) {
+ dev_err(dev, "Failed to transfer output report\n");
+ return ret;
+ }
+
+ mutex_unlock(&shid->lock);
+ ret = wait_for_completion_interruptible_timeout(&shid->output_done,
+ msecs_to_jiffies(1000));
+ mutex_lock(&shid->lock);
+ if (ret == 0) {
+ dev_err(dev, "Response timed out\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+/**
+ * Handle the reset response from the FW by sending a request for the device
+ * descriptor.
+ */
+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;
+ struct spi_hid_output_report report = {
+ .report_type = SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST,
+ .content_length = 0x0,
+ .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
+ .content = NULL,
+ };
+ int ret;
+
+ trace_spi_hid_reset_work(shid);
+
+ if (shid->ready) {
+ dev_err(dev, "Spontaneous FW reset!");
+ shid->ready = false;
+ sysfs_notify(&dev->kobj, NULL, "ready");
+ shid->dir_count++;
+ }
+
+ if (shid->power_state == SPI_HID_POWER_MODE_OFF)
+ return;
+
+ if (flush_work(&shid->create_device_work))
+ dev_err(dev, "Reset handler waited for create_device_work");
+
+ if (flush_work(&shid->refresh_device_work))
+ dev_err(dev, "Reset handler waited for refresh_device_work");
+
+ mutex_lock(&shid->lock);
+ ret = spi_hid_sync_request(shid, &report);
+ mutex_unlock(&shid->lock);
+ if (ret) {
+ dev_WARN_ONCE(dev, true,
+ "Failed to send device descriptor request\n");
+ spi_hid_error_handler(shid);
+ }
+}
+
+static int spi_hid_input_report_handler(struct spi_hid *shid,
+ struct spi_hid_input_buf *buf)
+{
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_input_report r;
+ int ret;
+
+ trace_spi_hid_input_report_handler(shid);
+
+ if (!shid->ready || shid->refresh_in_progress || !shid->hid)
+ return 0;
+
+ spi_hid_input_report_prepare(buf, &r);
+
+ ret = hid_input_report(shid->hid, HID_INPUT_REPORT,
+ r.content - 1,
+ r.content_length + 1, 1);
+
+ if (ret == -ENODEV || ret == -EBUSY) {
+ dev_err(dev, "ignoring report --> %d\n", ret);
+ return 0;
+ } else if (ret) {
+ dev_err(dev, "Bad input report, error %d\n", ret);
+ }
+
+ return ret;
+}
+
+static void spi_hid_response_handler(struct spi_hid *shid,
+ struct spi_hid_input_buf *buf)
+{
+ trace_spi_hid_response_handler(shid);
+
+ /* completion_done returns 0 if there are waiters, otherwise 1 */
+ if (completion_done(&shid->output_done)) {
+ dev_err(&shid->spi->dev, "Unexpected response report\n");
+ } else {
+ if (shid->input.body[0] ==
+ SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC ||
+ shid->input.body[0] ==
+ SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP) {
+ size_t response_length = (shid->input.body[1] |
+ (shid->input.body[2] << 8)) +
+ sizeof(shid->input.body);
+ memcpy(shid->response.body, shid->input.body,
+ response_length);
+ }
+ complete(&shid->output_done);
+ }
+}
+
+/*
+ * 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)
+{
+ int ret;
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_output_report report = {
+ .report_type = SPI_HID_OUTPUT_REPORT_TYPE_REPORT_DESC_REQUEST,
+ .content_length = 0,
+ .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
+ .content = NULL,
+ };
+
+ ret = spi_hid_sync_request(shid, &report);
+ if (ret) {
+ dev_err(dev,
+ "Expected report descriptor not received! Error %d\n",
+ ret);
+ spi_hid_error_handler(shid);
+ goto out;
+ }
+
+ ret = (shid->response.body[1] | (shid->response.body[2] << 8));
+ if (ret != shid->desc.report_descriptor_length) {
+ dev_err(dev,
+ "Received report descriptor length doesn't match device descriptor field, using min of the two\n");
+ ret = min_t(unsigned int, ret,
+ shid->desc.report_descriptor_length);
+ }
+out:
+ return ret;
+}
+
+static void 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_body body;
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_device_desc_raw *raw;
+ int ret;
+
+ trace_spi_hid_process_input_report(shid);
+
+ spi_hid_populate_input_header(buf->header, &header);
+ spi_hid_populate_input_body(buf->body, &body);
+
+ if (body.content_length > header.report_length) {
+ dev_err(dev, "Bad body length %d > %d\n", body.content_length,
+ header.report_length);
+ schedule_work(&shid->error_work);
+ return;
+ }
+
+ switch (body.report_type) {
+ case SPI_HID_INPUT_REPORT_TYPE_DATA:
+ ret = spi_hid_input_report_handler(shid, buf);
+ if (ret)
+ schedule_work(&shid->error_work);
+ break;
+ case SPI_HID_INPUT_REPORT_TYPE_RESET_RESP:
+ schedule_work(&shid->reset_work);
+ break;
+ case SPI_HID_INPUT_REPORT_TYPE_DEVICE_DESC:
+ /* Mark the completion done to avoid timeout */
+ spi_hid_response_handler(shid, buf);
+
+ /* Reset attempts at every device descriptor fetch */
+ shid->attempts = 0;
+
+ raw = (struct spi_hid_device_desc_raw *)buf->content;
+
+ /* Validate device descriptor length before parsing */
+ if (body.content_length != SPI_HID_DEVICE_DESCRIPTOR_LENGTH) {
+ dev_err(dev,
+ "Invalid content length %d, expected %d\n",
+ body.content_length,
+ SPI_HID_DEVICE_DESCRIPTOR_LENGTH);
+ schedule_work(&shid->error_work);
+ break;
+ }
+
+ if (le16_to_cpu(raw->wDeviceDescLength) !=
+ SPI_HID_DEVICE_DESCRIPTOR_LENGTH) {
+ dev_err(dev,
+ "Invalid wDeviceDescLength %d, expected %d\n",
+ raw->wDeviceDescLength,
+ SPI_HID_DEVICE_DESCRIPTOR_LENGTH);
+ schedule_work(&shid->error_work);
+ break;
+ }
+
+ spi_hid_parse_dev_desc(raw, &shid->desc);
+
+ if (shid->desc.hid_version != SPI_HID_SUPPORTED_VERSION) {
+ dev_err(dev,
+ "Unsupported device descriptor version %4x\n",
+ shid->desc.hid_version);
+ schedule_work(&shid->error_work);
+ break;
+ }
+
+ if (!shid->hid)
+ schedule_work(&shid->create_device_work);
+ else
+ schedule_work(&shid->refresh_device_work);
+
+ break;
+ case SPI_HID_INPUT_REPORT_TYPE_SET_OUTPUT_REPORT_RESP:
+ if (shid->desc.no_output_report_ack) {
+ dev_err(dev, "Unexpected output report response\n");
+ break;
+ }
+ fallthrough;
+ case SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP:
+ case SPI_HID_INPUT_REPORT_TYPE_SET_FEATURE_RESP:
+ if (!shid->ready) {
+ dev_err(dev,
+ "Unexpected response report while not ready: 0x%x\n",
+ body.report_type);
+ break;
+ }
+ fallthrough;
+ case SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC:
+ spi_hid_response_handler(shid, buf);
+ break;
+ /*
+ * FIXME: sending GET_INPUT and COMMAND reports not supported, thus
+ * throw away responses to those, they should never come.
+ */
+ case SPI_HID_INPUT_REPORT_TYPE_GET_INPUT_REPORT_RESP:
+ case SPI_HID_INPUT_REPORT_TYPE_COMMAND_RESP:
+ 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);
+ schedule_work(&shid->error_work);
+ break;
+ }
+}
+
+static int spi_hid_bus_validate_header(struct spi_hid *shid,
+ struct spi_hid_input_header *header)
+{
+ struct device *dev = &shid->spi->dev;
+
+ if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
+ dev_err(dev, "Unknown input report version (v 0x%x)\n",
+ header->version);
+ return -EINVAL;
+ }
+
+ if (shid->desc.max_input_length != 0 &&
+ header->report_length > shid->desc.max_input_length) {
+ dev_err(dev, "Input report body size %u > max expected of %u\n",
+ header->report_length,
+ shid->desc.max_input_length);
+ return -EMSGSIZE;
+ }
+
+ if (header->last_fragment_flag != 1) {
+ dev_err(dev, "Multi-fragment reports not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (header->sync_const != SPI_HID_INPUT_HEADER_SYNC_BYTE) {
+ dev_err(dev, "Invalid input report sync constant (0x%x)\n",
+ header->sync_const);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int spi_hid_create_device(struct spi_hid *shid)
+{
+ struct hid_device *hid;
+ struct device *dev = &shid->spi->dev;
+ int ret;
+
+ hid = hid_allocate_device();
+
+ if (IS_ERR(hid)) {
+ dev_err(dev, "Failed to allocate hid device: %ld\n",
+ PTR_ERR(hid));
+ ret = PTR_ERR(hid);
+ return ret;
+ }
+
+ 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 %04hX:%04hX",
+ hid->vendor, hid->product);
+ strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
+
+ shid->hid = hid;
+
+ ret = hid_add_device(hid);
+ if (ret) {
+ dev_err(dev, "Failed to add hid device: %d\n", ret);
+ /*
+ * We likely got here because report descriptor request timed
+ * out. Let's disconnect and destroy the hid_device structure.
+ */
+ hid = spi_hid_disconnect_hid(shid);
+ if (hid)
+ hid_destroy_device(hid);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void spi_hid_create_device_work(struct work_struct *work)
+{
+ struct spi_hid *shid =
+ container_of(work, struct spi_hid, create_device_work);
+ struct device *dev = &shid->spi->dev;
+ u8 prev_state = shid->power_state;
+ int ret;
+
+ trace_spi_hid_create_device_work(shid);
+
+ ret = spi_hid_create_device(shid);
+ if (ret) {
+ dev_err(dev, "Failed to create hid device\n");
+ return;
+ }
+
+ spi_hid_suspend(shid);
+
+ shid->attempts = 0;
+
+ dev_dbg(dev, "%s: %s -> %s\n", __func__,
+ spi_hid_power_mode_string(prev_state),
+ spi_hid_power_mode_string(shid->power_state));
+}
+
+static void spi_hid_refresh_device_work(struct work_struct *work)
+{
+ struct spi_hid *shid =
+ container_of(work, struct spi_hid, refresh_device_work);
+ struct device *dev = &shid->spi->dev;
+ struct hid_device *hid;
+ int ret;
+ u32 new_crc32;
+
+ trace_spi_hid_refresh_device_work(shid);
+
+ mutex_lock(&shid->lock);
+ ret = spi_hid_report_descriptor_request(shid);
+ mutex_unlock(&shid->lock);
+ if (ret < 0) {
+ dev_err(dev,
+ "Refresh: failed report descriptor request, error %d",
+ ret);
+ return;
+ }
+
+ new_crc32 = crc32_le(0, (unsigned char const *)shid->response.content,
+ (size_t)ret);
+ if (new_crc32 == shid->report_descriptor_crc32) {
+ shid->ready = true;
+ sysfs_notify(&dev->kobj, NULL, "ready");
+ return;
+ }
+
+ shid->report_descriptor_crc32 = new_crc32;
+ shid->refresh_in_progress = true;
+
+ hid = spi_hid_disconnect_hid(shid);
+ if (hid)
+ hid_destroy_device(hid);
+
+ ret = spi_hid_create_device(shid);
+ if (ret)
+ dev_err(dev, "Failed to create hid device\n");
+
+ shid->refresh_in_progress = false;
+ shid->ready = true;
+ sysfs_notify(&dev->kobj, NULL, "ready");
+}
+
+static void spi_hid_input_header_complete(void *_shid);
+
+static void spi_hid_input_body_complete(void *_shid)
+{
+ struct spi_hid *shid = _shid;
+ struct device *dev = &shid->spi->dev;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&shid->input_lock, flags);
+
+ if (shid->power_state == SPI_HID_POWER_MODE_OFF) {
+ dev_warn(dev,
+ "input body complete called while device is off\n");
+ goto out;
+ }
+
+ 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, resetting %d\n",
+ shid->input_message.status);
+ shid->bus_error_count++;
+ shid->bus_last_error = shid->input_message.status;
+ schedule_work(&shid->error_work);
+ goto out;
+ }
+
+ spi_hid_process_input_report(shid, &shid->input);
+
+ if (--shid->input_transfer_pending) {
+ struct spi_hid_input_buf *buf = &shid->input;
+
+ trace_spi_hid_header_transfer(shid);
+ ret = spi_hid_input_async(shid, buf->header,
+ sizeof(buf->header),
+ spi_hid_input_header_complete, true);
+ if (ret)
+ dev_err(dev, "failed to start header transfer %d\n",
+ ret);
+ }
+
+out:
+ spin_unlock_irqrestore(&shid->input_lock, flags);
+}
+
+static void spi_hid_input_header_complete(void *_shid)
+{
+ struct spi_hid *shid = _shid;
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_input_header header;
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&shid->input_lock, flags);
+
+ if (shid->power_state == SPI_HID_POWER_MODE_OFF) {
+ dev_warn(dev,
+ "input header complete called while device is off\n");
+ goto out;
+ }
+
+ 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, resetting, error %d\n",
+ shid->input_message.status);
+ shid->bus_error_count++;
+ shid->bus_last_error = shid->input_message.status;
+ schedule_work(&shid->error_work);
+ goto out;
+ }
+ spi_hid_populate_input_header(shid->input.header, &header);
+
+ ret = spi_hid_bus_validate_header(shid, &header);
+ if (ret) {
+ dev_err(dev, "failed to validate header: %d\n", ret);
+ print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
+ DUMP_PREFIX_NONE, 16, 1,
+ shid->input.header,
+ sizeof(shid->input.header),
+ false);
+ shid->bus_error_count++;
+ shid->bus_last_error = ret;
+ goto out;
+ }
+
+ ret = spi_hid_input_async(shid, shid->input.body, header.report_length,
+ spi_hid_input_body_complete, false);
+ if (ret)
+ dev_err(dev, "failed body async transfer: %d\n", ret);
+
+out:
+ if (ret)
+ shid->input_transfer_pending = 0;
+
+ spin_unlock_irqrestore(&shid->input_lock, flags);
+}
+
+static int spi_hid_get_request(struct spi_hid *shid, u8 content_id)
+{
+ int ret;
+ struct device *dev = &shid->spi->dev;
+ struct spi_hid_output_report report = {
+ .report_type = SPI_HID_OUTPUT_REPORT_TYPE_HID_GET_FEATURE,
+ .content_length = 0,
+ .content_id = content_id,
+ .content = NULL,
+ };
+
+ ret = spi_hid_sync_request(shid, &report);
+ if (ret) {
+ dev_err(dev,
+ "Expected get request response not received! Error %d\n",
+ ret);
+ schedule_work(&shid->error_work);
+ }
+
+ return ret;
+}
+
+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 = SPI_HID_OUTPUT_REPORT_TYPE_HID_SET_FEATURE,
+ .content_length = arg_len,
+ .content_id = content_id,
+ .content = arg_buf,
+ };
+
+ return spi_hid_sync_request(shid, &report);
+}
+
+static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
+{
+ struct spi_hid *shid = _shid;
+ struct device *dev = &shid->spi->dev;
+ int ret = 0;
+
+ spin_lock(&shid->input_lock);
+ trace_spi_hid_dev_irq(shid, irq);
+
+ if (shid->input_transfer_pending++)
+ goto out;
+
+ trace_spi_hid_header_transfer(shid);
+ ret = spi_hid_input_async(shid, shid->input.header,
+ sizeof(shid->input.header),
+ spi_hid_input_header_complete, true);
+ if (ret)
+ dev_err(dev, "Failed to start header transfer: %d\n", ret);
+
+out:
+ spin_unlock(&shid->input_lock);
+
+ return IRQ_HANDLED;
+}
+
+/* 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);
+
+ if (shid->desc.max_input_length < HID_MIN_BUFFER_SIZE) {
+ dev_err(&shid->spi->dev,
+ "HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
+ shid->desc.max_input_length);
+ return -EINVAL;
+ }
+
+ 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)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ u8 prev_state = shid->power_state;
+
+ if (shid->refresh_in_progress)
+ return 0;
+
+ spi_hid_resume(shid);
+
+ dev_dbg(dev, "%s: %s -> %s\n", __func__,
+ spi_hid_power_mode_string(prev_state),
+ spi_hid_power_mode_string(shid->power_state));
+
+ 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);
+ struct device *dev = &spi->dev;
+ u8 prev_state = shid->power_state;
+
+ if (shid->refresh_in_progress)
+ return;
+
+ spi_hid_suspend(shid);
+
+ shid->attempts = 0;
+
+ dev_dbg(dev, "%s: %s -> %s\n", __func__,
+ spi_hid_power_mode_string(prev_state),
+ spi_hid_power_mode_string(shid->power_state));
+}
+
+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 ret = 0;
+
+ mutex_lock(&shid->lock);
+ if (!shid->hid)
+ ret = -ENODEV;
+ mutex_unlock(&shid->lock);
+
+ return ret;
+}
+
+static int spi_hid_ll_parse(struct hid_device *hid)
+{
+ struct spi_device *spi = hid->driver_data;
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ int ret, len;
+
+ mutex_lock(&shid->lock);
+
+ len = spi_hid_report_descriptor_request(shid);
+ if (len < 0) {
+ dev_err(dev, "Report descriptor request failed, %d\n", len);
+ ret = len;
+ goto out;
+ }
+
+ /*
+ * 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.
+ */
+ ret = hid_parse_report(hid, (__u8 *)shid->response.content, len);
+ if (ret)
+ dev_err(dev, "failed parsing report: %d\n", ret);
+ else
+ shid->report_descriptor_crc32 = crc32_le(0,
+ (unsigned char const *)shid->response.content,
+ len);
+
+out:
+ mutex_unlock(&shid->lock);
+
+ return ret;
+}
+
+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;
+
+ if (!shid->ready) {
+ dev_err(&shid->spi->dev, "%s called in unready state\n",
+ __func__);
+ return -ENODEV;
+ }
+
+ mutex_lock(&shid->lock);
+
+ switch (reqtype) {
+ case HID_REQ_SET_REPORT:
+ if (buf[0] != reportnum) {
+ dev_err(dev, "report id mismatch\n");
+ ret = -EINVAL;
+ break;
+ }
+
+ ret = spi_hid_set_request(shid, &buf[1], len - 1,
+ reportnum);
+ if (ret) {
+ dev_err(dev, "failed to set report\n");
+ break;
+ }
+
+ ret = len;
+ break;
+ case HID_REQ_GET_REPORT:
+ ret = spi_hid_get_request(shid, reportnum);
+ if (ret) {
+ dev_err(dev, "failed to get report\n");
+ break;
+ }
+
+ ret = min_t(size_t, len,
+ shid->response.body[1] | (shid->response.body[2] << 8));
+ memcpy(buf, &shid->response.content, ret);
+ break;
+ default:
+ dev_err(dev, "invalid request type\n");
+ ret = -EIO;
+ }
+
+ mutex_unlock(&shid->lock);
+
+ return ret;
+}
+
+static int spi_hid_ll_output_report(struct hid_device *hid,
+ __u8 *buf, size_t len)
+{
+ int ret;
+ 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 = SPI_HID_OUTPUT_REPORT_TYPE_HID_OUTPUT_REPORT,
+ .content_length = len - 1,
+ .content_id = buf[0],
+ .content = &buf[1],
+ };
+
+ mutex_lock(&shid->lock);
+ if (!shid->ready) {
+ dev_err(dev, "%s called in unready state\n", __func__);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ if (shid->desc.no_output_report_ack)
+ ret = spi_hid_send_output_report(shid, &report);
+ else
+ ret = spi_hid_sync_request(shid, &report);
+
+ if (ret)
+ dev_err(dev, "failed to send output report\n");
+
+out:
+ mutex_unlock(&shid->lock);
+
+ if (ret > 0)
+ return -ret;
+
+ if (ret < 0)
+ return ret;
+
+ return len;
+}
+
+static struct hid_ll_driver spi_hid_ll_driver = {
+ .start = spi_hid_ll_start,
+ .stop = spi_hid_ll_stop,
+ .open = spi_hid_ll_open,
+ .close = spi_hid_ll_close,
+ .power = spi_hid_ll_power,
+ .parse = spi_hid_ll_parse,
+ .output_report = spi_hid_ll_output_report,
+ .raw_request = spi_hid_ll_raw_request,
+};
+
+static ssize_t ready_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%s\n",
+ shid->ready ? "ready" : "not ready");
+}
+static DEVICE_ATTR_RO(ready);
+
+static ssize_t bus_error_count_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d (%d)\n",
+ shid->bus_error_count, shid->bus_last_error);
+}
+static DEVICE_ATTR_RO(bus_error_count);
+
+static ssize_t regulator_error_count_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d (%d)\n",
+ shid->regulator_error_count,
+ shid->regulator_last_error);
+}
+static DEVICE_ATTR_RO(regulator_error_count);
+
+static ssize_t device_initiated_reset_count_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct spi_hid *shid = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", shid->dir_count);
+}
+static DEVICE_ATTR_RO(device_initiated_reset_count);
+
+static const struct attribute *const spi_hid_attributes[] = {
+ &dev_attr_ready.attr,
+ &dev_attr_bus_error_count.attr,
+ &dev_attr_regulator_error_count.attr,
+ &dev_attr_device_initiated_reset_count.attr,
+ NULL /* Terminator */
+};
+
+static int spi_hid_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct spi_hid *shid;
+ unsigned long irqflags;
+ int ret;
+
+ if (spi->irq <= 0) {
+ dev_err(dev, "Missing IRQ\n");
+ ret = spi->irq ?: -EINVAL;
+ goto err0;
+ }
+
+ shid = devm_kzalloc(dev, sizeof(struct spi_hid), GFP_KERNEL);
+ if (!shid) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ shid->spi = spi;
+ shid->power_state = SPI_HID_POWER_MODE_ON;
+ spi_set_drvdata(spi, shid);
+
+ ret = sysfs_create_files(&dev->kobj, spi_hid_attributes);
+ if (ret) {
+ dev_err(dev, "Unable to create sysfs attributes\n");
+ goto err0;
+ }
+
+ ret = spi_hid_of_populate_config(&shid->conf, dev);
+
+ /* 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->lock);
+ init_completion(&shid->output_done);
+
+ spin_lock_init(&shid->input_lock);
+ INIT_WORK(&shid->reset_work, spi_hid_reset_work);
+ INIT_WORK(&shid->create_device_work, spi_hid_create_device_work);
+ INIT_WORK(&shid->refresh_device_work, spi_hid_refresh_device_work);
+ INIT_WORK(&shid->error_work, spi_hid_error_work);
+
+ /*
+ * At the end of probe we initialize the device:
+ * 0) Default pinctrl in DT: assert reset, bias the interrupt line
+ * 1) sleep minimal reset delay
+ * 2) request IRQ
+ * 3) power up the device
+ * 4) sleep 5ms
+ * 5) deassert reset (high)
+ * 6) sleep 5ms
+ */
+
+ spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
+
+ irqflags = irq_get_trigger_type(spi->irq) | IRQF_ONESHOT;
+ ret = request_irq(spi->irq, spi_hid_dev_irq, irqflags,
+ dev_name(&spi->dev), shid);
+ if (ret)
+ goto err1;
+
+ ret = spi_hid_of_power_up(&shid->conf);
+ if (ret) {
+ dev_err(dev, "%s: could not power up\n", __func__);
+ shid->regulator_error_count++;
+ shid->regulator_last_error = ret;
+ goto err1;
+ }
+
+ spi_hid_of_deassert_reset(&shid->conf);
+
+ dev_err(dev, "%s: d3 -> %s\n", __func__,
+ spi_hid_power_mode_string(shid->power_state));
+
+ return 0;
+
+err1:
+ sysfs_remove_files(&dev->kobj, spi_hid_attributes);
+
+err0:
+ return ret;
+}
+
+static int spi_hid_remove(struct spi_device *spi)
+{
+ struct spi_hid *shid = spi_get_drvdata(spi);
+ struct device *dev = &spi->dev;
+ int ret;
+
+ spi_hid_of_assert_reset(&shid->conf);
+ ret = spi_hid_of_power_down(&shid->conf);
+ if (ret) {
+ dev_err(dev, "failed to disable regulator\n");
+ shid->regulator_error_count++;
+ shid->regulator_last_error = ret;
+ }
+ free_irq(spi->irq, shid);
+ sysfs_remove_files(&dev->kobj, spi_hid_attributes);
+ spi_hid_stop_hid(shid);
+
+ return 0;
+}
+
+static const struct spi_device_id spi_hid_id_table[] = {
+ { "hid", 0 },
+ { "hid-over-spi", 0 },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, spi_hid_id_table);
+
+static struct spi_driver spi_hid_driver = {
+ .driver = {
+ .name = "spi_hid",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(spi_hid_of_match),
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .probe = spi_hid_probe,
+ .remove = spi_hid_remove,
+ .id_table = spi_hid_id_table,
+};
+
+module_spi_driver(spi_hid_driver);
+
+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
new file mode 100644
index 000000000000..95e667ad53ec
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-core.h
@@ -0,0 +1,188 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * spi-hid-core.h
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ */
+
+#ifndef SPI_HID_CORE_H
+#define SPI_HID_CORE_H
+
+#include <linux/completion.h>
+#include <linux/kernel.h>
+#include <linux/spi/spi.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include "spi-hid-of.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
+
+/* Protocol message size constants */
+#define SPI_HID_READ_APPROVAL_LEN 5
+#define SPI_HID_INPUT_HEADER_LEN 4
+#define SPI_HID_INPUT_BODY_LEN 4
+#define SPI_HID_OUTPUT_HEADER_LEN 8
+#define SPI_HID_DEVICE_DESCRIPTOR_LENGTH 24
+
+/* Protocol message type constants */
+#define SPI_HID_INPUT_REPORT_TYPE_DATA 0x01
+#define SPI_HID_INPUT_REPORT_TYPE_RESET_RESP 0x03
+#define SPI_HID_INPUT_REPORT_TYPE_COMMAND_RESP 0x04
+#define SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP 0x05
+#define SPI_HID_INPUT_REPORT_TYPE_DEVICE_DESC 0x07
+#define SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC 0x08
+#define SPI_HID_INPUT_REPORT_TYPE_SET_FEATURE_RESP 0x09
+#define SPI_HID_INPUT_REPORT_TYPE_SET_OUTPUT_REPORT_RESP 0x0a
+#define SPI_HID_INPUT_REPORT_TYPE_GET_INPUT_REPORT_RESP 0x0b
+
+#define SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST 0x01
+#define SPI_HID_OUTPUT_REPORT_TYPE_REPORT_DESC_REQUEST 0x02
+#define SPI_HID_OUTPUT_REPORT_TYPE_HID_SET_FEATURE 0x03
+#define SPI_HID_OUTPUT_REPORT_TYPE_HID_GET_FEATURE 0x04
+#define SPI_HID_OUTPUT_REPORT_TYPE_HID_OUTPUT_REPORT 0x05
+#define SPI_HID_OUTPUT_REPORT_TYPE_INPUT_REPORT_REQUEST 0x06
+#define SPI_HID_OUTPUT_REPORT_TYPE_COMMAND 0x07
+
+#define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST 0x00
+
+/* Power mode constants */
+#define SPI_HID_POWER_MODE_ON 0x01
+#define SPI_HID_POWER_MODE_SLEEP 0x02
+#define SPI_HID_POWER_MODE_OFF 0x03
+#define SPI_HID_POWER_MODE_WAKING_SLEEP 0x04
+
+/* Raw input buffer with data from the bus */
+struct spi_hid_input_buf {
+ __u8 header[SPI_HID_INPUT_HEADER_LEN];
+ __u8 body[SPI_HID_INPUT_BODY_LEN];
+ u8 content[SZ_8K];
+};
+
+/* 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 input report body, excluding the content */
+struct spi_hid_input_body {
+ u8 report_type;
+ u16 content_length;
+ u8 content_id;
+};
+
+/* 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];
+ u8 content[SZ_8K];
+};
+
+/* Data necessary to send an output report */
+struct spi_hid_output_report {
+ u8 report_type;
+ u16 content_length;
+ u8 content_id;
+ u8 *content;
+};
+
+/* Raw content in device descriptor */
+struct spi_hid_device_desc_raw {
+ __le16 wDeviceDescLength;
+ __le16 bcdVersion;
+ __le16 wReportDescLength;
+ __le16 wMaxInputLength;
+ __le16 wMaxOutputLength;
+ __le16 wMaxFragmentLength;
+ __le16 wVendorID;
+ __le16 wProductID;
+ __le16 wVersionID;
+ __le16 wFlags;
+ __u8 reserved[4];
+} __packed;
+
+/* 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;
+ struct hid_device *hid;
+
+ struct spi_transfer input_transfer[2];
+ struct spi_transfer output_transfer;
+ struct spi_message input_message;
+ struct spi_message output_message;
+
+ struct spi_hid_of_config conf;
+ struct spi_hid_device_descriptor desc;
+ struct spi_hid_output_buf output;
+ struct spi_hid_input_buf input;
+ struct spi_hid_input_buf response;
+
+ spinlock_t input_lock;
+
+ u32 input_transfer_pending;
+
+ u8 power_state;
+
+ u8 attempts;
+
+ /*
+ * ready flag indicates that the FW is ready to accept commands and
+ * requests. The FW becomes ready after sending the report descriptor.
+ */
+ bool ready;
+ /*
+ * refresh_in_progress is set to true while the refresh_device worker
+ * thread is destroying and recreating the hidraw device. When this flag
+ * is set to true, the ll_close and ll_open functions will not cause
+ * power state changes.
+ */
+ bool refresh_in_progress;
+
+ struct work_struct reset_work;
+ struct work_struct create_device_work;
+ struct work_struct refresh_device_work;
+ struct work_struct error_work;
+
+ struct mutex lock;
+ struct completion output_done;
+
+ __u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN];
+ __u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
+
+ u32 report_descriptor_crc32;
+
+ u32 regulator_error_count;
+ int regulator_last_error;
+ u32 bus_error_count;
+ int bus_last_error;
+ u32 dir_count;
+};
+
+#endif
diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
new file mode 100755
index 000000000000..4896a90c2e8e
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-of.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID over SPI protocol, Open Firmware related code
+ * spi-hid-of.c
+ *
+ * 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/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+
+#include "spi-hid-core.h"
+
+const struct of_device_id spi_hid_of_match[] = {
+ { .compatible = "hid-over-spi" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, spi_hid_of_match);
+
+int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
+ struct device *dev)
+{
+ int ret;
+ u32 val;
+
+ ret = device_property_read_u32(dev, "input-report-header-address",
+ &val);
+ if (ret) {
+ dev_err(dev, "Input report header address not provided\n");
+ return -ENODEV;
+ }
+ conf->input_report_header_address = val;
+
+ ret = device_property_read_u32(dev, "input-report-body-address", &val);
+ if (ret) {
+ dev_err(dev, "Input report body address not provided\n");
+ return -ENODEV;
+ }
+ conf->input_report_body_address = val;
+
+ ret = device_property_read_u32(dev, "output-report-address", &val);
+ if (ret) {
+ dev_err(dev, "Output report address not provided\n");
+ return -ENODEV;
+ }
+ conf->output_report_address = val;
+
+ ret = device_property_read_u32(dev, "read-opcode", &val);
+ if (ret) {
+ dev_err(dev, "Read opcode not provided\n");
+ return -ENODEV;
+ }
+ conf->read_opcode = val;
+
+ ret = device_property_read_u32(dev, "write-opcode", &val);
+ if (ret) {
+ dev_err(dev, "Write opcode not provided\n");
+ return -ENODEV;
+ }
+ conf->write_opcode = val;
+
+ ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);
+ if (ret) {
+ dev_err(dev, "Post-power-on delay not provided\n");
+ return -ENODEV;
+ }
+ conf->post_power_on_delay_ms = val;
+
+ ret = device_property_read_u32(dev, "minimal-reset-delay-ms", &val);
+ if (ret) {
+ dev_err(dev, "Minimal reset time not provided\n");
+ return -ENODEV;
+ }
+ conf->minimal_reset_delay_ms = val;
+
+ /* FIXME: not reading flags from DT, multi-SPI modes not supported */
+
+ conf->supply = devm_regulator_get(dev, "vdd");
+ if (IS_ERR(conf->supply)) {
+ if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
+ dev_err(dev, "Failed to get regulator: %ld\n",
+ PTR_ERR(conf->supply));
+ return PTR_ERR(conf->supply);
+ }
+
+ conf->reset_gpio = devm_gpiod_get(dev, "reset-gpio", GPIOD_OUT_LOW);
+ if (IS_ERR(conf->reset_gpio)) {
+ dev_err(dev, "%s: error getting GPIO\n", __func__);
+ return PTR_ERR(conf->reset_gpio);
+ }
+
+ return 0;
+}
+
+int spi_hid_of_power_down(struct spi_hid_of_config *conf)
+{
+ if (regulator_is_enabled(conf->supply) == 0)
+ return 0;
+
+ return regulator_disable(conf->supply);
+}
+
+int spi_hid_of_power_up(struct spi_hid_of_config *conf)
+{
+ int ret;
+
+ if (regulator_is_enabled(conf->supply) > 0)
+ return 0;
+
+ ret = regulator_enable(conf->supply);
+
+ usleep_range(1000 * conf->post_power_on_delay_ms,
+ 1000 * (conf->post_power_on_delay_ms + 1));
+
+ return ret;
+}
+
+void spi_hid_of_assert_reset(struct spi_hid_of_config *conf)
+{
+ gpiod_set_value(conf->reset_gpio, 1);
+}
+
+void spi_hid_of_deassert_reset(struct spi_hid_of_config *conf)
+{
+ gpiod_set_value(conf->reset_gpio, 0);
+}
+
+void spi_hid_of_sleep_minimal_reset_delay(struct spi_hid_of_config *conf)
+{
+ usleep_range(1000 * conf->minimal_reset_delay_ms,
+ 1000 * (conf->minimal_reset_delay_ms + 1));
+}
\ No newline at end of file
diff --git a/drivers/hid/spi-hid/spi-hid-of.h b/drivers/hid/spi-hid/spi-hid-of.h
new file mode 100755
index 000000000000..5991011d8921
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid-of.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * spi-hid-of.h
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ */
+
+#ifndef SPI_HID_OF_H
+#define SPI_HID_OF_H
+
+extern const struct of_device_id spi_hid_of_match[];
+
+/* Config structure is filled with data from Device Tree */
+struct spi_hid_of_config {
+ u32 input_report_header_address;
+ u32 input_report_body_address;
+ u32 output_report_address;
+ u8 read_opcode;
+ u8 write_opcode;
+ u32 post_power_on_delay_ms;
+ u32 minimal_reset_delay_ms;
+ struct gpio_desc *reset_gpio;
+ struct regulator *supply;
+};
+
+int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
+ struct device *dev);
+int spi_hid_of_power_down(struct spi_hid_of_config *conf);
+int spi_hid_of_power_up(struct spi_hid_of_config *conf);
+void spi_hid_of_assert_reset(struct spi_hid_of_config *conf);
+void spi_hid_of_deassert_reset(struct spi_hid_of_config *conf);
+void spi_hid_of_sleep_minimal_reset_delay(struct spi_hid_of_config *conf);
+
+#endif
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..905de02d85aa
--- /dev/null
+++ b/drivers/hid/spi-hid/spi-hid_trace.h
@@ -0,0 +1,198 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * spi-hid_trace.h
+ *
+ * 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"
+
+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, len)
+ __field(int, ret)
+ __dynamic_array(u8, rx_buf, rx_len)
+ __dynamic_array(u8, tx_buf, tx_len)
+ ),
+
+ TP_fast_assign(
+ __entry->bus_num = shid->spi->controller->bus_num;
+ __entry->chip_select = shid->spi->chip_select;
+ __entry->len = rx_len + tx_len;
+ __entry->ret = ret;
+
+ memcpy(__get_dynamic_array(tx_buf), tx_buf, tx_len);
+ memcpy(__get_dynamic_array(rx_buf), rx_buf, rx_len);
+ ),
+
+ TP_printk("spi%d.%d: len=%d tx=[%*phD] rx=[%*phD] --> %d",
+ __entry->bus_num, __entry->chip_select, __entry->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_async,
+ 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)
+);
+
+DEFINE_EVENT(spi_hid_transfer, spi_hid_output_begin,
+ 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_output_end,
+ 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 = shid->spi->chip_select;
+ __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, input_transfer_pending)
+ __field(bool, ready)
+
+ __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 = shid->spi->chip_select;
+ __entry->power_state = shid->power_state;
+ __entry->input_transfer_pending = shid->input_transfer_pending;
+ __entry->ready = shid->ready;
+
+ __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 i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
+ __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->ready ? 'R' : 'r', __entry->input_transfer_pending)
+);
+
+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_work,
+ TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid)
+);
+
+DEFINE_EVENT(spi_hid, spi_hid_create_device_work,
+ TP_PROTO(struct spi_hid *shid),
+ TP_ARGS(shid)
+);
+
+DEFINE_EVENT(spi_hid, spi_hid_refresh_device_work,
+ 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 */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#define TRACE_INCLUDE_FILE spi-hid_trace
+#include <trace/define_trace.h>
diff --git a/drivers/hid/spi-hid/trace.c b/drivers/hid/spi-hid/trace.c
new file mode 100644
index 000000000000..aaa2da0c9642
--- /dev/null
+++ b/drivers/hid/spi-hid/trace.c
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * trace.c - SPI HID Trace Support
+ *
+ * Copyright (c) 2021 Microsoft Corporation
+ *
+ * Author: Felipe Balbi <felipe.balbi@microsoft.com>
+ */
+
+#define CREATE_TRACE_POINTS
+#include "spi-hid_trace.h"
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus
2022-01-15 2:31 ` [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus Dmitry Antipov
@ 2022-01-15 4:32 ` Dmitry Torokhov
2022-02-25 1:20 ` [EXTERNAL] " Dmitry Antipov
2022-01-15 6:10 ` kernel test robot
1 sibling, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2022-01-15 4:32 UTC (permalink / raw)
To: Dmitry Antipov
Cc: Jiri Kosina, Benjamin Tissoires, Rob Herring, Mark Brown,
Felipe Balbi, linux-input, devicetree, linux-spi, Dmitry Antipov
Hi Dmitry,
On Fri, Jan 14, 2022 at 06:31:35PM -0800, Dmitry Antipov wrote:
> From: Dmitry Antipov <dmanti@microsoft.com>
>
> 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.
The driver does not use threaded interrupts and instead uses a lot of
work structs, which makes it very complex. Please consider switching
over to threaded interrupt handler and see if some of the work can be
done in-place, I believe it will simplify the driver significantly.
More random comments below.
>
> Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> ---
> arch/arm64/configs/defconfig | 1 +
> drivers/hid/Kconfig | 2 +
> drivers/hid/Makefile | 1 +
> drivers/hid/spi-hid/Kconfig | 25 +
> drivers/hid/spi-hid/Makefile | 12 +
> drivers/hid/spi-hid/spi-hid-core.c | 1328 +++++++++++++++++++++++++++
> drivers/hid/spi-hid/spi-hid-core.h | 188 ++++
> drivers/hid/spi-hid/spi-hid-of.c | 148 +++
> drivers/hid/spi-hid/spi-hid-of.h | 34 +
> drivers/hid/spi-hid/spi-hid_trace.h | 198 ++++
> drivers/hid/spi-hid/trace.c | 11 +
> 11 files changed, 1948 insertions(+)
> create mode 100644 drivers/hid/spi-hid/Kconfig
> create mode 100644 drivers/hid/spi-hid/Makefile
> create mode 100644 drivers/hid/spi-hid/spi-hid-core.c
> create mode 100644 drivers/hid/spi-hid/spi-hid-core.h
> create mode 100755 drivers/hid/spi-hid/spi-hid-of.c
> create mode 100755 drivers/hid/spi-hid/spi-hid-of.h
> create mode 100644 drivers/hid/spi-hid/spi-hid_trace.h
> create mode 100644 drivers/hid/spi-hid/trace.c
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index f2e2b9bdd702..25249a4b0c8a 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -805,6 +805,7 @@ CONFIG_SND_AUDIO_GRAPH_CARD=m
> CONFIG_HID_MULTITOUCH=m
> CONFIG_I2C_HID_ACPI=m
> CONFIG_I2C_HID_OF=m
> +CONFIG_SPI_HID=m
> CONFIG_USB_CONN_GPIO=m
> CONFIG_USB=y
> CONFIG_USB_OTG=y
This chunk does not belong to this patch.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index a7c78ac96270..cd2c10703fcf 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1262,6 +1262,8 @@ source "drivers/hid/usbhid/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 55a6fa3eca5a..caf418dda343 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -144,6 +144,7 @@ obj-$(CONFIG_USB_MOUSE) += usbhid/
> obj-$(CONFIG_USB_KBD) += usbhid/
>
> obj-$(CONFIG_I2C_HID_CORE) += i2c-hid/
> +obj-$(CONFIG_SPI_HID) += spi-hid/
>
> obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
> obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/
> diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
> new file mode 100644
> index 000000000000..37302d658162
> --- /dev/null
> +++ b/drivers/hid/spi-hid/Kconfig
> @@ -0,0 +1,25 @@
> +#
> +# Copyright (c) 2021 Microsoft Corporation
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 2 as published by
> +# the Free Software Foundation.
> +#
> +menu "SPI HID support"
> + depends on SPI
> +
> +config SPI_HID
> + tristate "HID over SPI transport layer"
> + default n
> + depends on SPI && INPUT && OF
> + select HID
> + help
> + Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
> + other HID based devices which is connected to your computer via SPI.
> +
> + If unsure, say N.
> +
> + This support is also available as a module. If so, the module
> + will be called spi-hid.
> +
> +endmenu
> diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
> new file mode 100644
> index 000000000000..0d34a04b5dc7
> --- /dev/null
> +++ b/drivers/hid/spi-hid/Makefile
> @@ -0,0 +1,12 @@
> +#
> +# Copyright (c) 2021 Microsoft Corporation
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 2 as published by
> +# the Free Software Foundation.
> +#
> +# Makefile for the SPI input drivers
> +#
> +CFLAGS_trace.o = -I$(src)
> +obj-$(CONFIG_SPI_HID) += spi-hid.o
> +spi-hid-objs := spi-hid-core.o spi-hid-of.o trace.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..4d5a24613beb
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -0,0 +1,1328 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HID over SPI protocol implementation
> + * spi-hid-core.c
I recommend not to put file names in comments as they tend to get stale.
> + *
> + * Copyright (c) 2021 Microsoft Corporation
> + *
> + * 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/crc32.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/hid.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/of.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/string.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
> +
> +#include "spi-hid-core.h"
> +#include "spi-hid_trace.h"
> +#include "spi-hid-of.h"
> +#include "../hid-ids.h"
> +
> +#define SPI_HID_MAX_RESET_ATTEMPTS 3
> +
> +static struct hid_ll_driver spi_hid_ll_driver;
> +
> +static void spi_hid_populate_read_approvals(struct spi_hid_of_config *conf,
> + __u8 *header_buf, __u8 *body_buf)
I think this is largely matter of preference, but usually in kernel we
use u8, u16, etc, and variants with underscores are used in headers
exported to userspace.
> +{
> + header_buf[0] = conf->read_opcode;
> + header_buf[1] = (conf->input_report_header_address >> 16) & 0xff;
> + header_buf[2] = (conf->input_report_header_address >> 8) & 0xff;
> + header_buf[3] = (conf->input_report_header_address >> 0) & 0xff;
> + header_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
> +
> + body_buf[0] = conf->read_opcode;
> + body_buf[1] = (conf->input_report_body_address >> 16) & 0xff;
> + body_buf[2] = (conf->input_report_body_address >> 8) & 0xff;
> + body_buf[3] = (conf->input_report_body_address >> 0) & 0xff;
> + body_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
> +}
> +
> +static void spi_hid_parse_dev_desc(struct spi_hid_device_desc_raw *raw,
> + struct spi_hid_device_descriptor *desc)
> +{
> + desc->hid_version = le16_to_cpu(raw->bcdVersion);
> + desc->report_descriptor_length = le16_to_cpu(raw->wReportDescLength);
> + desc->max_input_length = le16_to_cpu(raw->wMaxInputLength);
> + desc->max_output_length = le16_to_cpu(raw->wMaxOutputLength);
> +
> + /* FIXME: multi-fragment not supported, field below not used */
> + desc->max_fragment_length = le16_to_cpu(raw->wMaxFragmentLength);
> +
> + desc->vendor_id = le16_to_cpu(raw->wVendorID);
> + desc->product_id = le16_to_cpu(raw->wProductID);
> + desc->version_id = le16_to_cpu(raw->wVersionID);
> + desc->no_output_report_ack = le16_to_cpu(raw->wFlags) & BIT(0);
> +}
> +
> +static void spi_hid_populate_input_header(__u8 *buf,
> + struct spi_hid_input_header *header)
> +{
> + header->version = buf[0] & 0xf;
> + header->report_length = (buf[1] | ((buf[2] & 0x3f) << 8)) * 4;
> + header->last_fragment_flag = (buf[2] & 0x40) >> 6;
> + header->sync_const = buf[3];
> +}
> +
> +static void spi_hid_populate_input_body(__u8 *buf,
> + struct spi_hid_input_body *body)
> +{
> + body->report_type = buf[0];
> + body->content_length = buf[1] | (buf[2] << 8);
Where possible we should use
get_unaligned_le16/put_unaligned_le16/cpu_to_le16/le16_to_cpu/etc.
It documents data structures better and may result in slightly better
performance.
> + body->content_id = buf[3];
> +}
> +
> +static void spi_hid_input_report_prepare(struct spi_hid_input_buf *buf,
> + struct spi_hid_input_report *report)
> +{
> + struct spi_hid_input_header header;
> + struct spi_hid_input_body body;
> +
> + spi_hid_populate_input_header(buf->header, &header);
> + spi_hid_populate_input_body(buf->body, &body);
> + report->report_type = body.report_type;
> + report->content_length = body.content_length;
> + report->content_id = body.content_id;
> + report->content = buf->content;
> +}
> +
> +static void spi_hid_populate_output_header(__u8 *buf,
> + struct spi_hid_of_config *conf,
> + struct spi_hid_output_report *report)
> +{
> + buf[0] = conf->write_opcode;
> + buf[1] = (conf->output_report_address >> 16) & 0xff;
> + buf[2] = (conf->output_report_address >> 8) & 0xff;
> + buf[3] = (conf->output_report_address >> 0) & 0xff;
> + buf[4] = report->report_type;
> + buf[5] = report->content_length & 0xff;
> + buf[6] = (report->content_length >> 8) & 0xff;
le16...
> + buf[7] = report->content_id;
> +}
> +
> +static int spi_hid_input_async(struct spi_hid *shid, void *buf, u16 length,
> + void (*complete)(void *), bool is_header)
> +{
> + int ret;
> + struct device *dev = &shid->spi->dev;
> +
> + 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);
> +
> + shid->input_message.complete = complete;
> + shid->input_message.context = shid;
> +
> + trace_spi_hid_input_async(shid,
> + shid->input_transfer[0].tx_buf,
> + shid->input_transfer[0].len,
> + shid->input_transfer[1].rx_buf,
> + shid->input_transfer[1].len, 0);
> +
> + ret = spi_async(shid->spi, &shid->input_message);
> + if (ret) {
> + dev_err(dev, "Error starting async transfer: %d, resetting\n",
> + ret);
> + shid->bus_error_count++;
> + shid->bus_last_error = ret;
> + schedule_work(&shid->error_work);
> + }
> +
> + return ret;
> +}
> +
> +static int spi_hid_output(struct spi_hid *shid, void *buf, u16 length)
> +{
> + struct spi_transfer transfer;
> + struct spi_message message;
> + int ret;
> +
> + memset(&transfer, 0, sizeof(transfer));
> +
> + transfer.tx_buf = buf;
> + transfer.len = length;
> +
> + spi_message_init_with_transfers(&message, &transfer, 1);
> +
> + /*
> + * REVISIT: Should output be asynchronous?
> + *
> + * According to Documentation/hid/hid-transport.rst, ->output_report()
> + * must be implemented as an asynchronous operation.
> + */
I am not quite sure what that doc means. Do they simply mean that the
driver should not wait for response from the device?
I think that transfer (send) can still happen synchronously.
> + trace_spi_hid_output_begin(shid, transfer.tx_buf,
> + transfer.len, NULL, 0, 0);
> +
> + ret = spi_sync(shid->spi, &message);
> +
> + trace_spi_hid_output_end(shid, transfer.tx_buf,
> + transfer.len, NULL, 0, ret);
> +
> + if (ret) {
> + shid->bus_error_count++;
> + shid->bus_last_error = ret;
> + }
> +
> + return ret;
> +}
> +
> +static const char *const spi_hid_power_mode_string(u8 power_state)
> +{
> + switch (power_state) {
> + case SPI_HID_POWER_MODE_ON:
> + return "d0";
> + case SPI_HID_POWER_MODE_SLEEP:
> + return "d2";
> + case SPI_HID_POWER_MODE_OFF:
> + return "d3";
> + case SPI_HID_POWER_MODE_WAKING_SLEEP:
> + return "d3*";
> + default:
> + return "unknown";
> + }
> +}
> +
> +static void spi_hid_suspend(struct spi_hid *shid)
> +{
> + struct device *dev = &shid->spi->dev;
> +
> + if (shid->power_state == SPI_HID_POWER_MODE_OFF)
> + return;
> +
> + disable_irq(shid->spi->irq);
> + shid->ready = false;
> + sysfs_notify(&dev->kobj, NULL, "ready");
> +
> + spi_hid_of_assert_reset(&shid->conf);
Are you sure you want to have reset asserted while the device is
sleeping? Is it really a reset pin, or is it actually a GPIO-controlled
regulator?
> +
> + shid->power_state = SPI_HID_POWER_MODE_OFF;
> +}
> +
> +static void spi_hid_resume(struct spi_hid *shid)
> +{
> + if (shid->power_state == SPI_HID_POWER_MODE_ON)
> + return;
> +
> + shid->power_state = SPI_HID_POWER_MODE_ON;
> + enable_irq(shid->spi->irq);
> + shid->input_transfer_pending = 0;
> +
> + spi_hid_of_deassert_reset(&shid->conf);
> +}
> +
> +static struct hid_device *spi_hid_disconnect_hid(struct spi_hid *shid)
> +{
> + struct hid_device *hid = shid->hid;
> +
> + shid->hid = NULL;
Umm, why the wrapper?
> +
> + return hid;
> +}
> +
> +static void spi_hid_stop_hid(struct spi_hid *shid)
> +{
> + struct hid_device *hid;
> +
> + hid = spi_hid_disconnect_hid(shid);
> + if (hid) {
> + cancel_work_sync(&shid->create_device_work);
> + cancel_work_sync(&shid->refresh_device_work);
> + hid_destroy_device(hid);
> + }
> +}
> +
> +static void spi_hid_error_handler(struct spi_hid *shid)
> +{
> + struct device *dev = &shid->spi->dev;
> + int ret;
> +
> + if (shid->power_state == SPI_HID_POWER_MODE_OFF)
> + return;
> +
> + if (shid->attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> + dev_err(dev, "unresponsive device, aborting.\n");
> + spi_hid_stop_hid(shid);
> + spi_hid_of_assert_reset(&shid->conf);
> + ret = spi_hid_of_power_down(&shid->conf);
> + if (ret) {
> + dev_err(dev, "failed to disable regulator\n");
> + shid->regulator_error_count++;
> + shid->regulator_last_error = ret;
> + }
> + return;
> + }
> +
> + trace_spi_hid_error_handler(shid);
> +
> + shid->ready = false;
> + sysfs_notify(&dev->kobj, NULL, "ready");
> +
> + spi_hid_of_assert_reset(&shid->conf);
> +
> + shid->power_state = SPI_HID_POWER_MODE_OFF;
> + shid->input_transfer_pending = 0;
> + cancel_work_sync(&shid->reset_work);
> +
> + spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
> +
> + shid->power_state = SPI_HID_POWER_MODE_ON;
> +
> + spi_hid_of_deassert_reset(&shid->conf);
> +}
> +
> +static void spi_hid_error_work(struct work_struct *work)
> +{
> + struct spi_hid *shid = container_of(work, struct spi_hid, error_work);
> +
> + spi_hid_error_handler(shid);
> +}
> +
> +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 ret;
> +
> + if (report->content_length > shid->desc.max_output_length) {
> + dev_err(dev, "Output report too big, content_length 0x%x\n",
> + report->content_length);
> + ret = -E2BIG;
> + goto out;
> + }
> +
> + 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);
> +
> + ret = spi_hid_output(shid, buf, padded_length);
> + if (ret) {
> + dev_err(dev, "Failed output transfer\n");
> + goto out;
> + }
> +
> + return 0;
> +
> +out:
> + return ret;
> +}
> +
> +static int spi_hid_sync_request(struct spi_hid *shid,
> + struct spi_hid_output_report *report)
> +{
> + struct device *dev = &shid->spi->dev;
> + int ret = 0;
> +
> + ret = spi_hid_send_output_report(shid, report);
> + if (ret) {
> + dev_err(dev, "Failed to transfer output report\n");
> + return ret;
> + }
> +
> + mutex_unlock(&shid->lock);
> + ret = wait_for_completion_interruptible_timeout(&shid->output_done,
> + msecs_to_jiffies(1000));
> + mutex_lock(&shid->lock);
Where is the completion reinitialized?
> + if (ret == 0) {
> + dev_err(dev, "Response timed out\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Handle the reset response from the FW by sending a request for the device
> + * descriptor.
> + */
> +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;
> + struct spi_hid_output_report report = {
> + .report_type = SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST,
> + .content_length = 0x0,
> + .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
> + .content = NULL,
> + };
> + int ret;
> +
> + trace_spi_hid_reset_work(shid);
> +
> + if (shid->ready) {
> + dev_err(dev, "Spontaneous FW reset!");
> + shid->ready = false;
> + sysfs_notify(&dev->kobj, NULL, "ready");
> + shid->dir_count++;
> + }
> +
> + if (shid->power_state == SPI_HID_POWER_MODE_OFF)
> + return;
> +
> + if (flush_work(&shid->create_device_work))
> + dev_err(dev, "Reset handler waited for create_device_work");
> +
> + if (flush_work(&shid->refresh_device_work))
> + dev_err(dev, "Reset handler waited for refresh_device_work");
> +
> + mutex_lock(&shid->lock);
> + ret = spi_hid_sync_request(shid, &report);
> + mutex_unlock(&shid->lock);
> + if (ret) {
> + dev_WARN_ONCE(dev, true,
> + "Failed to send device descriptor request\n");
> + spi_hid_error_handler(shid);
> + }
> +}
> +
> +static int spi_hid_input_report_handler(struct spi_hid *shid,
> + struct spi_hid_input_buf *buf)
> +{
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_input_report r;
> + int ret;
> +
> + trace_spi_hid_input_report_handler(shid);
> +
> + if (!shid->ready || shid->refresh_in_progress || !shid->hid)
> + return 0;
> +
> + spi_hid_input_report_prepare(buf, &r);
> +
> + ret = hid_input_report(shid->hid, HID_INPUT_REPORT,
> + r.content - 1,
> + r.content_length + 1, 1);
> +
> + if (ret == -ENODEV || ret == -EBUSY) {
> + dev_err(dev, "ignoring report --> %d\n", ret);
> + return 0;
> + } else if (ret) {
> + dev_err(dev, "Bad input report, error %d\n", ret);
> + }
> +
> + return ret;
> +}
> +
> +static void spi_hid_response_handler(struct spi_hid *shid,
> + struct spi_hid_input_buf *buf)
> +{
> + trace_spi_hid_response_handler(shid);
> +
> + /* completion_done returns 0 if there are waiters, otherwise 1 */
> + if (completion_done(&shid->output_done)) {
> + dev_err(&shid->spi->dev, "Unexpected response report\n");
> + } else {
> + if (shid->input.body[0] ==
> + SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC ||
> + shid->input.body[0] ==
> + SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP) {
> + size_t response_length = (shid->input.body[1] |
> + (shid->input.body[2] << 8)) +
> + sizeof(shid->input.body);
> + memcpy(shid->response.body, shid->input.body,
> + response_length);
> + }
> + complete(&shid->output_done);
> + }
> +}
> +
> +/*
> + * 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)
> +{
> + int ret;
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_output_report report = {
> + .report_type = SPI_HID_OUTPUT_REPORT_TYPE_REPORT_DESC_REQUEST,
> + .content_length = 0,
> + .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
> + .content = NULL,
> + };
> +
> + ret = spi_hid_sync_request(shid, &report);
> + if (ret) {
> + dev_err(dev,
> + "Expected report descriptor not received! Error %d\n",
> + ret);
> + spi_hid_error_handler(shid);
> + goto out;
> + }
> +
> + ret = (shid->response.body[1] | (shid->response.body[2] << 8));
le16...
> + if (ret != shid->desc.report_descriptor_length) {
> + dev_err(dev,
> + "Received report descriptor length doesn't match device descriptor field, using min of the two\n");
> + ret = min_t(unsigned int, ret,
> + shid->desc.report_descriptor_length);
> + }
> +out:
> + return ret;
> +}
> +
> +static void 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_body body;
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_device_desc_raw *raw;
> + int ret;
> +
> + trace_spi_hid_process_input_report(shid);
> +
> + spi_hid_populate_input_header(buf->header, &header);
> + spi_hid_populate_input_body(buf->body, &body);
> +
> + if (body.content_length > header.report_length) {
> + dev_err(dev, "Bad body length %d > %d\n", body.content_length,
> + header.report_length);
> + schedule_work(&shid->error_work);
> + return;
> + }
> +
> + switch (body.report_type) {
> + case SPI_HID_INPUT_REPORT_TYPE_DATA:
> + ret = spi_hid_input_report_handler(shid, buf);
> + if (ret)
> + schedule_work(&shid->error_work);
> + break;
> + case SPI_HID_INPUT_REPORT_TYPE_RESET_RESP:
> + schedule_work(&shid->reset_work);
> + break;
> + case SPI_HID_INPUT_REPORT_TYPE_DEVICE_DESC:
> + /* Mark the completion done to avoid timeout */
> + spi_hid_response_handler(shid, buf);
> +
> + /* Reset attempts at every device descriptor fetch */
> + shid->attempts = 0;
> +
> + raw = (struct spi_hid_device_desc_raw *)buf->content;
> +
> + /* Validate device descriptor length before parsing */
> + if (body.content_length != SPI_HID_DEVICE_DESCRIPTOR_LENGTH) {
> + dev_err(dev,
> + "Invalid content length %d, expected %d\n",
> + body.content_length,
> + SPI_HID_DEVICE_DESCRIPTOR_LENGTH);
> + schedule_work(&shid->error_work);
> + break;
> + }
> +
> + if (le16_to_cpu(raw->wDeviceDescLength) !=
> + SPI_HID_DEVICE_DESCRIPTOR_LENGTH) {
> + dev_err(dev,
> + "Invalid wDeviceDescLength %d, expected %d\n",
> + raw->wDeviceDescLength,
> + SPI_HID_DEVICE_DESCRIPTOR_LENGTH);
> + schedule_work(&shid->error_work);
> + break;
> + }
> +
> + spi_hid_parse_dev_desc(raw, &shid->desc);
> +
> + if (shid->desc.hid_version != SPI_HID_SUPPORTED_VERSION) {
> + dev_err(dev,
> + "Unsupported device descriptor version %4x\n",
> + shid->desc.hid_version);
> + schedule_work(&shid->error_work);
> + break;
> + }
> +
> + if (!shid->hid)
> + schedule_work(&shid->create_device_work);
> + else
> + schedule_work(&shid->refresh_device_work);
> +
> + break;
> + case SPI_HID_INPUT_REPORT_TYPE_SET_OUTPUT_REPORT_RESP:
> + if (shid->desc.no_output_report_ack) {
> + dev_err(dev, "Unexpected output report response\n");
> + break;
> + }
> + fallthrough;
> + case SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP:
> + case SPI_HID_INPUT_REPORT_TYPE_SET_FEATURE_RESP:
> + if (!shid->ready) {
> + dev_err(dev,
> + "Unexpected response report while not ready: 0x%x\n",
> + body.report_type);
> + break;
> + }
> + fallthrough;
> + case SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC:
> + spi_hid_response_handler(shid, buf);
> + break;
> + /*
> + * FIXME: sending GET_INPUT and COMMAND reports not supported, thus
> + * throw away responses to those, they should never come.
> + */
> + case SPI_HID_INPUT_REPORT_TYPE_GET_INPUT_REPORT_RESP:
> + case SPI_HID_INPUT_REPORT_TYPE_COMMAND_RESP:
> + 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);
> + schedule_work(&shid->error_work);
> + break;
> + }
> +}
> +
> +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> + struct spi_hid_input_header *header)
> +{
> + struct device *dev = &shid->spi->dev;
> +
> + if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> + dev_err(dev, "Unknown input report version (v 0x%x)\n",
> + header->version);
> + return -EINVAL;
> + }
> +
> + if (shid->desc.max_input_length != 0 &&
> + header->report_length > shid->desc.max_input_length) {
> + dev_err(dev, "Input report body size %u > max expected of %u\n",
> + header->report_length,
> + shid->desc.max_input_length);
> + return -EMSGSIZE;
> + }
> +
> + if (header->last_fragment_flag != 1) {
> + dev_err(dev, "Multi-fragment reports not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (header->sync_const != SPI_HID_INPUT_HEADER_SYNC_BYTE) {
> + dev_err(dev, "Invalid input report sync constant (0x%x)\n",
> + header->sync_const);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int spi_hid_create_device(struct spi_hid *shid)
> +{
> + struct hid_device *hid;
> + struct device *dev = &shid->spi->dev;
> + int ret;
> +
> + hid = hid_allocate_device();
> +
> + if (IS_ERR(hid)) {
> + dev_err(dev, "Failed to allocate hid device: %ld\n",
> + PTR_ERR(hid));
> + ret = PTR_ERR(hid);
> + return ret;
> + }
> +
> + 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 %04hX:%04hX",
> + hid->vendor, hid->product);
> + strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
> +
> + shid->hid = hid;
> +
> + ret = hid_add_device(hid);
> + if (ret) {
> + dev_err(dev, "Failed to add hid device: %d\n", ret);
> + /*
> + * We likely got here because report descriptor request timed
> + * out. Let's disconnect and destroy the hid_device structure.
> + */
> + hid = spi_hid_disconnect_hid(shid);
> + if (hid)
> + hid_destroy_device(hid);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void spi_hid_create_device_work(struct work_struct *work)
> +{
> + struct spi_hid *shid =
> + container_of(work, struct spi_hid, create_device_work);
> + struct device *dev = &shid->spi->dev;
> + u8 prev_state = shid->power_state;
> + int ret;
> +
> + trace_spi_hid_create_device_work(shid);
> +
> + ret = spi_hid_create_device(shid);
> + if (ret) {
> + dev_err(dev, "Failed to create hid device\n");
> + return;
> + }
> +
> + spi_hid_suspend(shid);
> +
> + shid->attempts = 0;
> +
> + dev_dbg(dev, "%s: %s -> %s\n", __func__,
> + spi_hid_power_mode_string(prev_state),
> + spi_hid_power_mode_string(shid->power_state));
> +}
> +
> +static void spi_hid_refresh_device_work(struct work_struct *work)
> +{
> + struct spi_hid *shid =
> + container_of(work, struct spi_hid, refresh_device_work);
> + struct device *dev = &shid->spi->dev;
> + struct hid_device *hid;
> + int ret;
> + u32 new_crc32;
> +
> + trace_spi_hid_refresh_device_work(shid);
> +
> + mutex_lock(&shid->lock);
> + ret = spi_hid_report_descriptor_request(shid);
> + mutex_unlock(&shid->lock);
> + if (ret < 0) {
> + dev_err(dev,
> + "Refresh: failed report descriptor request, error %d",
> + ret);
> + return;
> + }
> +
> + new_crc32 = crc32_le(0, (unsigned char const *)shid->response.content,
> + (size_t)ret);
> + if (new_crc32 == shid->report_descriptor_crc32) {
> + shid->ready = true;
> + sysfs_notify(&dev->kobj, NULL, "ready");
> + return;
> + }
> +
> + shid->report_descriptor_crc32 = new_crc32;
> + shid->refresh_in_progress = true;
> +
> + hid = spi_hid_disconnect_hid(shid);
> + if (hid)
> + hid_destroy_device(hid);
> +
> + ret = spi_hid_create_device(shid);
> + if (ret)
> + dev_err(dev, "Failed to create hid device\n");
> +
> + shid->refresh_in_progress = false;
> + shid->ready = true;
> + sysfs_notify(&dev->kobj, NULL, "ready");
> +}
> +
> +static void spi_hid_input_header_complete(void *_shid);
> +
> +static void spi_hid_input_body_complete(void *_shid)
> +{
> + struct spi_hid *shid = _shid;
> + struct device *dev = &shid->spi->dev;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&shid->input_lock, flags);
> +
> + if (shid->power_state == SPI_HID_POWER_MODE_OFF) {
> + dev_warn(dev,
> + "input body complete called while device is off\n");
> + goto out;
> + }
> +
> + 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, resetting %d\n",
> + shid->input_message.status);
> + shid->bus_error_count++;
> + shid->bus_last_error = shid->input_message.status;
> + schedule_work(&shid->error_work);
> + goto out;
> + }
> +
> + spi_hid_process_input_report(shid, &shid->input);
> +
> + if (--shid->input_transfer_pending) {
> + struct spi_hid_input_buf *buf = &shid->input;
> +
> + trace_spi_hid_header_transfer(shid);
> + ret = spi_hid_input_async(shid, buf->header,
> + sizeof(buf->header),
> + spi_hid_input_header_complete, true);
> + if (ret)
> + dev_err(dev, "failed to start header transfer %d\n",
> + ret);
> + }
> +
> +out:
> + spin_unlock_irqrestore(&shid->input_lock, flags);
> +}
> +
> +static void spi_hid_input_header_complete(void *_shid)
> +{
> + struct spi_hid *shid = _shid;
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_input_header header;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&shid->input_lock, flags);
> +
> + if (shid->power_state == SPI_HID_POWER_MODE_OFF) {
> + dev_warn(dev,
> + "input header complete called while device is off\n");
> + goto out;
> + }
> +
> + 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, resetting, error %d\n",
> + shid->input_message.status);
> + shid->bus_error_count++;
> + shid->bus_last_error = shid->input_message.status;
> + schedule_work(&shid->error_work);
> + goto out;
> + }
> + spi_hid_populate_input_header(shid->input.header, &header);
> +
> + ret = spi_hid_bus_validate_header(shid, &header);
> + if (ret) {
> + dev_err(dev, "failed to validate header: %d\n", ret);
> + print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
> + DUMP_PREFIX_NONE, 16, 1,
> + shid->input.header,
> + sizeof(shid->input.header),
> + false);
> + shid->bus_error_count++;
> + shid->bus_last_error = ret;
> + goto out;
> + }
> +
> + ret = spi_hid_input_async(shid, shid->input.body, header.report_length,
> + spi_hid_input_body_complete, false);
> + if (ret)
> + dev_err(dev, "failed body async transfer: %d\n", ret);
> +
> +out:
> + if (ret)
> + shid->input_transfer_pending = 0;
> +
> + spin_unlock_irqrestore(&shid->input_lock, flags);
> +}
> +
> +static int spi_hid_get_request(struct spi_hid *shid, u8 content_id)
> +{
> + int ret;
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_output_report report = {
> + .report_type = SPI_HID_OUTPUT_REPORT_TYPE_HID_GET_FEATURE,
> + .content_length = 0,
> + .content_id = content_id,
> + .content = NULL,
> + };
> +
> + ret = spi_hid_sync_request(shid, &report);
> + if (ret) {
> + dev_err(dev,
> + "Expected get request response not received! Error %d\n",
> + ret);
> + schedule_work(&shid->error_work);
> + }
> +
> + return ret;
> +}
> +
> +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 = SPI_HID_OUTPUT_REPORT_TYPE_HID_SET_FEATURE,
> + .content_length = arg_len,
> + .content_id = content_id,
> + .content = arg_buf,
> + };
> +
> + return spi_hid_sync_request(shid, &report);
> +}
> +
> +static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> +{
> + struct spi_hid *shid = _shid;
> + struct device *dev = &shid->spi->dev;
> + int ret = 0;
> +
> + spin_lock(&shid->input_lock);
> + trace_spi_hid_dev_irq(shid, irq);
> +
> + if (shid->input_transfer_pending++)
> + goto out;
> +
> + trace_spi_hid_header_transfer(shid);
> + ret = spi_hid_input_async(shid, shid->input.header,
> + sizeof(shid->input.header),
> + spi_hid_input_header_complete, true);
> + if (ret)
> + dev_err(dev, "Failed to start header transfer: %d\n", ret);
> +
> +out:
> + spin_unlock(&shid->input_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/* 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);
> +
> + if (shid->desc.max_input_length < HID_MIN_BUFFER_SIZE) {
> + dev_err(&shid->spi->dev,
> + "HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
> + shid->desc.max_input_length);
> + return -EINVAL;
> + }
> +
> + 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)
> +{
> + struct spi_device *spi = hid->driver_data;
> + struct spi_hid *shid = spi_get_drvdata(spi);
> + struct device *dev = &spi->dev;
> + u8 prev_state = shid->power_state;
> +
> + if (shid->refresh_in_progress)
> + return 0;
> +
> + spi_hid_resume(shid);
> +
> + dev_dbg(dev, "%s: %s -> %s\n", __func__,
> + spi_hid_power_mode_string(prev_state),
> + spi_hid_power_mode_string(shid->power_state));
> +
> + 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);
> + struct device *dev = &spi->dev;
> + u8 prev_state = shid->power_state;
> +
> + if (shid->refresh_in_progress)
> + return;
> +
> + spi_hid_suspend(shid);
> +
> + shid->attempts = 0;
> +
> + dev_dbg(dev, "%s: %s -> %s\n", __func__,
> + spi_hid_power_mode_string(prev_state),
> + spi_hid_power_mode_string(shid->power_state));
> +}
> +
> +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 ret = 0;
> +
> + mutex_lock(&shid->lock);
> + if (!shid->hid)
> + ret = -ENODEV;
> + mutex_unlock(&shid->lock);
> +
> + return ret;
> +}
> +
> +static int spi_hid_ll_parse(struct hid_device *hid)
> +{
> + struct spi_device *spi = hid->driver_data;
> + struct spi_hid *shid = spi_get_drvdata(spi);
> + struct device *dev = &spi->dev;
> + int ret, len;
> +
> + mutex_lock(&shid->lock);
> +
> + len = spi_hid_report_descriptor_request(shid);
> + if (len < 0) {
> + dev_err(dev, "Report descriptor request failed, %d\n", len);
> + ret = len;
> + goto out;
> + }
> +
> + /*
> + * 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.
> + */
> + ret = hid_parse_report(hid, (__u8 *)shid->response.content, len);
> + if (ret)
> + dev_err(dev, "failed parsing report: %d\n", ret);
> + else
> + shid->report_descriptor_crc32 = crc32_le(0,
> + (unsigned char const *)shid->response.content,
> + len);
> +
> +out:
> + mutex_unlock(&shid->lock);
> +
> + return ret;
> +}
> +
> +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;
> +
> + if (!shid->ready) {
> + dev_err(&shid->spi->dev, "%s called in unready state\n",
> + __func__);
> + return -ENODEV;
This will confuse users for no good reason. Do not let them call into
the driver until it is ready.
> + }
> +
> + mutex_lock(&shid->lock);
> +
> + switch (reqtype) {
> + case HID_REQ_SET_REPORT:
> + if (buf[0] != reportnum) {
> + dev_err(dev, "report id mismatch\n");
> + ret = -EINVAL;
> + break;
> + }
> +
> + ret = spi_hid_set_request(shid, &buf[1], len - 1,
> + reportnum);
> + if (ret) {
> + dev_err(dev, "failed to set report\n");
> + break;
> + }
> +
> + ret = len;
> + break;
> + case HID_REQ_GET_REPORT:
> + ret = spi_hid_get_request(shid, reportnum);
> + if (ret) {
> + dev_err(dev, "failed to get report\n");
> + break;
> + }
> +
> + ret = min_t(size_t, len,
> + shid->response.body[1] | (shid->response.body[2] << 8));
> + memcpy(buf, &shid->response.content, ret);
> + break;
> + default:
> + dev_err(dev, "invalid request type\n");
> + ret = -EIO;
> + }
> +
> + mutex_unlock(&shid->lock);
> +
> + return ret;
> +}
> +
> +static int spi_hid_ll_output_report(struct hid_device *hid,
> + __u8 *buf, size_t len)
> +{
> + int ret;
> + 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 = SPI_HID_OUTPUT_REPORT_TYPE_HID_OUTPUT_REPORT,
> + .content_length = len - 1,
> + .content_id = buf[0],
> + .content = &buf[1],
> + };
> +
> + mutex_lock(&shid->lock);
> + if (!shid->ready) {
> + dev_err(dev, "%s called in unready state\n", __func__);
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + if (shid->desc.no_output_report_ack)
> + ret = spi_hid_send_output_report(shid, &report);
> + else
> + ret = spi_hid_sync_request(shid, &report);
> +
> + if (ret)
> + dev_err(dev, "failed to send output report\n");
> +
> +out:
> + mutex_unlock(&shid->lock);
> +
> + if (ret > 0)
> + return -ret;
> +
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +static struct hid_ll_driver spi_hid_ll_driver = {
> + .start = spi_hid_ll_start,
> + .stop = spi_hid_ll_stop,
> + .open = spi_hid_ll_open,
> + .close = spi_hid_ll_close,
> + .power = spi_hid_ll_power,
> + .parse = spi_hid_ll_parse,
> + .output_report = spi_hid_ll_output_report,
> + .raw_request = spi_hid_ll_raw_request,
> +};
> +
> +static ssize_t ready_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%s\n",
> + shid->ready ? "ready" : "not ready");
Why is this needed? Why can't we simply delay registering HID device
until after we are ready to handle it? Or delay in open?
> +}
> +static DEVICE_ATTR_RO(ready);
> +
> +static ssize_t bus_error_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d (%d)\n",
> + shid->bus_error_count, shid->bus_last_error);
This and below I think better suited for debugfs.
> +}
> +static DEVICE_ATTR_RO(bus_error_count);
> +
> +static ssize_t regulator_error_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d (%d)\n",
> + shid->regulator_error_count,
> + shid->regulator_last_error);
> +}
> +static DEVICE_ATTR_RO(regulator_error_count);
> +
> +static ssize_t device_initiated_reset_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_hid *shid = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", shid->dir_count);
> +}
> +static DEVICE_ATTR_RO(device_initiated_reset_count);
> +
> +static const struct attribute *const spi_hid_attributes[] = {
> + &dev_attr_ready.attr,
> + &dev_attr_bus_error_count.attr,
> + &dev_attr_regulator_error_count.attr,
> + &dev_attr_device_initiated_reset_count.attr,
> + NULL /* Terminator */
> +};
> +
> +static int spi_hid_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct spi_hid *shid;
> + unsigned long irqflags;
> + int ret;
> +
> + if (spi->irq <= 0) {
> + dev_err(dev, "Missing IRQ\n");
> + ret = spi->irq ?: -EINVAL;
> + goto err0;
> + }
> +
> + shid = devm_kzalloc(dev, sizeof(struct spi_hid), GFP_KERNEL);
> + if (!shid) {
> + ret = -ENOMEM;
> + goto err0;
> + }
> +
> + shid->spi = spi;
> + shid->power_state = SPI_HID_POWER_MODE_ON;
> + spi_set_drvdata(spi, shid);
> +
> + ret = sysfs_create_files(&dev->kobj, spi_hid_attributes);
I'd recommend using devm* for all resources used by this driver.
> + if (ret) {
> + dev_err(dev, "Unable to create sysfs attributes\n");
> + goto err0;
> + }
> +
> + ret = spi_hid_of_populate_config(&shid->conf, dev);
Error handling?
> +
> + /* 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->lock);
> + init_completion(&shid->output_done);
> +
> + spin_lock_init(&shid->input_lock);
> + INIT_WORK(&shid->reset_work, spi_hid_reset_work);
> + INIT_WORK(&shid->create_device_work, spi_hid_create_device_work);
> + INIT_WORK(&shid->refresh_device_work, spi_hid_refresh_device_work);
> + INIT_WORK(&shid->error_work, spi_hid_error_work);
> +
> + /*
> + * At the end of probe we initialize the device:
> + * 0) Default pinctrl in DT: assert reset, bias the interrupt line
> + * 1) sleep minimal reset delay
> + * 2) request IRQ
> + * 3) power up the device
> + * 4) sleep 5ms
> + * 5) deassert reset (high)
> + * 6) sleep 5ms
> + */
> +
> + spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
> +
> + irqflags = irq_get_trigger_type(spi->irq) | IRQF_ONESHOT;
Specifying ONESHOT mode for non-threaded IRQ does not make much sense.
Why don't you use threaded IRQ and get rid of your async/work hanlding?
> + ret = request_irq(spi->irq, spi_hid_dev_irq, irqflags,
> + dev_name(&spi->dev), shid);
> + if (ret)
> + goto err1;
> +
> + ret = spi_hid_of_power_up(&shid->conf);
> + if (ret) {
> + dev_err(dev, "%s: could not power up\n", __func__);
> + shid->regulator_error_count++;
> + shid->regulator_last_error = ret;
> + goto err1;
> + }
> +
> + spi_hid_of_deassert_reset(&shid->conf);
> +
> + dev_err(dev, "%s: d3 -> %s\n", __func__,
> + spi_hid_power_mode_string(shid->power_state));
> +
> + return 0;
> +
> +err1:
> + sysfs_remove_files(&dev->kobj, spi_hid_attributes);
> +
> +err0:
> + return ret;
> +}
> +
> +static int spi_hid_remove(struct spi_device *spi)
> +{
> + struct spi_hid *shid = spi_get_drvdata(spi);
> + struct device *dev = &spi->dev;
> + int ret;
> +
> + spi_hid_of_assert_reset(&shid->conf);
> + ret = spi_hid_of_power_down(&shid->conf);
> + if (ret) {
> + dev_err(dev, "failed to disable regulator\n");
> + shid->regulator_error_count++;
> + shid->regulator_last_error = ret;
> + }
> + free_irq(spi->irq, shid);
> + sysfs_remove_files(&dev->kobj, spi_hid_attributes);
> + spi_hid_stop_hid(shid);
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id spi_hid_id_table[] = {
> + { "hid", 0 },
> + { "hid-over-spi", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(spi, spi_hid_id_table);
> +
> +static struct spi_driver spi_hid_driver = {
> + .driver = {
> + .name = "spi_hid",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(spi_hid_of_match),
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .probe = spi_hid_probe,
> + .remove = spi_hid_remove,
> + .id_table = spi_hid_id_table,
> +};
> +
> +module_spi_driver(spi_hid_driver);
> +
> +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
> new file mode 100644
> index 000000000000..95e667ad53ec
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.h
> @@ -0,0 +1,188 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * spi-hid-core.h
> + *
> + * Copyright (c) 2021 Microsoft Corporation
> + */
> +
> +#ifndef SPI_HID_CORE_H
> +#define SPI_HID_CORE_H
> +
> +#include <linux/completion.h>
> +#include <linux/kernel.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include "spi-hid-of.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
> +
> +/* Protocol message size constants */
> +#define SPI_HID_READ_APPROVAL_LEN 5
> +#define SPI_HID_INPUT_HEADER_LEN 4
> +#define SPI_HID_INPUT_BODY_LEN 4
> +#define SPI_HID_OUTPUT_HEADER_LEN 8
> +#define SPI_HID_DEVICE_DESCRIPTOR_LENGTH 24
> +
> +/* Protocol message type constants */
> +#define SPI_HID_INPUT_REPORT_TYPE_DATA 0x01
> +#define SPI_HID_INPUT_REPORT_TYPE_RESET_RESP 0x03
> +#define SPI_HID_INPUT_REPORT_TYPE_COMMAND_RESP 0x04
> +#define SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP 0x05
> +#define SPI_HID_INPUT_REPORT_TYPE_DEVICE_DESC 0x07
> +#define SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC 0x08
> +#define SPI_HID_INPUT_REPORT_TYPE_SET_FEATURE_RESP 0x09
> +#define SPI_HID_INPUT_REPORT_TYPE_SET_OUTPUT_REPORT_RESP 0x0a
> +#define SPI_HID_INPUT_REPORT_TYPE_GET_INPUT_REPORT_RESP 0x0b
> +
> +#define SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST 0x01
> +#define SPI_HID_OUTPUT_REPORT_TYPE_REPORT_DESC_REQUEST 0x02
> +#define SPI_HID_OUTPUT_REPORT_TYPE_HID_SET_FEATURE 0x03
> +#define SPI_HID_OUTPUT_REPORT_TYPE_HID_GET_FEATURE 0x04
> +#define SPI_HID_OUTPUT_REPORT_TYPE_HID_OUTPUT_REPORT 0x05
> +#define SPI_HID_OUTPUT_REPORT_TYPE_INPUT_REPORT_REQUEST 0x06
> +#define SPI_HID_OUTPUT_REPORT_TYPE_COMMAND 0x07
> +
> +#define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST 0x00
> +
> +/* Power mode constants */
> +#define SPI_HID_POWER_MODE_ON 0x01
> +#define SPI_HID_POWER_MODE_SLEEP 0x02
> +#define SPI_HID_POWER_MODE_OFF 0x03
> +#define SPI_HID_POWER_MODE_WAKING_SLEEP 0x04
> +
> +/* Raw input buffer with data from the bus */
> +struct spi_hid_input_buf {
> + __u8 header[SPI_HID_INPUT_HEADER_LEN];
> + __u8 body[SPI_HID_INPUT_BODY_LEN];
> + u8 content[SZ_8K];
> +};
> +
> +/* 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 input report body, excluding the content */
> +struct spi_hid_input_body {
> + u8 report_type;
> + u16 content_length;
> + u8 content_id;
> +};
> +
> +/* 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];
> + u8 content[SZ_8K];
> +};
> +
> +/* Data necessary to send an output report */
> +struct spi_hid_output_report {
> + u8 report_type;
> + u16 content_length;
> + u8 content_id;
> + u8 *content;
> +};
> +
> +/* Raw content in device descriptor */
> +struct spi_hid_device_desc_raw {
> + __le16 wDeviceDescLength;
> + __le16 bcdVersion;
> + __le16 wReportDescLength;
> + __le16 wMaxInputLength;
> + __le16 wMaxOutputLength;
> + __le16 wMaxFragmentLength;
> + __le16 wVendorID;
> + __le16 wProductID;
> + __le16 wVersionID;
> + __le16 wFlags;
> + __u8 reserved[4];
> +} __packed;
> +
> +/* 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;
bool?
> +};
> +
> +/* Driver context */
> +struct spi_hid {
> + struct spi_device *spi;
> + struct hid_device *hid;
> +
> + struct spi_transfer input_transfer[2];
> + struct spi_transfer output_transfer;
> + struct spi_message input_message;
> + struct spi_message output_message;
> +
> + struct spi_hid_of_config conf;
> + struct spi_hid_device_descriptor desc;
> + struct spi_hid_output_buf output;
> + struct spi_hid_input_buf input;
> + struct spi_hid_input_buf response;
> +
> + spinlock_t input_lock;
> +
> + u32 input_transfer_pending;
> +
> + u8 power_state;
> +
> + u8 attempts;
> +
> + /*
> + * ready flag indicates that the FW is ready to accept commands and
> + * requests. The FW becomes ready after sending the report descriptor.
> + */
> + bool ready;
> + /*
> + * refresh_in_progress is set to true while the refresh_device worker
> + * thread is destroying and recreating the hidraw device. When this flag
> + * is set to true, the ll_close and ll_open functions will not cause
> + * power state changes.
> + */
> + bool refresh_in_progress;
> +
> + struct work_struct reset_work;
> + struct work_struct create_device_work;
> + struct work_struct refresh_device_work;
> + struct work_struct error_work;
> +
> + struct mutex lock;
> + struct completion output_done;
> +
> + __u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN];
> + __u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
> +
> + u32 report_descriptor_crc32;
> +
> + u32 regulator_error_count;
> + int regulator_last_error;
> + u32 bus_error_count;
> + int bus_last_error;
> + u32 dir_count;
> +};
> +
> +#endif
> diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
> new file mode 100755
> index 000000000000..4896a90c2e8e
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-of.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HID over SPI protocol, Open Firmware related code
> + * spi-hid-of.c
> + *
> + * 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/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +
> +#include "spi-hid-core.h"
> +
> +const struct of_device_id spi_hid_of_match[] = {
> + { .compatible = "hid-over-spi" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, spi_hid_of_match);
> +
> +int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> + struct device *dev)
> +{
> + int ret;
> + u32 val;
> +
> + ret = device_property_read_u32(dev, "input-report-header-address",
> + &val);
> + if (ret) {
> + dev_err(dev, "Input report header address not provided\n");
> + return -ENODEV;
> + }
> + conf->input_report_header_address = val;
> +
> + ret = device_property_read_u32(dev, "input-report-body-address", &val);
> + if (ret) {
> + dev_err(dev, "Input report body address not provided\n");
> + return -ENODEV;
> + }
> + conf->input_report_body_address = val;
> +
> + ret = device_property_read_u32(dev, "output-report-address", &val);
> + if (ret) {
> + dev_err(dev, "Output report address not provided\n");
> + return -ENODEV;
> + }
> + conf->output_report_address = val;
> +
> + ret = device_property_read_u32(dev, "read-opcode", &val);
> + if (ret) {
> + dev_err(dev, "Read opcode not provided\n");
> + return -ENODEV;
> + }
> + conf->read_opcode = val;
> +
> + ret = device_property_read_u32(dev, "write-opcode", &val);
> + if (ret) {
> + dev_err(dev, "Write opcode not provided\n");
> + return -ENODEV;
> + }
> + conf->write_opcode = val;
> +
> + ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);
> + if (ret) {
> + dev_err(dev, "Post-power-on delay not provided\n");
> + return -ENODEV;
> + }
> + conf->post_power_on_delay_ms = val;
> +
> + ret = device_property_read_u32(dev, "minimal-reset-delay-ms", &val);
> + if (ret) {
> + dev_err(dev, "Minimal reset time not provided\n");
> + return -ENODEV;
> + }
> + conf->minimal_reset_delay_ms = val;
> +
> + /* FIXME: not reading flags from DT, multi-SPI modes not supported */
> +
> + conf->supply = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(conf->supply)) {
> + if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get regulator: %ld\n",
> + PTR_ERR(conf->supply));
> + return PTR_ERR(conf->supply);
> + }
> +
> + conf->reset_gpio = devm_gpiod_get(dev, "reset-gpio", GPIOD_OUT_LOW);
> + if (IS_ERR(conf->reset_gpio)) {
> + dev_err(dev, "%s: error getting GPIO\n", __func__);
> + return PTR_ERR(conf->reset_gpio);
> + }
> +
> + return 0;
> +}
> +
> +int spi_hid_of_power_down(struct spi_hid_of_config *conf)
> +{
> + if (regulator_is_enabled(conf->supply) == 0)
> + return 0;
> +
> + return regulator_disable(conf->supply);
> +}
> +
> +int spi_hid_of_power_up(struct spi_hid_of_config *conf)
> +{
> + int ret;
> +
> + if (regulator_is_enabled(conf->supply) > 0)
> + return 0;
> +
> + ret = regulator_enable(conf->supply);
> +
> + usleep_range(1000 * conf->post_power_on_delay_ms,
> + 1000 * (conf->post_power_on_delay_ms + 1));
> +
> + return ret;
> +}
> +
> +void spi_hid_of_assert_reset(struct spi_hid_of_config *conf)
> +{
> + gpiod_set_value(conf->reset_gpio, 1);
> +}
> +
> +void spi_hid_of_deassert_reset(struct spi_hid_of_config *conf)
> +{
> + gpiod_set_value(conf->reset_gpio, 0);
Why do we need these wrappers?
> +}
> +
> +void spi_hid_of_sleep_minimal_reset_delay(struct spi_hid_of_config *conf)
> +{
> + usleep_range(1000 * conf->minimal_reset_delay_ms,
> + 1000 * (conf->minimal_reset_delay_ms + 1));
> +}
> \ No newline at end of file
> diff --git a/drivers/hid/spi-hid/spi-hid-of.h b/drivers/hid/spi-hid/spi-hid-of.h
> new file mode 100755
> index 000000000000..5991011d8921
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-of.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * spi-hid-of.h
> + *
> + * Copyright (c) 2021 Microsoft Corporation
> + */
> +
> +#ifndef SPI_HID_OF_H
> +#define SPI_HID_OF_H
> +
> +extern const struct of_device_id spi_hid_of_match[];
> +
> +/* Config structure is filled with data from Device Tree */
> +struct spi_hid_of_config {
> + u32 input_report_header_address;
> + u32 input_report_body_address;
> + u32 output_report_address;
> + u8 read_opcode;
> + u8 write_opcode;
> + u32 post_power_on_delay_ms;
> + u32 minimal_reset_delay_ms;
> + struct gpio_desc *reset_gpio;
> + struct regulator *supply;
> +};
> +
> +int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> + struct device *dev);
> +int spi_hid_of_power_down(struct spi_hid_of_config *conf);
> +int spi_hid_of_power_up(struct spi_hid_of_config *conf);
> +void spi_hid_of_assert_reset(struct spi_hid_of_config *conf);
> +void spi_hid_of_deassert_reset(struct spi_hid_of_config *conf);
> +void spi_hid_of_sleep_minimal_reset_delay(struct spi_hid_of_config *conf);
> +
> +#endif
> 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..905de02d85aa
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid_trace.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * spi-hid_trace.h
> + *
> + * 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"
> +
> +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, len)
> + __field(int, ret)
> + __dynamic_array(u8, rx_buf, rx_len)
> + __dynamic_array(u8, tx_buf, tx_len)
> + ),
> +
> + TP_fast_assign(
> + __entry->bus_num = shid->spi->controller->bus_num;
> + __entry->chip_select = shid->spi->chip_select;
> + __entry->len = rx_len + tx_len;
> + __entry->ret = ret;
> +
> + memcpy(__get_dynamic_array(tx_buf), tx_buf, tx_len);
> + memcpy(__get_dynamic_array(rx_buf), rx_buf, rx_len);
> + ),
> +
> + TP_printk("spi%d.%d: len=%d tx=[%*phD] rx=[%*phD] --> %d",
> + __entry->bus_num, __entry->chip_select, __entry->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_async,
> + 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)
> +);
> +
> +DEFINE_EVENT(spi_hid_transfer, spi_hid_output_begin,
> + 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_output_end,
> + 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 = shid->spi->chip_select;
> + __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, input_transfer_pending)
> + __field(bool, ready)
> +
> + __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 = shid->spi->chip_select;
> + __entry->power_state = shid->power_state;
> + __entry->input_transfer_pending = shid->input_transfer_pending;
> + __entry->ready = shid->ready;
> +
> + __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 i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
> + __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->ready ? 'R' : 'r', __entry->input_transfer_pending)
> +);
> +
> +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_work,
> + TP_PROTO(struct spi_hid *shid),
> + TP_ARGS(shid)
> +);
> +
> +DEFINE_EVENT(spi_hid, spi_hid_create_device_work,
> + TP_PROTO(struct spi_hid *shid),
> + TP_ARGS(shid)
> +);
> +
> +DEFINE_EVENT(spi_hid, spi_hid_refresh_device_work,
> + 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 */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH .
> +#define TRACE_INCLUDE_FILE spi-hid_trace
> +#include <trace/define_trace.h>
> diff --git a/drivers/hid/spi-hid/trace.c b/drivers/hid/spi-hid/trace.c
> new file mode 100644
> index 000000000000..aaa2da0c9642
> --- /dev/null
> +++ b/drivers/hid/spi-hid/trace.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * trace.c - SPI HID Trace Support
> + *
> + * Copyright (c) 2021 Microsoft Corporation
> + *
> + * Author: Felipe Balbi <felipe.balbi@microsoft.com>
> + */
> +
> +#define CREATE_TRACE_POINTS
> +#include "spi-hid_trace.h"
> --
> 2.25.1
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus
2022-01-15 2:31 ` [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus Dmitry Antipov
@ 2022-01-15 6:10 ` kernel test robot
2022-01-15 6:10 ` kernel test robot
1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2022-01-15 6:10 UTC (permalink / raw)
To: Dmitry Antipov, Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov,
Rob Herring, Mark Brown, Felipe Balbi
Cc: kbuild-all, linux-input, devicetree, linux-spi, Dmitry Antipov
Hi Dmitry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hid/for-next]
[also build test WARNING on jikos-trivial/for-next v5.16 next-20220114]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dmitry-Antipov/Add-spi-hid-transport-for-HID-over-SPI-bus/20220115-103254
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20220115/202201151433.04afZ5aZ-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3773341a27b69fe38edd03823be246aa38001e6a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dmitry-Antipov/Add-spi-hid-transport-for-HID-over-SPI-bus/20220115-103254
git checkout 3773341a27b69fe38edd03823be246aa38001e6a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/hid/spi-hid/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/hid/spi-hid/spi-hid-core.c:203:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
203 | static const char *const spi_hid_power_mode_string(u8 power_state)
| ^~~~~
--
In file included from include/trace/define_trace.h:102,
from drivers/hid/spi-hid/spi-hid_trace.h:198,
from drivers/hid/spi-hid/trace.c:11:
drivers/hid/spi-hid/./spi-hid_trace.h: In function 'trace_raw_output_spi_hid':
>> drivers/hid/spi-hid/./spi-hid_trace.h:144:19: warning: format '%d' expects a matching 'int' argument [-Wformat=]
144 | TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/trace/trace_events.h:399:34: note: in definition of macro 'DECLARE_EVENT_CLASS'
399 | trace_event_printf(iter, print); \
| ^~~~~
drivers/hid/spi-hid/./spi-hid_trace.h:144:9: note: in expansion of macro 'TP_printk'
144 | TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
| ^~~~~~~~~
In file included from include/trace/trace_events.h:433,
from include/trace/define_trace.h:102,
from drivers/hid/spi-hid/spi-hid_trace.h:198,
from drivers/hid/spi-hid/trace.c:11:
drivers/hid/spi-hid/./spi-hid_trace.h:144:102: note: format string is defined here
144 | TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
| ~^
| |
| int
--
>> drivers/hid/spi-hid/trace.c:10: warning: expecting prototype for trace.c(). Prototype was for CREATE_TRACE_POINTS() instead
--
>> drivers/hid/spi-hid/spi-hid-core.c:378: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Handle the reset response from the FW by sending a request for the device
vim +203 drivers/hid/spi-hid/spi-hid-core.c
202
> 203 static const char *const spi_hid_power_mode_string(u8 power_state)
204 {
205 switch (power_state) {
206 case SPI_HID_POWER_MODE_ON:
207 return "d0";
208 case SPI_HID_POWER_MODE_SLEEP:
209 return "d2";
210 case SPI_HID_POWER_MODE_OFF:
211 return "d3";
212 case SPI_HID_POWER_MODE_WAKING_SLEEP:
213 return "d3*";
214 default:
215 return "unknown";
216 }
217 }
218
219 static void spi_hid_suspend(struct spi_hid *shid)
220 {
221 struct device *dev = &shid->spi->dev;
222
223 if (shid->power_state == SPI_HID_POWER_MODE_OFF)
224 return;
225
226 disable_irq(shid->spi->irq);
227 shid->ready = false;
228 sysfs_notify(&dev->kobj, NULL, "ready");
229
230 spi_hid_of_assert_reset(&shid->conf);
231
232 shid->power_state = SPI_HID_POWER_MODE_OFF;
233 }
234
235 static void spi_hid_resume(struct spi_hid *shid)
236 {
237 if (shid->power_state == SPI_HID_POWER_MODE_ON)
238 return;
239
240 shid->power_state = SPI_HID_POWER_MODE_ON;
241 enable_irq(shid->spi->irq);
242 shid->input_transfer_pending = 0;
243
244 spi_hid_of_deassert_reset(&shid->conf);
245 }
246
247 static struct hid_device *spi_hid_disconnect_hid(struct spi_hid *shid)
248 {
249 struct hid_device *hid = shid->hid;
250
251 shid->hid = NULL;
252
253 return hid;
254 }
255
256 static void spi_hid_stop_hid(struct spi_hid *shid)
257 {
258 struct hid_device *hid;
259
260 hid = spi_hid_disconnect_hid(shid);
261 if (hid) {
262 cancel_work_sync(&shid->create_device_work);
263 cancel_work_sync(&shid->refresh_device_work);
264 hid_destroy_device(hid);
265 }
266 }
267
268 static void spi_hid_error_handler(struct spi_hid *shid)
269 {
270 struct device *dev = &shid->spi->dev;
271 int ret;
272
273 if (shid->power_state == SPI_HID_POWER_MODE_OFF)
274 return;
275
276 if (shid->attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
277 dev_err(dev, "unresponsive device, aborting.\n");
278 spi_hid_stop_hid(shid);
279 spi_hid_of_assert_reset(&shid->conf);
280 ret = spi_hid_of_power_down(&shid->conf);
281 if (ret) {
282 dev_err(dev, "failed to disable regulator\n");
283 shid->regulator_error_count++;
284 shid->regulator_last_error = ret;
285 }
286 return;
287 }
288
289 trace_spi_hid_error_handler(shid);
290
291 shid->ready = false;
292 sysfs_notify(&dev->kobj, NULL, "ready");
293
294 spi_hid_of_assert_reset(&shid->conf);
295
296 shid->power_state = SPI_HID_POWER_MODE_OFF;
297 shid->input_transfer_pending = 0;
298 cancel_work_sync(&shid->reset_work);
299
300 spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
301
302 shid->power_state = SPI_HID_POWER_MODE_ON;
303
304 spi_hid_of_deassert_reset(&shid->conf);
305 }
306
307 static void spi_hid_error_work(struct work_struct *work)
308 {
309 struct spi_hid *shid = container_of(work, struct spi_hid, error_work);
310
311 spi_hid_error_handler(shid);
312 }
313
314 static int spi_hid_send_output_report(struct spi_hid *shid,
315 struct spi_hid_output_report *report)
316 {
317 struct spi_hid_output_buf *buf = &shid->output;
318 struct device *dev = &shid->spi->dev;
319 u16 report_length;
320 u16 padded_length;
321 u8 padding;
322 int ret;
323
324 if (report->content_length > shid->desc.max_output_length) {
325 dev_err(dev, "Output report too big, content_length 0x%x\n",
326 report->content_length);
327 ret = -E2BIG;
328 goto out;
329 }
330
331 spi_hid_populate_output_header(buf->header, &shid->conf, report);
332
333 if (report->content_length)
334 memcpy(&buf->content, report->content, report->content_length);
335
336 report_length = sizeof(buf->header) + report->content_length;
337 padded_length = round_up(report_length, 4);
338 padding = padded_length - report_length;
339 memset(&buf->content[report->content_length], 0, padding);
340
341 ret = spi_hid_output(shid, buf, padded_length);
342 if (ret) {
343 dev_err(dev, "Failed output transfer\n");
344 goto out;
345 }
346
347 return 0;
348
349 out:
350 return ret;
351 }
352
353 static int spi_hid_sync_request(struct spi_hid *shid,
354 struct spi_hid_output_report *report)
355 {
356 struct device *dev = &shid->spi->dev;
357 int ret = 0;
358
359 ret = spi_hid_send_output_report(shid, report);
360 if (ret) {
361 dev_err(dev, "Failed to transfer output report\n");
362 return ret;
363 }
364
365 mutex_unlock(&shid->lock);
366 ret = wait_for_completion_interruptible_timeout(&shid->output_done,
367 msecs_to_jiffies(1000));
368 mutex_lock(&shid->lock);
369 if (ret == 0) {
370 dev_err(dev, "Response timed out\n");
371 return -ETIMEDOUT;
372 }
373
374 return 0;
375 }
376
377 /**
> 378 * Handle the reset response from the FW by sending a request for the device
379 * descriptor.
380 */
381 static void spi_hid_reset_work(struct work_struct *work)
382 {
383 struct spi_hid *shid =
384 container_of(work, struct spi_hid, reset_work);
385 struct device *dev = &shid->spi->dev;
386 struct spi_hid_output_report report = {
387 .report_type = SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST,
388 .content_length = 0x0,
389 .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
390 .content = NULL,
391 };
392 int ret;
393
394 trace_spi_hid_reset_work(shid);
395
396 if (shid->ready) {
397 dev_err(dev, "Spontaneous FW reset!");
398 shid->ready = false;
399 sysfs_notify(&dev->kobj, NULL, "ready");
400 shid->dir_count++;
401 }
402
403 if (shid->power_state == SPI_HID_POWER_MODE_OFF)
404 return;
405
406 if (flush_work(&shid->create_device_work))
407 dev_err(dev, "Reset handler waited for create_device_work");
408
409 if (flush_work(&shid->refresh_device_work))
410 dev_err(dev, "Reset handler waited for refresh_device_work");
411
412 mutex_lock(&shid->lock);
413 ret = spi_hid_sync_request(shid, &report);
414 mutex_unlock(&shid->lock);
415 if (ret) {
416 dev_WARN_ONCE(dev, true,
417 "Failed to send device descriptor request\n");
418 spi_hid_error_handler(shid);
419 }
420 }
421
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus
@ 2022-01-15 6:10 ` kernel test robot
0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2022-01-15 6:10 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 11178 bytes --]
Hi Dmitry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hid/for-next]
[also build test WARNING on jikos-trivial/for-next v5.16 next-20220114]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dmitry-Antipov/Add-spi-hid-transport-for-HID-over-SPI-bus/20220115-103254
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20220115/202201151433.04afZ5aZ-lkp(a)intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3773341a27b69fe38edd03823be246aa38001e6a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dmitry-Antipov/Add-spi-hid-transport-for-HID-over-SPI-bus/20220115-103254
git checkout 3773341a27b69fe38edd03823be246aa38001e6a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/hid/spi-hid/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/hid/spi-hid/spi-hid-core.c:203:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
203 | static const char *const spi_hid_power_mode_string(u8 power_state)
| ^~~~~
--
In file included from include/trace/define_trace.h:102,
from drivers/hid/spi-hid/spi-hid_trace.h:198,
from drivers/hid/spi-hid/trace.c:11:
drivers/hid/spi-hid/./spi-hid_trace.h: In function 'trace_raw_output_spi_hid':
>> drivers/hid/spi-hid/./spi-hid_trace.h:144:19: warning: format '%d' expects a matching 'int' argument [-Wformat=]
144 | TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/trace/trace_events.h:399:34: note: in definition of macro 'DECLARE_EVENT_CLASS'
399 | trace_event_printf(iter, print); \
| ^~~~~
drivers/hid/spi-hid/./spi-hid_trace.h:144:9: note: in expansion of macro 'TP_printk'
144 | TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
| ^~~~~~~~~
In file included from include/trace/trace_events.h:433,
from include/trace/define_trace.h:102,
from drivers/hid/spi-hid/spi-hid_trace.h:198,
from drivers/hid/spi-hid/trace.c:11:
drivers/hid/spi-hid/./spi-hid_trace.h:144:102: note: format string is defined here
144 | TP_printk("spi%d.%d: (%04x:%04x v%d) HID v%d.%d state i:%d p:%d len i:%d o:%d r:%d flags %c:%d",
| ~^
| |
| int
--
>> drivers/hid/spi-hid/trace.c:10: warning: expecting prototype for trace.c(). Prototype was for CREATE_TRACE_POINTS() instead
--
>> drivers/hid/spi-hid/spi-hid-core.c:378: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Handle the reset response from the FW by sending a request for the device
vim +203 drivers/hid/spi-hid/spi-hid-core.c
202
> 203 static const char *const spi_hid_power_mode_string(u8 power_state)
204 {
205 switch (power_state) {
206 case SPI_HID_POWER_MODE_ON:
207 return "d0";
208 case SPI_HID_POWER_MODE_SLEEP:
209 return "d2";
210 case SPI_HID_POWER_MODE_OFF:
211 return "d3";
212 case SPI_HID_POWER_MODE_WAKING_SLEEP:
213 return "d3*";
214 default:
215 return "unknown";
216 }
217 }
218
219 static void spi_hid_suspend(struct spi_hid *shid)
220 {
221 struct device *dev = &shid->spi->dev;
222
223 if (shid->power_state == SPI_HID_POWER_MODE_OFF)
224 return;
225
226 disable_irq(shid->spi->irq);
227 shid->ready = false;
228 sysfs_notify(&dev->kobj, NULL, "ready");
229
230 spi_hid_of_assert_reset(&shid->conf);
231
232 shid->power_state = SPI_HID_POWER_MODE_OFF;
233 }
234
235 static void spi_hid_resume(struct spi_hid *shid)
236 {
237 if (shid->power_state == SPI_HID_POWER_MODE_ON)
238 return;
239
240 shid->power_state = SPI_HID_POWER_MODE_ON;
241 enable_irq(shid->spi->irq);
242 shid->input_transfer_pending = 0;
243
244 spi_hid_of_deassert_reset(&shid->conf);
245 }
246
247 static struct hid_device *spi_hid_disconnect_hid(struct spi_hid *shid)
248 {
249 struct hid_device *hid = shid->hid;
250
251 shid->hid = NULL;
252
253 return hid;
254 }
255
256 static void spi_hid_stop_hid(struct spi_hid *shid)
257 {
258 struct hid_device *hid;
259
260 hid = spi_hid_disconnect_hid(shid);
261 if (hid) {
262 cancel_work_sync(&shid->create_device_work);
263 cancel_work_sync(&shid->refresh_device_work);
264 hid_destroy_device(hid);
265 }
266 }
267
268 static void spi_hid_error_handler(struct spi_hid *shid)
269 {
270 struct device *dev = &shid->spi->dev;
271 int ret;
272
273 if (shid->power_state == SPI_HID_POWER_MODE_OFF)
274 return;
275
276 if (shid->attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
277 dev_err(dev, "unresponsive device, aborting.\n");
278 spi_hid_stop_hid(shid);
279 spi_hid_of_assert_reset(&shid->conf);
280 ret = spi_hid_of_power_down(&shid->conf);
281 if (ret) {
282 dev_err(dev, "failed to disable regulator\n");
283 shid->regulator_error_count++;
284 shid->regulator_last_error = ret;
285 }
286 return;
287 }
288
289 trace_spi_hid_error_handler(shid);
290
291 shid->ready = false;
292 sysfs_notify(&dev->kobj, NULL, "ready");
293
294 spi_hid_of_assert_reset(&shid->conf);
295
296 shid->power_state = SPI_HID_POWER_MODE_OFF;
297 shid->input_transfer_pending = 0;
298 cancel_work_sync(&shid->reset_work);
299
300 spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
301
302 shid->power_state = SPI_HID_POWER_MODE_ON;
303
304 spi_hid_of_deassert_reset(&shid->conf);
305 }
306
307 static void spi_hid_error_work(struct work_struct *work)
308 {
309 struct spi_hid *shid = container_of(work, struct spi_hid, error_work);
310
311 spi_hid_error_handler(shid);
312 }
313
314 static int spi_hid_send_output_report(struct spi_hid *shid,
315 struct spi_hid_output_report *report)
316 {
317 struct spi_hid_output_buf *buf = &shid->output;
318 struct device *dev = &shid->spi->dev;
319 u16 report_length;
320 u16 padded_length;
321 u8 padding;
322 int ret;
323
324 if (report->content_length > shid->desc.max_output_length) {
325 dev_err(dev, "Output report too big, content_length 0x%x\n",
326 report->content_length);
327 ret = -E2BIG;
328 goto out;
329 }
330
331 spi_hid_populate_output_header(buf->header, &shid->conf, report);
332
333 if (report->content_length)
334 memcpy(&buf->content, report->content, report->content_length);
335
336 report_length = sizeof(buf->header) + report->content_length;
337 padded_length = round_up(report_length, 4);
338 padding = padded_length - report_length;
339 memset(&buf->content[report->content_length], 0, padding);
340
341 ret = spi_hid_output(shid, buf, padded_length);
342 if (ret) {
343 dev_err(dev, "Failed output transfer\n");
344 goto out;
345 }
346
347 return 0;
348
349 out:
350 return ret;
351 }
352
353 static int spi_hid_sync_request(struct spi_hid *shid,
354 struct spi_hid_output_report *report)
355 {
356 struct device *dev = &shid->spi->dev;
357 int ret = 0;
358
359 ret = spi_hid_send_output_report(shid, report);
360 if (ret) {
361 dev_err(dev, "Failed to transfer output report\n");
362 return ret;
363 }
364
365 mutex_unlock(&shid->lock);
366 ret = wait_for_completion_interruptible_timeout(&shid->output_done,
367 msecs_to_jiffies(1000));
368 mutex_lock(&shid->lock);
369 if (ret == 0) {
370 dev_err(dev, "Response timed out\n");
371 return -ETIMEDOUT;
372 }
373
374 return 0;
375 }
376
377 /**
> 378 * Handle the reset response from the FW by sending a request for the device
379 * descriptor.
380 */
381 static void spi_hid_reset_work(struct work_struct *work)
382 {
383 struct spi_hid *shid =
384 container_of(work, struct spi_hid, reset_work);
385 struct device *dev = &shid->spi->dev;
386 struct spi_hid_output_report report = {
387 .report_type = SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST,
388 .content_length = 0x0,
389 .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
390 .content = NULL,
391 };
392 int ret;
393
394 trace_spi_hid_reset_work(shid);
395
396 if (shid->ready) {
397 dev_err(dev, "Spontaneous FW reset!");
398 shid->ready = false;
399 sysfs_notify(&dev->kobj, NULL, "ready");
400 shid->dir_count++;
401 }
402
403 if (shid->power_state == SPI_HID_POWER_MODE_OFF)
404 return;
405
406 if (flush_work(&shid->create_device_work))
407 dev_err(dev, "Reset handler waited for create_device_work");
408
409 if (flush_work(&shid->refresh_device_work))
410 dev_err(dev, "Reset handler waited for refresh_device_work");
411
412 mutex_lock(&shid->lock);
413 ret = spi_hid_sync_request(shid, &report);
414 mutex_unlock(&shid->lock);
415 if (ret) {
416 dev_WARN_ONCE(dev, true,
417 "Failed to send device descriptor request\n");
418 spi_hid_error_handler(shid);
419 }
420 }
421
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/5] Documentation: DT bindings for HID over SPI.
2022-01-15 2:31 ` [PATCH v3 3/5] Documentation: DT bindings for HID over SPI Dmitry Antipov
@ 2022-01-20 20:06 ` Rob Herring
2022-02-25 1:34 ` [EXTERNAL] " Dmitry Antipov
2022-01-24 10:32 ` Felipe Balbi
1 sibling, 1 reply; 13+ messages in thread
From: Rob Herring @ 2022-01-20 20:06 UTC (permalink / raw)
To: Dmitry Antipov
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Mark Brown,
Felipe Balbi, linux-input, devicetree, linux-spi, Dmitry Antipov
On Fri, Jan 14, 2022 at 06:31:33PM -0800, Dmitry Antipov wrote:
> From: Dmitry Antipov <dmanti@microsoft.com>
>
> Added documentation describes the required properties for implementing
> Device Tree for a device supporting HID over SPI and also provides an
> example.
Bindings are in DT schema format now.
>
> Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> ---
> .../bindings/input/hid-over-spi.txt | 43 +++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100755 Documentation/devicetree/bindings/input/hid-over-spi.txt
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.txt b/Documentation/devicetree/bindings/input/hid-over-spi.txt
> new file mode 100755
> index 000000000000..5eba95b5724e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/hid-over-spi.txt
> @@ -0,0 +1,43 @@
> +* HID over SPI Device-Tree bindings
> +
> +HID over SPI provides support for Human Interface Devices over the SPI bus. HID
> +Over SPI Protocol Specification 1.0 was written by Microsoft and is available at
> +https://www.microsoft.com/en-us/download/details.aspx?id=103325.
> +
> +If this binding is used, the kernel module spi-hid will handle the communication
What's a kernel module? ;) Bindings are OS independent (or supposed to
be), so kernel details do not belong here.
> +with the device and the generic hid core layer will handle the protocol.
> +
> +Required properties:
> +- compatible: must be "hid-over-spi"
Bindings describe devices, not protocols. There is no such
'hid-over-spi' device. Please see the existing hid-over-i2c binding.
It's fine to have this compatible, but only as a fallback compatible.
> +- interrupts: interrupt line
> +- vdd-supply: phandle of the regulator that provides the supply voltage
What happens when the device has 2 supplies? And there's some timing
requirement between them?
> +- reset_gpio-gpios: gpio wired to the device reset line
'reset-gpios' is the standard name.
> +- post-power-on-delay-ms: time required by the device after enabling its
> +regulators or powering it on, before it is ready for communication
> +- minimal-reset-delay-ms: minimum amount of time that device needs to be in
> +reset state for the reset to take effect
These properties are what happens when we try to do generic bindings.
It's a never-ending addition of more properties to try to describe
(poorly) the power sequencing requirements.
> +- input-report-header-address: this property and the rest are described in HID
> +Over SPI Protocol Spec 1.0
> +- input-report-body-address
> +- output-report-address
> +- read-opcode
> +- write-opcode
> +- flags
A bit too generic. We generally want to avoid having a property name
with 2 different types/meaning. It's not something we check for yet, but
I plan to at some point.
> +
> +Example:
> + spi-hid-dev0 {
> + compatible = "hid-over-spi";
> + reg = <0>;
> + interrupts-extended = <&tlmm 42 IRQ_TYPE_EDGE_FALLING>;
> + vdd-supply = <&pm8350c_l3>;
> + input-report-header-address = <0x1000>;
> + input-report-body-address = <0x1004>;
> + output-report-address = <0x2000>;
> + read-opcode = <0x0b>;
> + write-opcode = <0x02>;
> + flags = <0x00>;
> + reset_gpio-gpios = <&tlmm 25 GPIO_ACTIVE_LOW>;
> + post-power-on-delay-ms = <5>;
> + minimal-reset-delay-ms = <5>;
> +
> + };
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/5] Documentation: DT bindings for HID over SPI.
2022-01-15 2:31 ` [PATCH v3 3/5] Documentation: DT bindings for HID over SPI Dmitry Antipov
2022-01-20 20:06 ` Rob Herring
@ 2022-01-24 10:32 ` Felipe Balbi
1 sibling, 0 replies; 13+ messages in thread
From: Felipe Balbi @ 2022-01-24 10:32 UTC (permalink / raw)
To: Dmitry Antipov
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Mark Brown, linux-input, devicetree, linux-spi, Dmitry Antipov
Dmitry Antipov <daantipov@gmail.com> writes:
> From: Dmitry Antipov <dmanti@microsoft.com>
>
> Added documentation describes the required properties for implementing
> Device Tree for a device supporting HID over SPI and also provides an
> example.
>
> Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> ---
> .../bindings/input/hid-over-spi.txt | 43 +++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100755 Documentation/devicetree/bindings/input/hid-over-spi.txt
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.txt b/Documentation/devicetree/bindings/input/hid-over-spi.txt
> new file mode 100755
> index 000000000000..5eba95b5724e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/hid-over-spi.txt
> @@ -0,0 +1,43 @@
> +* HID over SPI Device-Tree bindings
> +
Some windows-style line endings leaked here. Perhaps run dos2unix on the file?
--
balbi
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [EXTERNAL] Re: [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus
2022-01-15 4:32 ` Dmitry Torokhov
@ 2022-02-25 1:20 ` Dmitry Antipov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-02-25 1:20 UTC (permalink / raw)
To: Dmitry Torokhov, Dmitry Antipov
Cc: Jiri Kosina, Benjamin Tissoires, Rob Herring, Mark Brown,
Felipe Balbi, linux-input, devicetree, linux-spi
Hello Dmitry,
Thank you for your comments. Most of your feedback was incorporated into
the v4 patch that I just submitted. I appreciate your help and apologize
for delay in my response.
On Fri, Jan 14, 2022 at 8:33 PM, Dmitry Torokhov wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Hi Dmitry,
>
> On Fri, Jan 14, 2022 at 06:31:35PM -0800, Dmitry Antipov wrote:
> > From: Dmitry Antipov <dmanti@microsoft.com>
> >
> > This driver follows HID Over SPI Protocol Specification 1.0 available at
> >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.m
> icrosoft.com%2Fen-
> us%2Fdownload%2Fdetails.aspx%3Fid%3D103325&data=04%7C01%7Cd
> manti%40microsoft.com%7C33c7f0f39c9349ef7d1d08d9d7e01d17%7C72f988
> bf86f141af91ab2d7cd011db47%7C1%7C0%7C637778180034947738%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C2000&sdata=4bdVX7cTKLt7dDBxxsrs3EG1j8k
> BE6h6dBWncpwH7EI%3D&reserved=0. 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.
>
> The driver does not use threaded interrupts and instead uses a lot of
> work structs, which makes it very complex. Please consider switching
> over to threaded interrupt handler and see if some of the work can be
> done in-place, I believe it will simplify the driver significantly.
The v4 patch that I just submitted uses a threaded interrupt handler
and spi_sync instead of spi_async and is a bit simpler. The four worker
threads are still needed and their work can't be done in place. Three of
them (reset_response, create, and refresh) send sync_request() which
requires an interrupt with a response to be handled. The error_work one
toggles power and would be racy if done in-place.
>
> More random comments below.
>
> >
> > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> > ---
> > arch/arm64/configs/defconfig | 1 +
> > drivers/hid/Kconfig | 2 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/spi-hid/Kconfig | 25 +
> > drivers/hid/spi-hid/Makefile | 12 +
> > drivers/hid/spi-hid/spi-hid-core.c | 1328 +++++++++++++++++++++++++++
> > drivers/hid/spi-hid/spi-hid-core.h | 188 ++++
> > drivers/hid/spi-hid/spi-hid-of.c | 148 +++
> > drivers/hid/spi-hid/spi-hid-of.h | 34 +
> > drivers/hid/spi-hid/spi-hid_trace.h | 198 ++++
> > drivers/hid/spi-hid/trace.c | 11 +
> > 11 files changed, 1948 insertions(+)
> > create mode 100644 drivers/hid/spi-hid/Kconfig
> > create mode 100644 drivers/hid/spi-hid/Makefile
> > create mode 100644 drivers/hid/spi-hid/spi-hid-core.c
> > create mode 100644 drivers/hid/spi-hid/spi-hid-core.h
> > create mode 100755 drivers/hid/spi-hid/spi-hid-of.c
> > create mode 100755 drivers/hid/spi-hid/spi-hid-of.h
> > create mode 100644 drivers/hid/spi-hid/spi-hid_trace.h
> > create mode 100644 drivers/hid/spi-hid/trace.c
> >
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index f2e2b9bdd702..25249a4b0c8a 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -805,6 +805,7 @@ CONFIG_SND_AUDIO_GRAPH_CARD=m
> > CONFIG_HID_MULTITOUCH=m
> > CONFIG_I2C_HID_ACPI=m
> > CONFIG_I2C_HID_OF=m
> > +CONFIG_SPI_HID=m
> > CONFIG_USB_CONN_GPIO=m
> > CONFIG_USB=y
> > CONFIG_USB_OTG=y
>
> This chunk does not belong to this patch.
Moved to a separate patch in v4.
>
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index a7c78ac96270..cd2c10703fcf 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -1262,6 +1262,8 @@ source "drivers/hid/usbhid/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 55a6fa3eca5a..caf418dda343 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -144,6 +144,7 @@ obj-$(CONFIG_USB_MOUSE) += usbhid/
> > obj-$(CONFIG_USB_KBD) += usbhid/
> >
> > obj-$(CONFIG_I2C_HID_CORE) += i2c-hid/
> > +obj-$(CONFIG_SPI_HID) += spi-hid/
> >
> > obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
> > obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/
> > diff --git a/drivers/hid/spi-hid/Kconfig b/drivers/hid/spi-hid/Kconfig
> > new file mode 100644
> > index 000000000000..37302d658162
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/Kconfig
> > @@ -0,0 +1,25 @@
> > +#
> > +# Copyright (c) 2021 Microsoft Corporation
> > +#
> > +# This program is free software; you can redistribute it and/or modify it
> > +# under the terms of the GNU General Public License version 2 as published
> by
> > +# the Free Software Foundation.
> > +#
> > +menu "SPI HID support"
> > + depends on SPI
> > +
> > +config SPI_HID
> > + tristate "HID over SPI transport layer"
> > + default n
> > + depends on SPI && INPUT && OF
> > + select HID
> > + help
> > + Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
> > + other HID based devices which is connected to your computer via SPI.
> > +
> > + If unsure, say N.
> > +
> > + This support is also available as a module. If so, the module
> > + will be called spi-hid.
> > +
> > +endmenu
> > diff --git a/drivers/hid/spi-hid/Makefile b/drivers/hid/spi-hid/Makefile
> > new file mode 100644
> > index 000000000000..0d34a04b5dc7
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/Makefile
> > @@ -0,0 +1,12 @@
> > +#
> > +# Copyright (c) 2021 Microsoft Corporation
> > +#
> > +# This program is free software; you can redistribute it and/or modify it
> > +# under the terms of the GNU General Public License version 2 as published
> by
> > +# the Free Software Foundation.
> > +#
> > +# Makefile for the SPI input drivers
> > +#
> > +CFLAGS_trace.o = -I$(src)
> > +obj-$(CONFIG_SPI_HID) += spi-hid.o
> > +spi-hid-objs := spi-hid-core.o spi-hid-of.o trace.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..4d5a24613beb
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/spi-hid-core.c
> > @@ -0,0 +1,1328 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * HID over SPI protocol implementation
> > + * spi-hid-core.c
>
> I recommend not to put file names in comments as they tend to get stale.
Addressed in v4.
>
> > + *
> > + * Copyright (c) 2021 Microsoft Corporation
> > + *
> > + * 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/crc32.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/hid.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/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/string.h>
> > +#include <linux/wait.h>
> > +#include <linux/workqueue.h>
> > +
> > +#include "spi-hid-core.h"
> > +#include "spi-hid_trace.h"
> > +#include "spi-hid-of.h"
> > +#include "../hid-ids.h"
> > +
> > +#define SPI_HID_MAX_RESET_ATTEMPTS 3
> > +
> > +static struct hid_ll_driver spi_hid_ll_driver;
> > +
> > +static void spi_hid_populate_read_approvals(struct spi_hid_of_config *conf,
> > + __u8 *header_buf, __u8 *body_buf)
>
> I think this is largely matter of preference, but usually in kernel we
> use u8, u16, etc, and variants with underscores are used in headers
> exported to userspace.
Addressed in v4.
>
> > +{
> > + header_buf[0] = conf->read_opcode;
> > + header_buf[1] = (conf->input_report_header_address >> 16) & 0xff;
> > + header_buf[2] = (conf->input_report_header_address >> 8) & 0xff;
> > + header_buf[3] = (conf->input_report_header_address >> 0) & 0xff;
> > + header_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
> > +
> > + body_buf[0] = conf->read_opcode;
> > + body_buf[1] = (conf->input_report_body_address >> 16) & 0xff;
> > + body_buf[2] = (conf->input_report_body_address >> 8) & 0xff;
> > + body_buf[3] = (conf->input_report_body_address >> 0) & 0xff;
> > + body_buf[4] = SPI_HID_READ_APPROVAL_CONSTANT;
> > +}
> > +
> > +static void spi_hid_parse_dev_desc(struct spi_hid_device_desc_raw *raw,
> > + struct spi_hid_device_descriptor
> *desc)
> > +{
> > + desc->hid_version = le16_to_cpu(raw->bcdVersion);
> > + desc->report_descriptor_length = le16_to_cpu(raw-
> >wReportDescLength);
> > + desc->max_input_length = le16_to_cpu(raw->wMaxInputLength);
> > + desc->max_output_length = le16_to_cpu(raw->wMaxOutputLength);
> > +
> > + /* FIXME: multi-fragment not supported, field below not used */
> > + desc->max_fragment_length = le16_to_cpu(raw-
> >wMaxFragmentLength);
> > +
> > + desc->vendor_id = le16_to_cpu(raw->wVendorID);
> > + desc->product_id = le16_to_cpu(raw->wProductID);
> > + desc->version_id = le16_to_cpu(raw->wVersionID);
> > + desc->no_output_report_ack = le16_to_cpu(raw->wFlags) & BIT(0);
> > +}
> > +
> > +static void spi_hid_populate_input_header(__u8 *buf,
> > + struct spi_hid_input_header *header)
> > +{
> > + header->version = buf[0] & 0xf;
> > + header->report_length = (buf[1] | ((buf[2] & 0x3f) << 8)) * 4;
> > + header->last_fragment_flag = (buf[2] & 0x40) >> 6;
> > + header->sync_const = buf[3];
> > +}
> > +
> > +static void spi_hid_populate_input_body(__u8 *buf,
> > + struct spi_hid_input_body *body)
> > +{
> > + body->report_type = buf[0];
> > + body->content_length = buf[1] | (buf[2] << 8);
>
> Where possible we should use
> get_unaligned_le16/put_unaligned_le16/cpu_to_le16/le16_to_cpu/etc.
> It documents data structures better and may result in slightly better
> performance.
Addressed in v4.
>
> > + body->content_id = buf[3];
> > +}
> > +
> > +static void spi_hid_input_report_prepare(struct spi_hid_input_buf *buf,
> > + struct spi_hid_input_report *report)
> > +{
> > + struct spi_hid_input_header header;
> > + struct spi_hid_input_body body;
> > +
> > + spi_hid_populate_input_header(buf->header, &header);
> > + spi_hid_populate_input_body(buf->body, &body);
> > + report->report_type = body.report_type;
> > + report->content_length = body.content_length;
> > + report->content_id = body.content_id;
> > + report->content = buf->content;
> > +}
> > +
> > +static void spi_hid_populate_output_header(__u8 *buf,
> > + struct spi_hid_of_config *conf,
> > + struct spi_hid_output_report *report)
> > +{
> > + buf[0] = conf->write_opcode;
> > + buf[1] = (conf->output_report_address >> 16) & 0xff;
> > + buf[2] = (conf->output_report_address >> 8) & 0xff;
> > + buf[3] = (conf->output_report_address >> 0) & 0xff;
> > + buf[4] = report->report_type;
> > + buf[5] = report->content_length & 0xff;
> > + buf[6] = (report->content_length >> 8) & 0xff;
>
> le16...
Not sure I follow, these are all 8-bit assignments.
>
> > + buf[7] = report->content_id;
> > +}
> > +
> > +static int spi_hid_input_async(struct spi_hid *shid, void *buf, u16 length,
> > + void (*complete)(void *), bool is_header)
> > +{
> > + int ret;
> > + struct device *dev = &shid->spi->dev;
> > +
> > + 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);
> > +
> > + shid->input_message.complete = complete;
> > + shid->input_message.context = shid;
> > +
> > + trace_spi_hid_input_async(shid,
> > + shid->input_transfer[0].tx_buf,
> > + shid->input_transfer[0].len,
> > + shid->input_transfer[1].rx_buf,
> > + shid->input_transfer[1].len, 0);
> > +
> > + ret = spi_async(shid->spi, &shid->input_message);
> > + if (ret) {
> > + dev_err(dev, "Error starting async transfer: %d, resetting\n",
> > + ret);
> > + shid->bus_error_count++;
> > + shid->bus_last_error = ret;
> > + schedule_work(&shid->error_work);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int spi_hid_output(struct spi_hid *shid, void *buf, u16 length)
> > +{
> > + struct spi_transfer transfer;
> > + struct spi_message message;
> > + int ret;
> > +
> > + memset(&transfer, 0, sizeof(transfer));
> > +
> > + transfer.tx_buf = buf;
> > + transfer.len = length;
> > +
> > + spi_message_init_with_transfers(&message, &transfer, 1);
> > +
> > + /*
> > + * REVISIT: Should output be asynchronous?
> > + *
> > + * According to Documentation/hid/hid-transport.rst, ->output_report()
> > + * must be implemented as an asynchronous operation.
> > + */
>
> I am not quite sure what that doc means. Do they simply mean that the
> driver should not wait for response from the device?
>
> I think that transfer (send) can still happen synchronously.
This revisit comment was based on incorrect documentation and has been
removed in v4.
>
> > + trace_spi_hid_output_begin(shid, transfer.tx_buf,
> > + transfer.len, NULL, 0, 0);
> > +
> > + ret = spi_sync(shid->spi, &message);
> > +
> > + trace_spi_hid_output_end(shid, transfer.tx_buf,
> > + transfer.len, NULL, 0, ret);
> > +
> > + if (ret) {
> > + shid->bus_error_count++;
> > + shid->bus_last_error = ret;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const char *const spi_hid_power_mode_string(u8 power_state)
> > +{
> > + switch (power_state) {
> > + case SPI_HID_POWER_MODE_ON:
> > + return "d0";
> > + case SPI_HID_POWER_MODE_SLEEP:
> > + return "d2";
> > + case SPI_HID_POWER_MODE_OFF:
> > + return "d3";
> > + case SPI_HID_POWER_MODE_WAKING_SLEEP:
> > + return "d3*";
> > + default:
> > + return "unknown";
> > + }
> > +}
> > +
> > +static void spi_hid_suspend(struct spi_hid *shid)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > +
> > + if (shid->power_state == SPI_HID_POWER_MODE_OFF)
> > + return;
> > +
> > + disable_irq(shid->spi->irq);
> > + shid->ready = false;
> > + sysfs_notify(&dev->kobj, NULL, "ready");
> > +
> > + spi_hid_of_assert_reset(&shid->conf);
>
> Are you sure you want to have reset asserted while the device is
> sleeping? Is it really a reset pin, or is it actually a GPIO-controlled
> regulator?
We do want to keep reset asserted during suspend. In v4 I also added
code to power down on suspend and power up on resume.
>
> > +
> > + shid->power_state = SPI_HID_POWER_MODE_OFF;
> > +}
> > +
> > +static void spi_hid_resume(struct spi_hid *shid)
> > +{
> > + if (shid->power_state == SPI_HID_POWER_MODE_ON)
> > + return;
> > +
> > + shid->power_state = SPI_HID_POWER_MODE_ON;
> > + enable_irq(shid->spi->irq);
> > + shid->input_transfer_pending = 0;
> > +
> > + spi_hid_of_deassert_reset(&shid->conf);
> > +}
> > +
> > +static struct hid_device *spi_hid_disconnect_hid(struct spi_hid *shid)
> > +{
> > + struct hid_device *hid = shid->hid;
> > +
> > + shid->hid = NULL;
>
> Umm, why the wrapper?
We use shid->hid to check if a HID device has been installed.
>
> > +
> > + return hid;
> > +}
> > +
> > +static void spi_hid_stop_hid(struct spi_hid *shid)
> > +{
> > + struct hid_device *hid;
> > +
> > + hid = spi_hid_disconnect_hid(shid);
> > + if (hid) {
> > + cancel_work_sync(&shid->create_device_work);
> > + cancel_work_sync(&shid->refresh_device_work);
> > + hid_destroy_device(hid);
> > + }
> > +}
> > +
> > +static void spi_hid_error_handler(struct spi_hid *shid)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > + int ret;
> > +
> > + if (shid->power_state == SPI_HID_POWER_MODE_OFF)
> > + return;
> > +
> > + if (shid->attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> > + dev_err(dev, "unresponsive device, aborting.\n");
> > + spi_hid_stop_hid(shid);
> > + spi_hid_of_assert_reset(&shid->conf);
> > + ret = spi_hid_of_power_down(&shid->conf);
> > + if (ret) {
> > + dev_err(dev, "failed to disable regulator\n");
> > + shid->regulator_error_count++;
> > + shid->regulator_last_error = ret;
> > + }
> > + return;
> > + }
> > +
> > + trace_spi_hid_error_handler(shid);
> > +
> > + shid->ready = false;
> > + sysfs_notify(&dev->kobj, NULL, "ready");
> > +
> > + spi_hid_of_assert_reset(&shid->conf);
> > +
> > + shid->power_state = SPI_HID_POWER_MODE_OFF;
> > + shid->input_transfer_pending = 0;
> > + cancel_work_sync(&shid->reset_work);
> > +
> > + spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
> > +
> > + shid->power_state = SPI_HID_POWER_MODE_ON;
> > +
> > + spi_hid_of_deassert_reset(&shid->conf);
> > +}
> > +
> > +static void spi_hid_error_work(struct work_struct *work)
> > +{
> > + struct spi_hid *shid = container_of(work, struct spi_hid, error_work);
> > +
> > + spi_hid_error_handler(shid);
> > +}
> > +
> > +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 ret;
> > +
> > + if (report->content_length > shid->desc.max_output_length) {
> > + dev_err(dev, "Output report too big, content_length 0x%x\n",
> > + report->content_length);
> > + ret = -E2BIG;
> > + goto out;
> > + }
> > +
> > + 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);
> > +
> > + ret = spi_hid_output(shid, buf, padded_length);
> > + if (ret) {
> > + dev_err(dev, "Failed output transfer\n");
> > + goto out;
> > + }
> > +
> > + return 0;
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static int spi_hid_sync_request(struct spi_hid *shid,
> > + struct spi_hid_output_report *report)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > + int ret = 0;
> > +
> > + ret = spi_hid_send_output_report(shid, report);
> > + if (ret) {
> > + dev_err(dev, "Failed to transfer output report\n");
> > + return ret;
> > + }
> > +
> > + mutex_unlock(&shid->lock);
> > + ret = wait_for_completion_interruptible_timeout(&shid->output_done,
> > + msecs_to_jiffies(1000));
> > + mutex_lock(&shid->lock);
>
> Where is the completion reinitialized?
Fixed in v4.
>
> > + if (ret == 0) {
> > + dev_err(dev, "Response timed out\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * Handle the reset response from the FW by sending a request for the
> device
> > + * descriptor.
> > + */
> > +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;
> > + struct spi_hid_output_report report = {
> > + .report_type =
> SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST,
> > + .content_length = 0x0,
> > + .content_id =
> SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
> > + .content = NULL,
> > + };
> > + int ret;
> > +
> > + trace_spi_hid_reset_work(shid);
> > +
> > + if (shid->ready) {
> > + dev_err(dev, "Spontaneous FW reset!");
> > + shid->ready = false;
> > + sysfs_notify(&dev->kobj, NULL, "ready");
> > + shid->dir_count++;
> > + }
> > +
> > + if (shid->power_state == SPI_HID_POWER_MODE_OFF)
> > + return;
> > +
> > + if (flush_work(&shid->create_device_work))
> > + dev_err(dev, "Reset handler waited for create_device_work");
> > +
> > + if (flush_work(&shid->refresh_device_work))
> > + dev_err(dev, "Reset handler waited for refresh_device_work");
> > +
> > + mutex_lock(&shid->lock);
> > + ret = spi_hid_sync_request(shid, &report);
> > + mutex_unlock(&shid->lock);
> > + if (ret) {
> > + dev_WARN_ONCE(dev, true,
> > + "Failed to send device descriptor request\n");
> > + spi_hid_error_handler(shid);
> > + }
> > +}
> > +
> > +static int spi_hid_input_report_handler(struct spi_hid *shid,
> > + struct spi_hid_input_buf *buf)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > + struct spi_hid_input_report r;
> > + int ret;
> > +
> > + trace_spi_hid_input_report_handler(shid);
> > +
> > + if (!shid->ready || shid->refresh_in_progress || !shid->hid)
> > + return 0;
> > +
> > + spi_hid_input_report_prepare(buf, &r);
> > +
> > + ret = hid_input_report(shid->hid, HID_INPUT_REPORT,
> > + r.content - 1,
> > + r.content_length + 1, 1);
> > +
> > + if (ret == -ENODEV || ret == -EBUSY) {
> > + dev_err(dev, "ignoring report --> %d\n", ret);
> > + return 0;
> > + } else if (ret) {
> > + dev_err(dev, "Bad input report, error %d\n", ret);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void spi_hid_response_handler(struct spi_hid *shid,
> > + struct spi_hid_input_buf *buf)
> > +{
> > + trace_spi_hid_response_handler(shid);
> > +
> > + /* completion_done returns 0 if there are waiters, otherwise 1 */
> > + if (completion_done(&shid->output_done)) {
> > + dev_err(&shid->spi->dev, "Unexpected response report\n");
> > + } else {
> > + if (shid->input.body[0] ==
> > + SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC
> ||
> > + shid->input.body[0] ==
> > +
> SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP) {
> > + size_t response_length = (shid->input.body[1] |
> > + (shid->input.body[2] << 8)) +
> > + sizeof(shid->input.body);
> > + memcpy(shid->response.body, shid->input.body,
> > + response_length);
> > + }
> > + complete(&shid->output_done);
> > + }
> > +}
> > +
> > +/*
> > + * 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)
> > +{
> > + int ret;
> > + struct device *dev = &shid->spi->dev;
> > + struct spi_hid_output_report report = {
> > + .report_type =
> SPI_HID_OUTPUT_REPORT_TYPE_REPORT_DESC_REQUEST,
> > + .content_length = 0,
> > + .content_id =
> SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
> > + .content = NULL,
> > + };
> > +
> > + ret = spi_hid_sync_request(shid, &report);
> > + if (ret) {
> > + dev_err(dev,
> > + "Expected report descriptor not received! Error %d\n",
> > + ret);
> > + spi_hid_error_handler(shid);
> > + goto out;
> > + }
> > +
> > + ret = (shid->response.body[1] | (shid->response.body[2] << 8));
>
> le16...
>
>
> > + if (ret != shid->desc.report_descriptor_length) {
> > + dev_err(dev,
> > + "Received report descriptor length doesn't match
> device descriptor field, using min of the two\n");
> > + ret = min_t(unsigned int, ret,
> > + shid->desc.report_descriptor_length);
> > + }
> > +out:
> > + return ret;
> > +}
> > +
> > +static void 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_body body;
> > + struct device *dev = &shid->spi->dev;
> > + struct spi_hid_device_desc_raw *raw;
> > + int ret;
> > +
> > + trace_spi_hid_process_input_report(shid);
> > +
> > + spi_hid_populate_input_header(buf->header, &header);
> > + spi_hid_populate_input_body(buf->body, &body);
> > +
> > + if (body.content_length > header.report_length) {
> > + dev_err(dev, "Bad body length %d > %d\n",
> body.content_length,
> > +
> header.report_length);
> > + schedule_work(&shid->error_work);
> > + return;
> > + }
> > +
> > + switch (body.report_type) {
> > + case SPI_HID_INPUT_REPORT_TYPE_DATA:
> > + ret = spi_hid_input_report_handler(shid, buf);
> > + if (ret)
> > + schedule_work(&shid->error_work);
> > + break;
> > + case SPI_HID_INPUT_REPORT_TYPE_RESET_RESP:
> > + schedule_work(&shid->reset_work);
> > + break;
> > + case SPI_HID_INPUT_REPORT_TYPE_DEVICE_DESC:
> > + /* Mark the completion done to avoid timeout */
> > + spi_hid_response_handler(shid, buf);
> > +
> > + /* Reset attempts at every device descriptor fetch */
> > + shid->attempts = 0;
> > +
> > + raw = (struct spi_hid_device_desc_raw *)buf->content;
> > +
> > + /* Validate device descriptor length before parsing */
> > + if (body.content_length !=
> SPI_HID_DEVICE_DESCRIPTOR_LENGTH) {
> > + dev_err(dev,
> > + "Invalid content length %d, expected %d\n",
> > + body.content_length,
> > + SPI_HID_DEVICE_DESCRIPTOR_LENGTH);
> > + schedule_work(&shid->error_work);
> > + break;
> > + }
> > +
> > + if (le16_to_cpu(raw->wDeviceDescLength) !=
> > +
> SPI_HID_DEVICE_DESCRIPTOR_LENGTH) {
> > + dev_err(dev,
> > + "Invalid wDeviceDescLength %d, expected
> %d\n",
> > + raw->wDeviceDescLength,
> > + SPI_HID_DEVICE_DESCRIPTOR_LENGTH);
> > + schedule_work(&shid->error_work);
> > + break;
> > + }
> > +
> > + spi_hid_parse_dev_desc(raw, &shid->desc);
> > +
> > + if (shid->desc.hid_version != SPI_HID_SUPPORTED_VERSION) {
> > + dev_err(dev,
> > + "Unsupported device descriptor version
> %4x\n",
> > + shid->desc.hid_version);
> > + schedule_work(&shid->error_work);
> > + break;
> > + }
> > +
> > + if (!shid->hid)
> > + schedule_work(&shid->create_device_work);
> > + else
> > + schedule_work(&shid->refresh_device_work);
> > +
> > + break;
> > + case SPI_HID_INPUT_REPORT_TYPE_SET_OUTPUT_REPORT_RESP:
> > + if (shid->desc.no_output_report_ack) {
> > + dev_err(dev, "Unexpected output report response\n");
> > + break;
> > + }
> > + fallthrough;
> > + case SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP:
> > + case SPI_HID_INPUT_REPORT_TYPE_SET_FEATURE_RESP:
> > + if (!shid->ready) {
> > + dev_err(dev,
> > + "Unexpected response report while not ready:
> 0x%x\n",
> > + body.report_type);
> > + break;
> > + }
> > + fallthrough;
> > + case SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC:
> > + spi_hid_response_handler(shid, buf);
> > + break;
> > + /*
> > + * FIXME: sending GET_INPUT and COMMAND reports not supported,
> thus
> > + * throw away responses to those, they should never come.
> > + */
> > + case SPI_HID_INPUT_REPORT_TYPE_GET_INPUT_REPORT_RESP:
> > + case SPI_HID_INPUT_REPORT_TYPE_COMMAND_RESP:
> > + 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);
> > + schedule_work(&shid->error_work);
> > + break;
> > + }
> > +}
> > +
> > +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> > + struct spi_hid_input_header *header)
> > +{
> > + struct device *dev = &shid->spi->dev;
> > +
> > + if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> > + dev_err(dev, "Unknown input report version (v 0x%x)\n",
> > + header->version);
> > + return -EINVAL;
> > + }
> > +
> > + if (shid->desc.max_input_length != 0 &&
> > + header->report_length > shid->desc.max_input_length)
> {
> > + dev_err(dev, "Input report body size %u > max expected of
> %u\n",
> > + header->report_length,
> > + shid->desc.max_input_length);
> > + return -EMSGSIZE;
> > + }
> > +
> > + if (header->last_fragment_flag != 1) {
> > + dev_err(dev, "Multi-fragment reports not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (header->sync_const != SPI_HID_INPUT_HEADER_SYNC_BYTE) {
> > + dev_err(dev, "Invalid input report sync constant (0x%x)\n",
> > + header->sync_const);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int spi_hid_create_device(struct spi_hid *shid)
> > +{
> > + struct hid_device *hid;
> > + struct device *dev = &shid->spi->dev;
> > + int ret;
> > +
> > + hid = hid_allocate_device();
> > +
> > + if (IS_ERR(hid)) {
> > + dev_err(dev, "Failed to allocate hid device: %ld\n",
> > + PTR_ERR(hid));
> > + ret = PTR_ERR(hid);
> > + return ret;
> > + }
> > +
> > + 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 %04hX:%04hX",
> > + hid->vendor, hid->product);
> > + strscpy(hid->phys, dev_name(&shid->spi->dev), sizeof(hid->phys));
> > +
> > + shid->hid = hid;
> > +
> > + ret = hid_add_device(hid);
> > + if (ret) {
> > + dev_err(dev, "Failed to add hid device: %d\n", ret);
> > + /*
> > + * We likely got here because report descriptor request timed
> > + * out. Let's disconnect and destroy the hid_device structure.
> > + */
> > + hid = spi_hid_disconnect_hid(shid);
> > + if (hid)
> > + hid_destroy_device(hid);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void spi_hid_create_device_work(struct work_struct *work)
> > +{
> > + struct spi_hid *shid =
> > + container_of(work, struct spi_hid, create_device_work);
> > + struct device *dev = &shid->spi->dev;
> > + u8 prev_state = shid->power_state;
> > + int ret;
> > +
> > + trace_spi_hid_create_device_work(shid);
> > +
> > + ret = spi_hid_create_device(shid);
> > + if (ret) {
> > + dev_err(dev, "Failed to create hid device\n");
> > + return;
> > + }
> > +
> > + spi_hid_suspend(shid);
> > +
> > + shid->attempts = 0;
> > +
> > + dev_dbg(dev, "%s: %s -> %s\n", __func__,
> > + spi_hid_power_mode_string(prev_state),
> > + spi_hid_power_mode_string(shid->power_state));
> > +}
> > +
> > +static void spi_hid_refresh_device_work(struct work_struct *work)
> > +{
> > + struct spi_hid *shid =
> > + container_of(work, struct spi_hid, refresh_device_work);
> > + struct device *dev = &shid->spi->dev;
> > + struct hid_device *hid;
> > + int ret;
> > + u32 new_crc32;
> > +
> > + trace_spi_hid_refresh_device_work(shid);
> > +
> > + mutex_lock(&shid->lock);
> > + ret = spi_hid_report_descriptor_request(shid);
> > + mutex_unlock(&shid->lock);
> > + if (ret < 0) {
> > + dev_err(dev,
> > + "Refresh: failed report descriptor request, error %d",
> > + ret);
> > + return;
> > + }
> > +
> > + new_crc32 = crc32_le(0, (unsigned char const *)shid-
> >response.content,
> > + (size_t)ret);
> > + if (new_crc32 == shid->report_descriptor_crc32) {
> > + shid->ready = true;
> > + sysfs_notify(&dev->kobj, NULL, "ready");
> > + return;
> > + }
> > +
> > + shid->report_descriptor_crc32 = new_crc32;
> > + shid->refresh_in_progress = true;
> > +
> > + hid = spi_hid_disconnect_hid(shid);
> > + if (hid)
> > + hid_destroy_device(hid);
> > +
> > + ret = spi_hid_create_device(shid);
> > + if (ret)
> > + dev_err(dev, "Failed to create hid device\n");
> > +
> > + shid->refresh_in_progress = false;
> > + shid->ready = true;
> > + sysfs_notify(&dev->kobj, NULL, "ready");
> > +}
> > +
> > +static void spi_hid_input_header_complete(void *_shid);
> > +
> > +static void spi_hid_input_body_complete(void *_shid)
> > +{
> > + struct spi_hid *shid = _shid;
> > + struct device *dev = &shid->spi->dev;
> > + unsigned long flags;
> > + int ret;
> > +
> > + spin_lock_irqsave(&shid->input_lock, flags);
> > +
> > + if (shid->power_state == SPI_HID_POWER_MODE_OFF) {
> > + dev_warn(dev,
> > + "input body complete called while device is off\n");
> > + goto out;
> > + }
> > +
> > + 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, resetting %d\n",
> > + shid->input_message.status);
> > + shid->bus_error_count++;
> > + shid->bus_last_error = shid->input_message.status;
> > + schedule_work(&shid->error_work);
> > + goto out;
> > + }
> > +
> > + spi_hid_process_input_report(shid, &shid->input);
> > +
> > + if (--shid->input_transfer_pending) {
> > + struct spi_hid_input_buf *buf = &shid->input;
> > +
> > + trace_spi_hid_header_transfer(shid);
> > + ret = spi_hid_input_async(shid, buf->header,
> > + sizeof(buf->header),
> > + spi_hid_input_header_complete, true);
> > + if (ret)
> > + dev_err(dev, "failed to start header transfer %d\n",
> > + ret);
> > + }
> > +
> > +out:
> > + spin_unlock_irqrestore(&shid->input_lock, flags);
> > +}
> > +
> > +static void spi_hid_input_header_complete(void *_shid)
> > +{
> > + struct spi_hid *shid = _shid;
> > + struct device *dev = &shid->spi->dev;
> > + struct spi_hid_input_header header;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + spin_lock_irqsave(&shid->input_lock, flags);
> > +
> > + if (shid->power_state == SPI_HID_POWER_MODE_OFF) {
> > + dev_warn(dev,
> > + "input header complete called while device is off\n");
> > + goto out;
> > + }
> > +
> > + 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, resetting, error %d\n",
> > + shid->input_message.status);
> > + shid->bus_error_count++;
> > + shid->bus_last_error = shid->input_message.status;
> > + schedule_work(&shid->error_work);
> > + goto out;
> > + }
> > + spi_hid_populate_input_header(shid->input.header, &header);
> > +
> > + ret = spi_hid_bus_validate_header(shid, &header);
> > + if (ret) {
> > + dev_err(dev, "failed to validate header: %d\n", ret);
> > + print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
> > + DUMP_PREFIX_NONE, 16, 1,
> > + shid->input.header,
> > + sizeof(shid->input.header),
> > + false);
> > + shid->bus_error_count++;
> > + shid->bus_last_error = ret;
> > + goto out;
> > + }
> > +
> > + ret = spi_hid_input_async(shid, shid->input.body, header.report_length,
> > + spi_hid_input_body_complete, false);
> > + if (ret)
> > + dev_err(dev, "failed body async transfer: %d\n", ret);
> > +
> > +out:
> > + if (ret)
> > + shid->input_transfer_pending = 0;
> > +
> > + spin_unlock_irqrestore(&shid->input_lock, flags);
> > +}
> > +
> > +static int spi_hid_get_request(struct spi_hid *shid, u8 content_id)
> > +{
> > + int ret;
> > + struct device *dev = &shid->spi->dev;
> > + struct spi_hid_output_report report = {
> > + .report_type =
> SPI_HID_OUTPUT_REPORT_TYPE_HID_GET_FEATURE,
> > + .content_length = 0,
> > + .content_id = content_id,
> > + .content = NULL,
> > + };
> > +
> > + ret = spi_hid_sync_request(shid, &report);
> > + if (ret) {
> > + dev_err(dev,
> > + "Expected get request response not received! Error
> %d\n",
> > + ret);
> > + schedule_work(&shid->error_work);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +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 =
> SPI_HID_OUTPUT_REPORT_TYPE_HID_SET_FEATURE,
> > + .content_length = arg_len,
> > + .content_id = content_id,
> > + .content = arg_buf,
> > + };
> > +
> > + return spi_hid_sync_request(shid, &report);
> > +}
> > +
> > +static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> > +{
> > + struct spi_hid *shid = _shid;
> > + struct device *dev = &shid->spi->dev;
> > + int ret = 0;
> > +
> > + spin_lock(&shid->input_lock);
> > + trace_spi_hid_dev_irq(shid, irq);
> > +
> > + if (shid->input_transfer_pending++)
> > + goto out;
> > +
> > + trace_spi_hid_header_transfer(shid);
> > + ret = spi_hid_input_async(shid, shid->input.header,
> > + sizeof(shid->input.header),
> > + spi_hid_input_header_complete, true);
> > + if (ret)
> > + dev_err(dev, "Failed to start header transfer: %d\n", ret);
> > +
> > +out:
> > + spin_unlock(&shid->input_lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/* 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);
> > +
> > + if (shid->desc.max_input_length < HID_MIN_BUFFER_SIZE) {
> > + dev_err(&shid->spi->dev,
> > + "HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
> > + shid->desc.max_input_length);
> > + return -EINVAL;
> > + }
> > +
> > + 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)
> > +{
> > + struct spi_device *spi = hid->driver_data;
> > + struct spi_hid *shid = spi_get_drvdata(spi);
> > + struct device *dev = &spi->dev;
> > + u8 prev_state = shid->power_state;
> > +
> > + if (shid->refresh_in_progress)
> > + return 0;
> > +
> > + spi_hid_resume(shid);
> > +
> > + dev_dbg(dev, "%s: %s -> %s\n", __func__,
> > + spi_hid_power_mode_string(prev_state),
> > + spi_hid_power_mode_string(shid->power_state));
> > +
> > + 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);
> > + struct device *dev = &spi->dev;
> > + u8 prev_state = shid->power_state;
> > +
> > + if (shid->refresh_in_progress)
> > + return;
> > +
> > + spi_hid_suspend(shid);
> > +
> > + shid->attempts = 0;
> > +
> > + dev_dbg(dev, "%s: %s -> %s\n", __func__,
> > + spi_hid_power_mode_string(prev_state),
> > + spi_hid_power_mode_string(shid->power_state));
> > +}
> > +
> > +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 ret = 0;
> > +
> > + mutex_lock(&shid->lock);
> > + if (!shid->hid)
> > + ret = -ENODEV;
> > + mutex_unlock(&shid->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int spi_hid_ll_parse(struct hid_device *hid)
> > +{
> > + struct spi_device *spi = hid->driver_data;
> > + struct spi_hid *shid = spi_get_drvdata(spi);
> > + struct device *dev = &spi->dev;
> > + int ret, len;
> > +
> > + mutex_lock(&shid->lock);
> > +
> > + len = spi_hid_report_descriptor_request(shid);
> > + if (len < 0) {
> > + dev_err(dev, "Report descriptor request failed, %d\n", len);
> > + ret = len;
> > + goto out;
> > + }
> > +
> > + /*
> > + * 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.
> > + */
> > + ret = hid_parse_report(hid, (__u8 *)shid->response.content, len);
> > + if (ret)
> > + dev_err(dev, "failed parsing report: %d\n", ret);
> > + else
> > + shid->report_descriptor_crc32 = crc32_le(0,
> > + (unsigned char const *)shid->response.content,
> > + len);
> > +
> > +out:
> > + mutex_unlock(&shid->lock);
> > +
> > + return ret;
> > +}
> > +
> > +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;
> > +
> > + if (!shid->ready) {
> > + dev_err(&shid->spi->dev, "%s called in unready state\n",
> > + __func__);
> > + return -ENODEV;
>
> This will confuse users for no good reason. Do not let them call into
> the driver until it is ready.
This is the biggest change in the v4 - we now wait_for_completion() in
ll_open() until the device is ready.
>
> > + }
> > +
> > + mutex_lock(&shid->lock);
> > +
> > + switch (reqtype) {
> > + case HID_REQ_SET_REPORT:
> > + if (buf[0] != reportnum) {
> > + dev_err(dev, "report id mismatch\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + ret = spi_hid_set_request(shid, &buf[1], len - 1,
> > + reportnum);
> > + if (ret) {
> > + dev_err(dev, "failed to set report\n");
> > + break;
> > + }
> > +
> > + ret = len;
> > + break;
> > + case HID_REQ_GET_REPORT:
> > + ret = spi_hid_get_request(shid, reportnum);
> > + if (ret) {
> > + dev_err(dev, "failed to get report\n");
> > + break;
> > + }
> > +
> > + ret = min_t(size_t, len,
> > + shid->response.body[1] | (shid->response.body[2] <<
> 8));
> > + memcpy(buf, &shid->response.content, ret);
> > + break;
> > + default:
> > + dev_err(dev, "invalid request type\n");
> > + ret = -EIO;
> > + }
> > +
> > + mutex_unlock(&shid->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int spi_hid_ll_output_report(struct hid_device *hid,
> > + __u8 *buf, size_t len)
> > +{
> > + int ret;
> > + 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 =
> SPI_HID_OUTPUT_REPORT_TYPE_HID_OUTPUT_REPORT,
> > + .content_length = len - 1,
> > + .content_id = buf[0],
> > + .content = &buf[1],
> > + };
> > +
> > + mutex_lock(&shid->lock);
> > + if (!shid->ready) {
> > + dev_err(dev, "%s called in unready state\n", __func__);
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + if (shid->desc.no_output_report_ack)
> > + ret = spi_hid_send_output_report(shid, &report);
> > + else
> > + ret = spi_hid_sync_request(shid, &report);
> > +
> > + if (ret)
> > + dev_err(dev, "failed to send output report\n");
> > +
> > +out:
> > + mutex_unlock(&shid->lock);
> > +
> > + if (ret > 0)
> > + return -ret;
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return len;
> > +}
> > +
> > +static struct hid_ll_driver spi_hid_ll_driver = {
> > + .start = spi_hid_ll_start,
> > + .stop = spi_hid_ll_stop,
> > + .open = spi_hid_ll_open,
> > + .close = spi_hid_ll_close,
> > + .power = spi_hid_ll_power,
> > + .parse = spi_hid_ll_parse,
> > + .output_report = spi_hid_ll_output_report,
> > + .raw_request = spi_hid_ll_raw_request,
> > +};
> > +
> > +static ssize_t ready_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct spi_hid *shid = dev_get_drvdata(dev);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n",
> > + shid->ready ? "ready" : "not ready");
>
> Why is this needed? Why can't we simply delay registering HID device
> until after we are ready to handle it? Or delay in open?
Delay in open is what was chosen as a better solution and is implemented
in v4.
>
> > +}
> > +static DEVICE_ATTR_RO(ready);
> > +
> > +static ssize_t bus_error_count_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct spi_hid *shid = dev_get_drvdata(dev);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%d (%d)\n",
> > + shid->bus_error_count, shid->bus_last_error);
>
> This and below I think better suited for debugfs.
These are used for telemetry on our device and customer builds don't
have debugfs enabled, thus we need to use sysfs.
>
> > +}
> > +static DEVICE_ATTR_RO(bus_error_count);
> > +
> > +static ssize_t regulator_error_count_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct spi_hid *shid = dev_get_drvdata(dev);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%d (%d)\n",
> > + shid->regulator_error_count,
> > + shid->regulator_last_error);
> > +}
> > +static DEVICE_ATTR_RO(regulator_error_count);
> > +
> > +static ssize_t device_initiated_reset_count_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct spi_hid *shid = dev_get_drvdata(dev);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%d\n", shid->dir_count);
> > +}
> > +static DEVICE_ATTR_RO(device_initiated_reset_count);
> > +
> > +static const struct attribute *const spi_hid_attributes[] = {
> > + &dev_attr_ready.attr,
> > + &dev_attr_bus_error_count.attr,
> > + &dev_attr_regulator_error_count.attr,
> > + &dev_attr_device_initiated_reset_count.attr,
> > + NULL /* Terminator */
> > +};
> > +
> > +static int spi_hid_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct spi_hid *shid;
> > + unsigned long irqflags;
> > + int ret;
> > +
> > + if (spi->irq <= 0) {
> > + dev_err(dev, "Missing IRQ\n");
> > + ret = spi->irq ?: -EINVAL;
> > + goto err0;
> > + }
> > +
> > + shid = devm_kzalloc(dev, sizeof(struct spi_hid), GFP_KERNEL);
> > + if (!shid) {
> > + ret = -ENOMEM;
> > + goto err0;
> > + }
> > +
> > + shid->spi = spi;
> > + shid->power_state = SPI_HID_POWER_MODE_ON;
> > + spi_set_drvdata(spi, shid);
> > +
> > + ret = sysfs_create_files(&dev->kobj, spi_hid_attributes);
>
> I'd recommend using devm* for all resources used by this driver.
>
> > + if (ret) {
> > + dev_err(dev, "Unable to create sysfs attributes\n");
> > + goto err0;
> > + }
> > +
> > + ret = spi_hid_of_populate_config(&shid->conf, dev);
>
> Error handling?
Added in v4.
>
> > +
> > + /* 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->lock);
> > + init_completion(&shid->output_done);
> > +
> > + spin_lock_init(&shid->input_lock);
> > + INIT_WORK(&shid->reset_work, spi_hid_reset_work);
> > + INIT_WORK(&shid->create_device_work, spi_hid_create_device_work);
> > + INIT_WORK(&shid->refresh_device_work,
> spi_hid_refresh_device_work);
> > + INIT_WORK(&shid->error_work, spi_hid_error_work);
> > +
> > + /*
> > + * At the end of probe we initialize the device:
> > + * 0) Default pinctrl in DT: assert reset, bias the interrupt line
> > + * 1) sleep minimal reset delay
> > + * 2) request IRQ
> > + * 3) power up the device
> > + * 4) sleep 5ms
> > + * 5) deassert reset (high)
> > + * 6) sleep 5ms
> > + */
> > +
> > + spi_hid_of_sleep_minimal_reset_delay(&shid->conf);
> > +
> > + irqflags = irq_get_trigger_type(spi->irq) | IRQF_ONESHOT;
>
> Specifying ONESHOT mode for non-threaded IRQ does not make much sense.
> Why don't you use threaded IRQ and get rid of your async/work hanlding?
Switched to threaded IRQ and spi_sync in v4.
>
> > + ret = request_irq(spi->irq, spi_hid_dev_irq, irqflags,
> > + dev_name(&spi->dev), shid);
> > + if (ret)
> > + goto err1;
> > +
> > + ret = spi_hid_of_power_up(&shid->conf);
> > + if (ret) {
> > + dev_err(dev, "%s: could not power up\n", __func__);
> > + shid->regulator_error_count++;
> > + shid->regulator_last_error = ret;
> > + goto err1;
> > + }
> > +
> > + spi_hid_of_deassert_reset(&shid->conf);
> > +
> > + dev_err(dev, "%s: d3 -> %s\n", __func__,
> > + spi_hid_power_mode_string(shid->power_state));
> > +
> > + return 0;
> > +
> > +err1:
> > + sysfs_remove_files(&dev->kobj, spi_hid_attributes);
> > +
> > +err0:
> > + return ret;
> > +}
> > +
> > +static int spi_hid_remove(struct spi_device *spi)
> > +{
> > + struct spi_hid *shid = spi_get_drvdata(spi);
> > + struct device *dev = &spi->dev;
> > + int ret;
> > +
> > + spi_hid_of_assert_reset(&shid->conf);
> > + ret = spi_hid_of_power_down(&shid->conf);
> > + if (ret) {
> > + dev_err(dev, "failed to disable regulator\n");
> > + shid->regulator_error_count++;
> > + shid->regulator_last_error = ret;
> > + }
> > + free_irq(spi->irq, shid);
> > + sysfs_remove_files(&dev->kobj, spi_hid_attributes);
> > + spi_hid_stop_hid(shid);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct spi_device_id spi_hid_id_table[] = {
> > + { "hid", 0 },
> > + { "hid-over-spi", 0 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(spi, spi_hid_id_table);
> > +
> > +static struct spi_driver spi_hid_driver = {
> > + .driver = {
> > + .name = "spi_hid",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(spi_hid_of_match),
> > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > + },
> > + .probe = spi_hid_probe,
> > + .remove = spi_hid_remove,
> > + .id_table = spi_hid_id_table,
> > +};
> > +
> > +module_spi_driver(spi_hid_driver);
> > +
> > +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
> > new file mode 100644
> > index 000000000000..95e667ad53ec
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/spi-hid-core.h
> > @@ -0,0 +1,188 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * spi-hid-core.h
> > + *
> > + * Copyright (c) 2021 Microsoft Corporation
> > + */
> > +
> > +#ifndef SPI_HID_CORE_H
> > +#define SPI_HID_CORE_H
> > +
> > +#include <linux/completion.h>
> > +#include <linux/kernel.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +
> > +#include "spi-hid-of.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
> > +
> > +/* Protocol message size constants */
> > +#define SPI_HID_READ_APPROVAL_LEN 5
> > +#define SPI_HID_INPUT_HEADER_LEN 4
> > +#define SPI_HID_INPUT_BODY_LEN 4
> > +#define SPI_HID_OUTPUT_HEADER_LEN 8
> > +#define SPI_HID_DEVICE_DESCRIPTOR_LENGTH 24
> > +
> > +/* Protocol message type constants */
> > +#define SPI_HID_INPUT_REPORT_TYPE_DATA 0x01
> > +#define SPI_HID_INPUT_REPORT_TYPE_RESET_RESP 0x03
> > +#define SPI_HID_INPUT_REPORT_TYPE_COMMAND_RESP
> 0x04
> > +#define SPI_HID_INPUT_REPORT_TYPE_GET_FEATURE_RESP 0x05
> > +#define SPI_HID_INPUT_REPORT_TYPE_DEVICE_DESC 0x07
> > +#define SPI_HID_INPUT_REPORT_TYPE_REPORT_DESC
> 0x08
> > +#define SPI_HID_INPUT_REPORT_TYPE_SET_FEATURE_RESP 0x09
> > +#define SPI_HID_INPUT_REPORT_TYPE_SET_OUTPUT_REPORT_RESP 0x0a
> > +#define SPI_HID_INPUT_REPORT_TYPE_GET_INPUT_REPORT_RESP
> 0x0b
> > +
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_DEVICE_DESC_REQUEST 0x01
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_REPORT_DESC_REQUEST 0x02
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_HID_SET_FEATURE 0x03
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_HID_GET_FEATURE 0x04
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_HID_OUTPUT_REPORT 0x05
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_INPUT_REPORT_REQUEST 0x06
> > +#define SPI_HID_OUTPUT_REPORT_TYPE_COMMAND 0x07
> > +
> > +#define SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST 0x00
> > +
> > +/* Power mode constants */
> > +#define SPI_HID_POWER_MODE_ON 0x01
> > +#define SPI_HID_POWER_MODE_SLEEP 0x02
> > +#define SPI_HID_POWER_MODE_OFF 0x03
> > +#define SPI_HID_POWER_MODE_WAKING_SLEEP 0x04
> > +
> > +/* Raw input buffer with data from the bus */
> > +struct spi_hid_input_buf {
> > + __u8 header[SPI_HID_INPUT_HEADER_LEN];
> > + __u8 body[SPI_HID_INPUT_BODY_LEN];
> > + u8 content[SZ_8K];
> > +};
> > +
> > +/* 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 input report body, excluding the content */
> > +struct spi_hid_input_body {
> > + u8 report_type;
> > + u16 content_length;
> > + u8 content_id;
> > +};
> > +
> > +/* 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];
> > + u8 content[SZ_8K];
> > +};
> > +
> > +/* Data necessary to send an output report */
> > +struct spi_hid_output_report {
> > + u8 report_type;
> > + u16 content_length;
> > + u8 content_id;
> > + u8 *content;
> > +};
> > +
> > +/* Raw content in device descriptor */
> > +struct spi_hid_device_desc_raw {
> > + __le16 wDeviceDescLength;
> > + __le16 bcdVersion;
> > + __le16 wReportDescLength;
> > + __le16 wMaxInputLength;
> > + __le16 wMaxOutputLength;
> > + __le16 wMaxFragmentLength;
> > + __le16 wVendorID;
> > + __le16 wProductID;
> > + __le16 wVersionID;
> > + __le16 wFlags;
> > + __u8 reserved[4];
> > +} __packed;
> > +
> > +/* 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;
>
> bool?
>
> > +};
> > +
> > +/* Driver context */
> > +struct spi_hid {
> > + struct spi_device *spi;
> > + struct hid_device *hid;
> > +
> > + struct spi_transfer input_transfer[2];
> > + struct spi_transfer output_transfer;
> > + struct spi_message input_message;
> > + struct spi_message output_message;
> > +
> > + struct spi_hid_of_config conf;
> > + struct spi_hid_device_descriptor desc;
> > + struct spi_hid_output_buf output;
> > + struct spi_hid_input_buf input;
> > + struct spi_hid_input_buf response;
> > +
> > + spinlock_t input_lock;
> > +
> > + u32 input_transfer_pending;
> > +
> > + u8 power_state;
> > +
> > + u8 attempts;
> > +
> > + /*
> > + * ready flag indicates that the FW is ready to accept commands and
> > + * requests. The FW becomes ready after sending the report descriptor.
> > + */
> > + bool ready;
> > + /*
> > + * refresh_in_progress is set to true while the refresh_device worker
> > + * thread is destroying and recreating the hidraw device. When this flag
> > + * is set to true, the ll_close and ll_open functions will not cause
> > + * power state changes.
> > + */
> > + bool refresh_in_progress;
> > +
> > + struct work_struct reset_work;
> > + struct work_struct create_device_work;
> > + struct work_struct refresh_device_work;
> > + struct work_struct error_work;
> > +
> > + struct mutex lock;
> > + struct completion output_done;
> > +
> > + __u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN];
> > + __u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
> > +
> > + u32 report_descriptor_crc32;
> > +
> > + u32 regulator_error_count;
> > + int regulator_last_error;
> > + u32 bus_error_count;
> > + int bus_last_error;
> > + u32 dir_count;
> > +};
> > +
> > +#endif
> > diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
> > new file mode 100755
> > index 000000000000..4896a90c2e8e
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/spi-hid-of.c
> > @@ -0,0 +1,148 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * HID over SPI protocol, Open Firmware related code
> > + * spi-hid-of.c
> > + *
> > + * 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/of.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> > +
> > +#include "spi-hid-core.h"
> > +
> > +const struct of_device_id spi_hid_of_match[] = {
> > + { .compatible = "hid-over-spi" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, spi_hid_of_match);
> > +
> > +int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> > + struct device *dev)
> > +{
> > + int ret;
> > + u32 val;
> > +
> > + ret = device_property_read_u32(dev, "input-report-header-address",
> > + &val);
> > + if (ret) {
> > + dev_err(dev, "Input report header address not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->input_report_header_address = val;
> > +
> > + ret = device_property_read_u32(dev, "input-report-body-address",
> &val);
> > + if (ret) {
> > + dev_err(dev, "Input report body address not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->input_report_body_address = val;
> > +
> > + ret = device_property_read_u32(dev, "output-report-address", &val);
> > + if (ret) {
> > + dev_err(dev, "Output report address not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->output_report_address = val;
> > +
> > + ret = device_property_read_u32(dev, "read-opcode", &val);
> > + if (ret) {
> > + dev_err(dev, "Read opcode not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->read_opcode = val;
> > +
> > + ret = device_property_read_u32(dev, "write-opcode", &val);
> > + if (ret) {
> > + dev_err(dev, "Write opcode not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->write_opcode = val;
> > +
> > + ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);
> > + if (ret) {
> > + dev_err(dev, "Post-power-on delay not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->post_power_on_delay_ms = val;
> > +
> > + ret = device_property_read_u32(dev, "minimal-reset-delay-ms", &val);
> > + if (ret) {
> > + dev_err(dev, "Minimal reset time not provided\n");
> > + return -ENODEV;
> > + }
> > + conf->minimal_reset_delay_ms = val;
> > +
> > + /* FIXME: not reading flags from DT, multi-SPI modes not supported */
> > +
> > + conf->supply = devm_regulator_get(dev, "vdd");
> > + if (IS_ERR(conf->supply)) {
> > + if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
> > + dev_err(dev, "Failed to get regulator: %ld\n",
> > + PTR_ERR(conf->supply));
> > + return PTR_ERR(conf->supply);
> > + }
> > +
> > + conf->reset_gpio = devm_gpiod_get(dev, "reset-gpio",
> GPIOD_OUT_LOW);
> > + if (IS_ERR(conf->reset_gpio)) {
> > + dev_err(dev, "%s: error getting GPIO\n", __func__);
> > + return PTR_ERR(conf->reset_gpio);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int spi_hid_of_power_down(struct spi_hid_of_config *conf)
> > +{
> > + if (regulator_is_enabled(conf->supply) == 0)
> > + return 0;
> > +
> > + return regulator_disable(conf->supply);
> > +}
> > +
> > +int spi_hid_of_power_up(struct spi_hid_of_config *conf)
> > +{
> > + int ret;
> > +
> > + if (regulator_is_enabled(conf->supply) > 0)
> > + return 0;
> > +
> > + ret = regulator_enable(conf->supply);
> > +
> > + usleep_range(1000 * conf->post_power_on_delay_ms,
> > + 1000 * (conf->post_power_on_delay_ms + 1));
> > +
> > + return ret;
> > +}
> > +
> > +void spi_hid_of_assert_reset(struct spi_hid_of_config *conf)
> > +{
> > + gpiod_set_value(conf->reset_gpio, 1);
> > +}
> > +
> > +void spi_hid_of_deassert_reset(struct spi_hid_of_config *conf)
> > +{
> > + gpiod_set_value(conf->reset_gpio, 0);
>
> Why do we need these wrappers?
>
> > +}
> > +
> > +void spi_hid_of_sleep_minimal_reset_delay(struct spi_hid_of_config *conf)
> > +{
> > + usleep_range(1000 * conf->minimal_reset_delay_ms,
> > + 1000 * (conf->minimal_reset_delay_ms + 1));
> > +}
> > \ No newline at end of file
> > diff --git a/drivers/hid/spi-hid/spi-hid-of.h b/drivers/hid/spi-hid/spi-hid-of.h
> > new file mode 100755
> > index 000000000000..5991011d8921
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/spi-hid-of.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * spi-hid-of.h
> > + *
> > + * Copyright (c) 2021 Microsoft Corporation
> > + */
> > +
> > +#ifndef SPI_HID_OF_H
> > +#define SPI_HID_OF_H
> > +
> > +extern const struct of_device_id spi_hid_of_match[];
> > +
> > +/* Config structure is filled with data from Device Tree */
> > +struct spi_hid_of_config {
> > + u32 input_report_header_address;
> > + u32 input_report_body_address;
> > + u32 output_report_address;
> > + u8 read_opcode;
> > + u8 write_opcode;
> > + u32 post_power_on_delay_ms;
> > + u32 minimal_reset_delay_ms;
> > + struct gpio_desc *reset_gpio;
> > + struct regulator *supply;
> > +};
> > +
> > +int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> > + struct device *dev);
> > +int spi_hid_of_power_down(struct spi_hid_of_config *conf);
> > +int spi_hid_of_power_up(struct spi_hid_of_config *conf);
> > +void spi_hid_of_assert_reset(struct spi_hid_of_config *conf);
> > +void spi_hid_of_deassert_reset(struct spi_hid_of_config *conf);
> > +void spi_hid_of_sleep_minimal_reset_delay(struct spi_hid_of_config *conf);
> > +
> > +#endif
> > 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..905de02d85aa
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/spi-hid_trace.h
> > @@ -0,0 +1,198 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * spi-hid_trace.h
> > + *
> > + * 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"
> > +
> > +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, len)
> > + __field(int, ret)
> > + __dynamic_array(u8, rx_buf, rx_len)
> > + __dynamic_array(u8, tx_buf, tx_len)
> > + ),
> > +
> > + TP_fast_assign(
> > + __entry->bus_num = shid->spi->controller->bus_num;
> > + __entry->chip_select = shid->spi->chip_select;
> > + __entry->len = rx_len + tx_len;
> > + __entry->ret = ret;
> > +
> > + memcpy(__get_dynamic_array(tx_buf), tx_buf, tx_len);
> > + memcpy(__get_dynamic_array(rx_buf), rx_buf, rx_len);
> > + ),
> > +
> > + TP_printk("spi%d.%d: len=%d tx=[%*phD] rx=[%*phD] --> %d",
> > + __entry->bus_num, __entry->chip_select, __entry->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_async,
> > + 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)
> > +);
> > +
> > +DEFINE_EVENT(spi_hid_transfer, spi_hid_output_begin,
> > + 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_output_end,
> > + 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 = shid->spi->chip_select;
> > + __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, input_transfer_pending)
> > + __field(bool, ready)
> > +
> > + __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 = shid->spi->chip_select;
> > + __entry->power_state = shid->power_state;
> > + __entry->input_transfer_pending = shid-
> >input_transfer_pending;
> > + __entry->ready = shid->ready;
> > +
> > + __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 i:%d p:%d
> len i:%d o:%d r:%d flags %c:%d",
> > + __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->ready ? 'R' : 'r', __entry->input_transfer_pending)
> > +);
> > +
> > +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_work,
> > + TP_PROTO(struct spi_hid *shid),
> > + TP_ARGS(shid)
> > +);
> > +
> > +DEFINE_EVENT(spi_hid, spi_hid_create_device_work,
> > + TP_PROTO(struct spi_hid *shid),
> > + TP_ARGS(shid)
> > +);
> > +
> > +DEFINE_EVENT(spi_hid, spi_hid_refresh_device_work,
> > + 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 */
> > +
> > +#undef TRACE_INCLUDE_PATH
> > +#define TRACE_INCLUDE_PATH .
> > +#define TRACE_INCLUDE_FILE spi-hid_trace
> > +#include <trace/define_trace.h>
> > diff --git a/drivers/hid/spi-hid/trace.c b/drivers/hid/spi-hid/trace.c
> > new file mode 100644
> > index 000000000000..aaa2da0c9642
> > --- /dev/null
> > +++ b/drivers/hid/spi-hid/trace.c
> > @@ -0,0 +1,11 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * trace.c - SPI HID Trace Support
> > + *
> > + * Copyright (c) 2021 Microsoft Corporation
> > + *
> > + * Author: Felipe Balbi <felipe.balbi@microsoft.com>
> > + */
> > +
> > +#define CREATE_TRACE_POINTS
> > +#include "spi-hid_trace.h"
> > --
> > 2.25.1
> >
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [EXTERNAL] Re: [PATCH v3 3/5] Documentation: DT bindings for HID over SPI.
2022-01-20 20:06 ` Rob Herring
@ 2022-02-25 1:34 ` Dmitry Antipov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Antipov @ 2022-02-25 1:34 UTC (permalink / raw)
To: Rob Herring, Dmitry Antipov
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Mark Brown,
Felipe Balbi, linux-input, devicetree, linux-spi
Hello Rob,
I just submitted patch v4 that addresses most of your feedback.
On Thu, Jan 20, 2022 at 12:07 PM,Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
>
> On Fri, Jan 14, 2022 at 06:31:33PM -0800, Dmitry Antipov wrote:
> > From: Dmitry Antipov <dmanti@microsoft.com>
> >
> > Added documentation describes the required properties for implementing
> > Device Tree for a device supporting HID over SPI and also provides an
> > example.
>
> Bindings are in DT schema format now.
>
> >
> > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> > ---
> > .../bindings/input/hid-over-spi.txt | 43 +++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> > create mode 100755
> > Documentation/devicetree/bindings/input/hid-over-spi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.txt
> > b/Documentation/devicetree/bindings/input/hid-over-spi.txt
> > new file mode 100755
> > index 000000000000..5eba95b5724e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/hid-over-spi.txt
> > @@ -0,0 +1,43 @@
> > +* HID over SPI Device-Tree bindings
> > +
> > +HID over SPI provides support for Human Interface Devices over the
> > +SPI bus. HID Over SPI Protocol Specification 1.0 was written by
> > +Microsoft and is available at
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.m
> icrosoft.com%2Fen-
> us%2Fdownload%2Fdetails.aspx%3Fid%3D103325&data=04%7C01%7Cd
> manti%40microsoft.com%7Cb0b84680fa2c45bc2f3a08d9dc5063f6%7C72f988
> bf86f141af91ab2d7cd011db47%7C1%7C0%7C637783060144589082%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C3000&sdata=bWGZDRYtVovy3XYlpDsZ8xR2AL
> be5YoP0iSZ8xORSyA%3D&reserved=0.
> > +
> > +If this binding is used, the kernel module spi-hid will handle the
> > +communication
>
> What's a kernel module? ;) Bindings are OS independent (or supposed to be), so
> kernel details do not belong here.
>
> > +with the device and the generic hid core layer will handle the protocol.
> > +
> > +Required properties:
> > +- compatible: must be "hid-over-spi"
>
> Bindings describe devices, not protocols. There is no such 'hid-over-spi' device.
> Please see the existing hid-over-i2c binding.
> It's fine to have this compatible, but only as a fallback compatible.
In the v4 patch the yaml file expects the device-specific compatible
string, but also 'hid-over-spi'.
>
> > +- interrupts: interrupt line
> > +- vdd-supply: phandle of the regulator that provides the supply
> > +voltage
>
> What happens when the device has 2 supplies? And there's some timing
> requirement between them?
I am not sure if there is a way to account for all possible scenarios.
Our device only has one power regulator and the binding is now
device-specific.
>
> > +- reset_gpio-gpios: gpio wired to the device reset line
>
> 'reset-gpios' is the standard name.
>
> > +- post-power-on-delay-ms: time required by the device after enabling
> > +its regulators or powering it on, before it is ready for
> > +communication
> > +- minimal-reset-delay-ms: minimum amount of time that device needs to
> > +be in reset state for the reset to take effect
>
> These properties are what happens when we try to do generic bindings.
> It's a never-ending addition of more properties to try to describe
> (poorly) the power sequencing requirements.
I have made these optional in the v4 patch.
>
> > +- input-report-header-address: this property and the rest are
> > +described in HID Over SPI Protocol Spec 1.0
> > +- input-report-body-address
> > +- output-report-address
> > +- read-opcode
> > +- write-opcode
> > +- flags
>
> A bit too generic. We generally want to avoid having a property name with 2
> different types/meaning. It's not something we check for yet, but I plan to at
> some point.
I've made "flags" more specific in the v4 patch.
>
> > +
> > +Example:
> > + spi-hid-dev0 {
> > + compatible = "hid-over-spi";
> > + reg = <0>;
> > + interrupts-extended = <&tlmm 42 IRQ_TYPE_EDGE_FALLING>;
> > + vdd-supply = <&pm8350c_l3>;
> > + input-report-header-address = <0x1000>;
> > + input-report-body-address = <0x1004>;
> > + output-report-address = <0x2000>;
> > + read-opcode = <0x0b>;
> > + write-opcode = <0x02>;
> > + flags = <0x00>;
> > + reset_gpio-gpios = <&tlmm 25 GPIO_ACTIVE_LOW>;
> > + post-power-on-delay-ms = <5>;
> > + minimal-reset-delay-ms = <5>;
> > +
> > + };
> > --
> > 2.25.1
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-02-25 1:34 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-15 2:31 [PATCH v3 0/5] Add spi-hid, transport for HID over SPI bus Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 1/5] HID: Add BUS_SPI support when printing out device info in hid_connect() Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 2/5] HID: define HID_SPI_DEVICE macro in hid.h Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 3/5] Documentation: DT bindings for HID over SPI Dmitry Antipov
2022-01-20 20:06 ` Rob Herring
2022-02-25 1:34 ` [EXTERNAL] " Dmitry Antipov
2022-01-24 10:32 ` Felipe Balbi
2022-01-15 2:31 ` [PATCH v3 4/5] Documentation: Correction in HID output_report callback description Dmitry Antipov
2022-01-15 2:31 ` [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus Dmitry Antipov
2022-01-15 4:32 ` Dmitry Torokhov
2022-02-25 1:20 ` [EXTERNAL] " Dmitry Antipov
2022-01-15 6:10 ` kernel test robot
2022-01-15 6:10 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.