USB: f81232: V8 patches

PATCH Superseded Peter Hung 2 participants 25 messages Feb 26, 2015 – Mar 16, 2015

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Peter Hung

author

2015-02-26 10:02 UTC #0 lore raw

v8 cover USB: f81232: V8 patches

This series patch V8 is changed from V7 as following:

1. The V7 MSR strange delta value is checked with locking problem. We changed f81232_set_mctrl() & f81232_read_msr() lock mechanism, the old version is only locked with variable protection, new version will lock from start to end with these 2 function.
2. f81232_set_baudrate() add error handling and no longer handle with baudrate B0, change to handle with f81232_set_termios()
3. When user set baudrate larger then 115200, we will change it with 115200 and tty_encode_baud_rate() in f81232_set_termios().
4. V7 f81232_set_baudrate() divisor declared with type u8, it will make baudrate set failed with smaller then B600 (115200/300=384 overflow :Q). We changed it with integer type for divisor larger then 256.

V7 (old change):

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 (10):
USB: f81232: rename private struct member name
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/disable method
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 | 551 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 453 insertions(+), 98 deletions(-)

Signature · 1 line

1.9.1

Peter Hung

author

2015-02-26 10:02 UTC #1 lore raw

PATCH v8 1/10 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

Peter Hung

author

2015-02-26 10:02 UTC #2 lore raw

PATCH v8 2/10 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 | 71 +++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

drivers/usb/serial/f81232.c +37 ~~-34~~

index 669a2f2..25b1a47 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) },

@@ -104,44 +105,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;
-	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);
+	char tty_flag;
+	unsigned int i;
+	u8 lsr;
 
-	if (!urb->actual_length)
+	if ((urb->actual_length < 2) || (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];
+
+		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

Peter Hung

author

2015-02-26 10:02 UTC #8 lore raw

PATCH v8 4/10 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.

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

---
 drivers/usb/serial/f81232.c | 126 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 118 insertions(+), 8 deletions(-)

drivers/usb/serial/f81232.c +118 -8

index cf5b902..46a81ef 100644
@@ -31,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

@@ -49,19 +56,112 @@ 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 *val)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(*val), 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 != sizeof(*val)) {
+		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;
+		*val = *tmp;
+	}
+
+	kfree(tmp);
+	return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	u8 current_msr;
+	struct tty_struct *tty;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	mutex_lock(&priv->lock);
+	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+			&current_msr);
+	if (status) {
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		mutex_unlock(&priv->lock);
+		return;
+	}
+
+	if (!(current_msr & UART_MSR_ANY_DELTA)) {
+		mutex_unlock(&priv->lock);
+		return;
+	}
+
+	priv->modem_status = current_msr;
+
+	if (current_msr & UART_MSR_DCTS)
+		port->icount.cts++;
+	if (current_msr & UART_MSR_DDSR)
+		port->icount.dsr++;
+	if (current_msr & UART_MSR_TERI)
+		port->icount.rng++;
+	if (current_msr & UART_MSR_DDCD) {
+		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);
+	mutex_unlock(&priv->lock);
+}
+
 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)

@@ -272,6 +372,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;

@@ -281,10 +389,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

Peter Hung

author

2015-02-26 10:02 UTC #15 lore raw

PATCH v8 6/10 USB: f81232: implement port enable/disable method

In reply to Peter Hung

We put FCR/IER initial step to f81232_port_enable()/f81232_port_disable(). 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 | 49 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

drivers/usb/serial/f81232.c +49 -0

index b70f9b9..a3f9530 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)

@@ -344,6 +346,48 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static int f81232_port_enable(struct usb_serial_port *port)
+{
+	u8 val;
+	int status;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	val = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
+			UART_FCR_CLEAR_XMIT;
+
+	status = f81232_set_register(port, FIFO_CONTROL_REGISTER, val);
+	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 */
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			UART_IER_MSI);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set IER: %d\n",
+			__func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
+static int f81232_port_disable(struct usb_serial_port *port)
+{
+	int status;
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, 0);
+	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)
 {

@@ -395,6 +439,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_enable(port);
+	if (result)
+		return result;
+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);

@@ -417,6 +465,7 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void f81232_close(struct usb_serial_port *port)
 {
+	f81232_port_disable(port);
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 }

Signature · 1 line

1.9.1

Peter Hung

author

2015-02-26 10:02 UTC #18 lore raw

PATCH v8 8/10 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 0580195..21f2342 100644
@@ -596,24 +596,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

Peter Hung

author

2015-02-26 10:02 UTC #21 lore raw

PATCH v8 10/10 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 0b7b69a..65d5cf6 100644
@@ -684,5 +684,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

Peter Hung

author

2015-02-26 10:03 UTC #20 lore raw

PATCH v8 9/10 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 21f2342..0b7b69a 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

Peter Hung

author

2015-02-26 10:04 UTC #16 lore raw

PATCH v8 7/10 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.

We are checking baudrate in set_termios() too, If baudrate larger then 115200, it will be changed to 115200 and use tty_encode_baud_rate() to encode into tty

f81232_set_baudrate() is also changed from V7. Add error handling when LCR get failed or set LCR UART_LCR_DLAB failed. and older version, divisor is declared with u8, it's will make failed with baudrate lower than 600 (115200/300=384). We had changed divisor to int type.

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

---
 drivers/usb/serial/f81232.c | 112 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 108 insertions(+), 4 deletions(-)

drivers/usb/serial/f81232.c +108 -4

index a3f9530..0580195 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,11 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(speed_t baudrate)
+{
+	return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
 {
 	int status;

@@ -346,6 +356,53 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static void f81232_set_baudrate(struct usb_serial_port *port, speed_t baudrate)
+{
+	u8 lcr;
+	int divisor, status = 0;
+
+	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);
+		return;
+	}
+
+	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);
+		return;
+	}
+
+	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);
+		goto reapply_lcr;
+	}
+
+	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);
+	}
+
+reapply_lcr:
+	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)
 {
 	u8 val;

@@ -391,15 +448,62 @@ static int f81232_port_disable(struct usb_serial_port *port)
 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;
+	speed_t baudrate;
 
 	/* 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);
+
+	baudrate = tty_get_baud_rate(tty);
+	if (baudrate > 0) {
+		if (baudrate > F81232_MAX_BAUDRATE)
+			baudrate = F81232_MAX_BAUDRATE;
+
+		tty_encode_baud_rate(tty, baudrate, baudrate);
+		f81232_set_baudrate(port, baudrate);
+	}
+
+	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

Peter Hung

author

2015-02-26 10:05 UTC #13 lore raw

PATCH v8 5/10 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 | 139 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 117 insertions(+), 22 deletions(-)

drivers/usb/serial/f81232.c +117 ~~-22~~

index 46a81ef..b70f9b9 100644
@@ -34,8 +34,10 @@ 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 SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

@@ -54,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;

@@ -95,6 +97,42 @@ static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
 	return status;
 }
 
+static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 val)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(val), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	*tmp = val;
+
+	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 != sizeof(val)) {
+		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;
+	}
+
+	kfree(tmp);
+	return status;
+}
+
 static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;

@@ -139,6 +177,51 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	mutex_unlock(&priv->lock);
 }
 
+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;
+
+	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__);
+		mutex_unlock(&priv->lock);
+		return status;
+	}
+
+	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)

@@ -250,12 +333,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 */

@@ -283,15 +360,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)

@@ -326,24 +423,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

Peter Hung

author

2015-02-26 10:05 UTC #7 lore raw

PATCH v8 3/10 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 25b1a47..cf5b902 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>

@@ -46,7 +46,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;
 };

@@ -227,17 +227,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);
 }

@@ -281,7 +280,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

Peter Hung

author

2015-03-05 05:59 UTC #22 lore raw

In reply to Peter Hung

Hello,

Peter Hung 於 2015/2/26 下午 06:02 寫道:

Quoted 9 lines

This series patch V8 is changed from V7 as following:
1. The V7 MSR strange delta value is checked with locking problem. We changed f81232_set_mctrl() & f81232_read_msr() lock mechanism, the old version is only locked with variable protection, new version will lock from start to end with these 2 function.
2. f81232_set_baudrate() add error handling and no longer handle with baudrate B0, change to handle with f81232_set_termios()
3. When user set baudrate larger then 115200, we will change it with 115200

Quoted 5 lines

    and tty_encode_baud_rate() in f81232_set_termios().
4. V7 f81232_set_baudrate() divisor declared with type u8, it will make baudrate set failed with smaller then B600 (115200/300=384 overflow :Q). We changed it with integer type for divisor larger then 256.

Did you received the series of F81232 V8 patches ? Please tell me if not received.

Thanks for your review.

Signature · 2 lines

With Best Regards,
Peter Hung

Johan Hovold

maintainer

2015-03-05 07:04 UTC #23 lore raw

Quoted 2 lines · Peter Hung

Did you received the series of F81232 V8 patches ? Please tell me if not received.

I got it. I just haven't had time to look at it yet. Will try to do so this week.

Johan

Johan Hovold

maintainer

2015-03-14 11:49 UTC #3 lore raw

Re: [PATCH V8 02/10] USB: f81232: implement RX bulk-in EP

In reply to Peter Hung

Quoted 17 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]>
static void f81232_process_read_urb(struct urb *urb) {
drivers/usb/serial/f81232.c +0 -8
 	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;
-	int i;
-
-	/* update line status */
-	spin_lock_irqsave(&priv->lock, flags);

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

Quoted 10 lines

drivers/usb/serial/f81232.c +4 -4

-	line_status = priv->modem_status;
-	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
-	spin_unlock_irqrestore(&priv->lock, flags);
+	char tty_flag;
+	unsigned int i;
+	u8 lsr;
 
-	if (!urb->actual_length)
+	if ((urb->actual_length < 2) || (urb->actual_length % 2))
 		return;

Not parsing short data (e.g. not divisible by 2) is OK I guess. You could also just discard the last odd byte, but that's up to you.

Either way, I think you should add a dev_warn here rather than just silently drop it.

Johan

Johan Hovold

maintainer

2015-03-14 12:02 UTC #9 lore raw

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

In reply to Peter Hung

Quoted 14 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.
Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 126 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 118 insertions(+), 8 deletions(-)
drivers/usb/serial/f81232.c +0 -0
index cf5b902..46a81ef 100644

drivers/usb/serial/f81232.c static const struct usb_device_id id_table[] = { · line 39

Quoted 9 lines

drivers/usb/serial/f81232.c +6 -0

@@ -31,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)

Could you indent the values using two tabs so the INTERRUPT_ENABLE_REGISTER in a following patch will also fit?

Quoted 20 lines

drivers/usb/serial/f81232.c +11 -0

+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02

@@ -49,19 +56,112 @@ 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 *val)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(*val), GFP_KERNEL);
+	if (!tmp)

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

Quoted 10 lines

drivers/usb/serial/f81232.c +10 -0

+		return -ENOMEM;
+
+	status = usb_control_msg(dev,
+				usb_rcvctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_GET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),

Please use sizeof(*val) here as well for consistency,

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

Quoted 8 lines

drivers/usb/serial/f81232.c +8 -0

+				USB_CTRL_GET_TIMEOUT);
+	if (status != sizeof(*val)) {
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);

Could you rewrite this as

	if (status < 0)
		status = usb_translate_errors(status);
	else
		status = 0;

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

Quoted 8 lines

drivers/usb/serial/f81232.c +8 -0

+	} else {
+		status = 0;
+		*val = *tmp;
+	}
+
+	kfree(tmp);
+	return status;
+}

Same comments apply to set_register in a follow up patch.

Looks good otherwise,
Johan

Johan Hovold

maintainer

2015-03-14 12:09 UTC #14 lore raw

Re: [PATCH V8 05/10] USB: f81232: implement MCR/MSR function

In reply to Peter Hung

Quoted 11 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]>
---
 drivers/usb/serial/f81232.c | 139 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 117 insertions(+), 22 deletions(-)

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

Quoted 4 lines

drivers/usb/serial/f81232.c +4 -0

+static int f81232_set_mctrl(struct usb_serial_port *port,
+					   unsigned int set, unsigned int clear)
+{
+	u8 urb_value;

Minor nit: could you just call this "val" as it's unrelated to any urb.

Also looks good otherwise.

Johan

Johan Hovold

maintainer

2015-03-14 12:24 UTC #17 lore raw

Re: [PATCH V8 07/10] USB: f81232: implement set_termios()

In reply to Peter Hung

Quoted 4 lines · Peter Hung

f81232_set_baudrate() is also changed from V7. Add error handling when LCR get failed or set LCR UART_LCR_DLAB failed. and older version, divisor is declared with u8, it's will make failed with baudrate lower than 600 (115200/300=384). We had changed divisor to int type.

No need to include this patch-revision changelog in the commit message. You put it under the cut-off-line (---) below though.

Quoted 25 lines

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

---
 drivers/usb/serial/f81232.c | 112 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 108 insertions(+), 4 deletions(-)

static int f81232_port_enable(struct usb_serial_port *port) {

drivers/usb/serial/f81232.c +4 -4

 	u8 val;

@@ -391,15 +448,62 @@ static int f81232_port_disable(struct usb_serial_port *port)
 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;
+	speed_t baudrate;
 
 	/* 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)

drivers/usb/serial/f81232.c static int f81232_port_disable(struct usb_serial_port *port) · line 469

Quoted 10 lines

drivers/usb/serial/f81232.c +10 -0

+		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);
+
+	baudrate = tty_get_baud_rate(tty);
+	if (baudrate > 0) {
+		if (baudrate > F81232_MAX_BAUDRATE)
+			baudrate = F81232_MAX_BAUDRATE;
+
+		tty_encode_baud_rate(tty, baudrate, baudrate);

You only need to update the termios baudrate in case you cannot set the baudrate requested (e.g. > F81232_MAX_BAUDRATE).

Quoted 26 lines

drivers/usb/serial/f81232.c +26 -0

+		f81232_set_baudrate(port, baudrate);
+	}
+
+	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;

drivers/usb/serial/f81232.c static int f81232_port_disable(struct usb_serial_port *port) · line 505

Quoted 10 lines

drivers/usb/serial/f81232.c +10 -0

+	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);

Please add brackets around the if block.

drivers/usb/serial/f81232.c static int f81232_port_disable(struct usb_serial_port *port) · line 509

Quoted 4 lines

drivers/usb/serial/f81232.c +1 -0

+
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)

Looks good otherwise.

Johan

Johan Hovold

maintainer

2015-03-14 12:28 UTC #19 lore raw

Re: [PATCH V8 08/10] USB: f81232: clarify f81232_ioctl() and fix

In reply to Peter Hung

Quoted 20 lines · 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 +1 -0
index 0580195..21f2342 100644
@@ -596,24 +596,32 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
 	return 0;
 }
 
+static int f81232_get_serial_info(struct usb_serial_port *port,

drivers/usb/serial/f81232.c static int f81232_carrier_raised(struct usb_serial_port *port) · line 609

Quoted 10 lines

drivers/usb/serial/f81232.c +10 -0

+		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;

Do you want to use you MAX_BAUDRATE define here as well?

Johan

Johan Hovold

maintainer

2015-03-14 12:30 UTC #24 lore raw

In reply to Johan Hovold

Quoted 7 lines · Johan Hovold

Quoted 2 lines · Peter Hung
Did you received the series of F81232 V8 patches ? Please tell me if not received.
I got it. I just haven't had time to look at it yet. Will try to do so this week.

I've commented on v8 now --sorry for the delay. Series looks really good now, just a few really minor issues left.

Thanks for doing this work!

Johan

Peter Hung

author

2015-03-16 02:53 UTC #4 lore raw

Re: [PATCH V8 02/10] USB: f81232: implement RX bulk-in EP

In reply to Johan Hovold

Hello,

Johan Hovold 於 2015/3/14 下午 07:48 寫道:

Quoted 10 lines · Peter Hung

drivers/usb/serial/f81232.c exit: · line 114
Quoted 3 lines
drivers/usb/serial/f81232.c +1 -1
-	if (!urb->actual_length)
+	if ((urb->actual_length < 2) || (urb->actual_length % 2))
  		return;
Not parsing short data (e.g. not divisible by 2) is OK I guess. You could also just discard the last odd byte, but that's up to you.
Either way, I think you should add a dev_warn here rather than just silently drop it.

With F81232, when it first submit with interrupt URB, it will response once with 1 bytes data. The data is hw current LSR, but it useless on open. It's should necessary with receiving data. When the device is working good, it's should acked with even size data.

To avoid confusing to user, I'll keep it without warning message.

Thanks

Signature · 2 lines

With Best Regards,
Peter Hung

Peter Hung

author

2015-03-16 03:08 UTC #10 lore raw

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

In reply to Johan Hovold

Hello,

Johan Hovold 於 2015/3/14 下午 08:02 寫道:

Quoted 11 lines · Peter Hung

drivers/usb/serial/f81232.c struct f81232_private { · line 88
Quoted 7 lines
drivers/usb/serial/f81232.c +7 -0
+	if (status != sizeof(*val)) {
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
Could you rewrite this as

Quoted 4 lines

	if (status < 0)
		status = usb_translate_errors(status);
	else
		status = 0;

In my definition the return value of set/getregister(), 0 is success, negative values are errors. The function usb_control_msg() return value is success transmited/received byte. It's maybe return 0. I want to treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It will return 0 back. So I need especially handle with status == 0.

I'll keep this sections.

Thanks for review

Signature · 2 lines

With Best Regards,
Peter Hung

Johan Hovold

maintainer

2015-03-16 08:52 UTC #5 lore raw

Re: [PATCH V8 02/10] USB: f81232: implement RX bulk-in EP

In reply to Peter Hung

Quoted 11 lines · Peter Hung

Hello,
Johan Hovold 於 2015/3/14 下午 07:48 寫道:
Quoted 8 lines · Peter Hung
drivers/usb/serial/f81232.c exit: · line 114
Quoted 3 lines
drivers/usb/serial/f81232.c +1 -1
-	if (!urb->actual_length)
+	if ((urb->actual_length < 2) || (urb->actual_length % 2))
  		return;
Not parsing short data (e.g. not divisible by 2) is OK I guess. You could also just discard the last odd byte, but that's up to you.

Quoted 7 lines

Quoted 2 lines
Either way, I think you should add a dev_warn here rather than just silently drop it.
With F81232, when it first submit with interrupt URB, it will response once with 1 bytes data. The data is hw current LSR, but it useless on open. It's should necessary with receiving data. When the device is working good, it's should acked with even size data.

Ok, thanks for clarifying that.

Quoted 1 line

To avoid confusing to user, I'll keep it without warning message.

Yes, skip the warning, but could you a short comment about this (e.g. the 1-byte packet on open) before you do the size check?

Thanks,
Johan

Johan Hovold

maintainer

2015-03-16 08:55 UTC #11 lore raw

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

In reply to Peter Hung

Quoted 19 lines · Peter Hung

Hello,
Johan Hovold 於 2015/3/14 下午 08:02 寫道:
Quoted 11 lines · Peter Hung
drivers/usb/serial/f81232.c struct f81232_private { · line 88
Quoted 7 lines
drivers/usb/serial/f81232.c +7 -0
+	if (status != sizeof(*val)) {
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
Could you rewrite this as
Quoted 4 lines
	if (status < 0)
		status = usb_translate_errors(status);
	else
		status = 0;

Quoted 5 lines

In my definition the return value of set/getregister(), 0 is success, negative values are errors. The function usb_control_msg() return value is success transmited/received byte. It's maybe return 0. I want to treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It will return 0 back. So I need especially handle with status == 0.

I meant to write

	if (status < 0)
 		status = usb_translate_errors(status);
	else
		status = -EIO;

which I think is more readable.

Sorry for the confusion.

Johan

Peter Hung

author

2015-03-16 09:15 UTC #6 lore raw

Re: [PATCH V8 02/10] USB: f81232: implement RX bulk-in EP

In reply to Johan Hovold

Hello,

Johan Hovold 於 2015/3/16 下午 04:52 寫道:

Quoted 5 lines · Peter Hung

Quoted 1 line
To avoid confusing to user, I'll keep it without warning message.
Yes, skip the warning, but could you a short comment about this (e.g. the 1-byte packet on open) before you do the size check?

Thanks for your mention, I will add it for next patch.

Signature · 2 lines

With Best Regards,
Peter Hung

Peter Hung

author

2015-03-16 09:18 UTC #12 lore raw

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

In reply to Johan Hovold

Hello,

Johan Hovold 於 2015/3/16 下午 04:55 寫道:

Quoted 14 lines · Peter Hung

Quoted 7 lines
Quoted 6 lines
Could you rewrite this as
	if (status < 0)
		status = usb_translate_errors(status);
	else
		status = 0;
Quoted 5 lines
In my definition the return value of set/getregister(), 0 is success, negative values are errors. The function usb_control_msg() return value is success transmited/received byte. It's maybe return 0. I want to treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It will return 0 back. So I need especially handle with status == 0.

Quoted 8 lines

I meant to write
	if (status < 0)
  		status = usb_translate_errors(status);
	else
		status = -EIO;
which I think is more readable.

It's looks more readable of the style that you mentioned. Thanks for your advice, I'll add it with next patch.

Signature · 2 lines

With Best Regards,
Peter Hung