RE: [EXTERNAL] Re: [PATCH v3 5/5] HID: add spi-hid, transport driver for HID over SPI bus

[Dmitry Antipov <dmanti@xxxxxxxxxxxxx>]

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@xxxxxxxxx>
> 
> Hi Dmitry,
> 
> On Fri, Jan 14, 2022 at 06:31:35PM -0800, Dmitry Antipov wrote:
> > From: Dmitry Antipov <dmanti@xxxxxxxxxxxxx>
> >
> > 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&amp;data=04%7C01%7Cd
> manti%40microsoft.com%7C33c7f0f39c9349ef7d1d08d9d7e01d17%7C72f988
> bf86f141af91ab2d7cd011db47%7C1%7C0%7C637778180034947738%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C2000&amp;sdata=4bdVX7cTKLt7dDBxxsrs3EG1j8k
> BE6h6dBWncpwH7EI%3D&amp;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@xxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>
> > + *  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@xxxxxxx>
> > + *  Copyright (c) 2005 Michael Haboustak <mike-@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>");
> > +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@xxxxxxxxx>
> > + * 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@xxxxxxx>
> > + * Copyright (c) 2005 Michael Haboustak <mike-@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > + */
> > +
> > +#define CREATE_TRACE_POINTS
> > +#include "spi-hid_trace.h"
> > --
> > 2.25.1
> >
> 
> Thanks.
> 
> --
> Dmitry