[PATCH v4 1/7] usb: serial: modify bulk-in/out size for F81232

[PATCH v4 1/7] usb: serial: modify bulk-in/out size for F81232

[Peter Hung]

The F81232 real bulk-in/out ep buffer size is 64Bytes

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index c5dc233..4f42e9d 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -304,8 +304,8 @@ static struct usb_serial_driver f81232_device = {
 	},
 	.id_table =		id_table,
 	.num_ports =		1,
-	.bulk_in_size =		256,
-	.bulk_out_size =	256,
+	.bulk_in_size =		64,
+	.bulk_out_size =	64,
 	.open =			f81232_open,
 	.close =		f81232_close,
 	.dtr_rts = 		f81232_dtr_rts,
-- 
1.9.1

[PATCH v4 2/7] usb: serial: modify author for F81232

[Peter Hung]

add 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 4f42e9d..9ef9775 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -330,5 +330,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

Re: [PATCH v4 2/7] usb: serial: modify author for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:36PM +0800, Peter Hung wrote:
> add co-author and fix no '>' in greg kh's email

This is trivial enough to be done in one patch I guess, but please put
this one last in the series.

Also for all your patches use a subject on the following format:

	"USB: f81232: ..."

Please also include a cover-letter (e.g. use git format-patch
--cover-letter) where you can describe what has changed when you submit
a new revision of the series.

Johan

[PATCH v4 3/7] usb: serial: implement read IIR/MSR ep for F81232

[Peter Hung]

The F81232 interrupt ep will continuously report IIR register value.
We had implement the interrupt callback to read IIR, If noticed with
MSR change, we will call worker to read MSR later.

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 9ef9775..274120d 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) },
@@ -44,23 +45,112 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define REGISTER_REQUEST 0xA0
+#define GET_REGISTER 0xc0
+#define SET_REGISTER 0x40
+#define F81232_USB_TIMEOUT 3000
+
+#define SERIAL_BASE_ADDRESS	   (0x0120)
+#define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
 struct f81232_private {
 	spinlock_t lock;
 	u8 line_control;
 	u8 line_status;
+
+	struct work_struct int_worker;
+	struct usb_serial_port *port;
 };
 
+static inline int f81232_get_register(struct usb_device *dev,
+							  u16 reg, u8 *data)
+{
+	int status;
+
+	status = usb_control_msg(dev,
+			 usb_rcvctrlpipe(dev, 0),
+			 REGISTER_REQUEST,
+			 GET_REGISTER,
+			 reg,
+			 0,
+			 data,
+			 sizeof(*data),
+			 F81232_USB_TIMEOUT);
+
+	if (status < 0) {
+		dev_dbg(&dev->dev,
+			"%s status: %d\n", __func__, status);
+	}
+
+	return status;
+}
+
+static void f81232_read_msr(struct f81232_private *priv)
+{
+	int status;
+	unsigned long flags;
+	u8 current_msr, old_msr;
+	struct usb_device *dev = priv->port->serial->dev;
+	struct tty_struct *tty;
+
+	status = f81232_get_register(dev, MODEM_STATUS_REGISTER, &current_msr);
+
+	if (status < 0) {
+		dev_dbg(&dev->dev, "%s fail, status: %d\n", __func__, status);
+		return;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+	old_msr = priv->line_status;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (current_msr & UART_MSR_ANY_DELTA) {
+		tty = tty_port_tty_get(&priv->port->port);
+
+		if (tty) {
+			if (current_msr & UART_MSR_DDCD) {
+				usb_serial_handle_dcd_change(priv->port,
+					tty, current_msr & UART_MSR_DCD);
+			}
+
+			tty_kref_put(tty);
+		}
+
+		spin_lock_irqsave(&priv->lock, flags);
+		priv->line_status = current_msr;
+		spin_unlock_irqrestore(&priv->lock, flags);
+
+		wake_up_interruptible(&priv->port->port.delta_msr_wait);
+	}
+
+	dev_dbg(&dev->dev, "%s: %x\n", __func__, priv->line_status);
+}
 static void f81232_update_line_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);
+	struct usb_device *dev = port->serial->dev;
+
+	if (!actual_length)
+		return;
+
+	switch (data[0] & 0x07) {
+	case 0x00: /* msr change */
+		dev_dbg(&dev->dev, "IIR: MSR Change: %x\n", data[0]);
+		schedule_work(&priv->int_worker);
+		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(&dev->dev, "IIR: LSR Change: %x\n", data[0]);
+		break;
+	}
 }
 
 static void f81232_read_int_callback(struct urb *urb)
@@ -270,6 +360,14 @@ static int f81232_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static void f81232_int_work_wq(struct work_struct *work)
+{
+	struct f81232_private *priv =
+		container_of(work, struct f81232_private, int_worker);
+
+	f81232_read_msr(priv);
+}
+
 static int f81232_port_probe(struct usb_serial_port *port)
 {
 	struct f81232_private *priv;
@@ -279,10 +377,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	spin_lock_init(&priv->lock);
+	INIT_WORK(&priv->int_worker, f81232_int_work_wq);
 
 	usb_set_serial_port_data(port, priv);
 
 	port->port.drain_delay = 256;
+	priv->port = port;
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v4 3/7] usb: serial: implement read IIR/MSR ep for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:37PM +0800, Peter Hung wrote:
> The F81232 interrupt ep will continuously report IIR register value.
> We had implement the interrupt callback to read IIR, If noticed with
> MSR change, we will call worker to read MSR later.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 114 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 107 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 9ef9775..274120d 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) },
> @@ -44,23 +45,112 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR		0x40
>  #define UART_CTS			0x80

Now that your including serial_req and using the standard register
masks, you should remove the old (incorrect ones) at some point. Most
are left unused after the whole series has been applied (except UART_DCD
and that looks like a bug).

>  
> +#define REGISTER_REQUEST 0xA0
> +#define GET_REGISTER 0xc0
> +#define SET_REGISTER 0x40

Add a F81232-prefix to these (and other driver specific defines), and a
new line before the time out.

> +#define F81232_USB_TIMEOUT 3000
> +
> +#define SERIAL_BASE_ADDRESS	   (0x0120)

No parentheses.

> +#define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)

Missing newline.

>  struct f81232_private {
>  	spinlock_t lock;
>  	u8 line_control;
>  	u8 line_status;

How about renaming this one modem_status?

> +
> +	struct work_struct int_worker;

Call this one interrupt_work instead.

> +	struct usb_serial_port *port;
>  };
>  
> +static inline int f81232_get_register(struct usb_device *dev,
> +							  u16 reg, u8 *data)

Let the compiler decide whether this should be inlined.

Should should probably also pass the usb-serial port rather than
usb_device and use &port->dev for the error message.

> +{
> +	int status;
> +
> +	status = usb_control_msg(dev,
> +			 usb_rcvctrlpipe(dev, 0),
> +			 REGISTER_REQUEST,
> +			 GET_REGISTER,
> +			 reg,
> +			 0,
> +			 data,
> +			 sizeof(*data),
> +			 F81232_USB_TIMEOUT);
> +
> +	if (status < 0) {
> +		dev_dbg(&dev->dev,
> +			"%s status: %d\n", __func__, status);

dev_err.

Is the line break needed still?

> +	}
> +
> +	return status;
> +}
> +
> +static void f81232_read_msr(struct f81232_private *priv)
> +{
> +	int status;
> +	unsigned long flags;
> +	u8 current_msr, old_msr;
> +	struct usb_device *dev = priv->port->serial->dev;
> +	struct tty_struct *tty;
> +
> +	status = f81232_get_register(dev, MODEM_STATUS_REGISTER, &current_msr);
> +

No newline before checking the return value. Comment applies to whole series.

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

You already logged the error in get_register, but use dev_err (and
&port->dev) if you want this here.

> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	old_msr = priv->line_status;
> +	spin_unlock_irqrestore(&priv->lock, flags);

You never use old_msr so just drop this bit.

> +
> +	if (current_msr & UART_MSR_ANY_DELTA) {

Just return unless there has been a change and reduce the indentation
below.

> +		tty = tty_port_tty_get(&priv->port->port);
> +
> +		if (tty) {
> +			if (current_msr & UART_MSR_DDCD) {
> +				usb_serial_handle_dcd_change(priv->port,
> +					tty, current_msr & UART_MSR_DCD);
> +			}
> +
> +			tty_kref_put(tty);
> +		}
> +
> +		spin_lock_irqsave(&priv->lock, flags);
> +		priv->line_status = current_msr;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +
> +		wake_up_interruptible(&priv->port->port.delta_msr_wait);
> +	}
> +
> +	dev_dbg(&dev->dev, "%s: %x\n", __func__, priv->line_status);
> +}

