Some minor style nits.
On Wed, Aug 06, 2014 at 01:35:32PM +0200, Luca Ellero wrote:
> +static int ni6501_send_command(struct comedi_device *dev, int command,
> + const u8 *port, u8 *bitmap)
> +{
> + 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 || !port)
> + return -EINVAL;
> +
> + if (command != SET_PORT_DIR && !bitmap)
^
Extra space character.
> + return -EINVAL;
> +
> + down(&devpriv->sem);
> +
> + switch (command) {
> + case READ_PORT:
> +
> + request_size = sizeof(READ_PORT_REQUEST);
> + /* 4 additional bytes for READ_PORT request */
> + response_size = sizeof(GENERIC_RESPONSE) + 4;
> +
> + memcpy(tx, READ_PORT_REQUEST, request_size);
> +
> + tx[14] = port[0];
> +
> + break;
> +
> + case WRITE_PORT:
> +
> + request_size = sizeof(WRITE_PORT_REQUEST);
> + response_size = sizeof(GENERIC_RESPONSE);
> +
> + memcpy(tx, WRITE_PORT_REQUEST, request_size);
> +
> + tx[14] = port[0];
> + tx[17] = bitmap[0];
> +
> + break;
> +
> + case SET_PORT_DIR:
> +
> + request_size = sizeof(SET_PORT_DIR_REQUEST);
> + response_size = sizeof(GENERIC_RESPONSE);
> +
> + memcpy(tx, SET_PORT_DIR_REQUEST, request_size);
> +
> + tx[14] = port[0];
> + tx[15] = port[1];
> + tx[16] = port[2];
> +
> + break;
> +
> + default:
> + ret = -EINVAL;
> + goto end;
> + }
> +
> + ret = usb_bulk_msg(usb,
> + usb_sndbulkpipe(usb,
> + devpriv->ep_tx->bEndpointAddress),
> + devpriv->usb_tx_buf,
> + request_size,
> + NULL,
> + NI6501_TIMEOUT);
> +
Don't leave a blank line here before checking ret. Especially the
ni6501_dio_insn_bits() has too many blanks lines. See below.
> + if (ret)
> + goto end;
> +
> + ret = usb_bulk_msg(usb,
> + usb_rcvbulkpipe(usb,
> + devpriv->ep_rx->bEndpointAddress),
> + devpriv->usb_rx_buf,
> + response_size,
> + NULL,
> + NI6501_TIMEOUT);
> +
> + if (ret)
> + goto end;
> +
> + /* Check if results are valid */
^
Extra space character.
[snip]
> +static int ni6501_dio_insn_bits(struct comedi_device *dev,
> + struct comedi_subdevice *s,
> + struct comedi_insn *insn,
> + unsigned int *data)
> +{
> + unsigned int mask;
> + int ret;
> + u8 port;
> + u8 bitmap;
> +
> + mask = comedi_dio_update_state(s, data);
> +
> + if (mask & 0xff) {
> + port = 0x00;
> + bitmap = s->state & 0xFF;
> +
> + ret = ni6501_send_command(dev, WRITE_PORT, &port, &bitmap);
> +
> + if (ret)
> + return ret;
> + }
> +
> + if (mask & 0xff00) {
> + port = 0x01;
> + bitmap = (s->state >> 8) & 0xFF;
> +
> + ret = ni6501_send_command(dev, WRITE_PORT, &port, &bitmap);
> +
> + if (ret)
> + return ret;
> + }
> +
> + if (mask & 0xff0000) {
> + port = 0x02;
> + bitmap = (s->state >> 16) & 0xFF;
> +
> + ret = ni6501_send_command(dev, WRITE_PORT, &port, &bitmap);
> +
> + if (ret)
> + return ret;
> + }
> +
> + /* Read port 0 */
This comment doesn't add anything because port = 0x00 below.
> +
> + port = 0x00;
> +
> + ret = ni6501_send_command(dev, READ_PORT, &port, &bitmap);
> +
> + if (ret)
> + return ret;
> +
> + data[1] = bitmap;
> +
You could do it like this:
data[1] = 0;
for (port = 0; port < 3; port++) {
ret = ni6501_send_command(dev, READ_PORT, &port, &bitmap);
if (ret)
return ret;
data[1] |= bitmap << port * 8;
}
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
