[PATCH 0/6] staging: comedi: usbduxsigma: fix some problems in command handling

[PATCH 0/6] staging: comedi: usbduxsigma: fix some problems in command handling

[Ian Abbott]

Fix some minor problems in the testing of asynchronous commands for the AI
and AO subdevices and remove some redundant code.

The main problem is that the testing of a new command can affect the
operation of an already running command, which it isn't supposed to do.  (In
practice, applications don't tend to test new commands while a command is
running on the same subdevice, so the bug can be classed as minor.)  This is
corrected by the patches 1 and 2, for the AI and AO subdevices,
respectively.

1) staging: comedi: usbduxsigma: don't clobber ai_timer in command test
2) staging: comedi: usbduxsigma: don't clobber ao_timer in command test
3) staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW
4) staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.
5) staging: comedi: usbduxsigma: remove unused "convert" timing for AO
6) staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.

 drivers/staging/comedi/drivers/usbduxsigma.c | 139 +++++++++++----------------
 1 file changed, 54 insertions(+), 85 deletions(-)

[PATCH 0/6] staging: comedi: usbduxsigma: fix some problems in command handling

[Ian Abbott]

Fix some minor problems in the testing of asynchronous commands for the AI
and AO subdevices and remove some redundant code.

The main problem is that the testing of a new command can affect the
operation of an already running command, which it isn't supposed to do.  (In
practice, applications don't tend to test new commands while a command is
running on the same subdevice, so the bug can be classed as minor.)  This is
corrected by the patches 1 and 2, for the AI and AO subdevices,
respectively.

1) staging: comedi: usbduxsigma: don't clobber ai_timer in command test
2) staging: comedi: usbduxsigma: don't clobber ao_timer in command test
3) staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW
4) staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.
5) staging: comedi: usbduxsigma: remove unused "convert" timing for AO
6) staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.

 drivers/staging/comedi/drivers/usbduxsigma.c | 139 +++++++++++----------------
 1 file changed, 54 insertions(+), 85 deletions(-)

[PATCH 1/6] staging: comedi: usbduxsigma: don't clobber ai_timer in command test

[Ian Abbott]

`devpriv->ai_timer` is used while an asynchronous command is running on
the AI subdevice.  It also gets modified by the subdevice's `cmdtest`
handler for checking new asynchronous commands
(`usbduxsigma_ai_cmdtest()`), which is not correct as it's allowed to
check new commands while an old command is still running.  Fix it by
moving the code which sets up `devpriv->ai_timer` and
`devpriv->ai_interval` into the subdevice's `cmd` handler,
`usbduxsigma_ai_cmd()`.

Note that the removed code in `usbduxsigma_ai_cmdtest()` checked that
`devpriv->ai_timer` did not end up less than than 1, but that could not
happen because `cmd->scan_begin_arg` had already been checked to be at
least the minimum required value (at least when `cmd->scan_begin_src ==
TRIG_TIMER`, which had also been checked to be the case).

Fixes: b986be8527c7 ("staging: comedi: usbduxsigma: tidy up analog input command support)
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: <stable@vger.kernel.org> # 3.19 onwards
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 37 ++++++++++++----------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index eaa9add..22517de 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -550,27 +550,6 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
-	/* Step 4: fix up any arguments */
-
-	if (high_speed) {
-		/*
-		 * every 2 channels get a time window of 125us. Thus, if we
-		 * sample all 16 channels we need 1ms. If we sample only one
-		 * channel we need only 125us
-		 */
-		devpriv->ai_interval = interval;
-		devpriv->ai_timer = cmd->scan_begin_arg / (125000 * interval);
-	} else {
-		/* interval always 1ms */
-		devpriv->ai_interval = 1;
-		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
-	}
-	if (devpriv->ai_timer < 1)
-		err |= -EINVAL;
-
-	if (err)
-		return 4;
-
 	return 0;
 }
 
@@ -668,6 +647,22 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
 
 	down(&devpriv->sem);
 
