Re: [PATCH v1 0/5] Add spi-hid, transport for HID over SPI bus

Hi Dmitry,

Thanks a lot for the submission. It is in a much better form than the
previous version.

I do have a few general comments and then will comment on each commit.

On Thu, Dec 30, 2021 at 12:11 AM Dmitry Antipov [off-list ref] wrote:

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.

I managed to find the spec on the website, but ideally could you add a
link here and in your 5/5 patch so we keep a trace of it?

In the initial commits there are some HID core changes to support a SPI
device, followed by extensions to hid_driver and hid_ll_driver structs
to allow for some error-handling logic delegation from the transport
layer to the device driver, and finally the SPI HID transport driver.

The 2 other comments I have are:

- your patches need to have the same Signed-off-by: line than the
From: line. If you can't use your email from your SoB line, just add
in the commit description the from line.
For example, the formatted patch would be:
---
From: Submitter <redacted>
Subject: This is patch one

From: Author Name <redacted>

This is the description

Signed-off-by: Author Name <redacted>
---

This should allow checkpatch to not complain about it too (tip: use
(and fix) `./scripts/checkpatch.pl -g ...HEAD~5` on your tree before
submitting)

- Please use a version numbering when submitting patches (well, at
least use v2 here).
We already saw a first submission, and adding a v2 and the changes is
nicer for the reviewers to know what we can assume that have been
fixed.

More onto each patch.

Cheers,
Benjamin

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
  HID: add on_transport_error() field to struct hid_driver
  HID: add reset() field to struct hid_ll_driver
  HID: add spi-hid, transport driver for HID over SPI bus

 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  | 1487 +++++++++++++++++++++++++++
 drivers/hid/spi-hid/spi-hid-core.h  |  201 ++++
 drivers/hid/spi-hid/spi-hid_trace.h |  197 ++++
 drivers/hid/spi-hid/trace.c         |   11 +
 include/linux/hid.h                 |   24 +
 11 files changed, 1964 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 100644 drivers/hid/spi-hid/spi-hid_trace.h
 create mode 100644 drivers/hid/spi-hid/trace.c

--
2.25.1