[PATCH 01/13] staging: comedi: pcmuio: fix interrupt requests

Ian Abbott abbotti at mev.co.uk
Fri Jul 26 12:09:59 UTC 2013


On 2013-07-25 17:33, H Hartley Sweeten wrote:
> On Thursday, July 25, 2013 5:34 AM, Ian Abbott wrote:
>> On 2013-07-24 19:45, H Hartley Sweeten wrote:
>>> Legacy (ISA) interrupts are not sharable so this driver should not
>>> be passing the IRQF_SHARED flag when requesting the interrupts.
>>>
>>> This driver supports two board types:
>>>     PCM-UIO48 with one asic (one interrupt source)
>>>     PCM-UIO96 with two asics (two interrupt sources)
>>>
>>> The PCM-UIO96 has a jumper that allows the two interrupt sources to
>>> share an interrupt. This is safe for legacy interrupts as long as
>>> the "shared" interrupt is handled by a single driver.
>>>
>>> Modify the request_irq() code in this driver to correctly request the
>>> interrupts. For the PCM-UI048 case (one asic) only one request_irq()
>>> is needed. For the PCM-UIO96 (two asics) there are two cases:
>>>
>>>     1) interrupt is shared, one request_irq() call
>>>     2) two interrupts, two request_irq() calls
>>>
>>> Do the first request_irq() in all cases. Only do the second if the
>>> boardinfo indicates that the board has two asics and the user passed
>>> the 2nd interrupt number.
>>>
>>> Pack the two interrupt numbers in dev->irq instead of storing them in
>>> the private data. During the detach of the board, this driver will
>>> free the 2nd irq if needed and the core will free the 1st.
>>>
>>> Move the board reset and interrupt request code so it occurs early
>>> in the attach. We can then check dev->irq to see if the subdevice
>>> interrupt support actually needs to be initialized.
>>>
>>> Add a call to pcmuio_reset() in the (*detach) to make sure the
>>> interrupts are disabled before freeing the irqs.
>>>
>>> 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/pcmuio.c | 96 ++++++++++++++++++---------------
>>>    1 file changed, 54 insertions(+), 42 deletions(-)
>>
>> Packing the two irq values in a single member seems a bit hackish.  I
>> think adding an 'irq2' member to struct pcmuio_private would be cleaner.
>
> It is a bit "hackish" but the hack is documented with comments. The 'irq' member
> is only used by the core in comedi_legacy_detach() so the hack will not have any
> side effects.

Agreed, but you have to massage dev->irq in your detach routine.  It 
just seems cleaner to keep the second irq in a separate member!

>
> <snip>
>
>>> @@ -617,6 +607,39 @@ static int pcmuio_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>>>    	for (asic = 0; asic < PCMUIO_MAX_ASICS; ++asic)
>>>    		spin_lock_init(&devpriv->asics[asic].spinlock);
>>>
>>> +	pcmuio_reset(dev);
>>> +
>>> +	/* request the 1st irq */
>>> +	irq = it->options[1];
>>> +	if (irq) {
>>> +		ret = request_irq(irq, pcmuio_interrupt, 0,
>>> +				  board->name, dev);
>>> +		if (ret == 0) {
>>> +			/* save the irq for the core to free */
>>> +			dev->irq = irq;
>>> +		}
>>> +	}
>>> +
>>> +	/* request the 2nd irq if needed */
>>> +	if (board->num_asics == 2 && dev->irq) {
>>> +		irq = it->options[2];
>>> +		if (irq && irq != dev->irq) {
>>> +			/* 1st and 2nd irq differ */
>>> +			ret = request_irq(irq, pcmuio_interrupt, 0,
>>> +					  board->name, dev);
>>> +			if (ret == 0) {
>>> +				/* save the irq for (*detach) to free */
>>> +				dev->irq |= (irq << 8);
>>> +			} else {
>>> +				free_irq(dev->irq, dev);
>>> +				dev->irq = 0;
>>> +			}
>>> +		} else {
>>> +			/* 1st and 2nd irq are the same (or 2nd is zero) */
>>> +			dev->irq |= (irq << 8);
>>> +		}
>>> +	}
>>> +
>>
>> It could possibly allow just the second IRQ to be used even if the first
>> IRQ is not used (0), but the original driver didn't allow that.
>
> This might be desired?

It doesn't really matter, but if two IRQs are being used and only one 
can be set up successfully, perhaps partial interrupt support is 
preferable to no interrupt support.

>
>>>    	n_subdevs = board->num_asics * 2;
>>>    	devpriv->sprivs = kcalloc(n_subdevs, sizeof(*subpriv), GFP_KERNEL);
>>>    	if (!devpriv->sprivs)
>>> @@ -639,7 +662,7 @@ static int pcmuio_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>>>    		s->n_chan = 24;
>>>
>>>    		/* subdevices 0 and 2 suppport interrupts */
>>> -		if ((sdev_no % 2) == 0) {
>>> +		if ((sdev_no % 2) == 0 && dev->irq) {
>>
>> You've checked if irq is set before allowing commands to be set-up, but
>> you haven't checked which of the possibly two irqs is set.
>
> Hmm.. True.
>
> I guess this test should really be:
>
> 		if ((sdev_no % 2) == 0 && (dev->irq >> (8 * (sdev_no % 2)))) {
>
> Then the subdevice command operations will only be hooked up if the irq is
> available.
>
> Can this be fixed as a separate patch or would you prefer that I rebase the
> series?

It can be a separate patch.

> Also, if you would prefer using a separate 'irq' member in the private data for
> the 2nd irq let me know. This will of course effect multiple parts of the series.

dev->irq itself doesn't seem to be used by the other patches, although 
the patches that change struct pcmuio_private would need rebasing.  It 
could be done as a separate patch to avoid that unless you're going to 
post a v2 series anyway.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-


More information about the devel mailing list