On 05/08/2014 17:58, Ian Abbott wrote:
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.
Hi Ian,
thanks for your comments.
I'll follow your suggestion and resend.
A question only:
apart from renaming file from ni_6501.c to ni_usb6501.c, should I rename
functions/variables as well? For example:
ni6501_auto_attach -> niusb6501_auto_attach
ni6501_private -> niusb6501_private
Thanks
--
Luca Ellero
E-mail: luca.ellero@xxxxxxxxxxxxxxxx
Internet: www.brickedbrain.com
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
