atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Sekhar Nori]

Nick,

I have been using your for-next branch to base my development of 
touchscreen support on TI's DRA7x EVM. With the recent updates,
it has worked out great and once I got the configuration right,
it was just a question of adding DT data for the platform.

Now, there is one problem with Stephen's patch defaulting the irqflags 
to IRQF_TRIGGER_FALLING. The interrupt controller I am using (ARM GIC) 
does not seem to support that and the device fails to probe:

[    1.932798] genirq: Setting trigger mode 2 for irq 151 failed (gic_set_type+0x0/0xf0)
[    1.941340] atmel_mxt_ts 0-004a: Failed to register interrupt
[    1.947452] atmel_mxt_ts: probe of 0-004a failed with error -22

Attached patch does the trick for me:

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 71154c2..f2d3f72 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3155,9 +3155,6 @@ static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client)
 	pdata->gpio_reset = of_get_named_gpio_flags(dev->of_node,
 		"atmel,reset-gpio", 0, NULL);
 
-	/* Default to this. Properties can be added to configure it later */
-	pdata->irqflags = IRQF_TRIGGER_FALLING;
-
 	of_property_read_string(dev->of_node, "atmel,cfg_name",
 				&pdata->cfg_name); 

Can you switch to leaving the platform to specify flags (at least for the DT case)?

Thanks,
Sekhar

RE: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Stephen Warren]

Sekhar Nori wrote at Tuesday, July 01, 2014 3:52 AM:
> Nick,
> 
> I have been using your for-next branch to base my development of
> touchscreen support on TI's DRA7x EVM. With the recent updates,
> it has worked out great and once I got the configuration right,
> it was just a question of adding DT data for the platform.
> 
> Now, there is one problem with Stephen's patch defaulting the irqflags
> to IRQF_TRIGGER_FALLING. The interrupt controller I am using (ARM GIC)
> does not seem to support that and the device fails to probe:

On the Tegra systems I have, IRQF_TRIGGER_FALLING is the correct (or at
least a valid) choice. That's probably because the Atmel IRQ signal is
routed to our GPIO controller, which is also an IRQ controller, and then
"forwarded" up the chain to the GIC, with the polarity the GIC expects.

If IRQ_TRIGGER_FALLING doesn't work everywhere, then we'll need to add
some kind of DT property to configure the polarity of the IRQ output.

> [    1.932798] genirq: Setting trigger mode 2 for irq 151 failed (gic_set_type+0x0/0xf0)
> [    1.941340] atmel_mxt_ts 0-004a: Failed to register interrupt
> [    1.947452] atmel_mxt_ts: probe of 0-004a failed with error -22
> 
> Attached patch does the trick for me:
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 71154c2..f2d3f72 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3155,9 +3155,6 @@ static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client)
>  	pdata->gpio_reset = of_get_named_gpio_flags(dev->of_node,
>  		"atmel,reset-gpio", 0, NULL);
> 
> -	/* Default to this. Properties can be added to configure it later */
> -	pdata->irqflags = IRQF_TRIGGER_FALLING;
> -
>  	of_property_read_string(dev->of_node, "atmel,cfg_name",
>  				&pdata->cfg_name);
> 
> Can you switch to leaving the platform to specify flags (at least for the DT case)?
> 
> Thanks,
> Sekhar

-- 
nvpublic

Re: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Sekhar Nori]

On Tuesday 01 July 2014 09:44 PM, Stephen Warren wrote:
> Sekhar Nori wrote at Tuesday, July 01, 2014 3:52 AM:
>> Nick,
>>
>> I have been using your for-next branch to base my development of
>> touchscreen support on TI's DRA7x EVM. With the recent updates,
>> it has worked out great and once I got the configuration right,
>> it was just a question of adding DT data for the platform.
>>
>> Now, there is one problem with Stephen's patch defaulting the irqflags
>> to IRQF_TRIGGER_FALLING. The interrupt controller I am using (ARM GIC)
>> does not seem to support that and the device fails to probe:
> 
> On the Tegra systems I have, IRQF_TRIGGER_FALLING is the correct (or at
> least a valid) choice. That's probably because the Atmel IRQ signal is
> routed to our GPIO controller, which is also an IRQ controller, and then
> "forwarded" up the chain to the GIC, with the polarity the GIC expects.
> 
> If IRQ_TRIGGER_FALLING doesn't work everywhere, then we'll need to add
> some kind of DT property to configure the polarity of the IRQ output.

