Re: [PATCH] staging: comedi: add NI USB-6501 initial support

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

 



On 2014-08-05 15:22, Luca Ellero wrote:
This is a preliminary version, some features are not implemented yet:
	GPIO: works
	counter: doesn't work

Signed-off-by: Luca Ellero <luca.ellero@xxxxxxxxxxxxxxxx>
---
This is a preliminary version of the NI USB-6501 driver.
Every comment/suggestion is welcome.
I plan to implement counter device in the next weeks, once this code will be
in the correct form.

Regards

It looks mostly okay, though has a few too many blank lines - running 'scripts/checkpatch.pl --strict drivers/staging/comedi/ni_6501.c' picks up a few whitespace problems.

One thought I had is that maybe "ni_usb6501" would be a better name to help distinguish it from the other "ni_65*" modules that are all PCI cards. It's not that important though. If you change it, the Kconfig option name can be changed to match, e.g. "CONFIG_COMEDI_NI_USB6501".

I've marked a few other issues below...

  drivers/staging/comedi/Kconfig           |    8 +
  drivers/staging/comedi/drivers/Makefile  |    1 +
  drivers/staging/comedi/drivers/ni_6501.c |  484 ++++++++++++++++++++++++++++++
  3 files changed, 493 insertions(+)
  create mode 100644 drivers/staging/comedi/drivers/ni_6501.c

diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
index 36f2c71..679f27a 100644
--- a/drivers/staging/comedi/Kconfig
+++ b/drivers/staging/comedi/Kconfig
@@ -1209,6 +1209,14 @@ config COMEDI_DT9812
  	  To compile this driver as a module, choose M here: the module will be
  	  called dt9812.

+config COMEDI_NI_6501
+	tristate "NI 6501 support"

It's better to use "NI USB-6501 support" there to match the model name.

+	---help---
+	  Enable support for the National Instruments 6501 USB module

And similar there.

checkpatch warns about the description being a bit short: "WARNING: please write a paragraph that describes the config symbol fully". You could add a bit of blurb to describe the device a bit more fully, e.g. that it has 24 GPIO lines and a 32-bit counter. Sometimes it's hard to make up enough stuff to avoid that checkpatch warning though!

+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ni_6501.
+
  config COMEDI_USBDUX
  	tristate "ITL USB-DUX-D support"
  	---help---
diff --git a/drivers/staging/comedi/drivers/Makefile b/drivers/staging/comedi/drivers/Makefile
index 8873d48..6682759 100644
--- a/drivers/staging/comedi/drivers/Makefile
+++ b/drivers/staging/comedi/drivers/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_COMEDI_QUATECH_DAQP_CS)	+= quatech_daqp_cs.o

  # Comedi USB drivers
  obj-$(CONFIG_COMEDI_DT9812)		+= dt9812.o
+obj-$(CONFIG_COMEDI_NI_6501)		+= ni_6501.o
  obj-$(CONFIG_COMEDI_USBDUX)		+= usbdux.o
  obj-$(CONFIG_COMEDI_USBDUXFAST)		+= usbduxfast.o
  obj-$(CONFIG_COMEDI_USBDUXSIGMA)	+= usbduxsigma.o
diff --git a/drivers/staging/comedi/drivers/ni_6501.c b/drivers/staging/comedi/drivers/ni_6501.c
new file mode 100644
index 0000000..e103171
--- /dev/null
+++ b/drivers/staging/comedi/drivers/ni_6501.c
@@ -0,0 +1,484 @@
+/*
+    comedi/drivers/ni_6501.c
+    Comedi driver for National Instruments USB-6501
+
+    COMEDI - Linux Control and Measurement Device Interface
+    Copyright (C) 2014 Luca Ellero <luca.ellero@xxxxxxxxxxxxxxxx>
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+*/
+/*
+Driver: ni_6501
+Description: National Instruments USB-6501 module
+Devices: [National Instruments] USB-6501 (ni_6501)
+Author: Luca Ellero <luca.ellero@xxxxxxxxxxxxxxxx>
+Updated: 5 Aug 2014
+Status: works
+
+This driver works, but counter device is not implemented yet.
+
+Configuration Options:
+  none
+*/

We're gradually migrating to the standard block comment format for those two comments now. For example, see the ni_labpc.c comedi driver.

+
+/*
+ * NI-6501 - USB PROTOCOL DESCRIPTION
+ *
+ * Every command is composed by two USB packets:
+ *	- request (out)
+ *	- response (in)
+ *
+ * Every packet is at least 12 bytes long, here is the meaning of
+ *	every field (all values are hex):
+ *
+ *	byte 0 is always 00
+ *	byte 1 is always 01
+ *	byte 2 is always 00
+ *	byte 3 is the total packet lenght

"length" is spelt wrong there.

+ *
+ *	byte 4 is always 00
+ *	byte 5 is is the total packet lenght - 4

And there.

I don't really have a problem with the rest of it, apart from the checkpatch.pl --strict issues, and the multiple blank lines where one blank line will suffice.

Not bad, overall.

--
-=( 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