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.
<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?
>> 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?
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.
Thanks,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
