[PATCH 04/19] staging: comedi: ni_mio_common: tidy up DIO subdevice ifdef'ery

Ian Abbott abbotti at mev.co.uk
Wed Apr 13 12:04:17 UTC 2016


On 12/04/16 22:12, H Hartley Sweeten wrote:
> This file is is bit of a mess. It's included by the ni_atmio, ni_mio_cs, and
> ni_pcimio drivers. The ni_pcimio driver is the only one that uses DMA. It
> defines PCIDMA so that the dma code is compiled it. This causes a bunch
> of ifdef'ery in the file.
>
> The DIO subdevice for the ni_pcidio "is_m_series" boards is quite different
> from the standard e-series DIO. Mainly it supports async commands that use
> DMA.
>
> Tidy up some of the ifdef'ery by adding ifdef to the subdevice init.
>
> Also and ifdef to the interrupt handler and remove the unnecessary if
> (!devpriv->is_m_series) check in handle_cdio_interrupt().
>
> Consolidate the other ifdef's to block out the affected code.
>
> 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/ni_mio_common.c | 26 ++++++++------------------
>   1 file changed, 8 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 11a2453..8544de1 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
[snip]
> @@ -3716,13 +3707,8 @@ static void handle_cdio_interrupt(struct comedi_device *dev)
>   	struct ni_private *devpriv = dev->private;
>   	unsigned cdio_status;
>   	struct comedi_subdevice *s = &dev->subdevices[NI_DIO_SUBDEV];
> -#ifdef PCIDMA
>   	unsigned long flags;
> -#endif
>
> -	if (!devpriv->is_m_series)
> -		return;

Removal of that test causes the function to screw up for E-series cards...

> -#ifdef PCIDMA
>   	spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
>   	if (devpriv->cdo_mite_chan) {
>   		unsigned cdo_mite_status =
> @@ -3735,7 +3721,6 @@ static void handle_cdio_interrupt(struct comedi_device *dev)
>   		mite_sync_output_dma(devpriv->cdo_mite_chan, s);
>   	}
>   	spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
> -#endif
>
>   	cdio_status = ni_readl(dev, NI_M_CDIO_STATUS_REG);
>   	if (cdio_status & NI_M_CDIO_STATUS_CDO_ERROR) {
> @@ -3751,6 +3736,7 @@ static void handle_cdio_interrupt(struct comedi_device *dev)
>   	}
>   	comedi_handle_events(dev, s);
>   }

...by reading a bogus (on E-series) register, possibly setting 
s->async->events and always calling comedi_handle_events() (which also 
uses s->async), even though s->async will be NULL for this subdevice on 
E-series cards.

So it needs some sort of test, and as its using M-series registers, 
testing devpriv->is_m_series seems reasonable enough.  Alternatively, it 
could test that s->async is non-NULL.

> +#endif /*  PCIDMA */
>
>   static int ni_serial_hw_readwrite8(struct comedi_device *dev,
>   				   struct comedi_subdevice *s,
> @@ -5266,7 +5252,9 @@ static irqreturn_t ni_E_interrupt(int irq, void *d)
>   		handle_b_interrupt(dev, b_status, ao_mite_status);
>   	handle_gpct_interrupt(dev, 0);
>   	handle_gpct_interrupt(dev, 1);
> +#ifdef PCIDMA
>   	handle_cdio_interrupt(dev);
> +#endif

Alternatively, handle_cdio_interrupt() could be called conditionally here.

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


More information about the devel mailing list