Yeah, I think so too.

Nick,

If you are going to rebase your branch, will you be able to fold in the
patch in my previous e-mail? Else, I can send a more formal patch to you.

Thanks,
Sekhar

Re: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Nick Dyer]

On 02/07/14 11:49, Sekhar Nori wrote:
> On Tuesday 01 July 2014 09:44 PM, Stephen Warren wrote:
>> On the Tegra systems I have, IRQF_TRIGGER_FALLING is the correct (or at
>> least a valid) choice. That's probably because the Atmel IRQ signal is
>> routed to our GPIO controller, which is also an IRQ controller, and then
>> "forwarded" up the chain to the GIC, with the polarity the GIC expects.
>>
>> If IRQ_TRIGGER_FALLING doesn't work everywhere, then we'll need to add
>> some kind of DT property to configure the polarity of the IRQ output.
> 
> Yeah, I think so too.
> 
> Nick,
> 
> If you are going to rebase your branch, will you be able to fold in the
> patch in my previous e-mail? Else, I can send a more formal patch to you.

Either IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW will work with these chips
(it isn't a question of polarity but whether it's edge- or level-
triggered). There isn't a sensible default. Atmel prefer IRQF_TRIGGER_LOW,
however I've seen some IRQ controllers will revert to a polled mode for
IRQF_TRIGGER_LOW, which kills performance.

So, the sensible course of action seems to be to remove the default
IRQF_TRIGGER_FALLING in the device tree parsing, and provide a device tree
parameter for the flags. If you agree, I will sort this out at my end, you
don't need to send a patch.

I have to leave it in in the case where there is neither static platform
data, or device tree node, because that is used for some systems, but that
shouldn't affect either of you.

BTW, I do have a set of patches ready to send, once this change is made.

cheers

Nick

Re: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Dmitry Torokhov]

On Wed, Jul 2, 2014 at 4:54 AM, Nick Dyer <nick.dyer@itdev.co.uk> wrote:
> On 02/07/14 11:49, Sekhar Nori wrote:
>> On Tuesday 01 July 2014 09:44 PM, Stephen Warren wrote:
>>> On the Tegra systems I have, IRQF_TRIGGER_FALLING is the correct (or at
>>> least a valid) choice. That's probably because the Atmel IRQ signal is
>>> routed to our GPIO controller, which is also an IRQ controller, and then
>>> "forwarded" up the chain to the GIC, with the polarity the GIC expects.
>>>
>>> If IRQ_TRIGGER_FALLING doesn't work everywhere, then we'll need to add
>>> some kind of DT property to configure the polarity of the IRQ output.
>>
>> Yeah, I think so too.
>>
>> Nick,
>>
>> If you are going to rebase your branch, will you be able to fold in the
>> patch in my previous e-mail? Else, I can send a more formal patch to you.
>
> Either IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW will work with these chips
> (it isn't a question of polarity but whether it's edge- or level-
> triggered). There isn't a sensible default. Atmel prefer IRQF_TRIGGER_LOW,
> however I've seen some IRQ controllers will revert to a polled mode for
> IRQF_TRIGGER_LOW, which kills performance.
>
> So, the sensible course of action seems to be to remove the default
> IRQF_TRIGGER_FALLING in the device tree parsing, and provide a device tree
> parameter for the flags. If you agree, I will sort this out at my end, you
> don't need to send a patch.
>
> I have to leave it in in the case where there is neither static platform
> data, or device tree node, because that is used for some systems, but that
> shouldn't affect either of you.