Missing newline.

>  static void f81232_update_line_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);
> +	struct usb_device *dev = port->serial->dev;

Use &port->dev for debugging and drop this one.

> +
> +	if (!actual_length)
> +		return;
> +
> +	switch (data[0] & 0x07) {
> +	case 0x00: /* msr change */
> +		dev_dbg(&dev->dev, "IIR: MSR Change: %x\n", data[0]);
> +		schedule_work(&priv->int_worker);
> +		break;
> +

I'd drop the newlines after 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(&dev->dev, "IIR: LSR Change: %x\n", data[0]);
> +		break;
> +	}
>  }
>  
>  static void f81232_read_int_callback(struct urb *urb)
> @@ -270,6 +360,14 @@ static int f81232_ioctl(struct tty_struct *tty,
>  	return -ENOIOCTLCMD;
>  }
>  
> +static void f81232_int_work_wq(struct work_struct *work)

Rename this f81232_interrupt_work.

> +{
> +	struct f81232_private *priv =
> +		container_of(work, struct f81232_private, int_worker);
> +
> +	f81232_read_msr(priv);
> +}
> +
>  static int f81232_port_probe(struct usb_serial_port *port)
>  {
>  	struct f81232_private *priv;
> @@ -279,10 +377,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&priv->lock);
> +	INIT_WORK(&priv->int_worker, f81232_int_work_wq);
>  
>  	usb_set_serial_port_data(port, priv);
>  
>  	port->port.drain_delay = 256;
> +	priv->port = port;
>  
>  	return 0;
>  }

This already looks much better, thanks for resending.

Johan

[PATCH v4 4/7] usb: serial: reimplement RX bulk-in ep for F81232

[Peter Hung]

The F81232 bulk-in is RX data 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 | 47 +++++++++++++++------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 274120d..12e1ae4 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -194,47 +194,30 @@ 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;
+	u8 line_status = 0;
 	int i;
 
-	/* update line status */
-	spin_lock_irqsave(&priv->lock, flags);
-	line_status = priv->line_status;
-	priv->line_status &= ~UART_STATE_TRANSIENT_MASK;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
 	if (!urb->actual_length)
 		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)]... */
+
+	if (urb->actual_length >= 2) {
+
+		for (i = 0 ; i < urb->actual_length ; i += 2) {
+			line_status |= data[i+0];
+			tty_insert_flip_string_fixed_flag(&port->port,
+				&data[i+1], tty_flag, 1);
+		}
+
+		if (unlikely(line_status & UART_LSR_OE))
+			tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
+
+		tty_flip_buffer_push(&port->port);
 	}
 
-	tty_flip_buffer_push(&port->port);
 }
 
 static int set_control_lines(struct usb_device *dev, u8 value)
-- 
1.9.1

Re: [PATCH v4 4/7] usb: serial: reimplement RX bulk-in ep for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:38PM +0800, Peter Hung wrote:
> The F81232 bulk-in is RX data 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 | 47 +++++++++++++++------------------------------
>  1 file changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 274120d..12e1ae4 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -194,47 +194,30 @@ 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;
> +	u8 line_status = 0;
>  	int i;
>  
> -	/* update line status */
> -	spin_lock_irqsave(&priv->lock, flags);
> -	line_status = priv->line_status;
> -	priv->line_status &= ~UART_STATE_TRANSIENT_MASK;
> -	spin_unlock_irqrestore(&priv->lock, flags);
> -
>  	if (!urb->actual_length)
>  		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)]... */
> +
> +	if (urb->actual_length >= 2) {

Just return unless length < 2 and reduce indentation below.

Should should probably make sure the returned length is even as well.

> +
> +		for (i = 0 ; i < urb->actual_length ; i += 2) {
> +			line_status |= data[i+0];
> +			tty_insert_flip_string_fixed_flag(&port->port,
> +				&data[i+1], tty_flag, 1);
> +		}
> +
> +		if (unlikely(line_status & UART_LSR_OE))
> +			tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);

