[PATCH v2] staging: comedi: adv_pci1710: fix analog output readback value

[Ian Abbott]

On 2014-02-05 21:59, H Hartley Sweeten wrote:
> The last value written to a analog output channel is cached in the
> private data of this driver for readback.
>
> Currently, the wrong value is cached in the (*insn_write) functions.
> The current code stores the data[n] value for readback afer the loop
> has written all the values. At this time 'n' points past the end of
> the data array.
>
> Fix the functions by using a local variable to hold the data being
> written to the analog output channel. This variable is then used
> after the loop is complete to store the readback value. The current
> value is retrieved before the loop in case no values are actually
> written..
>
> 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>
> ---
>
> v2: also fix readback in pci1720_insn_write_ao() as pointed out by
>      Ian Abbott
>
>   drivers/staging/comedi/drivers/adv_pci1710.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
> index 69a8298..f9d6111 100644
> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
> @@ -494,6 +494,7 @@ static int pci171x_insn_write_ao(struct comedi_device *dev,
>   				 struct comedi_insn *insn, unsigned int *data)
>   {
>   	struct pci1710_private *devpriv = dev->private;
> +	unsigned int val;
>   	int n, chan, range, ofs;
>
>   	chan = CR_CHAN(insn->chanspec);
> @@ -509,11 +510,14 @@ static int pci171x_insn_write_ao(struct comedi_device *dev,
>   		outw(devpriv->da_ranges, dev->iobase + PCI171x_DAREF);
>   		ofs = PCI171x_DA1;
>   	}
> +	val = devpriv->ao_data[chan];
>
> -	for (n = 0; n < insn->n; n++)
> -		outw(data[n], dev->iobase + ofs);
> +	for (n = 0; n < insn->n; n++) {
> +		val = data[n];
> +		outw(val, dev->iobase + ofs);
> +	}
>
> -	devpriv->ao_data[chan] = data[n];
> +	devpriv->ao_data[chan] = val;
>
>   	return n;
>
> @@ -679,6 +683,7 @@ static int pci1720_insn_write_ao(struct comedi_device *dev,
>   				 struct comedi_insn *insn, unsigned int *data)
>   {
>   	struct pci1710_private *devpriv = dev->private;
> +	unsigned int val;
>   	int n, rangereg, chan;
>
>   	chan = CR_CHAN(insn->chanspec);
> @@ -688,13 +693,15 @@ static int pci1720_insn_write_ao(struct comedi_device *dev,
>   		outb(rangereg, dev->iobase + PCI1720_RANGE);
>   		devpriv->da_ranges = rangereg;
>   	}
> +	val = devpriv->ao_data[chan];
>
>   	for (n = 0; n < insn->n; n++) {
> -		outw(data[n], dev->iobase + PCI1720_DA0 + (chan << 1));
> +		val = data[n];
> +		outw(val, dev->iobase + PCI1720_DA0 + (chan << 1));
>   		outb(0, dev->iobase + PCI1720_SYNCOUT);	/*  update outputs */
>   	}
>
> -	devpriv->ao_data[chan] = data[n];
> +	devpriv->ao_data[chan] = val;
>
>   	return n;
>   }
>

I'd have done:

	if (n)
		devpriv->ao_data[chan] = val;

and initialized val to 0 (in case the compiler issues a false 
"uninitialized" warning), just because it seems a tad more optimizable 
that way.  It doesn't matter though; your way is fine too!

Reviewed-by: Ian Abbott <abbotti at mev.co.uk>

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