[PATCH V5 0/8] USB: f81534: v5 patch

[PATCH V5 0/8] USB: f81534: v5 patch

[Peter Hung]

This series patch V5 is improvement from V4 as following:

1. transform all function not to use private data as parameter, using
   usb_serial_port instead.

2. Some init step we extract it from set_term() to f81232_port_init()
   and run it when open port only.

3. We'll force re-read msr in tiocmget() because the IIR with MSR change
   maybe delay received.

4. process_read_urb() add process of Break/FrameError/ParityError.

5. clarify a lot of code about Johan suggested.

Peter Hung (8):
  USB: f81232: Rename private struct member name
  USB: f81232: implement read IIR/MSR with endpoint
  USB: f81232: implement RX bulk-in ep
  USB: f81232: implement set_termios
  USB: f81232: implement MCR/MSR function
  USB: f81232: clarify f81232_ioctl()
  USB: f81232: fix error in f81232_carrier_raised()
  USB: f81232: modify/add author

 drivers/usb/serial/f81232.c | 471 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 375 insertions(+), 96 deletions(-)

-- 
1.9.1

[PATCH V5 1/8] USB: f81232: Rename private struct member name

[Peter Hung]

Change private struct member name from line_status to modem_status.
It will store MSR for some functions used

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index c5dc233..669a2f2 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -47,7 +47,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
-	u8 line_status;
+	u8 modem_status;
 };
 
 static void f81232_update_line_status(struct usb_serial_port *port,
@@ -113,8 +113,8 @@ static void f81232_process_read_urb(struct urb *urb)
 
 	/* update line status */
 	spin_lock_irqsave(&priv->lock, flags);
-	line_status = priv->line_status;
-	priv->line_status &= ~UART_STATE_TRANSIENT_MASK;
+	line_status = priv->modem_status;
+	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	if (!urb->actual_length)
@@ -241,7 +241,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 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)
+	if (priv->modem_status & UART_DCD)
 		return 1;
 	return 0;
 }
-- 
1.9.1

[PATCH V5 2/8] USB: f81232: implement read IIR/MSR with endpoint

[Peter Hung]

The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
, We will do read MSR with interrupt_work worker to do f81232_read_msr() func.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 669a2f2..ec4609d 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
+#include <linux/serial_reg.h>
 
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x1934, 0x0706) },
@@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0
+
+#define SERIAL_BASE_ADDRESS	   0x0120
+#define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 
@@ -48,19 +56,92 @@ struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
 	u8 modem_status;
