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@xxxxxxxxxxxxxxxxxxx>
> >> Cc: Ian Abbott <abbotti@xxxxxxxxx>
> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
