Skip to contentSkip to conversation

USB: f81232: V7 patches

PATCH Superseded Peter Hung 2 participants 18 messages Feb 24, 2015 – Feb 25, 2015

Subsystems (2)
USB USB serial

v7 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Peter Hung

author
#0 lore raw

v7 cover USB: f81232: V7 patches

This series patch V7 is changed from V6 as following:

1. The buffer of usb_control_msg() in set_register()/get_register() are change from local variable to kmalloc(). (PATCH V7 02/11)
2. Change all set_register()/get_register() return value 0 is success, otherwise are failed. ( return 0 of usb_control_msg() treat as -EIO, PATCH V7 02/11)
3. tty_port_tty_get() called only when DCD has changed. (PATCH V7 05/11)
4. remove likely()/unlikely() branch prediction.
5. Implement DTR/RTS control when baudrate B0. We drop DTR/RTS when B0, otherwise enable it. (PATCH V7 08/11)
6. Change private struct line_control to modem_control with meanful. (PATCH V7 06/11)
7. We confirmd MSR strange delta value is not locking-issue. The issue maybe reproduce with set MCR & get MSR before IIR notice with MSR changed.

V6 (old change):

1. transform all function not to use private data as parameter, using usb_serial_port instead.
2. process_read_urb() add process of Break/FrameError/ParityError/OE. (patch: 03/10)
3. fix calc_baud_divisor() will cause divide by zero with B0. (patch: 04/10)
4. Some init step we extract it from set_termios() to f81232_port_init() and run it when open port only. (patch: 04/10)
5. We'll force re-read msr in tiocmget() because the IIR with MSR change maybe delay received. (patch: 05/10)
6. fix MSR status bits changed but delta bits is 0 will cause read serial port malfunctional with update port status. (patch: 08/10)
7. Add MSR change statistic when MSR has been read. (patch: 09/10)
8. clarify a lot of code about Johan suggested.

Peter Hung (11):
USB: f81232: rename private struct member name
USB: f81232: add preparatory functions
USB: f81232: implement RX bulk-in EP
USB: f81232: change lock mechanism
USB: f81232: implement read IIR/MSR with endpoint
USB: f81232: implement MCR/MSR function
USB: f81232: implement port_enable function
USB: f81232: implement set_termios()
USB: f81232: clarify f81232_ioctl() and fix
USB: f81232: cleanup non-used define
USB: f81232: modify/add author

 drivers/usb/serial/f81232.c | 554 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 458 insertions(+), 96 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v7 1/11 USB: f81232: rename private struct member name

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

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/serial/f81232.c +4 -4
index c5dc233..669a2f2 100644
@@ -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;
 }
Signature · 1 line
1.9.1
author
#4 lore raw

PATCH v7 3/11 USB: f81232: implement RX bulk-in EP

In reply to Peter Hung

The F81232 bulk-in is RX data + LSR channel, data format is [LSR+Data][LSR+Data]..... , We had implemented in f81232_process_read_urb().

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 69 +++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)
drivers/usb/serial/f81232.c +36 -33
index 1f29b95..419e2d6 100644
@@ -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) },

@@ -183,44 +184,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;
+	u8 lsr;
 
