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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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