USB: f81232: V6 patches

PATCH Superseded Peter Hung 4 participants 21 messages Feb 16, 2015 – Feb 24, 2015

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Peter Hung

author

2015-02-16 07:58 UTC #0 lore raw

v6 cover USB: f81232: V6 patches

This series patch V6 is improvement from V5&V4 as following:

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.

Logs:

1. We had add dev_err() in set/get register function. Also add dev_err() in some function is to help us easily point out error position, so we still decide to remain it.

Thanks for reading.

Peter Hung (10):
USB: f81232: rename private struct member name
USB: f81232: implement read IIR/MSR with endpoint
USB: f81232: implement RX bulk-in ep
USB: f81232: implement set_termios
USB: f81232: implement MCR/MSR function
USB: f81232: clarify f81232_ioctl and fix
USB: f81232: fix error in f81232_carrier_raised()
USB: f81232: fix read MSR strange value
USB: f81232: implement delta change for MSR count
USB: f81232: modify/add author

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

Signature · 1 line

1.9.1

Peter Hung

author

2015-02-16 07:58 UTC #1 lore raw

PATCH v6 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-16 07:58 UTC #2 lore raw

PATCH v6 2/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() func.

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

---
 drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 100 insertions(+), 9 deletions(-)

drivers/usb/serial/f81232.c +100 -9

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

@@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0
+
+#define SERIAL_BASE_ADDRESS	   0x0120
+#define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02

@@ -48,19 +56,92 @@ struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
 	u8 modem_status;