-	/* 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;
+		lsr = data[i + 0];
+
+		if (lsr & UART_LSR_BRK_ERROR_BITS) {
+			if (lsr & UART_LSR_BI) {
+				tty_flag = TTY_BREAK;
+				port->icount.brk++;
+				usb_serial_handle_break(port);
+			} else if (lsr & UART_LSR_PE) {
+				tty_flag = TTY_PARITY;
+				port->icount.parity++;
+			} else if (lsr & UART_LSR_FE) {
+				tty_flag = TTY_FRAME;
+				port->icount.frame++;
+			}
+
+			if (lsr & 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]))
+				continue;
+		}
+
+		tty_insert_flip_char(&port->port, data[i + 1], tty_flag);
 	}
 
 	tty_flip_buffer_push(&port->port);
Signature · 1 line
1.9.1
author
#7 lore raw

PATCH v7 5/11 USB: f81232: implement read IIR/MSR with endpoint

In reply to 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() function.

We also confirmd MSR strange delta value is not locking-issue. The issue is set MCR & get MSR before IIR notice with MSR changed (Loopback only).

When we use RS232 loopback, assume doing RTS change will cause CTS change, DTR change will cause DCD/DSR change too.

Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of MSR(delta) maybe not changed when set & get MCR rapidly.

So we add more check not only UART_MSR_ANY_DELTA but also with comparing DCD/RI/DSR/CTS change with old value. Because of the state bit is always correct, we direct save msr when read.

The following step to reproduce this problem with while loop step 1~4:
1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 8 deletions(-)
drivers/usb/serial/f81232.c +98 -8
index bf072fe..339be30 100644
@@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_GET_REGISTER	0xc0
 #define F81232_SET_REGISTER	0x40
 
+#define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 

@@ -54,6 +57,8 @@ struct f81232_private {
 	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)

@@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
 	kfree(tmp);
 	return status;
 }
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	u8 current_msr, prev_msr;
+	u8 msr_mask = ~UART_MSR_ANY_DELTA;
+	u8 msr_changed_bit;
+	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) {
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		return;
+	}
+
+	/*
+	*  The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
+	*  maybe not change when set MCR & get MSR rapidly.
+	*
+	*  So we add more check with comparing DCD/RI/DSR/CTS
+	*  change. and direct save msr when read.
+	*/
+
+	mutex_lock(&priv->lock);
+	prev_msr = priv->modem_status;
+	priv->modem_status = current_msr;
+	mutex_unlock(&priv->lock);
+
+	if (!(current_msr & UART_MSR_ANY_DELTA) &&
+		!((prev_msr ^ current_msr) & msr_mask))
+		return;
+
+	/* find checked delta bits set */
+	msr_changed_bit =
+		(current_msr & UART_MSR_ANY_DELTA) << 4;
+
+	/* append with not delta but changed bits */
+	msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
+
+	if (msr_changed_bit & UART_MSR_CTS)
+		port->icount.cts++;
+	if (msr_changed_bit & UART_MSR_DSR)
+		port->icount.dsr++;
+	if (msr_changed_bit & UART_MSR_RI)
+		port->icount.rng++;
+	if (msr_changed_bit & UART_MSR_DCD) {
+
+		port->icount.dcd++;
+		tty = tty_port_tty_get(&port->port);
+		if (tty) {
+
+			usb_serial_handle_dcd_change(port, tty,
+					current_msr & UART_MSR_DCD);
+
+			tty_kref_put(tty);
+		}
+	}
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
-				      unsigned int actual_length)
+				      size_t 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: %02x\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: %02x\n", data[0]);
+		break;
+	}
 }
 
 static void f81232_read_int_callback(struct urb *urb)

@@ -351,6 +431,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;

@@ -360,10 +448,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	mutex_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;
 }
Signature · 1 line
1.9.1
author
#11 lore raw

PATCH v7 7/11 USB: f81232: implement port_enable function

In reply to Peter Hung

We put FCR/IER initial step to f81232_port_enable(). When port is open, it set MSR interrupt on. Otherwise set it off.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
drivers/usb/serial/f81232.c +39 -0
index 21f606f..f5c9060 100644
@@ -37,6 +37,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
 #define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 

