Re: [PATCH v2 6/9] staging: comedi: ni_usb6501: add ni6501_cnt_insn_config()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2014-09-17 17:57, Hartley Sweeten wrote:
On Tuesday, September 16, 2014 4:40 AM, Luca Ellero wrote:
Add function for counter subdevice configuration.

Signed-off-by: Luca Ellero <luca.ellero@xxxxxxxxxxxxxxxx>
---
  drivers/staging/comedi/drivers/ni_usb6501.c |   28 +++++++++++++++++++++++++++
  1 file changed, 28 insertions(+)

diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c b/drivers/staging/comedi/drivers/ni_usb6501.c
index 60fc1ee..31a798e 100644
--- a/drivers/staging/comedi/drivers/ni_usb6501.c
+++ b/drivers/staging/comedi/drivers/ni_usb6501.c
@@ -439,6 +439,34 @@ static int ni6501_dio_insn_bits(struct comedi_device *dev,
  	return insn->n;
  }

Again, this patch causes a build warning:
drivers/staging/comedi/drivers/ni_usb6501.c:442:12: warning: 'ni6501_cnt_insn_config' defined but not used [-Wunused-function]

But, it does fix the one caused by patch 5/9... ;-)

+static int ni6501_cnt_insn_config(struct comedi_device *dev,
+				  struct comedi_subdevice *s,
+				  struct comedi_insn *insn,
+				  unsigned int *data)
+{
+	int ret;
+	u32 counter = 0;
+
+	switch (data[0]) {
+	case INSN_CONFIG_ARM:
+		ret = ni6501_counter_command(dev, START_COUNTER, NULL);
+		break;
+	case INSN_CONFIG_DISARM:
+		ret = ni6501_counter_command(dev, STOP_COUNTER, NULL);
+		break;
+	case INSN_CONFIG_RESET:
+		ret = ni6501_counter_command(dev, STOP_COUNTER, NULL);
+		if (ret)
+			break;
+		ret = ni6501_counter_command(dev, WRITE_COUNTER, &counter);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}

ni6501_counter_command() returns 0 or an errno.

Ian suggested that this if fine because it will prevent the comedi
core from attempting to write the data array back to the user.
This is not true because the INSN_CONFIG instruction is defined as:

#define INSN_CONFIG		(3 | INSN_MASK_READ|INSN_MASK_WRITE)

And the do_insnlist_ioctl() function will copy back the data array
as long as the return value of parse_insn() is >= 0 and the insn has
INSN_MASK_READ set.

Yes, you're right. It copies insn->n elements of data back to the user. I was thinking it was using the return value for some reason.

For aesthetics, I would prefer this function returns the insn->n value
if it succeeds. This would work and follows the norm for other comedi
drivers:

	return ret ? ret : insn->n;

Yes, that's the norm. Although the comedi core doesn't care unless it's negative. (It might save a few instructions if we just returned 0.)

--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux