Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-12 Thread Jonathan Cameron 
On 11/11/16 11:37, Peter Rosin wrote:
> On 2016-11-09 16:06, Thomas Gleixner wrote:
>> On Wed, 9 Nov 2016, Peter Rosin wrote:
>>> On 2016-11-08 22:47, Thomas Gleixner wrote:
 I don't think you need extra race handling with that, but I might be wrong
 as usual.
>>>
>>> There's obviously no way to determine which of the timeout or the
>>> interrupt that happens first without some race handling, so I don't
>>> know what you mean? If the timeout happens first, there is also a
>>> need to handle late hits from the irq that might come in during the
>>> preparation for the next step in the binary search. It gets messy
>>> quickly compared to the simplicity of the current implementation.
>>
>> Gah, forgot about that timeout thingy. Fair enough.
>>
>> Feel free to add an 
>>
>> Acked-by: Thomas Gleixner 
> 
> Thanks for looking!
Applied to the togreg branch of iio.git and pushed out
as testing for the autobuilders to play with.

Excellent patch set.

Thanks,

Jonathan
> 
> Cheers,
> Peter
>

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-12 Thread Jonathan Cameron 
On 11/11/16 11:37, Peter Rosin wrote:
> On 2016-11-09 16:06, Thomas Gleixner wrote:
>> On Wed, 9 Nov 2016, Peter Rosin wrote:
>>> On 2016-11-08 22:47, Thomas Gleixner wrote:
 I don't think you need extra race handling with that, but I might be wrong
 as usual.
>>>
>>> There's obviously no way to determine which of the timeout or the
>>> interrupt that happens first without some race handling, so I don't
>>> know what you mean? If the timeout happens first, there is also a
>>> need to handle late hits from the irq that might come in during the
>>> preparation for the next step in the binary search. It gets messy
>>> quickly compared to the simplicity of the current implementation.
>>
>> Gah, forgot about that timeout thingy. Fair enough.
>>
>> Feel free to add an 
>>
>> Acked-by: Thomas Gleixner 
> 
> Thanks for looking!
Applied to the togreg branch of iio.git and pushed out
as testing for the autobuilders to play with.

Excellent patch set.

Thanks,

Jonathan
> 
> Cheers,
> Peter
>

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-12 Thread Jonathan Cameron 
On 08/11/16 17:03, Peter Rosin wrote:
> 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(>comp_lock);
>>> +   comp = env->comp;
>>> +   env->comp = 0;
>>> +   spin_unlock_irq(>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(>comp_lock);
>>> +   comp = env->comp;
>>> +   env->comp = 0;
>>> +   spin_unlock_irq(>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?
Nope, as far as I can recall it was precisely this dance that was 
I wanted Thomas to comment on :)  The inverted bit isn't as novel as
this ;)

Anyhow, thread ended up with a good conclusion so I'm happy.

Jonathan
> 
> Cheers,
> Peter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-12 Thread Jonathan Cameron 
On 08/11/16 17:03, Peter Rosin wrote:
> 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(>comp_lock);
>>> +   comp = env->comp;
>>> +   env->comp = 0;
>>> +   spin_unlock_irq(>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(>comp_lock);
>>> +   comp = env->comp;
>>> +   env->comp = 0;
>>> +   spin_unlock_irq(>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?
Nope, as far as I can recall it was precisely this dance that was 
I wanted Thomas to comment on :)  The inverted bit isn't as novel as
this ;)

Anyhow, thread ended up with a good conclusion so I'm happy.

Jonathan
> 
> Cheers,
> Peter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-11 Thread Peter Rosin 
On 2016-11-09 16:06, Thomas Gleixner wrote:
> On Wed, 9 Nov 2016, Peter Rosin wrote:
>> On 2016-11-08 22:47, Thomas Gleixner wrote:
>>> I don't think you need extra race handling with that, but I might be wrong
>>> as usual.
>>
>> There's obviously no way to determine which of the timeout or the
>> interrupt that happens first without some race handling, so I don't
>> know what you mean? If the timeout happens first, there is also a
>> need to handle late hits from the irq that might come in during the
>> preparation for the next step in the binary search. It gets messy
>> quickly compared to the simplicity of the current implementation.
> 
> Gah, forgot about that timeout thingy. Fair enough.
> 
> Feel free to add an 
> 
> Acked-by: Thomas Gleixner 

Thanks for looking!

Cheers,
Peter

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-11 Thread Peter Rosin 
On 2016-11-09 16:06, Thomas Gleixner wrote:
> On Wed, 9 Nov 2016, Peter Rosin wrote:
>> On 2016-11-08 22:47, Thomas Gleixner wrote:
>>> I don't think you need extra race handling with that, but I might be wrong
>>> as usual.
>>
>> There's obviously no way to determine which of the timeout or the
>> interrupt that happens first without some race handling, so I don't
>> know what you mean? If the timeout happens first, there is also a
>> need to handle late hits from the irq that might come in during the
>> preparation for the next step in the binary search. It gets messy
>> quickly compared to the simplicity of the current implementation.
> 
> Gah, forgot about that timeout thingy. Fair enough.
> 
> Feel free to add an 
> 
> Acked-by: Thomas Gleixner 