+
+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
-static void f81232_update_line_status(struct usb_serial_port *port,
+static int f81232_get_register(struct usb_serial_port *port,
+							  u16 reg, u8 *data)
+{
+	int status;
+	struct usb_device *dev = port->serial->dev;
+
+	status = usb_control_msg(dev,
+			 usb_rcvctrlpipe(dev, 0),
+			 F81232_REGISTER_REQUEST,
+			 F81232_GET_REGISTER,
+			 reg,
+			 0,
+			 data,
+			 sizeof(*data),
+			 USB_CTRL_GET_TIMEOUT);
+	if (status < 0)
+		dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+	return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	unsigned long flags;
+	u8 current_msr;
+	struct tty_struct *tty;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+			&current_msr);
+	if (status < 0) {
+		/* Retain the error even reported in f81232_get_register()
+		     to make debug easily :D */
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		return;
+	}
+
+	if (!(current_msr & UART_MSR_ANY_DELTA))
+		return;
+
+	tty = tty_port_tty_get(&port->port);
+	if (tty) {
+		if (current_msr & UART_MSR_DDCD) {
+			usb_serial_handle_dcd_change(port, tty,
+				current_msr & UART_MSR_DCD);
+		}
+
+		tty_kref_put(tty);
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+	priv->modem_status = current_msr;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
+static void f81232_update_modem_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
 {
-	/*
-	 * FIXME: Update port->icount, and call
-	 *
-	 *		wake_up_interruptible(&port->port.delta_msr_wait);
-	 *
-	 *	  on MSR changes.
-	 */
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (!actual_length)
+		return;
+
+	switch (data[0] & 0x07) {
+	case 0x00: /* msr change */
+		dev_dbg(&port->dev, "IIR: MSR Change: %x\n", data[0]);
+		schedule_work(&priv->interrupt_work);
+		break;
+	case 0x02: /* tx-empty */
+		break;
+	case 0x04: /* rx data available */
+		break;
+	case 0x06: /* lsr change */
+		/* we can forget it. the LSR will read from bulk-in */
+		dev_dbg(&port->dev, "IIR: LSR Change: %x\n", data[0]);
+		break;
+	}
 }
 
 static void f81232_read_int_callback(struct urb *urb)

@@ -91,7 +172,7 @@ static void f81232_read_int_callback(struct urb *urb)
 	usb_serial_debug_data(&port->dev, __func__,
 			      urb->actual_length, urb->transfer_buffer);
 
-	f81232_update_line_status(port, data, actual_length);
+	f81232_update_modem_status(port, data, actual_length);
 
 exit:
 	retval = usb_submit_urb(urb, GFP_ATOMIC);

@@ -270,6 +351,14 @@ static int f81232_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static void  f81232_interrupt_work(struct work_struct *work)
+{
+	struct f81232_private *priv =
+		container_of(work, struct f81232_private, interrupt_work);
+
+	f81232_read_msr(priv->port);
+}
+
 static int f81232_port_probe(struct usb_serial_port *port)
 {
 	struct f81232_private *priv;

@@ -279,10 +368,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	spin_lock_init(&priv->lock);
+	INIT_WORK(&priv->interrupt_work,  f81232_interrupt_work);
 
 	usb_set_serial_port_data(port, priv);
 
 	port->port.drain_delay = 256;
+	priv->port = port;
 
 	return 0;
 }

Signature · 1 line

1.9.1

Johan Hovold

maintainer

2015-02-17 08:28 UTC #3 lore raw

Re: [PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

Quoted 21 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() func.
Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 100 insertions(+), 9 deletions(-)
drivers/usb/serial/f81232.c +1 -0
index 669a2f2..ec4609d 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 static const struct usb_device_id id_table[] = { · line 36

Quoted 6 lines

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

@@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0

Indent these define values using a tab as well.

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

Quoted 12 lines

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

+
+#define SERIAL_BASE_ADDRESS	   0x0120
+#define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02

@@ -48,19 +56,92 @@ struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
 	u8 modem_status;
+

Newline not needed.

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

Quoted 7 lines

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

+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
-static void f81232_update_line_status(struct usb_serial_port *port,
+static int f81232_get_register(struct usb_serial_port *port,
+							  u16 reg, u8 *data)

No need to break this line any more.

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

Quoted 13 lines

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

+{
+	int status;
+	struct usb_device *dev = port->serial->dev;
+
+	status = usb_control_msg(dev,
+			 usb_rcvctrlpipe(dev, 0),
+			 F81232_REGISTER_REQUEST,
+			 F81232_GET_REGISTER,
+			 reg,
+			 0,
+			 data,
+			 sizeof(*data),
+			 USB_CTRL_GET_TIMEOUT);

There's some odd indentation above and in other patches as well (in this case two tabs and one space). Please either just tabs or, if you prefer, add additional spaces to align all the arguments with the first.

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

Quoted 1 line

drivers/usb/serial/f81232.c +1 -0
+	if (status < 0)

Zero is actually also an error here (returned length was 0). You may want to let this function return -EIO in that case (and perhaps also return 0 on success) or you need to fix this at every call site.

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

Quoted 1 line

drivers/usb/serial/f81232.c +1 -0
+		dev_err(&port->dev, "%s status: %d\n", __func__, status);

Please spell out that there was an error, for example,

	"%s failed: %d\n"

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

Quoted 3 lines

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

Could you add the get/set register helpers in a separate preparatory patch?

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

Quoted 11 lines

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

+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	unsigned long flags;
+	u8 current_msr;
+	struct tty_struct *tty;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+			&current_msr);

You cannot use a variable on the stack for usb_control_msg as the buffer must be DMA-able (on all platforms). Use kzalloc.

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

Quoted 3 lines

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

+	if (status < 0) {
+		/* Retain the error even reported in f81232_get_register()
+		     to make debug easily :D */

No need for this comment.

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

Quoted 12 lines

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

+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		return;
+	}
+
+	if (!(current_msr & UART_MSR_ANY_DELTA))
+		return;
+
+	tty = tty_port_tty_get(&port->port);
+	if (tty) {
+		if (current_msr & UART_MSR_DDCD) {
+			usb_serial_handle_dcd_change(port, tty,
+				current_msr & UART_MSR_DCD);

Please indent continuation lines at least two tabs further (i.e add one more tab here).

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

Quoted 4 lines

drivers/usb/serial/f81232.c +4 -0
+		}
+
+		tty_kref_put(tty);
+	}

You should rewrite this so you only get the tty reference if you're actually gonna use it.

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

Quoted 9 lines

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

+
+	spin_lock_irqsave(&priv->lock, flags);
+	priv->modem_status = current_msr;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
+static void f81232_update_modem_status(struct usb_serial_port *port,

This one should still be called f81232_update_line_status.

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

Quoted 2 lines

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

 				      unsigned char *data,
 				      unsigned int actual_length)

Just call it length or size, you can use size_t.

Quoted 6 lines

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

 {
-	/*
-	 * FIXME: Update port->icount, and call
-	 *
-	 *		wake_up_interruptible(&port->port.delta_msr_wait);
-	 *

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

Quoted 10 lines

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

-	 *	  on MSR changes.
-	 */
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (!actual_length)
+		return;
+
+	switch (data[0] & 0x07) {
+	case 0x00: /* msr change */
+		dev_dbg(&port->dev, "IIR: MSR Change: %x\n", data[0]);

%02x

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

Quoted 9 lines

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

+		schedule_work(&priv->interrupt_work);
+		break;
+	case 0x02: /* tx-empty */
+		break;
+	case 0x04: /* rx data available */
+		break;
+	case 0x06: /* lsr change */
+		/* we can forget it. the LSR will read from bulk-in */
+		dev_dbg(&port->dev, "IIR: LSR Change: %x\n", data[0]);

%02x

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

Quoted 3 lines

drivers/usb/serial/f81232.c +2 -0
+		break;
+	}
 }

Johan

Peter Hung

author

2015-02-16 08:00 UTC #4 lore raw

PATCH v6 3/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 reimplemented in this patch.

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

---
 drivers/usb/serial/f81232.c | 68 +++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

drivers/usb/serial/f81232.c +35 ~~-33~~

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

Signature · 1 line

1.9.1

Greg KH

maintainer

2015-02-16 19:41 UTC #5 lore raw

Re: [PATCH V6 03/10] USB: f81232: implement RX bulk-in ep

Quoted 52 lines · Peter Hung

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

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

---
 drivers/usb/serial/f81232.c | 68 +++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

drivers/usb/serial/f81232.c +2 ~~-29~~

index ec4609d..9ea498a 100644
@@ -185,44 +185,46 @@ exit:
 static void f81232_process_read_urb(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
-	struct f81232_private *priv = usb_get_serial_port_data(port);
 	unsigned char *data = urb->transfer_buffer;
-	char tty_flag = TTY_NORMAL;
-	unsigned long flags;
-	u8 line_status;
+	char tty_flag;
 	int i;
 
-	/* update line status */
-	spin_lock_irqsave(&priv->lock, flags);
-	line_status = priv->modem_status;
-	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	if (!urb->actual_length)
+	if (urb->actual_length < 2)
 		return;
 
-	/* break takes precedence over parity, */
-	/* which takes precedence over framing errors */
-	if (line_status & UART_BREAK_ERROR)
-		tty_flag = TTY_BREAK;
-	else if (line_status & UART_PARITY_ERROR)
-		tty_flag = TTY_PARITY;
-	else if (line_status & UART_FRAME_ERROR)
-		tty_flag = TTY_FRAME;
-	dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
-
-	/* overrun is special, not associated with a char */
-	if (line_status & UART_OVERRUN_ERROR)
-		tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
-
-	if (port->port.console && port->sysrq) {
-		for (i = 0; i < urb->actual_length; ++i)
-			if (!usb_serial_handle_sysrq_char(port, data[i]))
-				tty_insert_flip_char(&port->port, data[i],

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

Quoted 10 lines

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

-						tty_flag);
-	} else {
-		tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
-							urb->actual_length);
+	/* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
+
+	for (i = 0 ; i < urb->actual_length ; i += 2) {
+		tty_flag = TTY_NORMAL;
+
+		if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {

Never use unlikely() unless you can prove that it actually matters if you use it. Hint, it's almost impossible to prove, so don't use it, the compiler and processor look-ahead is almost smarter than we are.

thanks,

greg k-h

Peter Hung

author

2015-02-17 01:42 UTC #6 lore raw

Re: [PATCH V6 03/10] USB: f81232: implement RX bulk-in ep

Hello,

Greg KH 於 2015/2/17 上午 03:41 寫道:

Quoted 5 lines

drivers/usb/serial/f81232.c exit: · line 200
Quoted 1 line
drivers/usb/serial/f81232.c +1 -0
+		if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
Never use unlikely() unless you can prove that it actually matters if you use it. Hint, it's almost impossible to prove, so don't use it, the compiler and processor look-ahead is almost smarter than we are.

Thanks for your hint. I'll remove it with next patch.

Signature · 2 lines

With Best Regards,
Peter Hung

David Laight

2015-02-17 10:06 UTC #7 lore raw

RE: [PATCH V6 03/10] USB: f81232: implement RX bulk-in ep

In reply to Greg KH

From: Greg KH

Quoted 8 lines

drivers/usb/serial/f81232.c exit: · line 200
Quoted 4 lines
drivers/usb/serial/f81232.c +4 -0
+	for (i = 0 ; i < urb->actual_length ; i += 2) {
+		tty_flag = TTY_NORMAL;
+
+		if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
Never use unlikely() unless you can prove that it actually matters if you use it. Hint, it's almost impossible to prove, so don't use it, the compiler and processor look-ahead is almost smarter than we are.

That just isn't true.

The compiler cannot know the actual control flow - so cannot correctly arrange the code so that the branches are statically predicted correctly for the required path (usually the most common path).

There are a lot of places where a few extra clocks for a mispredicted branch don't really matter, and even in very hot paths where it does matter it can be quite difficult to get the compiler to optimise the branches 'correctly' - you can need to add asm comments in order to generate non-empty code blocks.

In addition unlikely() is also a note to the human reader.

I did a lot of work adding likely/unlikely to some code in order to minimise the 'worst case' code path. I got there, but some parts were initially non-intuitive.

david

'Greg KH'

maintainer

2015-02-17 14:48 UTC #8 lore raw

Re: [PATCH V6 03/10] USB: f81232: implement RX bulk-in ep

Quoted 22 lines · David Laight

From: Greg KH
Quoted 8 lines
drivers/usb/serial/f81232.c exit: · line 200
Quoted 4 lines
drivers/usb/serial/f81232.c +4 -0
+	for (i = 0 ; i < urb->actual_length ; i += 2) {
+		tty_flag = TTY_NORMAL;
+
+		if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
Never use unlikely() unless you can prove that it actually matters if you use it. Hint, it's almost impossible to prove, so don't use it, the compiler and processor look-ahead is almost smarter than we are.
That just isn't true.
The compiler cannot know the actual control flow - so cannot correctly arrange the code so that the branches are statically predicted correctly for the required path (usually the most common path).
There are a lot of places where a few extra clocks for a mispredicted branch don't really matter, and even in very hot paths where it does matter it can be quite difficult to get the compiler to optimise the branches 'correctly' - you can need to add asm comments in order to generate non-empty code blocks.

Quoted 5 lines

In addition unlikely() is also a note to the human reader.
I did a lot of work adding likely/unlikely to some code in order to minimise the 'worst case' code path. I got there, but some parts were initially non-intuitive.

Yes, but remember that old patch that Andi did to actually check to see if unlikely/likely mattered and was placed correctly? Turns out that 90% of the usages were wrong. So humans are horrible at using these markings, so I will not accept them unless you can _prove_ it matters in the code.

For a urb callback, that's not an issue at all, the usb callback is so slow that you will almost never make a difference, sorry.

So again, don't do it in driver code unless you can prove it.

thanks,

greg k-h

Peter Hung

author

2015-02-16 07:58 UTC #9 lore raw

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

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

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

---
 drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 7 deletions(-)

drivers/usb/serial/f81232.c +138 -7

index 9ea498a..06d1eb0 100644
@@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE 115200L
+
 /* USB Control EP parameter */
 #define F81232_REGISTER_REQUEST 0xA0
 #define F81232_GET_REGISTER 0xc0
+#define F81232_SET_REGISTER 0x40
 
 #define SERIAL_BASE_ADDRESS	   0x0120
+#define RECEIVE_BUFFER_REGISTER    (0x00 + SERIAL_BASE_ADDRESS)
+#define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

@@ -61,6 +69,14 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(u32 baudrate)
+{
+	if (!baudrate)
+		return 0;
+	else
+		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
 static int f81232_get_register(struct usb_serial_port *port,
 							  u16 reg, u8 *data)
 {

@@ -82,6 +98,27 @@ static int f81232_get_register(struct usb_serial_port *port,
 	return status;
 }
 
+static int f81232_set_register(struct usb_serial_port *port,
+							  u16 reg, u8 data)
+{
+	int status;
+	struct usb_device *dev = port->serial->dev;
+
+	status = usb_control_msg(dev,
+			usb_sndctrlpipe(dev, 0),
+			F81232_REGISTER_REQUEST,
+			F81232_SET_REGISTER,
+			reg,
+			0,
+			&data,
+			sizeof(data),
+			USB_CTRL_SET_TIMEOUT);
+	if (status < 0)
+		dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+	return status;
+}
+
 static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;

@@ -247,18 +284,106 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
+{
+	u8 divisor;
+	int status = 0;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 UART_LCR_DLAB); /* DLAB */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n",
+			__func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n",
+			__func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+			status, __LINE__);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, 0x00);
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+			status, __LINE__);
+	}
+}
+
+static int f81232_port_init(struct usb_serial_port *port)
+{
+	u8 data;
+	int status = 0;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+			| UART_FCR_CLEAR_XMIT;
+
+	status |= f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	data = UART_IER_MSI;
+
+	/* IER */
+	status |= f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+	if (status < 0) {
+		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	/* FIXME - Stubbed out for now */
+	u8 new_lcr = 0;
+	int status = 0;
 
-	/* Don't change anything if nothing has changed */
-	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
-		return;
+	f81232_set_baudrate(port, tty_get_baud_rate(tty));
+
+	if (C_PARENB(tty)) {
+		new_lcr |= UART_LCR_PARITY;
+
+		if (!C_PARODD(tty))
+			new_lcr |= UART_LCR_EPAR;
+
+		if (C_CMSPAR(tty))
+			new_lcr |= UART_LCR_SPAR;
+	}
+
+	if (C_CSTOPB(tty))
+		new_lcr |= UART_LCR_STOP;
+
+	switch (C_CSIZE(tty)) {
+	case CS5:
+		new_lcr |= UART_LCR_WLEN5;
+		break;
+	case CS6:
+		new_lcr |= UART_LCR_WLEN6;
+		break;
+	case CS7:
+		new_lcr |= UART_LCR_WLEN7;
+		break;
+	default:
+	case CS8:
+		new_lcr |= UART_LCR_WLEN8;
+		break;
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
+	if (status < 0)
+		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
 
-	/* Do the real work here... */
-	if (old_termios)
-		tty_termios_copy_hw(&tty->termios, old_termios);
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)

@@ -278,6 +403,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_init(port);
+	if (result) {
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+		return result;
+	}
+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);

Signature · 1 line

1.9.1

Johan Hovold

maintainer

2015-02-17 08:57 UTC #10 lore raw

Re: [PATCH V6 04/10] USB: f81232: implement set_termios

Quoted 17 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.
Some init step extract to f81232_port_init(), called once with open(). And refine baudrate setting to f81232_set_baudrate()
Signed-off-by Peter Hung <[email protected]>
---
 drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 138 insertions(+), 7 deletions(-)
drivers/usb/serial/f81232.c +0 -0
index 9ea498a..06d1eb0 100644

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

Quoted 5 lines

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

@@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE 115200L

No need for L.

Quoted 15 lines

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

+
 /* USB Control EP parameter */
 #define F81232_REGISTER_REQUEST 0xA0
 #define F81232_GET_REGISTER 0xc0
+#define F81232_SET_REGISTER 0x40
 
 #define SERIAL_BASE_ADDRESS	   0x0120
+#define RECEIVE_BUFFER_REGISTER    (0x00 + SERIAL_BASE_ADDRESS)
+#define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

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

Quoted 10 lines

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

@@ -61,6 +69,14 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(u32 baudrate)
+{
+	if (!baudrate)
+		return 0;
+	else
+		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}

You should handle B0 at the call site (and in that case keep the current baudrate).

drivers/usb/serial/f81232.c static int f81232_get_register(struct usb_serial_port *port, · line 102

Quoted 10 lines

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

+
 static int f81232_get_register(struct usb_serial_port *port,
 							  u16 reg, u8 *data)
 {

@@ -82,6 +98,27 @@ static int f81232_get_register(struct usb_serial_port *port,
 	return status;
 }
 
+static int f81232_set_register(struct usb_serial_port *port,
+							  u16 reg, u8 data)

No need to break line.

drivers/usb/serial/f81232.c static int f81232_get_register(struct usb_serial_port *port, · line 115

Quoted 13 lines

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

+{
+	int status;
+	struct usb_device *dev = port->serial->dev;
+
+	status = usb_control_msg(dev,
+			usb_sndctrlpipe(dev, 0),
+			F81232_REGISTER_REQUEST,
+			F81232_SET_REGISTER,
+			reg,
+			0,
+			&data,
+			sizeof(data),
+			USB_CTRL_SET_TIMEOUT);

As for f81232_get_register, you must use a DMA-able buffer for the data buffer. Either allocate it in this function or at the call site.

drivers/usb/serial/f81232.c static int f81232_get_register(struct usb_serial_port *port, · line 120

Quoted 5 lines

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

+	if (status < 0)
+		dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+	return status;
+}

Same comments apply as for f81232_set_register on the previous patch.

Quoted 8 lines

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

+
 static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;

@@ -247,18 +284,106 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }

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

Quoted 10 lines

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

+static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
+{
+	u8 divisor;
+	int status = 0;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 UART_LCR_DLAB); /* DLAB */
+	if (status < 0) {

As with f81232_get_register 0 is also an error.

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

Quoted 2 lines

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

+		dev_err(&port->dev, "%s status: %d line:%d\n",
+			__func__, status, __LINE__);

Don't use __LINE__ in your error messages. The dynamic debugging infrastructure can add that if ever needed.

Spell out the errors instead (e.g. "failed to set DLAB").

Shouldn't you be ORing DLAB to the current state of LCR by the way?

And make sure to bail out on errors here.

Quoted 7 lines

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

+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n",
+			__func__, status, __LINE__);

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

Quoted 10 lines

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

+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+			status, __LINE__);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, 0x00);

Only clear DLAB?

Quoted 6 lines

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

+	if (status < 0) {
+		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+			status, __LINE__);
+	}
+}
+

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

Quoted 10 lines

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

+static int f81232_port_init(struct usb_serial_port *port)
+{
+	u8 data;
+	int status = 0;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+			| UART_FCR_CLEAR_XMIT;
+
+	status |= f81232_set_register(port, FIFO_CONTROL_REGISTER, data);

Just assign return value and don't initialise above.

And just bail out on errors directly here.

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

Quoted 6 lines

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

+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	data = UART_IER_MSI;
+
+	/* IER */
+	status |= f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);

Just use = here as well.

Quoted 9 lines

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

+	if (status < 0) {
+		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
 static void f81232_set_termios(struct tty_struct *tty,

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

Quoted 10 lines

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

 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	/* FIXME - Stubbed out for now */
+	u8 new_lcr = 0;
+	int status = 0;
 
-	/* Don't change anything if nothing has changed */
-	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
-		return;
+	f81232_set_baudrate(port, tty_get_baud_rate(tty));

So here you should check for B0 and in that case simply not update the baudrate.

B0 is, as I believe I already mentioned, used to hang up any modem by dropping DTR.

Quoted 41 lines

drivers/usb/serial/f81232.c +33 -3

+
+	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;
+		b<reak;
+	default:z
+	case CS8:
+		new_lcr |= UART_LCR_WLEN8;
+		break;
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
+	if (status < 0)
+		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
 
-	/* Do the real work here... */
-	if (old_termios)
-		tty_termios_copy_hw(&tty->termios, old_termios);
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)

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

Quoted 8 lines

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

@@ -278,6 +403,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_init(port);
+	if (result) {
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+		return result;
+	}

Don't you wan't to reverse f81232_port_init at close (e.g. disable interrupts)?

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

Quoted 4 lines

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

+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);

Johan

Peter Hung

author

2015-02-16 07:58 UTC #11 lore raw

PATCH v6 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().

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

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

---
 drivers/usb/serial/f81232.c | 98 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 77 insertions(+), 21 deletions(-)

drivers/usb/serial/f81232.c +77 ~~-21~~

index 06d1eb0..e1cdf42 100644
@@ -44,6 +44,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
 #define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
 #define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
+#define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

@@ -156,6 +157,50 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	wake_up_interruptible(&port->port.delta_msr_wait);
 }
 
+static int f81232_set_mctrl(struct usb_serial_port *port,
+					   unsigned int set, unsigned int clear)
+{
+	u8 urb_value;
+	int status;
+	unsigned long flags;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+		return 0;	/* no change */
+
+	/* 'set' takes precedence over 'clear' */
+	clear &= ~set;
+
+	/* force enable interrupt with OUT2 */
+	urb_value = UART_MCR_OUT2 | priv->line_control;
+
+	if (clear & TIOCM_DTR)
+		urb_value &= ~UART_MCR_DTR;
+
+	if (clear & TIOCM_RTS)
+		urb_value &= ~UART_MCR_RTS;
+
+	if (set & TIOCM_DTR)
+		urb_value |= UART_MCR_DTR;
+
+	if (set & TIOCM_RTS)
+		urb_value |= UART_MCR_RTS;
+
+	dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+			urb_value, priv->line_control);
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status < 0) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+	} else {
+		spin_lock_irqsave(&priv->lock, flags);
+		priv->line_control = urb_value;
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+
+	return status;
+}
+
 static void f81232_update_modem_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)

