Re: [PATCH 07/49] staging: comedi: amplc_pci230: factor out step 5 of (*do_cmdtest)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2014-04-15 18:37, H Hartley Sweeten wrote:
Step 5 of the (*do_cmdtest) validates that the chanlist, chan/range/aref/etc,
is compatible with the actual hardware. At this point in the (*do_cmdtest)
the chanlist is valid, due to checks in the core, so the sanity check is not
needed.

For aesthetics, factor the step 5 code into a helper function. Tidy up the
factored out code.

Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
  drivers/staging/comedi/drivers/amplc_pci230.c | 306 ++++++++++++--------------
  1 file changed, 135 insertions(+), 171 deletions(-)

diff --git a/drivers/staging/comedi/drivers/amplc_pci230.c b/drivers/staging/comedi/drivers/amplc_pci230.c
index 99e6083..20765f1 100644
--- a/drivers/staging/comedi/drivers/amplc_pci230.c
+++ b/drivers/staging/comedi/drivers/amplc_pci230.c
@@ -953,6 +953,37 @@ static int pci230_ao_rinsn(struct comedi_device *dev,
  	return i;
  }

+static int pci230_ao_check_chanlist(struct comedi_device *dev,
+				    struct comedi_subdevice *s,
+				    struct comedi_cmd *cmd)
+{
+	unsigned int prev_chan = CR_CHAN(cmd->chanlist[0]);
+	unsigned int range0 = CR_RANGE(cmd->chanlist[0]);
+	int i;
+
+	for (i = 1; i < cmd->chanlist_len; i++) {
+		unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+		unsigned int range = CR_RANGE(cmd->chanlist[i]);
+
+		if (chan < prev_chan) {
+			dev_dbg(dev->class_dev,
+				"%s: channel numbers must increase\n",
+				__func__);
+			return -EINVAL;
+		}
+
+		if (range != range0) {
+			dev_dbg(dev->class_dev,
+				"%s: channels must have the same range\n",
+				__func__);
+			return -EINVAL;
+		}
+
+		prev_chan = chan;
+	}
+	return 0;
+}
+
  static int pci230_ao_cmdtest(struct comedi_device *dev,
  			     struct comedi_subdevice *s, struct comedi_cmd *cmd)
  {
@@ -1065,48 +1096,9 @@ static int pci230_ao_cmdtest(struct comedi_device *dev,
  	if (err)
  		return 4;

-	/* Step 5: check channel list if it exists. */
-
-	if (cmd->chanlist && cmd->chanlist_len > 0) {
-		enum {
-			seq_err = (1 << 0),
-			range_err = (1 << 1)
-		};
-		unsigned int errors;
-		unsigned int n;
-		unsigned int chan, prev_chan;
-		unsigned int range, first_range;
-
-		prev_chan = CR_CHAN(cmd->chanlist[0]);
-		first_range = CR_RANGE(cmd->chanlist[0]);
-		errors = 0;
-		for (n = 1; n < cmd->chanlist_len; n++) {
-			chan = CR_CHAN(cmd->chanlist[n]);
-			range = CR_RANGE(cmd->chanlist[n]);
-			/* Channel numbers must strictly increase. */
-			if (chan < prev_chan)
-				errors |= seq_err;
-
-			/* Ranges must be the same. */
-			if (range != first_range)
-				errors |= range_err;
-
-			prev_chan = chan;
-		}
-		if (errors != 0) {
-			err++;
-			if ((errors & seq_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: channel numbers must increase\n",
-					__func__);
-			}
-			if ((errors & range_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: channels must have the same range\n",
-					__func__);
-			}
-		}
-	}
+	/* Step 5: check channel list */
+
+	err |= pci230_ao_check_chanlist(dev, s, cmd);

  	if (err)
  		return 5;
@@ -1552,6 +1544,105 @@ static int pci230_ai_check_scan_period(struct comedi_cmd *cmd)
  	return !err;
  }

As for the remarks in patch 02, it's possible for chanlist to be NULL or for chanlist_len to be 0, so pci230_ao_check_chanlist() should only be called if chanlist is non-NULL and chanlist_len is non-0.

But if chanlist is forced to be NULL by __comedi_get_user_chanlist() when chanlist_len is 0, it would be sufficient to check that chanlist is non-NULL before calling pci230_ao_check_chanlist().


+static int pci230_ai_check_chanlist(struct comedi_device *dev,
+				    struct comedi_subdevice *s,
+				    struct comedi_cmd *cmd)
+{
+	struct pci230_private *devpriv = dev->private;
+	unsigned int max_diff_chan = (s->n_chan / 2) - 1;
+	unsigned int prev_chan = 0;
+	unsigned int prev_range = 0;
+	unsigned int prev_aref = 0;
+	unsigned int prev_polarity = 0;
+	unsigned int subseq_len = 0;
+	int i;
+
+	for (i = 0; i < cmd->chanlist_len; i++) {
+		unsigned int chanspec = cmd->chanlist[i];
+		unsigned int chan = CR_CHAN(chanspec);
+		unsigned int range = CR_RANGE(chanspec);
+		unsigned int aref = CR_AREF(chanspec);
+		unsigned int polarity = pci230_ai_bipolar[range];
+
+		if (aref == AREF_DIFF && chan > max_diff_chan) {
+			dev_dbg(dev->class_dev,
+				"%s: differential channel number out of range 0 to %u\n",
+				__func__, max_diff_chan);
+			return -EINVAL;
+		}
+
+		if (i > 0) {
+			if (chan <= prev_chan && subseq_len == 0)
+				subseq_len = i;
+
+			if (subseq_len > 0 &&
+			    cmd->chanlist[i % subseq_len] != chanspec) {
+				dev_dbg(dev->class_dev,
+					"%s: channel numbers must increase or sequence must repeat exactly\n",
+					__func__);
+				return -EINVAL;
+			}
+
+			if (aref != prev_aref) {
+				dev_dbg(dev->class_dev,
+					"%s: channel sequence analogue references must be all the same (single-ended or differential)\n",
+					__func__);
+				return -EINVAL;
+			}
+
+			if (polarity != prev_polarity) {
+				dev_dbg(dev->class_dev,
+					"%s: channel sequence ranges must be all bipolar or all unipolar\n",
+					__func__);
+				return -EINVAL;
+			}
+
+			if (aref != AREF_DIFF && range != prev_range &&
+			    ((chan ^ prev_chan) & ~1) == 0) {
+				dev_dbg(dev->class_dev,
+					"%s: single-ended channel pairs must have the same range\n",
+					__func__);
+				return -EINVAL;
+			}
+		}
+		prev_chan = chan;
+		prev_range = range;
+		prev_aref = aref;
+		prev_polarity = polarity;
+	}
+
+	if (subseq_len == 0)
+		subseq_len = cmd->chanlist_len;
+
+	if ((cmd->chanlist_len % subseq_len) != 0) {
+		dev_dbg(dev->class_dev,
+			"%s: channel numbers must increase or sequence must repeat exactly\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * Buggy PCI230+ or PCI260+ requires channel 0 to be (first) in the
+	 * sequence if the sequence contains more than one channel. Hardware
+	 * versions 1 and 2 have the bug.  There is no hardware version 3.
+	 *
+	 * Actually, there are two firmwares that report themselves as
+	 * hardware version 1 (the boards have different ADC chips with
+	 * slightly different timing requirements, which was supposed to
+	 * be invisible to software). The first one doesn't seem to have
+	 * the bug, but the second one does, and we can't tell them apart!
+	 */
+	if (devpriv->hwver > 0 && devpriv->hwver < 4) {
+		if (subseq_len > 1 && CR_CHAN(cmd->chanlist[0]) != 0) {
+			dev_info(dev->class_dev,
+				 "%s: Buggy PCI230+/260+ h/w version %u requires first channel of multi-channel sequence to be 0 (corrected in h/w version 4)\n",
+				 __func__, devpriv->hwver);
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
  static int pci230_ai_cmdtest(struct comedi_device *dev,
  			     struct comedi_subdevice *s, struct comedi_cmd *cmd)
  {
@@ -1740,136 +1831,9 @@ static int pci230_ai_cmdtest(struct comedi_device *dev,
  	if (err)
  		return 4;

-	/* Step 5: check channel list if it exists. */
-
-	if (cmd->chanlist && cmd->chanlist_len > 0) {
-		enum {
-			seq_err = 1 << 0,
-			rangepair_err = 1 << 1,
-			polarity_err = 1 << 2,
-			aref_err = 1 << 3,
-			diffchan_err = 1 << 4,
-			buggy_chan0_err = 1 << 5
-		};
-		unsigned int errors;
-		unsigned int chan, prev_chan;
-		unsigned int range, prev_range;
-		unsigned int polarity, prev_polarity;
-		unsigned int aref, prev_aref;
-		unsigned int subseq_len;
-		unsigned int n;
-
-		subseq_len = 0;
-		errors = 0;
-		prev_chan = prev_aref = prev_range = prev_polarity = 0;
-		for (n = 0; n < cmd->chanlist_len; n++) {
-			chan = CR_CHAN(cmd->chanlist[n]);
-			range = CR_RANGE(cmd->chanlist[n]);
-			aref = CR_AREF(cmd->chanlist[n]);
-			polarity = pci230_ai_bipolar[range];
-			/* Only the first half of the channels are available if
-			 * differential.  (These are remapped in software.  In
-			 * hardware, only the even channels are available.) */
-			if ((aref == AREF_DIFF)
-			    && (chan >= (s->n_chan / 2))) {
-				errors |= diffchan_err;
-			}
-			if (n > 0) {
-				/* Channel numbers must strictly increase or
-				 * subsequence must repeat exactly. */
-				if ((chan <= prev_chan)
-				    && (subseq_len == 0)) {
-					subseq_len = n;
-				}
-				if ((subseq_len > 0)
-				    && (cmd->chanlist[n] !=
-					cmd->chanlist[n % subseq_len])) {
-					errors |= seq_err;
-				}
-				/* Channels must have same AREF. */
-				if (aref != prev_aref)
-					errors |= aref_err;
-
-				/* Channel ranges must have same polarity. */
-				if (polarity != prev_polarity)
-					errors |= polarity_err;
-
-				/* Single-ended channel pairs must have same
-				 * range.  */
-				if ((aref != AREF_DIFF)
-				    && (((chan ^ prev_chan) & ~1) == 0)
-				    && (range != prev_range)) {
-					errors |= rangepair_err;
-				}
-			}
-			prev_chan = chan;
-			prev_range = range;
-			prev_aref = aref;
-			prev_polarity = polarity;
-		}
-		if (subseq_len == 0) {
-			/* Subsequence is whole sequence. */
-			subseq_len = n;
-		}
-		/* If channel list is a repeating subsequence, need a whole
-		 * number of repeats. */
-		if ((n % subseq_len) != 0)
-			errors |= seq_err;
+	/* Step 5: check channel list */

-		if ((devpriv->hwver > 0) && (devpriv->hwver < 4)) {
-			/*
-			 * Buggy PCI230+ or PCI260+ requires channel 0 to be
-			 * (first) in the sequence if the sequence contains
-			 * more than one channel.  Hardware versions 1 and 2
-			 * have the bug.  There is no hardware version 3.
-			 *
-			 * Actually, there are two firmwares that report
-			 * themselves as hardware version 1 (the boards
-			 * have different ADC chips with slightly different
-			 * timing requirements, which was supposed to be
-			 * invisible to software).  The first one doesn't
-			 * seem to have the bug, but the second one
-			 * does, and we can't tell them apart!
-			 */
-			if ((subseq_len > 1)
-			    && (CR_CHAN(cmd->chanlist[0]) != 0)) {
-				errors |= buggy_chan0_err;
-			}
-		}
-		if (errors != 0) {
-			err++;
-			if ((errors & seq_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: channel numbers must increase or sequence must repeat exactly\n",
-					__func__);
-			}
-			if ((errors & rangepair_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: single-ended channel pairs must have the same range\n",
-					__func__);
-			}
-			if ((errors & polarity_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: channel sequence ranges must be all bipolar or all unipolar\n",
-					__func__);
-			}
-			if ((errors & aref_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: channel sequence analogue references must be all the same (single-ended or differential)\n",
-					__func__);
-			}
-			if ((errors & diffchan_err) != 0) {
-				dev_dbg(dev->class_dev,
-					"%s: differential channel number out of range 0 to %u\n",
-					__func__, (s->n_chan / 2) - 1);
-			}
-			if ((errors & buggy_chan0_err) != 0) {
-				dev_info(dev->class_dev,
-					 "amplc_pci230: ai_cmdtest: Buggy PCI230+/260+ h/w version %u requires first channel of multi-channel sequence to be 0 (corrected in h/w version 4)\n",
-					 devpriv->hwver);
-			}
-		}
-	}
+	err |= pci230_ai_check_chanlist(dev, s, cmd);

  	if (err)
  		return 5;


Ditto. pci230_ai_check_chanlist() should only be called if cmd->chanlist is non-NULL and cmd->chanlist_len is non-0. But if chanlist is forced to be NULL by __comedi_get_user_chanlist() when chanlist_len is 0, it would be sufficient to check that chanlist is non-NULL before calling pci230_ai_check_chanlist().

--
-=( 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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux