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

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

[Peter Hung]

This series patch V8 is changed from V7 as following:

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

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

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

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

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

-- 
1.9.1

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

[Peter Hung]

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

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

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

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

[Peter Hung]

The F81232 bulk-in is RX data + LSR channel, data format is
[LSR+Data][LSR+Data]..... , We had implemented in f81232_process_read_urb().

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

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

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

[Johan Hovold]

On Thu, Feb 26, 2015 at 06:02:08PM +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>

>  static void f81232_process_read_urb(struct urb *urb)
>  {
>  	struct usb_serial_port *port = urb->context;
> -	struct f81232_private *priv = usb_get_serial_port_data(port);
>  	unsigned char *data = urb->transfer_buffer;
> -	char tty_flag = TTY_NORMAL;
> -	unsigned long flags;
> -	u8 line_status;
> -	int i;
> -
> -	/* update line status */
> -	spin_lock_irqsave(&priv->lock, flags);
> -	line_status = priv->modem_status;
> -	priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
> -	spin_unlock_irqrestore(&priv->lock, flags);
> +	char tty_flag;
> +	unsigned int i;
> +	u8 lsr;
>  
> -	if (!urb->actual_length)
> +	if ((urb->actual_length < 2) || (urb->actual_length % 2))
>  		return;

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

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

Johan

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

[Peter Hung]

Hello,

Johan Hovold 於 2015/3/14 下午 07:48 寫道:
> On Thu, Feb 26, 2015 at 06:02:08PM +0800, Peter Hung wrote:
>> -	if (!urb->actual_length)
>> +	if ((urb->actual_length < 2) || (urb->actual_length % 2))
>>   		return;
>
> Not parsing short data (e.g. not divisible by 2) is OK I guess. You
> could also just discard the last odd byte, but that's up to you.
>
> Either way, I think you should add a dev_warn here rather than just
> silently drop it.

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

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

Thanks
-- 
With Best Regards,
Peter Hung

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

[Johan Hovold]

On Mon, Mar 16, 2015 at 10:53:38AM +0800, Peter Hung wrote:
> Hello,
> 
> Johan Hovold 於 2015/3/14 下午 07:48 寫道:
> > On Thu, Feb 26, 2015 at 06:02:08PM +0800, Peter Hung wrote:
> >> -	if (!urb->actual_length)
> >> +	if ((urb->actual_length < 2) || (urb->actual_length % 2))
> >>   		return;
> >
> > Not parsing short data (e.g. not divisible by 2) is OK I guess. You
> > could also just discard the last odd byte, but that's up to you.
> >
> > Either way, I think you should add a dev_warn here rather than just
> > silently drop it.
> 
> With F81232, when it first submit with interrupt URB, it will response 
> once with 1 bytes data. The data is hw current LSR, but it useless on 
> open. It's should necessary with receiving data. When the device is 
> working good, it's should acked with even size data.

Ok, thanks for clarifying that.

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

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

Thanks,
Johan

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

[Peter Hung]

Hello,

Johan Hovold 於 2015/3/16 下午 04:52 寫道:
> On Mon, Mar 16, 2015 at 10:53:38AM +0800, Peter Hung wrote:
>> To avoid confusing to user, I'll keep it without warning message.
>
> Yes, skip the warning, but could you a short comment about this (e.g.
> the 1-byte packet on open) before you do the size check?

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

-- 
With Best Regards,
Peter Hung

[PATCH V8 03/10] 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 25b1a47..cf5b902 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>
@@ -46,7 +46,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_CTS			0x80
 
 struct f81232_private {
-	spinlock_t lock;
+	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
 };
@@ -227,17 +227,16 @@ static void f81232_close(struct usb_serial_port *port)
 static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 {
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	unsigned long flags;
 	u8 control;
 
-	spin_lock_irqsave(&priv->lock, flags);
+	mutex_lock(&priv->lock);
 	/* Change DTR and RTS */
 	if (on)
 		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
 	else
 		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
 	control = priv->line_control;
-	spin_unlock_irqrestore(&priv->lock, flags);
+	mutex_unlock(&priv->lock);
 	set_control_lines(port->serial->dev, control);
 }
 
@@ -281,7 +280,7 @@ static int f81232_port_probe(struct usb_serial_port *port)
 	if (!priv)
 		return -ENOMEM;
 
-	spin_lock_init(&priv->lock);
+	mutex_init(&priv->lock);
 
 	usb_set_serial_port_data(port, priv);
 
-- 
1.9.1

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

[Peter Hung]

The interrupt endpoint will report current IIR. If we got IIR with MSR changed
, We will do read MSR with interrupt_work worker to do f81232_read_msr()
function.

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index cf5b902..46a81ef 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -31,6 +31,13 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST	0xa0
+#define F81232_GET_REGISTER	0xc0
+
+#define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR			0x01
 #define CONTROL_RTS			0x02
 
@@ -49,19 +56,112 @@ struct f81232_private {
 	struct mutex lock;
 	u8 line_control;
 	u8 modem_status;
+	struct work_struct interrupt_work;
+	struct usb_serial_port *port;
 };
 
+static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(*val), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	status = usb_control_msg(dev,
+				usb_rcvctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_GET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_GET_TIMEOUT);
+	if (status != sizeof(*val)) {
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else {
+		status = 0;
+		*val = *tmp;
+	}
+
+	kfree(tmp);
+	return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+	int status;
+	u8 current_msr;
+	struct tty_struct *tty;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	mutex_lock(&priv->lock);
+	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+			&current_msr);
+	if (status) {
+		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+		mutex_unlock(&priv->lock);
+		return;
+	}
+
+	if (!(current_msr & UART_MSR_ANY_DELTA)) {
+		mutex_unlock(&priv->lock);
+		return;
+	}
+
+	priv->modem_status = current_msr;
+
+	if (current_msr & UART_MSR_DCTS)
+		port->icount.cts++;
+	if (current_msr & UART_MSR_DDSR)
+		port->icount.dsr++;
+	if (current_msr & UART_MSR_TERI)
+		port->icount.rng++;
+	if (current_msr & UART_MSR_DDCD) {
+		port->icount.dcd++;
+		tty = tty_port_tty_get(&port->port);
+		if (tty) {
+			usb_serial_handle_dcd_change(port, tty,
+					current_msr & UART_MSR_DCD);
+
+			tty_kref_put(tty);
+		}
+	}
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+	mutex_unlock(&priv->lock);
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
-				      unsigned int actual_length)
+				      size_t actual_length)
 {
-	/*
-	 * FIXME: Update port->icount, and call
-	 *
-	 *		wake_up_interruptible(&port->port.delta_msr_wait);
-	 *
-	 *	  on MSR changes.
-	 */
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (!actual_length)
+		return;
+
+	switch (data[0] & 0x07) {
+	case 0x00: /* msr change */
+		dev_dbg(&port->dev, "IIR: MSR Change: %02x\n", data[0]);
+		schedule_work(&priv->interrupt_work);
+		break;
+	case 0x02: /* tx-empty */
+		break;
+	case 0x04: /* rx data available */
+		break;
+	case 0x06: /* lsr change */
+		/* we can forget it. the LSR will read from bulk-in */
+		dev_dbg(&port->dev, "IIR: LSR Change: %02x\n", data[0]);
+		break;
+	}
 }
 
 static void f81232_read_int_callback(struct urb *urb)