+	if (devpriv->high_speed) {
+		/*
+		 * every 2 channels get a time window of 125us. Thus, if we
+		 * sample all 16 channels we need 1ms. If we sample only one
+		 * channel we need only 125us
+		 */
+		unsigned int interval = usbduxsigma_chans_to_interval(len);
+
+		devpriv->ai_interval = interval;
+		devpriv->ai_timer = cmd->scan_begin_arg / (125000 * interval);
+	} else {
+		/* interval always 1ms */
+		devpriv->ai_interval = 1;
+		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
+	}
+
 	for (i = 0; i < len; i++) {
 		unsigned int chan  = CR_CHAN(cmd->chanlist[i]);
 
-- 
2.1.4

[PATCH 1/6] staging: comedi: usbduxsigma: don't clobber ai_timer in command test

[Ian Abbott]

`devpriv->ai_timer` is used while an asynchronous command is running on
the AI subdevice.  It also gets modified by the subdevice's `cmdtest`
handler for checking new asynchronous commands
(`usbduxsigma_ai_cmdtest()`), which is not correct as it's allowed to
check new commands while an old command is still running.  Fix it by
moving the code which sets up `devpriv->ai_timer` and
`devpriv->ai_interval` into the subdevice's `cmd` handler,
`usbduxsigma_ai_cmd()`.

Note that the removed code in `usbduxsigma_ai_cmdtest()` checked that
`devpriv->ai_timer` did not end up less than than 1, but that could not
happen because `cmd->scan_begin_arg` had already been checked to be at
least the minimum required value (at least when `cmd->scan_begin_src ==
TRIG_TIMER`, which had also been checked to be the case).

Fixes: b986be8527c7 ("staging: comedi: usbduxsigma: tidy up analog input command support)
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Bernd Porr <mail@berndporr.me.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: <stable@vger.kernel.org> # 3.19 onwards
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 37 ++++++++++++----------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index eaa9add..22517de 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -550,27 +550,6 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
-	/* Step 4: fix up any arguments */
-
-	if (high_speed) {
-		/*
-		 * every 2 channels get a time window of 125us. Thus, if we
-		 * sample all 16 channels we need 1ms. If we sample only one
-		 * channel we need only 125us
-		 */
-		devpriv->ai_interval = interval;
-		devpriv->ai_timer = cmd->scan_begin_arg / (125000 * interval);
-	} else {
-		/* interval always 1ms */
-		devpriv->ai_interval = 1;
-		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
-	}
-	if (devpriv->ai_timer < 1)
-		err |= -EINVAL;
-
-	if (err)
-		return 4;
-
 	return 0;
 }
 
@@ -668,6 +647,22 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
 
 	down(&devpriv->sem);
 
+	if (devpriv->high_speed) {
+		/*
+		 * every 2 channels get a time window of 125us. Thus, if we
+		 * sample all 16 channels we need 1ms. If we sample only one
+		 * channel we need only 125us
+		 */
+		unsigned int interval = usbduxsigma_chans_to_interval(len);
+
+		devpriv->ai_interval = interval;
+		devpriv->ai_timer = cmd->scan_begin_arg / (125000 * interval);
+	} else {
+		/* interval always 1ms */
+		devpriv->ai_interval = 1;
+		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
+	}
+
 	for (i = 0; i < len; i++) {
 		unsigned int chan  = CR_CHAN(cmd->chanlist[i]);
 
-- 
2.1.4

[PATCH 2/6] staging: comedi: usbduxsigma: don't clobber ao_timer in command test

[Ian Abbott]

`devpriv->ao_timer` is used while an asynchronous command is running on
the AO subdevice.  It also gets modified by the subdevice's `cmdtest`
handler for checking new asynchronous commands,
`usbduxsigma_ao_cmdtest()`, which is not correct as it's allowed to
check new commands while an old command is still running.  Fix it by
moving the code which sets up `devpriv->ao_timer` into the subdevice's
`cmd` handler, `usbduxsigma_ao_cmd()`.

Note that the removed code in `usbduxsigma_ao_cmdtest()` checked that
`devpriv->ao_timer` did not end up less that 1, but that could not
happen due because `cmd->scan_begin_arg` or `cmd->convert_arg` had
already been range-checked.

Also note that we tested the `high_speed` variable in the old code, but
that is currently always 0 and means that we always use "scan" timing
(`cmd->scan_begin_src == TRIG_TIMER` and `cmd->convert_src == TRIG_NOW`)
and never "convert" (individual sample) timing (`cmd->scan_begin_src ==
TRIG_FOLLOW` and `cmd->convert_src == TRIG_TIMER`).  The moved code
tests `cmd->convert_src` instead to decide whether "scan" or "convert"
timing is being used, although currently only "scan" timing is
supported.

Fixes: fb1ef622e7a3 ("staging: comedi: usbduxsigma: tidy up analog output command support")
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: <stable@vger.kernel.org> # 3.19 onwards
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 33 ++++++++++++----------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 22517de..dc0b25a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -912,25 +912,6 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
-	/* Step 4: fix up any arguments */
-
-	/* we count in timer steps */
-	if (high_speed) {
-		/* timing of the conversion itself: every 125 us */
-		devpriv->ao_timer = cmd->convert_arg / 125000;
-	} else {
-		/*
-		 * timing of the scan: every 1ms
-		 * we get all channels at once
-		 */
-		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
-	}
-	if (devpriv->ao_timer < 1)
-		err |= -EINVAL;
-
-	if (err)
-		return 4;
-
 	return 0;
 }
 
@@ -943,6 +924,20 @@ static int usbduxsigma_ao_cmd(struct comedi_device *dev,
 
 	down(&devpriv->sem);
 
+	if (cmd->convert_src == TRIG_TIMER) {
+		/*
+		 * timing of the conversion itself: every 125 us
+		 * at high speed (not used yet)
+		 */
+		devpriv->ao_timer = cmd->convert_arg / 125000;
+	} else {
+		/*
+		 * timing of the scan: every 1ms
+		 * we get all channels at once
+		 */
+		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
+	}
+
 	devpriv->ao_counter = devpriv->ao_timer;
 
 	if (cmd->start_src == TRIG_NOW) {
-- 
2.1.4

[PATCH 2/6] staging: comedi: usbduxsigma: don't clobber ao_timer in command test

[Ian Abbott]

`devpriv->ao_timer` is used while an asynchronous command is running on
the AO subdevice.  It also gets modified by the subdevice's `cmdtest`
handler for checking new asynchronous commands,
`usbduxsigma_ao_cmdtest()`, which is not correct as it's allowed to
check new commands while an old command is still running.  Fix it by
moving the code which sets up `devpriv->ao_timer` into the subdevice's
`cmd` handler, `usbduxsigma_ao_cmd()`.

Note that the removed code in `usbduxsigma_ao_cmdtest()` checked that
`devpriv->ao_timer` did not end up less that 1, but that could not
happen due because `cmd->scan_begin_arg` or `cmd->convert_arg` had
already been range-checked.

Also note that we tested the `high_speed` variable in the old code, but
that is currently always 0 and means that we always use "scan" timing
(`cmd->scan_begin_src == TRIG_TIMER` and `cmd->convert_src == TRIG_NOW`)
and never "convert" (individual sample) timing (`cmd->scan_begin_src ==
TRIG_FOLLOW` and `cmd->convert_src == TRIG_TIMER`).  The moved code
tests `cmd->convert_src` instead to decide whether "scan" or "convert"
timing is being used, although currently only "scan" timing is
supported.

Fixes: fb1ef622e7a3 ("staging: comedi: usbduxsigma: tidy up analog output command support")
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Bernd Porr <mail@berndporr.me.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: <stable@vger.kernel.org> # 3.19 onwards
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 33 ++++++++++++----------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 22517de..dc0b25a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -912,25 +912,6 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
-	/* Step 4: fix up any arguments */
-
-	/* we count in timer steps */
-	if (high_speed) {
-		/* timing of the conversion itself: every 125 us */
-		devpriv->ao_timer = cmd->convert_arg / 125000;
-	} else {
-		/*
-		 * timing of the scan: every 1ms
-		 * we get all channels at once
-		 */
-		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
-	}
-	if (devpriv->ao_timer < 1)
-		err |= -EINVAL;
-
-	if (err)
-		return 4;
-
 	return 0;
 }
 
@@ -943,6 +924,20 @@ static int usbduxsigma_ao_cmd(struct comedi_device *dev,
 
 	down(&devpriv->sem);
 
+	if (cmd->convert_src == TRIG_TIMER) {
+		/*
+		 * timing of the conversion itself: every 125 us
+		 * at high speed (not used yet)
+		 */
+		devpriv->ao_timer = cmd->convert_arg / 125000;
+	} else {
+		/*
+		 * timing of the scan: every 1ms
+		 * we get all channels at once
+		 */
+		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
+	}
+
 	devpriv->ao_counter = devpriv->ao_timer;
 
 	if (cmd->start_src == TRIG_NOW) {
-- 
2.1.4

[PATCH 3/6] staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW

[Ian Abbott]

The AI subdevice `cmdtest` handler `usbduxsigma_ai_cmdtest()` ensures
that `cmd->scan_begin_src == TRIG_TIMER` by the end of step 2 of the
command checking code, so assume that this is the case for step 3
onwards and remove the redundant code.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 47 +++++++++++-----------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index dc0b25a..65a0df4 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -481,6 +481,7 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 	struct usbduxsigma_private *devpriv = dev->private;
 	int high_speed = devpriv->high_speed;
 	int interval = usbduxsigma_chans_to_interval(cmd->chanlist_len);
+	unsigned int tmp;
 	int err = 0;
 
 	/* Step 1 : check if triggers are trivially valid */
@@ -508,36 +509,26 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 
 	err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
 
-	if (cmd->scan_begin_src == TRIG_FOLLOW)	/* internal trigger */
-		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
+	if (high_speed) {
+		/*
+		 * In high speed mode microframes are possible.
+		 * However, during one microframe we can roughly
+		 * sample two channels. Thus, the more channels
+		 * are in the channel list the more time we need.
+		 */
+		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
+						    (125000 * interval));
 
-	if (cmd->scan_begin_src == TRIG_TIMER) {
-		unsigned int tmp;
-
-		if (high_speed) {
-			/*
-			 * In high speed mode microframes are possible.
-			 * However, during one microframe we can roughly
-			 * sample two channels. Thus, the more channels
-			 * are in the channel list the more time we need.
-			 */
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
-							    (1000000 / 8 *
-							     interval));
-
-			tmp = (cmd->scan_begin_arg / 125000) * 125000;
-		} else {
-			/* full speed */
-			/* 1kHz scans every USB frame */
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
-							    1000000);
-
-			tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
-		}
-		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
+		tmp = (cmd->scan_begin_arg / 125000) * 125000;
+	} else {
+		/* full speed */
+		/* 1kHz scans every USB frame */
+		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
+						    1000000);
+
+		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
 	}
+	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
 
 	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
 					   cmd->chanlist_len);
