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

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

[Marian-Cristian Rotariu]

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@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm/boot/dts/r8a7745-iwg22d-sodimm.dts | 46 +++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7745-iwg22d-sodimm.dts b/arch/arm/boot/dts/r8a7745-iwg22d-sodimm.dts
index ce6603b..1051d82 100644
--- 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";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x44>;
+		interrupt-parent = <&gpio4>;
+		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+		irq-gpio = <&gpio4 4 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-0 = <&touch>;
+		pinctrl-names = "default";
+		id = <0>;
+		blocks = <0x5>;
+		irq-trigger = <0x1>;
+
+		stmpe_touchscreen {
+			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>;
+			/* 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>;
+		};
+	};
+
 	sgtl5000: codec@a {
 		compatible = "fsl,sgtl5000";
 		#sound-dai-cells = <0>;
@@ -181,6 +222,11 @@
 		function = "ssi";
 	};
 
+	touch: stmpe811 {
+		groups = "intc_irq0";
+		function = "intc";
+	};
+
 	usb0_pins: usb0 {
 		groups = "usb0";
 		function = "usb0";
-- 
2.7.4

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

[Geert Uytterhoeven]

Hi Marian-Cristian,

On Tue, Feb 4, 2020 at 10:23 AM Marian-Cristian Rotariu
<marian-cristian.rotariu.rb@bp.renesas.com> 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@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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@linux-m68k.org

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

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

[Marian-Cristian Rotariu]

Hi Geert,

Thank you for the review.

> > --- 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"?

The device is a MFD and the bindings doc is here:
Documentation/devicetree/bindings/mfd/stmpe.txt

You need to add its specific function as a child node of the mfd dt node. In our
case it is a touchscreen:
Documentation/devicetree/bindings/input/touchscreen/stmpe.txt

> 
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> 
> Not documented in the DT bindings (but used in the example).

In the child node you do not need the reg property, therefore the example is not
applicable. I will remove the above in the v2 patch.

> 
> > +               reg = <0x44>;
> > +               interrupt-parent = <&gpio4>;
> > +               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> 
> This should be "<4 IRQ_TYPE_LEVEL_LOW>", to refer to GP4_4.

Indeed, I will fix it in v2.

> 
> > +               irq-gpio = <&gpio4 4 IRQ_TYPE_LEVEL_LOW>;
> 
> "irq-gpio" is not documented in the DT bindings.

See "Documentation/devicetree/bindings/mfd/stmpe.txt"

> 
> > +               pinctrl-0 = <&touch>;
> > +               pinctrl-names = "default";
> > +               id = <0>;
> > +               blocks = <0x5>;
> > +               irq-trigger = <0x1>;
> 
> Above 3 are not documented.

These must be relics from an old driver that we have for an older kernel
version. I will remove all 3 as the current driver is not using them.

> 
> > +
> > +               stmpe_touchscreen {
> 
> Missing unit-address.

I will remove the reg property, therefore no unit-address requirement.

> 
> > +                       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>.

I will add the value that matches the one from the comment.

> 
> > +                       /* 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>.

As above.

> 
> > +               };
> > +       };
> > +
> >         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.

I will use the first approach as the patch looks more compact.

Best regards,
Marian

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

[Geert Uytterhoeven]

Hi Marian,

On Wed, Mar 4, 2020 at 1:38 PM Marian-Cristian Rotariu
<marian-cristian.rotariu.rb@bp.renesas.com> wrote:
> > > --- 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"?
>
> The device is a MFD and the bindings doc is here:
> Documentation/devicetree/bindings/mfd/stmpe.txt

Thanks, I hadn't considered that file when looking for "st,stmpe811",
due to the regex used in the document.

> You need to add its specific function as a child node of the mfd dt node. In our
> case it is a touchscreen:
> Documentation/devicetree/bindings/input/touchscreen/stmpe.txt

OK.

> > > +               reg = <0x44>;
> > > +               interrupt-parent = <&gpio4>;
> > > +               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> >
> > This should be "<4 IRQ_TYPE_LEVEL_LOW>", to refer to GP4_4.
>
> Indeed, I will fix it in v2.
>
> >
> > > +               irq-gpio = <&gpio4 4 IRQ_TYPE_LEVEL_LOW>;
> >
> > "irq-gpio" is not documented in the DT bindings.
>
> See "Documentation/devicetree/bindings/mfd/stmpe.txt"

I believe you cannot use the same GPIO as an interrupt and as a GPIO at
the same time.  Don't you get a -EBUSY somewhere?
Perhaps it worked due to the typo above?

As both interrupts and irq-gpio are documented to be optional
properties, probably they are mutually exclusive, and you can drop
irq-gpio?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Marian-Cristian Rotariu]

Hi Geert,

> > > > +               reg = <0x44>;
> > > > +               interrupt-parent = <&gpio4>;
> > > > +               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > >
> > > This should be "<4 IRQ_TYPE_LEVEL_LOW>", to refer to GP4_4.
> >
> > Indeed, I will fix it in v2.
> >
> > >
> > > > +               irq-gpio = <&gpio4 4 IRQ_TYPE_LEVEL_LOW>;
> > >
> > > "irq-gpio" is not documented in the DT bindings.
> >
> > See "Documentation/devicetree/bindings/mfd/stmpe.txt"
> 
> I believe you cannot use the same GPIO as an interrupt and as a GPIO at the
> same time.  Don't you get a -EBUSY somewhere?
> Perhaps it worked due to the typo above?
> 
> As both interrupts and irq-gpio are documented to be optional properties,
> probably they are mutually exclusive, and you can drop irq-gpio?

Yes, this is redundant. They are mutually exclusive in the driver code with
irq-gpio having precedence over the interrupts/interrupt-parent registration. 
Therefore, I will remove the irq-gpio property as interrupts/interrupt-parent
pair does the job.

Thank you,
Marian