[PATCH 13/22] staging: comedi: adv_pci1710: don't check the chanlist twice

Ian Abbott abbotti at mev.co.uk
Thu Apr 24 11:05:30 UTC 2014


On 2014-04-24 00:07, H Hartley Sweeten wrote:
> The chanlist is checked in Step 5 of the (*do_cmdtest) there is no
> reason to check it again in the (*do_cmd). The only reasonm its done
> is to get the actual 'seglen', the non-repeating length of the chanlist.
>
> Save the 'seglen' found by pci171x_ai_check_chanlist() in the private
> data and use that in the (*do_cmd).

Since do_cmdtest can be done while a command is running, one needs to be 
careful to make sure that any private data modified by do_cmdtest is not 
used by the running command, and only used by do_cmd when setting up the 
next command.

It's fine in this case though as devpriv->act_chanlist_len is only used 
while setting up the next command, although perhaps it ought to be 
renamed to devpriv->saved_chanlist_len, possibly by a subsequent patch. 
  (Also, devpriv->act_chanlist_pos isn't used so can be removed.)

>
> Refactor the error handling in pci171x_ai_check_chanlist() to work like
> the cfc_check_*() helpers.
>
> 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/adv_pci1710.c | 29 +++++++++++++++-------------
>   1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
> index 59c8bc4..6698d3c 100644
> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
> @@ -330,6 +330,7 @@ static int pci171x_ai_check_chanlist(struct comedi_device *dev,
>   				     struct comedi_subdevice *s,
>   				     struct comedi_cmd *cmd)
>   {
> +	struct pci1710_private *devpriv = dev->private;
>   	unsigned int chan0 = CR_CHAN(cmd->chanlist[0]);
>   	unsigned int last_aref = CR_AREF(cmd->chanlist[0]);
>   	unsigned int next_chan = (chan0 + 1) & s->n_chan;
> @@ -337,8 +338,10 @@ static int pci171x_ai_check_chanlist(struct comedi_device *dev,
>   	unsigned int seglen;
>   	int i;
>
> -	if (cmd->chanlist_len == 1)
> -		return 1; /* seglen=1 */
> +	if (cmd->chanlist_len == 1) {
> +		devpriv->act_chanlist_len = cmd->chanlist_len;
> +		return 0;
> +	}
>
>   	/* first channel is always ok */
>   	chansegment[0] = cmd->chanlist[0];
> @@ -353,7 +356,7 @@ static int pci171x_ai_check_chanlist(struct comedi_device *dev,
>   		if (aref == AREF_DIFF && (chan & 1)) {
>   			dev_err(dev->class_dev,
>   				"Odd channel cannot be differential input!\n");
> -			return 0;
> +			return -EINVAL;
>   		}
>
>   		if (last_aref == AREF_DIFF)
> @@ -362,7 +365,7 @@ static int pci171x_ai_check_chanlist(struct comedi_device *dev,
>   			dev_err(dev->class_dev,
>   				"channel list must be continuous! chanlist[%i]=%d but must be %d or %d!\n",
>   				i, chan, next_chan, chan0);
> -			return 0;
> +			return -EINVAL;
>   		}
>
>   		/* next correct channel in list */
> @@ -381,10 +384,12 @@ static int pci171x_ai_check_chanlist(struct comedi_device *dev,
>   				CR_CHAN(cmd->chanlist[i % seglen]),
>   				CR_RANGE(cmd->chanlist[i % seglen]),
>   				CR_AREF(chansegment[i % seglen]));
> -			return 0;
> +			return -EINVAL;
>   		}
>   	}
> -	return seglen;
> +	devpriv->act_chanlist_len = seglen;
> +
> +	return 0;
>   }
>
>   static void setup_channel_list(struct comedi_device *dev,
> @@ -396,7 +401,6 @@ static void setup_channel_list(struct comedi_device *dev,
>   	struct pci1710_private *devpriv = dev->private;
>   	unsigned int i, range, chanprog;
>
> -	devpriv->act_chanlist_len = seglen;
>   	devpriv->act_chanlist_pos = 0;
>
>   	for (i = 0; i < seglen; i++) {	/*  store range list to card */
> @@ -953,7 +957,6 @@ static int pci171x_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>   	struct pci1710_private *devpriv = dev->private;
>   	struct comedi_cmd *cmd = &s->async->cmd;
>   	unsigned int divisor1 = 0, divisor2 = 0;
> -	unsigned int seglen;
>   	int mode;
>
>   	if (cmd->convert_src == TRIG_TIMER) {
> @@ -967,10 +970,8 @@ static int pci171x_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>
>   	start_pacer(dev, -1, 0, 0);	/*  stop pacer */
>
> -	seglen = pci171x_ai_check_chanlist(dev, s, cmd);
> -	if (seglen < 1)
> -		return -EINVAL;
> -	setup_channel_list(dev, s, cmd->chanlist, cmd->chanlist_len, seglen);
> +	setup_channel_list(dev, s, cmd->chanlist, cmd->chanlist_len,
> +			   devpriv->act_chanlist_len);
>
>   	outb(0, dev->iobase + PCI171x_CLRFIFO);
>   	outb(0, dev->iobase + PCI171x_CLRINT);
> @@ -1104,7 +1105,9 @@ static int pci171x_ai_cmdtest(struct comedi_device *dev,
>
>   	/* Step 5: check channel list */
>
> -	if (!pci171x_ai_check_chanlist(dev, s, cmd))
> +	err |= pci171x_ai_check_chanlist(dev, s, cmd);
> +
> +	if (err)
>   		return 5;
>
>   	return 0;
>


-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-


More information about the devel mailing list