[PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Javier Martinez Canillas]

The Atmel maXTouch driver assumed that the IRQ type flags will
always be passed using platform data but this is not true when
booting using Device Trees. In these setups the interrupt type
was ignored by the driver when requesting an IRQ.

This means that it will fail if a machine specified other type
than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
that was parsed by OF from the "interrupt" Device Tree propery.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 03b8571..0fb56c9 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -22,6 +22,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c/atmel_mxt_ts.h>
 #include <linux/input/mt.h>
+#include <linux/irq.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/slab.h>
@@ -2130,6 +2131,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	struct mxt_data *data;
 	const struct mxt_platform_data *pdata;
 	int error;
+	unsigned long irqflags;
 
 	pdata = dev_get_platdata(&client->dev);
 	if (!pdata) {
@@ -2156,8 +2158,13 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	init_completion(&data->reset_completion);
 	init_completion(&data->crc_completion);
 
+	if (client->dev.of_node)
+		irqflags = irq_get_trigger_type(client->irq);
+	else
+		irqflags = pdata->irqflags;
+
 	error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
-				     pdata->irqflags | IRQF_ONESHOT,
+				     irqflags | IRQF_ONESHOT,
 				     client->name, data);
 	if (error) {
 		dev_err(&client->dev, "Failed to register interrupt\n");
-- 
2.0.0.rc2

[PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Javier Martinez Canillas]

The Atmel maXTouch driver allows to dynamically define the
event keycodes set that the device is capable of. This may
not be evident when reading the DT binding document so add
an example to make it easier to understand how this works.

Also, the current documentation says that the array limit
is four entries but the driver dynamically allocates the
keymap array and does not limit the array size.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index baef432..be50476 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -11,7 +11,7 @@ Required properties:
 
 Optional properties for main touchpad device:
 
-- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
+- linux,gpio-keymap: An array of entries indicating the Linux
     keycode generated by each GPIO. Linux keycodes are defined in
     <dt-bindings/input/input.h>.
 
@@ -22,4 +22,10 @@ Example:
 		reg = <0x4b>;
 		interrupt-parent = <&gpio>;
 		interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
+		linux,gpio-keymap = < BTN_LEFT
+				      BTN_TOOL_FINGER
+				      BTN_TOOL_DOUBLETAP
+				      BTN_TOOL_TRIPLETAP
+				      BTN_TOOL_QUADTAP
+				      BTN_TOOL_QUINTTAP >;
 	};
-- 
2.0.0.rc2

Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Nick Dyer]

On 07/08/14 01:48, Javier Martinez Canillas wrote:
> The Atmel maXTouch driver allows to dynamically define the
> event keycodes set that the device is capable of. This may
> not be evident when reading the DT binding document so add
> an example to make it easier to understand how this works.
> 
> Also, the current documentation says that the array limit
> is four entries but the driver dynamically allocates the
> keymap array and does not limit the array size.

There is a physical limit to the number of GPIOs on the device. The number
4 is wrong, the protocol does allow for up to 8 GPIOs. But it is a hard limit.

> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
> index baef432..be50476 100644
> --- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
> +++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
> @@ -11,7 +11,7 @@ Required properties:
>  
>  Optional properties for main touchpad device:
>  
> -- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
> +- linux,gpio-keymap: An array of entries indicating the Linux
>      keycode generated by each GPIO. Linux keycodes are defined in
>      <dt-bindings/input/input.h>.
>  
> @@ -22,4 +22,10 @@ Example:
>  		reg = <0x4b>;
>  		interrupt-parent = <&gpio>;
>  		interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
> +		linux,gpio-keymap = < BTN_LEFT
> +				      BTN_TOOL_FINGER
> +				      BTN_TOOL_DOUBLETAP
> +				      BTN_TOOL_TRIPLETAP
> +				      BTN_TOOL_QUADTAP
> +				      BTN_TOOL_QUINTTAP >;

I'm afraid you have misunderstood the impact of this change to the way that
the GPIOs coming in to the touch controller are mapped to key codes. Look
at the protocol guide for T19.

The DOUBLE/TRIPLE/QUAD/QUINTTAP stuff is filled in for us by the input core
when we use INPUT_MT_POINTER, anyway.

Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Javier Martinez Canillas]

Hello Nick,

On 08/07/2014 02:38 PM, Nick Dyer wrote:
>> 
>> Also, the current documentation says that the array limit
>> is four entries but the driver dynamically allocates the
>> keymap array and does not limit the array size.
> 
> There is a physical limit to the number of GPIOs on the device. The number
> 4 is wrong, the protocol does allow for up to 8 GPIOs. But it is a hard limit.
>

Thanks a lot for the explanation, then I guess s/4/8 is enough.

>> 
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>  Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
>> index baef432..be50476 100644
>> --- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
>> +++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
>> @@ -11,7 +11,7 @@ Required properties:
>>  
>>  Optional properties for main touchpad device:
>>  
>> -- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
>> +- linux,gpio-keymap: An array of entries indicating the Linux
>>      keycode generated by each GPIO. Linux keycodes are defined in
>>      <dt-bindings/input/input.h>.
>>  
>> @@ -22,4 +22,10 @@ Example:
>>  		reg = <0x4b>;
>>  		interrupt-parent = <&gpio>;
>>  		interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
>> +		linux,gpio-keymap = < BTN_LEFT
>> +				      BTN_TOOL_FINGER
>> +				      BTN_TOOL_DOUBLETAP
>> +				      BTN_TOOL_TRIPLETAP
>> +				      BTN_TOOL_QUADTAP
>> +				      BTN_TOOL_QUINTTAP >;
> 
> I'm afraid you have misunderstood the impact of this change to the way that
> the GPIOs coming in to the touch controller are mapped to key codes. Look

Unfortunately there are no boards in mainline using this "linux,gpio-keymap"
property so I tried to figure out what the expected values were by reading the
driver. So is more than possible that I got them wrong.

By passing all these keycodes the touchpad worked as expected for me and the
driver did the same than the Chrome OS driver that has these keycodes hardcoded
when is_tp is true.

> at the protocol guide for T19.
> 

I don't have access to proper documentation and I wouldn't expect people to have
access to non-public docs in order to use a Device Tree binding.

That's why I wanted to document an example, so using this property could be
easier for others and they shouldn't have to look at the driver in order to
figure it out (and getting it wrong as you said :) )

So it would be great if you could provide an example on how this is supposed to
be used.

> The DOUBLE/TRIPLE/QUAD/QUINTTAP stuff is filled in for us by the input core
> when we use INPUT_MT_POINTER, anyway.
> 

Thanks for the hint, I didn't know that this was the case but I just looked at
input_mt_init_slots() [0] and indeed those are not needed.

Best regards,
Javier

[0]: http://lxr.free-electrons.com/source/drivers/input/input-mt.c#L69

Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Javier Martinez Canillas]

Hello Nick,

On 08/08/2014 04:52 PM, Javier Martinez Canillas wrote:
On 08/07/2014 02:38 PM, Nick Dyer wrote:
>> >>
>> 
>> I'm afraid you have misunderstood the impact of this change to the way that
>> the GPIOs coming in to the touch controller are mapped to key codes. Look
> 
> Unfortunately there are no boards in mainline using this "linux,gpio-keymap"
> property so I tried to figure out what the expected values were by reading the
> driver. So is more than possible that I got them wrong.
> 
> By passing all these keycodes the touchpad worked as expected for me and the
> driver did the same than the Chrome OS driver that has these keycodes hardcoded
> when is_tp is true.
> 
>> at the protocol guide for T19.
>> 
> 
> I don't have access to proper documentation and I wouldn't expect people to have
> access to non-public docs in order to use a Device Tree binding.
> 
> That's why I wanted to document an example, so using this property could be
> easier for others and they shouldn't have to look at the driver in order to
> figure it out (and getting it wrong as you said :) )
> 
> So it would be great if you could provide an example on how this is supposed to
> be used.
> 

Any comments on this? I would really appreciate if you can expand on how
this DT property is supposed to be used so I can re-spin the atmel support
patch for Peach boards.

Thanks a lot and best regards,
Javier

Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Nick Dyer]

On 15/08/14 13:01, Javier Martinez Canillas wrote:
>> By passing all these keycodes the touchpad worked as expected for me and the
>> driver did the same than the Chrome OS driver that has these keycodes hardcoded
>> when is_tp is true.
>>
>>> at the protocol guide for T19.
>>
>> I don't have access to proper documentation and I wouldn't expect people to have
>> access to non-public docs in order to use a Device Tree binding.
>>
>> That's why I wanted to document an example, so using this property could be
>> easier for others and they shouldn't have to look at the driver in order to
>> figure it out (and getting it wrong as you said :) )
>>
>> So it would be great if you could provide an example on how this is supposed to
>> be used.
> 
> Any comments on this? I would really appreciate if you can expand on how
> this DT property is supposed to be used so I can re-spin the atmel support
> patch for Peach boards.

The below patch improves the documentation for the gpio-property. Stephen
Warren has a good example here:
https://github.com/swarren/linux-tegra/commit/09789801

trackpad@4b {
  compatible = "atmel,maxtouch";
  reg = <0x4b>;
  interrupt-parent = <&gpio>;
  interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
  linux,gpio-keymap = <0 0 0 BTN_LEFT>;
};

This maps BTN_LEFT to the 4th bit of the T19 message. I haven't looked up
what GPIO number that corresponds to on the mXT224SL that he has, it varies
with the particular maXTouch device you have.

Hope this helps.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index baef432..1852906 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -11,10 +11,17 @@ Required properties:

 Optional properties for main touchpad device:

-- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
-    keycode generated by each GPIO. Linux keycodes are defined in
+- linux,gpio-keymap: When enabled, the SPT_GPIOPWN_T19 object sends messages
+    on GPIO bit changes. An array of up to 8 entries can be provided
+    indicating the Linux keycode mapped to each bit of the status byte,
+    starting at the LSB. Linux keycodes are defined in
     <dt-bindings/input/input.h>.

+    Note: the numbering of the GPIOs and the bit they start at varies between
+    maXTouch devices. You must either refer to the documentation, or
+    experiment to determine which bit corresponds to which input. Use
+    KEY_RESERVED for unused padding values.
+
 Example:

        touch@4b {
-- 
1.9.1

Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Stephen Warren]

On 08/15/2014 10:08 AM, Nick Dyer wrote:
> On 15/08/14 13:01, Javier Martinez Canillas wrote:
>>> By passing all these keycodes the touchpad worked as expected for me and the
>>> driver did the same than the Chrome OS driver that has these keycodes hardcoded
>>> when is_tp is true.
>>>
>>>> at the protocol guide for T19.
>>>
>>> I don't have access to proper documentation and I wouldn't expect people to have
>>> access to non-public docs in order to use a Device Tree binding.
>>>
>>> That's why I wanted to document an example, so using this property could be
>>> easier for others and they shouldn't have to look at the driver in order to
>>> figure it out (and getting it wrong as you said :) )
>>>
>>> So it would be great if you could provide an example on how this is supposed to
>>> be used.
>>
>> Any comments on this? I would really appreciate if you can expand on how
>> this DT property is supposed to be used so I can re-spin the atmel support
>> patch for Peach boards.
>
> The below patch improves the documentation for the gpio-property.

That patch makes sense, and is a nice description,
Acked-by: Stephen Warren <swarren@nvidia.com>

> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt

>   Example:
>
>          touch@4b {
>

Perhaps it makes sense to add a linux,gpio-keymap property into the 
example too though; IIRC there was an earlier patch to the docs that did 
this?

[PATCH] Input: atmel_mxt_ts - fix merge in DT documentation

[Nick Dyer]

On 15/08/14 17:13, Stephen Warren wrote:
>>> Any comments on this? I would really appreciate if you can expand on how
>>> this DT property is supposed to be used so I can re-spin the atmel support
>>> patch for Peach boards.
>>
>> The below patch improves the documentation for the gpio-property.
> 
> That patch makes sense, and is a nice description,
> Acked-by: Stephen Warren <swarren@nvidia.com>

Hi Dmitry-

Something went a bit wrong in merging f5940231a - there's a bit of repeated
text that's been introduced.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index 0ac23f2..1852906 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -11,10 +11,6 @@ Required properties:

 Optional properties for main touchpad device:

-- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
-    keycode generated by each GPIO. Linux keycodes are defined in
-    <dt-bindings/input/input.h>.
-
 - linux,gpio-keymap: When enabled, the SPT_GPIOPWN_T19 object sends messages
     on GPIO bit changes. An array of up to 8 entries can be provided
     indicating the Linux keycode mapped to each bit of the status byte,
-- 
1.9.1

Re: [PATCH] Input: atmel_mxt_ts - fix merge in DT documentation

[Dmitry Torokhov]

On Thu, Sep 11, 2014 at 03:52:46PM +0100, Nick Dyer wrote:
> On 15/08/14 17:13, Stephen Warren wrote:
> >>> Any comments on this? I would really appreciate if you can expand on how
> >>> this DT property is supposed to be used so I can re-spin the atmel support
> >>> patch for Peach boards.
> >>
> >> The below patch improves the documentation for the gpio-property.
> > 
> > That patch makes sense, and is a nice description,
> > Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> Hi Dmitry-
> 
> Something went a bit wrong in merging f5940231a - there's a bit of repeated
> text that's been introduced.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>

Hmm, not sure how I messed this up, but I will queue the patch for the
next push.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example

[Javier Martinez Canillas]

Hello Nick,

On 08/15/2014 06:08 PM, Nick Dyer wrote:
>> 
>> Any comments on this? I would really appreciate if you can expand on how
>> this DT property is supposed to be used so I can re-spin the atmel support
>> patch for Peach boards.
> 
> The below patch improves the documentation for the gpio-property. Stephen
> Warren has a good example here:
> https://github.com/swarren/linux-tegra/commit/09789801
> 
> trackpad@4b {
>   compatible = "atmel,maxtouch";
>   reg = <0x4b>;
>   interrupt-parent = <&gpio>;
>   interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
>   linux,gpio-keymap = <0 0 0 BTN_LEFT>;
> };
> 
> This maps BTN_LEFT to the 4th bit of the T19 message. I haven't looked up
> what GPIO number that corresponds to on the mXT224SL that he has, it varies
> with the particular maXTouch device you have.
> 
> Hope this helps.
>

Thanks a lot for the patch and the pointer to Stephen's DTS. I'll do some
experimentation then to figure out the right values since I don't have
proper documentation. Fortunately the Chrome OS 3.8 downstream driver
works on -next so I can use evdev to compare if both drivers behave the same.

> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>

Best regards,
Javier

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Tomasz Figa]

Hi Javier,

On 07.08.2014 02:48, Javier Martinez Canillas wrote:
> The Atmel maXTouch driver assumed that the IRQ type flags will
> always be passed using platform data but this is not true when
> booting using Device Trees. In these setups the interrupt type
> was ignored by the driver when requesting an IRQ.
> 
> This means that it will fail if a machine specified other type
> than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
> that was parsed by OF from the "interrupt" Device Tree propery.

Have you observed an actual failure due to this? I believe that
irq_of_parse_and_map() already sets up IRQ trigger type based on DT
data, by calling irq_create_of_mapping() which in turn calls
irq_set_irq_type().

> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 03b8571..0fb56c9 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -22,6 +22,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c/atmel_mxt_ts.h>
>  #include <linux/input/mt.h>
> +#include <linux/irq.h>
>  #include <linux/interrupt.h>
>  #include <linux/of.h>
>  #include <linux/slab.h>
> @@ -2130,6 +2131,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	struct mxt_data *data;
>  	const struct mxt_platform_data *pdata;
>  	int error;
> +	unsigned long irqflags;
>  
>  	pdata = dev_get_platdata(&client->dev);
>  	if (!pdata) {
> @@ -2156,8 +2158,13 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	init_completion(&data->reset_completion);
>  	init_completion(&data->crc_completion);
>  
> +	if (client->dev.of_node)
> +		irqflags = irq_get_trigger_type(client->irq);

It might be a bit cleaner to just assign the flags to pdata->irqflags in
mxt_parse_dt() instead. That would also account for the fact that pdata,
if provided, should have priority over DT.

Best regards,
Tomasz

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Javier Martinez Canillas]

Hello Tomasz,

Thanks a lot for your feedback.

On 08/07/2014 03:14 AM, Tomasz Figa wrote:
> Hi Javier,
> 
> 
> Have you observed an actual failure due to this? I believe that

Yes, I found this issue since the driver was not taking into account the value
defined in the edge/level type cells from the "interrupts" DT property.

Only doing the change in the following patch was not enough:

[PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad [0].

> irq_of_parse_and_map() already sets up IRQ trigger type based on DT
> data, by calling irq_create_of_mapping() which in turn calls
> irq_set_irq_type().
>

Right but somehow when the IRQ is actually requested the type is overwritten by
the value passed to request_threaded_irq() and interrupts are not being
generated by the device without this patch.

Do you think that this is a bug in the "interrupt-parent" irqchip driver or the
IRQ core? I'm not that familiar with the IRQ subsystem.

>>  
>> +	if (client->dev.of_node)
>> +		irqflags = irq_get_trigger_type(client->irq);
> 
> It might be a bit cleaner to just assign the flags to pdata->irqflags in
> mxt_parse_dt() instead. That would also account for the fact that pdata,
> if provided, should have priority over DT.
> 

You are totally right, also this will break if CONFIG_OF is not enabled since
dev.of_node will not be defined. While this already is taken into account for
mxt_parse_dt() by defining an empty function.

I'll change it in v2 if getting the flags from the driver is the right approach
instead of fixing the irqchip driver or the IRQ core.

> Best regards,
> Tomasz
> 

Best regards,
Javier

[0]: http://www.spinics.net/lists/kernel/msg1802099.html

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Dmitry Torokhov]

On Thu, Aug 07, 2014 at 03:47:24AM +0200, Javier Martinez Canillas wrote:
> Hello Tomasz,
> 
> Thanks a lot for your feedback.
> 
> On 08/07/2014 03:14 AM, Tomasz Figa wrote:
> > Hi Javier,
> > 
> > 
> > Have you observed an actual failure due to this? I believe that
> 
> Yes, I found this issue since the driver was not taking into account the value
> defined in the edge/level type cells from the "interrupts" DT property.
> 
> Only doing the change in the following patch was not enough:
> 
> [PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad [0].
> 
> > irq_of_parse_and_map() already sets up IRQ trigger type based on DT
> > data, by calling irq_create_of_mapping() which in turn calls
> > irq_set_irq_type().
> >
> 
> Right but somehow when the IRQ is actually requested the type is overwritten by
> the value passed to request_threaded_irq() and interrupts are not being
> generated by the device without this patch.
> 
> Do you think that this is a bug in the "interrupt-parent" irqchip driver or the
> IRQ core? I'm not that familiar with the IRQ subsystem.

No, this is clearly driver fault - it smashed previously done setup with new
flags.

> 
> >>  
> >> +	if (client->dev.of_node)
> >> +		irqflags = irq_get_trigger_type(client->irq);
> > 
> > It might be a bit cleaner to just assign the flags to pdata->irqflags in
> > mxt_parse_dt() instead. That would also account for the fact that pdata,
> > if provided, should have priority over DT.
> > 
> 
> You are totally right, also this will break if CONFIG_OF is not enabled since
> dev.of_node will not be defined. While this already is taken into account for
> mxt_parse_dt() by defining an empty function.
> 
> I'll change it in v2 if getting the flags from the driver is the right approach

Yes, please.

> instead of fixing the irqchip driver or the IRQ core.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Javier Martinez Canillas]