-- 
2.1.4

[PATCH 3/6] staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW

[Ian Abbott]

The AI subdevice `cmdtest` handler `usbduxsigma_ai_cmdtest()` ensures
that `cmd->scan_begin_src == TRIG_TIMER` by the end of step 2 of the
command checking code, so assume that this is the case for step 3
onwards and remove the redundant code.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Bernd Porr <mail@berndporr.me.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 47 +++++++++++-----------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index dc0b25a..65a0df4 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -481,6 +481,7 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 	struct usbduxsigma_private *devpriv = dev->private;
 	int high_speed = devpriv->high_speed;
 	int interval = usbduxsigma_chans_to_interval(cmd->chanlist_len);
+	unsigned int tmp;
 	int err = 0;
 
 	/* Step 1 : check if triggers are trivially valid */
@@ -508,36 +509,26 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 
 	err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
 
-	if (cmd->scan_begin_src == TRIG_FOLLOW)	/* internal trigger */
-		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
+	if (high_speed) {
+		/*
+		 * In high speed mode microframes are possible.
+		 * However, during one microframe we can roughly
+		 * sample two channels. Thus, the more channels
+		 * are in the channel list the more time we need.
+		 */
+		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
+						    (125000 * interval));
 
-	if (cmd->scan_begin_src == TRIG_TIMER) {
-		unsigned int tmp;
-
-		if (high_speed) {
-			/*
-			 * In high speed mode microframes are possible.
-			 * However, during one microframe we can roughly
-			 * sample two channels. Thus, the more channels
-			 * are in the channel list the more time we need.
-			 */
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
-							    (1000000 / 8 *
-							     interval));
-
-			tmp = (cmd->scan_begin_arg / 125000) * 125000;
-		} else {
-			/* full speed */
-			/* 1kHz scans every USB frame */
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
-							    1000000);
-
-			tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
-		}
-		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
+		tmp = (cmd->scan_begin_arg / 125000) * 125000;
+	} else {
+		/* full speed */
+		/* 1kHz scans every USB frame */
+		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
+						    1000000);
+
+		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
 	}
+	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
 
 	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
 					   cmd->chanlist_len);
-- 
2.1.4

[PATCH 4/6] staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.

[Ian Abbott]

The return value of the `cmdtest` handler for a subdevice checks the
prospective new command in various steps and returns the step number at
which any problem was detected, or 0 if no problem was detected.  It is
allowed to modify the command in various ways at each step.  Corrections
for out-of-range values are generally made at step 3, and minor
adjustments such as rounding are generally made at step 4.

The `cmdtest` handler for the AI subdevice (`usbduxsigma_ai_cmdtest()`)
currently modifies `cmd->scan_begin_arg` to bring it into range and
round it down at step 3.  Move the rounding down part to step 4 to
follow the usual Comedi convention.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 65a0df4..4655048 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -518,17 +518,12 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 		 */
 		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
 						    (125000 * interval));
-
-		tmp = (cmd->scan_begin_arg / 125000) * 125000;
 	} else {
 		/* full speed */
 		/* 1kHz scans every USB frame */
 		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
 						    1000000);
-
-		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
 	}
-	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
 
 	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
 					   cmd->chanlist_len);
@@ -541,6 +536,14 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
+	/* Step 4: fix up any arguments */
+
+	tmp = rounddown(cmd->scan_begin_arg, high_speed ? 125000 : 1000000);
+	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
+
+	if (err)
+		return 4;
+
 	return 0;
 }
 
-- 
2.1.4

[PATCH 4/6] staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.

[Ian Abbott]

The return value of the `cmdtest` handler for a subdevice checks the
prospective new command in various steps and returns the step number at
which any problem was detected, or 0 if no problem was detected.  It is
allowed to modify the command in various ways at each step.  Corrections
for out-of-range values are generally made at step 3, and minor
adjustments such as rounding are generally made at step 4.

