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

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