| Message ID | 1580808174-11289-1-git-send-email-marian-cristian.rotariu.rb@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | ARM: dts: iwg22d-sodimm: Enable touchscreen | expand |
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
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
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
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
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";