[PATCH 49/51 v2] staging: comedi: ni_labpc: fix labpc_eeprom_insn_write()

[Greg Kroah-Hartman]

On Mon, Mar 25, 2013 at 11:35:12AM -0500, H Hartley Sweeten wrote:
> On Monday, March 25, 2013 3:50 AM, Ian Abbott wrote:
> > On 2013-03-22 16:58, H Hartley Sweeten wrote:
> >> The comedi core expects the (*insn_write) operations to write insn->n
> >> values and return the number of values actually wrote.
> >>
> >> Make this function work like the core expects.
> >>
> >> Also, remove the noise about invalid channels.
> >>
> >> 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/ni_labpc.c | 20 ++++++++++----------
> >>   1 file changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/staging/comedi/drivers/ni_labpc.c b/drivers/staging/comedi/drivers/ni_labpc.c
> >> index 0afcede..6461a46 100644
> >> --- a/drivers/staging/comedi/drivers/ni_labpc.c
> >> +++ b/drivers/staging/comedi/drivers/ni_labpc.c
> >> @@ -1568,21 +1568,21 @@ static int labpc_eeprom_insn_write(struct comedi_device *dev,
> >>   				   struct comedi_insn *insn,
> >>   				   unsigned int *data)
> >>   {
> >> -	int channel = CR_CHAN(insn->chanspec);
> >> +	unsigned int chan = CR_CHAN(insn->chanspec);
> >>   	int ret;
> >> +	int i;
> >>
> >> -	/*  only allow writes to user area of eeprom */
> >> -	if (channel < 16 || channel > 127) {
> >> -		dev_dbg(dev->class_dev,
> >> -			"eeprom writes are only allowed to channels 16 through 127 (the pointer and user areas)\n");
> >> +	/* only allow writes to user area of eeprom */
> >> +	if (chan < 16 || chan > 127)
> >>   		return -EINVAL;
> >> -	}
> >>
> >> -	ret = labpc_eeprom_write(dev, channel, data[0]);
> >> -	if (ret < 0)
> >> -		return ret;
> >> +	for (i = 0; i < insn->n; i++) {
> >> +		ret = labpc_eeprom_write(dev, chan, data[i]);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >>
> >> -	return 1;
> >> +	return insn->n;
> >>   }
> >
> > This might not actually work if insn->n > 0 as the second call to 
> > labpc_eeprom_write() might time out (it starts off reading a status 
> > register up to 10000 times waiting for a previous call to 
> > labpc_eeprom_write() to complete, but the only delay in the loop is the 
> > register access itself).
> >
> > It's probably safer to just write the last data value as follows, as the 
> > preceding data would be overwritten anyway:
> > 
> > 	if (insn->n > 0) {
> > 		ret = labpc_eeprom_write(dev, chan, data[insn->n - 1]);
> >		if (ret)
> > 			return ret;
> >	}
> >
> >	return insn->n;
> >
> 
> Good point. I'll fix this in v2.

This was v2 already, right?

I'll apply the patches up to this one for now.

thanks,

greg k-h