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@xxxxxxxxxxxxxxx>
>> 
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/