Thanks for looking!

Cheers,
Peter

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-09 Thread Thomas Gleixner 
On Wed, 9 Nov 2016, Peter Rosin wrote:
> On 2016-11-08 22:47, Thomas Gleixner wrote:
> > I don't think you need extra race handling with that, but I might be wrong
> > as usual.
> 
> There's obviously no way to determine which of the timeout or the
> interrupt that happens first without some race handling, so I don't
> know what you mean? If the timeout happens first, there is also a
> need to handle late hits from the irq that might come in during the
> preparation for the next step in the binary search. It gets messy
> quickly compared to the simplicity of the current implementation.

Gah, forgot about that timeout thingy. Fair enough.

Feel free to add an 

Acked-by: Thomas Gleixner 

Thanks,

tglx

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-09 Thread Thomas Gleixner 
On Wed, 9 Nov 2016, Peter Rosin wrote:
> On 2016-11-08 22:47, Thomas Gleixner wrote:
> > I don't think you need extra race handling with that, but I might be wrong
> > as usual.
> 
> There's obviously no way to determine which of the timeout or the
> interrupt that happens first without some race handling, so I don't
> know what you mean? If the timeout happens first, there is also a
> need to handle late hits from the irq that might come in during the
> preparation for the next step in the binary search. It gets messy
> quickly compared to the simplicity of the current implementation.

Gah, forgot about that timeout thingy. Fair enough.

Feel free to add an 

Acked-by: Thomas Gleixner 

Thanks,

tglx

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-09 Thread Peter Rosin 
On 2016-11-08 22:47, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> So, to sum up, in order for this to work with threaded oneshot
>> interrupts, I still need to either keep the enable/sync/enable-dance
>> or tweak the irq core to handle my case better. The only gain would
>> be that I could fire the next step of the search from the threaded
>> irq handler directly (but it needs some new race-killing code).
>> Or am I missing something? If not, there's no pressing reason to
>> switch to threaded oneshot interrupts, right?
> 
> There is no pressing reason, but that misfire prevention dance looks
> fragile and overly complex to me.

I don't see any fragility? But I wouldn't complain if there was some
simpler way to enable an irq in such a way that only new and fresh
irqs are considered.

> The completely untested patch below should block the replay for edge

[it is missing a ')', see inline comment below]

> interrupts from the core code. It also makes sure that the edge interrupt
> is masked until the thread handler returns. All you have to do is
> requesting your threaded handler with IRQF_ONESHOT | IRQF_NO_REPLAY.

It doesn't appear to work. I still get irqs that reasonably should
only have happened *during* the handling of a previous irq. And I
still need to dance when enabling the irq, otherwise I get spurious
hits.

> I don't think you need extra race handling with that, but I might be wrong
> as usual.

There's obviously no way to determine which of the timeout or the
interrupt that happens first without some race handling, so I don't
know what you mean? If the timeout happens first, there is also a
need to handle late hits from the irq that might come in during the
preparation for the next step in the binary search. It gets messy
quickly compared to the simplicity of the current implementation.

Cheers,
Peter

