Re: [PATCH] iuu_patch call for reviews/comments

> +config USB_SERIAL_IUU
> +	tristate "USB Infinity USB Unlimited Phoenix Driver (Experimental)"
> +	depends on USB_SERIAL && EXPERIMENTAL
> +	help
> +	  Say Y here if you want to use a IUU in phoenix mode and get
> +	  an extra ttyUSBx device. More information available on
> +	  http://eczema.ecze.com/iuu_phoenix.html
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called iuu_phoenix.o
> +

Looks ok. Might be good to put the extra info in the kernel documentation
directory once its fairly stabilized (just so its not lost if the domain
moves
etc)

>  config USB_SERIAL_KEYSPAN_PDA
>  	tristate "USB Keyspan PDA Single Port Serial Driver"
>  	depends on USB_SERIAL
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 61166ad..cd26931 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile

OK


> +	int TIOSTATUS;		/* store IUART SIGNAL for tiocmget call */

Trivial: lower case for variable names, upper for defines [coding style]

> +static int iuu_alloc_buf(struct iuu_private *priv)
> +{

> +		dbg("%s problem allocation buffer", __FUNCTION__);

Trivial: problem allocating buffer

> +/* turbo parameter */
> +
> +int boost = 100;
> +int clockmode = 1;
> +int cdmode = 1;
> +int iuu_cardin ;
> +int iuu_cardout ;
> +int xmas = 0;

Should all be static


> +static int iuu_startup(struct usb_serial *serial)
> +{
> +	struct iuu_private *priv;
> +	priv = kzalloc(sizeof(struct iuu_private), GFP_KERNEL);
> +	dbg("%s- priv allocation success", __FUNCTION__);
> +	if (!priv)
> +		return -ENOMEM;

Should the dbg() be after the check it worked ?


> +	dbg("%s (%d) msg = %04x", __FUNCTION__, port->number, mask);
> +	switch (cmd) {
> +	case TCGETS:
> +		return (0);

NAK still wrong. You cannot handle TCGET/SET* speed ioctls this way you
must use the ->set_termios method.


> +	case TCSBRK:
> +		dbg("%s (%d) TIOBRK", __FUNCTION__, port->number);
> +		return 0;

Don't fake unsupported stuff, it causes problems long term

> +int
> +iuu_led(struct usb_serial_port *port, unsigned int R, unsigned int G,
> +	unsigned int B, u8 f)
> +{
> +	int status;
> +	u8 *buf;
> +	buf = kmalloc(sizeof(u_int8_t) * 8, GFP_KERNEL);
> +
> +	dbg("%s - enter", __FUNCTION__);
> +
> +	buf[0] = IUU_SET_LED;
> +	buf[1] = R & 0xFF;


Missing NULL check

> +	memset(port->write_urb->transfer_buffer, IUU_SET_LED , 1);
> +	memset(port->write_urb->transfer_buffer + 1, priv->R1 , 1);
> +	memset(port->write_urb->transfer_buffer + 2, priv->R2, 1);
> +	memset(port->write_urb->transfer_buffer + 3, priv->G1, 1);
> +	memset(port->write_urb->transfer_buffer + 4, priv->G2, 1);
> +	memset(port->write_urb->transfer_buffer + 5, priv->B1, 1);
> +	memset(port->write_urb->transfer_buffer + 6, priv->B2, 1);
> +	memset(port->write_urb->transfer_buffer + 7, F, 1);

Would be much clearer if these were do as

	char *ptr = port->blah->transfer_buffer;
	*ptr++ = IUU_SET_LED;
	*ptr++ = priv->R1;
	...

You also seem to have multiple functions doing the same thing except for
the first byte so this could be a seperate helper routine


> +	if (tty && urb->actual_length && data != NULL ) {
> +		tty_buffer_request_room(tty, urb->actual_length + 1);
> +		for (i = 0; i < urb->actual_length; ++i)
> +			tty_insert_flip_char(tty, data[i], TTY_NORMAL);

As I said last time just use:

	if (tty && urb->actual_lenth && data)
		tty_insert_flip_string(tty, data, urb->actual_length);
> +		tty_flip_buffer_push(tty);
> +	}



> +static int iuu_bulk_write(struct usb_serial_port *port)
> +{


> +	memset(port->write_urb->transfer_buffer, IUU_UART_ESC, 1);
> +	memset(port->write_urb->transfer_buffer + 1, IUU_UART_TX, 1);
> +	memset(port->write_urb->transfer_buffer + 2, priv->writelen, 1);

Similar comments about 1 byte memsets


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel