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@xxxxxxxxxxxxxxx: 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@xxxxxxxxxxxxxxx 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> > > > + > > > +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@xxxxxxxxxxxxxxx > > > S: Maintained > > > F: drivers/misc/hisi_hikey_usb.c > > > > > > +HIMAX HID OVER SPI TOUCHSCREEN SUPPORT > > > +M: Tylor Yang <tylor_yang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> > > > +L: linux-input@xxxxxxxxxxxxxxx > > > +S: Supported > > > +F: Documentation/devicetree/bindings/input/himax,hid-over-spi.yaml > > > + > > > HIMAX HX83112B TOUCHSCREEN SUPPORT > > > M: Job Noorman <job@xxxxxxxxxxxx> > > > L: linux-input@xxxxxxxxxxxxxxx > > > -- > > > 2.25.1 > > > > >
Attachment:
signature.asc
Description: PGP signature