[PATCH V6 00/10] USB: f81232: V6 patches

[PATCH V6 00/10] USB: f81232: V6 patches

[Peter Hung]

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(-)

-- 
1.9.1

[PATCH V6 01/10] USB: f81232: rename private struct member name

[Peter Hung]

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

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

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

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

[Peter Hung]

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

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

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

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

[Johan Hovold]

On Mon, Feb 16, 2015 at 03:57:54PM +0800, Peter Hung wrote:
> The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
> , We will do read MSR with interrupt_work worker to do f81232_read_msr() func.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 100 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..ec4609d 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,6 +23,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/usb.h>
>  #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
>  
>  static const struct usb_device_id id_table[] = {
>  	{ USB_DEVICE(0x1934, 0x0706) },
> @@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST 0xA0
> +#define F81232_GET_REGISTER 0xc0

Indent these define values using a tab as well.

> +
> +#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.

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

> +{
> +	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.

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

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

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

	"%s failed: %d\n"

> +
> +	return status;
> +}

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

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

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

No need for this comment.

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

> +		}
> +
> +		tty_kref_put(tty);
> +	}

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

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

>  				      unsigned char *data,
>  				      unsigned int actual_length)

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

>  {
> -	/*
> -	 * 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]);

%02x

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

> +		break;
> +	}
>  }

Johan

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

[Peter Hung]

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

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

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

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

[Greg KH]

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

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

[Peter Hung]

Hello,

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

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

-- 
With Best Regards,
Peter Hung

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

[David Laight]

From: Greg KH
> > +	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

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

['Greg KH']

On Tue, Feb 17, 2015 at 10:06:07AM +0000, David Laight wrote:
> From: Greg KH
> > > +	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.

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

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

[Peter Hung]

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

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

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

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

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

[Johan Hovold]

On Mon, Feb 16, 2015 at 03:57:56PM +0800, Peter Hung wrote:
> The original driver had do not any h/w change in driver.
> This patch implements with configure H/W for
> baud/parity/word length/stop bits functional.
> 
> Some init step extract to f81232_port_init(), called once with open().
> And refine baudrate setting to f81232_set_baudrate()
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 138 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 9ea498a..06d1eb0 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* Maximum baudrate for F81232 */
> +#define F81232_MAX_BAUDRATE 115200L

No need for L.

> +
>  /* 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);
> +}

You should handle B0 at the call site (and in that case keep the current
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)

No need to break line.

> +{
> +	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.

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

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

As with f81232_get_register 0 is also an error.

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

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

Only clear DLAB?

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

Just assign return value and don't initialise above.

And just bail out on errors directly here.

> +
> +	/* 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.

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

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.

> +
> +	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)
> @@ -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)?

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

Johan

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

[Peter Hung]

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

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

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

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

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

[Johan Hovold]

On Mon, Feb 16, 2015 at 03:57:57PM +0800, Peter Hung wrote:
> This patch implement relative MCR/MSR function, such like
> tiocmget()/tiocmset()/dtr_rts().
> 
> The f81232_set_mctrl() replace set_control_lines() to do MCR control
> so we clean-up the set_control_lines() function.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 98 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 77 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 06d1eb0..e1cdf42 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -44,6 +44,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
>  #define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
>  #define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
> +#define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
>  #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
>  
>  #define CONTROL_DTR			0x01
> @@ -156,6 +157,50 @@ static void f81232_read_msr(struct usb_serial_port *port)
>  	wake_up_interruptible(&port->port.delta_msr_wait);
>  }
>  
> +static int f81232_set_mctrl(struct usb_serial_port *port,
> +					   unsigned int set, unsigned int clear)
> +{
> +	u8 urb_value;
> +	int status;
> +	unsigned long flags;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> +	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
> +		return 0;	/* no change */
> +
> +	/* 'set' takes precedence over 'clear' */
> +	clear &= ~set;
> +
> +	/* force enable interrupt with OUT2 */
> +	urb_value = UART_MCR_OUT2 | priv->line_control;

You should rename this one priv->modem_control.

And what about locking above?

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

Return here on errors and skip the else branch.

> +	} 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.

> +	}
> +
> +	return status;

Return 0;

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

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

Johan

[PATCH V6 06/10] USB: f81232: clarify f81232_ioctl and fix

[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 <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index e1cdf42..e70ec62 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -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;
 	}
-- 
1.9.1

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

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index e70ec62..c87a3eb 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -47,20 +47,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 
-#define CONTROL_DTR			0x01
-#define CONTROL_RTS			0x02
-
-#define UART_STATE			0x08
-#define UART_STATE_TRANSIENT_MASK	0x74
-#define UART_DCD			0x01
-#define UART_DSR			0x02
-#define UART_BREAK_ERROR		0x04
-#define UART_RING			0x08
-#define UART_FRAME_ERROR		0x10
-#define UART_PARITY_ERROR		0x20
-#define UART_OVERRUN_ERROR		0x40
-#define UART_CTS			0x80
-
 struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
@@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 static int f81232_carrier_raised(struct usb_serial_port *port)
 {
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+	if (priv->modem_status & UART_MSR_DCD)
 		return 1;
 	return 0;
 }
-- 
1.9.1

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

[Johan Hovold]

On Mon, Feb 16, 2015 at 03:57:59PM +0800, Peter Hung wrote:
> It's should compared with UART_MSR_DCD, not UART_DCD.
> also we clean-up some non-used define to avoid impropriety use.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index e70ec62..c87a3eb 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -47,20 +47,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define MODEM_CONTROL_REGISTER     (0x04 + SERIAL_BASE_ADDRESS)
>  #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
>  
> -#define CONTROL_DTR			0x01
> -#define CONTROL_RTS			0x02
> -
> -#define UART_STATE			0x08
> -#define UART_STATE_TRANSIENT_MASK	0x74
> -#define UART_DCD			0x01
> -#define UART_DSR			0x02
> -#define UART_BREAK_ERROR		0x04
> -#define UART_RING			0x08
> -#define UART_FRAME_ERROR		0x10
> -#define UART_PARITY_ERROR		0x20
> -#define UART_OVERRUN_ERROR		0x40
> -#define UART_CTS			0x80
> -
>  struct f81232_private {
>  	spinlock_t lock;
>  	u8 line_control;
> @@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
>  static int f81232_carrier_raised(struct usb_serial_port *port)
>  {
>  	struct f81232_private *priv = usb_get_serial_port_data(port);
> -	if (priv->modem_status & UART_DCD)
> +	if (priv->modem_status & UART_MSR_DCD)

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

>  		return 1;
>  	return 0;
>  }

Thanks,
Johan

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

[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 <hpeter+linux_kernel@gmail.com>
---
 drivers/usb/serial/f81232.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index c87a3eb..94c05d7 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -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);
 }
 
-- 
1.9.1

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

[Johan Hovold]

On Mon, Feb 16, 2015 at 03:58:00PM +0800, Peter Hung wrote:
> 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.
 
> 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

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

[Peter Hung]

Hello,

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

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

Thank for your review.

I'll apply all suggestions to next v7 patch.

-- 
With Best Regards,
Peter Hung

[PATCH V6 09/10] USB: f81232: implement delta change for MSR count

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 94c05d7..5134a19 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -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);
-- 
1.9.1

[PATCH V6 10/10] USB: f81232: modify/add author

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 5134a19..5e35859 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -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 <gregkh@linuxfoundation.org");
+MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@linuxfoundation.org>");
+MODULE_AUTHOR("Peter Hong <peter_hong@fintek.com.tw>");
 MODULE_LICENSE("GPL v2");
-- 
1.9.1