@@ -272,6 +372,14 @@ static int f81232_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static void  f81232_interrupt_work(struct work_struct *work)
+{
+	struct f81232_private *priv =
+		container_of(work, struct f81232_private, interrupt_work);
+
+	f81232_read_msr(priv->port);
+}
+
 static int f81232_port_probe(struct usb_serial_port *port)
 {
 	struct f81232_private *priv;
@@ -281,10 +389,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
 		return -ENOMEM;
 
 	mutex_init(&priv->lock);
+	INIT_WORK(&priv->interrupt_work,  f81232_interrupt_work);
 
 	usb_set_serial_port_data(port, priv);
 
 	port->port.drain_delay = 256;
+	priv->port = port;
 
 	return 0;
 }
-- 
1.9.1

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

[Johan Hovold]

On Thu, Feb 26, 2015 at 06:02:10PM +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.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 126 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 118 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index cf5b902..46a81ef 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -31,6 +31,13 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST	0xa0
> +#define F81232_GET_REGISTER	0xc0
> +
> +#define SERIAL_BASE_ADDRESS	0x0120
> +#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)

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

> +
>  #define CONTROL_DTR			0x01
>  #define CONTROL_RTS			0x02
>  
> @@ -49,19 +56,112 @@ struct f81232_private {
>  	struct mutex lock;
>  	u8 line_control;
>  	u8 modem_status;
> +	struct work_struct interrupt_work;
> +	struct usb_serial_port *port;
>  };
>  
> +static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
> +{
> +	int status;
> +	u8 *tmp;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	tmp = kmalloc(sizeof(*val), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	status = usb_control_msg(dev,
> +				usb_rcvctrlpipe(dev, 0),
> +				F81232_REGISTER_REQUEST,
> +				F81232_GET_REGISTER,
> +				reg,
> +				0,
> +				tmp,
> +				sizeof(u8),

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

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

Could you rewrite this as

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

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

Same comments apply to set_register in a follow up patch.

Looks good otherwise,
Johan

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

[Peter Hung]

Hello,

Johan Hovold 於 2015/3/14 下午 08:02 寫道:
> On Thu, Feb 26, 2015 at 06:02:10PM +0800, Peter Hung wrote:
>> +	if (status != sizeof(*val)) {
>> +		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
>> +
>> +		if (status == 0)
>> +			status = -EIO;
>> +		else
>> +			status = usb_translate_errors(status);
>
> Could you rewrite this as
>
> 	if (status < 0)
> 		status = usb_translate_errors(status);
> 	else
> 		status = 0;

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

I'll keep this sections.

Thanks for review
-- 
With Best Regards,
Peter Hung

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

[Johan Hovold]

On Mon, Mar 16, 2015 at 11:08:29AM +0800, Peter Hung wrote:
> Hello,
> 
> Johan Hovold 於 2015/3/14 下午 08:02 寫道:
> > On Thu, Feb 26, 2015 at 06:02:10PM +0800, Peter Hung wrote:
> >> +	if (status != sizeof(*val)) {
> >> +		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> >> +
> >> +		if (status == 0)
> >> +			status = -EIO;
> >> +		else
> >> +			status = usb_translate_errors(status);
> >
> > Could you rewrite this as
> >
> > 	if (status < 0)
> > 		status = usb_translate_errors(status);
> > 	else
> > 		status = 0;
> 
> In my definition the return value of set/getregister(), 0 is success, 
> negative values are errors. The function usb_control_msg() return value 
> is success transmited/received byte. It's maybe return 0. I want to 
> treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It 
> will return 0 back. So I need especially handle with status == 0.

I meant to write

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

which I think is more readable.

Sorry for the confusion.

Johan

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

[Peter Hung]

Hello,

Johan Hovold 於 2015/3/16 下午 04:55 寫道:
> On Mon, Mar 16, 2015 at 11:08:29AM +0800, Peter Hung wrote:
>>> Could you rewrite this as
>>>
>>> 	if (status < 0)
>>> 		status = usb_translate_errors(status);
>>> 	else
>>> 		status = 0;
>>
>> In my definition the return value of set/getregister(), 0 is success,
>> negative values are errors. The function usb_control_msg() return value
>> is success transmited/received byte. It's maybe return 0. I want to
>> treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It
>> will return 0 back. So I need especially handle with status == 0.
>
> I meant to write
>
> 	if (status < 0)
>   		status = usb_translate_errors(status);
> 	else
> 		status = -EIO;
>
> which I think is more readable.

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

-- 
With Best Regards,
Peter Hung

[PATCH V8 05/10] 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 | 139 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 117 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 46a81ef..b70f9b9 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -34,8 +34,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
 /* USB Control EP parameter */
 #define F81232_REGISTER_REQUEST	0xa0
 #define F81232_GET_REGISTER	0xc0
+#define F81232_SET_REGISTER	0x40
 
 #define SERIAL_BASE_ADDRESS	0x0120
+#define MODEM_CONTROL_REGISTER	(0x04 + SERIAL_BASE_ADDRESS)
 #define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
 
 #define CONTROL_DTR			0x01
@@ -54,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct f81232_private {
 	struct mutex lock;
-	u8 line_control;
+	u8 modem_control;
 	u8 modem_status;
 	struct work_struct interrupt_work;
 	struct usb_serial_port *port;
@@ -95,6 +97,42 @@ static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
 	return status;
 }
 
+static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 val)
+{
+	int status;
+	u8 *tmp;
+	struct usb_device *dev = port->serial->dev;
+
+	tmp = kmalloc(sizeof(val), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	*tmp = val;
+
+	status = usb_control_msg(dev,
+				usb_sndctrlpipe(dev, 0),
+				F81232_REGISTER_REQUEST,
+				F81232_SET_REGISTER,
+				reg,
+				0,
+				tmp,
+				sizeof(u8),
+				USB_CTRL_SET_TIMEOUT);
+	if (status != sizeof(val)) {
+		dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+		if (status == 0)
+			status = -EIO;
+		else
+			status = usb_translate_errors(status);
+	} else {
+		status = 0;
+	}
+
+	kfree(tmp);
+	return status;
+}
+
 static void f81232_read_msr(struct usb_serial_port *port)
 {
 	int status;
@@ -139,6 +177,51 @@ static void f81232_read_msr(struct usb_serial_port *port)
 	mutex_unlock(&priv->lock);
 }
 
+static int f81232_set_mctrl(struct usb_serial_port *port,
+					   unsigned int set, unsigned int clear)
+{
+	u8 urb_value;
+	int status;
+	struct f81232_private *priv = usb_get_serial_port_data(port);
+
+	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+		return 0;	/* no change */
+
+	/* 'set' takes precedence over 'clear' */
+	clear &= ~set;
+
+	/* force enable interrupt with OUT2 */
+	mutex_lock(&priv->lock);
+	urb_value = UART_MCR_OUT2 | priv->modem_control;
+
+	if (clear & TIOCM_DTR)
+		urb_value &= ~UART_MCR_DTR;
+
+	if (clear & TIOCM_RTS)
+		urb_value &= ~UART_MCR_RTS;
+
+	if (set & TIOCM_DTR)
+		urb_value |= UART_MCR_DTR;
+
+	if (set & TIOCM_RTS)
+		urb_value |= UART_MCR_RTS;
+
+	dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+			urb_value, priv->modem_control);
+
+	status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+	if (status) {
+		dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+		mutex_unlock(&priv->lock);
+		return status;
+	}
+
+	priv->modem_control = urb_value;
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
 static void f81232_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      size_t actual_length)
@@ -250,12 +333,6 @@ static void f81232_process_read_urb(struct urb *urb)
 	tty_flip_buffer_push(&port->port);
 }
 
-static int set_control_lines(struct usb_device *dev, u8 value)
-{
-	/* FIXME - Stubbed out for now */
-	return 0;
-}
-
 static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
 	/* FIXME - Stubbed out for now */
@@ -283,15 +360,35 @@ static void f81232_set_termios(struct tty_struct *tty,
 
 static int f81232_tiocmget(struct tty_struct *tty)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	int r;
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81232_private *port_priv = usb_get_serial_port_data(port);
+	u8 mcr, msr;
+
+	/* force get current MSR changed state */
+	f81232_read_msr(port);
+
+	mutex_lock(&port_priv->lock);
+	mcr = port_priv->modem_control;
+	msr = port_priv->modem_status;
+	mutex_unlock(&port_priv->lock);
+
+	r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+		(mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+		(msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+		(msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+		(msr & UART_MSR_RI ? TIOCM_RI : 0) |
+		(msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+	return r;
 }
 
 static int f81232_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear)
 {
-	/* FIXME - Stubbed out for now */
-	return 0;
+	struct usb_serial_port *port = tty->driver_data;
+
+	return f81232_set_mctrl(port, set, clear);
 }
 
 static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
@@ -326,24 +423,22 @@ static void f81232_close(struct usb_serial_port *port)
 
 static void f81232_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct f81232_private *priv = usb_get_serial_port_data(port);
-	u8 control;
-
-	mutex_lock(&priv->lock);
-	/* Change DTR and RTS */
 	if (on)
-		priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
 	else
-		priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
-	control = priv->line_control;
-	mutex_unlock(&priv->lock);
-	set_control_lines(port->serial->dev, control);
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static int f81232_carrier_raised(struct usb_serial_port *port)
 {
+	u8 msr;
 	struct f81232_private *priv = usb_get_serial_port_data(port);
-	if (priv->modem_status & UART_DCD)
+
+	mutex_lock(&priv->lock);
+	msr = priv->modem_status;
+	mutex_unlock(&priv->lock);
+
+	if (msr & UART_MSR_DCD)
 		return 1;
 	return 0;
 }
-- 
1.9.1

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

[Johan Hovold]

On Thu, Feb 26, 2015 at 06:02:11PM +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>
> ---
>  drivers/usb/serial/f81232.c | 139 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 117 insertions(+), 22 deletions(-)

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

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

Also looks good otherwise.

Johan

[PATCH V8 06/10] USB: f81232: implement port enable/disable method

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index b70f9b9..a3f9530 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)
 
@@ -344,6 +346,48 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static int f81232_port_enable(struct usb_serial_port *port)
+{
+	u8 val;
+	int status;
+
+	/* fifo on, trigger8, clear TX/RX*/
+	val = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
+			UART_FCR_CLEAR_XMIT;
+
+	status = f81232_set_register(port, FIFO_CONTROL_REGISTER, val);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set FCR: %d\n",
+			__func__, status);
+		return status;
+	}
+
+	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			UART_IER_MSI);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set IER: %d\n",
+			__func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
+static int f81232_port_disable(struct usb_serial_port *port)
+{
+	int status;
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, 0);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set IER: %d\n",
+			__func__, status);
+		return status;
+	}
+
+	return 0;
+}
+
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
@@ -395,6 +439,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
+	result = f81232_port_enable(port);
+	if (result)
+		return result;
+
 	/* Setup termios */
 	if (tty)
 		f81232_set_termios(tty, port, NULL);