The `cmdtest` handler for the AI subdevice (`usbduxsigma_ai_cmdtest()`)
currently modifies `cmd->scan_begin_arg` to bring it into range and
round it down at step 3.  Move the rounding down part to step 4 to
follow the usual Comedi convention.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Bernd Porr <mail@berndporr.me.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 65a0df4..4655048 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -518,17 +518,12 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 		 */
 		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
 						    (125000 * interval));
-
-		tmp = (cmd->scan_begin_arg / 125000) * 125000;
 	} else {
 		/* full speed */
 		/* 1kHz scans every USB frame */
 		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
 						    1000000);
-
-		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
 	}
-	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
 
 	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
 					   cmd->chanlist_len);
@@ -541,6 +536,14 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
+	/* Step 4: fix up any arguments */
+
+	tmp = rounddown(cmd->scan_begin_arg, high_speed ? 125000 : 1000000);
+	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
+
+	if (err)
+		return 4;
+
 	return 0;
 }
 
-- 
2.1.4

[PATCH 5/6] staging: comedi: usbduxsigma: remove unused "convert" timing for AO

[Ian Abbott]

The `cmdtest` and `cmd` handlers for the AO subdevice
(`usbduxsigma_ao_cmdtest()` and `usbduxsigma_ao_cmd()`) support "scan"
timing of commands with all channels updated every "scan" period.  There
is some disabled code to use "convert" timing in high speed mode.  That
would allow channels to be updated sequentially every "convert" period.
Since that code is incomplete and currently disabled, remove it to
simplify the existing code.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 58 ++++++++--------------------
 1 file changed, 17 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 4655048..d97253e 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -838,28 +838,20 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 {
 	struct usbduxsigma_private *devpriv = dev->private;
 	int err = 0;
-	int high_speed;
-	unsigned int flags;
-
-	/* high speed conversions are not used yet */
-	high_speed = 0;		/* (devpriv->high_speed) */
 
 	/* Step 1 : check if triggers are trivially valid */
 
 	err |= comedi_check_trigger_src(&cmd->start_src, TRIG_NOW | TRIG_INT);
 
-	if (high_speed) {
-		/*
-		 * start immediately a new scan
-		 * the sampling rate is set by the coversion rate
-		 */
-		flags = TRIG_FOLLOW;
-	} else {
-		/* start a new scan (output at once) with a timer */
-		flags = TRIG_TIMER;
-	}
-	err |= comedi_check_trigger_src(&cmd->scan_begin_src, flags);
-
+	/*
+	 * For now, always use "scan" timing with all channels updated at once
+	 * (cmd->scan_begin_src == TRIG_TIMER, cmd->convert_src == TRIG_NOW).
+	 *
+	 * In a future version, "convert" timing with channels updated
+	 * indivually may be supported in high speed mode
+	 * (cmd->scan_begin_src == TRIG_FOLLOW, cmd->convert_src == TRIG_TIMER).
+	 */
+	err |= comedi_check_trigger_src(&cmd->scan_begin_src, TRIG_TIMER);
 	err |= comedi_check_trigger_src(&cmd->convert_src, TRIG_NOW);
 	err |= comedi_check_trigger_src(&cmd->scan_end_src, TRIG_COUNT);
 	err |= comedi_check_trigger_src(&cmd->stop_src, TRIG_COUNT | TRIG_NONE);
@@ -883,17 +875,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 
 	err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
 
-	if (cmd->scan_begin_src == TRIG_FOLLOW)	/* internal trigger */
-		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
-
-	if (cmd->scan_begin_src == TRIG_TIMER) {
-		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
-						    1000000);
-	}
-
-	/* not used now, is for later use */
-	if (cmd->convert_src == TRIG_TIMER)
-		err |= comedi_check_trigger_arg_min(&cmd->convert_arg, 125000);
+	err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg, 1000000);
 
 	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
 					   cmd->chanlist_len);
@@ -918,19 +900,13 @@ static int usbduxsigma_ao_cmd(struct comedi_device *dev,
 
 	down(&devpriv->sem);
 
-	if (cmd->convert_src == TRIG_TIMER) {
-		/*
-		 * timing of the conversion itself: every 125 us
-		 * at high speed (not used yet)
-		 */
-		devpriv->ao_timer = cmd->convert_arg / 125000;
-	} else {
-		/*
-		 * timing of the scan: every 1ms
-		 * we get all channels at once
-		 */
-		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
-	}
+	/*
+	 * For now, only "scan" timing is supported.  A future version may
+	 * support "convert" timing in high speed mode.
+	 *
+	 * Timing of the scan: every 1ms all channels updated at once.
+	 */
+	devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
 
 	devpriv->ao_counter = devpriv->ao_timer;
 
-- 
2.1.4

[PATCH 5/6] staging: comedi: usbduxsigma: remove unused "convert" timing for AO

[Ian Abbott]

The `cmdtest` and `cmd` handlers for the AO subdevice
(`usbduxsigma_ao_cmdtest()` and `usbduxsigma_ao_cmd()`) support "scan"
timing of commands with all channels updated every "scan" period.  There
is some disabled code to use "convert" timing in high speed mode.  That
would allow channels to be updated sequentially every "convert" period.
Since that code is incomplete and currently disabled, remove it to
simplify the existing code.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Bernd Porr <mail@berndporr.me.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 58 ++++++++--------------------
 1 file changed, 17 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 4655048..d97253e 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -838,28 +838,20 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 {
 	struct usbduxsigma_private *devpriv = dev->private;
 	int err = 0;
-	int high_speed;
-	unsigned int flags;
-
-	/* high speed conversions are not used yet */
-	high_speed = 0;		/* (devpriv->high_speed) */
 
 	/* Step 1 : check if triggers are trivially valid */
 
 	err |= comedi_check_trigger_src(&cmd->start_src, TRIG_NOW | TRIG_INT);
 
-	if (high_speed) {
-		/*
-		 * start immediately a new scan
-		 * the sampling rate is set by the coversion rate
-		 */
-		flags = TRIG_FOLLOW;
-	} else {
-		/* start a new scan (output at once) with a timer */
-		flags = TRIG_TIMER;
-	}
-	err |= comedi_check_trigger_src(&cmd->scan_begin_src, flags);
-
+	/*
+	 * For now, always use "scan" timing with all channels updated at once
+	 * (cmd->scan_begin_src == TRIG_TIMER, cmd->convert_src == TRIG_NOW).
+	 *
+	 * In a future version, "convert" timing with channels updated
+	 * indivually may be supported in high speed mode
+	 * (cmd->scan_begin_src == TRIG_FOLLOW, cmd->convert_src == TRIG_TIMER).
+	 */
+	err |= comedi_check_trigger_src(&cmd->scan_begin_src, TRIG_TIMER);
 	err |= comedi_check_trigger_src(&cmd->convert_src, TRIG_NOW);
 	err |= comedi_check_trigger_src(&cmd->scan_end_src, TRIG_COUNT);
 	err |= comedi_check_trigger_src(&cmd->stop_src, TRIG_COUNT | TRIG_NONE);
@@ -883,17 +875,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 
 	err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
 
-	if (cmd->scan_begin_src == TRIG_FOLLOW)	/* internal trigger */
-		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
-
-	if (cmd->scan_begin_src == TRIG_TIMER) {
-		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
-						    1000000);
-	}
-
-	/* not used now, is for later use */
-	if (cmd->convert_src == TRIG_TIMER)
-		err |= comedi_check_trigger_arg_min(&cmd->convert_arg, 125000);
+	err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg, 1000000);
 
 	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
 					   cmd->chanlist_len);
