On Tuesday, September 16, 2014 4:40 AM, Luca Ellero wrote:
> Signed-off-by: Luca Ellero <luca.ellero@xxxxxxxxxxxxxxxx>
> ---
> drivers/staging/comedi/drivers/ni_usb6501.c | 110 +++++++++++++++++++++++++++
> 1 file changed, 110 insertions(+)
>
> diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c b/drivers/staging/comedi/drivers/ni_usb6501.c
> index f55b9f8..60fc1ee 100644
> --- a/drivers/staging/comedi/drivers/ni_usb6501.c
> +++ b/drivers/staging/comedi/drivers/ni_usb6501.c
> @@ -272,6 +272,116 @@ end:
> return ret;
> }
I'm not a fan of the build warning:
drivers/staging/comedi/drivers/ni_usb6501.c:275:12: warning: 'ni6501_counter_command' defined but not used [-Wunused-function]
But I guess it's not a big deal. The only real fix is to merge patches
5, 6, 7, 8, and 9 into a single "add counter subdevice" patch.
The rest of the comments are just nit-picks.
> +static int ni6501_counter_command(struct comedi_device *dev, int command,
> + u32 *counter)
> +{
> + struct usb_device *usb = comedi_to_usb_dev(dev);
> + struct ni6501_private *devpriv = dev->private;
> + int request_size, response_size;
> + u8 *tx = devpriv->usb_tx_buf;
> + int ret;
> +
> + if (!tx)
> + return -EINVAL;
This test is not necessary. If 'devpriv->usb_tx_buf' is not allocated
the (*auto_attach) will fail.
> +
> + if ((command == READ_COUNTER || command == WRITE_COUNTER) && !counter)
> + return -EINVAL;
The 'counter' is actually the 'value' read or written to the then counter. The
variable name seems to suggest that it is the counter number.
It might be clearer to rename this parameter the 'val' or something
similar.
> +
> + down(&devpriv->sem);
> +
> + switch (command) {
> + case START_COUNTER:
> +
Unnecessary white space.
> + request_size = sizeof(START_COUNTER_REQUEST);
> + response_size = sizeof(GENERIC_RESPONSE);
> +
> + memcpy(tx, START_COUNTER_REQUEST, request_size);
> +
Again, unnecessary white space. Same with the remaining case/break
statements.
> + break;
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