+
+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
-static void f81232_update_line_status(struct usb_serial_port *port,
+static int f81232_get_register(struct usb_serial_port *port,
+							  u16 reg, u8 *data)
+{
+	int status;
+	struct usb_device *dev = port->serial->dev;
+
+	status = usb_control_msg(dev,
+			 usb_rcvctrlpipe(dev, 0),
+			 F81232_REGISTER_REQUEST,
+			 F81232_GET_REGISTER,
+			 reg,
+			 0,
+			 data,
+			 sizeof(*data),
+			 USB_CTRL_GET_TIMEOUT);
+	if (status < 0)
+		dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+	return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	unsigned long flags;
+	u8 current_msr;
+	struct tty_struct *tty;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+			&current_msr);
+	if (status < 0) {
+		/* Retain the error even reported in f81232_get_register()
+		     to make debug easily :D */
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		return;
+	}
+
+	if (!(current_msr & UART_MSR_ANY_DELTA))
+		return;
+
+	tty = tty_port_tty_get(&port->port);
+	if (tty) {
+		if (current_msr & UART_MSR_DDCD) {
+			usb_serial_handle_dcd_change(port, tty,
+				current_msr & UART_MSR_DCD);
+		}
+
+		tty_kref_put(tty);
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+	priv->modem_status = current_msr;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
+static void f81232_update_modem_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
 {
-	/*
-	 * FIXME: Update port->icount, and call
-	 *
-	 *		wake_up_interruptible(&port->port.delta_msr_wait);
-	 *
-	 *	  on MSR changes.
-	 */
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (!actual_length)
+		return;
+
+	switch (data[0] & 0x07) {
+	case 0x00: /* msr change */
+		dev_dbg(&port->dev, "IIR: MSR Change: %x\n", data[0]);
+		schedule_work(&priv->interrupt_work);
+		break;
+	case 0x02: /* tx-empty */
+		break;
+	case 0x04: /* rx data available */
+		break;
+	case 0x06: /* lsr change */
+		/* we can forget it. the LSR will read from bulk-in */
+		dev_dbg(&port->dev, "IIR: LSR Change: %x\n", data[0]);
+		break;
+	}
 }
 
 static void f81232_read_int_callback(struct urb *urb)
@@ -91,7 +172,7 @@ static void f81232_read_int_callback(struct urb *urb)
 	usb_serial_debug_data(&port->dev, __func__,
 			      urb->actual_length, urb->transfer_buffer);
 
-	f81232_update_line_status(port, data, actual_length);
+	f81232_update_modem_status(port, data, actual_length);
 
 exit:
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -270,6 +351,14 @@ static int f81232_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static void  f81232_interrupt_work(struct work_struct *work)
+{
+	struct f81232_private *priv =
+		container_of(work, struct f81232_private, interrupt_work);
+
+	f81232_read_msr(priv->port);
+}
+
 static int f81232_port_probe(struct usb_serial_port *port)
 {
 	struct f81232_private *priv;
@@ -279,10 +368,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	spin_lock_init(&priv->lock);
+	INIT_WORK(&priv->interrupt_work,  f81232_interrupt_work);
 
 	usb_set_serial_port_data(port, priv);
 
 	port->port.drain_delay = 256;
+	priv->port = port;
 
 	return 0;
 }
-- 
1.9.1

[PATCH V5 3/8] USB: f81232: implement RX bulk-in ep

[Peter Hung]

The F81232 bulk-in is RX data + LSR channel, data format is
[LSR+Data][LSR+Data]..... , We had reimplemented in this patch.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 68 +++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index ec4609d..9ea498a 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -185,44 +185,46 @@ exit:
 static void f81232_process_read_urb(struct urb *urb)
 {
 	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;
+	char tty_flag;
 	int i;
 
-	/* update line status */
-	spin_lock_irqsave(&priv->lock, flags);
-	line_status = priv->modem_status;
-	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	if (!urb->actual_length)
+	if (urb->actual_length < 2)
 		return;
 
-	/* break takes precedence over parity, */
-	/* which takes precedence over framing errors */
-	if (line_status & UART_BREAK_ERROR)
-		tty_flag = TTY_BREAK;
-	else if (line_status & UART_PARITY_ERROR)
-		tty_flag = TTY_PARITY;
-	else if (line_status & UART_FRAME_ERROR)
-		tty_flag = TTY_FRAME;
-	dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
-
-	/* overrun is special, not associated with a char */
-	if (line_status & UART_OVERRUN_ERROR)
-		tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
-
-	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);
+	/* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
+
+	for (i = 0 ; i < urb->actual_length ; i += 2) {
+		tty_flag = TTY_NORMAL;
+
+		if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
+			if (data[i+0] & UART_LSR_BI) {
+				tty_flag = TTY_BREAK;
+				port->icount.brk++;
+				usb_serial_handle_break(port);
+			} else if (data[i+0] & UART_LSR_PE) {
+				tty_flag = TTY_PARITY;
+				port->icount.parity++;
+			} else if (data[i+0] & UART_LSR_FE) {
+				tty_flag = TTY_FRAME;
+				port->icount.frame++;
+			}
+
+			if (data[0] & UART_LSR_OE) {
+				port->icount.overrun++;
+				tty_insert_flip_char(&port->port, 0,
+					TTY_OVERRUN);
+			}
+		}
+
+		if (port->port.console && port->sysrq) {
+			if (!usb_serial_handle_sysrq_char(port, data[i+1]))
+				tty_insert_flip_char(&port->port, data[i+1],
+					tty_flag);
+		} else {
+			tty_insert_flip_string_fixed_flag(&port->port,
+				&data[i+1], tty_flag, 1);
+		}
 	}
 
 	tty_flip_buffer_push(&port->port);
-- 
1.9.1

[PATCH V5 4/8] USB: f81232: implement set_termios

[Peter Hung]

The original driver had do not any h/w change in driver.
This patch implements with configure H/W for
baud/parity/word length/stop bits functional.

Some init step extract to f81232_port_init(), called once with open().
And refine baudrate setting to f81232_set_baudrate()

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 9ea498a..06d1eb0 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE 115200L
+
 /* USB Control EP parameter */
 #define F81232_REGISTER_REQUEST 0xA0
 #define F81232_GET_REGISTER 0xc0
+#define F81232_SET_REGISTER 0x40
 
 #define SERIAL_BASE_ADDRESS	   0x0120
+#define RECEIVE_BUFFER_REGISTER    (0x00 + SERIAL_BASE_ADDRESS)
+#define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01
@@ -61,6 +69,14 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(u32 baudrate)
+{
+	if (!baudrate)
+		return 0;
+	else
+		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
 static int f81232_get_register(struct usb_serial_port *port,
 							  u16 reg, u8 *data)
 {
@@ -82,6 +98,27 @@ static int f81232_get_register(struct usb_serial_port *port,
 	return status;
 }
 
+static int f81232_set_register(struct usb_serial_port *port,
+							  u16 reg, u8 data)
+{
+	int status;
+	struct usb_device *dev = port->serial->dev;
+
+	status = usb_control_msg(dev,
+			usb_sndctrlpipe(dev, 0),
+			F81232_REGISTER_REQUEST,
+			F81232_SET_REGISTER,
+			reg,
+			0,
+			&data,
+			sizeof(data),
+			USB_CTRL_SET_TIMEOUT);
+	if (status < 0)
+		dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+	return status;
+}
+
 static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;
@@ -247,18 +284,106 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
+{
+	u8 divisor;
+	int status = 0;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 UART_LCR_DLAB); /* DLAB */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n",
+			__func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n",
+			__func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+			status, __LINE__);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, 0x00);
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+			status, __LINE__);
+	}
+}
+
+static int f81232_port_init(struct usb_serial_port *port)
+{
+	u8 data;
+	int status = 0;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+			| UART_FCR_CLEAR_XMIT;
+
+	status |= f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	data = UART_IER_MSI;
+
+	/* IER */
+	status |= f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+	if (status < 0) {
+		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	/* FIXME - Stubbed out for now */
+	u8 new_lcr = 0;
+	int status = 0;
 
-	/* Don't change anything if nothing has changed */
-	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
-		return;
+	f81232_set_baudrate(port, tty_get_baud_rate(tty));
+
+	if (C_PARENB(tty)) {
+		new_lcr |= UART_LCR_PARITY;
+
+		if (!C_PARODD(tty))
+			new_lcr |= UART_LCR_EPAR;
+
+		if (C_CMSPAR(tty))
+			new_lcr |= UART_LCR_SPAR;
+	}
+
+	if (C_CSTOPB(tty))
+		new_lcr |= UART_LCR_STOP;
+
+	switch (C_CSIZE(tty)) {
+	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(port, LINE_CONTROL_REGISTER, new_lcr);
+	if (status < 0)
+		dev_err(&port->dev, "%s set error: %d\n", __func__, 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)
@@ -278,6 +403,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_init(port);
+	if (result) {
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+		return result;
+	}
+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);
-- 
1.9.1

[PATCH V5 5/8] USB: f81232: implement MCR/MSR function

[Peter Hung]

This patch implement relative MCR/MSR function, such like
tiocmget()/tiocmset()/dtr_rts().

The f81232_set_mctrl() replace set_control_lines() to do MCR control
so we clean-up the set_control_lines() function.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 98 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 77 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 06d1eb0..e1cdf42 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -44,6 +44,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
 #define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
 #define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
+#define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01
@@ -156,6 +157,50 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	wake_up_interruptible(&port->port.delta_msr_wait);
 }
 
+static int f81232_set_mctrl(struct usb_serial_port *port,
+					   unsigned int set, unsigned int clear)
+{
+	u8 urb_value;
+	int status;
+	unsigned long flags;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+		return 0;	/* no change */
+
+	/* 'set' takes precedence over 'clear' */
+	clear &= ~set;
+
+	/* force enable interrupt with OUT2 */
+	urb_value = UART_MCR_OUT2 | priv->line_control;
+
+	if (clear & TIOCM_DTR)
+		urb_value &= ~UART_MCR_DTR;
+
+	if (clear & TIOCM_RTS)
+		urb_value &= ~UART_MCR_RTS;
+
+	if (set & TIOCM_DTR)
+		urb_value |= UART_MCR_DTR;
+
+	if (set & TIOCM_RTS)
+		urb_value |= UART_MCR_RTS;
+
+	dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+			urb_value, priv->line_control);
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status < 0) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+	} else {
+		spin_lock_irqsave(&priv->lock, flags);
+		priv->line_control = urb_value;
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+
+	return status;
+}
+
 static void f81232_update_modem_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