@@ -918,19 +900,13 @@ static int usbduxsigma_ao_cmd(struct comedi_device *dev,
 
 	down(&devpriv->sem);
 
-	if (cmd->convert_src == TRIG_TIMER) {
-		/*
-		 * timing of the conversion itself: every 125 us
-		 * at high speed (not used yet)
-		 */
-		devpriv->ao_timer = cmd->convert_arg / 125000;
-	} else {
-		/*
-		 * timing of the scan: every 1ms
-		 * we get all channels at once
-		 */
-		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
-	}
+	/*
+	 * For now, only "scan" timing is supported.  A future version may
+	 * support "convert" timing in high speed mode.
+	 *
+	 * Timing of the scan: every 1ms all channels updated at once.
+	 */
+	devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
 
 	devpriv->ao_counter = devpriv->ao_timer;
 
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 6/6] staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.

[Ian Abbott]

The return value of the `cmdtest` handler for a subdevice checks the
prospective new command in various steps and returns the step number at
which any problem was detected, or 0 if no problem was detected.  It is
allowed to modify the command in various ways at each step.  Corrections
for out-of-range values are generally made at step 3, and minor
adjustments such as rounding are generally made at step 4.

The `cmdtest` handler for the AO subdevice (`usbduxsigma_ao_cmdtest()`)
currently range checks the timings at step 3.  Since the running command
will round down the timings, add code to round them down at step 4.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index d97253e..e22c374 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -837,6 +837,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 				  struct comedi_cmd *cmd)
 {
 	struct usbduxsigma_private *devpriv = dev->private;
+	unsigned int tmp;
 	int err = 0;
 
 	/* Step 1 : check if triggers are trivially valid */
@@ -888,6 +889,14 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
+	/* Step 4: fix up any arguments */
+
+	tmp = rounddown(cmd->scan_begin_arg, 1000000);
+	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
+
+	if (err)
+		return 4;
+
 	return 0;
 }
 
-- 
2.1.4

[PATCH 6/6] staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.

[Ian Abbott]

The return value of the `cmdtest` handler for a subdevice checks the
prospective new command in various steps and returns the step number at
which any problem was detected, or 0 if no problem was detected.  It is
allowed to modify the command in various ways at each step.  Corrections
for out-of-range values are generally made at step 3, and minor
adjustments such as rounding are generally made at step 4.

The `cmdtest` handler for the AO subdevice (`usbduxsigma_ao_cmdtest()`)
currently range checks the timings at step 3.  Since the running command
will round down the timings, add code to round them down at step 4.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Bernd Porr <mail@berndporr.me.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/usbduxsigma.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index d97253e..e22c374 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -837,6 +837,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 				  struct comedi_cmd *cmd)
 {
 	struct usbduxsigma_private *devpriv = dev->private;
+	unsigned int tmp;
 	int err = 0;
 
 	/* Step 1 : check if triggers are trivially valid */
@@ -888,6 +889,14 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
 	if (err)
 		return 3;
 
+	/* Step 4: fix up any arguments */
+
+	tmp = rounddown(cmd->scan_begin_arg, 1000000);
+	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
+
+	if (err)
+		return 4;
+
 	return 0;
 }
 
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: comedi: usbduxsigma: don't clobber ai_timer in command test

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> `devpriv->ai_timer` is used while an asynchronous command is running on
> the AI subdevice.  It also gets modified by the subdevice's `cmdtest`
> handler for checking new asynchronous commands
> (`usbduxsigma_ai_cmdtest()`), which is not correct as it's allowed to
> check new commands while an old command is still running.  Fix it by
> moving the code which sets up `devpriv->ai_timer` and
> `devpriv->ai_interval` into the subdevice's `cmd` handler,
> `usbduxsigma_ai_cmd()`.
> 
> Note that the removed code in `usbduxsigma_ai_cmdtest()` checked that
> `devpriv->ai_timer` did not end up less than than 1, but that could not
> happen because `cmd->scan_begin_arg` had already been checked to be at
> least the minimum required value (at least when `cmd->scan_begin_src ==
> TRIG_TIMER`, which had also been checked to be the case).
> 
> Fixes: b986be8527c7 ("staging: comedi: usbduxsigma: tidy up analog input command support)
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> Cc: <stable@vger.kernel.org> # 3.19 onwards
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 37 ++++++++++++----------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index eaa9add..22517de 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -550,27 +550,6 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  	if (err)
>  		return 3;
>  
> -	/* Step 4: fix up any arguments */
> -
> -	if (high_speed) {
> -		/*
> -		 * every 2 channels get a time window of 125us. Thus, if we
> -		 * sample all 16 channels we need 1ms. If we sample only one
> -		 * channel we need only 125us
> -		 */
> -		devpriv->ai_interval = interval;
> -		devpriv->ai_timer = cmd->scan_begin_arg / (125000 * interval);
> -	} else {
> -		/* interval always 1ms */
> -		devpriv->ai_interval = 1;
> -		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
> -	}
> -	if (devpriv->ai_timer < 1)
> -		err |= -EINVAL;
> -
> -	if (err)
> -		return 4;
> -
>  	return 0;
>  }
>  
> @@ -668,6 +647,22 @@ static int usbduxsigma_ai_cmd(struct comedi_device *dev,
>  
>  	down(&devpriv->sem);
>  
> +	if (devpriv->high_speed) {
> +		/*
> +		 * every 2 channels get a time window of 125us. Thus, if we
> +		 * sample all 16 channels we need 1ms. If we sample only one
> +		 * channel we need only 125us
> +		 */
> +		unsigned int interval = usbduxsigma_chans_to_interval(len);
> +
> +		devpriv->ai_interval = interval;
> +		devpriv->ai_timer = cmd->scan_begin_arg / (125000 * interval);
> +	} else {
> +		/* interval always 1ms */
> +		devpriv->ai_interval = 1;
> +		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
> +	}
> +
>  	for (i = 0; i < len; i++) {
>  		unsigned int chan  = CR_CHAN(cmd->chanlist[i]);
>  

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069

Re: [PATCH 2/6] staging: comedi: usbduxsigma: don't clobber ao_timer in command test

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> `devpriv->ao_timer` is used while an asynchronous command is running on
> the AO subdevice.  It also gets modified by the subdevice's `cmdtest`
> handler for checking new asynchronous commands,
> `usbduxsigma_ao_cmdtest()`, which is not correct as it's allowed to
> check new commands while an old command is still running.  Fix it by
> moving the code which sets up `devpriv->ao_timer` into the subdevice's
> `cmd` handler, `usbduxsigma_ao_cmd()`.
> 
> Note that the removed code in `usbduxsigma_ao_cmdtest()` checked that
> `devpriv->ao_timer` did not end up less that 1, but that could not
> happen due because `cmd->scan_begin_arg` or `cmd->convert_arg` had
> already been range-checked.
> 
> Also note that we tested the `high_speed` variable in the old code, but
> that is currently always 0 and means that we always use "scan" timing
> (`cmd->scan_begin_src == TRIG_TIMER` and `cmd->convert_src == TRIG_NOW`)
> and never "convert" (individual sample) timing (`cmd->scan_begin_src ==
> TRIG_FOLLOW` and `cmd->convert_src == TRIG_TIMER`).  The moved code
> tests `cmd->convert_src` instead to decide whether "scan" or "convert"
> timing is being used, although currently only "scan" timing is
> supported.
> 
> Fixes: fb1ef622e7a3 ("staging: comedi: usbduxsigma: tidy up analog output command support")
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> Cc: <stable@vger.kernel.org> # 3.19 onwards
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 33 ++++++++++++----------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index 22517de..dc0b25a 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -912,25 +912,6 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  	if (err)
>  		return 3;
>  
> -	/* Step 4: fix up any arguments */
> -
> -	/* we count in timer steps */
> -	if (high_speed) {
> -		/* timing of the conversion itself: every 125 us */
> -		devpriv->ao_timer = cmd->convert_arg / 125000;
> -	} else {
> -		/*
> -		 * timing of the scan: every 1ms
> -		 * we get all channels at once
> -		 */
> -		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
> -	}
> -	if (devpriv->ao_timer < 1)
> -		err |= -EINVAL;
> -
> -	if (err)
> -		return 4;
> -
>  	return 0;
>  }
>  
> @@ -943,6 +924,20 @@ static int usbduxsigma_ao_cmd(struct comedi_device *dev,
>  
>  	down(&devpriv->sem);
>  
> +	if (cmd->convert_src == TRIG_TIMER) {
> +		/*
> +		 * timing of the conversion itself: every 125 us
> +		 * at high speed (not used yet)
> +		 */
> +		devpriv->ao_timer = cmd->convert_arg / 125000;
> +	} else {
> +		/*
> +		 * timing of the scan: every 1ms
> +		 * we get all channels at once
> +		 */
> +		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
> +	}
> +
>  	devpriv->ao_counter = devpriv->ao_timer;
>  
>  	if (cmd->start_src == TRIG_NOW) {

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069

Re: [PATCH 3/6] staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> The AI subdevice `cmdtest` handler `usbduxsigma_ai_cmdtest()` ensures
> that `cmd->scan_begin_src == TRIG_TIMER` by the end of step 2 of the
> command checking code, so assume that this is the case for step 3
> onwards and remove the redundant code.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 47 +++++++++++-----------------
>  1 file changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index dc0b25a..65a0df4 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -481,6 +481,7 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  	struct usbduxsigma_private *devpriv = dev->private;
>  	int high_speed = devpriv->high_speed;
>  	int interval = usbduxsigma_chans_to_interval(cmd->chanlist_len);
> +	unsigned int tmp;
>  	int err = 0;
>  
>  	/* Step 1 : check if triggers are trivially valid */
> @@ -508,36 +509,26 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  
>  	err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
>  
> -	if (cmd->scan_begin_src == TRIG_FOLLOW)	/* internal trigger */
> -		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
> +	if (high_speed) {
> +		/*
> +		 * In high speed mode microframes are possible.
> +		 * However, during one microframe we can roughly
> +		 * sample two channels. Thus, the more channels
> +		 * are in the channel list the more time we need.
> +		 */
> +		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
> +						    (125000 * interval));
>  
> -	if (cmd->scan_begin_src == TRIG_TIMER) {
> -		unsigned int tmp;
> -
> -		if (high_speed) {
> -			/*
> -			 * In high speed mode microframes are possible.
> -			 * However, during one microframe we can roughly
> -			 * sample two channels. Thus, the more channels
> -			 * are in the channel list the more time we need.
> -			 */
> -			err |= comedi_check_trigger_arg_min(&cmd->
> -							    scan_begin_arg,
> -							    (1000000 / 8 *
> -							     interval));
> -
> -			tmp = (cmd->scan_begin_arg / 125000) * 125000;
> -		} else {
> -			/* full speed */
> -			/* 1kHz scans every USB frame */
> -			err |= comedi_check_trigger_arg_min(&cmd->
> -							    scan_begin_arg,
> -							    1000000);
> -
> -			tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
> -		}
> -		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
> +		tmp = (cmd->scan_begin_arg / 125000) * 125000;
> +	} else {
> +		/* full speed */
> +		/* 1kHz scans every USB frame */
> +		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
> +						    1000000);
> +
> +		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
>  	}
> +	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
>  
>  	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
>  					   cmd->chanlist_len);

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069

Re: [PATCH 4/6] staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> The return value of the `cmdtest` handler for a subdevice checks the
> prospective new command in various steps and returns the step number at
> which any problem was detected, or 0 if no problem was detected.  It is
> allowed to modify the command in various ways at each step.  Corrections
> for out-of-range values are generally made at step 3, and minor
> adjustments such as rounding are generally made at step 4.
> 
> The `cmdtest` handler for the AI subdevice (`usbduxsigma_ai_cmdtest()`)
> currently modifies `cmd->scan_begin_arg` to bring it into range and
> round it down at step 3.  Move the rounding down part to step 4 to
> follow the usual Comedi convention.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index 65a0df4..4655048 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -518,17 +518,12 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  		 */
>  		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
>  						    (125000 * interval));
> -
> -		tmp = (cmd->scan_begin_arg / 125000) * 125000;
>  	} else {
>  		/* full speed */
>  		/* 1kHz scans every USB frame */
>  		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
>  						    1000000);
> -
> -		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
>  	}
> -	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
>  
>  	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
>  					   cmd->chanlist_len);
> @@ -541,6 +536,14 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  	if (err)
>  		return 3;
>  
> +	/* Step 4: fix up any arguments */
> +
> +	tmp = rounddown(cmd->scan_begin_arg, high_speed ? 125000 : 1000000);
> +	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
> +
> +	if (err)
> +		return 4;
> +
>  	return 0;
>  }
>  

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069

