On Tuesday, April 19, 2016 3:23 AM, Ian Abbott wrote:
> On 18/04/16 21:28, H Hartley Sweeten wrote:
>> Introduce a helper function to handle the ack of a LINKC interrupt.
>> Tidy up the drivers that use the new helper.
>>
>> The mite_get_status() function is not only used by the mite driver.
>> Make it static and remove the export.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/comedi/drivers/mite.c | 17 ++++++++++++++--
>> drivers/staging/comedi/drivers/mite.h | 2 +-
>> drivers/staging/comedi/drivers/ni_mio_common.c | 28 +++++---------------------
>> drivers/staging/comedi/drivers/ni_pcidio.c | 10 ++-------
>> drivers/staging/comedi/drivers/ni_tiocmd.c | 11 ++--------
>> 5 files changed, 25 insertions(+), 43 deletions(-)
>>
> [snip]
>> diff --git a/drivers/staging/comedi/drivers/ni_pcidio.c b/drivers/staging/comedi/drivers/ni_pcidio.c
>> index c044c8b..b67358d 100644
>> --- a/drivers/staging/comedi/drivers/ni_pcidio.c
>> +++ b/drivers/staging/comedi/drivers/ni_pcidio.c
>> @@ -381,12 +381,10 @@ static irqreturn_t nidio_interrupt(int irq, void *d)
>> struct nidio96_private *devpriv = dev->private;
>> struct comedi_subdevice *s = dev->read_subdev;
>> struct comedi_async *async = s->async;
>> - struct mite_struct *mite = devpriv->mite;
>> unsigned int auxdata;
>> int flags;
>> int status;
>> int work = 0;
>> - unsigned int m_status = 0;
>>
>> /* interrupcions parasites */
>> if (!dev->attached) {
>> @@ -401,14 +399,10 @@ static irqreturn_t nidio_interrupt(int irq, void *d)
>> flags = readb(dev->mmio + Group_1_Flags);
>>
>> spin_lock(&devpriv->mite_channel_lock);
>> - if (devpriv->di_mite_chan)
>> - m_status = mite_get_status(devpriv->di_mite_chan);
>> + if (devpriv->di_mite_chan) {
>> + unsigned int m_status = mite_ack_linkc(devpriv->di_mite_chan);
>>
>> - if (m_status & CHSR_INT) {
>
> Is the removal of that `m_status & CHSR_INT` test deliberate? It looks
> a bit iffy.
Yes. This is the only driver that uses mite that checks the CHSR_INT bit.
Unfortunately I have not been able to find any "good" information on the
NI MITE ASIC. My guess is that this bit is a global interrupt status bit. It gets
set if any of the other interrupt sources are generating an interrupt.
The mite_get_status() function also does not check for CHSR_INT when
acking the CHSR_DONE interrupt so I assume that the extra check is not
needed.
Let me know if you prefer to leave in the check.
Side-note regarding the CHSR bits. Some of the subdevices that use mite
also check for "unknown" interrupts.
ni_pcimio for the DIO "read_subdev"
ni_mio_common for the AI "read_subdev"
for the AO "write_subdev"
But others don't:
ni_mio_common for the DIO subdevice
ni_tiocmd for the ni_660x COUNTER subdevices
ni_tiocmd for the ni_mio_common COUNTER subdevices
How do you feel about removing the "unknown" interrupt checks?
If they are removed the CHSR_* defines in mite.h can be moved to the
mite driver and not be needlessly exposed.
>> if (m_status & CHSR_LINKC) {
>> - writel(CHOR_CLRLC,
>> - mite->mite_io_addr +
>> - MITE_CHOR(devpriv->di_mite_chan->channel));
>> mite_sync_input_dma(devpriv->di_mite_chan, s);
>> /* XXX need to byteswap */
>> }
>> diff --git a/drivers/staging/comedi/drivers/ni_tiocmd.c b/drivers/staging/comedi/drivers/ni_tiocmd.c
>> index 3c3f543..e9ee06b 100644
>> --- a/drivers/staging/comedi/drivers/ni_tiocmd.c
>> +++ b/drivers/staging/comedi/drivers/ni_tiocmd.c
>> @@ -400,7 +400,6 @@ void ni_tio_handle_interrupt(struct ni_gpct *counter,
>> struct comedi_subdevice *s)
>> {
>> unsigned int cidx = counter->counter_index;
>> - unsigned int gpct_mite_status;
>> unsigned long flags;
>> int gate_error;
>> int tc_error;
>> @@ -430,15 +429,9 @@ void ni_tio_handle_interrupt(struct ni_gpct *counter,
>> }
>> spin_lock_irqsave(&counter->lock, flags);
>> if (!counter->mite_chan) {
>> - spin_unlock_irqrestore(&counter->lock, flags);
>> - return;
>> + mite_ack_linkc(counter->mite_chan);
>> + mite_sync_input_dma(counter->mite_chan, s);
>
> That's wrong. I think you forgot to invert the `!counter->mite_chan` test.
Ugh... Messed that up during a rebase... I'll fix and repost after I hear back
from you on the CHSR_INT issue.
Thanks,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
