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@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
