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
