This series patch V6 is improvement from V5&V4 as following:
1. transform all function not to use private data as parameter, using
usb_serial_port instead.
2. process_read_urb() add process of Break/FrameError/ParityError/OE.
(patch: 03/10)
3. fix calc_baud_divisor() will cause divide by zero with B0. (patch: 04/10)
4. Some init step we extract it from set_termios() to f81232_port_init()
and run it when open port only. (patch: 04/10)
5. We'll force re-read msr in tiocmget() because the IIR with MSR change
maybe delay received. (patch: 05/10)
6. fix MSR status bits changed but delta bits is 0 will cause read serial port
malfunctional with update port status. (patch: 08/10)
7. Add MSR change statistic when MSR has been read. (patch: 09/10)
8. clarify a lot of code about Johan suggested.
Logs:
1. We had add dev_err() in set/get register function. Also add dev_err()
in some function is to help us easily point out error position, so we
still decide to remain it.
Thanks for reading.
Peter Hung (10):
USB: f81232: rename private struct member name
USB: f81232: implement read IIR/MSR with endpoint
USB: f81232: implement RX bulk-in ep
USB: f81232: implement set_termios
USB: f81232: implement MCR/MSR function
USB: f81232: clarify f81232_ioctl and fix
USB: f81232: fix error in f81232_carrier_raised()
USB: f81232: fix read MSR strange value
USB: f81232: implement delta change for MSR count
USB: f81232: modify/add author
drivers/usb/serial/f81232.c | 495 +++++++++++++++++++++++++++++++++++---------
1 file changed, 399 insertions(+), 96 deletions(-)
--
1.9.1
Change private struct member name from line_status to modem_status.
It will store MSR for some functions used
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
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
The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
, We will do read MSR with interrupt_work worker to do f81232_read_msr() func.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 100 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 669a2f2..ec4609d 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -23,6 +23,7 @@
#include <linux/uaccess.h>
#include <linux/usb.h>
#include <linux/usb/serial.h>
+#include <linux/serial_reg.h>
static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x1934, 0x0706) },
@@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
};
MODULE_DEVICE_TABLE(usb, id_table);
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0
+
+#define SERIAL_BASE_ADDRESS 0x0120
+#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
+
#define CONTROL_DTR 0x01
#define CONTROL_RTS 0x02
@@ -48,19 +56,92 @@ struct f81232_private {
spinlock_t lock;
u8 line_control;
u8 modem_status;
+
+ struct work_struct interrupt_work;
+ struct usb_serial_port *port;
};
-static void f81232_update_line_status(struct usb_serial_port *port,
+static int f81232_get_register(struct usb_serial_port *port,
+ u16 reg, u8 *data)
+{
+ int status;
+ struct usb_device *dev = port->serial->dev;
+
+ status = usb_control_msg(dev,
+ usb_rcvctrlpipe(dev, 0),
+ F81232_REGISTER_REQUEST,
+ F81232_GET_REGISTER,
+ reg,
+ 0,
+ data,
+ sizeof(*data),
+ USB_CTRL_GET_TIMEOUT);
+ if (status < 0)
+ dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+ return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+ int status;
+ unsigned long flags;
+ u8 current_msr;
+ struct tty_struct *tty;
+ struct f81232_private *priv = usb_get_serial_port_data(port);
+
+ status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+ ¤t_msr);
+ if (status < 0) {
+ /* Retain the error even reported in f81232_get_register()
+ to make debug easily :D */
+ dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+ return;
+ }
+
+ if (!(current_msr & UART_MSR_ANY_DELTA))
+ return;
+
+ tty = tty_port_tty_get(&port->port);
+ if (tty) {
+ if (current_msr & UART_MSR_DDCD) {
+ usb_serial_handle_dcd_change(port, tty,
+ current_msr & UART_MSR_DCD);
+ }
+
+ tty_kref_put(tty);
+ }
+
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->modem_status = current_msr;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
+static void f81232_update_modem_status(struct usb_serial_port *port,
unsigned char *data,
unsigned int actual_length)
{
- /*
- * FIXME: Update port->icount, and call
- *
- * wake_up_interruptible(&port->port.delta_msr_wait);
- *
- * on MSR changes.
- */
+ struct f81232_private *priv = usb_get_serial_port_data(port);
+
+ if (!actual_length)
+ return;
+
+ switch (data[0] & 0x07) {
+ case 0x00: /* msr change */
+ dev_dbg(&port->dev, "IIR: MSR Change: %x\n", data[0]);
+ schedule_work(&priv->interrupt_work);
+ break;
+ case 0x02: /* tx-empty */
+ break;
+ case 0x04: /* rx data available */
+ break;
+ case 0x06: /* lsr change */
+ /* we can forget it. the LSR will read from bulk-in */
+ dev_dbg(&port->dev, "IIR: LSR Change: %x\n", data[0]);
+ break;
+ }
}
static void f81232_read_int_callback(struct urb *urb)
@@ -91,7 +172,7 @@ static void f81232_read_int_callback(struct urb *urb)
usb_serial_debug_data(&port->dev, __func__,
urb->actual_length, urb->transfer_buffer);
- f81232_update_line_status(port, data, actual_length);
+ f81232_update_modem_status(port, data, actual_length);
exit:
retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -270,6 +351,14 @@ static int f81232_ioctl(struct tty_struct *tty,
return -ENOIOCTLCMD;
}
+static void f81232_interrupt_work(struct work_struct *work)
+{
+ struct f81232_private *priv =
+ container_of(work, struct f81232_private, interrupt_work);
+
+ f81232_read_msr(priv->port);
+}
+
static int f81232_port_probe(struct usb_serial_port *port)
{
struct f81232_private *priv;
@@ -279,10 +368,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
return -ENOMEM;
spin_lock_init(&priv->lock);
+ INIT_WORK(&priv->interrupt_work, f81232_interrupt_work);
usb_set_serial_port_data(port, priv);
port->port.drain_delay = 256;
+ priv->port = port;
return 0;
}
--
1.9.1
The F81232 bulk-in is RX data + LSR channel, data format is
[LSR+Data][LSR+Data]..... , We had reimplemented in this patch.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 68 +++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index ec4609d..9ea498a 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -185,44 +185,46 @@ exit:
static void f81232_process_read_urb(struct urb *urb)
{
struct usb_serial_port *port = urb->context;
- struct f81232_private *priv = usb_get_serial_port_data(port);
unsigned char *data = urb->transfer_buffer;
- char tty_flag = TTY_NORMAL;
- unsigned long flags;
- u8 line_status;
+ char tty_flag;
int i;
- /* update line status */
- spin_lock_irqsave(&priv->lock, flags);
- line_status = priv->modem_status;
- priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
- spin_unlock_irqrestore(&priv->lock, flags);
-
- if (!urb->actual_length)
+ if (urb->actual_length < 2)
return;
- /* break takes precedence over parity, */
- /* which takes precedence over framing errors */
- if (line_status & UART_BREAK_ERROR)
- tty_flag = TTY_BREAK;
- else if (line_status & UART_PARITY_ERROR)
- tty_flag = TTY_PARITY;
- else if (line_status & UART_FRAME_ERROR)
- tty_flag = TTY_FRAME;
- dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
-
- /* overrun is special, not associated with a char */
- if (line_status & UART_OVERRUN_ERROR)
- tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
-
- if (port->port.console && port->sysrq) {
- for (i = 0; i < urb->actual_length; ++i)
- if (!usb_serial_handle_sysrq_char(port, data[i]))
- tty_insert_flip_char(&port->port, data[i],
- tty_flag);
- } else {
- tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
- urb->actual_length);
+ /* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
+
+ for (i = 0 ; i < urb->actual_length ; i += 2) {
+ tty_flag = TTY_NORMAL;
+
+ if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
+ if (data[i+0] & UART_LSR_BI) {
+ tty_flag = TTY_BREAK;
+ port->icount.brk++;
+ usb_serial_handle_break(port);
+ } else if (data[i+0] & UART_LSR_PE) {
+ tty_flag = TTY_PARITY;
+ port->icount.parity++;
+ } else if (data[i+0] & UART_LSR_FE) {
+ tty_flag = TTY_FRAME;
+ port->icount.frame++;
+ }
+
+ if (data[0] & UART_LSR_OE) {
+ port->icount.overrun++;
+ tty_insert_flip_char(&port->port, 0,
+ TTY_OVERRUN);
+ }
+ }
+
+ if (port->port.console && port->sysrq) {
+ if (!usb_serial_handle_sysrq_char(port, data[i+1]))
+ tty_insert_flip_char(&port->port, data[i+1],
+ tty_flag);
+ } else {
+ tty_insert_flip_string_fixed_flag(&port->port,
+ &data[i+1], tty_flag, 1);
+ }
}
tty_flip_buffer_push(&port->port);
--
1.9.1
The original driver had do not any h/w change in driver.
This patch implements with configure H/W for
baud/parity/word length/stop bits functional.
Some init step extract to f81232_port_init(), called once with open().
And refine baudrate setting to f81232_set_baudrate()
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
1 file changed, 138 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 9ea498a..06d1eb0 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
};
MODULE_DEVICE_TABLE(usb, id_table);
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE 115200L
+
/* USB Control EP parameter */
#define F81232_REGISTER_REQUEST 0xA0
#define F81232_GET_REGISTER 0xc0
+#define F81232_SET_REGISTER 0x40
#define SERIAL_BASE_ADDRESS 0x0120
+#define RECEIVE_BUFFER_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
+#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
#define CONTROL_DTR 0x01
@@ -61,6 +69,14 @@ struct f81232_private {
struct usb_serial_port *port;
};
+static int calc_baud_divisor(u32 baudrate)
+{
+ if (!baudrate)
+ return 0;
+ else
+ return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
static int f81232_get_register(struct usb_serial_port *port,
u16 reg, u8 *data)
{
@@ -82,6 +98,27 @@ static int f81232_get_register(struct usb_serial_port *port,
return status;
}
+static int f81232_set_register(struct usb_serial_port *port,
+ u16 reg, u8 data)
+{
+ int status;
+ struct usb_device *dev = port->serial->dev;
+
+ status = usb_control_msg(dev,
+ usb_sndctrlpipe(dev, 0),
+ F81232_REGISTER_REQUEST,
+ F81232_SET_REGISTER,
+ reg,
+ 0,
+ &data,
+ sizeof(data),
+ USB_CTRL_SET_TIMEOUT);
+ if (status < 0)
+ dev_err(&port->dev, "%s status: %d\n", __func__, status);
+
+ return status;
+}
+
static void f81232_read_msr(struct usb_serial_port *port)
{
int status;
@@ -247,18 +284,106 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
*/
}
+static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
+{
+ u8 divisor;
+ int status = 0;
+
+ divisor = calc_baud_divisor(baudrate);
+
+ status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+ UART_LCR_DLAB); /* DLAB */
+ if (status < 0) {
+ dev_err(&port->dev, "%s status: %d line:%d\n",
+ __func__, status, __LINE__);
+ }
+
+ status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+ divisor & 0x00ff); /* low */
+ if (status < 0) {
+ dev_err(&port->dev, "%s status: %d line:%d\n",
+ __func__, status, __LINE__);
+ }
+
+ status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+ (divisor & 0xff00) >> 8); /* high */
+ if (status < 0) {
+ dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+ status, __LINE__);
+ }
+
+ status = f81232_set_register(port, LINE_CONTROL_REGISTER, 0x00);
+ if (status < 0) {
+ dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
+ status, __LINE__);
+ }
+}
+
+static int f81232_port_init(struct usb_serial_port *port)
+{
+ u8 data;
+ int status = 0;
+
+ /* fifo on, trigger8, clear TX/RX*/
+ data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+ | UART_FCR_CLEAR_XMIT;
+
+ status |= f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
+
+ /* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+ data = UART_IER_MSI;
+
+ /* IER */
+ status |= f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+ if (status < 0) {
+ dev_err(&port->dev, "%s set error: %d\n", __func__, status);
+ return status;
+ }
+
+ return 0;
+}
+
static void f81232_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
- /* FIXME - Stubbed out for now */
+ u8 new_lcr = 0;
+ int status = 0;
- /* Don't change anything if nothing has changed */
- if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
- return;
+ f81232_set_baudrate(port, tty_get_baud_rate(tty));
+
+ if (C_PARENB(tty)) {
+ new_lcr |= UART_LCR_PARITY;
+
+ if (!C_PARODD(tty))
+ new_lcr |= UART_LCR_EPAR;
+
+ if (C_CMSPAR(tty))
+ new_lcr |= UART_LCR_SPAR;
+ }
+
+ if (C_CSTOPB(tty))
+ new_lcr |= UART_LCR_STOP;
+
+ switch (C_CSIZE(tty)) {
+ case CS5:
+ new_lcr |= UART_LCR_WLEN5;
+ break;
+ case CS6:
+ new_lcr |= UART_LCR_WLEN6;
+ break;
+ case CS7:
+ new_lcr |= UART_LCR_WLEN7;
+ break;
+ default:
+ case CS8:
+ new_lcr |= UART_LCR_WLEN8;
+ break;
+ }
+
+ status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
+ if (status < 0)
+ dev_err(&port->dev, "%s set error: %d\n", __func__, status);
- /* Do the real work here... */
- if (old_termios)
- tty_termios_copy_hw(&tty->termios, old_termios);
}
static int f81232_tiocmget(struct tty_struct *tty)
@@ -278,6 +403,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
{
int result;
+ result = f81232_port_init(port);
+ if (result) {
+ dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+ return result;
+ }
+
/* Setup termios */
if (tty)
f81232_set_termios(tty, port, NULL);
--
1.9.1
This patch implement relative MCR/MSR function, such like
tiocmget()/tiocmset()/dtr_rts().
The f81232_set_mctrl() replace set_control_lines() to do MCR control
so we clean-up the set_control_lines() function.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 98 +++++++++++++++++++++++++++++++++++----------
1 file changed, 77 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 06d1eb0..e1cdf42 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -44,6 +44,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
+#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
#define CONTROL_DTR 0x01
@@ -156,6 +157,50 @@ static void f81232_read_msr(struct usb_serial_port *port)
wake_up_interruptible(&port->port.delta_msr_wait);
}
+static int f81232_set_mctrl(struct usb_serial_port *port,
+ unsigned int set, unsigned int clear)
+{
+ u8 urb_value;
+ int status;
+ unsigned long flags;
+ struct f81232_private *priv = usb_get_serial_port_data(port);
+
+ if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+ return 0; /* no change */
+
+ /* 'set' takes precedence over 'clear' */
+ clear &= ~set;
+
+ /* force enable interrupt with OUT2 */
+ urb_value = UART_MCR_OUT2 | priv->line_control;
+
+ if (clear & TIOCM_DTR)
+ urb_value &= ~UART_MCR_DTR;
+
+ if (clear & TIOCM_RTS)
+ urb_value &= ~UART_MCR_RTS;
+
+ if (set & TIOCM_DTR)
+ urb_value |= UART_MCR_DTR;
+
+ if (set & TIOCM_RTS)
+ urb_value |= UART_MCR_RTS;
+
+ dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+ urb_value, priv->line_control);
+
+ status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+ if (status < 0) {
+ dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+ } else {
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->line_control = urb_value;
+ spin_unlock_irqrestore(&priv->lock, flags);
+ }
+
+ return status;
+}
+
static void f81232_update_modem_status(struct usb_serial_port *port,
unsigned char *data,
unsigned int actual_length)
@@ -267,12 +312,6 @@ static void f81232_process_read_urb(struct urb *urb)
tty_flip_buffer_push(&port->port);
}
-static int set_control_lines(struct usb_device *dev, u8 value)
-{
- /* FIXME - Stubbed out for now */
- return 0;
-}
-
static void f81232_break_ctl(struct tty_struct *tty, int break_state)
{
/* FIXME - Stubbed out for now */
@@ -388,15 +427,41 @@ static void f81232_set_termios(struct tty_struct *tty,
static int f81232_tiocmget(struct tty_struct *tty)
{
- /* FIXME - Stubbed out for now */
- return 0;
+ int r;
+ struct usb_serial_port *port = tty->driver_data;
+ struct f81232_private *port_priv = usb_get_serial_port_data(port);
+ unsigned long flags;
+ u8 mcr, msr;
+
+ /* force get current MSR changed state */
+ f81232_read_msr(port);
+
+ spin_lock_irqsave(&port_priv->lock, flags);
+ mcr = port_priv->line_control;
+ msr = port_priv->modem_status;
+ spin_unlock_irqrestore(&port_priv->lock, flags);
+
+ r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+ (mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+ (msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+ (msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+ (msr & UART_MSR_RI ? TIOCM_RI : 0) |
+ (msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+ return r;
}
static int f81232_tiocmset(struct tty_struct *tty,
unsigned int set, unsigned int clear)
{
- /* FIXME - Stubbed out for now */
- return 0;
+ int status;
+ struct usb_serial_port *port = tty->driver_data;
+
+ status = f81232_set_mctrl(port, set, clear);
+ if (status < 0)
+ return usb_translate_errors(status);
+ else
+ return 0;
}
static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
@@ -437,19 +502,10 @@ static void f81232_close(struct usb_serial_port *port)
static void f81232_dtr_rts(struct usb_serial_port *port, int on)
{
- struct f81232_private *priv = usb_get_serial_port_data(port);
- unsigned long flags;
- u8 control;
-
- spin_lock_irqsave(&priv->lock, flags);
- /* Change DTR and RTS */
if (on)
- priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
+ f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
else
- priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
- control = priv->line_control;
- spin_unlock_irqrestore(&priv->lock, flags);
- set_control_lines(port->serial->dev, control);
+ f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
}
static int f81232_carrier_raised(struct usb_serial_port *port)
--
1.9.1
We extract TIOCGSERIAL section in f81232_ioctl() to f81232_get_serial_info()
to make it clarify.
Also we fix device type from 16654 to 16550A, and set it's baud_base
to 115200 (1.8432MHz/16)
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index e1cdf42..e70ec62 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -516,24 +516,32 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
return 0;
}
+static int f81232_get_serial_info(struct usb_serial_port *port,
+ unsigned long arg)
+{
+ struct serial_struct ser;
+
+ memset(&ser, 0, sizeof(ser));
+
+ ser.type = PORT_16550A;
+ ser.line = port->minor;
+ ser.port = port->port_number;
+ ser.baud_base = 115200;
+
+ if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+ return -EFAULT;
+
+ return 0;
+}
+
static int f81232_ioctl(struct tty_struct *tty,
unsigned int cmd, unsigned long arg)
{
- struct serial_struct ser;
struct usb_serial_port *port = tty->driver_data;
switch (cmd) {
case TIOCGSERIAL:
- memset(&ser, 0, sizeof ser);
- ser.type = PORT_16654;
- ser.line = port->minor;
- ser.port = port->port_number;
- ser.baud_base = 460800;
-
- if (copy_to_user((void __user *)arg, &ser, sizeof ser))
- return -EFAULT;
-
- return 0;
+ return f81232_get_serial_info(port, arg);
default:
break;
}
--
1.9.1
It's should compared with UART_MSR_DCD, not UART_DCD.
also we clean-up some non-used define to avoid impropriety use.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index e70ec62..c87a3eb 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -47,20 +47,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
-#define CONTROL_DTR 0x01
-#define CONTROL_RTS 0x02
-
-#define UART_STATE 0x08
-#define UART_STATE_TRANSIENT_MASK 0x74
-#define UART_DCD 0x01
-#define UART_DSR 0x02
-#define UART_BREAK_ERROR 0x04
-#define UART_RING 0x08
-#define UART_FRAME_ERROR 0x10
-#define UART_PARITY_ERROR 0x20
-#define UART_OVERRUN_ERROR 0x40
-#define UART_CTS 0x80
-
struct f81232_private {
spinlock_t lock;
u8 line_control;
@@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
static int f81232_carrier_raised(struct usb_serial_port *port)
{
struct f81232_private *priv = usb_get_serial_port_data(port);
- if (priv->modem_status & UART_DCD)
+ if (priv->modem_status & UART_MSR_DCD)
return 1;
return 0;
}
--
1.9.1
When we use RS232 loopback, assume doing RTS change will cause
CTS change, DTR change will cause DCD/DSR change too.
Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of
MSR(delta) maybe not changed when set & get MCR fasterly.
So we add more check not only UART_MSR_ANY_DELTA but also with
comparing DCD/RI/DSR/CTS change with old value. Due to the state
bit is always correct, we direct save msr when read.
The following step to reproduce this problem with while loop step 1~4:
1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index c87a3eb..94c05d7 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -110,7 +110,8 @@ static void f81232_read_msr(struct usb_serial_port *port)
{
int status;
unsigned long flags;
- u8 current_msr;
+ u8 current_msr, prev_msr;
+ u8 msr_mask = ~UART_MSR_ANY_DELTA;
struct tty_struct *tty;
struct f81232_private *priv = usb_get_serial_port_data(port);
@@ -123,7 +124,21 @@ static void f81232_read_msr(struct usb_serial_port *port)
return;
}
- if (!(current_msr & UART_MSR_ANY_DELTA))
+ /*
+ * The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
+ * maybe not change when set & get MCR fasterly.
+ *
+ * So we add more check with comparing DCD/RI/DSR/CTS
+ * change. and direct save msr when read.
+ */
+
+ spin_lock_irqsave(&priv->lock, flags);
+ prev_msr = priv->modem_status;
+ priv->modem_status = current_msr;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ if (!(current_msr & UART_MSR_ANY_DELTA) &&
+ !((prev_msr ^ current_msr) & msr_mask))
return;
tty = tty_port_tty_get(&port->port);
@@ -136,10 +151,6 @@ static void f81232_read_msr(struct usb_serial_port *port)
tty_kref_put(tty);
}
- spin_lock_irqsave(&priv->lock, flags);
- priv->modem_status = current_msr;
- spin_unlock_irqrestore(&priv->lock, flags);
-
wake_up_interruptible(&port->port.delta_msr_wait);
}
--
1.9.1
We implement delta change for MSR counting. This patch is referenced
from ftdi_sio.c
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 94c05d7..5134a19 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -112,6 +112,7 @@ static void f81232_read_msr(struct usb_serial_port *port)
unsigned long flags;
u8 current_msr, prev_msr;
u8 msr_mask = ~UART_MSR_ANY_DELTA;
+ u8 msr_changed_bit;
struct tty_struct *tty;
struct f81232_private *priv = usb_get_serial_port_data(port);
@@ -141,14 +142,30 @@ static void f81232_read_msr(struct usb_serial_port *port)
!((prev_msr ^ current_msr) & msr_mask))
return;
- tty = tty_port_tty_get(&port->port);
- if (tty) {
- if (current_msr & UART_MSR_DDCD) {
+ /* find checked delta bits set */
+ msr_changed_bit =
+ (current_msr & UART_MSR_ANY_DELTA) << 4;
+
+ /* append with not delta but changed bits */
+ msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
+
+ if (msr_changed_bit & UART_MSR_CTS)
+ port->icount.cts++;
+ if (msr_changed_bit & UART_MSR_DSR)
+ port->icount.dsr++;
+ if (msr_changed_bit & UART_MSR_RI)
+ port->icount.rng++;
+ if (msr_changed_bit & UART_MSR_DCD) {
+
+ port->icount.dcd++;
+ tty = tty_port_tty_get(&port->port);
+ if (tty) {
+
usb_serial_handle_dcd_change(port, tty,
current_msr & UART_MSR_DCD);
- }
- tty_kref_put(tty);
+ tty_kref_put(tty);
+ }
}
wake_up_interruptible(&port->port.delta_msr_wait);
--
1.9.1
Add me to co-author and fix no '>' in greg kh's email
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 5134a19..5e35859 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -632,5 +632,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
module_usb_serial_driver(serial_drivers, id_table);
MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
-MODULE_AUTHOR("Greg Kroah-Hartman <[email protected]");
+MODULE_AUTHOR("Greg Kroah-Hartman <[email protected]>");
+MODULE_AUTHOR("Peter Hong <[email protected]>");
MODULE_LICENSE("GPL v2");
--
1.9.1
On Mon, Feb 16, 2015 at 03:57:55PM +0800, Peter Hung wrote:
> The F81232 bulk-in is RX data + LSR channel, data format is
> [LSR+Data][LSR+Data]..... , We had reimplemented in this patch.
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 68 +++++++++++++++++++++++----------------------
> 1 file changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index ec4609d..9ea498a 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -185,44 +185,46 @@ exit:
> static void f81232_process_read_urb(struct urb *urb)
> {
> struct usb_serial_port *port = urb->context;
> - struct f81232_private *priv = usb_get_serial_port_data(port);
> unsigned char *data = urb->transfer_buffer;
> - char tty_flag = TTY_NORMAL;
> - unsigned long flags;
> - u8 line_status;
> + char tty_flag;
> int i;
>
> - /* update line status */
> - spin_lock_irqsave(&priv->lock, flags);
> - line_status = priv->modem_status;
> - priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
> - spin_unlock_irqrestore(&priv->lock, flags);
> -
> - if (!urb->actual_length)
> + if (urb->actual_length < 2)
> return;
>
> - /* break takes precedence over parity, */
> - /* which takes precedence over framing errors */
> - if (line_status & UART_BREAK_ERROR)
> - tty_flag = TTY_BREAK;
> - else if (line_status & UART_PARITY_ERROR)
> - tty_flag = TTY_PARITY;
> - else if (line_status & UART_FRAME_ERROR)
> - tty_flag = TTY_FRAME;
> - dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> -
> - /* overrun is special, not associated with a char */
> - if (line_status & UART_OVERRUN_ERROR)
> - tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> -
> - if (port->port.console && port->sysrq) {
> - for (i = 0; i < urb->actual_length; ++i)
> - if (!usb_serial_handle_sysrq_char(port, data[i]))
> - tty_insert_flip_char(&port->port, data[i],
> - tty_flag);
> - } else {
> - tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
> - urb->actual_length);
> + /* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
> +
> + for (i = 0 ; i < urb->actual_length ; i += 2) {
> + tty_flag = TTY_NORMAL;
> +
> + if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
Never use unlikely() unless you can prove that it actually matters if
you use it. Hint, it's almost impossible to prove, so don't use it, the
compiler and processor look-ahead is almost smarter than we are.
thanks,
greg k-h
Hello,
Greg KH 於 2015/2/17 上午 03:41 寫道:
>> + if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
>
> Never use unlikely() unless you can prove that it actually matters if
> you use it. Hint, it's almost impossible to prove, so don't use it, the
> compiler and processor look-ahead is almost smarter than we are.
Thanks for your hint. I'll remove it with next patch.
--
With Best Regards,
Peter Hung
On Mon, Feb 16, 2015 at 03:57:54PM +0800, Peter Hung wrote:
> The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
> , We will do read MSR with interrupt_work worker to do f81232_read_msr() func.
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 100 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..ec4609d 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,6 +23,7 @@
> #include <linux/uaccess.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
>
> static const struct usb_device_id id_table[] = {
> { USB_DEVICE(0x1934, 0x0706) },
> @@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
> };
> MODULE_DEVICE_TABLE(usb, id_table);
>
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST 0xA0
> +#define F81232_GET_REGISTER 0xc0
Indent these define values using a tab as well.
> +
> +#define SERIAL_BASE_ADDRESS 0x0120
> +#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
> +
> #define CONTROL_DTR 0x01
> #define CONTROL_RTS 0x02
>
> @@ -48,19 +56,92 @@ struct f81232_private {
> spinlock_t lock;
> u8 line_control;
> u8 modem_status;
> +
Newline not needed.
> + struct work_struct interrupt_work;
> + struct usb_serial_port *port;
> };
>
> -static void f81232_update_line_status(struct usb_serial_port *port,
> +static int f81232_get_register(struct usb_serial_port *port,
> + u16 reg, u8 *data)
No need to break this line any more.
> +{
> + int status;
> + struct usb_device *dev = port->serial->dev;
> +
> + status = usb_control_msg(dev,
> + usb_rcvctrlpipe(dev, 0),
> + F81232_REGISTER_REQUEST,
> + F81232_GET_REGISTER,
> + reg,
> + 0,
> + data,
> + sizeof(*data),
> + USB_CTRL_GET_TIMEOUT);
There's some odd indentation above and in other patches as well (in this
case two tabs and one space). Please either just tabs or, if you prefer,
add additional spaces to align all the arguments with the first.
> + if (status < 0)
Zero is actually also an error here (returned length was 0). You may
want to let this function return -EIO in that case (and perhaps also
return 0 on success) or you need to fix this at every call site.
> + dev_err(&port->dev, "%s status: %d\n", __func__, status);
Please spell out that there was an error, for example,
"%s failed: %d\n"
> +
> + return status;
> +}
Could you add the get/set register helpers in a separate preparatory patch?
> +
> +static void f81232_read_msr(struct usb_serial_port *port)
> +{
> + int status;
> + unsigned long flags;
> + u8 current_msr;
> + struct tty_struct *tty;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + status = f81232_get_register(port, MODEM_STATUS_REGISTER,
> + ¤t_msr);
You cannot use a variable on the stack for usb_control_msg as the buffer
must be DMA-able (on all platforms). Use kzalloc.
> + if (status < 0) {
> + /* Retain the error even reported in f81232_get_register()
> + to make debug easily :D */
No need for this comment.
> + dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
> + return;
> + }
> +
> + if (!(current_msr & UART_MSR_ANY_DELTA))
> + return;
> +
> + tty = tty_port_tty_get(&port->port);
> + if (tty) {
> + if (current_msr & UART_MSR_DDCD) {
> + usb_serial_handle_dcd_change(port, tty,
> + current_msr & UART_MSR_DCD);
Please indent continuation lines at least two tabs further (i.e add one
more tab here).
> + }
> +
> + tty_kref_put(tty);
> + }
You should rewrite this so you only get the tty reference if you're
actually gonna use it.
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->modem_status = current_msr;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + wake_up_interruptible(&port->port.delta_msr_wait);
> +}
> +
> +static void f81232_update_modem_status(struct usb_serial_port *port,
This one should still be called f81232_update_line_status.
> unsigned char *data,
> unsigned int actual_length)
Just call it length or size, you can use size_t.
> {
> - /*
> - * FIXME: Update port->icount, and call
> - *
> - * wake_up_interruptible(&port->port.delta_msr_wait);
> - *
> - * on MSR changes.
> - */
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + if (!actual_length)
> + return;
> +
> + switch (data[0] & 0x07) {
> + case 0x00: /* msr change */
> + dev_dbg(&port->dev, "IIR: MSR Change: %x\n", data[0]);
%02x
> + schedule_work(&priv->interrupt_work);
> + break;
> + case 0x02: /* tx-empty */
> + break;
> + case 0x04: /* rx data available */
> + break;
> + case 0x06: /* lsr change */
> + /* we can forget it. the LSR will read from bulk-in */
> + dev_dbg(&port->dev, "IIR: LSR Change: %x\n", data[0]);
%02x
> + break;
> + }
> }
Johan
On Mon, Feb 16, 2015 at 03:57:56PM +0800, Peter Hung wrote:
> The original driver had do not any h/w change in driver.
> This patch implements with configure H/W for
> baud/parity/word length/stop bits functional.
>
> Some init step extract to f81232_port_init(), called once with open().
> And refine baudrate setting to f81232_set_baudrate()
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 138 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 9ea498a..06d1eb0 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
> };
> MODULE_DEVICE_TABLE(usb, id_table);
>
> +/* Maximum baudrate for F81232 */
> +#define F81232_MAX_BAUDRATE 115200L
No need for L.
> +
> /* USB Control EP parameter */
> #define F81232_REGISTER_REQUEST 0xA0
> #define F81232_GET_REGISTER 0xc0
> +#define F81232_SET_REGISTER 0x40
>
> #define SERIAL_BASE_ADDRESS 0x0120
> +#define RECEIVE_BUFFER_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> #define CONTROL_DTR 0x01
> @@ -61,6 +69,14 @@ struct f81232_private {
> struct usb_serial_port *port;
> };
>
> +static int calc_baud_divisor(u32 baudrate)
> +{
> + if (!baudrate)
> + return 0;
> + else
> + return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
> +}
You should handle B0 at the call site (and in that case keep the current
baudrate).
> +
> static int f81232_get_register(struct usb_serial_port *port,
> u16 reg, u8 *data)
> {
> @@ -82,6 +98,27 @@ static int f81232_get_register(struct usb_serial_port *port,
> return status;
> }
>
> +static int f81232_set_register(struct usb_serial_port *port,
> + u16 reg, u8 data)
No need to break line.
> +{
> + int status;
> + struct usb_device *dev = port->serial->dev;
> +
> + status = usb_control_msg(dev,
> + usb_sndctrlpipe(dev, 0),
> + F81232_REGISTER_REQUEST,
> + F81232_SET_REGISTER,
> + reg,
> + 0,
> + &data,
> + sizeof(data),
> + USB_CTRL_SET_TIMEOUT);
As for f81232_get_register, you must use a DMA-able buffer for the
data buffer. Either allocate it in this function or at the call site.
> + if (status < 0)
> + dev_err(&port->dev, "%s status: %d\n", __func__, status);
> +
> + return status;
> +}
Same comments apply as for f81232_set_register on the previous patch.
> +
> static void f81232_read_msr(struct usb_serial_port *port)
> {
> int status;
> @@ -247,18 +284,106 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> */
> }
>
> +static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
> +{
> + u8 divisor;
> + int status = 0;
> +
> + divisor = calc_baud_divisor(baudrate);
> +
> + status = f81232_set_register(port, LINE_CONTROL_REGISTER,
> + UART_LCR_DLAB); /* DLAB */
> + if (status < 0) {
As with f81232_get_register 0 is also an error.
> + dev_err(&port->dev, "%s status: %d line:%d\n",
> + __func__, status, __LINE__);
Don't use __LINE__ in your error messages. The dynamic debugging
infrastructure can add that if ever needed.
Spell out the errors instead (e.g. "failed to set DLAB").
Shouldn't you be ORing DLAB to the current state of LCR by the way?
And make sure to bail out on errors here.
> + }
> +
> + status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
> + divisor & 0x00ff); /* low */
> + if (status < 0) {
> + dev_err(&port->dev, "%s status: %d line:%d\n",
> + __func__, status, __LINE__);
> + }
> +
> + status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
> + (divisor & 0xff00) >> 8); /* high */
> + if (status < 0) {
> + dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
> + status, __LINE__);
> + }
> +
> + status = f81232_set_register(port, LINE_CONTROL_REGISTER, 0x00);
Only clear DLAB?
> + if (status < 0) {
> + dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
> + status, __LINE__);
> + }
> +}
> +
> +static int f81232_port_init(struct usb_serial_port *port)
> +{
> + u8 data;
> + int status = 0;
> +
> + /* fifo on, trigger8, clear TX/RX*/
> + data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
> + | UART_FCR_CLEAR_XMIT;
> +
> + status |= f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
Just assign return value and don't initialise above.
And just bail out on errors directly here.
> +
> + /* MSR Interrupt only, LSR will read from Bulk-in odd byte */
> + data = UART_IER_MSI;
> +
> + /* IER */
> + status |= f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
Just use = here as well.
> + if (status < 0) {
> + dev_err(&port->dev, "%s set error: %d\n", __func__, status);
> + return status;
> + }
> +
> + return 0;
> +}
> +
> static void f81232_set_termios(struct tty_struct *tty,
> struct usb_serial_port *port, struct ktermios *old_termios)
> {
> - /* FIXME - Stubbed out for now */
> + u8 new_lcr = 0;
> + int status = 0;
>
> - /* Don't change anything if nothing has changed */
> - if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> - return;
> + f81232_set_baudrate(port, tty_get_baud_rate(tty));
So here you should check for B0 and in that case simply not update the
baudrate.
B0 is, as I believe I already mentioned, used to hang up any modem by
dropping DTR.
> +
> + if (C_PARENB(tty)) {
> + new_lcr |= UART_LCR_PARITY;
> +
> + if (!C_PARODD(tty))
> + new_lcr |= UART_LCR_EPAR;
> +
> + if (C_CMSPAR(tty))
> + new_lcr |= UART_LCR_SPAR;
> + }
> +
> + if (C_CSTOPB(tty))
> + new_lcr |= UART_LCR_STOP;
> +
> + switch (C_CSIZE(tty)) {
> + case CS5:
> + new_lcr |= UART_LCR_WLEN5;
> + break;
> + case CS6:
> + new_lcr |= UART_LCR_WLEN6;
> + break;
> + case CS7:
> + new_lcr |= UART_LCR_WLEN7;
> + b<reak;
> + default:z
> + case CS8:
> + new_lcr |= UART_LCR_WLEN8;
> + break;
> + }
> +
> + status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
> + if (status < 0)
> + dev_err(&port->dev, "%s set error: %d\n", __func__, status);
>
> - /* Do the real work here... */
> - if (old_termios)
> - tty_termios_copy_hw(&tty->termios, old_termios);
> }
>
> static int f81232_tiocmget(struct tty_struct *tty)
> @@ -278,6 +403,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
> {
> int result;
>
> + result = f81232_port_init(port);
> + if (result) {
> + dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
> + return result;
> + }
Don't you wan't to reverse f81232_port_init at close (e.g. disable
interrupts)?
> +
> /* Setup termios */
> if (tty)
> f81232_set_termios(tty, port, NULL);
Johan
On Mon, Feb 16, 2015 at 03:57:57PM +0800, Peter Hung wrote:
> This patch implement relative MCR/MSR function, such like
> tiocmget()/tiocmset()/dtr_rts().
>
> The f81232_set_mctrl() replace set_control_lines() to do MCR control
> so we clean-up the set_control_lines() function.
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 98 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 77 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 06d1eb0..e1cdf42 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -44,6 +44,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
> #define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
> #define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
> +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> #define CONTROL_DTR 0x01
> @@ -156,6 +157,50 @@ static void f81232_read_msr(struct usb_serial_port *port)
> wake_up_interruptible(&port->port.delta_msr_wait);
> }
>
> +static int f81232_set_mctrl(struct usb_serial_port *port,
> + unsigned int set, unsigned int clear)
> +{
> + u8 urb_value;
> + int status;
> + unsigned long flags;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
> + return 0; /* no change */
> +
> + /* 'set' takes precedence over 'clear' */
> + clear &= ~set;
> +
> + /* force enable interrupt with OUT2 */
> + urb_value = UART_MCR_OUT2 | priv->line_control;
You should rename this one priv->modem_control.
And what about locking above?
> +
> + if (clear & TIOCM_DTR)
> + urb_value &= ~UART_MCR_DTR;
> +
> + if (clear & TIOCM_RTS)
> + urb_value &= ~UART_MCR_RTS;
> +
> + if (set & TIOCM_DTR)
> + urb_value |= UART_MCR_DTR;
> +
> + if (set & TIOCM_RTS)
> + urb_value |= UART_MCR_RTS;
> +
> + dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
> + urb_value, priv->line_control);
> +
> + status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
> + if (status < 0) {
> + dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
Return here on errors and skip the else branch.
> + } else {
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->line_control = urb_value;
> + spin_unlock_irqrestore(&priv->lock, flags);
It looks like you could replace the private spin lock with a mutex and
protect all accesses to MCR and MSR using it.
> + }
> +
> + return status;
Return 0;
> +}
> +
> static void f81232_update_modem_status(struct usb_serial_port *port,
> unsigned char *data,
> unsigned int actual_length)
> @@ -267,12 +312,6 @@ static void f81232_process_read_urb(struct urb *urb)
> tty_flip_buffer_push(&port->port);
> }
>
> -static int set_control_lines(struct usb_device *dev, u8 value)
> -{
> - /* FIXME - Stubbed out for now */
> - return 0;
> -}
> -
> static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> {
> /* FIXME - Stubbed out for now */
> @@ -388,15 +427,41 @@ static void f81232_set_termios(struct tty_struct *tty,
>
> static int f81232_tiocmget(struct tty_struct *tty)
> {
> - /* FIXME - Stubbed out for now */
> - return 0;
> + int r;
> + struct usb_serial_port *port = tty->driver_data;
> + struct f81232_private *port_priv = usb_get_serial_port_data(port);
> + unsigned long flags;
> + u8 mcr, msr;
> +
> + /* force get current MSR changed state */
> + f81232_read_msr(port);
> +
> + spin_lock_irqsave(&port_priv->lock, flags);
> + mcr = port_priv->line_control;
> + msr = port_priv->modem_status;
> + spin_unlock_irqrestore(&port_priv->lock, flags);
> +
> + r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
> + (mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
> + (msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
> + (msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
> + (msr & UART_MSR_RI ? TIOCM_RI : 0) |
> + (msr & UART_MSR_DSR ? TIOCM_DSR : 0);
> +
> + return r;
> }
>
> static int f81232_tiocmset(struct tty_struct *tty,
> unsigned int set, unsigned int clear)
> {
> - /* FIXME - Stubbed out for now */
> - return 0;
> + int status;
> + struct usb_serial_port *port = tty->driver_data;
> +
> + status = f81232_set_mctrl(port, set, clear);
> + if (status < 0)
> + return usb_translate_errors(status);
> + else
> + return 0;
Remove the else branch and just return 0 at the end.
Johan
On Mon, Feb 16, 2015 at 03:57:59PM +0800, Peter Hung wrote:
> It's should compared with UART_MSR_DCD, not UART_DCD.
> also we clean-up some non-used define to avoid impropriety use.
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index e70ec62..c87a3eb 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -47,20 +47,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> -#define CONTROL_DTR 0x01
> -#define CONTROL_RTS 0x02
> -
> -#define UART_STATE 0x08
> -#define UART_STATE_TRANSIENT_MASK 0x74
> -#define UART_DCD 0x01
> -#define UART_DSR 0x02
> -#define UART_BREAK_ERROR 0x04
> -#define UART_RING 0x08
> -#define UART_FRAME_ERROR 0x10
> -#define UART_PARITY_ERROR 0x20
> -#define UART_OVERRUN_ERROR 0x40
> -#define UART_CTS 0x80
> -
> struct f81232_private {
> spinlock_t lock;
> u8 line_control;
> @@ -511,7 +497,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
> static int f81232_carrier_raised(struct usb_serial_port *port)
> {
> struct f81232_private *priv = usb_get_serial_port_data(port);
> - if (priv->modem_status & UART_DCD)
> + if (priv->modem_status & UART_MSR_DCD)
Could you fix the separately before removing the unused defines and also
add the missing locking?
> return 1;
> return 0;
> }
Thanks,
Johan
On Mon, Feb 16, 2015 at 03:58:00PM +0800, Peter Hung wrote:
> When we use RS232 loopback, assume doing RTS change will cause
> CTS change, DTR change will cause DCD/DSR change too.
>
> Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of
> MSR(delta) maybe not changed when set & get MCR fasterly.
I think you mean "rapidly" here.
> So we add more check not only UART_MSR_ANY_DELTA but also with
> comparing DCD/RI/DSR/CTS change with old value. Due to the state
> bit is always correct, we direct save msr when read.
>
> The following step to reproduce this problem with while loop step 1~4:
> 1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
> 2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
> 3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
> 4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
Without having looked at this very closely; are you sure this is a
hardware issue and not related to the locking issues I pointed at in my
comments to tiocmset?
Johan
From: Greg KH
> > + for (i = 0 ; i < urb->actual_length ; i += 2) {
> > + tty_flag = TTY_NORMAL;
> > +
> > + if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
>
> Never use unlikely() unless you can prove that it actually matters if
> you use it. Hint, it's almost impossible to prove, so don't use it, the
> compiler and processor look-ahead is almost smarter than we are.
That just isn't true.
The compiler cannot know the actual control flow - so cannot correctly
arrange the code so that the branches are statically predicted
correctly for the required path (usually the most common path).
There are a lot of places where a few extra clocks for a mispredicted
branch don't really matter, and even in very hot paths where it does
matter it can be quite difficult to get the compiler to optimise the
branches 'correctly' - you can need to add asm comments in order to
generate non-empty code blocks.
In addition unlikely() is also a note to the human reader.
I did a lot of work adding likely/unlikely to some code in order
to minimise the 'worst case' code path. I got there, but some
parts were initially non-intuitive.
david
On Tue, Feb 17, 2015 at 10:06:07AM +0000, David Laight wrote:
> From: Greg KH
> > > + for (i = 0 ; i < urb->actual_length ; i += 2) {
> > > + tty_flag = TTY_NORMAL;
> > > +
> > > + if (unlikely(data[i+0] & UART_LSR_BRK_ERROR_BITS)) {
> >
> > Never use unlikely() unless you can prove that it actually matters if
> > you use it. Hint, it's almost impossible to prove, so don't use it, the
> > compiler and processor look-ahead is almost smarter than we are.
>
> That just isn't true.
>
> The compiler cannot know the actual control flow - so cannot correctly
> arrange the code so that the branches are statically predicted
> correctly for the required path (usually the most common path).
>
> There are a lot of places where a few extra clocks for a mispredicted
> branch don't really matter, and even in very hot paths where it does
> matter it can be quite difficult to get the compiler to optimise the
> branches 'correctly' - you can need to add asm comments in order to
> generate non-empty code blocks.
>
> In addition unlikely() is also a note to the human reader.
>
> I did a lot of work adding likely/unlikely to some code in order
> to minimise the 'worst case' code path. I got there, but some
> parts were initially non-intuitive.
Yes, but remember that old patch that Andi did to actually check to see
if unlikely/likely mattered and was placed correctly? Turns out that
90% of the usages were wrong. So humans are horrible at using these
markings, so I will not accept them unless you can _prove_ it matters in
the code.
For a urb callback, that's not an issue at all, the usb callback is so
slow that you will almost never make a difference, sorry.
So again, don't do it in driver code unless you can prove it.
thanks,
greg k-h
Hello,
Johan Hovold 於 2015/2/17 下午 05:51 寫道:
>> So we add more check not only UART_MSR_ANY_DELTA but also with
>> comparing DCD/RI/DSR/CTS change with old value. Due to the state
>> bit is always correct, we direct save msr when read.
>>
>> The following step to reproduce this problem with while loop step 1~4:
>> 1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
>> 2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
>> 3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
>> 4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
>
> Without having looked at this very closely; are you sure this is a
> hardware issue and not related to the locking issues I pointed at in my
> comments to tiocmset?
>
> Johan
>
Thank for your review.
I'll apply all suggestions to next v7 patch.
--
With Best Regards,
Peter Hung