[PATCH 1/6] staging: comedi: addi_apci_1564: clarify change-of-state interrupt support

Ian Abbott abbotti at mev.co.uk
Mon Jun 6 19:33:14 UTC 2016


On 06/06/16 19:25, Hartley Sweeten wrote:
> On Friday, June 03, 2016 2:58 AM, Ian Abbott wrote:
>> On 02/06/16 22:58, H Hartley Sweeten wrote:
>>> This board supports change-of-state interrupts on digital inputs 4 to 19
>>> not 0 to 15.
>>>
>>> The current code "works" but it could set inappropriate bits in the mode1
>>> and mode2 registers that setup which channels are enabled. It also doesn't
>>> return the status of the upper 4 channels (19 to 16).
>>>
>>> Fix the comment and mask the mode1/mode2 values so that only the interrupt
>>> capable channels can be enabled.
>>>
>>> Add the SDF_LSAMPL flag to the subdevice so that 32-bit samples are used
>>> instead of 16-bit ones. This allows returning the upper 4 channels. Use
>>> the remaining bits in the sample to return "event" flags to the user.
>>>
>>> The timer and counter subdevices can also generate interrupts and are a bit
>>> hacked. They don't currently follow the comedi API and they use send_sig()
>>> to let the task that know that the interrupt occured. The "event" flags will
>>> be used instead when these subdevices are fixed.
>>>
>>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>>> ---
>>>    drivers/staging/comedi/drivers/addi_apci_1564.c | 37 +++++++++++++++++--------
>>>    1 file changed, 26 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
>>> index f1ccfbd..37e18b3 100644
>
> <snip>
>
>>> +#define APCI1564_DI_INT_MODE_MASK		0x000ffff0 /* chans [19:4] */
>
> <snip>
>
>>> +#define APCI1564_EVENT_MASK			0xfff0000f /* all but [19:4] */
>
> <snip>
>
>>> +	s->state &= ~APCI1564_EVENT_MASK;
>>> +
>>    	status = inl(dev->iobase + APCI1564_DI_IRQ_REG);
>>    	if (status & APCI1564_DI_IRQ_ENA) {
>> -		/* disable the interrupt */
>> +		s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);
>> +		s->state |= APCI1564_EVENT_COS;
>
> <snip>
>
>> +	if (s->state & APCI1564_EVENT_MASK) {
>> +		comedi_buf_write_samples(s, &s->state, 1);
>> +		comedi_handle_events(dev, s);
>> +	}
>> +
>>    	return IRQ_HANDLED;
>>    }
>
>> I'm struggling to see how that works.  It looks like once
>> APCI1564_EVENT_COS has been set in s->state, it never gets cleared (or
>> at least it only gets cleared momentarily), and after that, the `if
>> (s->state & APCI1564_EVENT_MASK)` test will always be true, and so it
>> will write a sample to the buffer on every interrupt.
>
> Ian,
>
> 1) When an interrupt occurs the last "events" are masked off
>      (leaving the last state of the digital inputs).
> 2) If the digital inputs caused the interrupt, the state of the
>      digital inputs is read and the event bit APCI1564_EVENT_COS
>      is set.

Okay, I think I must have got something inverted in my head at the time 
I wrote my reply.

Follow-up question: when you do:

		s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG);

should that be ANDed with something (possibly 
APCI1564_DI_INT_MODE_MASK), or are the other bits (including the new 
"event" bits) guaranteed to read zero?


-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-


More information about the devel mailing list