Re: [PATCH 1/1] usb: serial: Fintek F81232 driver improvement

[Johan Hovold]

On Mon, Jan 19, 2015 at 09:54:55AM +0800, Peter Hung wrote:
> The original driver completed with TX function, but RX/MSR/MCR/LSR is not
> workable with this driver. So we rewrite it to make this device workable.
> 
> This patch is tested with PassMark BurnInTest with Cycle-to-115200 +
> MCR/MSR check for 15mins & checked with Suspend-To-RAM/DISK
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@xxxxxxxxx>

Thanks for the patch.

You need to break these changes up into several patches adding the
various features and submit it as a series. The rule of thumb is one
self-contained, logical change per patch (e.g. "fix x", "refactor y",
"add function z").

A few initial comments follow inline below.

> ---
>  drivers/usb/serial/f81232.c | 528 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 440 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index c5dc233..5ae6bc9 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,9 +23,16 @@
>  #include <linux/uaccess.h>
>  #include <linux/usb.h>
>  #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/version.h>
> +
> +#define FINTEK_MAGIC 'F'
> +#define FINTEK_GET_ID		_IOR(FINTEK_MAGIC, 3, int)

Adding new ioctls is hardly ever accepted, and definitely not for
retrieving static information that is already provided through sysfs
(idVendor, idProduct).

> +#define FINTEK_VENDOR_ID	0x1934
> +#define FINTEK_DEVICE_ID	0x0706	/* RS232 1 port */
>  
>  static const struct usb_device_id id_table[] = {
> -	{ USB_DEVICE(0x1934, 0x0706) },
> +	{ USB_DEVICE(FINTEK_VENDOR_ID, FINTEK_DEVICE_ID) },

So just drop these changes.

>  	{ }					/* Terminating entry */
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
> @@ -37,30 +44,257 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_STATE_TRANSIENT_MASK	0x74
>  #define UART_DCD			0x01
>  #define UART_DSR			0x02
> -#define UART_BREAK_ERROR		0x04
>  #define UART_RING			0x08
> -#define UART_FRAME_ERROR		0x10
> -#define UART_PARITY_ERROR		0x20
> -#define UART_OVERRUN_ERROR		0x40
>  #define UART_CTS			0x80
>  
> +
> +#define UART_BREAK_ERROR		0x10
> +#define UART_FRAME_ERROR		0x08
> +#define UART_PARITY_ERROR		0x04
> +#define UART_OVERRUN_ERROR		0x02
> +
> +
> +#define  SERIAL_EVEN_PARITY         (UART_LCR_PARITY | UART_LCR_EPAR)
> +
> +
> +#define REGISTER_REQUEST 0xA0
> +#define F81232_USB_TIMEOUT 1000
> +#define F81232_USB_RETRY 20
> +
> +
> +#define SERIAL_BASE_ADDRESS	   ((__u16)0x0120)
> +#define RECEIVE_BUFFER_REGISTER    ((__u16)(0x00) + SERIAL_BASE_ADDRESS)
> +#define TRANSMIT_HOLDING_REGISTER  ((__u16)(0x00) + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_ENABLE_REGISTER  ((__u16)(0x01) + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_IDENT_REGISTER   ((__u16)(0x02) + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER      ((__u16)(0x02) + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER      ((__u16)(0x03) + SERIAL_BASE_ADDRESS)
> +#define MODEM_CONTROL_REGISTER     ((__u16)(0x04) + SERIAL_BASE_ADDRESS)
> +#define LINE_STATUS_REGISTER       ((__u16)(0x05) + SERIAL_BASE_ADDRESS)
> +#define MODEM_STATUS_REGISTER      ((__u16)(0x06) + SERIAL_BASE_ADDRESS)

No need for casts.

> +
> +static int m_enable_debug;
> +
> +module_param(m_enable_debug, int, S_IRUGO);
> +MODULE_PARM_DESC(m_enable_debug, "Debugging mode enabled or not");

Don't add module parameters, use dynamic debugging.

> +
> +#define LOG_MESSAGE(x, y, ...)	\
> +	printk(x  y, ##__VA_ARGS__)
> +
> +#define LOG_DEBUG_MESSAGE(level, y, ...)	\
> +	do { if (unlikely(m_enable_debug))  \
> +			printk(level  y, ##__VA_ARGS__); } while (0)

Don't add your own debug macros, use dev_dbg.

> +
> +
>  struct f81232_private {
>  	spinlock_t lock;
> -	u8 line_control;
> -	u8 line_status;
> +	u8 modem_control;
> +	u8 modem_status;
> +	struct usb_device *dev;
> +
> +	struct work_struct int_worker;
> +	struct usb_serial_port *port;
>  };
>  
> -static void f81232_update_line_status(struct usb_serial_port *port,
> -				      unsigned char *data,
> -				      unsigned int actual_length)
> +
> +static inline int calc_baud_divisor(u32 baudrate)
>  {
> -	/*
> -	 * FIXME: Update port->icount, and call
> -	 *
> -	 *		wake_up_interruptible(&port->port.delta_msr_wait);
> -	 *
> -	 *	  on MSR changes.
> -	 */
> +	u32 divisor, rem;
> +
> +	divisor = 115200L / baudrate;
> +	rem = 115200L % baudrate;
> +
> +	/* Round to nearest divisor */
> +	if (((rem * 2) >= baudrate) && (baudrate != 110))
> +		divisor++;
> +
> +	return divisor;
> +}
> +
> +
> +static inline int f81232_get_register(struct usb_device *dev,
> +	u16 reg, u8 *data)
> +{
> +	int status;
> +	int i = 0;
> +timeout_get_repeat:
> +
> +	status = usb_control_msg(dev,
> +			 usb_rcvctrlpipe(dev, 0),
> +			 REGISTER_REQUEST,
> +			 0xc0,

Avoid magic constants, use defines with descriptive names.

> +			 reg,
> +			 0,
> +			 data,
> +			 sizeof(*data),
> +			 F81232_USB_TIMEOUT);
> +	if (status < 0) {
> +		i++;
> +
> +		if (i < F81232_USB_RETRY) {
> +			mdelay(1);
> +			goto timeout_get_repeat;

Why do you need to retry? You should probably just fail, otherwise
implement this a proper loop.

> +		}
> +	}
> +	return status;
> +}
> +
> +
> +static inline int f81232_set_register(struct usb_device *dev,
> +	u16 reg, u8 data)
> +{
> +	int status;
> +	int i = 0;
> +
> +timeout_set_repeat:
> +	status = 0;
> +
> +	status = usb_control_msg(dev,
> +		 usb_sndctrlpipe(dev, 0),
> +		 REGISTER_REQUEST,
> +		 0x40,
> +		 reg,
> +		 0,
> +		 &data,
> +		 1,
> +		 F81232_USB_TIMEOUT);
> +
> +	if (status < 0) {
> +		i++;
> +		if (i < F81232_USB_RETRY) {
> +			mdelay(1);
> +			goto timeout_set_repeat;

Same as above.

> +		}
> +	}
> +
> +	return status;
> +}

[...]

> -static void f81232_process_read_urb(struct urb *urb)
> +static void f81232_read_bulk_callback(struct urb *urb)

Why are you renaming this function (hint: you shouldn't).

>  {
>  	struct usb_serial_port *port = urb->context;
> -	struct f81232_private *priv = usb_get_serial_port_data(port);
>  	unsigned char *data = urb->transfer_buffer;
>  	char tty_flag = TTY_NORMAL;
> -	unsigned long flags;
> -	u8 line_status;
> +	u8 line_status = 0;
>  	int i;
>  
> -	/* update line status */
> -	spin_lock_irqsave(&priv->lock, flags);
> -	line_status = priv->line_status;
> -	priv->line_status &= ~UART_STATE_TRANSIENT_MASK;
> -	spin_unlock_irqrestore(&priv->lock, flags);
>  
>  	if (!urb->actual_length)
>  		return;
>  
>  	/* break takes precedence over parity, */
>  	/* which takes precedence over framing errors */
> +
> +#if 0
>  	if (line_status & UART_BREAK_ERROR)
>  		tty_flag = TTY_BREAK;
>  	else if (line_status & UART_PARITY_ERROR)
> @@ -129,28 +358,22 @@ static void f81232_process_read_urb(struct urb *urb)
>  	else if (line_status & UART_FRAME_ERROR)
>  		tty_flag = TTY_FRAME;
>  	dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> +#endif

Either remove or fix code, don't keep it unless used.

> -	/* overrun is special, not associated with a char */
> -	if (line_status & UART_OVERRUN_ERROR)
> -		tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> +	if (urb->actual_length >= 2) {
>  
> -	if (port->port.console && port->sysrq) {
> -		for (i = 0; i < urb->actual_length; ++i)
> -			if (!usb_serial_handle_sysrq_char(port, data[i]))
> -				tty_insert_flip_char(&port->port, data[i],
> -						tty_flag);
> -	} else {
> -		tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
> -							urb->actual_length);
> -	}
> +		for (i = 0 ; i < urb->actual_length ; i += 2) {
> +			line_status |= data[i+0];
> +			tty_insert_flip_string_fixed_flag(&port->port,
> +				&data[i+1], tty_flag, 1);
> +		}
>  
> -	tty_flip_buffer_push(&port->port);
> -}
> +		if (unlikely(line_status & UART_OVERRUN_ERROR))
> +			tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> +
> +		tty_flip_buffer_push(&port->port);
> +	}
>  
> -static int set_control_lines(struct usb_device *dev, u8 value)
> -{
> -	/* FIXME - Stubbed out for now */
> -	return 0;
>  }
>  
>  static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> @@ -165,30 +388,117 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>  }
>  
>  static void f81232_set_termios(struct tty_struct *tty,
> -		struct usb_serial_port *port, struct ktermios *old_termios)
> +			struct usb_serial_port *port,
> +			struct ktermios *old_termios)
>  {
> -	/* FIXME - Stubbed out for now */
> +	u16 divisor;
> +	u16 new_lcr = 0;
> +/*
> +The following comment is for legacy 3.7.0- kernel, You
> +can uncomment and build it if toy need
> +*/
>  
> -	/* Don't change anything if nothing has changed */
> -	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> -		return;
> +/*
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 7, 0)
> +	struct ktermios *termios = &tty->termios;
> +#else
> +	struct ktermios *termios = tty->termios;
> +#endif
> +*/

We don't want this. Don't use conditional compilation, and especially
not support older kernels like this.

> +	struct ktermios *termios = &tty->termios;
> +
> +	unsigned int cflag = termios->c_cflag;
> +	int status;
> +
> +	struct usb_device *dev = port->serial->dev;
> +
> +	divisor = calc_baud_divisor(tty_get_baud_rate(tty));
> +
> +	status = f81232_set_register(dev, LINE_CONTROL_REGISTER,
> +		UART_LCR_DLAB); /* DLAB */
> +	mdelay(1);

Why are you adding these delays?

> +	status = f81232_set_register(dev, RECEIVE_BUFFER_REGISTER,
> +		divisor & 0x00ff); /* low */
> +	mdelay(1);
> +	status = f81232_set_register(dev, INTERRUPT_ENABLE_REGISTER,
> +		(divisor & 0xff00) >> 8); /* high */
> +	mdelay(1);
> +	status = f81232_set_register(dev, LINE_CONTROL_REGISTER, 0x00);
> +	mdelay(1);
> +
> +	if (cflag & PARENB) {
> +		if (cflag & PARODD)
> +			new_lcr |= UART_LCR_PARITY; /* odd */
> +		else
> +			new_lcr |= SERIAL_EVEN_PARITY; /* even */
> +	}
> +
> +
> +	if (cflag & CSTOPB)
> +		new_lcr |= UART_LCR_STOP;
> +	else
> +		new_lcr &= ~UART_LCR_STOP;
> +
> +	switch (cflag & CSIZE) {
> +	case CS5:
> +		new_lcr |= UART_LCR_WLEN5;
> +		break;
> +	case CS6:
> +		new_lcr |= UART_LCR_WLEN6;
> +		break;
> +	case CS7:
> +		new_lcr |= UART_LCR_WLEN7;
> +		break;
> +	default:
> +	case CS8:
> +		new_lcr |= UART_LCR_WLEN8;
> +		break;
> +	}
> +
> +	status |= f81232_set_register(dev, LINE_CONTROL_REGISTER, new_lcr);
> +
> +	status |= f81232_set_register(dev, FIFO_CONTROL_REGISTER,
> +						  0x87); /* fifo, trigger8 */
> +	status |= f81232_set_register(dev,
> +		INTERRUPT_ENABLE_REGISTER, 0xf); /* IER */
> +
> +	if (status < 0) {
> +		LOG_MESSAGE(KERN_INFO,
> +			"[Fintek]: LINE_CONTROL_REGISTER set error: %d\n"
> +			, status);
> +	}
>  
> -	/* Do the real work here... */
> -	if (old_termios)
> -		tty_termios_copy_hw(&tty->termios, old_termios);
>  }
>  
>  static int f81232_tiocmget(struct tty_struct *tty)
>  {
> -	/* FIXME - Stubbed out for now */
> -	return 0;
> +	int r;
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct f81232_private *port_priv = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
> +	LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_tiocmget in\n");
> +	spin_lock_irqsave(&port_priv->lock, flags);
> +	r = (port_priv->modem_control & UART_MCR_DTR ? TIOCM_DTR : 0) |
> +		(port_priv->modem_control & UART_MCR_RTS ? TIOCM_RTS : 0) |
> +		(port_priv->modem_status & UART_MSR_CTS ? TIOCM_CTS : 0) |
> +		(port_priv->modem_status & UART_MSR_DCD ? TIOCM_CAR : 0) |
> +		(port_priv->modem_status & UART_MSR_RI ? TIOCM_RI : 0) |
> +		(port_priv->modem_status & UART_MSR_DSR ? TIOCM_DSR : 0);
> +	spin_unlock_irqrestore(&port_priv->lock, flags);

Use a temporary variable for the status.

> +	LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_tiocmget out\n");
> +	return r;
>  }
>  
>  static int f81232_tiocmset(struct tty_struct *tty,
> -			unsigned int set, unsigned int clear)
> +						   unsigned int set,
> +						   unsigned int clear)
>  {
> -	/* FIXME - Stubbed out for now */
> -	return 0;
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct f81232_private *port_priv =
> +		usb_get_serial_port_data(port);
> +
> +	return update_mctrl(port_priv, set, clear);
>  }
>  
>  static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
> @@ -201,12 +511,14 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>  
>  	result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
>  	if (result) {
> -		dev_err(&port->dev, "%s - failed submitting interrupt urb,"
> -			" error %d\n", __func__, result);
> +		dev_err(&port->dev,
> +			"%s - failed submitting interrupt urb, error %d\n"
> +				, __func__, result);

Fix this separately as well.

>  		return result;
>  	}
>  
>  	result = usb_serial_generic_open(tty, port);
> +

Don't do random whitespace changes (here or elsewhere).

>  	if (result) {
>  		usb_kill_urb(port->interrupt_in_urb);
>  		return result;
> @@ -217,6 +529,7 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>  
>  static void f81232_close(struct usb_serial_port *port)
>  {
> +
>  	usb_serial_generic_close(port);
>  	usb_kill_urb(port->interrupt_in_urb);
>  }
> @@ -224,52 +537,89 @@ static void f81232_close(struct usb_serial_port *port)
>  static void f81232_dtr_rts(struct usb_serial_port *port, int on)
>  {
>  	struct f81232_private *priv = usb_get_serial_port_data(port);
> -	unsigned long flags;
> -	u8 control;
>  
> -	spin_lock_irqsave(&priv->lock, flags);
> -	/* Change DTR and RTS */
>  	if (on)
> -		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
> +		update_mctrl(priv, TIOCM_DTR | TIOCM_RTS, 0);
>  	else
> -		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
> -	control = priv->line_control;
> -	spin_unlock_irqrestore(&priv->lock, flags);
> -	set_control_lines(port->serial->dev, control);
> +		update_mctrl(priv, 0, TIOCM_DTR | TIOCM_RTS);
>  }
>  
>  static int f81232_carrier_raised(struct usb_serial_port *port)
>  {
>  	struct f81232_private *priv = usb_get_serial_port_data(port);
> -	if (priv->line_status & UART_DCD)
> +	unsigned long flags;
> +	int modem_status;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	modem_status = priv->modem_status;
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	if (modem_status & UART_DCD)
>  		return 1;
>  	return 0;
>  }
>  
> +static int f81232_get_id(struct usb_serial_port *port, int __user *arg)
> +{
> +	/* 0x19340706 */
> +	int data = (FINTEK_VENDOR_ID << 16) | FINTEK_DEVICE_ID;
> +
> +	if (copy_to_user((int __user *) arg, &data, sizeof(int)))
> +		return -EFAULT;
> +
> +	return 0;
> +}

So drop this.

> +
> +
>  static int f81232_ioctl(struct tty_struct *tty,
> -			unsigned int cmd, unsigned long arg)
> +						unsigned int cmd,
> +						unsigned long arg)
>  {
>  	struct serial_struct ser;
>  	struct usb_serial_port *port = tty->driver_data;
>  
>  	switch (cmd) {
>  	case TIOCGSERIAL:
> -		memset(&ser, 0, sizeof ser);
> -		ser.type = PORT_16654;
> +		memset(&ser, 0, sizeof(ser));
> +		ser.flags		= ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
> +		ser.xmit_fifo_size	= port->bulk_out_size;
> +		ser.close_delay		= 5*HZ;
> +		ser.closing_wait	= 30*HZ;
> +
> +		ser.type = PORT_16550A;
>  		ser.line = port->minor;
>  		ser.port = port->port_number;
> -		ser.baud_base = 460800;
> +		ser.baud_base = 115200;
>  
> -		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
> +		if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
>  			return -EFAULT;
>  
>  		return 0;
> +
> +	case FINTEK_GET_ID:
> +		return f81232_get_id(port, (int __user *)arg);
> +
>  	default:
>  		break;
>  	}
>  	return -ENOIOCTLCMD;
>  }
>  
> +
> +
> +
> +static void f81232_int_work_wq(struct work_struct *work)
> +{
> +	struct f81232_private *priv =
> +		container_of(work, struct f81232_private, int_worker);
> +
> +
> +	LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_int_work_wq\n");
> +	f81232_read_msr(priv);
> +
> +
> +}
> +
>  static int f81232_port_probe(struct usb_serial_port *port)
>  {
>  	struct f81232_private *priv;
> @@ -279,10 +629,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&priv->lock);
> +	INIT_WORK(&priv->int_worker, f81232_int_work_wq);
>  
>  	usb_set_serial_port_data(port, priv);
>  
> -	port->port.drain_delay = 256;
> +	priv->dev = port->serial->dev;
> +	priv->port = port;

No need to store either of these in the private data.

>  	return 0;
>  }
> @@ -304,22 +656,21 @@ static struct usb_serial_driver f81232_device = {
>  	},
>  	.id_table =		id_table,
>  	.num_ports =		1,
> -	.bulk_in_size =		256,
> -	.bulk_out_size =	256,
> +	.bulk_in_size =		64,
> +	.bulk_out_size =	64,

Why are you reducing the buffer sizes?

>  	.open =			f81232_open,
>  	.close =		f81232_close,
> -	.dtr_rts = 		f81232_dtr_rts,
> +	.dtr_rts =		f81232_dtr_rts,

Again, don't include random whitespace changes.

>  	.carrier_raised =	f81232_carrier_raised,
>  	.ioctl =		f81232_ioctl,
>  	.break_ctl =		f81232_break_ctl,
>  	.set_termios =		f81232_set_termios,
>  	.tiocmget =		f81232_tiocmget,
>  	.tiocmset =		f81232_tiocmset,
> -	.tiocmiwait =		usb_serial_generic_tiocmiwait,
> -	.process_read_urb =	f81232_process_read_urb,
> +	.process_read_urb = f81232_read_bulk_callback,
>  	.read_int_callback =	f81232_read_int_callback,
>  	.port_probe =		f81232_port_probe,
> -	.port_remove =		f81232_port_remove,
> +	.port_remove = f81232_port_remove,

Ditto.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/