In this case board code should take care of setting up the interrupt
properly and the driver should simply use 0 as flags in request_irq().
By the way, doesn't generic DT infrastructure already allow specifying
interrupt triggers and sets them up properly?

Thanks.

-- 
Dmitry

Re: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Nick Dyer]

On 02/07/14 18:25, Dmitry Torokhov wrote:
> In this case board code should take care of setting up the interrupt
> properly and the driver should simply use 0 as flags in request_irq().
> By the way, doesn't generic DT infrastructure already allow specifying
> interrupt triggers and sets them up properly?

Yes. I've checked and tested this behaviour today. If you don't specify a
trigger type, then when you request the irq it trusts that the platform has
already set it up correctly:
http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1208

Stephen: you should be able to set up the interrupt config in the device
tree. I believe this should be done by putting IRQ_TYPE_LEVEL_LOW or
IRQ_TYPE_EDGE_FALLING in the third field in the "interrupts" definition,
although I haven't a tegra DTS device to test on at the moment.

I've applied the patch below, which is basically what Sekhar had
originally, with a minor extra fix.

I've merged it into the appropriate parts of my for upstream series at
https://github.com/ndyer/linux/commits/for-next

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
b/drivers/input/touchscree
index dc46100..15efb3b 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1453,7 +1453,7 @@ static int mxt_check_retrigen(struct mxt_data *data)
        int error;
        int val;

-       if (data->pdata->irqflags & IRQF_TRIGGER_LOW)
+       if (irq_get_trigger_type(data->irq) & IRQF_TRIGGER_LOW)
                return 0;

        if (data->T18_address) {
@@ -3155,9 +3155,6 @@ static struct mxt_platform_data *mxt_parse_dt(struct
i2c_c
        pdata->gpio_reset = of_get_named_gpio_flags(dev->of_node,
                "atmel,reset-gpio", 0, NULL);

-       /* Default to this. Properties can be added to configure it later */
-       pdata->irqflags = IRQF_TRIGGER_FALLING;
-
        of_property_read_string(dev->of_node, "atmel,cfg_name",
                                &pdata->cfg_name);

Re: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING

[Sekhar Nori]

On Wednesday 02 July 2014 05:24 PM, Nick Dyer wrote:
> On 02/07/14 11:49, Sekhar Nori wrote:
>> On Tuesday 01 July 2014 09:44 PM, Stephen Warren wrote:
>>> On the Tegra systems I have, IRQF_TRIGGER_FALLING is the correct (or at
>>> least a valid) choice. That's probably because the Atmel IRQ signal is
>>> routed to our GPIO controller, which is also an IRQ controller, and then
>>> "forwarded" up the chain to the GIC, with the polarity the GIC expects.
>>>
>>> If IRQ_TRIGGER_FALLING doesn't work everywhere, then we'll need to add
>>> some kind of DT property to configure the polarity of the IRQ output.
>>
>> Yeah, I think so too.
>>
>> Nick,
>>
>> If you are going to rebase your branch, will you be able to fold in the
>> patch in my previous e-mail? Else, I can send a more formal patch to you.
> 
> Either IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW will work with these chips
> (it isn't a question of polarity but whether it's edge- or level-
> triggered). There isn't a sensible default. Atmel prefer IRQF_TRIGGER_LOW,
> however I've seen some IRQ controllers will revert to a polled mode for
> IRQF_TRIGGER_LOW, which kills performance.
> 
> So, the sensible course of action seems to be to remove the default
> IRQF_TRIGGER_FALLING in the device tree parsing, and provide a device tree
> parameter for the flags. If you agree, I will sort this out at my end, you
> don't need to send a patch.

Sounds good.

> I have to leave it in in the case where there is neither static platform
> data, or device tree node, because that is used for some systems, but that
> shouldn't affect either of you.
> 
> BTW, I do have a set of patches ready to send, once this change is made.

It will be great if you could CC me on that posting so I could test and
ack.

Regards,
Sekhar