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

Wed Aug 6 08:49:10 UTC 2014

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 at brickedbrain.com>
>> ---
>> 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 at brickedbrain.com>
>> +
>> +    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 at brickedbrain.com>
>> +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 at brickedbrain.com
Internet: www.brickedbrain.com