[PATCH v2 0/3] Add Atmel maXTouch support for Peach Pit

[PATCH v2 0/3] Add Atmel maXTouch support for Peach Pit

[Javier Martinez Canillas]

This is a second version of the series that adds support
for the atmel trackpad found on the Exynos5420 Peach Pit.
The first version was [0] but I missunderstood the DT
bindings since I don't have documentation about this hw.
After the feedback provided by Nick Dyer and Yufeng Shen
I think understand now how it works and why my earlier
(wrong) attempt used to work as well.

I've dropped support for Peach Pi since the Atmel chip
in that machine uses a different T100 touchscreen object
instead of the T9 used by the touchpad in Peach Pit. And
that object is not supported by the Atmel mXT driver yet.

The first patch adds the needed DTS changes to the Peach
Pit device tree file, the second patch enables the driver
in the Exynos-specific kernel config and the third patch
enables the driver on the ARMv7 multi-platform config file.

I chose to build as a module in the later because only some
multi-platform supported machines have this touchpad device.

Javier Martinez Canillas (2):
  ARM: exynos_defconfig: Enable Atmel maXTouch support
  ARM: multi_v7_defconfig: Enable Atmel maXTouch support

Sjoerd Simons (1):
  ARM: dts: Add Peach Pit dts entry for Atmel touchpad

 arch/arm/boot/dts/exynos5420-peach-pit.dts | 31 ++++++++++++++++++++++++++++++
 arch/arm/configs/exynos_defconfig          |  1 +
 arch/arm/configs/multi_v7_defconfig        |  1 +
 3 files changed, 33 insertions(+)

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/6/589

[PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Javier Martinez Canillas]

From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

The Peach Pit board has an Atmel maXTouch trackpad device.
Add the needed Device Tree nodes to support it.

This Device Tree change is based on the Chrome OS 3.8 tree
but adapted to use the mainline Atmel maXTouch DT binding.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
[javier.martinez: added linux,gpio-keymap property and changed IRQ type]
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes since v1:
 - Change trackpad IRQ pad function from 0x0 (GPIO input) to 0xf (GPIO IRQ).
   suggested by Tomasz Figa.
 - Remove BTN_TOOL_* from "linux,gpio-keymap" property since those are set
   by input mt core if INPUT_MT_POINTER is set. Suggested by Nick Dyer.
 - Use correct values for "linux,gpio-keymap" property. Suggested by Nick Dyer.
 - Remove support for Peach Pi board since it uses a different Atmel touchpad
   that requires an Atmel object protocol  (T100) not supported by the driver.
 - Use IRQ type constants from <dt-bindings/interrupt-controller/irq.h> instead
   of magic numbers. Suggested by Andreas Farber.
---
 arch/arm/boot/dts/exynos5420-peach-pit.dts | 31 ++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 228a6b1..e4f82d5 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -11,6 +11,7 @@
 /dts-v1/;
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/interrupt-controller/irq.h>
 #include "exynos5420.dtsi"
 
 / {
@@ -157,6 +158,29 @@
 	};
 };
 
+&hsi2c_8 {
+	status = "okay";
+	clock-frequency = <333000>;
+
+	trackpad@4b {
+		compatible="atmel,maxtouch";
+		reg=<0x4b>;
+		interrupt-parent=<&gpx1>;
+		interrupts=<1 IRQ_TYPE_EDGE_FALLING>;
+		wakeup-source;
+		pinctrl-names = "default";
+		pinctrl-0 = <&trackpad_irq>;
+		linux,gpio-keymap = < KEY_RESERVED
+				      KEY_RESERVED
+				      0		/* GPIO 0 */
+				      0		/* GPIO 1 */
+				      0		/* GPIO 2 */
+				      BTN_LEFT 	/* GPIO 3 */
+				      KEY_RESERVED
+				      KEY_RESERVED >;
+	};
+};
+
 &hsi2c_9 {
 	status = "okay";
 	clock-frequency = <400000>;
@@ -249,6 +273,13 @@
 		samsung,pin-drv = <0>;
 	};
 
+	trackpad_irq: trackpad-irq {
+		samsung,pins = "gpx1-1";
+		samsung,pin-function = <0xf>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	power_key_irq: power-key-irq {
 		samsung,pins = "gpx1-2";
 		samsung,pin-function = <0>;
-- 
2.0.1

[PATCH v2 2/3] ARM: exynos_defconfig: Enable Atmel maXTouch support

[Javier Martinez Canillas]

Many Exynos based Chromebooks have an Atmel trackpad so enable
support for it by default will make easier for users.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/configs/exynos_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/exynos_defconfig b/arch/arm/configs/exynos_defconfig
index fc7d168..4e18ab5 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -60,6 +60,7 @@ CONFIG_KEYBOARD_CROS_EC=y
 # CONFIG_MOUSE_PS2 is not set
 CONFIG_MOUSE_CYAPA=y
 CONFIG_INPUT_TOUCHSCREEN=y
+CONFIG_TOUCHSCREEN_ATMEL_MXT=y
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_SAMSUNG=y
 CONFIG_SERIAL_SAMSUNG_CONSOLE=y
-- 
2.0.1

[PATCH v2 3/3] ARM: multi_v7_defconfig: Enable Atmel maXTouch support

[Javier Martinez Canillas]

Some boards with multi platform support (e.g: Exynos based
Chromebooks) have an Atmel trackpad so enable support for
as a module will make easier for users.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 5fb95fb..d139a8d 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -164,6 +164,7 @@ CONFIG_KEYBOARD_ST_KEYSCAN=y
 CONFIG_KEYBOARD_CROS_EC=y
 CONFIG_MOUSE_PS2_ELANTECH=y
 CONFIG_INPUT_TOUCHSCREEN=y
+CONFIG_TOUCHSCREEN_ATMEL_MXT=m
 CONFIG_TOUCHSCREEN_STMPE=y
 CONFIG_INPUT_MISC=y
 CONFIG_INPUT_MPU3050=y
-- 
2.0.1

Re: [PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Andreas Färber]

Hi Javier,

Am 26.08.2014 17:28, schrieb Javier Martinez Canillas:
> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> The Peach Pit board has an Atmel maXTouch trackpad device.
> Add the needed Device Tree nodes to support it.
> 
> This Device Tree change is based on the Chrome OS 3.8 tree
> but adapted to use the mainline Atmel maXTouch DT binding.
> 
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> [javier.martinez: added linux,gpio-keymap property and changed IRQ type]
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Changes since v1:
>  - Change trackpad IRQ pad function from 0x0 (GPIO input) to 0xf (GPIO IRQ).
>    suggested by Tomasz Figa.
>  - Remove BTN_TOOL_* from "linux,gpio-keymap" property since those are set
>    by input mt core if INPUT_MT_POINTER is set. Suggested by Nick Dyer.
>  - Use correct values for "linux,gpio-keymap" property. Suggested by Nick Dyer.
>  - Remove support for Peach Pi board since it uses a different Atmel touchpad
>    that requires an Atmel object protocol  (T100) not supported by the driver.
>  - Use IRQ type constants from <dt-bindings/interrupt-controller/irq.h> instead
>    of magic numbers. Suggested by Andreas Farber.
> ---
>  arch/arm/boot/dts/exynos5420-peach-pit.dts | 31 ++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
> index 228a6b1..e4f82d5 100644
> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
> @@ -11,6 +11,7 @@
>  /dts-v1/;
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
>  #include "exynos5420.dtsi"
>  
>  / {
> @@ -157,6 +158,29 @@
>  	};
>  };
>  
> +&hsi2c_8 {
> +	status = "okay";
> +	clock-frequency = <333000>;
> +
> +	trackpad@4b {
> +		compatible="atmel,maxtouch";
> +		reg=<0x4b>;
> +		interrupt-parent=<&gpx1>;
> +		interrupts=<1 IRQ_TYPE_EDGE_FALLING>;

Nit: Here's a style break (4x spaces around '=' missing).

> +		wakeup-source;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&trackpad_irq>;
> +		linux,gpio-keymap = < KEY_RESERVED
> +				      KEY_RESERVED
> +				      0		/* GPIO 0 */
> +				      0		/* GPIO 1 */
> +				      0		/* GPIO 2 */
> +				      BTN_LEFT 	/* GPIO 3 */
> +				      KEY_RESERVED
> +				      KEY_RESERVED >;
> +	};

Coincidentally, I experimentally came up with a very similar DT node for
Spring the weekend:

+	trackpad@4b {
+		compatible = "atmel,maxtouch";
+		reg = <0x4b>;
+		interrupt-parent = <&gpx1>;
+		interrupts = <2 IRQ_TYPE_NONE>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&trackpad_irq>;
+		linux,gpio-keymap = <KEY_RESERVED
+		                     KEY_RESERVED
+		                     KEY_RESERVED
+		                     KEY_RESERVED
+		                     KEY_RESERVED
+		                     BTN_LEFT>;
+		wakeup-source;
+	};

0 == KEY_RESERVED, so you can consistently use it for GPIO 0-2, too. :)

I probably should add the two trailing _RESERVEDs, too?

With my above snippet I got an awful lot of "Interrupt triggered but
zero messages" warnings (which I simply commented out as quickfix).
Is that why you are using _EDGE_FALLING? Or pin-function 0xf?
(In my case the ChromeOS DT had IRQ_TYPE_NONE and pin-function 0x0.)

Regards,
Andreas

> +};
> +
>  &hsi2c_9 {
>  	status = "okay";
>  	clock-frequency = <400000>;
> @@ -249,6 +273,13 @@
>  		samsung,pin-drv = <0>;
>  	};
>  
> +	trackpad_irq: trackpad-irq {
> +		samsung,pins = "gpx1-1";
> +		samsung,pin-function = <0xf>;
> +		samsung,pin-pud = <0>;
> +		samsung,pin-drv = <0>;
> +	};
> +
>  	power_key_irq: power-key-irq {
>  		samsung,pins = "gpx1-2";
>  		samsung,pin-function = <0>;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Javier Martinez Canillas]

