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