Re: [PATCH v3 09/11] dt-bindings: input: Document hid-over-spi DT schema

[Benjamin Tissoires <bentiss@kernel.org>]

On Apr 13 2026, Rob Herring wrote:
> On Fri, Apr 10, 2026 at 06:35:00PM +0100, Conor Dooley wrote:
> > On Thu, Apr 09, 2026 at 10:16:46AM -0700, Dmitry Torokhov wrote:
> > > On Thu, Apr 09, 2026 at 05:02:11PM +0100, Conor Dooley wrote:
> > > > On Thu, Apr 02, 2026 at 01:59:46AM +0000, Jingyuan Liang wrote:
> > > > > Documentation describes the required and optional properties for
> > > > > implementing Device Tree for a Microsoft G6 Touch Digitizer that
> > > > > supports HID over SPI Protocol 1.0 specification.
> > > > > 
> > > > > The properties are common to HID over SPI.
> > > > > 
> > > > > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> > > > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> > > > > Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
> > > > > ---
> > > > >  .../devicetree/bindings/input/hid-over-spi.yaml    | 126 +++++++++++++++++++++
> > > > >  1 file changed, 126 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..d1b0a2e26c32
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> > > > > @@ -0,0 +1,126 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/input/hid-over-spi.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: HID over SPI Devices
> > > > > +
> > > > > +maintainers:
> > > > > +  - Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > > +  - Jiri Kosina <jkosina@suse.cz>
> > > > 
> > > > Why them and not you, the developers of the series?
> > > > 
> > > > > +
> > > > > +description: |+
> > > > > +  HID over SPI provides support for various Human Interface Devices over the
> > > > > +  SPI bus. These devices can be for example touchpads, keyboards, touch screens
> > > > > +  or sensors.
> > > > > +
> > > > > +  The specification has been written by Microsoft and is currently available
> > > > > +  here: https://www.microsoft.com/en-us/download/details.aspx?id=103325
> > > > > +
> > > > > +  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.
> > > > 
> > > > This is not relevant to the binding, please remove it.
> > > > 
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    oneOf:
> > > > > +      - items:
> > > > > +          - enum:
> > > > > +              - microsoft,g6-touch-digitizer
> > > > > +          - const: hid-over-spi
> > > > > +      - description: Just "hid-over-spi" alone is allowed, but not recommended.
> > > > > +        const: hid-over-spi
> > > > 
> > > > Why is it allowed but not recommended? Seems to me like we should
> > > > require device-specific compatibles.
> > > 
> > > Why would we want to change the driver code to add a new compatible each
> > > time a vendor decides to create a chip that is fully hid-spi-protocol
> > > compliant? Or is the plan to still allow "hid-over-spi" fallback but
> > > require device-specific compatible that will be ignored unless there is
> > > device-specific quirk needed?
> 
> The plan is the latter case (the 1st entry up above). The comment is 
> remove the 2nd entry (with 'Just "hid-over-spi" alone is allowed, but 
> not recommended.').
> 
> > This has nothing to do with the driver, just the oddity of having a
> > comment saying that not having a device specific compatible was
> > permitted by not recommended in a binding. Requiring device-specific
> > compatibles is the norm after all and a comment like this makes draws
> > more attention to the fact that this is abnormal. Regardless of what the
> > driver does, device-specific compatibles should be required.
> > 
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  reset-gpios:
> > > > > +    maxItems: 1
> > > > > +    description:
> > > > > +      GPIO specifier for the digitizer's reset pin (active low). The line must
> > > > > +      be flagged with GPIO_ACTIVE_LOW.
> > > > > +
> > > > > +  vdd-supply:
> > > > > +    description:
> > > > > +      Regulator for the VDD supply voltage.
> > > > > +
> > > > > +  input-report-header-address:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    minimum: 0
> > > > > +    maximum: 0xffffff
> > > > > +    description:
> > > > > +      A value to be included in the Read Approval packet, listing an address of
> > > > > +      the input report header to be put on the SPI bus. This address has 24
> > > > > +      bits.
> > > > > +
> > > > > +  input-report-body-address:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    minimum: 0
> > > > > +    maximum: 0xffffff
> > > > > +    description:
> > > > > +      A value to be included in the Read Approval packet, listing an address of
> > > > > +      the input report body to be put on the SPI bus. This address has 24 bits.
> > > > > +
> > > > > +  output-report-address:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    minimum: 0
> > > > > +    maximum: 0xffffff
> > > > > +    description:
> > > > > +      A value to be included in the Output Report sent by the host, listing an
> > > > > +      address where the output report on the SPI bus is to be written to. This
> > > > > +      address has 24 bits.
> > > > > +
> > > > > +  read-opcode:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > > +    description:
> > > > > +      Value to be used in Read Approval packets. 1 byte.
> > > > > +
> > > > > +  write-opcode:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > > +    description:
> > > > > +      Value to be used in Write Approval packets. 1 byte.
> > > > 
> > > > Why can none of these things be determined from the device's compatible?
> > > > On the surface, they like the kinds of things that could/should be.
> > > 
> > > Why would we want to keep tables of these values in the kernel and again
> > > have to update the driver for each new chip?
> > 
> > That's pretty normal though innit? It's what match data does.
> > If someone wants to have properties that communicate data that
> > can be determined from the compatible, they need to provide
> > justification why it is being done.
> 
> IIRC, it was explained in prior versions the spec itself says these 
> values vary by device. If we expect variation, then I think these 
> properties are fine. But please capture the reasoning for them in this 
> patch or we will just keep asking the same questions over and over. 
> 

Dmitry, FWIW, we roughly had the exact same of argument with Rob with
i2c-hid :)

It took me a while, but I finally understood the rationale and agreed to
it (using the i2c-hid examples):

Most i2c-hid devices are following the spec and rely purely on ACPI and
the DT declaration of i2c-hid -> they are working fine and we don't need
anything else for them. They declare their compatible and the i2c-hid
compatible, and they work great.

But some devices need a reset line. But the i2c-hid spec doesn't mention
a reset line at all. And some other devices need a reset line with a
different timing. etc... Relying purely on the i2c-hid driver means that
the driver needs to now the platform the device is on and the exact
device we have in front of us. i2c-hid provide a VID/PID through the
protocol, but we are still lacking information: in some cases, the
timing of the reset line for the same device differs depending on the
platform they run.

Having a device specific compatible means that we can make use of it
before we load i2c-hid. This is why we have i2c-hid-core and module
specifics on the side. Those extra module can do all the oddities they
want, like having 2 or 3 reset lines, but in the end they are using the
core i2c-hid once they are set up.

Think of it as a way to quirk the device upfront without polluting the
i2c-hid processing.

That allowed us to clean up the i2c-hid code by removing the non
standard regulators, reset lines, quirks that are device specific and
keep it closer to the spec.

Cheers,
Benjamin