Hello Andreas,

On 08/27/2014 12:53 AM, Andreas Färber wrote:
>>  
>> +&hsi2c_8 {
>> +	status = "okay";
>> +	clock-frequency = <333000>;
>> +
>> +	trackpad@4b {
>> +		compatible="atmel,maxtouch";
>> +		reg=<0x4b>;
>> +		interrupt-parent=<&gpx1>;
>> +		interrupts=<1 IRQ_TYPE_EDGE_FALLING>;
> 
> Nit: Here's a style break (4x spaces around '=' missing).
>

True, these bits were copied from the downstream Chrome OS verbatim and
the missing space around '=' was there, I missed that when reviewing.

I'll re-spin and fix those style issues.

>> +		wakeup-source;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&trackpad_irq>;
>> +		linux,gpio-keymap = < KEY_RESERVED
>> +				      KEY_RESERVED
>> +				      0		/* GPIO 0 */
>> +				      0		/* GPIO 1 */
>> +				      0		/* GPIO 2 */
>> +				      BTN_LEFT 	/* GPIO 3 */
>> +				      KEY_RESERVED
>> +				      KEY_RESERVED >;
>> +	};
> 
> Coincidentally, I experimentally came up with a very similar DT node for
> Spring the weekend:
> 
> +	trackpad@4b {
> +		compatible = "atmel,maxtouch";
> +		reg = <0x4b>;
> +		interrupt-parent = <&gpx1>;
> +		interrupts = <2 IRQ_TYPE_NONE>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&trackpad_irq>;
> +		linux,gpio-keymap = <KEY_RESERVED
> +		                     KEY_RESERVED
> +		                     KEY_RESERVED
> +		                     KEY_RESERVED
> +		                     KEY_RESERVED
> +		                     BTN_LEFT>;
> +		wakeup-source;
> +	};
> 
> 0 == KEY_RESERVED, so you can consistently use it for GPIO 0-2, too. :)
> 

I know that the value of KEY_RESERVED is 0 but I didn't use KEY_RESERVED
for the GPIO on purpose.

What I understood is that the SPT_GPIOPWN_T19 object sends messages using
a status byte so you have a maximum of 8 GPIO but not every maXTouch
devices use all of them. So in the particular case of the device in the
Peach Pit, from the 8 possible GPIO only 4 can be used and these are pins
2-5. So in theory you could connect 3 more GPIO in case you had more
buttons (e.g: BTN_RIGHT, BTN_MIDDLE) but only 1 is used since the
Chromebook just have BTN_LEFT.

Nick sent a patch [0] that extend the atmel touchpad DT binding and the
doc says "Use KEY_RESERVED for unused padding values". But is not clear
what value you should use for GPIO that are actually supported by the
device but have no keycode associated.

So by using 0 instead of KEY_RESERVED I wanted to document that pins 2-4
are actually supported and not reserved by the device but there is no
keycode associated with that GPIO.

If there was a BTN_NONE or KEY_UNUSED it would had been better but I think
that making a distinction between these two cases (reserved pin vs GPIO
available but not used) is useful.

> I probably should add the two trailing _RESERVEDs, too?
> 

I see that is used for properties that are arrays, for example
"linux,keymap" in Documentation/devicetree/bindings/input/matrix-keymap.txt.

> With my above snippet I got an awful lot of "Interrupt triggered but
> zero messages" warnings (which I simply commented out as quickfix).
> Is that why you are using _EDGE_FALLING? Or pin-function 0xf?
> (In my case the ChromeOS DT had IRQ_TYPE_NONE and pin-function 0x0.)
>

These are two separate but related things:

a) IRQF_TRIGGER_FALLING:

Yes, the Chrome OS DT for Peach Pit also has IRQ_TYPE_NONE but the DTS is
not correct.

If you look at the Chrome OS atmel driver
(drivers/input/touchscreen/atmel_mxt_ts.c), it passes IRQF_TRIGGER_FALLING
to request_threaded_irq():

/* Default to falling edge if no platform data provided */
irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING;
error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
			     irqflags | IRQF_ONESHOT,
			     client->name, data);

The above code is wrong since is overwriting the edge/level type flags set
by OF when parsing the "interrupts" property so you have to use the
expected IRQ flags in your DTS.

b) pin-function 0xf instead of 0x0:

The pin-function 0x0 is GPIO input while 0xf is GPIO IRQ. Usually on other
SoCs to use a GPIO IRQ you just configure the pad as GPIO input and then
enable the pin as an interrupt but on Exynos SoC these are really two
different functions. So if you configure the pin as GPIO input and this
happens after the pin is configured as GPIO IRQ, interrupts are not triggered.

I faced that issue before [1] and was solved with Tomasz's commit:

f6a8249 pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs

which changes the pinctrl-exynos driver to setup a pin as GPIO IRQ on
.irq_request_resources instead of .irq_set_type. So, with that patch even
when pin-function re-configures the function to GPIO input, is then
configured as GPIO IRQ when request_threaded_irq() is called.

So probably is working for you just because you tested on linux-next that
already has Tomasz's changes but still the correct thing to do is to setup
the pin as 0xf. This change probably is needed on other pins used as GPIO
IRQ that are using 0x0 now.

Sorry, the email became longer than I wanted but I hope is helpful to you.

> Regards,
> Andreas
> 

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/15/244
[1]: https://lkml.org/lkml/2014/8/8/621

Re: [PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Andreas Färber]

Hi Javier,