@@ -267,12 +312,6 @@ static void f81232_process_read_urb(struct urb *urb)
 	tty_flip_buffer_push(&port->port);
 }
 
-static int set_control_lines(struct usb_device *dev, u8 value)
-{
-	/* FIXME - Stubbed out for now */
-	return 0;
-}
-
 static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
 	/* FIXME - Stubbed out for now */

@@ -388,15 +427,41 @@ static void f81232_set_termios(struct tty_struct *tty,
 
 static int f81232_tiocmget(struct tty_struct *tty)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	int r;
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *port_priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	u8 mcr, msr;
+
+	/* force get current MSR changed state */
+	f81232_read_msr(port);
+
+	spin_lock_irqsave(&port_priv->lock, flags);
+	mcr = port_priv->line_control;
+	msr = port_priv->modem_status;
+	spin_unlock_irqrestore(&port_priv->lock, flags);
+
+	r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+		(mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+		(msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+		(msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+		(msr & UART_MSR_RI ? TIOCM_RI : 0) |
+		(msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+	return r;
 }
 
 static int f81232_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	int status;
+	struct usb_serial_port *port = tty->driver_data;
+
+	status = f81232_set_mctrl(port, set, clear);
+	if (status < 0)
+		return usb_translate_errors(status);
+	else
+		return 0;
 }
 
 static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)

@@ -437,19 +502,10 @@ static void f81232_close(struct usb_serial_port *port)
 
 static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct f81232_private *priv = usb_get_serial_port_data(port);
-	unsigned long flags;
-	u8 control;
-
-	spin_lock_irqsave(&priv->lock, flags);
-	/* Change DTR and RTS */
 	if (on)
-		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
 	else
-		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
-	control = priv->line_control;
-	spin_unlock_irqrestore(&priv->lock, flags);
-	set_control_lines(port->serial->dev, control);
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static int f81232_carrier_raised(struct usb_serial_port *port)

Signature · 1 line

1.9.1

Johan Hovold

maintainer

2015-02-17 09:59 UTC #12 lore raw

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

Quoted 33 lines · Peter Hung

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

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

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

---
 drivers/usb/serial/f81232.c | 98 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 77 insertions(+), 21 deletions(-)

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

index 06d1eb0..e1cdf42 100644
@@ -44,6 +44,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
 #define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
 #define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
+#define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01

@@ -156,6 +157,50 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	wake_up_interruptible(&port->port.delta_msr_wait);
 }
 
+static int f81232_set_mctrl(struct usb_serial_port *port,
+					   unsigned int set, unsigned int clear)
+{
+	u8 urb_value;
+	int status;
+	unsigned long flags;

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

Quoted 10 lines

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

+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+		return 0;	/* no change */
+
+	/* 'set' takes precedence over 'clear' */
+	clear &= ~set;
+
+	/* force enable interrupt with OUT2 */
+	urb_value = UART_MCR_OUT2 | priv->line_control;

You should rename this one priv->modem_control.

And what about locking above?

Quoted 9 lines

drivers/usb/serial/f81232.c +9 -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;

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

Quoted 10 lines

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

+
+	if (set & TIOCM_RTS)
+		urb_value |= UART_MCR_RTS;
+
+	dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+			urb_value, priv->line_control);
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status < 0) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);