Hello Dmitry,

On 08/07/2014 08:09 AM, Dmitry Torokhov wrote:
>> 
>> > irq_of_parse_and_map() already sets up IRQ trigger type based on DT
>> > data, by calling irq_create_of_mapping() which in turn calls
>> > irq_set_irq_type().
>> >
>> 
>> Right but somehow when the IRQ is actually requested the type is overwritten by
>> the value passed to request_threaded_irq() and interrupts are not being
>> generated by the device without this patch.
>> 
>> Do you think that this is a bug in the "interrupt-parent" irqchip driver or the
>> IRQ core? I'm not that familiar with the IRQ subsystem.
> 
> No, this is clearly driver fault - it smashed previously done setup with new
> flags.
>

Thanks a lot for the clarification. That was my understanding as well but wanted
to be sure.

>> > 
>> > It might be a bit cleaner to just assign the flags to pdata->irqflags in
>> > mxt_parse_dt() instead. That would also account for the fact that pdata,
>> > if provided, should have priority over DT.
>> > 
>> 
>> You are totally right, also this will break if CONFIG_OF is not enabled since
>> dev.of_node will not be defined. While this already is taken into account for
>> mxt_parse_dt() by defining an empty function.
>> 
>> I'll change it in v2 if getting the flags from the driver is the right approach
> 
> Yes, please.
> 

Just posted a v2 [0] with Tomasz suggestion and the patch is indeed a lot cleaner.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/7/82

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Dmitry Torokhov]

On Thu, Aug 07, 2014 at 09:49:49AM +0200, Javier Martinez Canillas wrote:
> Hello Dmitry,
> 
> On 08/07/2014 08:09 AM, Dmitry Torokhov wrote:
> >> 
> >> > irq_of_parse_and_map() already sets up IRQ trigger type based on DT
> >> > data, by calling irq_create_of_mapping() which in turn calls
> >> > irq_set_irq_type().
> >> >
> >> 
> >> Right but somehow when the IRQ is actually requested the type is overwritten by
> >> the value passed to request_threaded_irq() and interrupts are not being
> >> generated by the device without this patch.
> >> 
> >> Do you think that this is a bug in the "interrupt-parent" irqchip driver or the
> >> IRQ core? I'm not that familiar with the IRQ subsystem.
> > 
> > No, this is clearly driver fault - it smashed previously done setup with new
> > flags.
> >
> 
> Thanks a lot for the clarification. That was my understanding as well but wanted
> to be sure.

Actually, I take this back. In mainline everything as it should: if
pdata does not specify particular trigger the flags requested end up
being IRQF_ONESHOT, which should preserve trigger bits previously set up
by the board or OF code. In Chrome kernel we have:

	/* 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);

which I believe should go away. If it is needed on ACPI systems we need
to figure out how to do things we can do with OF there.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Javier Martinez Canillas]

Hello,

On 08/07/2014 06:47 PM, Dmitry Torokhov wrote:
> 
> Actually, I take this back. In mainline everything as it should: if
> pdata does not specify particular trigger the flags requested end up
> being IRQF_ONESHOT, which should preserve trigger bits previously set up
> by the board or OF code. In Chrome kernel we have:
> 

In theory it should work as Dmitry and Nick said since if no trigger flags are
set then whatever was set before (by OF, platform code, ACPI) should be used.

I also verified what Tomasz mentioned that the IRQ trigger type is parsed and
set correctly by OF:

irq_of_parse_and_map()
	 irq_create_of_mapping()
		irq_set_irq_type()
			__irq_set_trigger()
				chip->irq_set_type()
					exynos_irq_set_type()

But for some reason it doesn't work for me unless I set the trigger type in the
flags passed to request_threaded_irq().

I found that what makes it to work is the logic in __setup_irq() [0] that Nick
pointed out on a previous email:

		/* Setup the type (level, edge polarity) if configured: */
		if (new->flags & IRQF_TRIGGER_MASK) {
			ret = __irq_set_trigger(desc, irq,
					new->flags & IRQF_TRIGGER_MASK);

			if (ret)
				goto out_mask;
		}