Am 27.08.2014 09:13, schrieb Javier Martinez Canillas:
> On 08/27/2014 12:53 AM, Andreas Färber wrote:
>>>  
>>> +&hsi2c_8 {
>>> +	status = "okay";
>>> +	clock-frequency = <333000>;
>>> +
>>> +	trackpad@4b {
>>> +		compatible="atmel,maxtouch";
>>> +		reg=<0x4b>;
>>> +		interrupt-parent=<&gpx1>;
>>> +		interrupts=<1 IRQ_TYPE_EDGE_FALLING>;
>>
>> Nit: Here's a style break (4x spaces around '=' missing).
>>
> 
> True, these bits were copied from the downstream Chrome OS verbatim and
> the missing space around '=' was there, I missed that when reviewing.
> 
> I'll re-spin and fix those style issues.
> 
>>> +		wakeup-source;
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&trackpad_irq>;
>>> +		linux,gpio-keymap = < KEY_RESERVED
>>> +				      KEY_RESERVED
>>> +				      0		/* GPIO 0 */
>>> +				      0		/* GPIO 1 */
>>> +				      0		/* GPIO 2 */
>>> +				      BTN_LEFT 	/* GPIO 3 */
>>> +				      KEY_RESERVED
>>> +				      KEY_RESERVED >;
>>> +	};
>>
>> Coincidentally, I experimentally came up with a very similar DT node for
>> Spring the weekend:
>>
>> +	trackpad@4b {
>> +		compatible = "atmel,maxtouch";
>> +		reg = <0x4b>;
>> +		interrupt-parent = <&gpx1>;
>> +		interrupts = <2 IRQ_TYPE_NONE>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&trackpad_irq>;
>> +		linux,gpio-keymap = <KEY_RESERVED
>> +		                     KEY_RESERVED
>> +		                     KEY_RESERVED
>> +		                     KEY_RESERVED
>> +		                     KEY_RESERVED
>> +		                     BTN_LEFT>;
>> +		wakeup-source;
>> +	};
>>
>> 0 == KEY_RESERVED, so you can consistently use it for GPIO 0-2, too. :)
>>
> 
> I know that the value of KEY_RESERVED is 0 but I didn't use KEY_RESERVED
> for the GPIO on purpose.
> 
> What I understood is that the SPT_GPIOPWN_T19 object sends messages using
> a status byte so you have a maximum of 8 GPIO but not every maXTouch
> devices use all of them. So in the particular case of the device in the
> Peach Pit, from the 8 possible GPIO only 4 can be used and these are pins
> 2-5. So in theory you could connect 3 more GPIO in case you had more
> buttons (e.g: BTN_RIGHT, BTN_MIDDLE) but only 1 is used since the
> Chromebook just have BTN_LEFT.

FWIW when I press to the bottom right of my touchpad, I do get
right-click functionality even with just BTN_LEFT specified in the
keymap. Magic. :)

> Nick sent a patch [0] that extend the atmel touchpad DT binding and the
> doc says "Use KEY_RESERVED for unused padding values". But is not clear
> what value you should use for GPIO that are actually supported by the
> device but have no keycode associated.
> 
> So by using 0 instead of KEY_RESERVED I wanted to document that pins 2-4
> are actually supported and not reserved by the device but there is no
> keycode associated with that GPIO.

You already documented that via comments though.

> If there was a BTN_NONE or KEY_UNUSED it would had been better but I think
> that making a distinction between these two cases (reserved pin vs GPIO
> available but not used) is useful.

Maybe Nick can comment here.

>> I probably should add the two trailing _RESERVEDs, too?
>>
> 
> I see that is used for properties that are arrays, for example
> "linux,keymap" in Documentation/devicetree/bindings/input/matrix-keymap.txt.

That does not answer my question: Do all maxTouch touchpads (or
specifically that in Spring) need eight entries, padded with said
KEY_RESERVED? In my experiments using just six entries (i.e., until the
non-zero entry) worked okay - so does this T19 message have specifically
eight bits? Tegra used just four entries iirc.

>> With my above snippet I got an awful lot of "Interrupt triggered but
>> zero messages" warnings (which I simply commented out as quickfix).
>> Is that why you are using _EDGE_FALLING? Or pin-function 0xf?
>> (In my case the ChromeOS DT had IRQ_TYPE_NONE and pin-function 0x0.)
>>
> 
> These are two separate but related things:
> 
> a) IRQF_TRIGGER_FALLING:
> 
> Yes, the Chrome OS DT for Peach Pit also has IRQ_TYPE_NONE but the DTS is
> not correct.
> 
> If you look at the Chrome OS atmel driver
> (drivers/input/touchscreen/atmel_mxt_ts.c), it passes IRQF_TRIGGER_FALLING
> to request_threaded_irq():
> 
> /* Default to falling edge if no platform data provided */
> irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING;
> error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
> 			     irqflags | IRQF_ONESHOT,
> 			     client->name, data);
> 
> The above code is wrong since is overwriting the edge/level type flags set
> by OF when parsing the "interrupts" property so you have to use the
> expected IRQ flags in your DTS.
> 
> b) pin-function 0xf instead of 0x0:
> 
> The pin-function 0x0 is GPIO input while 0xf is GPIO IRQ. Usually on other
> SoCs to use a GPIO IRQ you just configure the pad as GPIO input and then
> enable the pin as an interrupt but on Exynos SoC these are really two
> different functions. So if you configure the pin as GPIO input and this
> happens after the pin is configured as GPIO IRQ, interrupts are not triggered.
> 
> I faced that issue before [1] and was solved with Tomasz's commit:
> 
> f6a8249 pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs
> 
> which changes the pinctrl-exynos driver to setup a pin as GPIO IRQ on
> .irq_request_resources instead of .irq_set_type. So, with that patch even
> when pin-function re-configures the function to GPIO input, is then
> configured as GPIO IRQ when request_threaded_irq() is called.
> 
> So probably is working for you just because you tested on linux-next that
> already has Tomasz's changes but still the correct thing to do is to setup
> the pin as 0xf. This change probably is needed on other pins used as GPIO
> IRQ that are using 0x0 now.
> 
> Sorry, the email became longer than I wanted but I hope is helpful to you.

Thanks for the explanations, I'll test those settings on Spring then.

Could you point me to what ChromeOS tree and branch I should be looking
at? For instance, the linux-next.git chromeos-3.8 branch did not have
any DT for Spring. Therefore my series is based on /proc/device-tree
rather than any ChromeOS source code.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Javier Martinez Canillas]

Hello Andreas,

On 08/27/2014 03:11 PM, Andreas Färber wrote:
> Hi Javier,
>>>
>>> +	trackpad@4b {
>>> +		compatible = "atmel,maxtouch";
>>> +		reg = <0x4b>;
>>> +		interrupt-parent = <&gpx1>;
>>> +		interrupts = <2 IRQ_TYPE_NONE>;
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&trackpad_irq>;
>>> +		linux,gpio-keymap = <KEY_RESERVED
>>> +		                     KEY_RESERVED
>>> +		                     KEY_RESERVED
>>> +		                     KEY_RESERVED
>>> +		                     KEY_RESERVED
>>> +		                     BTN_LEFT>;
>>> +		wakeup-source;
>>> +	};
>>>
>>> 0 == KEY_RESERVED, so you can consistently use it for GPIO 0-2, too. :)
>>>
>> 
>> I know that the value of KEY_RESERVED is 0 but I didn't use KEY_RESERVED
>> for the GPIO on purpose.
>> 
>> What I understood is that the SPT_GPIOPWN_T19 object sends messages using
>> a status byte so you have a maximum of 8 GPIO but not every maXTouch
>> devices use all of them. So in the particular case of the device in the
>> Peach Pit, from the 8 possible GPIO only 4 can be used and these are pins
>> 2-5. So in theory you could connect 3 more GPIO in case you had more
>> buttons (e.g: BTN_RIGHT, BTN_MIDDLE) but only 1 is used since the
>> Chromebook just have BTN_LEFT.
> 
> FWIW when I press to the bottom right of my touchpad, I do get
> right-click functionality even with just BTN_LEFT specified in the
> keymap. Magic. :)
>

