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