So __irq_set_trigger() is only executed if the struct irqaction flags has a
trigger flag which makes sense since this shouldn't be necessary with DT due
__irq_set_trigger() being called from irq_create_of_mapping() before when the
"interrupts" property is parsed.

But for some reason it is necessary for me... I checked that struct irq_data
state_use_accessors value is correct and even tried setting that value to
new->flags after the mentioned code block but it makes no difference. Input
events are not reported by evtest and AFAICT interrupts are not being generated.

It works though if the trigger type is passed to request_threaded_irq() like
$subject does or if new->flags are set before the new->flags & IRQF_TRIGGER_MASK
conditional.

For example, with the following changes interrupts are fired correctly:

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..2d8adbb 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1169,6 +1169,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
struct irqaction *new)

                init_waitqueue_head(&desc->wait_for_threads);

+               if (!(new->flags & IRQF_TRIGGER_MASK))
+                       new->flags |= irqd_get_trigger_type(desc);
+
                /* Setup the type (level, edge polarity) if configured: */
                if (new->flags & IRQF_TRIGGER_MASK) {
                        ret = __irq_set_trigger(desc, irq,

Any ideas what could be wrong here?
				
> 	/* 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);
>

Exactly, that's how I found the issue. When I compared both drivers I noticed
that the Chrome OS driver did that and since all the supported platforms are DT
based, the above is equivalent to just IRQF_TRIGGER_FALLING | IRQF_ONESHOT.

So according to my explanation, new->flags & IRQF_TRIGGER_MASK is true so
__irq_set_trigger() is executed and that's why it works with the Chrome OS driver.

In fact the Chrome OS DTS does not set a trigger type in the "interrupts" property:

trackpad@4b {
	reg=<0x4b>;
	compatible="atmel,atmel_mxt_tp";
	interrupts=<1 0>;
	interrupt-parent=<&gpx1>;
	wakeup-source;
};


> which I believe should go away. If it is needed on ACPI systems we need
> to figure out how to do things we can do with OF there.
> 

The above code should not be related to ACPI systems since whatever code that
parses an ACPI table should just call irq_set_irq_type() like is made by OF, so
request_threaded_irq() should just work with ACPI too.

I agree it should go away but first I want to understand why is needed in the
first place. Unfortunately commit:

031f136 ("Input: atmel_mxt_ts - Set default irqflags when there is no pdata")

from the Chrome OS 3.8 does not explain why this is needed, instead of adding
this information in the DTS (e.g: interrupts=<1 IRQ_TYPE_EDGE_FALLING>).

> Thanks.
>

Best regards,
Javier

[0]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1172

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Tomasz Figa]

+CC Thomas, Jason and Ben

On 08.08.2014 15:24, Javier Martinez Canillas wrote:
> Hello,
> 
> On 08/07/2014 06:47 PM, Dmitry Torokhov wrote:
>>
>> Actually, I take this back. In mainline everything as it should: if
>> pdata does not specify particular trigger the flags requested end up
>> being IRQF_ONESHOT, which should preserve trigger bits previously set up
>> by the board or OF code. In Chrome kernel we have:
>>
> 
> In theory it should work as Dmitry and Nick said since if no trigger flags are
> set then whatever was set before (by OF, platform code, ACPI) should be used.
> 
> I also verified what Tomasz mentioned that the IRQ trigger type is parsed and
> set correctly by OF:
> 
> irq_of_parse_and_map()
> 	 irq_create_of_mapping()
> 		irq_set_irq_type()
> 			__irq_set_trigger()
> 				chip->irq_set_type()
> 					exynos_irq_set_type()
> 
> But for some reason it doesn't work for me unless I set the trigger type in the
> flags passed to request_threaded_irq().
> 
> I found that what makes it to work is the logic in __setup_irq() [0] that Nick
> pointed out on a previous email:
> 
> 		/* Setup the type (level, edge polarity) if configured: */
> 		if (new->flags & IRQF_TRIGGER_MASK) {
> 			ret = __irq_set_trigger(desc, irq,
> 					new->flags & IRQF_TRIGGER_MASK);
> 
> 			if (ret)
> 				goto out_mask;
> 		}
> 
> So __irq_set_trigger() is only executed if the struct irqaction flags has a
> trigger flag which makes sense since this shouldn't be necessary with DT due
> __irq_set_trigger() being called from irq_create_of_mapping() before when the
> "interrupts" property is parsed.
> 
> But for some reason it is necessary for me... I checked that struct irq_data
> state_use_accessors value is correct and even tried setting that value to
> new->flags after the mentioned code block but it makes no difference. Input
> events are not reported by evtest and AFAICT interrupts are not being generated.
> 
> It works though if the trigger type is passed to request_threaded_irq() like
> $subject does or if new->flags are set before the new->flags & IRQF_TRIGGER_MASK
> conditional.
> 
> For example, with the following changes interrupts are fired correctly:
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 3dc6a61..2d8adbb 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1169,6 +1169,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
> struct irqaction *new)
> 
>                 init_waitqueue_head(&desc->wait_for_threads);
> 
> +               if (!(new->flags & IRQF_TRIGGER_MASK))
> +                       new->flags |= irqd_get_trigger_type(desc);
> +
>                 /* Setup the type (level, edge polarity) if configured: */
>                 if (new->flags & IRQF_TRIGGER_MASK) {
>                         ret = __irq_set_trigger(desc, irq,
> 
> Any ideas what could be wrong here?
> 				
>> 	/* 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);
>>
> 
> Exactly, that's how I found the issue. When I compared both drivers I noticed
> that the Chrome OS driver did that and since all the supported platforms are DT
> based, the above is equivalent to just IRQF_TRIGGER_FALLING | IRQF_ONESHOT.
> 
> So according to my explanation, new->flags & IRQF_TRIGGER_MASK is true so
> __irq_set_trigger() is executed and that's why it works with the Chrome OS driver.
> 
> In fact the Chrome OS DTS does not set a trigger type in the "interrupts" property:
> 
> trackpad@4b {
> 	reg=<0x4b>;
> 	compatible="atmel,atmel_mxt_tp";
> 	interrupts=<1 0>;
> 	interrupt-parent=<&gpx1>;
> 	wakeup-source;
> };
> 
> 
>> which I believe should go away. If it is needed on ACPI systems we need
>> to figure out how to do things we can do with OF there.
>>
> 
> The above code should not be related to ACPI systems since whatever code that
> parses an ACPI table should just call irq_set_irq_type() like is made by OF, so
> request_threaded_irq() should just work with ACPI too.
> 
> I agree it should go away but first I want to understand why is needed in the
> first place. Unfortunately commit:
> 
> 031f136 ("Input: atmel_mxt_ts - Set default irqflags when there is no pdata")
> 
> from the Chrome OS 3.8 does not explain why this is needed, instead of adding
> this information in the DTS (e.g: interrupts=<1 IRQ_TYPE_EDGE_FALLING>).
> 
>> Thanks.
>>
> 
> Best regards,
> Javier
> 
> [0]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1172
> 

Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting

[Nick Dyer]

On 07/08/14 01:48, Javier Martinez Canillas wrote:
> The Atmel maXTouch driver assumed that the IRQ type flags will
> always be passed using platform data but this is not true when
> booting using Device Trees. In these setups the interrupt type
> was ignored by the driver when requesting an IRQ.
> 
> This means that it will fail if a machine specified other type
> than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
> that was parsed by OF from the "interrupt" Device Tree propery.

I do not believe you are correct about this. There is a bit of code here:
http://lxr.free-electrons.com/source/kernel/irq/manage.c?v=3.16#L1172
which means that in the device tree case, if we call request_threaded_irq()
with no trigger bits set, it will trust whatever it already there. I did
test this back in July and it appeared to work correctly.

Have you tested this change is actually necessary?