On Thursday, May 29, 2014 5:29 AM, Ian Abbott wrote: > From: Fred Brooks <frederick.bro...@microchip.com> > > Add support for switching the input range and the single-ended/ > differential input mode for the AI subdevice. We needed to clear the > FIFO of data before the conversion to handle card mode switching > glitches. > > [ Minor whitespace fixes and driver comment reformatting. - Ian ] > > Signed-off-by: Fred Brooks <frederick.bro...@microchip.com> > Signed-off-by: Ian Abbott <abbo...@mev.co.uk> > --- > v2: Set authorship to Fred Brooks and updated patch description as > suggested by Dan Carpenter. > --- > drivers/staging/comedi/drivers/ni_daq_700.c | 53 > +++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 10 deletions(-)
Ian, I couple nitpicks on this patch but nothing big. Ignore all of these comments if you wish. Reviewed-by: H Hartley Sweeten <hswee...@visionengravers.com> > diff --git a/drivers/staging/comedi/drivers/ni_daq_700.c > b/drivers/staging/comedi/drivers/ni_daq_700.c > index 55d80ef..e66c0b9 100644 > --- a/drivers/staging/comedi/drivers/ni_daq_700.c > +++ b/drivers/staging/comedi/drivers/ni_daq_700.c > @@ -24,21 +24,30 @@ > * based on ni_daq_dio24 by Daniel Vecino Castel <dvec...@able.es> > * Devices: [National Instruments] PCMCIA DAQ-Card-700 (ni_daq_700) > * Status: works > - * Updated: Wed, 19 Sep 2012 12:07:20 +0000 > + * Updated: Wed, 21 May 2014 12:07:20 +0000 > * > * The daqcard-700 appears in Comedi as a digital I/O subdevice (0) with > - * 16 channels and a analog input subdevice (1) with 16 single-ended > channels. > + * 16 channels and a analog input subdevice (1) with 16 single-ended channels > + * or 8 differential channels, and three input ranges. > * > * Digital: The channel 0 corresponds to the daqcard-700's output > * port, bit 0; channel 8 corresponds to the input port, bit 0. > * > * Digital direction configuration: channels 0-7 output, 8-15 input. > * > - * Analog: The input range is 0 to 4095 for -10 to +10 volts > + * Analog: The input range is 0 to 4095 with a default of -10 to +10 volts. > + * Valid ranges: > + * 0 for -10 to 10V bipolar > + * 1 for -5 to 5V bipolar > + * 2 for -2.5 to 2.5V bipolar > + * > * IRQ is assigned but not used. > * > * Version 0.1 Original DIO only driver > * Version 0.2 DIO and basic AI analog input support on 16 se channels > + * Version 0.3 Add SE or DIFF mode and range switching +-10,+-5,+-2.5 > + * Clear the FIFO of data before the conversion to handle card > + * mode switching glitches. Is this version information really necessary? > * > * Manuals: Register level: http://www.ni.com/pdf/manuals/340698.pdf > * User Manual: http://www.ni.com/pdf/manuals/320676d.pdf > @@ -68,6 +77,17 @@ > #define CDA_R2 0x0A /* RW 8bit */ > #define CMO_R 0x0B /* RO 8bit */ > #define TIC_R 0x06 /* WO 8bit */ > +/* daqcard700 modes */ > +#define CMD_R3_DIFF 0x04 /* diff mode */ > + > +static const struct comedi_lrange range_daq700_ai = { > + 3, > + { > + BIP_RANGE(10), > + BIP_RANGE(5), > + BIP_RANGE(2.5) > + } > +}; The "normal" form for this in the comedi drivers is: 3, { > static int daq700_dio_insn_bits(struct comedi_device *dev, > struct comedi_subdevice *s, > @@ -130,20 +150,34 @@ static int daq700_ai_rinsn(struct comedi_device *dev, > struct comedi_subdevice *s, > struct comedi_insn *insn, unsigned int *data) > { > - int n, chan; > + int n; > int d; > int ret; > + unsigned int chan = CR_CHAN(insn->chanspec); > + unsigned int aref = CR_AREF(insn->chanspec); > + unsigned int range = CR_RANGE(insn->chanspec); > + unsigned int r3_bits = 0; Is the whitespace really necessary on the initialized variables? Also, I think the uninitialized variables should be listed after the initialized ones. > + > + /* set channel input modes */ > + if (aref == AREF_DIFF) > + r3_bits |= CMD_R3_DIFF; > + /* write channel mode/range */ > + if (range >= 1) > + range++; /* convert range to hardware value */ > + outb(r3_bits | (range & 0x03), dev->iobase + CMD_R3); > > - chan = CR_CHAN(insn->chanspec); > /* write channel to multiplexer */ > /* set mask scan bit high to disable scanning */ > - outb(chan | 0x80, dev->iobase + CMD_R1); > + outb((chan & 0x0f) | 0x80, dev->iobase + CMD_R1); > > /* convert n samples */ > for (n = 0; n < insn->n; n++) { > /* trigger conversion with out0 L to H */ > outb(0x00, dev->iobase + CMD_R2); /* enable ADC conversions */ > outb(0x30, dev->iobase + CMO_R); /* mode 0 out0 L, from H */ > + outb(0x00, dev->iobase + ADCLEAR_R); /* clear the ADC FIFO */ > + /* read 16bit junk from FIFO to clear */ > + inw(dev->iobase + ADFIFO_R); > /* mode 1 out0 H, L to H, start conversion */ > outb(0x32, dev->iobase + CMO_R); > > @@ -219,11 +253,10 @@ static int daq700_auto_attach(struct comedi_device *dev, > /* DAQCard-700 ai */ > s = &dev->subdevices[1]; > s->type = COMEDI_SUBD_AI; > - /* we support single-ended (ground) */ > - s->subdev_flags = SDF_READABLE | SDF_GROUND; > + s->subdev_flags = SDF_READABLE | SDF_GROUND | SDF_DIFF; > s->n_chan = 16; > s->maxdata = (1 << 12) - 1; > - s->range_table = &range_bipolar10; > + s->range_table = &range_daq700_ai; > s->insn_read = daq700_ai_rinsn; > daq700_ai_config(dev, s); > > @@ -260,5 +293,5 @@ module_comedi_pcmcia_driver(daq700_driver, > daq700_cs_driver); > MODULE_AUTHOR("Fred Brooks <nsasp...@nsaspook.com>"); > MODULE_DESCRIPTION( > "Comedi driver for National Instruments PCMCIA DAQCard-700 DIO/AI"); > -MODULE_VERSION("0.2.00"); > +MODULE_VERSION("0.3.00"); Is the MODULE_VERSION necessary? The vmk80xx and this driver are the only ones that have it. > MODULE_LICENSE("GPL"); > -- > 1.9.2 Regards, Hartley _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
