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

[Conor Dooley <conor@kernel.org>]

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

On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:
> Hi Conor,
> 
> > > Additional optional arguments:
> > > ic-det-delay-ms and ic-resume-delay-ms are using to solve runtime
> > > conditions.
> 
> > Runtime conditions? Aren't thєse properties of the panel & therefore
> > fixed? If they were runtime conditions, then setting them statically in
> > your DT is not going to work, right?
> 
> Because each platform's display driver ready time is different. TP part
> need to avoid this timing by measuring the waveform of LCD reset pin
> low period and TP probe timing. For example, if LCD rst pin low from
> timestamp 100 to 800, TP driver probe at 600. TP probe will fail. Then
> user should set ic-det-delay-ms bigger than 200, to avoid LCD rst low
> timing. As you can see, the timing needs to be measured at runtime to
> decide how long it should be. Then, if the condition is not changed, the
> value could keep the same.

That sounds to me like something you would test once for a given
platform and then the values are static. If you are actually changing it
at *runtime*, how is doing it through DT suitable? Does your firmware do
the tests & then set the values in DT dynamically?

> 
> > It looks like you deleted all of the properties from the previous
> > submission of these changes. I don't really understand that, it kinda
> > feels just like appeasement, as you must have needed those properties
> > to do the firmware loading etc. How are you filling the gap those
> > properties have left, when you still only have a single compatible
> > string in thㄟs binding? Is there a way to do runtime detection of which
> > chip you're dealing with that you are now using?
> 
> After reviewing, I found the properties could go to IC driver settings :
> "himax,heatmap_16bits" because it depends on IC's ability;

How do you detect the IC's abilities?

> Some
> could remove and use default values: "himax,fw_size",
> "himax,boot_time_fw_upgrade". "himax,fw_size" has a default value in
> IC settings, and likely won't change in this IC.

Okay.

> The behavior of "himax,boot_time_fw_upgrade" seems not stable and
> should be removed. "himax,fw_in_flash", I use the kernel config for
> user to select.

That seems like a bad idea, we want to be able to build one kernel that
works for all hardware at the same time.

> "himax,pid" could be remove and use default firmware name
> "himax_i2chid.bin" to load. It was added because users may desire to
> choose a special name like "himax_i2chid_{pid}.bin" instead of the default
> one.
> It also could be replaced with newly added "himax",id-gpios" which is still
> experimental.

Also, pleae don't top post, but instead reply in-line with my comments,
as I have done here.

> Btw, I encounter an error of patch [2/2], which says:
> BOUNCE linux-input@vger.kernel.org: Message too long (>100000 chars)
> and the patch didn't appear at patchwork.kernel.org. What should I do to
> deal with this problem?

No idea. Maybe try to split it into multiple patches?
The other option is to also cc patches@lists.linux.dev as that has some
higher capacities, but that's not going to be a silver bullet.

Thanks,
Conor.
> 
> 
> Thanks,
> Tylor
> 
> 
> On Tue, Sep 19, 2023 at 4:41 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > Hey,
> >
> >
> > On Tue, Sep 19, 2023 at 10:49:42AM +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 pin to disable
> > > and enable interrupt when suspend/resume. An optional power control
> > > pin if target board needed. Panel id pins for identify panel is also
> > > an option.
> > >
> > > Additional optional arguments:
> > > ic-det-delay-ms and ic-resume-delay-ms are using to solve runtime
> > > conditions.
> >
> > Runtime conditions? Aren't thєse properties of the panel & therefore
> > fixed? If they were runtime conditions, then setting them statically in
> > your DT is not going to work, right?
> >
> > >
> > > This patch also add maintainer of this driver.
> > >
> > > Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> >
> > It looks like you deleted all of the properties from the previous
> > submission of these changes. I don't really understand that, it kinda
> > feels just like appeasement, as you must have needed those properties
> > to do the firmware loading etc. How are you filling the gap those
> > properties have left, when you still only have a single compatible
> > string in thㄟs binding? Is there a way to do runtime detection of which
> > chip you're dealing with that you are now using?
> >
> > Confused,
> > Conor.
> >
> > > ---
> > >  .../bindings/input/himax,hid-over-spi.yaml    | 109 ++++++++++++++++++
> > >  MAINTAINERS                                   |   6 +
> > >  2 files changed, 115 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/input/himax,hid-over-spi.yaml
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/input/himax,hid-over-spi.yaml
> > b/Documentation/devicetree/bindings/input/himax,hid-over-spi.yaml
> > > new file mode 100644
> > > index 000000000000..3ee3a89842ac
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/himax,hid-over-spi.yaml
> > > @@ -0,0 +1,109 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/himax,hid-over-spi.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-over-spi
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  himax,rst-gpio:
> > > +    maxItems: 1
> > > +    description: Reset device, active low signal.
> > > +
> > > +  himax,irq-gpio:
> > > +    maxItems: 1
> > > +    description: Interrupt request, active low signal.
> > > +
> > > +  himax,3v3-gpio:
> > > +    maxItems: 1
> > > +    description: GPIO to control 3.3V power supply.
> > > +
> > > +  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.
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - himax,rst-gpio
> > > +  - himax,irq-gpio
> > > +
> > > +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-over-spi";
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            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;
> > > +
> > > +            himax,rst-gpio = <&gpio1 8 GPIO_ACTIVE_LOW>;
> > > +            himax,irq-gpio = <&gpio1 7 GPIO_ACTIVE_LOW>;
> > > +            himax,3v3-gpio = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > > +            himax,ic-det-delay-ms = <500>;
> > > +            himax,ic-resume-delay-ms = <100>;
> > > +        };
> > > +    };
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index bf0f54c24f81..452701261bec 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9323,6 +9323,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-over-spi.yaml
> > > +
> > >  HIMAX HX83112B TOUCHSCREEN SUPPORT
> > >  M:   Job Noorman <job@noorman.info>
> > >  L:   linux-input@vger.kernel.org
> > > --
> > > 2.25.1
> > >
> >

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