[PATCH V7 00/11] USB: f81232: V7 patches

[PATCH V7 00/11] USB: f81232: V7 patches

[Peter Hung]

This series patch V7 is changed from V6 as following:

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

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

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

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

-- 
1.9.1

[PATCH V7 01/11] 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 V7 02/11] USB: f81232: add preparatory functions

[Peter Hung]

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

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 669a2f2..1f29b95 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -30,6 +30,11 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST	0xA0
+#define F81232_GET_REGISTER	0xc0
+#define F81232_SET_REGISTER	0x40
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 
@@ -50,6 +55,80 @@ struct f81232_private {
 	u8 modem_status;
 };
 
+static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	if (!data)
+		return -EFAULT;
+
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	status = usb_control_msg(dev,
+				usb_rcvctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_GET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_GET_TIMEOUT);
+	if (status <= 0) {
+		/* show something with failed */
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else {
+		status = 0; /* on success */
+		memcpy((void*) data, (void*) tmp, sizeof(u8));
+	}
+
+	kfree(tmp);
+	return status;
+}
+
+static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	memcpy((void*) tmp, (void*) &data, sizeof(u8));
+
+	status = usb_control_msg(dev,
+				usb_sndctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_SET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_SET_TIMEOUT);
+	if (status <= 0) {
+		/* show something with failed */
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else
+		status = 0; /* on success */
+
+	kfree(tmp);
+	return status;
+}
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
-- 
1.9.1

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

[Johan Hovold]

On Tue, Feb 24, 2015 at 06:16:17PM +0800, Peter Hung wrote:
> We add f81232_get_register()/f81232_set_register() and necessary defines
> for future patch with communication with F81232 USB control endpoint.
>
> Because of this is a preparatory patch, there are unused function warning
> with compiling. The functions will used with following patches.

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

> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..1f29b95 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -30,6 +30,11 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST	0xA0
> +#define F81232_GET_REGISTER	0xc0
> +#define F81232_SET_REGISTER	0x40
> +
>  #define CONTROL_DTR			0x01
>  #define CONTROL_RTS			0x02
>  
> @@ -50,6 +55,80 @@ struct f81232_private {
>  	u8 modem_status;
>  };
>  
> +static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> +{

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

> +	int status;
> +	u8 *tmp;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	if (!data)
> +		return -EFAULT;

No need to test for NULL here.

> +
> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;

Use sizeof(*val) throughout.

> +
> +	status = usb_control_msg(dev,
> +				usb_rcvctrlpipe(dev, 0),
> +				F81232_REGISTER_REQUEST,
> +				F81232_GET_REGISTER,
> +				reg,
> +				0,
> +				tmp,
> +				sizeof(u8),
> +				USB_CTRL_GET_TIMEOUT);
> +	if (status <= 0) {

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

> +		/* show something with failed */

Drop the comment.

> +		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> +
> +		if (status == 0)
> +			status = -EIO;
> +		else
> +			status = usb_translate_errors(status);
> +	} else {
> +		status = 0; /* on success */

Comment not needed.

> +		memcpy((void*) data, (void*) tmp, sizeof(u8));

Just do

	*val = *tmp;

here.

> +	}
> +
> +	kfree(tmp);
> +	return status;
> +}
> +
> +static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)

Use val instead of data here as well.

> +{
> +	int status;
> +	u8 *tmp;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	memcpy((void*) tmp, (void*) &data, sizeof(u8));

*tmp = *val

> +
> +	status = usb_control_msg(dev,
> +				usb_sndctrlpipe(dev, 0),
> +				F81232_REGISTER_REQUEST,
> +				F81232_SET_REGISTER,
> +				reg,
> +				0,
> +				tmp,
> +				sizeof(u8),
> +				USB_CTRL_SET_TIMEOUT);
> +	if (status <= 0) {
> +		/* show something with failed */
> +		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> +
> +		if (status == 0)
> +			status = -EIO;
> +		else
> +			status = usb_translate_errors(status);
> +	} else
> +		status = 0; /* on success */

Missing brackets on else branch. No need for comment.

> +
> +	kfree(tmp);
> +	return status;
> +}

Most comments to get_register apply here as well.

>  static void f81232_update_line_status(struct usb_serial_port *port,
>  				      unsigned char *data,
>  				      unsigned int actual_length)

Thanks,
Johan

[PATCH V7 03/11] 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 implemented in f81232_process_read_urb().

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

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

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

[Johan Hovold]

On Tue, Feb 24, 2015 at 06:16:18PM +0800, Peter Hung wrote:
> The F81232 bulk-in is RX data + LSR channel, data format is
> [LSR+Data][LSR+Data]..... , We had implemented in f81232_process_read_urb().
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 69 +++++++++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 1f29b95..419e2d6 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) },
> @@ -183,44 +184,46 @@ exit:
>  static void f81232_process_read_urb(struct urb *urb)
>  {
>  	struct usb_serial_port *port = urb->context;
> -	struct f81232_private *priv = usb_get_serial_port_data(port);
>  	unsigned char *data = urb->transfer_buffer;
> -	char tty_flag = TTY_NORMAL;
> -	unsigned long flags;
> -	u8 line_status;
> +	char tty_flag;
>  	int i;

Use an unsigned type for i.

> +	u8 lsr;
>  
> -	/* update line status */
> -	spin_lock_irqsave(&priv->lock, flags);
> -	line_status = priv->modem_status;
> -	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
	> -	spin_unlock_irqrestore(&priv->lock, flags);
> -
> -	if (!urb->actual_length)
> +	if (urb->actual_length < 2)
>  		return;
>  
> -	/* break takes precedence over parity, */
> -	/* which takes precedence over framing errors */
> -	if (line_status & UART_BREAK_ERROR)
> -		tty_flag = TTY_BREAK;
> -	else if (line_status & UART_PARITY_ERROR)
> -		tty_flag = TTY_PARITY;
> -	else if (line_status & UART_FRAME_ERROR)
> -		tty_flag = TTY_FRAME;
> -	dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> -
> -	/* overrun is special, not associated with a char */
> -	if (line_status & UART_OVERRUN_ERROR)
> -		tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> -
> -	if (port->port.console && port->sysrq) {
> -		for (i = 0; i < urb->actual_length; ++i)
> -			if (!usb_serial_handle_sysrq_char(port, data[i]))
> -				tty_insert_flip_char(&port->port, data[i],
> -						tty_flag);
> -	} else {
> -		tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
> -							urb->actual_length);
> +	/* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
> +
> +	for (i = 0; i < urb->actual_length; i += 2) {

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

> +		tty_flag = TTY_NORMAL;
> +		lsr = data[i + 0];

Just use data[i] here.

> +
> +		if (lsr & UART_LSR_BRK_ERROR_BITS) {
> +			if (lsr & UART_LSR_BI) {
> +				tty_flag = TTY_BREAK;
> +				port->icount.brk++;
> +				usb_serial_handle_break(port);
> +			} else if (lsr & UART_LSR_PE) {
> +				tty_flag = TTY_PARITY;
> +				port->icount.parity++;
> +			} else if (lsr & UART_LSR_FE) {
> +				tty_flag = TTY_FRAME;
> +				port->icount.frame++;
> +			}
> +
> +			if (lsr & UART_LSR_OE) {
> +				port->icount.overrun++;
> +				tty_insert_flip_char(&port->port, 0,
> +					TTY_OVERRUN);

Use at least two tabs indentation for continuation lines.

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

Looks good now, otherwise.

Thanks,
Johan

[PATCH V7 04/11] USB: f81232: change lock mechanism

[Peter Hung]

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

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 419e2d6..bf072fe 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -19,7 +19,7 @@
 #include <linux/serial.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
@@ -51,7 +51,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_CTS			0x80
 
 struct f81232_private {
-	spinlock_t lock;
+	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
 };
@@ -306,17 +306,16 @@ static void f81232_close(struct usb_serial_port *port)
 static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 {
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	unsigned long flags;
 	u8 control;
 
-	spin_lock_irqsave(&priv->lock, flags);
+	mutex_lock(&priv->lock);
 	/* Change DTR and RTS */
 	if (on)
 		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
 	else
 		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
 	control = priv->line_control;
-	spin_unlock_irqrestore(&priv->lock, flags);
+	mutex_unlock(&priv->lock);
 	set_control_lines(port->serial->dev, control);
 }
 
@@ -360,7 +359,7 @@ static int f81232_port_probe(struct usb_serial_port *port)
 	if (!priv)
 		return -ENOMEM;
 
-	spin_lock_init(&priv->lock);
+	mutex_init(&priv->lock);
 
 	usb_set_serial_port_data(port, priv);
 
-- 
1.9.1

[PATCH V7 05/11] 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()
function.

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

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

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

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

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index bf072fe..339be30 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_GET_REGISTER	0xc0
 #define F81232_SET_REGISTER	0x40
 
+#define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 
@@ -54,6 +57,8 @@ struct f81232_private {
 	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
@@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
 	kfree(tmp);
 	return status;
 }
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	u8 current_msr, prev_msr;
+	u8 msr_mask = ~UART_MSR_ANY_DELTA;
+	u8 msr_changed_bit;
+	struct tty_struct *tty;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+			&current_msr);
+	if (status) {
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		return;
+	}
+
+	/*
+	*  The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
+	*  maybe not change when set MCR & get MSR rapidly.
+	*
+	*  So we add more check with comparing DCD/RI/DSR/CTS
+	*  change. and direct save msr when read.
+	*/
+
+	mutex_lock(&priv->lock);
+	prev_msr = priv->modem_status;
+	priv->modem_status = current_msr;
+	mutex_unlock(&priv->lock);
+
+	if (!(current_msr & UART_MSR_ANY_DELTA) &&
+		!((prev_msr ^ current_msr) & msr_mask))
+		return;
+
+	/* find checked delta bits set */
+	msr_changed_bit =
+		(current_msr & UART_MSR_ANY_DELTA) << 4;
+
+	/* append with not delta but changed bits */
+	msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
+
+	if (msr_changed_bit & UART_MSR_CTS)
+		port->icount.cts++;
+	if (msr_changed_bit & UART_MSR_DSR)
+		port->icount.dsr++;
+	if (msr_changed_bit & UART_MSR_RI)
+		port->icount.rng++;
+	if (msr_changed_bit & UART_MSR_DCD) {
+
+		port->icount.dcd++;
+		tty = tty_port_tty_get(&port->port);
+		if (tty) {
+
+			usb_serial_handle_dcd_change(port, tty,
+					current_msr & UART_MSR_DCD);
+
+			tty_kref_put(tty);
+		}
+	}
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
-				      unsigned int actual_length)
+				      size_t actual_length)
 {
-	/*
-	 * FIXME: Update port->icount, and call
-	 *
-	 *		wake_up_interruptible(&port->port.delta_msr_wait);
-	 *
-	 *	  on MSR changes.
-	 */
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (!actual_length)
+		return;
+
+	switch (data[0] & 0x07) {
+	case 0x00: /* msr change */
+		dev_dbg(&port->dev, "IIR: MSR Change: %02x\n", data[0]);
+		schedule_work(&priv->interrupt_work);
+		break;
+	case 0x02: /* tx-empty */
+		break;
+	case 0x04: /* rx data available */
+		break;
+	case 0x06: /* lsr change */
+		/* we can forget it. the LSR will read from bulk-in */
+		dev_dbg(&port->dev, "IIR: LSR Change: %02x\n", data[0]);
+		break;
+	}
 }
 
 static void f81232_read_int_callback(struct urb *urb)
@@ -351,6 +431,14 @@ static int f81232_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static void  f81232_interrupt_work(struct work_struct *work)
+{
+	struct f81232_private *priv =
+		container_of(work, struct f81232_private, interrupt_work);
+
+	f81232_read_msr(priv->port);
+}
+
 static int f81232_port_probe(struct usb_serial_port *port)
 {
 	struct f81232_private *priv;
@@ -360,10 +448,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	mutex_init(&priv->lock);
+	INIT_WORK(&priv->interrupt_work,  f81232_interrupt_work);
 
 	usb_set_serial_port_data(port, priv);
 
 	port->port.drain_delay = 256;
+	priv->port = port;
 
 	return 0;
 }
-- 
1.9.1

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

[Johan Hovold]

On Tue, Feb 24, 2015 at 06:16:20PM +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()
> function.
> 
> We also confirmd MSR strange delta value is not locking-issue. The issue
> is set MCR & get MSR before IIR notice with MSR changed (Loopback only).

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

You need to update dtr_rts as well.

> When we use RS232 loopback, assume doing RTS change will cause
> CTS change, DTR change will cause DCD/DSR change too.
> 
> Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of
> MSR(delta) maybe not changed when set & get MCR rapidly.
> 
> So we add more check not only UART_MSR_ANY_DELTA but also with
> comparing DCD/RI/DSR/CTS change with old value. Because of the state
> bit is always correct, we direct save msr when read.
> 
> The following step to reproduce this problem with while loop step 1~4:
> 1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
> 2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
> 3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
> 4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 98 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index bf072fe..339be30 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define F81232_GET_REGISTER	0xc0
>  #define F81232_SET_REGISTER	0x40
>  
> +#define SERIAL_BASE_ADDRESS	0x0120
> +#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
> +
>  #define CONTROL_DTR			0x01
>  #define CONTROL_RTS			0x02
>  
> @@ -54,6 +57,8 @@ struct f81232_private {
>  	struct mutex lock;
>  	u8 line_control;
>  	u8 modem_status;
> +	struct work_struct interrupt_work;
> +	struct usb_serial_port *port;
>  };
>  
>  static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> @@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
>  	kfree(tmp);
>  	return status;
>  }
> +
> +static void f81232_read_msr(struct usb_serial_port *port)
> +{
> +	int status;
> +	u8 current_msr, prev_msr;
> +	u8 msr_mask = ~UART_MSR_ANY_DELTA;
> +	u8 msr_changed_bit;
> +	struct tty_struct *tty;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> +	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
> +			&current_msr);
> +	if (status) {
> +		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
> +		return;
> +	}
> +
> +	/*
> +	*  The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
> +	*  maybe not change when set MCR & get MSR rapidly.
> +	*
> +	*  So we add more check with comparing DCD/RI/DSR/CTS
> +	*  change. and direct save msr when read.
> +	*/

The asterisks should be aligned.

> +
> +	mutex_lock(&priv->lock);
> +	prev_msr = priv->modem_status;
> +	priv->modem_status = current_msr;
> +	mutex_unlock(&priv->lock);
> +
> +	if (!(current_msr & UART_MSR_ANY_DELTA) &&
> +		!((prev_msr ^ current_msr) & msr_mask))
> +		return;
> +
> +	/* find checked delta bits set */
> +	msr_changed_bit =
> +		(current_msr & UART_MSR_ANY_DELTA) << 4;
> +
> +	/* append with not delta but changed bits */
> +	msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
> +
> +	if (msr_changed_bit & UART_MSR_CTS)
> +		port->icount.cts++;
> +	if (msr_changed_bit & UART_MSR_DSR)
> +		port->icount.dsr++;
> +	if (msr_changed_bit & UART_MSR_RI)
> +		port->icount.rng++;
> +	if (msr_changed_bit & UART_MSR_DCD) {
> +

No empty line.

> +		port->icount.dcd++;
> +		tty = tty_port_tty_get(&port->port);
> +		if (tty) {
> +

No empty line.

> +			usb_serial_handle_dcd_change(port, tty,
> +					current_msr & UART_MSR_DCD);
> +
> +			tty_kref_put(tty);
> +		}
> +	}
> +
> +	wake_up_interruptible(&port->port.delta_msr_wait);
> +}

Johan

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

[Peter Hung]

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

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 339be30..21f606f 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01
@@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct f81232_private {
 	struct mutex lock;
-	u8 line_control;
+	u8 modem_control;
 	u8 modem_status;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;
@@ -198,6 +199,52 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	wake_up_interruptible(&port->port.delta_msr_wait);
 }
 
+static int f81232_set_mctrl(struct usb_serial_port *port,
+					   unsigned int set, unsigned int clear)
+{
+	u8 urb_value;
+	int status;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+		return 0;	/* no change */
+
+	/* 'set' takes precedence over 'clear' */
+	clear &= ~set;
+
+	/* force enable interrupt with OUT2 */
+	mutex_lock(&priv->lock);
+	urb_value = UART_MCR_OUT2 | priv->modem_control;
+	mutex_unlock(&priv->lock);
+
+	if (clear & TIOCM_DTR)
+		urb_value &= ~UART_MCR_DTR;
+
+	if (clear & TIOCM_RTS)
+		urb_value &= ~UART_MCR_RTS;
+
+	if (set & TIOCM_DTR)
+		urb_value |= UART_MCR_DTR;
+
+	if (set & TIOCM_RTS)
+		urb_value |= UART_MCR_RTS;
+
+	dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+			urb_value, priv->modem_control);
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+		return status;
+	} else {
+		mutex_lock(&priv->lock);
+		priv->modem_control = urb_value;
+		mutex_unlock(&priv->lock);
+	}
+
+	return 0;
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      size_t actual_length)
@@ -309,12 +356,6 @@ static void f81232_process_read_urb(struct urb *urb)
 	tty_flip_buffer_push(&port->port);
 }
 
-static int set_control_lines(struct usb_device *dev, u8 value)
-{
-	/* FIXME - Stubbed out for now */
-	return 0;
-}
-
 static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
 	/* FIXME - Stubbed out for now */
@@ -342,15 +383,35 @@ static void f81232_set_termios(struct tty_struct *tty,
 
 static int f81232_tiocmget(struct tty_struct *tty)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	int r;
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *port_priv = usb_get_serial_port_data(port);
+	u8 mcr, msr;
+
+	/* force get current MSR changed state */
+	f81232_read_msr(port);
+
+	mutex_lock(&port_priv->lock);
+	mcr = port_priv->modem_control;
+	msr = port_priv->modem_status;
+	mutex_unlock(&port_priv->lock);
+
+	r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+		(mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+		(msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+		(msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+		(msr & UART_MSR_RI ? TIOCM_RI : 0) |
+		(msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+	return r;
 }
 
 static int f81232_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	struct usb_serial_port *port = tty->driver_data;
+
+	return f81232_set_mctrl(port, set, clear);
 }
 
 static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
@@ -385,24 +446,22 @@ static void f81232_close(struct usb_serial_port *port)
 
 static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct f81232_private *priv = usb_get_serial_port_data(port);
-	u8 control;
-
-	mutex_lock(&priv->lock);
-	/* Change DTR and RTS */
 	if (on)
-		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
 	else
-		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
-	control = priv->line_control;
-	mutex_unlock(&priv->lock);
-	set_control_lines(port->serial->dev, control);
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static int f81232_carrier_raised(struct usb_serial_port *port)
 {
+	u8 msr;
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+
+	mutex_lock(&priv->lock);
+	msr = priv->modem_status;
+	mutex_unlock(&priv->lock);
+
+	if (msr & UART_MSR_DCD)
 		return 1;
 	return 0;
 }
-- 
1.9.1

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

[Johan Hovold]

On Tue, Feb 24, 2015 at 06:16:21PM +0800, Peter Hung wrote:
> This patch implement relative MCR/MSR function, such like
> tiocmget()/tiocmset()/dtr_rts()/carrier_raised()
> 
> original f81232_carrier_raised() compared with wrong value UART_DCD.
> It's should compared with UART_MSR_DCD.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>

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

> ---
>  drivers/usb/serial/f81232.c | 103 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 81 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 339be30..21f606f 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define F81232_SET_REGISTER	0x40
>  
>  #define SERIAL_BASE_ADDRESS	0x0120
> +#define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
>  #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
>  
>  #define CONTROL_DTR			0x01
> @@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  
>  struct f81232_private {
>  	struct mutex lock;
> -	u8 line_control;
> +	u8 modem_control;
>  	u8 modem_status;
>  	struct work_struct interrupt_work;
>  	struct usb_serial_port *port;
> @@ -198,6 +199,52 @@ static void f81232_read_msr(struct usb_serial_port *port)
>  	wake_up_interruptible(&port->port.delta_msr_wait);
>  }
>  
> +static int f81232_set_mctrl(struct usb_serial_port *port,
> +					   unsigned int set, unsigned int clear)
> +{
> +	u8 urb_value;
> +	int status;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> +	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
> +		return 0;	/* no change */
> +
> +	/* 'set' takes precedence over 'clear' */
> +	clear &= ~set;
> +
> +	/* force enable interrupt with OUT2 */
> +	mutex_lock(&priv->lock);
> +	urb_value = UART_MCR_OUT2 | priv->modem_control;
> +	mutex_unlock(&priv->lock);

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

> +
> +	if (clear & TIOCM_DTR)
> +		urb_value &= ~UART_MCR_DTR;
> +
> +	if (clear & TIOCM_RTS)
> +		urb_value &= ~UART_MCR_RTS;
> +
> +	if (set & TIOCM_DTR)
> +		urb_value |= UART_MCR_DTR;
> +
> +	if (set & TIOCM_RTS)
> +		urb_value |= UART_MCR_RTS;
> +
> +	dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
> +			urb_value, priv->modem_control);
> +
> +	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
> +	if (status) {
> +		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
> +		return status;
> +	} else {
> +		mutex_lock(&priv->lock);
> +		priv->modem_control = urb_value;
> +		mutex_unlock(&priv->lock);
> +	}

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

> +
> +	return 0;
> +}

Johan

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

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 21f606f..f5c9060 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -37,6 +37,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
 #define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
@@ -367,6 +369,33 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static int f81232_port_enable(struct usb_serial_port *port, int enable)
+{
+	u8 data = 0;
+	int status;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+			| UART_FCR_CLEAR_XMIT;
+
+	status = f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set FCR: %d\n", __func__, status);
+		return status;
+	}
+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	data = enable ? UART_IER_MSI : 0;
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set IER: %d\n", __func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
@@ -418,6 +447,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_enable(port, 1);
+	if (result) {
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+		return result;
+	}
+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);
@@ -440,6 +475,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void f81232_close(struct usb_serial_port *port)
 {
+	int result = f81232_port_enable(port, 0);
+	if (result)
+		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 }
-- 
1.9.1

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

[Johan Hovold]

On Tue, Feb 24, 2015 at 06:16:22PM +0800, Peter Hung wrote:
> We put FCR/IER initial step to f81232_port_enable(). When port is open,
> it set MSR interrupt on. Otherwise set it off.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 21f606f..f5c9060 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -37,6 +37,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define F81232_SET_REGISTER	0x40
>  
>  #define SERIAL_BASE_ADDRESS	0x0120
> +#define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
>  #define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
>  #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
>  
> @@ -367,6 +369,33 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>  	 */
>  }
>  
> +static int f81232_port_enable(struct usb_serial_port *port, int enable)

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

> +{
> +	u8 data = 0;
> +	int status;
> +
> +	/* fifo on, trigger8, clear TX/RX*/
> +	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
> +			| UART_FCR_CLEAR_XMIT;

Put the final | before the line break.

> +
> +	status = f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to set FCR: %d\n", __func__, status);
> +		return status;
> +	}
> +
> +	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
> +	data = enable ? UART_IER_MSI : 0;
> +
> +	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to set IER: %d\n", __func__, status);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
>  static void f81232_set_termios(struct tty_struct *tty,
>  		struct usb_serial_port *port, struct ktermios *old_termios)
>  {
> @@ -418,6 +447,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>  {
>  	int result;
>  
> +	result = f81232_port_enable(port, 1);
> +	if (result) {
> +		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
> +		return result;

You already log the error in port_enable.

> +	}
> +
>  	/* Setup termios */
>  	if (tty)
>  		f81232_set_termios(tty, port, NULL);
> @@ -440,6 +475,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>  
>  static void f81232_close(struct usb_serial_port *port)
>  {
> +	int result = f81232_port_enable(port, 0);

Separate the declaration from the function call.

> +	if (result)
> +		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
> +

Already logged as well.

>  	usb_serial_generic_close(port);
>  	usb_kill_urb(port->interrupt_in_urb);
>  }

Johan

[PATCH V7 08/11] 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 in f81232_set_termios().

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index f5c9060..0c96b9a 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE	115200
+
 /* USB Control EP parameter */
 #define F81232_REGISTER_REQUEST	0xA0
 #define F81232_GET_REGISTER	0xc0
 #define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define RECEIVE_BUFFER_REGISTER	(0x00 + SERIAL_BASE_ADDRESS)
 #define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
 #define FIFO_CONTROL_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER	(0x03 + SERIAL_BASE_ADDRESS)
 #define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
@@ -64,6 +69,14 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(u32 baudrate)
+{
+	if (!baudrate)
+		return 0;
+	else
+		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
 {
 	int status;
@@ -369,6 +382,52 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
+{
+	u8 divisor, lcr;
+	int status = 0;
+
+	if (!baudrate)
+		return;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_get_register(port, LINE_CONTROL_REGISTER,
+			 &lcr); /* get LCR */
+	if (status) {
+		dev_err(&port->dev, "%s failed to get LCR: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 lcr | UART_LCR_DLAB); /* Enable DLAB */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",
+			__func__, status);
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			lcr & ~UART_LCR_DLAB);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+}
+
 static int f81232_port_enable(struct usb_serial_port *port, int enable)
 {
 	u8 data = 0;
@@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable)
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	/* FIXME - Stubbed out for now */
+	u8 new_lcr = 0;
+	int status = 0;
+
 
 	/* Don't change anything if nothing has changed */
 	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
 		return;
 
-	/* Do the real work here... */
-	if (old_termios)
-		tty_termios_copy_hw(&tty->termios, old_termios);
+	if (C_BAUD(tty) == B0)
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
+	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
+
+	f81232_set_baudrate(port, tty_get_baud_rate(tty));
+
+	if (C_PARENB(tty)) {
+		new_lcr |= UART_LCR_PARITY;
+
+		if (!C_PARODD(tty))
+			new_lcr |= UART_LCR_EPAR;
+
+		if (C_CMSPAR(tty))
+			new_lcr |= UART_LCR_SPAR;
+	}
+
+	if (C_CSTOPB(tty))
+		new_lcr |= UART_LCR_STOP;
+
+	switch (C_CSIZE(tty)) {
+	case CS5:
+		new_lcr |= UART_LCR_WLEN5;
+		break;
+	case CS6:
+		new_lcr |= UART_LCR_WLEN6;
+		break;
+	case CS7:
+		new_lcr |= UART_LCR_WLEN7;
+		break;
+	default:
+	case CS8:
+		new_lcr |= UART_LCR_WLEN8;
+		break;
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
+	if (status)
+		dev_err(&port->dev, "%s failed to set LCR: %d\n", __func__, status);
+
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)
-- 
1.9.1

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

[Johan Hovold]

On Tue, Feb 24, 2015 at 06:16:23PM +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 in f81232_set_termios().
> 
> This patch also implement DTR/RTS control when baudrate B0.
> We drop DTR/RTS when B0, otherwise enable it.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index f5c9060..0c96b9a 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* Maximum baudrate for F81232 */
> +#define F81232_MAX_BAUDRATE	115200
> +
>  /* USB Control EP parameter */
>  #define F81232_REGISTER_REQUEST	0xA0
>  #define F81232_GET_REGISTER	0xc0
>  #define F81232_SET_REGISTER	0x40
>  
>  #define SERIAL_BASE_ADDRESS	0x0120
> +#define RECEIVE_BUFFER_REGISTER	(0x00 + SERIAL_BASE_ADDRESS)
>  #define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
>  #define FIFO_CONTROL_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER	(0x03 + SERIAL_BASE_ADDRESS)
>  #define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
>  #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
>  
> @@ -64,6 +69,14 @@ struct f81232_private {
>  	struct usb_serial_port *port;
>  };
>  
> +static int calc_baud_divisor(u32 baudrate)
> +{
> +	if (!baudrate)
> +		return 0;

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

> +	else
> +		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
> +}
> +
>  static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
>  {
>  	int status;
> @@ -369,6 +382,52 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>  	 */
>  }
>  
> +static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
> +{
> +	u8 divisor, lcr;
> +	int status = 0;
> +
> +	if (!baudrate)
> +		return;
> +
> +	divisor = calc_baud_divisor(baudrate);
> +
> +	status = f81232_get_register(port, LINE_CONTROL_REGISTER,
> +			 &lcr); /* get LCR */
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to get LCR: %d\n",
> +			__func__, status);
> +	}
> +
> +	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
> +			 lcr | UART_LCR_DLAB); /* Enable DLAB */
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
> +			__func__, status);
> +	}
> +
> +	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
> +			 divisor & 0x00ff); /* low */
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
> +			__func__, status);
> +	}
> +
> +	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
> +			 (divisor & 0xff00) >> 8); /* high */
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",
> +			__func__, status);
> +	}
> +
> +	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
> +			lcr & ~UART_LCR_DLAB);
> +	if (status) {
> +		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
> +			__func__, status);
> +	}
> +}

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

