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

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

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux