Re: [PATCH] ARM: dts: iwg22d-sodimm: Enable touchscreen

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marian-Cristian,

On Tue, Feb 4, 2020 at 10:23 AM Marian-Cristian Rotariu
<marian-cristian.rotariu.rb@xxxxxxxxxxxxxx> wrote:
> In one of the iWave-G22D development board variants, called Generic SODIMM
> Development Platform, we have an LCD with touchscreen. The resistive touch
> controller, STMPE811 is on the development board and is connected through
> the i2c5 of the RZ-G1E.
>
> Additionally, this controller should generate an interrupt to the CPU and
> it is connected through GPIO4,4 to the GIC.
>
> Touch was tested with one of our iW-RainboW-G22D-SODIMM RZ/G1E development
> platforms.
>
> More details on the iWave website:
> https://www.iwavesystems.com/rz-g1e-sodimm-development-kit.html
>
> Signed-off-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@xxxxxxxxxxxxxx>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/arch/arm/boot/dts/r8a7745-iwg22d-sodimm.dts
> +++ b/arch/arm/boot/dts/r8a7745-iwg22d-sodimm.dts
> @@ -128,6 +128,47 @@
>         status = "okay";
>         clock-frequency = <400000>;
>
> +       stmpe811@44 {
> +               compatible = "st,stmpe811";

According to the DT bindings, this must be "st,stmpe-ts", but the example
also uses "st,stmpe811"?

> +               #address-cells = <1>;
> +               #size-cells = <0>;

Not documented in the DT bindings (but used in the example).

> +               reg = <0x44>;
> +               interrupt-parent = <&gpio4>;
> +               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;

This should be "<4 IRQ_TYPE_LEVEL_LOW>", to refer to GP4_4.

> +               irq-gpio = <&gpio4 4 IRQ_TYPE_LEVEL_LOW>;

"irq-gpio" is not documented in the DT bindings.

> +               pinctrl-0 = <&touch>;
> +               pinctrl-names = "default";
> +               id = <0>;
> +               blocks = <0x5>;
> +               irq-trigger = <0x1>;

Above 3 are not documented.

> +
> +               stmpe_touchscreen {

Missing unit-address.

> +                       compatible = "st,stmpe-ts";
> +                       reg = <0>;
> +                       /* 3.25 MHz ADC clock speed */
> +                       st,adc-freq = <3>;
> +                       /* 8 sample average control */
> +                       st,ave-ctrl = <2>;
> +                       /* 7 length fractional part in z */
> +                       st,fraction-z = <7>;
> +                       /*
> +                        * 50 mA typical 80 mA max touchscreen drivers
> +                        * current limit value
> +                        */
> +                       st,i-drive = <0>;

Bindings say <0> corresponds to 20 mA.
Either the comment is wrong, or this should be <1>.

> +                       /* 12-bit ADC */
> +                       st,mod-12b = <1>;
> +                       /* internal ADC reference */
> +                       st,ref-sel = <0>;
> +                       /* ADC converstion time: 80 clocks */
> +                       st,sample-time = <4>;
> +                       /* 1 ms panel driver settling time */
> +                       st,settling = <3>;
> +                       /* 5 ms touch detect interrupt delay */
> +                       st,touch-det-delay = <4>;

Bindings say <4> corresponds to 1 ms.
Either the comment is wrong, or this should be <5>.

> +               };
> +       };
> +
>         sgtl5000: codec@a {
>                 compatible = "fsl,sgtl5000";
>                 #sound-dai-cells = <0>;
> @@ -181,6 +222,11 @@
>                 function = "ssi";
>         };
>
> +       touch: stmpe811 {
> +               groups = "intc_irq0";
> +               function = "intc";

This does not match using GP4_4 for the interrupt.

Either you use GP4_4:

    interrupt-parent = <&gpio4>;
    interrupts = <4 IRQ_TYPE_LEVEL_LOW>;

which doesn't require any explicit pin control setup (the gpio-rcar
driver takes care of that),
or you use IRQ0:

    interrupt-parent = <&irqc>;
    interrupts = <0 IRQ_TYPE_LEVEL_LOW>;

The latter does require explicit pin control setup.

> +       };
> +
>         usb0_pins: usb0 {
>                 groups = "usb0";
>                 function = "usb0";

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux