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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6021 bytes --]

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?

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.

> It also probably firmware-dependent.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]