@@ -417,6 +465,7 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void f81232_close(struct usb_serial_port *port)
 {
+	f81232_port_disable(port);
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 }
-- 
1.9.1

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

[Peter Hung]

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

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

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

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index a3f9530..0580195 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,11 @@ struct f81232_private {
 	struct usb_serial_port *port;
 };
 
+static int calc_baud_divisor(speed_t baudrate)
+{
+	return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
 static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *val)
 {
 	int status;
@@ -346,6 +356,53 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 	 */
 }
 
+static void f81232_set_baudrate(struct usb_serial_port *port, speed_t baudrate)
+{
+	u8 lcr;
+	int divisor, status = 0;
+
+	divisor = calc_baud_divisor(baudrate);
+
+	status = f81232_get_register(port, LINE_CONTROL_REGISTER,
+			 &lcr); /* get LCR */
+	if (status) {
+		dev_err(&port->dev, "%s failed to get LCR: %d\n",
+			__func__, status);
+		return;
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			 lcr | UART_LCR_DLAB); /* Enable DLAB */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+		return;
+	}
+
+	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+			 divisor & 0x00ff); /* low */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
+			__func__, status);
+		goto reapply_lcr;
+	}
+
+	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+			 (divisor & 0xff00) >> 8); /* high */
+	if (status) {
+		dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",
+			__func__, status);
+	}
+
+reapply_lcr:
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+			lcr & ~UART_LCR_DLAB);
+	if (status) {
+		dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+			__func__, status);
+	}
+}
+
 static int f81232_port_enable(struct usb_serial_port *port)
 {
 	u8 val;
@@ -391,15 +448,62 @@ static int f81232_port_disable(struct usb_serial_port *port)
 static void f81232_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	/* FIXME - Stubbed out for now */
+	u8 new_lcr = 0;
+	int status = 0;
+	speed_t baudrate;
 
 	/* Don't change anything if nothing has changed */
 	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
 		return;
 
-	/* Do the real work here... */
-	if (old_termios)
-		tty_termios_copy_hw(&tty->termios, old_termios);
+	if (C_BAUD(tty) == B0)
+		f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
+	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+		f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
+
+	baudrate = tty_get_baud_rate(tty);
+	if (baudrate > 0) {
+		if (baudrate > F81232_MAX_BAUDRATE)
+			baudrate = F81232_MAX_BAUDRATE;
+
+		tty_encode_baud_rate(tty, baudrate, baudrate);
+		f81232_set_baudrate(port, baudrate);
+	}
+
+	if (C_PARENB(tty)) {
+		new_lcr |= UART_LCR_PARITY;
+
+		if (!C_PARODD(tty))
+			new_lcr |= UART_LCR_EPAR;
+
+		if (C_CMSPAR(tty))
+			new_lcr |= UART_LCR_SPAR;
+	}
+
+	if (C_CSTOPB(tty))
+		new_lcr |= UART_LCR_STOP;
+
+	switch (C_CSIZE(tty)) {
+	case CS5:
+		new_lcr |= UART_LCR_WLEN5;
+		break;
+	case CS6:
+		new_lcr |= UART_LCR_WLEN6;
+		break;
+	case CS7:
+		new_lcr |= UART_LCR_WLEN7;
+		break;
+	default:
+	case CS8:
+		new_lcr |= UART_LCR_WLEN8;
+		break;
+	}
+
+	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
+	if (status)
+		dev_err(&port->dev, "%s failed to set LCR: %d\n",
+			__func__, status);
+
 }
 
 static int f81232_tiocmget(struct tty_struct *tty)
-- 
1.9.1

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

[Johan Hovold]

On Thu, Feb 26, 2015 at 06:02:13PM +0800, Peter Hung wrote:

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

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

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

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

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

> +		f81232_set_baudrate(port, baudrate);
> +	}
> +
> +	if (C_PARENB(tty)) {
> +		new_lcr |= UART_LCR_PARITY;
> +
> +		if (!C_PARODD(tty))
> +			new_lcr |= UART_LCR_EPAR;
> +
> +		if (C_CMSPAR(tty))
> +			new_lcr |= UART_LCR_SPAR;
> +	}
> +
> +	if (C_CSTOPB(tty))
> +		new_lcr |= UART_LCR_STOP;
> +
> +	switch (C_CSIZE(tty)) {
> +	case CS5:
> +		new_lcr |= UART_LCR_WLEN5;
> +		break;
> +	case CS6:
> +		new_lcr |= UART_LCR_WLEN6;
> +		break;
> +	case CS7:
> +		new_lcr |= UART_LCR_WLEN7;
> +		break;
> +	default:
> +	case CS8:
> +		new_lcr |= UART_LCR_WLEN8;
> +		break;
> +	}
> +
> +	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
> +	if (status)
> +		dev_err(&port->dev, "%s failed to set LCR: %d\n",
> +			__func__, status);