Re: [PATCH 4/6] staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> The return value of the `cmdtest` handler for a subdevice checks the
> prospective new command in various steps and returns the step number at
> which any problem was detected, or 0 if no problem was detected.  It is
> allowed to modify the command in various ways at each step.  Corrections
> for out-of-range values are generally made at step 3, and minor
> adjustments such as rounding are generally made at step 4.
> 
> The `cmdtest` handler for the AI subdevice (`usbduxsigma_ai_cmdtest()`)
> currently modifies `cmd->scan_begin_arg` to bring it into range and
> round it down at step 3.  Move the rounding down part to step 4 to
> follow the usual Comedi convention.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index 65a0df4..4655048 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -518,17 +518,12 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  		 */
>  		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
>  						    (125000 * interval));
> -
> -		tmp = (cmd->scan_begin_arg / 125000) * 125000;
>  	} else {
>  		/* full speed */
>  		/* 1kHz scans every USB frame */
>  		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
>  						    1000000);
> -
> -		tmp = (cmd->scan_begin_arg / 1000000) * 1000000;
>  	}
> -	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
>  
>  	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
>  					   cmd->chanlist_len);
> @@ -541,6 +536,14 @@ static int usbduxsigma_ai_cmdtest(struct comedi_device *dev,
>  	if (err)
>  		return 3;
>  
> +	/* Step 4: fix up any arguments */
> +
> +	tmp = rounddown(cmd->scan_begin_arg, high_speed ? 125000 : 1000000);
> +	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
> +
> +	if (err)
> +		return 4;
> +
>  	return 0;
>  }
>  

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: comedi: usbduxsigma: remove unused "convert" timing for AO

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> The `cmdtest` and `cmd` handlers for the AO subdevice
> (`usbduxsigma_ao_cmdtest()` and `usbduxsigma_ao_cmd()`) support "scan"
> timing of commands with all channels updated every "scan" period.  There
> is some disabled code to use "convert" timing in high speed mode.  That
> would allow channels to be updated sequentially every "convert" period.
> Since that code is incomplete and currently disabled, remove it to
> simplify the existing code.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 58 ++++++++--------------------
>  1 file changed, 17 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index 4655048..d97253e 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -838,28 +838,20 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  {
>  	struct usbduxsigma_private *devpriv = dev->private;
>  	int err = 0;
> -	int high_speed;
> -	unsigned int flags;
> -
> -	/* high speed conversions are not used yet */
> -	high_speed = 0;		/* (devpriv->high_speed) */
>  
>  	/* Step 1 : check if triggers are trivially valid */
>  
>  	err |= comedi_check_trigger_src(&cmd->start_src, TRIG_NOW | TRIG_INT);
>  
> -	if (high_speed) {
> -		/*
> -		 * start immediately a new scan
> -		 * the sampling rate is set by the coversion rate
> -		 */
> -		flags = TRIG_FOLLOW;
> -	} else {
> -		/* start a new scan (output at once) with a timer */
> -		flags = TRIG_TIMER;
> -	}
> -	err |= comedi_check_trigger_src(&cmd->scan_begin_src, flags);
> -
> +	/*
> +	 * For now, always use "scan" timing with all channels updated at once
> +	 * (cmd->scan_begin_src == TRIG_TIMER, cmd->convert_src == TRIG_NOW).
> +	 *
> +	 * In a future version, "convert" timing with channels updated
> +	 * indivually may be supported in high speed mode
> +	 * (cmd->scan_begin_src == TRIG_FOLLOW, cmd->convert_src == TRIG_TIMER).
> +	 */
> +	err |= comedi_check_trigger_src(&cmd->scan_begin_src, TRIG_TIMER);
>  	err |= comedi_check_trigger_src(&cmd->convert_src, TRIG_NOW);
>  	err |= comedi_check_trigger_src(&cmd->scan_end_src, TRIG_COUNT);
>  	err |= comedi_check_trigger_src(&cmd->stop_src, TRIG_COUNT | TRIG_NONE);
> @@ -883,17 +875,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  
>  	err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
>  
> -	if (cmd->scan_begin_src == TRIG_FOLLOW)	/* internal trigger */
> -		err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
> -
> -	if (cmd->scan_begin_src == TRIG_TIMER) {
> -		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
> -						    1000000);
> -	}
> -
> -	/* not used now, is for later use */
> -	if (cmd->convert_src == TRIG_TIMER)
> -		err |= comedi_check_trigger_arg_min(&cmd->convert_arg, 125000);
> +	err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg, 1000000);
>  
>  	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
>  					   cmd->chanlist_len);
> @@ -918,19 +900,13 @@ static int usbduxsigma_ao_cmd(struct comedi_device *dev,
>  
>  	down(&devpriv->sem);
>  
> -	if (cmd->convert_src == TRIG_TIMER) {
> -		/*
> -		 * timing of the conversion itself: every 125 us
> -		 * at high speed (not used yet)
> -		 */
> -		devpriv->ao_timer = cmd->convert_arg / 125000;
> -	} else {
> -		/*
> -		 * timing of the scan: every 1ms
> -		 * we get all channels at once
> -		 */
> -		devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
> -	}
> +	/*
> +	 * For now, only "scan" timing is supported.  A future version may
> +	 * support "convert" timing in high speed mode.
> +	 *
> +	 * Timing of the scan: every 1ms all channels updated at once.
> +	 */
> +	devpriv->ao_timer = cmd->scan_begin_arg / 1000000;
>  
>  	devpriv->ao_counter = devpriv->ao_timer;
>  

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069

Re: [PATCH 6/6] staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> The return value of the `cmdtest` handler for a subdevice checks the
> prospective new command in various steps and returns the step number at
> which any problem was detected, or 0 if no problem was detected.  It is
> allowed to modify the command in various ways at each step.  Corrections
> for out-of-range values are generally made at step 3, and minor
> adjustments such as rounding are generally made at step 4.
> 
> The `cmdtest` handler for the AO subdevice (`usbduxsigma_ao_cmdtest()`)
> currently range checks the timings at step 3.  Since the running command
> will round down the timings, add code to round them down at step 4.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index d97253e..e22c374 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -837,6 +837,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  				  struct comedi_cmd *cmd)
>  {
>  	struct usbduxsigma_private *devpriv = dev->private;
> +	unsigned int tmp;
>  	int err = 0;
>  
>  	/* Step 1 : check if triggers are trivially valid */
> @@ -888,6 +889,14 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  	if (err)
>  		return 3;
>  
> +	/* Step 4: fix up any arguments */
> +
> +	tmp = rounddown(cmd->scan_begin_arg, 1000000);
> +	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
> +
> +	if (err)
> +		return 4;
> +
>  	return 0;
>  }
>  

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069