Right, I'm not an expert in input but after asking a colleague he
explained to me that user-space input drivers deal with the MT tracking
for you, but there is still a single event code reported (BTN_LEFT) as an
indication that the touchpad has been physically clicked. That's why you
are having right click even when the hardware is not reporting it.

You can confirm that by running plain evdev or by clicking on the touchpad
using an object (e.g: a pencil) so ABS_MT_* events won't be reported.

>> Nick sent a patch [0] that extend the atmel touchpad DT binding and the
>> doc says "Use KEY_RESERVED for unused padding values". But is not clear
>> what value you should use for GPIO that are actually supported by the
>> device but have no keycode associated.
>> 
>> So by using 0 instead of KEY_RESERVED I wanted to document that pins 2-4
>> are actually supported and not reserved by the device but there is no
>> keycode associated with that GPIO.
> 
> You already documented that via comments though.
>

Yes but still I don't like to use KEY_RESERVED for all the pins since some
of them are not really reserved. Can we agree on disagree here and after
Nick answer I can post as a follow-up patch after this series gets merged?

>> If there was a BTN_NONE or KEY_UNUSED it would had been better but I think
>> that making a distinction between these two cases (reserved pin vs GPIO
>> available but not used) is useful.
> 
> Maybe Nick can comment here.
> 
>>> I probably should add the two trailing _RESERVEDs, too?
>>>
>> 
>> I see that is used for properties that are arrays, for example
>> "linux,keymap" in Documentation/devicetree/bindings/input/matrix-keymap.txt.
> 
> That does not answer my question: Do all maxTouch touchpads (or
> specifically that in Spring) need eight entries, padded with said
> KEY_RESERVED? In my experiments using just six entries (i.e., until the
> non-zero entry) worked okay - so does this T19 message have specifically
> eight bits? Tegra used just four entries iirc.
> 

I guess there are other touchpads that have a different offset (e.g: using
the last 4 pins) so I think that is good as a way of documenting the
layout even if not strictly necessary for the driver.

But again, without proper documentation is hard to say so I'll let Nick to
answer that.

>>> With my above snippet I got an awful lot of "Interrupt triggered but
>>> zero messages" warnings (which I simply commented out as quickfix).
>>> Is that why you are using _EDGE_FALLING? Or pin-function 0xf?
>>> (In my case the ChromeOS DT had IRQ_TYPE_NONE and pin-function 0x0.)
>>>
>> 
>> These are two separate but related things:
>> 
>> a) IRQF_TRIGGER_FALLING:
>> 
>> Yes, the Chrome OS DT for Peach Pit also has IRQ_TYPE_NONE but the DTS is
>> not correct.
>> 
>> If you look at the Chrome OS atmel driver
>> (drivers/input/touchscreen/atmel_mxt_ts.c), it passes IRQF_TRIGGER_FALLING
>> to request_threaded_irq():
>> 
>> /* Default to falling edge if no platform data provided */
>> irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING;
>> error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
>> 			     irqflags | IRQF_ONESHOT,
>> 			     client->name, data);
>> 
>> The above code is wrong since is overwriting the edge/level type flags set
>> by OF when parsing the "interrupts" property so you have to use the
>> expected IRQ flags in your DTS.
>> 
>> b) pin-function 0xf instead of 0x0:
>> 
>> The pin-function 0x0 is GPIO input while 0xf is GPIO IRQ. Usually on other
>> SoCs to use a GPIO IRQ you just configure the pad as GPIO input and then
>> enable the pin as an interrupt but on Exynos SoC these are really two
>> different functions. So if you configure the pin as GPIO input and this
>> happens after the pin is configured as GPIO IRQ, interrupts are not triggered.
>> 
>> I faced that issue before [1] and was solved with Tomasz's commit:
>> 
>> f6a8249 pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs
>> 
>> which changes the pinctrl-exynos driver to setup a pin as GPIO IRQ on
>> .irq_request_resources instead of .irq_set_type. So, with that patch even
>> when pin-function re-configures the function to GPIO input, is then
>> configured as GPIO IRQ when request_threaded_irq() is called.
>> 
>> So probably is working for you just because you tested on linux-next that
>> already has Tomasz's changes but still the correct thing to do is to setup
>> the pin as 0xf. This change probably is needed on other pins used as GPIO
>> IRQ that are using 0x0 now.
>> 
>> Sorry, the email became longer than I wanted but I hope is helpful to you.
> 
> Thanks for the explanations, I'll test those settings on Spring then.
> 
> Could you point me to what ChromeOS tree and branch I should be looking
> at? For instance, the linux-next.git chromeos-3.8 branch did not have
> any DT for Spring. Therefore my series is based on /proc/device-tree
> rather than any ChromeOS source code.
> 

I use as a reference
https://chromium.googlesource.com/chromiumos/third_party/kernel branch
chromeos-3.8

> Thanks,
> Andreas
> 

Best regards,
Javier

Re: [PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Nick Dyer]

On 27/08/14 15:22, Javier Martinez Canillas wrote:
>>> If there was a BTN_NONE or KEY_UNUSED it would had been better but I think
>>> that making a distinction between these two cases (reserved pin vs GPIO
>>> available but not used) is useful.
>>
>> Maybe Nick can comment here.

Yes, this is probably useful to document. However, I fear that it's not
going to be obvious what the distinction is to someone who doesn't have the
Atmel docs. Perhaps it would be clearer to just do something like:

		linux,gpio-keymap = <KEY_RESERVED
		                     KEY_RESERVED
		                     KEY_RESERVED /* GPIO0 */
	        	             KEY_RESERVED /* GPIO1 */
		                     KEY_RESERVED /* GPIO2 */
		                     BTN_LEFT>;   /* GPIO3 */

If you omit any trailing KEY_RESERVED values it doesn't affect anything.

I was also going to suggest that you put something like "/* Atmel mXT224SL
*/" in your .dts file so it's clear what version of the maXTouch device you
are configuring.

Re: [PATCH v2 1/3] ARM: dts: Add Peach Pit dts entry for Atmel touchpad

[Javier Martinez Canillas]

Hello Nick,

On 09/02/2014 03:46 PM, Nick Dyer wrote:
> On 27/08/14 15:22, Javier Martinez Canillas wrote:
>>>> If there was a BTN_NONE or KEY_UNUSED it would had been better but I think
>>>> that making a distinction between these two cases (reserved pin vs GPIO
>>>> available but not used) is useful.
>>>
>>> Maybe Nick can comment here.
> 
> Yes, this is probably useful to document. However, I fear that it's not
> going to be obvious what the distinction is to someone who doesn't have the
> Atmel docs. Perhaps it would be clearer to just do something like:
> 
> 		linux,gpio-keymap = <KEY_RESERVED
> 		                     KEY_RESERVED
> 		                     KEY_RESERVED /* GPIO0 */
> 	        	             KEY_RESERVED /* GPIO1 */
> 		                     KEY_RESERVED /* GPIO2 */
> 		                     BTN_LEFT>;   /* GPIO3 */
> 
> If you omit any trailing KEY_RESERVED values it doesn't affect anything.
> 
> I was also going to suggest that you put something like "/* Atmel mXT224SL
> */" in your .dts file so it's clear what version of the maXTouch device you
> are configuring.
> 

Ok, I'll re-spin with those changes. Thanks a lot for your feedback.

Best regards,
Javier