Re: [PATCH v4 3/6] Documentation: DT bindings for Microsoft G6 Touch Digitizer

[Dmitry Antipov <dmanti@xxxxxxxxxxxxx>]

On Fri, Feb 25, 2022 at 06:59AM, Rob Herring wrote:
> 
> On Thu, Feb 24, 2022 at 04:59:33PM -0800, Dmitry Antipov wrote:
> > From: Dmitry Antipov <dmanti@xxxxxxxxxxxxx>
> 
> Please follow the conventions of the subsystem for the subject:
> 
> dt-bindings: input: ...

Will do, thank you.

> 
> >
> > 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.
> >
> > Signed-off-by: Dmitry Antipov <dmanti@xxxxxxxxxxxxx>
> > ---
> >  .../input/microsoft,g6-touch-digitizer.yaml   | 105 ++++++++++++++++++
> >  1 file changed, 105 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/input/microsoft,g6-touch-digitizer
> > .y
> > aml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/input/microsoft,g6-touch-digitiz
> > er
> > .yaml
> > b/Documentation/devicetree/bindings/input/microsoft,g6-touch-digitiz
> > er
> > .yaml
> > new file mode 100644
> > index 000000000000..e516717527e9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/microsoft,g6-touch-dig
> > +++ it
> > +++ izer.yaml
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: microsoft,g6-touch-digitizer
> > +      - items:
> > +        - const: microsoft,g6-touch-digitizer
> > +        - const: hid-over-spi
> 
> Why are both cases needed?
> 
> Assuming you keep the 2nd case, you will need a custom 'select' to 
> avoid applying this schema to another binding using 'hid-over-spi':
> 
> select:
>   properties:
>     compatible:
>       contains:
> 	const: microsoft,g6-touch-digitizer
> 
>   required:
>     - compatible
> 

We decided to make the driver compatible with "microsoft,g6-touch-digitizer" as
well as "hid-over-spi", so the next revision of the patch will list
"microsoft,g6-touch-digitizer" as the only compatible value.

> 
> > +
> > +  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
> > +    description:
> > +      This property and the rest are described in HID Over SPI 
> > + Protocol Spec 1.0
> 
> Each property needs a description and a more specific spec location.
> 
> No constraints on the values? 0 - 2^32 is valid?
> 

Thank you. Will provide description for each property and will specify the bit
length of each address if outlined by the spec.

> > +
> > +  input-report-body-address:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  output-report-address:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  read-opcode:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  write-opcode:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  hid-over-spi-flags:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> 
> > +  post-power-on-delay-ms:
> > +    description:
> > +      Optional time in ms required by the device after enabling its regulators
> > +      or powering it on, before it is ready for communication.
> > +
> > +  minimal-reset-delay-ms:
> > +    description:
> > +      Optional minimum amount of time in ms that device needs to be 
> > + in
> reset
> > +      state for the reset to take effect.
> 
> These should be implied by the compatible string.

If I'm understanding you correctly here, you are asking for the driver to have
the heuristics to apply specific delays based on which compatible string is
published in the device tree. However, the same touch digitizer but on different
boards may have different delay requirements based on the voltage regulator used
or line capacitance. Therefore, I would argue it is better to have this delay
information provided by the device tree.

> 
> > +
> > +required:
> > +  - compatible
> > +  - interrupts
> > +  - reset-gpios
> 
> It's not allowed to have reset under h/w control?

No, the spec requires a discrete reset line.

> 
> > +  - vdd-supply
> > +  - input-report-header-address
> > +  - input-report-body-address
> > +  - output-report-address
> > +  - read-opcode
> > +  - write-opcode
> > +  - hid-over-spi-flags
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    spi-hid-dev0 {
> 
> hid@0
> 
> And you'll need to define a spi bus.

Thank you, will address in the next patch.

> 
> > +      compatible = "microsoft,g6-touch-digitizer", "hid-over-spi";
> > +      reg = <0>;
> > +      interrupts-extended = <&gpio 42 IRQ_TYPE_EDGE_FALLING>;
> > +      reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
> > +      vdd-supply = <&pm8350c_l3>;
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&ts_d6_reset_assert &ts_d6_int_bias>;
> > +      input-report-header-address = <0x1000>;
> > +      input-report-body-address = <0x1004>;
> > +      output-report-address = <0x2000>;
> > +      read-opcode = <0x0b>;
> > +      write-opcode = <0x02>;
> > +      hid-over-spi-flags = <0x00>;
> > +      post-power-on-delay-ms = <5>;
> > +      minimal-reset-delay-ms = <5>;
> > +    };
> > --
> > 2.25.1
> >
> >