> 8<--
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -74,6 +74,7 @@
>  #define IRQF_NO_THREAD   0x0001
>  #define IRQF_EARLY_RESUME0x0002
>  #define IRQF_COND_SUSPEND0x0004
> +#define IRQF_NO_REPLAY   0x0008
>  
>  #define IRQF_TIMER   (__IRQF_TIMER | IRQF_NO_SUSPEND | 
> IRQF_NO_THREAD)
>  
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -57,6 +57,7 @@ enum {
>   IRQS_WAITING= 0x0080,
>   IRQS_PENDING= 0x0200,
>   IRQS_SUSPENDED  = 0x0800,
> + IRQS_NO_REPLAY  = 0x1000,
>  };
>  
>  #include "debug.h"
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1212,7 +1212,8 @@ static int
>*/
>   if (!((old->flags & new->flags) & IRQF_SHARED) ||
>   ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
> - ((old->flags ^ new->flags) & IRQF_ONESHOT))
> + ((old->flags ^ new->flags) & IRQF_ONESHOT) ||
> + ((old->flags ^ new->flags) & IRQF_NO_REPLAY))
>   goto mismatch;
>  
>   /* All handlers must agree on per-cpuness */
> @@ -1324,6 +1325,9 @@ static int
>   if (new->flags & IRQF_ONESHOT)
>   desc->istate |= IRQS_ONESHOT;
>  
> + if (new->flags & IRQF_NO_REPLAY)
> + desc->istate |= IRQS_NO_REPLAY;
> +
>   if (irq_settings_can_autoenable(desc))
>   irq_startup(desc, true);
>   else
> --- a/kernel/irq/resend.c
> +++ b/kernel/irq/resend.c
> @@ -56,12 +56,12 @@ static DECLARE_TASKLET(resend_tasklet, r
>  void check_irq_resend(struct irq_desc *desc)
>  {
>   /*
> -  * We do not resend level type interrupts. Level type
> -  * interrupts are resent by hardware when they are still
> -  * active. Clear the pending bit so suspend/resume does not
> -  * get confused.
> +  * We do not resend level type interrupts. Level type interrupts
> +  * are resent by hardware when they are still active. Also prevent
> +  * resend when the user requested so.  Clear the pending bit so
> +  * suspend/resume does not get confused.
>*/
> - if (irq_settings_is_level(desc)) {
> + if (irq_settings_is_level(desc) || (desc->istate & IRQS_NO_REPLAY)) {
>   desc->istate &= ~IRQS_PENDING;
>   return;
>   }
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -643,7 +643,10 @@ void handle_edge_irq(struct irq_desc *de
>   kstat_incr_irqs_this_cpu(desc);
>  
>   /* Start handling the irq */
> - desc->irq_data.chip->irq_ack(>irq_data);
> + if (!(desc->istate & (IRQS_NO_REPLAY | IRQS_ONESHOT))

Missing ')' at the end.

> + desc->irq_data.chip->irq_ack(>irq_data);
> + else
> + mask_ack_irq(desc);
>  
>   do {
>   if (unlikely(!desc->action)) {
> 
>

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-09 Thread Peter Rosin 
On 2016-11-08 22:47, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> So, to sum up, in order for this to work with threaded oneshot
>> interrupts, I still need to either keep the enable/sync/enable-dance
>> or tweak the irq core to handle my case better. The only gain would
>> be that I could fire the next step of the search from the threaded
>> irq handler directly (but it needs some new race-killing code).
>> Or am I missing something? If not, there's no pressing reason to
>> switch to threaded oneshot interrupts, right?
> 
> There is no pressing reason, but that misfire prevention dance looks
> fragile and overly complex to me.

I don't see any fragility? But I wouldn't complain if there was some
simpler way to enable an irq in such a way that only new and fresh
irqs are considered.

> The completely untested patch below should block the replay for edge

[it is missing a ')', see inline comment below]

> interrupts from the core code. It also makes sure that the edge interrupt
> is masked until the thread handler returns. All you have to do is
> requesting your threaded handler with IRQF_ONESHOT | IRQF_NO_REPLAY.

It doesn't appear to work. I still get irqs that reasonably should
only have happened *during* the handling of a previous irq. And I
still need to dance when enabling the irq, otherwise I get spurious
hits.

> I don't think you need extra race handling with that, but I might be wrong
> as usual.

There's obviously no way to determine which of the timeout or the
interrupt that happens first without some race handling, so I don't
know what you mean? If the timeout happens first, there is also a
need to handle late hits from the irq that might come in during the
preparation for the next step in the binary search. It gets messy
quickly compared to the simplicity of the current implementation.

Cheers,
Peter

> 8<--
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -74,6 +74,7 @@
>  #define IRQF_NO_THREAD   0x0001
>  #define IRQF_EARLY_RESUME0x0002
>  #define IRQF_COND_SUSPEND0x0004
> +#define IRQF_NO_REPLAY   0x0008
>  
>  #define IRQF_TIMER   (__IRQF_TIMER | IRQF_NO_SUSPEND | 
> IRQF_NO_THREAD)
>  
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -57,6 +57,7 @@ enum {
>   IRQS_WAITING= 0x0080,
>   IRQS_PENDING= 0x0200,
>   IRQS_SUSPENDED  = 0x0800,
> + IRQS_NO_REPLAY  = 0x1000,
>  };
>  
>  #include "debug.h"
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1212,7 +1212,8 @@ static int
>*/
>   if (!((old->flags & new->flags) & IRQF_SHARED) ||
>   ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
> - ((old->flags ^ new->flags) & IRQF_ONESHOT))
> + ((old->flags ^ new->flags) & IRQF_ONESHOT) ||
> + ((old->flags ^ new->flags) & IRQF_NO_REPLAY))
>   goto mismatch;
>  
>   /* All handlers must agree on per-cpuness */
> @@ -1324,6 +1325,9 @@ static int
>   if (new->flags & IRQF_ONESHOT)
>   desc->istate |= IRQS_ONESHOT;
>  
> + if (new->flags & IRQF_NO_REPLAY)
> + desc->istate |= IRQS_NO_REPLAY;
> +
>   if (irq_settings_can_autoenable(desc))
>   irq_startup(desc, true);
>   else
> --- a/kernel/irq/resend.c
> +++ b/kernel/irq/resend.c
> @@ -56,12 +56,12 @@ static DECLARE_TASKLET(resend_tasklet, r
>  void check_irq_resend(struct irq_desc *desc)
>  {
>   /*
> -  * We do not resend level type interrupts. Level type
> -  * interrupts are resent by hardware when they are still
> -  * active. Clear the pending bit so suspend/resume does not
> -  * get confused.
> +  * We do not resend level type interrupts. Level type interrupts
> +  * are resent by hardware when they are still active. Also prevent
> +  * resend when the user requested so.  Clear the pending bit so
> +  * suspend/resume does not get confused.
>*/
> - if (irq_settings_is_level(desc)) {
> + if (irq_settings_is_level(desc) || (desc->istate & IRQS_NO_REPLAY)) {
>   desc->istate &= ~IRQS_PENDING;
>   return;
>   }
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -643,7 +643,10 @@ void handle_edge_irq(struct irq_desc *de
>   kstat_incr_irqs_this_cpu(desc);
>  
>   /* Start handling the irq */
> - desc->irq_data.chip->irq_ack(>irq_data);
> + if (!(desc->istate & (IRQS_NO_REPLAY | IRQS_ONESHOT))

Missing ')' at the end.

> + desc->irq_data.chip->irq_ack(>irq_data);
> + else
> + mask_ack_irq(desc);
>  
>   do {
>   if (unlikely(!desc->action)) {
> 
>

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Peter Rosin 
On 2016-11-08 19:38, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> On 2016-11-08 16:59, Thomas Gleixner wrote:
> 
>>> 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..
> 
> Well, what guarantees you that the DAC is writeable from IRQ context? It
> might be hanging off an i2c/spi bus as well

Right, the DAC is actually on an i2c-bus so it's not possible to call
iio_write_channel_raw in hard irq context, I did not disagree with that.
But that was not the reason for not calling it in (hard or threaded) irq
context. The reason is that this was the simplest way to kill the race
with the timeout, and I wasn't in that much of a hurry to start the next
step (in the binary search) and was completely happy with just recording
if any interrupt did happen and then continue when the timeout eventually
happened. It's not like the delayed work connected to the timeout can be
left out anyway, since the design is to not get an interrupt before the
timeout in about half the cases. In some cases there might not be any
interrupts in eons...

>>> 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.
> 
> Threaded ONESHOT irqs work this way:
> 
>  interrupt fires
>mask interrupt
>handler thread is woken
>  
>  handler thread runs
>invokes isr
>unmask interrupt
> 
> So if you rewrite the DAC to the new value in your ISR, then you should not
> get any spurious interrupt.
> 
> Note, that this only works for level type interrupts.
> 
> We do not mask edge type interrupts as we might lose an edge, but if that
> helps the cause of your problem it's simple enough to make it conditionally
> doing so in the core.
> 
>> 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?
> 
> Pending irqs are only replayed, when you reenable an interrupt via
> enable_irq(). That can happen either by software or by hardware.

Ah, of course, the interrupt core does its best to not lose interrupts, but
in this case it is actually desired to lose the interrupts that happen
while the irq is disabled. Which means that the enable/sync/enable-dance is
needed for oneshot interrupts as well. Either that or make tweaks to the
core (yes, irqs are edge-triggered interrupts in my case).

>> 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.
> 
> So the main issue I'm seing here, is that your comparator does not have
> means to prevent it from firing interrupts.

Right, it's just a discrete op-amp. Only way to "turn it off", is to feed
it input that makes it silent. But I'd rather not have code in this driver
that knows how to do that, since then the driver mutates from something
fairly generic to something that is very specific with hairy dependencies.
It would probably also require code to handle trailing interrupts caused
by setting up the silent state. It just sounds horrible compared to
simply disabling the interrupt (and doing a dance when enabling them).

>> 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?
> 
> Flipping the dectection level of the interrupt is fine, but what's the
> guarantee that it is correct in the first place? I don't see anything which
> makes that sure at all. Aside of that this bit does not makes sense:

That "guarantee" comes from devicetree. I.e. the person writing the
dts.

>> +env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
>> +if

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Peter Rosin 
On 2016-11-08 19:38, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> On 2016-11-08 16:59, Thomas Gleixner wrote:
> 
>>> 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..
> 
> Well, what guarantees you that the DAC is writeable from IRQ context? It
> might be hanging off an i2c/spi bus as well

Right, the DAC is actually on an i2c-bus so it's not possible to call
iio_write_channel_raw in hard irq context, I did not disagree with that.
But that was not the reason for not calling it in (hard or threaded) irq
context. The reason is that this was the simplest way to kill the race
with the timeout, and I wasn't in that much of a hurry to start the next
step (in the binary search) and was completely happy with just recording
if any interrupt did happen and then continue when the timeout eventually
happened. It's not like the delayed work connected to the timeout can be
left out anyway, since the design is to not get an interrupt before the
timeout in about half the cases. In some cases there might not be any
interrupts in eons...

>>> 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.
> 
> Threaded ONESHOT irqs work this way:
> 
>  interrupt fires
>mask interrupt
>handler thread is woken
>  
>  handler thread runs
>invokes isr
>unmask interrupt
> 
> So if you rewrite the DAC to the new value in your ISR, then you should not
> get any spurious interrupt.
> 
> Note, that this only works for level type interrupts.
> 
> We do not mask edge type interrupts as we might lose an edge, but if that
> helps the cause of your problem it's simple enough to make it conditionally
> doing so in the core.
> 
>> 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?
> 
> Pending irqs are only replayed, when you reenable an interrupt via
> enable_irq(). That can happen either by software or by hardware.

Ah, of course, the interrupt core does its best to not lose interrupts, but
in this case it is actually desired to lose the interrupts that happen
while the irq is disabled. Which means that the enable/sync/enable-dance is
needed for oneshot interrupts as well. Either that or make tweaks to the
core (yes, irqs are edge-triggered interrupts in my case).

>> 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.
> 
> So the main issue I'm seing here, is that your comparator does not have
> means to prevent it from firing interrupts.

Right, it's just a discrete op-amp. Only way to "turn it off", is to feed
it input that makes it silent. But I'd rather not have code in this driver
that knows how to do that, since then the driver mutates from something
fairly generic to something that is very specific with hairy dependencies.
It would probably also require code to handle trailing interrupts caused
by setting up the silent state. It just sounds horrible compared to
simply disabling the interrupt (and doing a dance when enabling them).

>> 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?
> 
> Flipping the dectection level of the interrupt is fine, but what's the
> guarantee that it is correct in the first place? I don't see anything which
> makes that sure at all. Aside of that this bit does not makes sense:

That "guarantee" comes from devicetree. I.e. the person writing the
dts.

>> +env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
>> +if

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Thomas Gleixner 
On Tue, 8 Nov 2016, Peter Rosin wrote:
> So, to sum up, in order for this to work with threaded oneshot
> interrupts, I still need to either keep the enable/sync/enable-dance
> or tweak the irq core to handle my case better. The only gain would
> be that I could fire the next step of the search from the threaded
> irq handler directly (but it needs some new race-killing code).
> Or am I missing something? If not, there's no pressing reason to
> switch to threaded oneshot interrupts, right?

There is no pressing reason, but that misfire prevention dance looks
fragile and overly complex to me.

The completely untested patch below should block the replay for edge
interrupts from the core code. It also makes sure that the edge interrupt
is masked until the thread handler returns. All you have to do is
requesting your threaded handler with IRQF_ONESHOT | IRQF_NO_REPLAY.

I don't think you need extra race handling with that, but I might be wrong
as usual.

Thanks,

tglx

8<--
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -74,6 +74,7 @@
 #define IRQF_NO_THREAD 0x0001
 #define IRQF_EARLY_RESUME  0x0002
 #define IRQF_COND_SUSPEND  0x0004
+#define IRQF_NO_REPLAY 0x0008
 
 #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | 
IRQF_NO_THREAD)
 
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -57,6 +57,7 @@ enum {
IRQS_WAITING= 0x0080,
IRQS_PENDING= 0x0200,
IRQS_SUSPENDED  = 0x0800,
+   IRQS_NO_REPLAY  = 0x1000,
 };
 
 #include "debug.h"
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1212,7 +1212,8 @@ static int
 */
if (!((old->flags & new->flags) & IRQF_SHARED) ||
((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
-   ((old->flags ^ new->flags) & IRQF_ONESHOT))
+   ((old->flags ^ new->flags) & IRQF_ONESHOT) ||
+   ((old->flags ^ new->flags) & IRQF_NO_REPLAY))
goto mismatch;
 
/* All handlers must agree on per-cpuness */
@@ -1324,6 +1325,9 @@ static int
if (new->flags & IRQF_ONESHOT)
desc->istate |= IRQS_ONESHOT;
 
+   if (new->flags & IRQF_NO_REPLAY)
+   desc->istate |= IRQS_NO_REPLAY;
+
if (irq_settings_can_autoenable(desc))
irq_startup(desc, true);
else
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -56,12 +56,12 @@ static DECLARE_TASKLET(resend_tasklet, r
 void check_irq_resend(struct irq_desc *desc)
 {
/*
-* We do not resend level type interrupts. Level type
-* interrupts are resent by hardware when they are still
-* active. Clear the pending bit so suspend/resume does not
-* get confused.
+* We do not resend level type interrupts. Level type interrupts
+* are resent by hardware when they are still active. Also prevent
+* resend when the user requested so.  Clear the pending bit so
+* suspend/resume does not get confused.
 */
-   if (irq_settings_is_level(desc)) {
+   if (irq_settings_is_level(desc) || (desc->istate & IRQS_NO_REPLAY)) {
desc->istate &= ~IRQS_PENDING;
return;
}
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -643,7 +643,10 @@ void handle_edge_irq(struct irq_desc *de
kstat_incr_irqs_this_cpu(desc);
 
/* Start handling the irq */
-   desc->irq_data.chip->irq_ack(>irq_data);
+   if (!(desc->istate & (IRQS_NO_REPLAY | IRQS_ONESHOT))
+   desc->irq_data.chip->irq_ack(>irq_data);
+   else
+   mask_ack_irq(desc);
 
do {
if (unlikely(!desc->action)) {

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Thomas Gleixner 
On Tue, 8 Nov 2016, Peter Rosin wrote:
> So, to sum up, in order for this to work with threaded oneshot
> interrupts, I still need to either keep the enable/sync/enable-dance
> or tweak the irq core to handle my case better. The only gain would
> be that I could fire the next step of the search from the threaded
> irq handler directly (but it needs some new race-killing code).
> Or am I missing something? If not, there's no pressing reason to
> switch to threaded oneshot interrupts, right?

There is no pressing reason, but that misfire prevention dance looks
fragile and overly complex to me.

The completely untested patch below should block the replay for edge
interrupts from the core code. It also makes sure that the edge interrupt
is masked until the thread handler returns. All you have to do is
requesting your threaded handler with IRQF_ONESHOT | IRQF_NO_REPLAY.

I don't think you need extra race handling with that, but I might be wrong
as usual.

Thanks,

tglx

8<--
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -74,6 +74,7 @@
 #define IRQF_NO_THREAD 0x0001
 #define IRQF_EARLY_RESUME  0x0002
 #define IRQF_COND_SUSPEND  0x0004
+#define IRQF_NO_REPLAY 0x0008
 
 #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | 
IRQF_NO_THREAD)
 
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -57,6 +57,7 @@ enum {
IRQS_WAITING= 0x0080,
IRQS_PENDING= 0x0200,
IRQS_SUSPENDED  = 0x0800,
+   IRQS_NO_REPLAY  = 0x1000,
 };
 
 #include "debug.h"
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1212,7 +1212,8 @@ static int
 */
if (!((old->flags & new->flags) & IRQF_SHARED) ||
((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
-   ((old->flags ^ new->flags) & IRQF_ONESHOT))
+   ((old->flags ^ new->flags) & IRQF_ONESHOT) ||
+   ((old->flags ^ new->flags) & IRQF_NO_REPLAY))
goto mismatch;
 
/* All handlers must agree on per-cpuness */
@@ -1324,6 +1325,9 @@ static int
if (new->flags & IRQF_ONESHOT)
desc->istate |= IRQS_ONESHOT;
 
+   if (new->flags & IRQF_NO_REPLAY)
+   desc->istate |= IRQS_NO_REPLAY;
+
if (irq_settings_can_autoenable(desc))
irq_startup(desc, true);
else
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -56,12 +56,12 @@ static DECLARE_TASKLET(resend_tasklet, r
 void check_irq_resend(struct irq_desc *desc)
 {
/*
-* We do not resend level type interrupts. Level type
-* interrupts are resent by hardware when they are still
-* active. Clear the pending bit so suspend/resume does not
-* get confused.
+* We do not resend level type interrupts. Level type interrupts
+* are resent by hardware when they are still active. Also prevent
+* resend when the user requested so.  Clear the pending bit so
+* suspend/resume does not get confused.
 */
-   if (irq_settings_is_level(desc)) {
+   if (irq_settings_is_level(desc) || (desc->istate & IRQS_NO_REPLAY)) {
desc->istate &= ~IRQS_PENDING;
return;
}
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -643,7 +643,10 @@ void handle_edge_irq(struct irq_desc *de
kstat_incr_irqs_this_cpu(desc);
 
/* Start handling the irq */
-   desc->irq_data.chip->irq_ack(>irq_data);
+   if (!(desc->istate & (IRQS_NO_REPLAY | IRQS_ONESHOT))
+   desc->irq_data.chip->irq_ack(>irq_data);
+   else
+   mask_ack_irq(desc);
 
do {
if (unlikely(!desc->action)) {

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Peter Rosin 
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(>comp_lock);
>> +comp = env->comp;
>> +env->comp = 0;
>> +spin_unlock_irq(>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(>comp_lock);
>> +comp = env->comp;
>> +env->comp = 0;
>> +spin_unlock_irq(>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

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Peter Rosin 
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(>comp_lock);
>> +comp = env->comp;
>> +env->comp = 0;
>> +spin_unlock_irq(>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(>comp_lock);
>> +comp = env->comp;
>> +env->comp = 0;
>> +spin_unlock_irq(>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

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Thomas Gleixner 
On Tue, 8 Nov 2016, Peter Rosin wrote:
> On 2016-11-08 16:59, Thomas Gleixner wrote:

> > 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..

Well, what guarantees you that the DAC is writeable from IRQ context? It
might be hanging off an i2c/spi bus as well

> > 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.

Threaded ONESHOT irqs work this way:

 interrupt fires
   mask interrupt
   handler thread is woken
 
 handler thread runs
   invokes isr
   unmask interrupt

So if you rewrite the DAC to the new value in your ISR, then you should not
get any spurious interrupt.

Note, that this only works for level type interrupts.

We do not mask edge type interrupts as we might lose an edge, but if that
helps the cause of your problem it's simple enough to make it conditionally
doing so in the core.

> 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?

Pending irqs are only replayed, when you reenable an interrupt via
enable_irq(). That can happen either by software or by hardware.

> 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.

So the main issue I'm seing here, is that your comparator does not have
means to prevent it from firing interrupts.

> 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?

Flipping the dectection level of the interrupt is fine, but what's the
guarantee that it is correct in the first place? I don't see anything which
makes that sure at all. Aside of that this bit does not makes sense:

> + env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
> + if (env->comp_irq_trigger & IRQF_TRIGGER_RISING)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_FALLING;

What's the |= about?

> + if (env->comp_irq_trigger & IRQF_TRIGGER_FALLING)

and this should be 'else if'. If the interrupt is configured for both
edges, which is possible with some interrupt controllers then the whole
thing does not work at all.

> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_RISING;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_HIGH)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_LOW;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_LOW)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_HIGH;

Thanks,

tglx

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Thomas Gleixner 
On Tue, 8 Nov 2016, Peter Rosin wrote:
> On 2016-11-08 16:59, Thomas Gleixner wrote:

> > 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..

Well, what guarantees you that the DAC is writeable from IRQ context? It
might be hanging off an i2c/spi bus as well

> > 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.

Threaded ONESHOT irqs work this way:

 interrupt fires
   mask interrupt
   handler thread is woken
 
 handler thread runs
   invokes isr
   unmask interrupt

So if you rewrite the DAC to the new value in your ISR, then you should not
get any spurious interrupt.

Note, that this only works for level type interrupts.

We do not mask edge type interrupts as we might lose an edge, but if that
helps the cause of your problem it's simple enough to make it conditionally
doing so in the core.

> 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?

Pending irqs are only replayed, when you reenable an interrupt via
enable_irq(). That can happen either by software or by hardware.

> 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.

So the main issue I'm seing here, is that your comparator does not have
means to prevent it from firing interrupts.

> 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?

Flipping the dectection level of the interrupt is fine, but what's the
guarantee that it is correct in the first place? I don't see anything which
makes that sure at all. Aside of that this bit does not makes sense:

> + env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
> + if (env->comp_irq_trigger & IRQF_TRIGGER_RISING)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_FALLING;

What's the |= about?

> + if (env->comp_irq_trigger & IRQF_TRIGGER_FALLING)

and this should be 'else if'. If the interrupt is configured for both
edges, which is possible with some interrupt controllers then the whole
thing does not work at all.

> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_RISING;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_HIGH)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_LOW;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_LOW)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_HIGH;

Thanks,

tglx

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Thomas Gleixner 
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(>comp_lock);
> + comp = env->comp;
> + env->comp = 0;
> + spin_unlock_irq(>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(>comp_lock);
> + comp = env->comp;
> + env->comp = 0;
> + spin_unlock_irq(>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?

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.

Thanks,

tglx

Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Thomas Gleixner 
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(>comp_lock);
> + comp = env->comp;
> + env->comp = 0;
> + spin_unlock_irq(>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(>comp_lock);
> + comp = env->comp;
> + env->comp = 0;
> + spin_unlock_irq(>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?

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.

Thanks,

tglx

[PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Peter Rosin 
The DAC is used to find the peak level of an alternating voltage input
signal by a binary search using the output of a comparator wired to
an interrupt pin. Like so:
  _
 | \
input +-->---|+ \
 |   \
   .---. |}---.
   |   | |   /|
   |dac|-->--|- / |
   |   | |_/  |
   |   |  |
   |   |  |
   |irq|--<---'
   |   |
   '---'

Signed-off-by: Peter Rosin 
---
 .../testing/sysfs-bus-iio-adc-envelope-detector|  36 ++
 MAINTAINERS|   2 +
 drivers/iio/adc/Kconfig|  10 +
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/envelope-detector.c| 422 +
 5 files changed, 471 insertions(+)
 create mode 100644 
Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
 create mode 100644 drivers/iio/adc/envelope-detector.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector 
b/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
new file mode 100644
index ..2071f9bcfaa5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
@@ -0,0 +1,36 @@
+What:  /sys/bus/iio/devices/iio:deviceX/in_altvoltageY_invert
+Date:  October 2016
+KernelVersion: 4.9
+Contact:   Peter Rosin 
+Description:
+   The DAC is used to find the peak level of an alternating
+   voltage input signal by a binary search using the output
+   of a comparator wired to an interrupt pin. Like so:
+  _
+ | \
+input +-->---|+ \
+ |   \
+   .---. |}---.
+   |   | |   /|
+   |dac|-->--|- / |
+   |   | |_/  |
+   |   |  |
+   |   |  |
+   |irq|--<---'
+   |   |
+   '---'
+   The boolean invert attribute (0/1) should be set when the
+   input signal is centered around the maximum value of the
+   dac instead of zero. The envelope detector will search
+   from below in this case and will also invert the result.
+   The edge/level of the interrupt is also switched to its
+   opposite value.
+
+What:  /sys/bus/iio/devices/iio:deviceX/in_altvoltageY_compare_interval
+Date:  October 2016
+KernelVersion: 4.9
+Contact:   Peter Rosin 
+Description:
+   Number of milliseconds to wait for the comparator in each
+   step of the binary search for the input peak level. Needs
+   to relate to the frequency of the input signal.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e13066ca3a2..539b20baf791 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6136,7 +6136,9 @@ IIO ENVELOPE DETECTOR
 M: Peter Rosin 
 L: linux-...@vger.kernel.org
 S: Maintained
+F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
 F: Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
+F: drivers/iio/adc/envelope-detector.c
 
 IIO SUBSYSTEM AND DRIVERS
 M: Jonathan Cameron 
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c051490eff..de4030894f23 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -195,6 +195,16 @@ config DA9150_GPADC
  To compile this driver as a module, choose M here: the module will be
  called berlin2-adc.
 
+config ENVELOPE_DETECTOR
+   tristate "Envelope detector using a DAC and a comparator"
+   depends on OF
+   help
+ Say yes here to build support for an envelope detector using a DAC
+ and a comparator.
+
+ To compile this driver as a module, choose M here: the module will be
+ called envelope-detector.
+
 config EXYNOS_ADC
tristate "Exynos ADC driver support"
depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && 
COMPILE_TEST)
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7a40c04c311f..0d773c6a0578 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
 obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
+obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o

[PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator

2016-11-08 Thread Peter Rosin 
The DAC is used to find the peak level of an alternating voltage input
signal by a binary search using the output of a comparator wired to
an interrupt pin. Like so:
  _
 | \
input +-->---|+ \
 |   \
   .---. |}---.
   |   | |   /|
   |dac|-->--|- / |
   |   | |_/  |
   |   |  |
   |   |  |
   |irq|--<---'
   |   |
   '---'

Signed-off-by: Peter Rosin 
---
 .../testing/sysfs-bus-iio-adc-envelope-detector|  36 ++
 MAINTAINERS|   2 +
 drivers/iio/adc/Kconfig|  10 +
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/envelope-detector.c| 422 +
 5 files changed, 471 insertions(+)
 create mode 100644 
Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
 create mode 100644 drivers/iio/adc/envelope-detector.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector 
b/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
new file mode 100644
index ..2071f9bcfaa5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
@@ -0,0 +1,36 @@
+What:  /sys/bus/iio/devices/iio:deviceX/in_altvoltageY_invert
+Date:  October 2016
+KernelVersion: 4.9
+Contact:   Peter Rosin 
+Description:
+   The DAC is used to find the peak level of an alternating
+   voltage input signal by a binary search using the output
+   of a comparator wired to an interrupt pin. Like so:
+  _
+ | \
+input +-->---|+ \
+ |   \
+   .---. |}---.
+   |   | |   /|
+   |dac|-->--|- / |
+   |   | |_/  |
+   |   |  |
+   |   |  |
+   |irq|--<---'
+   |   |
+   '---'
+   The boolean invert attribute (0/1) should be set when the
+   input signal is centered around the maximum value of the
+   dac instead of zero. The envelope detector will search
+   from below in this case and will also invert the result.
+   The edge/level of the interrupt is also switched to its
+   opposite value.
+
+What:  /sys/bus/iio/devices/iio:deviceX/in_altvoltageY_compare_interval
+Date:  October 2016
+KernelVersion: 4.9
+Contact:   Peter Rosin 
+Description:
+   Number of milliseconds to wait for the comparator in each
+   step of the binary search for the input peak level. Needs
+   to relate to the frequency of the input signal.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e13066ca3a2..539b20baf791 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6136,7 +6136,9 @@ IIO ENVELOPE DETECTOR
 M: Peter Rosin 
 L: linux-...@vger.kernel.org
 S: Maintained
+F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
 F: Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
+F: drivers/iio/adc/envelope-detector.c
 
 IIO SUBSYSTEM AND DRIVERS
 M: Jonathan Cameron 
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c051490eff..de4030894f23 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -195,6 +195,16 @@ config DA9150_GPADC
  To compile this driver as a module, choose M here: the module will be
  called berlin2-adc.
 
+config ENVELOPE_DETECTOR
+   tristate "Envelope detector using a DAC and a comparator"
+   depends on OF
+   help
+ Say yes here to build support for an envelope detector using a DAC
+ and a comparator.
+
+ To compile this driver as a module, choose M here: the module will be
+ called envelope-detector.
+
 config EXYNOS_ADC
tristate "Exynos ADC driver support"
depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && 
COMPILE_TEST)
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7a40c04c311f..0d773c6a0578 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
 obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
+obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o