On 2016-11-08 16:59, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> +/*
>> + * The envelope_detector_comp_latch function works together with the compare
>> + * interrupt service routine below (envelope_detector_comp_isr) as a latch
>> + * (one-bit memory) for if the interrupt has triggered since last calling
>> + * this function.
>> + * The ..._comp_isr function disables the interrupt so that the cpu does not
>> + * need to service a possible interrupt flood from the comparator when no-one
>> + * cares anyway, and this ..._comp_latch function reenables them again if
>> + * needed.
>> + */
>> +static int envelope_detector_comp_latch(struct envelope *env)
>> +{
>> + int comp;
>> +
>> + spin_lock_irq(&env->comp_lock);
>> + comp = env->comp;
>> + env->comp = 0;
>> + spin_unlock_irq(&env->comp_lock);
>> +
>> + if (!comp)
>> + return 0;
>> +
>> + /*
>> + * The irq was disabled, and is reenabled just now.
>> + * But there might have been a pending irq that
>> + * happened while the irq was disabled that fires
>> + * just as the irq is reenabled. That is not what
>> + * is desired.
>> + */
>> + enable_irq(env->comp_irq);
>> +
>> + /* So, synchronize this possibly pending irq... */
>> + synchronize_irq(env->comp_irq);
>> +
>> + /* ...and redo the whole dance. */
>> + spin_lock_irq(&env->comp_lock);
>> + comp = env->comp;
>> + env->comp = 0;
>> + spin_unlock_irq(&env->comp_lock);
>> +
>> + if (comp)
>> + enable_irq(env->comp_irq);
>
> So you need that whole dance including the delayed work because you cannot
> call iio_write_channel_raw() from hard interrupt context, right?
It's not the "cannot call from hard irq context" that made me do that, it's...
> So you might just register a threaded interrupt handler, which should make
> this whole thing way simpler.
>
> devm_request_threaded_irq(dev, irq, NULL, your_isr, IRQF_ONESHOT, ...);
>
> The core will mask the interrupt line until the threaded handler is
> finished. The threaded handler is invoked with preemption enabled, so you
> can sleep there as long as you want. So you can do everything in your
> handler and the above dance is just not required.
...that I couldn't work out how to reenable a oneshot irq once it had fired,
short of freeing the irq and requesting it again. That seemed entirely
bogus, the driver shouldn't risk losing a resource like that so I don't know
what I didn't see? Or maybe it was that I had a hard time resolving the race
between the irq and the timeout in a nice way. I honestly don't remember
why exactly I abandoned oneshot irqs, but this enable/sync/enable dance
was much nicer than what I came up with for the oneshot irq solution I
originally worked on.
Or maybe I had problems with the possibly pending irq also when using a
oneshot irq, but didn't realize it? That was something I discovered quite
late in the process, some time after moving away from oneshot irqs. Are
pending irqs cleared when requesting (or reenabling, however that is done)
a oneshot irq?
Anyway, I do not want the interrupt to be serviced when no one is interested,
since I'm afraid that nasty input might generate a flood of interrupts that
might disturb other things that the cpu is doing. Which means that I need
to enable/disable the interrupt as needed.
However, what *I* thought Jonathan wanted input on was the part where the
interrupt edge/level is flipped when requesting "inverted" signals in
envelope_store_invert(). That could perhaps be seen as unorthodox and in
need of more eyes?
Cheers,
Peter
--
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