@@ -367,6 +369,33 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static int f81232_port_enable(struct usb_serial_port *port, int enable)
+{
+	u8 data = 0;
+	int status;
+
+	/* 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);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set FCR: %d\n", __func__, status);
+		return status;
+	}
+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	data = enable ? UART_IER_MSI : 0;
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set IER: %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)
 {

@@ -418,6 +447,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_enable(port, 1);
+	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);

@@ -440,6 +475,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void f81232_close(struct usb_serial_port *port)
 {
+	int result = f81232_port_enable(port, 0);
+	if (result)
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 }
Signature · 1 line
1.9.1
author
#15 lore raw

PATCH v7 9/11 USB: f81232: clarify f81232_ioctl() and fix

In reply to Peter Hung

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

Also we fix device type from 16654 to 16550A, and set it's baud_base to 115200 (1.8432MHz/16).

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
drivers/usb/serial/f81232.c +19 -11
index 0c96b9a..3b0da70 100644
@@ -603,24 +603,32 @@ 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.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;
 	}
Signature · 1 line
1.9.1
author
#17 lore raw

PATCH v7 11/11 USB: f81232: modify/add author

In reply to Peter Hung

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

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/serial/f81232.c +2 -1
index ee9a2a2..23f0a17 100644
@@ -691,5 +691,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 <[email protected]");
+MODULE_AUTHOR("Greg Kroah-Hartman <[email protected]>");
+MODULE_AUTHOR("Peter Hong <[email protected]>");
 MODULE_LICENSE("GPL v2");
Signature · 1 line
1.9.1
author
#16 lore raw

PATCH v7 10/11 USB: f81232: cleanup non-used define

In reply to Peter Hung

We remove non-used define in this patch to avoid wrong usage.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 14 --------------
 1 file changed, 14 deletions(-)
drivers/usb/serial/f81232.c +0 -14
index 3b0da70..ee9a2a2 100644
@@ -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 {
 	struct mutex lock;
 	u8 modem_control;
Signature · 1 line
1.9.1
author
#13 lore raw

PATCH v7 8/11 USB: f81232: implement set_termios()

In reply to 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 in f81232_set_termios().

This patch also implement DTR/RTS control when baudrate B0. We drop DTR/RTS when B0, otherwise enable it.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 4 deletions(-)
drivers/usb/serial/f81232.c +102 -4
index f5c9060..0c96b9a 100644
@@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE	115200
+
 /* 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_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 

@@ -64,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)
 {
 	int status;

@@ -369,6 +382,52 @@ 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, lcr;
+	int status = 0;
+
+	if (!baudrate)
+		return;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_get_register(port, LINE_CONTROL_REGISTER,
+			 &lcr); /* get LCR */
+	if (status) {
+		dev_err(&port->dev, "%s failed to get LCR: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 lcr | UART_LCR_DLAB); /* Enable DLAB */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			lcr & ~UART_LCR_DLAB);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+}
+
 static int f81232_port_enable(struct usb_serial_port *port, int enable)
 {
 	u8 data = 0;

@@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable)
 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;
 
-	/* Do the real work here... */
-	if (old_termios)
-		tty_termios_copy_hw(&tty->termios, old_termios);
+	if (C_BAUD(tty) == B0)
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
+	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
+
+	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)
+		dev_err(&port->dev, "%s failed to set LCR: %d\n", __func__, status);
+
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)
Signature · 1 line
1.9.1
author
#9 lore raw

PATCH v7 6/11 USB: f81232: implement MCR/MSR function

In reply to Peter Hung

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

original f81232_carrier_raised() compared with wrong value UART_DCD. It's should compared with UART_MSR_DCD.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 103 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 81 insertions(+), 22 deletions(-)
drivers/usb/serial/f81232.c +81 -22
index 339be30..21f606f 100644
@@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

@@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct f81232_private {
 	struct mutex lock;
-	u8 line_control;
+	u8 modem_control;
 	u8 modem_status;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;

@@ -198,6 +199,52 @@ 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;
+	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 */
+	mutex_lock(&priv->lock);
+	urb_value = UART_MCR_OUT2 | priv->modem_control;
+	mutex_unlock(&priv->lock);
+
+	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->modem_control);
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+		return status;
+	} else {
+		mutex_lock(&priv->lock);
+		priv->modem_control = urb_value;
+		mutex_unlock(&priv->lock);
+	}
+
+	return 0;
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      size_t actual_length)

@@ -309,12 +356,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 */

@@ -342,15 +383,35 @@ 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);
+	u8 mcr, msr;
+
+	/* force get current MSR changed state */
+	f81232_read_msr(port);
+
+	mutex_lock(&port_priv->lock);
+	mcr = port_priv->modem_control;
+	msr = port_priv->modem_status;
+	mutex_unlock(&port_priv->lock);
+
+	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;
+	struct usb_serial_port *port = tty->driver_data;
+
+	return f81232_set_mctrl(port, set, clear);
 }
 
 static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)

