diff mbox series

[v3,1/4] dt-bindings: input: Introduce Himax HID-over-SPI device

Message ID 20231017091900.801989-2-tylor_yang@himax.corp-partner.google.com (mailing list archive)
State New
Headers show
Series HID: touchscreen: add himax hid-over-spi driver | expand

Commit Message

Tylor Yang Oct. 17, 2023, 9:18 a.m. UTC
The Himax HID-over-SPI framework support for Himax touchscreen ICs
that report HID packet through SPI bus. The driver core need reset
 pin to meet reset timing spec. of IC. An interrupt to disable
and enable interrupt when suspend/resume. Two optional power control
 if target board needed.

Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
---
 .../devicetree/bindings/input/himax,hid.yaml  | 123 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 2 files changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/himax,hid.yaml

Comments

Conor Dooley Oct. 17, 2023, 1:59 p.m. UTC | #1
Yo,

On Tue, Oct 17, 2023 at 05:18:57PM +0800, Tylor Yang wrote:
> The Himax HID-over-SPI framework support for Himax touchscreen ICs
> that report HID packet through SPI bus. The driver core need reset
>  pin to meet reset timing spec. of IC. An interrupt to disable
> and enable interrupt when suspend/resume. Two optional power control
>  if target board needed.
> 
> Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> ---
>  .../devicetree/bindings/input/himax,hid.yaml  | 123 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/himax,hid.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/himax,hid.yaml b/Documentation/devicetree/bindings/input/himax,hid.yaml
> new file mode 100644
> index 000000000000..9ba86fe1b7da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/himax,hid.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/himax,hid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Himax TDDI devices using SPI to send HID packets
> +
> +maintainers:
> +  - Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> +
> +description: |
> +  Support the Himax TDDI devices which using SPI interface to acquire
> +  HID packets from the device. The device needs to be initialized using
> +  Himax protocol before it start sending HID packets.
> +
> +properties:
> +  compatible:
> +    const: himax,hid

This compatible seems far too generic. Why are there not device specific
compatibles for each TDDI device?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset:
> +    maxItems: 1
> +    description: Reset device, active low signal.
> +
> +  vccd-supply:
> +    description:
> +      Optional regulator for the 1.8V voltage.
> +
> +  vcca-supply:
> +    description:
> +      Optional regulator for the analog 3.3V voltage.
> +
> +  himax,id-gpios:
> +    maxItems: 8
> +    description: GPIOs to read physical Panel ID. Optional.
> +
> +  spi-cpha: true
> +  spi-cpol: true

> +  himax,ic-det-delay-ms:
> +    description:
> +      Due to TDDI properties, the TPIC detection timing must after the
> +      display panel initialized. This property is used to specify the
> +      delay time when TPIC detection and display panel initialization
> +      timing are overlapped. How much milliseconds to delay before TPIC
> +      detection start.
> +
> +  himax,ic-resume-delay-ms:
> +    description:
> +      Due to TDDI properties, the TPIC resume timing must after the
> +      display panel resumed. This property is used to specify the
> +      delay time when TPIC resume and display panel resume
> +      timing are overlapped. How much milliseconds to delay before TPIC
> +      resume start.


> +  panel:
> +    description:
> +      The node of the display panel device. The driver will use this
> +      node to get the project ID of the display panel. Optional.
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      himax,pid:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        minItems: 1
> +        maxItems: 8
> +        items:
> +          minimum: 0
> +          maximum: 65535
> +        description:
> +          When only one value exist, represent Project ID of the device.
> +          When multiple values exist, order in event number value represnet
> +          id value from id-gpios and odd number value represent Project ID
> +          relatives to prior id value. This is used to specify the firmware
> +          for the device.

I am sorry, but I still fail to understand why using device specific
compatibles & firmware-name does not work here. It still seems like this
property exists purely because you do not know what device you are
because of a lack of specific compatibles.

Thanks,
Conor.