> +
>  static int f81232_port_enable(struct usb_serial_port *port, int enable)
>  {
>  	u8 data = 0;
> @@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable)
>  static void f81232_set_termios(struct tty_struct *tty,
>  		struct usb_serial_port *port, struct ktermios *old_termios)
>  {
> -	/* FIXME - Stubbed out for now */
> +	u8 new_lcr = 0;
> +	int status = 0;
> +
>  
>  	/* Don't change anything if nothing has changed */
>  	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
>  		return;
>  
> -	/* Do the real work here... */
> -	if (old_termios)
> -		tty_termios_copy_hw(&tty->termios, old_termios);
> +	if (C_BAUD(tty) == B0)
> +		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
> +	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
> +		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
> +
> +	f81232_set_baudrate(port, tty_get_baud_rate(tty));

Check for B0 here instead of in the helper.

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

Johan

[PATCH V7 09/11] 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 0c96b9a..3b0da70 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -603,24 +603,32 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
 	return 0;
 }
 
+static int f81232_get_serial_info(struct usb_serial_port *port,
+		unsigned long arg)
+{
+	struct serial_struct ser;
+
+	memset(&ser, 0, sizeof(ser));
+
+	ser.type = PORT_16550A;
+	ser.line = port->minor;
+	ser.port = port->port_number;
+	ser.baud_base = 115200;
+
+	if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int f81232_ioctl(struct tty_struct *tty,
 			unsigned int cmd, unsigned long arg)
 {
-	struct serial_struct ser;
 	struct usb_serial_port *port = tty->driver_data;
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		memset(&ser, 0, sizeof ser);
-		ser.type = PORT_16654;
-		ser.line = port->minor;
-		ser.port = port->port_number;
-		ser.baud_base = 460800;
-
-		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
-			return -EFAULT;
-
-		return 0;
+		return f81232_get_serial_info(port, arg);
 	default:
 		break;
 	}
-- 
1.9.1

[PATCH V7 10/11] USB: f81232: cleanup non-used define

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 3b0da70..ee9a2a2 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 {
 	struct mutex lock;
 	u8 modem_control;
-- 
1.9.1

[PATCH V7 11/11] 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 ee9a2a2..23f0a17 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -691,5 +691,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
 module_usb_serial_driver(serial_drivers, id_table);
 
 MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
-MODULE_AUTHOR("Greg Kroah-Hartman <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