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

[PATCH v2 1/1] 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>
---

This patch was first sent as a part of a two part series along
with [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example.

But there are no dependencies between these two patches so there
is no need to resend that one with no changes for v2.

Changes since v1:
 - Assign flags to pdata->irqflags in mxt_parse_dt() instead of probe().
   Suggested by Tomasz Figa.

 drivers/input/touchscreen/atmel_mxt_ts.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 03b8571..5c8cbd3 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>
@@ -2093,6 +2094,8 @@ static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
+	pdata->irqflags = irq_get_trigger_type(client->irq);
+
 	if (of_find_property(client->dev.of_node, "linux,gpio-keymap",
 			     &proplen)) {
 		pdata->t19_num_keys = proplen / sizeof(u32);
-- 
2.0.0.rc2

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

[Nick Dyer]

On 07/08/14 08:44, 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.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

I'm happy for this to go in if Dmitry will accept it. It does seem to be a
quirk of some platforms that it is necessary, but it's only one line.

Thanks for spending so much time debugging this.

Signed-off-by: Nick Dyer <nick.dyer-8YpZQSjhsQH10XsdtD+oqA@public.gmane.org>

> ---
> 
> This patch was first sent as a part of a two part series along
> with [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example.
> 
> But there are no dependencies between these two patches so there
> is no need to resend that one with no changes for v2.
> 
> Changes since v1:
>  - Assign flags to pdata->irqflags in mxt_parse_dt() instead of probe().
>    Suggested by Tomasz Figa.
> 
>  drivers/input/touchscreen/atmel_mxt_ts.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 03b8571..5c8cbd3 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>
> @@ -2093,6 +2094,8 @@ static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client)
>  	if (!pdata)
>  		return ERR_PTR(-ENOMEM);
>  
> +	pdata->irqflags = irq_get_trigger_type(client->irq);
> +
>  	if (of_find_property(client->dev.of_node, "linux,gpio-keymap",
>  			     &proplen)) {
>  		pdata->t19_num_keys = proplen / sizeof(u32);
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dmitry Torokhov]

On Fri, Aug 08, 2014 at 03:07:33PM +0100, Nick Dyer wrote:
> On 07/08/14 08:44, 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.
> > 
> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> 
> I'm happy for this to go in if Dmitry will accept it. It does seem to be a
> quirk of some platforms that it is necessary, but it's only one line.

I'd rather not as it masks the deeper platform issue. There might be
other drovers also expecting platform/OF code set up interrupt triggers
and working/not working by chance.

Can we figure out why the platform in question needs this change?

Thanks.

-- 
Dmitry

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

[Javier Martinez Canillas]

+Thomas Gleixner, Jason Cooper, Benjamin Herrenschmidt and Thomas Abraham

Hello Dmitry,

On 08/08/2014 06:21 PM, Dmitry Torokhov wrote:
> On Fri, Aug 08, 2014 at 03:07:33PM +0100, Nick Dyer wrote:
>> On 07/08/14 08:44, 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.
>> > 
>> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> 
>> I'm happy for this to go in if Dmitry will accept it. It does seem to be a
>> quirk of some platforms that it is necessary, but it's only one line.
> 
> I'd rather not as it masks the deeper platform issue. There might be
> other drovers also expecting platform/OF code set up interrupt triggers
> and working/not working by chance.
> 

I totally agree. When posted the patch I thought that it was the right fix but
after your explanation and studying the IRQ core I see that as you said this i
just hiding a more fundamental issue. We should fix the root cause instead of
adding a workaround for every driver.

> Can we figure out why the platform in question needs this change?
> 

I dig further on this. First I wanted to see if the problem was on IRQ core or
in the irqchip driver so I tried calling the chip's .irq_set_type function
handler directly using the flags set by __irq_set_trigger() the first time that
it's called from OF when the "interrupts" property is parsed.

Doing that makes the device to trigger interrupts so the problem seems to be
related to the pinctrl-exynos driver and is not in the IRQ core.

So, this is the change I did just for testing purposes to make it work again:

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

 			if (ret)
 				goto out_mask;
+		} else if (irq == 283 /* mapped IRQ number for touchpad */) {
+			struct irq_chip *chip = desc->irq_data.chip;
+			chip->irq_set_type(&desc->irq_data,
+					   irq_get_trigger_type(irq));
 		}

 		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \


It seems as if the first call to exynos_irq_set_type() that is made by OF is a
no-op while the second call is the one that actually setups the hw correctly.
Does this make any sense? Maybe is related to the pin not being muxed in the
correct function when the "interrupts" property is parsed by OF?

Now I added some debug logs to see what could be different but all the variables
have the same values in both cases:

When called happens due irq_parse_and_map():

irq 283 type 2 pin 1 shift 4 base 4026679296 reg_con 3588 con 32 flags 0 mask 0

When called happens due request_threaded_irq():

irq 283 type 2 pin 1 shift 4 base 4026679296 reg_con 3588 con 32 flags 0 mask 0

I would really appreciate if someone that is more familiar with the driver or
this chip can provide some hints.

Thanks a lot and best regards,
Javier

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

[Javier Martinez Canillas]

Hello,

On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
> 
> 
> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
> no-op while the second call is the one that actually setups the hw correctly.
> Does this make any sense? Maybe is related to the pin not being muxed in the
> correct function when the "interrupts" property is parsed by OF?
> 

So after a conversation with Tomasz Figa over IRC the problem was after all that
the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
and not a direct cause.

The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
function handler. So what happened was that OF parsed the "interrupts" property
and called exynos_irq_set_type() which did the pinmux setup.

But after that, due a DTS pinctrl configuration the pin function was changed as
a GPIO input and that happened before the atmel driver was probed. So when the
driver called request_threaded_irq(), the correct flags were used but the pin
was not configured as an IRQ anymore so IRQ were not fired.

Setting a trigger type just had the side effect of calling exynos_irq_set_type()
which again setup the pin as an IRQ.

To fix the issue a variation of patch [0] will be posted that moves the IRQ
pinmux setup from .irq_set_type to the .irq_request_resources function handler.
That way the pin will be setup as IRQ regardless of the the trigger type [1]
when someone calls request_[threaded]_irq().

Only the mentioned patch fixes the issue but Tomasz said that even a call to
gpio_direction_{input,output} can change the pin configuration so he will post
another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
reconfiguration.

Thanks a lot and best regards,
Javier

[0]: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/34259/focus=34261
[1]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1162

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

[Tomasz Figa]

+CC Linus, as this became also pinctrl related.

On 08.08.2014 20:29, Javier Martinez Canillas wrote:
> Hello,
> 
> On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>>
>>
>> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
>> no-op while the second call is the one that actually setups the hw correctly.
>> Does this make any sense? Maybe is related to the pin not being muxed in the
>> correct function when the "interrupts" property is parsed by OF?
>>
> 
> So after a conversation with Tomasz Figa over IRC the problem was after all that
> the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
> and not a direct cause.
> 
> The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
> function handler. So what happened was that OF parsed the "interrupts" property
> and called exynos_irq_set_type() which did the pinmux setup.
> 
> But after that, due a DTS pinctrl configuration the pin function was changed as
> a GPIO input and that happened before the atmel driver was probed. So when the
> driver called request_threaded_irq(), the correct flags were used but the pin
> was not configured as an IRQ anymore so IRQ were not fired.
> 
> Setting a trigger type just had the side effect of calling exynos_irq_set_type()
> which again setup the pin as an IRQ.
> 
> To fix the issue a variation of patch [0] will be posted that moves the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources function handler.
> That way the pin will be setup as IRQ regardless of the the trigger type [1]
> when someone calls request_[threaded]_irq().
> 
> Only the mentioned patch fixes the issue but Tomasz said that even a call to
> gpio_direction_{input,output} can change the pin configuration so he will post
> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
> reconfiguration.

To add a bit more information about the hardware, setting up a GPIO
interrupt on Samsung SoCs is a two-step operation - in addition to
trigger configuration in a dedicated register, the pinmux must be also
reconfigured to GPIO interrupt, which is a different function than
normal GPIO input, although I/O-wise they both behave in the same way
and gpio_get_value() can be used on a pin configured as IRQ as well.

I'm afraid that such design implies that handling of this in the driver
must be done on a very low level, because it involves three possible
interfaces changing the pinmux - pinctrl, GPIO and irqchip and such
subtletes like gpio_direction_input() that shouldn't neither fail if a
pin is already configured as an interrupt nor change the configuration
to normal input.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Doug Anderson]

Hi,

On Fri, Aug 8, 2014 at 11:29 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello,
>
> On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>>
>>
>> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
>> no-op while the second call is the one that actually setups the hw correctly.
>> Does this make any sense? Maybe is related to the pin not being muxed in the
>> correct function when the "interrupts" property is parsed by OF?
>>
>
> So after a conversation with Tomasz Figa over IRC the problem was after all that
> the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
> and not a direct cause.
>
> The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
> function handler. So what happened was that OF parsed the "interrupts" property
> and called exynos_irq_set_type() which did the pinmux setup.
>
> But after that, due a DTS pinctrl configuration the pin function was changed as
> a GPIO input and that happened before the atmel driver was probed. So when the
> driver called request_threaded_irq(), the correct flags were used but the pin
> was not configured as an IRQ anymore so IRQ were not fired.
>
> Setting a trigger type just had the side effect of calling exynos_irq_set_type()
> which again setup the pin as an IRQ.
>
> To fix the issue a variation of patch [0] will be posted that moves the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources function handler.
> That way the pin will be setup as IRQ regardless of the the trigger type [1]
> when someone calls request_[threaded]_irq().
>
> Only the mentioned patch fixes the issue but Tomasz said that even a call to
> gpio_direction_{input,output} can change the pin configuration so he will post
> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
> reconfiguration.

Would just making a device tree change fix this?  AKA for the pin, do:

  samsung,pin-function = <0xf>;

I have some vague recollection that I set interrupts to pin-function
"0" by default for some reason (assuming they would go to 0xf when
interrupts were enabled).  ...but I can't for the life of me remember
if it was a good reason or just seemed like the right thing to do.