This needs some more work as you need to determine tty_flag for each
character based on the line status using the priorities that you remove
above (i.e. break takes precedence over parity, etc).

Also insert a overrun char for every individual overrun error.

> +
> +		tty_flip_buffer_push(&port->port);
>  	}
>  
> -	tty_flip_buffer_push(&port->port);
>  }
>  
>  static int set_control_lines(struct usb_device *dev, u8 value)

Johan

[PATCH v4 5/7] usb: serial: implement set_termios for F81232

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 12e1ae4..248f40d 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -51,6 +51,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define F81232_USB_TIMEOUT 3000
 
 #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)
 struct f81232_private {
 	spinlock_t lock;
@@ -61,6 +65,20 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static inline int calc_baud_divisor(u32 baudrate)
+{
+	u32 divisor, rem;
+
+	divisor = 115200L / baudrate;
+	rem = 115200L % baudrate;
+
+	/* Round to nearest divisor */
+	if (((rem * 2) >= baudrate) && (baudrate != 110))
+		divisor++;
+
+	return divisor;
+}
+
 static inline int f81232_get_register(struct usb_device *dev,
 							  u16 reg, u8 *data)
 {
@@ -84,6 +102,29 @@ static inline int f81232_get_register(struct usb_device *dev,
 	return status;
 }
 
+static inline int f81232_set_register(struct usb_device *dev,
+							  u16 reg, u8 data)
+{
+	int status;
+
+	status = usb_control_msg(dev,
+			usb_sndctrlpipe(dev, 0),
+			REGISTER_REQUEST,
+			SET_REGISTER,
+			reg,
+			0,
+			&data,
+			1,
+			F81232_USB_TIMEOUT);
+
+	if (status < 0) {
+		dev_dbg(&dev->dev,
+			"%s status: %d\n", __func__, status);
+	}
+
+	return status;
+}
+
 static void f81232_read_msr(struct f81232_private *priv)
 {
 	int status;
@@ -240,15 +281,104 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	/* FIXME - Stubbed out for now */
+	u16 divisor;
+	u16 new_lcr = 0;
+	u8 data;
+	int status;
+	struct ktermios *termios = &tty->termios;
+	struct usb_device *dev = port->serial->dev;
+	unsigned int cflag = termios->c_cflag;
 
-	/* Don't change anything if nothing has changed */
-	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
-		return;
+	divisor = calc_baud_divisor(tty_get_baud_rate(tty));
+
+	status = f81232_set_register(dev, LINE_CONTROL_REGISTER,
+			 UART_LCR_DLAB); /* DLAB */
+
+	if (status < 0) {
+		dev_dbg(&dev->dev,
+			"%s status: %d line:%d\n", __func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(dev, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+
+	if (status < 0) {
+		dev_dbg(&dev->dev,
+			"%s status: %d line:%d\n", __func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(dev, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+
+	if (status < 0) {
+		dev_dbg(&dev->dev,
+			"%s status: %d line:%d\n", __func__, status, __LINE__);
+	}
+
+	status = f81232_set_register(dev, LINE_CONTROL_REGISTER, 0x00);
+
+	if (status < 0) {
+		dev_dbg(&dev->dev,
+			"%s status: %d line:%d\n", __func__, status, __LINE__);
+	}
+
+	if (cflag & PARENB) {
+		new_lcr |= UART_LCR_PARITY; /* default parity on/odd */
+
+		if (cflag & CMSPAR) {
+			if (cflag & PARODD) {
+				/* stick mark */
+				new_lcr |= UART_LCR_SPAR;
+			} else {
+				 /* stick space */
+				new_lcr |= (UART_LCR_EPAR | UART_LCR_SPAR);
+			}
+		} else {
+			if (!(cflag & PARODD)) {
+				/* even */
+				new_lcr |= UART_LCR_EPAR;
+			}
+		}
+	}
+
+	if (cflag & CSTOPB)
+		new_lcr |= UART_LCR_STOP;
+	else
+		new_lcr &= ~UART_LCR_STOP;
+
+	switch (cflag & CSIZE) {
+	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(dev, LINE_CONTROL_REGISTER, new_lcr);
+
+	/* 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(dev, FIFO_CONTROL_REGISTER, data);
+
+	data = UART_IER_MSI | UART_IER_RLSI | UART_IER_THRI | UART_IER_RDI;
+
+	status |= f81232_set_register(dev,
+			  INTERRUPT_ENABLE_REGISTER, 0xf); /* IER */
+
+	if (status < 0)
+		dev_err(&port->dev, "LINE_CONTROL_REGISTER set error: %d\n",
+			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)
-- 
1.9.1

Re: [PATCH v4 5/7] usb: serial: implement set_termios for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:39PM +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.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 144 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 137 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 12e1ae4..248f40d 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -51,6 +51,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define F81232_USB_TIMEOUT 3000
>  
>  #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)
>  struct f81232_private {
>  	spinlock_t lock;
> @@ -61,6 +65,20 @@ struct f81232_private {
>  	struct usb_serial_port *port;
>  };
>  
> +static inline int calc_baud_divisor(u32 baudrate)

No need for inline.

> +{
> +	u32 divisor, rem;
> +
> +	divisor = 115200L / baudrate;
> +	rem = 115200L % baudrate;

Use a define for the base baud rate. Is 115200 really the maximum baud
rate?
	
> +
> +	/* Round to nearest divisor */
> +	if (((rem * 2) >= baudrate) && (baudrate != 110))
> +		divisor++;

Can't you use DIV_ROUND_CLOSEST here as serial core does?

> +
> +	return divisor;
> +}
> +
>  static inline int f81232_get_register(struct usb_device *dev,
>  							  u16 reg, u8 *data)

No inline.

>  {
> @@ -84,6 +102,29 @@ static inline int f81232_get_register(struct usb_device *dev,
>  	return status;
>  }
>  
> +static inline int f81232_set_register(struct usb_device *dev,
> +							  u16 reg, u8 data)

Pass the usb-serial port instead of usb device here as well.

> +{
> +	int status;
> +
> +	status = usb_control_msg(dev,
> +			usb_sndctrlpipe(dev, 0),
> +			REGISTER_REQUEST,
> +			SET_REGISTER,
> +			reg,
> +			0,
> +			&data,
> +			1,

sizeof(data) for clarity?

> +			F81232_USB_TIMEOUT);
> +
> +	if (status < 0) {
> +		dev_dbg(&dev->dev,
> +			"%s status: %d\n", __func__, status);

dev_err, no line break

> +	}
> +
> +	return status;
> +}
> +
>  static void f81232_read_msr(struct f81232_private *priv)
>  {
>  	int status;
> @@ -240,15 +281,104 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>  static void f81232_set_termios(struct tty_struct *tty,
>  		struct usb_serial_port *port, struct ktermios *old_termios)
>  {
> -	/* FIXME - Stubbed out for now */
> +	u16 divisor;
> +	u16 new_lcr = 0;

Why u16 for an 8-bit register?

> +	u8 data;
> +	int status;
> +	struct ktermios *termios = &tty->termios;
> +	struct usb_device *dev = port->serial->dev;
> +	unsigned int cflag = termios->c_cflag;

Use the cflag macros directly below (e.g. C_PARENB(tty)) and you won't
need this (or termios above).

>  
> -	/* Don't change anything if nothing has changed */
> -	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> -		return;
> +	divisor = calc_baud_divisor(tty_get_baud_rate(tty));

You get a division by zero here of the baud rate is 0 (B0 is used to
drop DTR/RTS).

> +
> +	status = f81232_set_register(dev, LINE_CONTROL_REGISTER,
> +			 UART_LCR_DLAB); /* DLAB */
> +

No newline before testing return values (again, applies to whole
series).

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

Use dev_err for errors throughout, but remember that you already log
errors in the accessor function.

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

Perhaps factor the above out in a f81232_set_baudrate helper?

> +
> +	if (cflag & PARENB) {
> +		new_lcr |= UART_LCR_PARITY; /* default parity on/odd */
> +
> +		if (cflag & CMSPAR) {
> +			if (cflag & PARODD) {
> +				/* stick mark */
> +				new_lcr |= UART_LCR_SPAR;
> +			} else {
> +				 /* stick space */
> +				new_lcr |= (UART_LCR_EPAR | UART_LCR_SPAR);
> +			}
> +		} else {
> +			if (!(cflag & PARODD)) {
> +				/* even */
> +				new_lcr |= UART_LCR_EPAR;
> +			}
> +		}
> +	}

Looks like you could simplify the above to something like

	if (C_PARENB(tty) {
		lcr |= UART_LCR_PARITY;

		if (!C_PARODD(tty))
			lcr |= UART_LCR_EPAR

		if (C_CMSPAR(tty))
			lcr |= UART_LCR_SPAR;
	}

> +
> +	if (cflag & CSTOPB)
> +		new_lcr |= UART_LCR_STOP;
> +	else
> +		new_lcr &= ~UART_LCR_STOP;

No need to clear bits since you initialise to 0.

> +
> +	switch (cflag & CSIZE) {
> +	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(dev, LINE_CONTROL_REGISTER, new_lcr);
> +
> +	/* 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(dev, FIFO_CONTROL_REGISTER, data);
> +
> +	data = UART_IER_MSI | UART_IER_RLSI | UART_IER_THRI | UART_IER_RDI;
> +
> +	status |= f81232_set_register(dev,
> +			  INTERRUPT_ENABLE_REGISTER, 0xf); /* IER */
> +
> +	if (status < 0)
> +		dev_err(&port->dev, "LINE_CONTROL_REGISTER set error: %d\n",
> +			status);

Some of the above should only be needed when initialising (or opening)
the port and not on every set_termios, right?

>  
> -	/* Do the real work here... */
> -	if (old_termios)
> -		tty_termios_copy_hw(&tty->termios, old_termios);
>  }
>  
>  static int f81232_tiocmget(struct tty_struct *tty)

Johan

[PATCH v4 6/7] usb: serial: implement MCR/MSR function for F81232

[Peter Hung]

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

The update_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 | 105 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 83 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 248f40d..0ed7e36 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -38,11 +38,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #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
 
 #define REGISTER_REQUEST 0xA0
@@ -55,6 +51,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)
 struct f81232_private {
 	spinlock_t lock;
@@ -165,6 +162,62 @@ static void f81232_read_msr(struct f81232_private *priv)
 
 	dev_dbg(&dev->dev, "%s: %x\n", __func__, priv->line_status);
 }
+
+static inline int update_mctrl(struct f81232_private *port_priv,
+					   unsigned int set, unsigned int clear)
+{
+	struct usb_device *dev = port_priv->port->serial->dev;
+	u8 urb_value;
+	int status;
+	unsigned long flags;
+
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0) {
+		dev_dbg(&dev->dev, "%s fail - DTR|RTS %d\n",
+			__func__, __LINE__);
+		return 0;	/* no change */
+	}
+
+	clear &= ~set;	/* 'set' takes precedence over 'clear' */
+	urb_value = 8 | port_priv->line_control;
+
+	if (clear & TIOCM_DTR) {
+		urb_value &= ~UART_MCR_DTR;
+		dev_dbg(&dev->dev, "clear DTR\n");
+	}
+
+	if (clear & TIOCM_RTS) {
+		urb_value &= ~UART_MCR_RTS;
+		dev_dbg(&dev->dev, "clear RTS\n");
+	}
+
+	if (set & TIOCM_DTR) {
+		urb_value |= UART_MCR_DTR;
+		dev_dbg(&dev->dev, "set DTR\n");
+	}
+
+	if (set & TIOCM_RTS) {
+		urb_value |= UART_MCR_RTS;
+		dev_dbg(&dev->dev, "set RTS\n");
+	}
+
+	dev_dbg(&dev->dev, "%s n:%x o:%x\n", __func__, urb_value,
+			port_priv->line_control);
+
+	status = f81232_set_register(dev, MODEM_CONTROL_REGISTER, urb_value);
+
+	if (status < 0) {
+		dev_dbg(&dev->dev, "%s read MSR status < 0\n", __func__);
+	} else {
+		spin_lock_irqsave(&port_priv->lock, flags);
+		port_priv->line_control = urb_value;
+		spin_unlock_irqrestore(&port_priv->lock, flags);
+	}
+
+	f81232_read_msr(port_priv);
+
+	return status;
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
@@ -261,12 +314,6 @@ static void f81232_process_read_urb(struct urb *urb)
 
 }
 
-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 */
@@ -383,14 +430,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);
+	unsigned long flags;
+	u8 mcr, msr;
+
+	spin_lock_irqsave(&port_priv->lock, flags);
+	mcr = port_priv->line_control;
+	msr = port_priv->line_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 */
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *port_priv =
+		usb_get_serial_port_data(port);
+
+	update_mctrl(port_priv, set, clear);
 	return 0;
 }
 
@@ -427,18 +495,11 @@ 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);
+		update_mctrl(priv, 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);
+		update_mctrl(priv, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static int f81232_carrier_raised(struct usb_serial_port *port)
-- 
1.9.1

Re: [PATCH v4 6/7] usb: serial: implement MCR/MSR function for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:40PM +0800, Peter Hung wrote:
> This patch implement relative MCR/MSR function, such like
> tiocmget()/tiocmset()/dtr_rts().
> 
> The update_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 | 105 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 83 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 248f40d..0ed7e36 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -38,11 +38,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #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
>  
>  #define REGISTER_REQUEST 0xA0
> @@ -55,6 +51,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)
>  struct f81232_private {
>  	spinlock_t lock;
> @@ -165,6 +162,62 @@ static void f81232_read_msr(struct f81232_private *priv)
>  
>  	dev_dbg(&dev->dev, "%s: %x\n", __func__, priv->line_status);
>  }
> +
> +static inline int update_mctrl(struct f81232_private *port_priv,
> +					   unsigned int set, unsigned int clear)

Rename this on f81232_set_mctrl or similar, and pass the usb-serial port
instead of port-data.

Drop the inline (from all you functions).

> +{
> +	struct usb_device *dev = port_priv->port->serial->dev;
> +	u8 urb_value;
> +	int status;
> +	unsigned long flags;
> +
> +	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0) {
> +		dev_dbg(&dev->dev, "%s fail - DTR|RTS %d\n",
> +			__func__, __LINE__);

Just skip the dev_dbg.

> +		return 0;	/* no change */
> +	}
> +
> +	clear &= ~set;	/* 'set' takes precedence over 'clear' */
> +	urb_value = 8 | port_priv->line_control;

Use the UART_MCR_OUT2 define if needed (and comment on why).

> +
> +	if (clear & TIOCM_DTR) {
> +		urb_value &= ~UART_MCR_DTR;
> +		dev_dbg(&dev->dev, "clear DTR\n");
> +	}
> +
> +	if (clear & TIOCM_RTS) {
> +		urb_value &= ~UART_MCR_RTS;
> +		dev_dbg(&dev->dev, "clear RTS\n");
> +	}
> +
> +	if (set & TIOCM_DTR) {
> +		urb_value |= UART_MCR_DTR;
> +		dev_dbg(&dev->dev, "set DTR\n");
> +	}
> +
> +	if (set & TIOCM_RTS) {
> +		urb_value |= UART_MCR_RTS;
> +		dev_dbg(&dev->dev, "set RTS\n");
> +	}

You don't need all those dev_dbg, the one below should suffice.

> +
> +	dev_dbg(&dev->dev, "%s n:%x o:%x\n", __func__, urb_value,
> +			port_priv->line_control);

Use %02x, and it doesn't hurt spelling out "new" and "old".

> +
> +	status = f81232_set_register(dev, MODEM_CONTROL_REGISTER, urb_value);
> +
> +	if (status < 0) {
> +		dev_dbg(&dev->dev, "%s read MSR status < 0\n", __func__);

dev_err, and the error message is incorrect (you're updating MCR).

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

Won't you get an interrupt if MSR changes?

> +
> +	return status;
> +}
> +
>  static void f81232_update_line_status(struct usb_serial_port *port,
>  				      unsigned char *data,
>  				      unsigned int actual_length)
> @@ -261,12 +314,6 @@ static void f81232_process_read_urb(struct urb *urb)
>  
>  }
>  
> -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 */
> @@ -383,14 +430,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);
> +	unsigned long flags;
> +	u8 mcr, msr;
> +
> +	spin_lock_irqsave(&port_priv->lock, flags);
> +	mcr = port_priv->line_control;
> +	msr = port_priv->line_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 */
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct f81232_private *port_priv =
> +		usb_get_serial_port_data(port);
> +
> +	update_mctrl(port_priv, set, clear);

You should return any errors here (after passing it to
usb_translate_error()).

>  	return 0;
>  }
>  
> @@ -427,18 +495,11 @@ 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);
> +		update_mctrl(priv, 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);
> +		update_mctrl(priv, 0, TIOCM_DTR | TIOCM_RTS);
>  }
>  
>  static int f81232_carrier_raised(struct usb_serial_port *port)

Johan

[PATCH v4 7/7] usb: serial: modify ioctl TIOCGSERIAL for F81232

[Peter Hung]

Set correct product type from 16654 to 16550A and
fix the ioctl TIOCGSERIAL return struct values.

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 0ed7e36..4d3aba8 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -518,13 +518,18 @@ static int f81232_ioctl(struct tty_struct *tty,
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		memset(&ser, 0, sizeof ser);
-		ser.type = PORT_16654;
+		memset(&ser, 0, sizeof(ser));
+		ser.flags		= ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
+		ser.xmit_fifo_size	= port->bulk_out_size;
+		ser.close_delay		= 5*HZ;
+		ser.closing_wait	= 30*HZ;
+
+		ser.type = PORT_16550A;
 		ser.line = port->minor;
 		ser.port = port->port_number;
-		ser.baud_base = 460800;
+		ser.baud_base = 115200;
 
-		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
+		if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
 			return -EFAULT;
 
 		return 0;
-- 
1.9.1

Re: [PATCH v4 7/7] usb: serial: modify ioctl TIOCGSERIAL for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:41PM +0800, Peter Hung wrote:
> Set correct product type from 16654 to 16550A and
> fix the ioctl TIOCGSERIAL return struct values.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 0ed7e36..4d3aba8 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -518,13 +518,18 @@ static int f81232_ioctl(struct tty_struct *tty,
>  
>  	switch (cmd) {
>  	case TIOCGSERIAL:

First of all, please break this out into a f81232_get_serial_info helper
function.

> -		memset(&ser, 0, sizeof ser);
> -		ser.type = PORT_16654;
> +		memset(&ser, 0, sizeof(ser));
> +		ser.flags		= ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
> +		ser.xmit_fifo_size	= port->bulk_out_size;
> +		ser.close_delay		= 5*HZ;
> +		ser.closing_wait	= 30*HZ;

Spaces around *

> +

No newline.

> +		ser.type = PORT_16550A;
>  		ser.line = port->minor;
>  		ser.port = port->port_number;
> -		ser.baud_base = 460800;
> +		ser.baud_base = 115200;
>  
> -		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
> +		if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
>  			return -EFAULT;
>  
>  		return 0;

Johan

Re: [PATCH v4 1/7] usb: serial: modify bulk-in/out size for F81232

[Johan Hovold]

On Fri, Jan 30, 2015 at 02:13:35PM +0800, Peter Hung wrote:
> The F81232 real bulk-in/out ep buffer size is 64Bytes
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index c5dc233..4f42e9d 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -304,8 +304,8 @@ static struct usb_serial_driver f81232_device = {
>  	},
>  	.id_table =		id_table,
>  	.num_ports =		1,
> -	.bulk_in_size =		256,
> -	.bulk_out_size =	256,
> +	.bulk_in_size =		64,
> +	.bulk_out_size =	64,

These buffer sizes can be larger than the endpoint sizes for increased
throughput (the host controller driver will break them up).

If you still want them to match the endpoint sizes you should just leave
them unset (0) and usb-serial core will set them for you.

Johan