@@ -267,12 +312,6 @@ static void f81232_process_read_urb(struct urb *urb)
 	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)
 {
 	/* FIXME - Stubbed out for now */
@@ -388,15 +427,41 @@ static void f81232_set_termios(struct tty_struct *tty,
 
 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;
+	u8 mcr, msr;
+
+	/* force get current MSR changed state */
+	f81232_read_msr(port);
+
+	spin_lock_irqsave(&port_priv->lock, flags);
+	mcr = port_priv->line_control;
+	msr = port_priv->modem_status;
+	spin_unlock_irqrestore(&port_priv->lock, flags);
+
+	r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+		(mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+		(msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+		(msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+		(msr & UART_MSR_RI ? TIOCM_RI : 0) |
+		(msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+	return r;
 }
 
 static int f81232_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	int status;
+	struct usb_serial_port *port = tty->driver_data;
+
+	status = f81232_set_mctrl(port, set, clear);
+	if (status < 0)
+		return usb_translate_errors(status);
+	else
+		return 0;
 }
 
 static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
@@ -437,19 +502,10 @@ 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);
+		f81232_set_mctrl(port, 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);
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static int f81232_carrier_raised(struct usb_serial_port *port)
-- 
1.9.1

[PATCH V5 6/8] USB: f81232: clarify f81232_ioctl()

[Peter Hung]

We extract TIOCGSERIAL section in f81232_ioctl() to f81232_get_serial_info()
to make it clarify

The f81232_set_mctrl() replace set_control_lines() to do MCR control
so we clean-up the set_control_lines() function.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index e1cdf42..4dddb44 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -516,24 +516,36 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
 	return 0;
 }
 
+static int f81232_get_serial_info(struct usb_serial_port *port,
+		unsigned long arg)
+{
+	struct serial_struct ser;
+
+	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 = 115200;
+
+	if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int f81232_ioctl(struct tty_struct *tty,
 			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;
-		ser.line = port->minor;
-		ser.port = port->port_number;
-		ser.baud_base = 460800;
-
-		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
-			return -EFAULT;
-
-		return 0;
+		return f81232_get_serial_info(port, arg);
 	default:
 		break;
 	}
-- 
1.9.1

[PATCH V5 7/8] USB: f81232: fix error in f81232_carrier_raised()

[Peter Hung]

It's should compared with UART_MSR_DCD, not UART_DCD.
also we clean-up some non-used define to avoid impropriety use.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 4dddb44..07abf0c 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -47,20 +47,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
-#define CONTROL_DTR			0x01
-#define CONTROL_RTS			0x02
-
-#define UART_STATE			0x08
-#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
-
 struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
@@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 static int f81232_carrier_raised(struct usb_serial_port *port)
 {
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+	if (priv->modem_status & UART_MSR_DCD)
 		return 1;
 	return 0;
 }
-- 
1.9.1

[PATCH V5 8/8] USB: f81232: modify/add author

[Peter Hung]

Add me to co-author and fix no '>' in greg kh's email

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 07abf0c..8799b66 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -608,5 +608,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
 module_usb_serial_driver(serial_drivers, id_table);
 
 MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
-MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@linuxfoundation.org");
+MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@linuxfoundation.org>");
+MODULE_AUTHOR("Peter Hong <peter_hong@fintek.com.tw>");
 MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH V5 6/8] USB: f81232: clarify f81232_ioctl()

[Sergei Shtylyov]

Hello.

On 2/6/2015 12:46 PM, Peter Hung wrote:

> We extract TIOCGSERIAL section in f81232_ioctl() to f81232_get_serial_info()
> to make it clarify

    You're also changing 'ser.baud_rate' from 460800 to 115200. And explicitly 
overriding some previously initialized to 0 fields.

> The f81232_set_mctrl() replace set_control_lines() to do MCR control
> so we clean-up the set_control_lines() function.

    I don't see where are you doing this...

> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>   drivers/usb/serial/f81232.c | 34 +++++++++++++++++++++++-----------
>   1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index e1cdf42..4dddb44 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -516,24 +516,36 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
>   	return 0;
>   }
>
> +static int f81232_get_serial_info(struct usb_serial_port *port,
> +		unsigned long arg)
> +{
> +	struct serial_struct ser;
> +
> +	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 = 115200;
> +
> +	if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
>   static int f81232_ioctl(struct tty_struct *tty,
>   			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;
> -		ser.line = port->minor;
> -		ser.port = port->port_number;
> -		ser.baud_base = 460800;
> -
> -		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
> -			return -EFAULT;
> -
> -		return 0;
> +		return f81232_get_serial_info(port, arg);
>   	default:
>   		break;
>   	}

WBR, Sergei

Re: [PATCH V5 6/8] USB: f81232: clarify f81232_ioctl()

[Peter Hung]

Hello,

Sergei Shtylyov 於 2015/2/6 下午 08:21 寫道:
>> We extract TIOCGSERIAL section in f81232_ioctl() to
>> f81232_get_serial_info()
>> to make it clarify
>
>     You're also changing 'ser.baud_rate' from 460800 to 115200. And
> explicitly overriding some previously initialized to 0 fields.

F81232 max baudrate is only 115200bps, so I set it for
1.8432MHz/16 = 115200.

We had add some closing time referenced from serial_core.c. The default
value is:

port->close_delay     = HZ / 2;	/* .5 seconds */
port->closing_wait    = 30 * HZ;/* 30 seconds */

We had increasing close_delay about 10x to

port->close_delay     = 5 * HZ ;

>
>> The f81232_set_mctrl() replace set_control_lines() to do MCR control
>> so we clean-up the set_control_lines() function.
>
>     I don't see where are you doing this...
>

This text is my patch V5 5/8 second section. I had wrong operation of 
copy & paste. It's doesn't need for this patch, sorry for it.

-- 
With Best Regards,
Peter Hung

Re: [PATCH V5 6/8] USB: f81232: clarify f81232_ioctl()

[Johan Hovold]

On Mon, Feb 09, 2015 at 02:59:12PM +0800, Peter Hung wrote:
> Hello,
> 
> Sergei Shtylyov 於 2015/2/6 下午 08:21 寫道:
> >> We extract TIOCGSERIAL section in f81232_ioctl() to
> >> f81232_get_serial_info()
> >> to make it clarify
> >
> >     You're also changing 'ser.baud_rate' from 460800 to 115200. And
> > explicitly overriding some previously initialized to 0 fields.
> 
> F81232 max baudrate is only 115200bps, so I set it for
> 1.8432MHz/16 = 115200.
> 
> We had add some closing time referenced from serial_core.c. The default
> value is:
> 
> port->close_delay     = HZ / 2;	/* .5 seconds */
> port->closing_wait    = 30 * HZ;/* 30 seconds */
> 
> We had increasing close_delay about 10x to
> 
> port->close_delay     = 5 * HZ ;

You're never changing anything, you're just reporting an incorrect value
to userspace here.

The value you should be returning is
jiffies_to_msecs(port->port.closing_wait) / 10, unless the value is
ASYNC_CLOSING_WAIT_NONE in which case you simply return that, and
similarly for close_delay.

> >> The f81232_set_mctrl() replace set_control_lines() to do MCR control
> >> so we clean-up the set_control_lines() function.
> >
> >     I don't see where are you doing this...
> >
> 
> This text is my patch V5 5/8 second section. I had wrong operation of 
> copy & paste. It's doesn't need for this patch, sorry for it.

Make sure to update the commit log for the next revision so that it
describes what you actually do.

I will probably not have time to review this version this week I'm
afraid.

Johan

Re: [PATCH V5 6/8] USB: f81232: clarify f81232_ioctl()

[Peter Hung]

Hello,

Johan Hovold 於 2015/2/9 下午 04:42 寫道:

> The value you should be returning is
> jiffies_to_msecs(port->port.closing_wait) / 10, unless the value is
> ASYNC_CLOSING_WAIT_NONE in which case you simply return that, and
> similarly for close_delay.

I'll try to fix it, or reuse default value next version.

> Make sure to update the commit log for the next revision so that it
> describes what you actually do.
>
> I will probably not have time to review this version this week I'm
> afraid.

OK, I'll review it again, fix and resend it after Chinese New Year (2/23+).

Thanks all seniors.

-- 
With Best Regards,
Peter Hung

Re: [PATCH V5 3/8] USB: f81232: implement RX bulk-in ep

[Johan Hovold]

On Fri, Feb 06, 2015 at 05:46:49PM +0800, Peter Hung wrote:
> The F81232 bulk-in is RX data + LSR channel, data format is
> [LSR+Data][LSR+Data]..... , We had reimplemented in this patch.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 68 +++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index ec4609d..9ea498a 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -185,44 +185,46 @@ exit:
>  static void f81232_process_read_urb(struct urb *urb)
>  {
>  	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;
> +	char tty_flag;
>  	int i;
>  
> -	/* update line status */
> -	spin_lock_irqsave(&priv->lock, flags);
> -	line_status = priv->modem_status;
> -	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
> -	spin_unlock_irqrestore(&priv->lock, flags);
> -
> -	if (!urb->actual_length)
> +	if (urb->actual_length < 2)
>  		return;
>  
> -	/* break takes precedence over parity, */
> -	/* which takes precedence over framing errors */
> -	if (line_status & UART_BREAK_ERROR)
> -		tty_flag = TTY_BREAK;
> -	else if (line_status & UART_PARITY_ERROR)
> -		tty_flag = TTY_PARITY;
> -	else if (line_status & UART_FRAME_ERROR)
> -		tty_flag = TTY_FRAME;
> -	dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> -
> -	/* overrun is special, not associated with a char */
> -	if (line_status & UART_OVERRUN_ERROR)
> -		tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> -
> -	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);
> +	/* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
> +
> +	for (i = 0 ; i < urb->actual_length ; i += 2) {

No space after ;.

> +		tty_flag = TTY_NORMAL;
> +
> +		if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {

Always use spaces around binary operators such as +, but in this case
I think it would be preferable to introduce a temporary variable u8 lsr,
which you update at the beginning of the loop construct.


> +			if (data[i+0] & UART_LSR_BI) {
> +				tty_flag = TTY_BREAK;
> +				port->icount.brk++;
> +				usb_serial_handle_break(port);
> +			} else if (data[i+0] & UART_LSR_PE) {
> +				tty_flag = TTY_PARITY;
> +				port->icount.parity++;
> +			} else if (data[i+0] & UART_LSR_FE) {
> +				tty_flag = TTY_FRAME;
> +				port->icount.frame++;
> +			}
> +
> +			if (data[0] & UART_LSR_OE) {

You probably want data[i] (lsr) here as well.

> +				port->icount.overrun++;
> +				tty_insert_flip_char(&port->port, 0,
> +					TTY_OVERRUN);
> +			}
> +		}
> +
> +		if (port->port.console && port->sysrq) {
> +			if (!usb_serial_handle_sysrq_char(port, data[i+1]))

Spaces around + again.

> +				tty_insert_flip_char(&port->port, data[i+1],
> +					tty_flag);
> +		} else {
> +			tty_insert_flip_string_fixed_flag(&port->port,
> +				&data[i+1], tty_flag, 1);
> +		}

Use tty_insert_flip_char in both branches. In fact, you could rewrite
this as:

		if (port->port.console && port->sysrq) {
			if (usb_serial_handle_sysrq_char(port, data[i + 1]))
				continue;
		}

		tty_insert_flip_char(&port->port, data[i + 1], tty_flag);
		
>  	}
>  
>  	tty_flip_buffer_push(&port->port);

I'll try to look at the rest of the series soon.

Johan

Re: [PATCH V5 3/8] USB: f81232: implement RX bulk-in ep

[Johan Hovold]

On Mon, Feb 16, 2015 at 07:59:45PM +0700, Johan Hovold wrote:
> On Fri, Feb 06, 2015 at 05:46:49PM +0800, Peter Hung wrote:

> I'll try to look at the rest of the series soon.

Managed to comment on v5 rather than v6, but looks like comments on this
patch still apply.

Johan