Return here on errors and skip the else branch.

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

Quoted 4 lines

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

+	} else {
+		spin_lock_irqsave(&priv->lock, flags);
+		priv->line_control = urb_value;
+		spin_unlock_irqrestore(&priv->lock, flags);

It looks like you could replace the private spin lock with a mutex and protect all accesses to MCR and MSR using it.

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

Quoted 3 lines

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

Return 0;

Quoted 51 lines

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

+}
+
 static void f81232_update_modem_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)

@@ -267,12 +312,6 @@ static void f81232_process_read_urb(struct urb *urb)
 	tty_flip_buffer_push(&port->port);
 }
 
-static int set_control_lines(struct usb_device *dev, u8 value)
-{
-	/* FIXME - Stubbed out for now */
-	return 0;
-}
-
 static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
 	/* FIXME - Stubbed out for now */

@@ -388,15 +427,41 @@ static void f81232_set_termios(struct tty_struct *tty,
 
 static int f81232_tiocmget(struct tty_struct *tty)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	int r;
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *port_priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	u8 mcr, msr;
+
+	/* force get current MSR changed state */
+	f81232_read_msr(port);
+
+	spin_lock_irqsave(&port_priv->lock, flags);
+	mcr = port_priv->line_control;
+	msr = port_priv->modem_status;
+	spin_unlock_irqrestore(&port_priv->lock, flags);
+
+	r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+		(mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+		(msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+		(msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+		(msr & UART_MSR_RI ? TIOCM_RI : 0) |
+		(msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+	return r;
 }
 
 static int f81232_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear)
 {

drivers/usb/serial/f81232.c static void f81232_set_termios(struct tty_struct *tty, · line 464

Quoted 10 lines

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

-	/* FIXME - Stubbed out for now */
-	return 0;
+	int status;
+	struct usb_serial_port *port = tty->driver_data;
+
+	status = f81232_set_mctrl(port, set, clear);
+	if (status < 0)
+		return usb_translate_errors(status);
+	else
+		return 0;

Remove the else branch and just return 0 at the end.

Johan

Peter Hung

author

2015-02-16 07:58 UTC #13 lore raw

PATCH v6 6/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 e1cdf42..e70ec62 100644
@@ -516,24 +516,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-16 07:58 UTC #14 lore raw

PATCH v6 7/10 USB: f81232: fix error in f81232_carrier_raised()

In reply to Peter Hung

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

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

---
 drivers/usb/serial/f81232.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

drivers/usb/serial/f81232.c +1 ~~-15~~

index e70ec62..c87a3eb 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 {
 	spinlock_t lock;
 	u8 line_control;

@@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 static int f81232_carrier_raised(struct usb_serial_port *port)
 {
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+	if (priv->modem_status & UART_MSR_DCD)
 		return 1;
 	return 0;
 }

Signature · 1 line

1.9.1

Johan Hovold

maintainer

2015-02-17 09:43 UTC #15 lore raw

Re: [PATCH V6 07/10] USB: f81232: fix error in f81232_carrier_raised()

Quoted 34 lines · Peter Hung

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

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

---
 drivers/usb/serial/f81232.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

drivers/usb/serial/f81232.c +0 ~~-14~~

index e70ec62..c87a3eb 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 {
 	spinlock_t lock;
 	u8 line_control;

drivers/usb/serial/f81232.c static void f81232_dtr_rts(struct usb_serial_port *port, int on) · line 500

Quoted 5 lines

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

@@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 static int f81232_carrier_raised(struct usb_serial_port *port)
 {
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+	if (priv->modem_status & UART_MSR_DCD)

Could you fix the separately before removing the unused defines and also add the missing locking?

drivers/usb/serial/f81232.c static void f81232_dtr_rts(struct usb_serial_port *port, int on) · line 503

Quoted 3 lines

drivers/usb/serial/f81232.c +0 -0
 		return 1;
 	return 0;
 }

Thanks,
Johan

Peter Hung

author

2015-02-16 07:58 UTC #16 lore raw

PATCH v6 8/10 USB: f81232: fix read MSR strange value

In reply to Peter Hung

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 fasterly.

So we add more check not only UART_MSR_ANY_DELTA but also with comparing DCD/RI/DSR/CTS change with old value. Due to 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 | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

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

index c87a3eb..94c05d7 100644
@@ -110,7 +110,8 @@ static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;
 	unsigned long flags;
-	u8 current_msr;
+	u8 current_msr, prev_msr;
+	u8 msr_mask = ~UART_MSR_ANY_DELTA;
 	struct tty_struct *tty;
 	struct f81232_private *priv = usb_get_serial_port_data(port);

@@ -123,7 +124,21 @@ static void f81232_read_msr(struct usb_serial_port *port)
 		return;
 	}
 
-	if (!(current_msr & UART_MSR_ANY_DELTA))
+	/*
+	*  The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
+	*  maybe not change when set & get MCR fasterly.
+	*
+	*  So we add more check with comparing DCD/RI/DSR/CTS
+	*  change. and direct save msr when read.
+	*/
+
+	spin_lock_irqsave(&priv->lock, flags);
+	prev_msr = priv->modem_status;
+	priv->modem_status = current_msr;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (!(current_msr & UART_MSR_ANY_DELTA) &&
+		!((prev_msr ^ current_msr) & msr_mask))
 		return;
 
 	tty = tty_port_tty_get(&port->port);

@@ -136,10 +151,6 @@ static void f81232_read_msr(struct usb_serial_port *port)
 		tty_kref_put(tty);
 	}
 
-	spin_lock_irqsave(&priv->lock, flags);
-	priv->modem_status = current_msr;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
 	wake_up_interruptible(&port->port.delta_msr_wait);
 }

Signature · 1 line

1.9.1

Johan Hovold

maintainer

2015-02-17 09:51 UTC #17 lore raw

Re: [PATCH V6 08/10] USB: f81232: fix read MSR strange value

Quoted 5 lines · Peter Hung

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 fasterly.

I think you mean "rapidly" here.

Quoted 9 lines

So we add more check not only UART_MSR_ANY_DELTA but also with comparing DCD/RI/DSR/CTS change with old value. Due to 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

Without having looked at this very closely; are you sure this is a hardware issue and not related to the locking issues I pointed at in my comments to tiocmset?

Johan

Peter Hung

author

2015-02-24 02:17 UTC #18 lore raw

Re: [PATCH V6 08/10] USB: f81232: fix read MSR strange value

Hello,

Johan Hovold 於 2015/2/17 下午 05:51 寫道:

Quoted 10 lines

Quoted 9 lines
So we add more check not only UART_MSR_ANY_DELTA but also with comparing DCD/RI/DSR/CTS change with old value. Due to 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

Quoted 6 lines

Without having looked at this very closely; are you sure this is a hardware issue and not related to the locking issues I pointed at in my comments to tiocmset?
Johan

Thank for your review.

I'll apply all suggestions to next v7 patch.

Signature · 2 lines

With Best Regards,
Peter Hung

Peter Hung

author

2015-02-16 07:58 UTC #19 lore raw

PATCH v6 9/10 USB: f81232: implement delta change for MSR count

In reply to Peter Hung

We implement delta change for MSR counting. This patch is referenced from ftdi_sio.c

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

---
 drivers/usb/serial/f81232.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

drivers/usb/serial/f81232.c +22 -5

index 94c05d7..5134a19 100644
@@ -112,6 +112,7 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	unsigned long flags;
 	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);

@@ -141,14 +142,30 @@ static void f81232_read_msr(struct usb_serial_port *port)
 		!((prev_msr ^ current_msr) & msr_mask))
 		return;
 
-	tty = tty_port_tty_get(&port->port);
-	if (tty) {
-		if (current_msr & UART_MSR_DDCD) {
+	/* 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);
+			tty_kref_put(tty);
+		}
 	}
 
 	wake_up_interruptible(&port->port.delta_msr_wait);

Signature · 1 line

1.9.1

Peter Hung

author

2015-02-16 07:59 UTC #20 lore raw

PATCH v6 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 5134a19..5e35859 100644
@@ -632,5 +632,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