Please add brackets around the if block.

> +
>  }
>  
>  static int f81232_tiocmget(struct tty_struct *tty)

Looks good otherwise.

Johan

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

[Peter Hung]

We extract TIOCGSERIAL section in f81232_ioctl() to f81232_get_serial_info()
to make it clarify.

Also we fix device type from 16654 to 16550A, and set it's baud_base
to 115200 (1.8432MHz/16).

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 0580195..21f2342 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -596,24 +596,32 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
 	return 0;
 }
 
+static int f81232_get_serial_info(struct usb_serial_port *port,
+		unsigned long arg)
+{
+	struct serial_struct ser;
+
+	memset(&ser, 0, sizeof(ser));
+
+	ser.type = PORT_16550A;
+	ser.line = port->minor;
+	ser.port = port->port_number;
+	ser.baud_base = 115200;
+
+	if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int f81232_ioctl(struct tty_struct *tty,
 			unsigned int cmd, unsigned long arg)
 {
-	struct serial_struct ser;
 	struct usb_serial_port *port = tty->driver_data;
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		memset(&ser, 0, sizeof ser);
-		ser.type = PORT_16654;
-		ser.line = port->minor;
-		ser.port = port->port_number;
-		ser.baud_base = 460800;
-
-		if (copy_to_user((void __user *)arg, &ser, sizeof ser))
-			return -EFAULT;
-
-		return 0;
+		return f81232_get_serial_info(port, arg);
 	default:
 		break;
 	}
-- 
1.9.1

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

[Johan Hovold]

On Thu, Feb 26, 2015 at 06:02:14PM +0800, Peter Hung wrote:
> 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 0580195..21f2342 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -596,24 +596,32 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
>  	return 0;
>  }
>  
> +static int f81232_get_serial_info(struct usb_serial_port *port,
> +		unsigned long arg)
> +{
> +	struct serial_struct ser;
> +
> +	memset(&ser, 0, sizeof(ser));
> +
> +	ser.type = PORT_16550A;
> +	ser.line = port->minor;
> +	ser.port = port->port_number;
> +	ser.baud_base = 115200;

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

Johan

[PATCH V8 09/10] 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 21f2342..0b7b69a 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 V8 10/10] USB: f81232: modify/add author

[Peter Hung]

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

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

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 0b7b69a..65d5cf6 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -684,5 +684,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
 module_usb_serial_driver(serial_drivers, id_table);
 
 MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
-MODULE_AUTHOR("Greg Kroah-Hartman <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 V8 00/10] USB: f81232: V8 patches

[Peter Hung]

Hello,

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

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

Thanks for your review.

-- 
With Best Regards,
Peter Hung

Re: [PATCH V8 00/10] USB: f81232: V8 patches

[Johan Hovold]

On Thu, Mar 05, 2015 at 01:59:45PM +0800, Peter Hung wrote:

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

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

Johan

Re: [PATCH V8 00/10] USB: f81232: V8 patches

[Johan Hovold]

On Thu, Mar 05, 2015 at 08:03:59AM +0100, Johan Hovold wrote:
> On Thu, Mar 05, 2015 at 01:59:45PM +0800, Peter Hung wrote:
> 
> > Did you received the series of F81232 V8 patches ? Please tell me if
> > not received.
> 
> I got it. I just haven't had time to look at it yet. Will try to do so
> this week.

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

Thanks for doing this work!

Johan