-Doug

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

[Javier Martinez Canillas]

Hello Doug,

On 08/08/2014 10:54 PM, Doug Anderson wrote:
> Hi,
>>
>> To fix the issue a variation of patch [0] will be posted that moves the IRQ
>> pinmux setup from .irq_set_type to the .irq_request_resources function handler.
>> That way the pin will be setup as IRQ regardless of the the trigger type [1]
>> when someone calls request_[threaded]_irq().
>>
>> Only the mentioned patch fixes the issue but Tomasz said that even a call to
>> gpio_direction_{input,output} can change the pin configuration so he will post
>> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
>> reconfiguration.
> 
> Would just making a device tree change fix this?  AKA for the pin, do:
> 
>   samsung,pin-function = <0xf>;
> 

Yes, when configuring the pin as 0xf (GPIO interrupt) instead of 0x0 (GPIO
input) the issue is not present indeed. So after Nick answer my question about
what I got wrong with the "linux,gpio-keymap" property, I will change the pin
function when re-posing the DTS changes for the atmel touchpad.

> I have some vague recollection that I set interrupts to pin-function
> "0" by default for some reason (assuming they would go to 0xf when
> interrupts were enabled).  ...but I can't for the life of me remember
> if it was a good reason or just seemed like the right thing to do.
> 

It would be great to know if there is a good reason. I see indeed that all
pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are
using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a
difference between GPIO-IRQ and GPIO input I guess that it's better to change
those to 0xf instead. What do you think?

Regardless of this though I think that both the patch to move the IRQ
pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
to prevent any pinmux reconfiguration are good improvements to avoid future
issues like the one we found here.

> -Doug
> 

Best regards,
Javier

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

[Doug Anderson]

Javier,

On Fri, Aug 8, 2014 at 3:26 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>> I have some vague recollection that I set interrupts to pin-function
>> "0" by default for some reason (assuming they would go to 0xf when
>> interrupts were enabled).  ...but I can't for the life of me remember
>> if it was a good reason or just seemed like the right thing to do.
>>
>
> It would be great to know if there is a good reason. I see indeed that all
> pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are
> using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a
> difference between GPIO-IRQ and GPIO input I guess that it's better to change
> those to 0xf instead. What do you think?

I think it's worth trying out.  If there are no problems with it then
let's do it.

My vague recollection is that I was worried that pinctrl would take
effect right at driver probe time (maybe this used to happen? or maybe
I imagined it?) and that configuring to 0xf at this point in time
would cause a spurious interrupt.  I can't remember ever testing it so
it was probably just something I imagined.  Even if it was configured
as 0xf I'd imagine that the interrupt would be masked anyway so there
should be no spurious interrupt, right?


> Regardless of this though I think that both the patch to move the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
> to prevent any pinmux reconfiguration are good improvements to avoid future
> issues like the one we found here.

OK.  I'll let you, Tomasz, and Linus figure out what's best here since
I haven't done extensive thinking on it.  ;)

-Doug

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

[Linus Walleij]

I guess this discussion is about drivers/pinctrl/samsung/pinctrl-exynos.c?

Or else I'm not really following this... $SUBJECT is a bit confusing.

On Sat, Aug 9, 2014 at 12:26 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:

> Regardless of this though I think that both the patch to move the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
> to prevent any pinmux reconfiguration are good improvements to avoid future
> issues like the one we found here.

I think someone should look into switching the Samsung/Exynos pinctrl
driver to the gpiolib irqchip helpers, I looked at it but was scared by
the special wkup chip and stuff I can't test.

The irqchip helpers will atleast help out in flagging GPIO lines as
used for IRQs so the core can keep track of stuff and show that
properly in debugfs.

The orthogonality compliance between GPIO and irqchip must
however be solved in the driver itself, the core only helps out
in blocking some abuse of the API.

Yours,
Linus Walleij

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

[Javier Martinez Canillas]

Hello Linus,

On 08/11/2014 10:32 AM, Linus Walleij wrote:
> I guess this discussion is about drivers/pinctrl/samsung/pinctrl-exynos.c?
> 
> Or else I'm not really following this... $SUBJECT is a bit confusing.
> 

Yes, the thing is that at the beginning I (wrongly) thought that the IRQ trigger
type flags defined in the DTS were discarded if someone called
request_threaded_irq() with a flags != 0. And since the atmel touchpad driver
was pass pdata->irqflags | IRQF_ONESHOT I posted this patch to get the trigger
type parsed from DT and pass it to request_threaded_irq().

But after digging further I found that the issue was in the pinctrl-exynos
driver and Tomasz explained to me that the real cause was that the pin was being
configured from GPIO IRQ to GPIO input (which on other platforms is just the
same mode but on Exynos are two different modes).

At the end passing the trigger type to request_threaded_irq() just had the side
effect to call the exynos .irq_set_type function handler that reconfigured the
pin as GPIO-IRQ and that is why it made it to work. So I posted a patch from
Tomasz that fixes the real cause [0].

I'm so sorry for the confusion.

> Yours,
> Linus Walleij
> 

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/8/8/962