Re: [PATCH 6/6] staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.

[Bernd Porr]

Reviewed-by: Bernd Porr <mail@berndporr.me.uk>

Ian Abbott wrote:
> The return value of the `cmdtest` handler for a subdevice checks the
> prospective new command in various steps and returns the step number at
> which any problem was detected, or 0 if no problem was detected.  It is
> allowed to modify the command in various ways at each step.  Corrections
> for out-of-range values are generally made at step 3, and minor
> adjustments such as rounding are generally made at step 4.
> 
> The `cmdtest` handler for the AO subdevice (`usbduxsigma_ao_cmdtest()`)
> currently range checks the timings at step 3.  Since the running command
> will round down the timings, add code to round them down at step 4.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/usbduxsigma.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
> index d97253e..e22c374 100644
> --- a/drivers/staging/comedi/drivers/usbduxsigma.c
> +++ b/drivers/staging/comedi/drivers/usbduxsigma.c
> @@ -837,6 +837,7 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  				  struct comedi_cmd *cmd)
>  {
>  	struct usbduxsigma_private *devpriv = dev->private;
> +	unsigned int tmp;
>  	int err = 0;
>  
>  	/* Step 1 : check if triggers are trivially valid */
> @@ -888,6 +889,14 @@ static int usbduxsigma_ao_cmdtest(struct comedi_device *dev,
>  	if (err)
>  		return 3;
>  
> +	/* Step 4: fix up any arguments */
> +
> +	tmp = rounddown(cmd->scan_begin_arg, 1000000);
> +	err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, tmp);
> +
> +	if (err)
> +		return 4;
> +
>  	return 0;
>  }
>  

-- 
http://www.berndporr.me.uk
http://www.linux-usb-daq.co.uk
http://www.imdb.com/name/nm3293421/
+44 (0)7840 340069
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 0/6] staging: comedi: usbduxsigma: fix some problems in command handling

[Hartley Sweeten]

On Thursday, July 23, 2015 8:47 AM, Ian Abbott wrote:
>
> Fix some minor problems in the testing of asynchronous commands for the AI
> and AO subdevices and remove some redundant code.
>
> The main problem is that the testing of a new command can affect the
> operation of an already running command, which it isn't supposed to do.  (In
> practice, applications don't tend to test new commands while a command is
> running on the same subdevice, so the bug can be classed as minor.)  This is
> corrected by the patches 1 and 2, for the AI and AO subdevices,
> respectively.
>
> 1) staging: comedi: usbduxsigma: don't clobber ai_timer in command test
> 2) staging: comedi: usbduxsigma: don't clobber ao_timer in command test
> 3) staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW
> 4) staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.
> 5) staging: comedi: usbduxsigma: remove unused "convert" timing for AO
> 6) staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.
>
>  drivers/staging/comedi/drivers/usbduxsigma.c | 139 +++++++++++----------------
>  1 file changed, 54 insertions(+), 85 deletions(-)

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

RE: [PATCH 0/6] staging: comedi: usbduxsigma: fix some problems in command handling

[Hartley Sweeten]

On Thursday, July 23, 2015 8:47 AM, Ian Abbott wrote:
>
> Fix some minor problems in the testing of asynchronous commands for the AI
> and AO subdevices and remove some redundant code.
>
> The main problem is that the testing of a new command can affect the
> operation of an already running command, which it isn't supposed to do.  (In
> practice, applications don't tend to test new commands while a command is
> running on the same subdevice, so the bug can be classed as minor.)  This is
> corrected by the patches 1 and 2, for the AI and AO subdevices,
> respectively.
>
> 1) staging: comedi: usbduxsigma: don't clobber ai_timer in command test
> 2) staging: comedi: usbduxsigma: don't clobber ao_timer in command test
> 3) staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW
> 4) staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4.
> 5) staging: comedi: usbduxsigma: remove unused "convert" timing for AO
> 6) staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4.
>
>  drivers/staging/comedi/drivers/usbduxsigma.c | 139 +++++++++++----------------
>  1 file changed, 54 insertions(+), 85 deletions(-)

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>