+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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