> +
> +    required:
> +      - himax,pid
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - reset
> +
> +unevaluatedProperties: false
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        touchscreen@0 {
> +            compatible = "himax,hid";
> +            reg = <0x0>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +            pinctrl-0 = <&touch_pins>;
> +            pinctrl-names = "default";
> +
> +            spi-max-frequency = <12500000>;
> +            spi-cpha;
> +            spi-cpol;
> +
> +            reset = <&gpio1 8 GPIO_ACTIVE_LOW>;
> +            himax,ic-det-delay-ms = <500>;
> +            himax,ic-resume-delay-ms = <100>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..883870ab316f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9340,6 +9340,12 @@ L:	linux-kernel@vger.kernel.org
>  S:	Maintained
>  F:	drivers/misc/hisi_hikey_usb.c
>  
> +HIMAX HID OVER SPI TOUCHSCREEN SUPPORT
> +M:	Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> +L:	linux-input@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/input/himax,hid.yaml
> +
>  HIMAX HX83112B TOUCHSCREEN SUPPORT
>  M:	Job Noorman <job@noorman.info>
>  L:	linux-input@vger.kernel.org
> -- 
> 2.25.1
>
Krzysztof Kozlowski Oct. 17, 2023, 4:58 p.m. UTC | #2
On 17/10/2023 15:59, Conor Dooley wrote:
> Yo,
> 
> On Tue, Oct 17, 2023 at 05:18:57PM +0800, Tylor Yang wrote:
>> The Himax HID-over-SPI framework support for Himax touchscreen ICs
>> that report HID packet through SPI bus. The driver core need reset
>>  pin to meet reset timing spec. of IC. An interrupt to disable
>> and enable interrupt when suspend/resume. Two optional power control
>>  if target board needed.
>>
>> Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
>> ---
>>  .../devicetree/bindings/input/himax,hid.yaml  | 123 ++++++++++++++++++
>>  MAINTAINERS                                   |   6 +
>>  2 files changed, 129 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/input/himax,hid.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/input/himax,hid.yaml b/Documentation/devicetree/bindings/input/himax,hid.yaml
>> new file mode 100644
>> index 000000000000..9ba86fe1b7da
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/himax,hid.yaml
>> @@ -0,0 +1,123 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/input/himax,hid.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Himax TDDI devices using SPI to send HID packets
>> +
>> +maintainers:
>> +  - Tylor Yang <tylor_yang@himax.corp-partner.google.com>
>> +
>> +description: |
>> +  Support the Himax TDDI devices which using SPI interface to acquire
>> +  HID packets from the device. The device needs to be initialized using
>> +  Himax protocol before it start sending HID packets.
>> +
>> +properties:
>> +  compatible:
>> +    const: himax,hid
> 
> This compatible seems far too generic. Why are there not device specific
> compatibles for each TDDI device?

Which was pointed out by Rob in v2, so his feedback was ignored.

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  reset:
>> +    maxItems: 1
>> +    description: Reset device, active low signal.

No, come one, read feedback from Rob.

>> +
>> +  vccd-supply:
>> +    description:
>> +      Optional regulator for the 1.8V voltage.
>> +
>> +  vcca-supply:
>> +    description:
>> +      Optional regulator for the analog 3.3V voltage.
>> +
>> +  himax,id-gpios:
>> +    maxItems: 8
>> +    description: GPIOs to read physical Panel ID. Optional.
>> +
>> +  spi-cpha: true
>> +  spi-cpol: true
> 
>> +  himax,ic-det-delay-ms:
>> +    description:
>> +      Due to TDDI properties, the TPIC detection timing must after the
>> +      display panel initialized. This property is used to specify the
>> +      delay time when TPIC detection and display panel initialization
>> +      timing are overlapped. How much milliseconds to delay before TPIC
>> +      detection start.
>> +
>> +  himax,ic-resume-delay-ms:
>> +    description:
>> +      Due to TDDI properties, the TPIC resume timing must after the
>> +      display panel resumed. This property is used to specify the
>> +      delay time when TPIC resume and display panel resume
>> +      timing are overlapped. How much milliseconds to delay before TPIC
>> +      resume start.
> 

No improvements here...

You must implement all feedback from v2. Not pieces of it.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/himax,hid.yaml b/Documentation/devicetree/bindings/input/himax,hid.yaml
new file mode 100644
index 000000000000..9ba86fe1b7da
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/himax,hid.yaml
@@ -0,0 +1,123 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/himax,hid.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax TDDI devices using SPI to send HID packets
+
+maintainers:
+  - Tylor Yang <tylor_yang@himax.corp-partner.google.com>
+
+description: |
+  Support the Himax TDDI devices which using SPI interface to acquire
+  HID packets from the device. The device needs to be initialized using
+  Himax protocol before it start sending HID packets.
+
+properties:
+  compatible:
+    const: himax,hid
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset:
+    maxItems: 1
+    description: Reset device, active low signal.
+
+  vccd-supply:
+    description:
+      Optional regulator for the 1.8V voltage.
+
+  vcca-supply:
+    description:
+      Optional regulator for the analog 3.3V voltage.
+
+  himax,id-gpios:
+    maxItems: 8
+    description: GPIOs to read physical Panel ID. Optional.
+
+  spi-cpha: true
+  spi-cpol: true
+
+  himax,ic-det-delay-ms:
+    description:
+      Due to TDDI properties, the TPIC detection timing must after the
+      display panel initialized. This property is used to specify the
+      delay time when TPIC detection and display panel initialization
+      timing are overlapped. How much milliseconds to delay before TPIC
+      detection start.
+
+  himax,ic-resume-delay-ms:
+    description:
+      Due to TDDI properties, the TPIC resume timing must after the
+      display panel resumed. This property is used to specify the
+      delay time when TPIC resume and display panel resume
+      timing are overlapped. How much milliseconds to delay before TPIC
+      resume start.
+
+  panel:
+    description:
+      The node of the display panel device. The driver will use this
+      node to get the project ID of the display panel. Optional.
+    type: object
+    additionalProperties: false
+
+    properties:
+      himax,pid:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        minItems: 1
+        maxItems: 8
+        items:
+          minimum: 0
+          maximum: 65535
+        description:
+          When only one value exist, represent Project ID of the device.
+          When multiple values exist, order in event number value represnet
+          id value from id-gpios and odd number value represent Project ID
+          relatives to prior id value. This is used to specify the firmware
+          for the device.
+
+    required:
+      - himax,pid
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset
+
+unevaluatedProperties: false
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        touchscreen@0 {
+            compatible = "himax,hid";
+            reg = <0x0>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+            pinctrl-0 = <&touch_pins>;
+            pinctrl-names = "default";
+
+            spi-max-frequency = <12500000>;
+            spi-cpha;
+            spi-cpol;
+
+            reset = <&gpio1 8 GPIO_ACTIVE_LOW>;
+            himax,ic-det-delay-ms = <500>;
+            himax,ic-resume-delay-ms = <100>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 7a7bd8bd80e9..883870ab316f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9340,6 +9340,12 @@  L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	drivers/misc/hisi_hikey_usb.c
 
+HIMAX HID OVER SPI TOUCHSCREEN SUPPORT
+M:	Tylor Yang <tylor_yang@himax.corp-partner.google.com>
+L:	linux-input@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/input/himax,hid.yaml
+
 HIMAX HX83112B TOUCHSCREEN SUPPORT
 M:	Job Noorman <job@noorman.info>
 L:	linux-input@vger.kernel.org