@@ -385,24 +446,22 @@ 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);
-	u8 control;
-
-	mutex_lock(&priv->lock);
-	/* 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;
-	mutex_unlock(&priv->lock);
-	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)
 {
+	u8 msr;
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+
+	mutex_lock(&priv->lock);
+	msr = priv->modem_status;
+	mutex_unlock(&priv->lock);
+
+	if (msr & UART_MSR_DCD)
 		return 1;
 	return 0;
 }
Signature · 1 line
1.9.1
author
#6 lore raw

PATCH v7 4/11 USB: f81232: change lock mechanism

In reply to Peter Hung

The original driver lock with spin_lock_irqsave()/spin_unlock_irqrestore() because of it's maybe used in interrupt context f81232_process_read_urb().

We had remove it from previous patch "implement RX bulk-in EP", so we can change it from busying loop spin_lock to sleepable mutex_lock.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/usb/serial/f81232.c +5 -6
index 419e2d6..bf072fe 100644
@@ -19,7 +19,7 @@
 #include <linux/serial.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>

@@ -51,7 +51,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_CTS			0x80
 
 struct f81232_private {
-	spinlock_t lock;
+	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
 };

@@ -306,17 +306,16 @@ 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);
+	mutex_lock(&priv->lock);
 	/* Change DTR and RTS */
 	if (on)
 		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
 	else
 		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
 	control = priv->line_control;
-	spin_unlock_irqrestore(&priv->lock, flags);
+	mutex_unlock(&priv->lock);
 	set_control_lines(port->serial->dev, control);
 }
 

@@ -360,7 +359,7 @@ static int f81232_port_probe(struct usb_serial_port *port)
 	if (!priv)
 		return -ENOMEM;
 
-	spin_lock_init(&priv->lock);
+	mutex_init(&priv->lock);
 
 	usb_set_serial_port_data(port, priv);
Signature · 1 line
1.9.1
author
#2 lore raw

PATCH v7 2/11 USB: f81232: add preparatory functions

In reply to Peter Hung

We add f81232_get_register()/f81232_set_register() and necessary defines for future patch with communication with F81232 USB control endpoint.

Because of this is a preparatory patch, there are unused function warning with compiling. The functions will used with following patches.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
drivers/usb/serial/f81232.c +79 -0
index 669a2f2..1f29b95 100644
@@ -30,6 +30,11 @@ 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 F81232_SET_REGISTER	0x40
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 

@@ -50,6 +55,80 @@ struct f81232_private {
 	u8 modem_status;
 };
 
+static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	if (!data)
+		return -EFAULT;
+
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	status = usb_control_msg(dev,
+				usb_rcvctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_GET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_GET_TIMEOUT);
+	if (status <= 0) {
+		/* show something with failed */
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else {
+		status = 0; /* on success */
+		memcpy((void*) data, (void*) tmp, sizeof(u8));
+	}
+
+	kfree(tmp);
+	return status;
+}
+
+static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	memcpy((void*) tmp, (void*) &data, sizeof(u8));
+
+	status = usb_control_msg(dev,
+				usb_sndctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_SET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_SET_TIMEOUT);
+	if (status <= 0) {
+		/* show something with failed */
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else
+		status = 0; /* on success */
+
+	kfree(tmp);
+	return status;
+}
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
Signature · 1 line
1.9.1
maintainer
#8 lore raw

Re: [PATCH V7 05/11] USB: f81232: implement read IIR/MSR with endpoint

In reply to Peter Hung
Quoted 6 lines · 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() function.

We also confirmd MSR strange delta value is not locking-issue. The issue is set MCR & get MSR before IIR notice with MSR changed (Loopback only).

But you are still not doing the actual register access under the mutex, so this still sounds like it could be due to the shadow registers not matching the hardware registers.

You need to update dtr_rts as well.

Quoted 62 lines

When we use RS232 loopback, assume doing RTS change will cause CTS change, DTR change will cause DCD/DSR change too.

Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of MSR(delta) maybe not changed when set & get MCR rapidly.

So we add more check not only UART_MSR_ANY_DELTA but also with comparing DCD/RI/DSR/CTS change with old value. Because of the state bit is always correct, we direct save msr when read.

The following step to reproduce this problem with while loop step 1~4:
1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 8 deletions(-)
drivers/usb/serial/f81232.c +19 -0
index bf072fe..339be30 100644
@@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_GET_REGISTER	0xc0
 #define F81232_SET_REGISTER	0x40
 
+#define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 

@@ -54,6 +57,8 @@ struct f81232_private {
 	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)

@@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
 	kfree(tmp);
 	return status;
 }
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	u8 current_msr, prev_msr;
+	u8 msr_mask = ~UART_MSR_ANY_DELTA;
+	u8 msr_changed_bit;
+	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) {
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);

drivers/usb/serial/f81232.c static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data) · line 161

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+		return;
+	}
+
+	/*
+	*  The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
+	*  maybe not change when set MCR & get MSR rapidly.
+	*
+	*  So we add more check with comparing DCD/RI/DSR/CTS
+	*  change. and direct save msr when read.
+	*/

The asterisks should be aligned.

Quoted 15 lines
drivers/usb/serial/f81232.c +15 -0
+
+	mutex_lock(&priv->lock);
+	prev_msr = priv->modem_status;
+	priv->modem_status = current_msr;
+	mutex_unlock(&priv->lock);
+
+	if (!(current_msr & UART_MSR_ANY_DELTA) &&
+		!((prev_msr ^ current_msr) & msr_mask))
+		return;
+
+	/* find checked delta bits set */
+	msr_changed_bit =
+		(current_msr & UART_MSR_ANY_DELTA) << 4;
+
+	/* append with not delta but changed bits */

drivers/usb/serial/f81232.c static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data) · line 186

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+	msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
+
+	if (msr_changed_bit & UART_MSR_CTS)
+		port->icount.cts++;
+	if (msr_changed_bit & UART_MSR_DSR)
+		port->icount.dsr++;
+	if (msr_changed_bit & UART_MSR_RI)
+		port->icount.rng++;
+	if (msr_changed_bit & UART_MSR_DCD) {
+

No empty line.

drivers/usb/serial/f81232.c static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data) · line 190

Quoted 4 lines
drivers/usb/serial/f81232.c +4 -0
+		port->icount.dcd++;
+		tty = tty_port_tty_get(&port->port);
+		if (tty) {
+

No empty line.

drivers/usb/serial/f81232.c static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data) · line 199

Quoted 9 lines
drivers/usb/serial/f81232.c +9 -0
+			usb_serial_handle_dcd_change(port, tty,
+					current_msr & UART_MSR_DCD);
+
+			tty_kref_put(tty);
+		}
+	}
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}

Johan

maintainer
#5 lore raw

Re: [PATCH V7 03/11] USB: f81232: implement RX bulk-in EP

In reply to Peter Hung
Quoted 21 lines · Peter Hung

The F81232 bulk-in is RX data + LSR channel, data format is [LSR+Data][LSR+Data]..... , We had implemented in f81232_process_read_urb().

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 69 +++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)
drivers/usb/serial/f81232.c +1 -0
index 1f29b95..419e2d6 100644
@@ -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) },

drivers/usb/serial/f81232.c exit: · line 189

Quoted 10 lines
drivers/usb/serial/f81232.c +1 -4
@@ -183,44 +184,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;

Use an unsigned type for i.

drivers/usb/serial/f81232.c exit: · line 196

Quoted 6 lines
drivers/usb/serial/f81232.c +1 -4
+	u8 lsr;
 
-	/* 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);
Quoted 20 lines
drivers/usb/serial/f81232.c +1 -17
-
-	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) {

drivers/usb/serial/f81232.c exit: · line 197

Quoted 10 lines
drivers/usb/serial/f81232.c +3 -7
-		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) {

Again, what if the length is not a multiple of 2?

drivers/usb/serial/f81232.c exit: · line 199

Quoted 2 lines
drivers/usb/serial/f81232.c +2 -0
+		tty_flag = TTY_NORMAL;
+		lsr = data[i + 0];

Just use data[i] here.

Quoted 8 lines
drivers/usb/serial/f81232.c +8 -0
+
+		if (lsr & UART_LSR_BRK_ERROR_BITS) {
+			if (lsr & UART_LSR_BI) {
+				tty_flag = TTY_BREAK;
+				port->icount.brk++;
+				usb_serial_handle_break(port);
+			} else if (lsr & UART_LSR_PE) {
+				tty_flag = TTY_PARITY;

drivers/usb/serial/f81232.c exit: · line 217

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+				port->icount.parity++;
+			} else if (lsr & UART_LSR_FE) {
+				tty_flag = TTY_FRAME;
+				port->icount.frame++;
+			}
+
+			if (lsr & UART_LSR_OE) {
+				port->icount.overrun++;
+				tty_insert_flip_char(&port->port, 0,
+					TTY_OVERRUN);

Use at least two tabs indentation for continuation lines.

drivers/usb/serial/f81232.c exit: · line 229

Quoted 12 lines
drivers/usb/serial/f81232.c +9 -0
+			}
+		}
+
+		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);

Looks good now, otherwise.

Thanks,
Johan

maintainer
#12 lore raw

Re: [PATCH V7 07/11] USB: f81232: implement port_enable function

In reply to Peter Hung
Quoted 22 lines · Peter Hung

We put FCR/IER initial step to f81232_port_enable(). When port is open, it set MSR interrupt on. Otherwise set it off.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
drivers/usb/serial/f81232.c +2 -0
index 21f606f..f5c9060 100644
@@ -37,6 +37,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
 #define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)

drivers/usb/serial/f81232.c static void f81232_break_ctl(struct tty_struct *tty, int break_state) · line 372

Quoted 4 lines
drivers/usb/serial/f81232.c +1 -0
@@ -367,6 +369,33 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static int f81232_port_enable(struct usb_serial_port *port, int enable)

Split this in two functions, enable and disable, since you don't need to set FCR on close.

drivers/usb/serial/f81232.c static void f81232_break_ctl(struct tty_struct *tty, int break_state) · line 379

Quoted 7 lines
drivers/usb/serial/f81232.c +7 -0
+{
+	u8 data = 0;
+	int status;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+			| UART_FCR_CLEAR_XMIT;

Put the final | before the line break.

Quoted 23 lines
drivers/usb/serial/f81232.c +19 -0
+
+	status = f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set FCR: %d\n", __func__, status);
+		return status;
+	}
+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	data = enable ? UART_IER_MSI : 0;
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set IER: %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)
 {

drivers/usb/serial/f81232.c static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port) · line 453

Quoted 7 lines
drivers/usb/serial/f81232.c +4 -0
@@ -418,6 +447,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_enable(port, 1);
+	if (result) {
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+		return result;

You already log the error in port_enable.

drivers/usb/serial/f81232.c static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port) · line 478

Quoted 10 lines
drivers/usb/serial/f81232.c +3 -0
+	}
+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);

@@ -440,6 +475,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void f81232_close(struct usb_serial_port *port)
 {
+	int result = f81232_port_enable(port, 0);

Separate the declaration from the function call.

drivers/usb/serial/f81232.c static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port) · line 481

Quoted 3 lines
drivers/usb/serial/f81232.c +3 -0
+	if (result)
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+

Already logged as well.

drivers/usb/serial/f81232.c static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port) · line 484

Quoted 3 lines
drivers/usb/serial/f81232.c +0 -0
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 }

Johan

maintainer
#14 lore raw

Re: [PATCH V7 08/11] USB: f81232: implement set_termios()

In reply to Peter Hung
Quoted 37 lines · 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 in f81232_set_termios().

This patch also implement DTR/RTS control when baudrate B0. We drop DTR/RTS when B0, otherwise enable it.

Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 4 deletions(-)
drivers/usb/serial/f81232.c +5 -0
index f5c9060..0c96b9a 100644
@@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE	115200
+
 /* 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_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)

drivers/usb/serial/f81232.c struct f81232_private { · line 75

Quoted 7 lines
drivers/usb/serial/f81232.c +4 -0
@@ -64,6 +69,14 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(u32 baudrate)
+{
+	if (!baudrate)
+		return 0;

Check no longer needed since you added the check at the call site. Just add a comment. In fact it look like you can get rid of this function now.

Quoted 46 lines
drivers/usb/serial/f81232.c +39 -0
+	else
+		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
 {
 	int status;

@@ -369,6 +382,52 @@ 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, lcr;
+	int status = 0;
+
+	if (!baudrate)
+		return;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_get_register(port, LINE_CONTROL_REGISTER,
+			 &lcr); /* get LCR */
+	if (status) {
+		dev_err(&port->dev, "%s failed to get LCR: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 lcr | UART_LCR_DLAB); /* Enable DLAB */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",

drivers/usb/serial/f81232.c static void f81232_break_ctl(struct tty_struct *tty, int break_state) · line 429

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			lcr & ~UART_LCR_DLAB);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+}

Again, what about the error handling? No need to continue setting the other registers should the first one fail, right? And you always want to restore LCR to its previous value.

Quoted 16 lines
drivers/usb/serial/f81232.c +4 -1
+
 static int f81232_port_enable(struct usb_serial_port *port, int enable)
 {
 	u8 data = 0;

@@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable)
 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;

drivers/usb/serial/f81232.c static int f81232_port_enable(struct usb_serial_port *port, int enable) · line 474

Quoted 10 lines
drivers/usb/serial/f81232.c +6 -3
 
-	/* Do the real work here... */
-	if (old_termios)
-		tty_termios_copy_hw(&tty->termios, old_termios);
+	if (C_BAUD(tty) == B0)
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
+	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
+
+	f81232_set_baudrate(port, tty_get_baud_rate(tty));

Check for B0 here instead of in the helper.

And what about unsupported baudrates (e.g. > 115200 baud)? You should return the baudrate actually used in that case using tty_termios_encode_baud_rate.

Johan

maintainer
#10 lore raw

Re: [PATCH V7 06/11] USB: f81232: implement MCR/MSR function

In reply to Peter Hung
Quoted 7 lines · Peter Hung

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

original f81232_carrier_raised() compared with wrong value UART_DCD. It's should compared with UART_MSR_DCD.

Signed-off-by Peter Hung <[email protected]>

This patch as well as a few of the other patches in v7 now have checkpatch warnings and errors.

Quoted 36 lines
---
 drivers/usb/serial/f81232.c | 103 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 81 insertions(+), 22 deletions(-)
drivers/usb/serial/f81232.c +9 -1
index 339be30..21f606f 100644
@@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

@@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct f81232_private {
 	struct mutex lock;
-	u8 line_control;
+	u8 modem_control;
 	u8 modem_status;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;

@@ -198,6 +199,52 @@ 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;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+

drivers/usb/serial/f81232.c static void f81232_read_msr(struct usb_serial_port *port) · line 218

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+		return 0;	/* no change */
+
+	/* 'set' takes precedence over 'clear' */
+	clear &= ~set;
+
+	/* force enable interrupt with OUT2 */
+	mutex_lock(&priv->lock);
+	urb_value = UART_MCR_OUT2 | priv->modem_control;
+	mutex_unlock(&priv->lock);

So this is one of the places where the port mute should protect the whole operation.

Quoted 15 lines
drivers/usb/serial/f81232.c +15 -0
+
+	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->modem_control);

drivers/usb/serial/f81232.c static void f81232_read_msr(struct usb_serial_port *port) · line 243

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+		return status;
+	} else {
+		mutex_lock(&priv->lock);
+		priv->modem_control = urb_value;
+		mutex_unlock(&priv->lock);
+	}

No need for else-branch as you return on errors.

drivers/usb/serial/f81232.c static void f81232_read_msr(struct usb_serial_port *port) · line 246

Quoted 3 lines
drivers/usb/serial/f81232.c +3 -0
+
+	return 0;
+}

Johan

maintainer
#3 lore raw

Re: [PATCH V7 02/11] USB: f81232: add preparatory functions

In reply to Peter Hung
Quoted 5 lines · Peter Hung

We add f81232_get_register()/f81232_set_register() and necessary defines for future patch with communication with F81232 USB control endpoint.

Because of this is a preparatory patch, there are unused function warning with compiling. The functions will used with following patches.

Ok, adding these separately facilitates review, but please add them in the patches that first use them after addressing the comments below (to avoid those build warning). Sorry about the extra work.

Quoted 22 lines
Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
drivers/usb/serial/f81232.c +5 -0
index 669a2f2..1f29b95 100644
@@ -30,6 +30,11 @@ 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 F81232_SET_REGISTER	0x40
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02

drivers/usb/serial/f81232.c struct f81232_private { · line 59

Quoted 5 lines
drivers/usb/serial/f81232.c +2 -0
@@ -50,6 +55,80 @@ struct f81232_private {
 	u8 modem_status;
 };
 
+static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
+{

Please rename "data" to "val" here (and in set_register).

drivers/usb/serial/f81232.c struct f81232_private { · line 65

Quoted 6 lines
drivers/usb/serial/f81232.c +6 -0
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	if (!data)
+		return -EFAULT;

No need to test for NULL here.

drivers/usb/serial/f81232.c struct f81232_private { · line 69

Quoted 4 lines
drivers/usb/serial/f81232.c +4 -0
+
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;

Use sizeof(*val) throughout.

drivers/usb/serial/f81232.c struct f81232_private { · line 80

Quoted 11 lines
drivers/usb/serial/f81232.c +11 -0
+
+	status = usb_control_msg(dev,
+				usb_rcvctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_GET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_GET_TIMEOUT);
+	if (status <= 0) {

Test for status != sizeof(*val) here.

drivers/usb/serial/f81232.c struct f81232_private { · line 81

Quoted 1 line
drivers/usb/serial/f81232.c +1 -0
+		/* show something with failed */

Drop the comment.

drivers/usb/serial/f81232.c struct f81232_private { · line 89

Quoted 8 lines
drivers/usb/serial/f81232.c +8 -0
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else {
+		status = 0; /* on success */

Comment not needed.

drivers/usb/serial/f81232.c struct f81232_private { · line 90

Quoted 1 line
drivers/usb/serial/f81232.c +1 -0
+		memcpy((void*) data, (void*) tmp, sizeof(u8));

Just do

	*val = *tmp;

here.

drivers/usb/serial/f81232.c struct f81232_private { · line 97

Quoted 7 lines
drivers/usb/serial/f81232.c +7 -0
+	}
+
+	kfree(tmp);
+	return status;
+}
+
+static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)

Use val instead of data here as well.

drivers/usb/serial/f81232.c struct f81232_private { · line 107

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	memcpy((void*) tmp, (void*) &data, sizeof(u8));

*tmp = *val

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+
+	status = usb_control_msg(dev,
+				usb_sndctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_SET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_SET_TIMEOUT);

drivers/usb/serial/f81232.c struct f81232_private { · line 127

Quoted 10 lines
drivers/usb/serial/f81232.c +10 -0
+	if (status <= 0) {
+		/* show something with failed */
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else
+		status = 0; /* on success */

Missing brackets on else branch. No need for comment.

drivers/usb/serial/f81232.c struct f81232_private { · line 95

Quoted 4 lines
drivers/usb/serial/f81232.c +4 -0
+
+	kfree(tmp);
+	return status;
+}

Most comments to get_register apply here as well.

drivers/usb/serial/f81232.c struct f81232_private { · line 134

Quoted 3 lines
drivers/usb/serial/f81232.c +0 -0
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)

Thanks,
Johan