This series patch V7 is changed from V6 as following:
1. The buffer of usb_control_msg() in set_register()/get_register() are change
from local variable to kmalloc(). (PATCH V7 02/11)
2. Change all set_register()/get_register() return value 0 is success,
otherwise are failed. ( return 0 of usb_control_msg() treat as -EIO,
PATCH V7 02/11)
3. tty_port_tty_get() called only when DCD has changed. (PATCH V7 05/11)
4. remove likely()/unlikely() branch prediction.
5. Implement DTR/RTS control when baudrate B0. We drop DTR/RTS when B0,
otherwise enable it. (PATCH V7 08/11)
6. Change private struct line_control to modem_control with meanful.
(PATCH V7 06/11)
7. We confirmd MSR strange delta value is not locking-issue. The issue
maybe reproduce with set MCR & get MSR before IIR notice with MSR changed.
V6 (old change):
1. transform all function not to use private data as parameter, using
usb_serial_port instead.
2. process_read_urb() add process of Break/FrameError/ParityError/OE.
(patch: 03/10)
3. fix calc_baud_divisor() will cause divide by zero with B0. (patch: 04/10)
4. Some init step we extract it from set_termios() to f81232_port_init()
and run it when open port only. (patch: 04/10)
5. We'll force re-read msr in tiocmget() because the IIR with MSR change
maybe delay received. (patch: 05/10)
6. fix MSR status bits changed but delta bits is 0 will cause read serial port
malfunctional with update port status. (patch: 08/10)
7. Add MSR change statistic when MSR has been read. (patch: 09/10)
8. clarify a lot of code about Johan suggested.
Peter Hung (11):
USB: f81232: rename private struct member name
USB: f81232: add preparatory functions
USB: f81232: implement RX bulk-in EP
USB: f81232: change lock mechanism
USB: f81232: implement read IIR/MSR with endpoint
USB: f81232: implement MCR/MSR function
USB: f81232: implement port_enable function
USB: f81232: implement set_termios()
USB: f81232: clarify f81232_ioctl() and fix
USB: f81232: cleanup non-used define
USB: f81232: modify/add author
drivers/usb/serial/f81232.c | 554 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 458 insertions(+), 96 deletions(-)
--
1.9.1
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
We add f81232_get_register()/f81232_set_register() and necessary defines
for future patch with communication with F81232 USB control endpoint.
Because of this is a preparatory patch, there are unused function warning
with compiling. The functions will used with following patches.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 669a2f2..1f29b95 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -30,6 +30,11 @@ static const struct usb_device_id id_table[] = {
};
MODULE_DEVICE_TABLE(usb, id_table);
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0
+#define F81232_SET_REGISTER 0x40
+
#define CONTROL_DTR 0x01
#define CONTROL_RTS 0x02
@@ -50,6 +55,80 @@ struct f81232_private {
u8 modem_status;
};
+static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
+{
+ int status;
+ u8 *tmp;
+ struct usb_device *dev = port->serial->dev;
+
+ if (!data)
+ return -EFAULT;
+
+ tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ status = usb_control_msg(dev,
+ usb_rcvctrlpipe(dev, 0),
+ F81232_REGISTER_REQUEST,
+ F81232_GET_REGISTER,
+ reg,
+ 0,
+ tmp,
+ sizeof(u8),
+ USB_CTRL_GET_TIMEOUT);
+ if (status <= 0) {
+ /* show something with failed */
+ dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+ if (status == 0)
+ status = -EIO;
+ else
+ status = usb_translate_errors(status);
+ } else {
+ status = 0; /* on success */
+ memcpy((void*) data, (void*) tmp, sizeof(u8));
+ }
+
+ kfree(tmp);
+ return status;
+}
+
+static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
+{
+ int status;
+ u8 *tmp;
+ struct usb_device *dev = port->serial->dev;
+
+ tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ memcpy((void*) tmp, (void*) &data, sizeof(u8));
+
+ status = usb_control_msg(dev,
+ usb_sndctrlpipe(dev, 0),
+ F81232_REGISTER_REQUEST,
+ F81232_SET_REGISTER,
+ reg,
+ 0,
+ tmp,
+ sizeof(u8),
+ USB_CTRL_SET_TIMEOUT);
+ if (status <= 0) {
+ /* show something with failed */
+ dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
+
+ if (status == 0)
+ status = -EIO;
+ else
+ status = usb_translate_errors(status);
+ } else
+ status = 0; /* on success */
+
+ kfree(tmp);
+ return status;
+}
static void f81232_update_line_status(struct usb_serial_port *port,
unsigned char *data,
unsigned int actual_length)
--
1.9.1
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 <[email protected]>
---
drivers/usb/serial/f81232.c | 69 +++++++++++++++++++++++----------------------
1 file changed, 36 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 1f29b95..419e2d6 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -23,6 +23,7 @@
#include <linux/uaccess.h>
#include <linux/usb.h>
#include <linux/usb/serial.h>
+#include <linux/serial_reg.h>
static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x1934, 0x0706) },
@@ -183,44 +184,46 @@ exit:
static void f81232_process_read_urb(struct urb *urb)
{
struct usb_serial_port *port = urb->context;
- struct f81232_private *priv = usb_get_serial_port_data(port);
unsigned char *data = urb->transfer_buffer;
- char tty_flag = TTY_NORMAL;
- unsigned long flags;
- u8 line_status;
+ char tty_flag;
int i;
+ u8 lsr;
- /* update line status */
- spin_lock_irqsave(&priv->lock, flags);
- line_status = priv->modem_status;
- priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
- spin_unlock_irqrestore(&priv->lock, flags);
-
- if (!urb->actual_length)
+ if (urb->actual_length < 2)
return;
- /* break takes precedence over parity, */
- /* which takes precedence over framing errors */
- if (line_status & UART_BREAK_ERROR)
- tty_flag = TTY_BREAK;
- else if (line_status & UART_PARITY_ERROR)
- tty_flag = TTY_PARITY;
- else if (line_status & UART_FRAME_ERROR)
- tty_flag = TTY_FRAME;
- dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
-
- /* overrun is special, not associated with a char */
- if (line_status & UART_OVERRUN_ERROR)
- tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
-
- if (port->port.console && port->sysrq) {
- for (i = 0; i < urb->actual_length; ++i)
- if (!usb_serial_handle_sysrq_char(port, data[i]))
- tty_insert_flip_char(&port->port, data[i],
- tty_flag);
- } else {
- tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
- urb->actual_length);
+ /* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
+
+ for (i = 0; i < urb->actual_length; i += 2) {
+ tty_flag = TTY_NORMAL;
+ lsr = data[i + 0];
+
+ if (lsr & UART_LSR_BRK_ERROR_BITS) {
+ if (lsr & UART_LSR_BI) {
+ tty_flag = TTY_BREAK;
+ port->icount.brk++;
+ usb_serial_handle_break(port);
+ } else if (lsr & UART_LSR_PE) {
+ tty_flag = TTY_PARITY;
+ port->icount.parity++;
+ } else if (lsr & UART_LSR_FE) {
+ tty_flag = TTY_FRAME;
+ port->icount.frame++;
+ }
+
+ if (lsr & UART_LSR_OE) {
+ port->icount.overrun++;
+ tty_insert_flip_char(&port->port, 0,
+ TTY_OVERRUN);
+ }
+ }
+
+ if (port->port.console && port->sysrq) {
+ if (usb_serial_handle_sysrq_char(port, data[i + 1]))
+ continue;
+ }
+
+ tty_insert_flip_char(&port->port, data[i + 1], tty_flag);
}
tty_flip_buffer_push(&port->port);
--
1.9.1
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 <[email protected]>
---
drivers/usb/serial/f81232.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 419e2d6..bf072fe 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -19,7 +19,7 @@
#include <linux/serial.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/usb.h>
#include <linux/usb/serial.h>
@@ -51,7 +51,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define UART_CTS 0x80
struct f81232_private {
- spinlock_t lock;
+ struct mutex lock;
u8 line_control;
u8 modem_status;
};
@@ -306,17 +306,16 @@ static void f81232_close(struct usb_serial_port *port)
static void f81232_dtr_rts(struct usb_serial_port *port, int on)
{
struct f81232_private *priv = usb_get_serial_port_data(port);
- unsigned long flags;
u8 control;
- spin_lock_irqsave(&priv->lock, flags);
+ mutex_lock(&priv->lock);
/* Change DTR and RTS */
if (on)
priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
else
priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
control = priv->line_control;
- spin_unlock_irqrestore(&priv->lock, flags);
+ mutex_unlock(&priv->lock);
set_control_lines(port->serial->dev, control);
}
@@ -360,7 +359,7 @@ static int f81232_port_probe(struct usb_serial_port *port)
if (!priv)
return -ENOMEM;
- spin_lock_init(&priv->lock);
+ mutex_init(&priv->lock);
usb_set_serial_port_data(port, priv);
--
1.9.1
The interrupt endpoint will report current IIR. If we got IIR with MSR changed
, We will do read MSR with interrupt_work worker to do f81232_read_msr()
function.
We also confirmd MSR strange delta value is not locking-issue. The issue
is set MCR & get MSR before IIR notice with MSR changed (Loopback only).
When we use RS232 loopback, assume doing RTS change will cause
CTS change, DTR change will cause DCD/DSR change too.
Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of
MSR(delta) maybe not changed when set & get MCR rapidly.
So we add more check not only UART_MSR_ANY_DELTA but also with
comparing DCD/RI/DSR/CTS change with old value. Because of the state
bit is always correct, we direct save msr when read.
The following step to reproduce this problem with while loop step 1~4:
1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 98 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index bf072fe..339be30 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define F81232_GET_REGISTER 0xc0
#define F81232_SET_REGISTER 0x40
+#define SERIAL_BASE_ADDRESS 0x0120
+#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
+
#define CONTROL_DTR 0x01
#define CONTROL_RTS 0x02
@@ -54,6 +57,8 @@ struct f81232_private {
struct mutex lock;
u8 line_control;
u8 modem_status;
+ struct work_struct interrupt_work;
+ struct usb_serial_port *port;
};
static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
@@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
kfree(tmp);
return status;
}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+ int status;
+ u8 current_msr, prev_msr;
+ u8 msr_mask = ~UART_MSR_ANY_DELTA;
+ u8 msr_changed_bit;
+ struct tty_struct *tty;
+ struct f81232_private *priv = usb_get_serial_port_data(port);
+
+ status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+ ¤t_msr);
+ if (status) {
+ dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
+ return;
+ }
+
+ /*
+ * The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
+ * maybe not change when set MCR & get MSR rapidly.
+ *
+ * So we add more check with comparing DCD/RI/DSR/CTS
+ * change. and direct save msr when read.
+ */
+
+ mutex_lock(&priv->lock);
+ prev_msr = priv->modem_status;
+ priv->modem_status = current_msr;
+ mutex_unlock(&priv->lock);
+
+ if (!(current_msr & UART_MSR_ANY_DELTA) &&
+ !((prev_msr ^ current_msr) & msr_mask))
+ return;
+
+ /* find checked delta bits set */
+ msr_changed_bit =
+ (current_msr & UART_MSR_ANY_DELTA) << 4;
+
+ /* append with not delta but changed bits */
+ msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
+
+ if (msr_changed_bit & UART_MSR_CTS)
+ port->icount.cts++;
+ if (msr_changed_bit & UART_MSR_DSR)
+ port->icount.dsr++;
+ if (msr_changed_bit & UART_MSR_RI)
+ port->icount.rng++;
+ if (msr_changed_bit & UART_MSR_DCD) {
+
+ port->icount.dcd++;
+ tty = tty_port_tty_get(&port->port);
+ if (tty) {
+
+ usb_serial_handle_dcd_change(port, tty,
+ current_msr & UART_MSR_DCD);
+
+ tty_kref_put(tty);
+ }
+ }
+
+ wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
static void f81232_update_line_status(struct usb_serial_port *port,
unsigned char *data,
- unsigned int actual_length)
+ size_t actual_length)
{
- /*
- * FIXME: Update port->icount, and call
- *
- * wake_up_interruptible(&port->port.delta_msr_wait);
- *
- * on MSR changes.
- */
+ struct f81232_private *priv = usb_get_serial_port_data(port);
+
+ if (!actual_length)
+ return;
+
+ switch (data[0] & 0x07) {
+ case 0x00: /* msr change */
+ dev_dbg(&port->dev, "IIR: MSR Change: %02x\n", data[0]);
+ schedule_work(&priv->interrupt_work);
+ break;
+ case 0x02: /* tx-empty */
+ break;
+ case 0x04: /* rx data available */
+ break;
+ case 0x06: /* lsr change */
+ /* we can forget it. the LSR will read from bulk-in */
+ dev_dbg(&port->dev, "IIR: LSR Change: %02x\n", data[0]);
+ break;
+ }
}
static void f81232_read_int_callback(struct urb *urb)
@@ -351,6 +431,14 @@ static int f81232_ioctl(struct tty_struct *tty,
return -ENOIOCTLCMD;
}
+static void f81232_interrupt_work(struct work_struct *work)
+{
+ struct f81232_private *priv =
+ container_of(work, struct f81232_private, interrupt_work);
+
+ f81232_read_msr(priv->port);
+}
+
static int f81232_port_probe(struct usb_serial_port *port)
{
struct f81232_private *priv;
@@ -360,10 +448,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
return -ENOMEM;
mutex_init(&priv->lock);
+ INIT_WORK(&priv->interrupt_work, f81232_interrupt_work);
usb_set_serial_port_data(port, priv);
port->port.drain_delay = 256;
+ priv->port = port;
return 0;
}
--
1.9.1
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 <[email protected]>
---
drivers/usb/serial/f81232.c | 103 ++++++++++++++++++++++++++++++++++----------
1 file changed, 81 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 339be30..21f606f 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define F81232_SET_REGISTER 0x40
#define SERIAL_BASE_ADDRESS 0x0120
+#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
#define CONTROL_DTR 0x01
@@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
struct f81232_private {
struct mutex lock;
- u8 line_control;
+ u8 modem_control;
u8 modem_status;
struct work_struct interrupt_work;
struct usb_serial_port *port;
@@ -198,6 +199,52 @@ static void f81232_read_msr(struct usb_serial_port *port)
wake_up_interruptible(&port->port.delta_msr_wait);
}
+static int f81232_set_mctrl(struct usb_serial_port *port,
+ unsigned int set, unsigned int clear)
+{
+ u8 urb_value;
+ int status;
+ struct f81232_private *priv = usb_get_serial_port_data(port);
+
+ if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
+ return 0; /* no change */
+
+ /* 'set' takes precedence over 'clear' */
+ clear &= ~set;
+
+ /* force enable interrupt with OUT2 */
+ mutex_lock(&priv->lock);
+ urb_value = UART_MCR_OUT2 | priv->modem_control;
+ mutex_unlock(&priv->lock);
+
+ if (clear & TIOCM_DTR)
+ urb_value &= ~UART_MCR_DTR;
+
+ if (clear & TIOCM_RTS)
+ urb_value &= ~UART_MCR_RTS;
+
+ if (set & TIOCM_DTR)
+ urb_value |= UART_MCR_DTR;
+
+ if (set & TIOCM_RTS)
+ urb_value |= UART_MCR_RTS;
+
+ dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
+ urb_value, priv->modem_control);
+
+ status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
+ if (status) {
+ dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
+ return status;
+ } else {
+ mutex_lock(&priv->lock);
+ priv->modem_control = urb_value;
+ mutex_unlock(&priv->lock);
+ }
+
+ return 0;
+}
+
static void f81232_update_line_status(struct usb_serial_port *port,
unsigned char *data,
size_t actual_length)
@@ -309,12 +356,6 @@ static void f81232_process_read_urb(struct urb *urb)
tty_flip_buffer_push(&port->port);
}
-static int set_control_lines(struct usb_device *dev, u8 value)
-{
- /* FIXME - Stubbed out for now */
- return 0;
-}
-
static void f81232_break_ctl(struct tty_struct *tty, int break_state)
{
/* FIXME - Stubbed out for now */
@@ -342,15 +383,35 @@ static void f81232_set_termios(struct tty_struct *tty,
static int f81232_tiocmget(struct tty_struct *tty)
{
- /* FIXME - Stubbed out for now */
- return 0;
+ int r;
+ struct usb_serial_port *port = tty->driver_data;
+ struct f81232_private *port_priv = usb_get_serial_port_data(port);
+ u8 mcr, msr;
+
+ /* force get current MSR changed state */
+ f81232_read_msr(port);
+
+ mutex_lock(&port_priv->lock);
+ mcr = port_priv->modem_control;
+ msr = port_priv->modem_status;
+ mutex_unlock(&port_priv->lock);
+
+ r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
+ (mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
+ (msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
+ (msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
+ (msr & UART_MSR_RI ? TIOCM_RI : 0) |
+ (msr & UART_MSR_DSR ? TIOCM_DSR : 0);
+
+ return r;
}
static int f81232_tiocmset(struct tty_struct *tty,
unsigned int set, unsigned int clear)
{
- /* FIXME - Stubbed out for now */
- return 0;
+ struct usb_serial_port *port = tty->driver_data;
+
+ return f81232_set_mctrl(port, set, clear);
}
static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
@@ -385,24 +446,22 @@ static void f81232_close(struct usb_serial_port *port)
static void f81232_dtr_rts(struct usb_serial_port *port, int on)
{
- struct f81232_private *priv = usb_get_serial_port_data(port);
- u8 control;
-
- mutex_lock(&priv->lock);
- /* Change DTR and RTS */
if (on)
- priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
+ f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
else
- priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
- control = priv->line_control;
- mutex_unlock(&priv->lock);
- set_control_lines(port->serial->dev, control);
+ f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
}
static int f81232_carrier_raised(struct usb_serial_port *port)
{
+ u8 msr;
struct f81232_private *priv = usb_get_serial_port_data(port);
- if (priv->modem_status & UART_DCD)
+
+ mutex_lock(&priv->lock);
+ msr = priv->modem_status;
+ mutex_unlock(&priv->lock);
+
+ if (msr & UART_MSR_DCD)
return 1;
return 0;
}
--
1.9.1
We put FCR/IER initial step to f81232_port_enable(). When port is open,
it set MSR interrupt on. Otherwise set it off.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 21f606f..f5c9060 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -37,6 +37,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define F81232_SET_REGISTER 0x40
#define SERIAL_BASE_ADDRESS 0x0120
+#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
@@ -367,6 +369,33 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
*/
}
+static int f81232_port_enable(struct usb_serial_port *port, int enable)
+{
+ u8 data = 0;
+ int status;
+
+ /* fifo on, trigger8, clear TX/RX*/
+ data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
+ | UART_FCR_CLEAR_XMIT;
+
+ status = f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
+ if (status) {
+ dev_err(&port->dev, "%s failed to set FCR: %d\n", __func__, status);
+ return status;
+ }
+
+ /* MSR Interrupt only, LSR will read from Bulk-in odd byte */
+ data = enable ? UART_IER_MSI : 0;
+
+ status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
+ if (status) {
+ dev_err(&port->dev, "%s failed to set IER: %d\n", __func__, status);
+ return status;
+ }
+
+ return 0;
+}
+
static void f81232_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
@@ -418,6 +447,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
{
int result;
+ result = f81232_port_enable(port, 1);
+ if (result) {
+ dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+ return result;
+ }
+
/* Setup termios */
if (tty)
f81232_set_termios(tty, port, NULL);
@@ -440,6 +475,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
static void f81232_close(struct usb_serial_port *port)
{
+ int result = f81232_port_enable(port, 0);
+ if (result)
+ dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
+
usb_serial_generic_close(port);
usb_kill_urb(port->interrupt_in_urb);
}
--
1.9.1
The original driver had do not any h/w change in driver.
This patch implements with configure H/W for
baud/parity/word length/stop bits functional in f81232_set_termios().
This patch also implement DTR/RTS control when baudrate B0.
We drop DTR/RTS when B0, otherwise enable it.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 102 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index f5c9060..0c96b9a 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = {
};
MODULE_DEVICE_TABLE(usb, id_table);
+/* Maximum baudrate for F81232 */
+#define F81232_MAX_BAUDRATE 115200
+
/* USB Control EP parameter */
#define F81232_REGISTER_REQUEST 0xA0
#define F81232_GET_REGISTER 0xc0
#define F81232_SET_REGISTER 0x40
#define SERIAL_BASE_ADDRESS 0x0120
+#define RECEIVE_BUFFER_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
@@ -64,6 +69,14 @@ struct f81232_private {
struct usb_serial_port *port;
};
+static int calc_baud_divisor(u32 baudrate)
+{
+ if (!baudrate)
+ return 0;
+ else
+ return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
+}
+
static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
{
int status;
@@ -369,6 +382,52 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
*/
}
+static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
+{
+ u8 divisor, lcr;
+ int status = 0;
+
+ if (!baudrate)
+ return;
+
+ divisor = calc_baud_divisor(baudrate);
+
+ status = f81232_get_register(port, LINE_CONTROL_REGISTER,
+ &lcr); /* get LCR */
+ if (status) {
+ dev_err(&port->dev, "%s failed to get LCR: %d\n",
+ __func__, status);
+ }
+
+ status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+ lcr | UART_LCR_DLAB); /* Enable DLAB */
+ if (status) {
+ dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+ __func__, status);
+ }
+
+ status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
+ divisor & 0x00ff); /* low */
+ if (status) {
+ dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
+ __func__, status);
+ }
+
+ status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
+ (divisor & 0xff00) >> 8); /* high */
+ if (status) {
+ dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",
+ __func__, status);
+ }
+
+ status = f81232_set_register(port, LINE_CONTROL_REGISTER,
+ lcr & ~UART_LCR_DLAB);
+ if (status) {
+ dev_err(&port->dev, "%s failed to set DLAB: %d\n",
+ __func__, status);
+ }
+}
+
static int f81232_port_enable(struct usb_serial_port *port, int enable)
{
u8 data = 0;
@@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable)
static void f81232_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
- /* FIXME - Stubbed out for now */
+ u8 new_lcr = 0;
+ int status = 0;
+
/* Don't change anything if nothing has changed */
if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
return;
- /* Do the real work here... */
- if (old_termios)
- tty_termios_copy_hw(&tty->termios, old_termios);
+ if (C_BAUD(tty) == B0)
+ f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
+ else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+ f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
+
+ f81232_set_baudrate(port, tty_get_baud_rate(tty));
+
+ if (C_PARENB(tty)) {
+ new_lcr |= UART_LCR_PARITY;
+
+ if (!C_PARODD(tty))
+ new_lcr |= UART_LCR_EPAR;
+
+ if (C_CMSPAR(tty))
+ new_lcr |= UART_LCR_SPAR;
+ }
+
+ if (C_CSTOPB(tty))
+ new_lcr |= UART_LCR_STOP;
+
+ switch (C_CSIZE(tty)) {
+ case CS5:
+ new_lcr |= UART_LCR_WLEN5;
+ break;
+ case CS6:
+ new_lcr |= UART_LCR_WLEN6;
+ break;
+ case CS7:
+ new_lcr |= UART_LCR_WLEN7;
+ break;
+ default:
+ case CS8:
+ new_lcr |= UART_LCR_WLEN8;
+ break;
+ }
+
+ status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
+ if (status)
+ dev_err(&port->dev, "%s failed to set LCR: %d\n", __func__, status);
+
}
static int f81232_tiocmget(struct tty_struct *tty)
--
1.9.1
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 0c96b9a..3b0da70 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -603,24 +603,32 @@ static int f81232_carrier_raised(struct usb_serial_port *port)
return 0;
}
+static int f81232_get_serial_info(struct usb_serial_port *port,
+ unsigned long arg)
+{
+ struct serial_struct ser;
+
+ memset(&ser, 0, sizeof(ser));
+
+ ser.type = PORT_16550A;
+ ser.line = port->minor;
+ ser.port = port->port_number;
+ ser.baud_base = 115200;
+
+ if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
+ return -EFAULT;
+
+ return 0;
+}
+
static int f81232_ioctl(struct tty_struct *tty,
unsigned int cmd, unsigned long arg)
{
- struct serial_struct ser;
struct usb_serial_port *port = tty->driver_data;
switch (cmd) {
case TIOCGSERIAL:
- memset(&ser, 0, sizeof ser);
- ser.type = PORT_16654;
- ser.line = port->minor;
- ser.port = port->port_number;
- ser.baud_base = 460800;
-
- if (copy_to_user((void __user *)arg, &ser, sizeof ser))
- return -EFAULT;
-
- return 0;
+ return f81232_get_serial_info(port, arg);
default:
break;
}
--
1.9.1
We remove non-used define in this patch to avoid wrong usage.
Signed-off-by: Peter Hung <[email protected]>
---
drivers/usb/serial/f81232.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 3b0da70..ee9a2a2 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -47,20 +47,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
-#define CONTROL_DTR 0x01
-#define CONTROL_RTS 0x02
-
-#define UART_STATE 0x08
-#define UART_STATE_TRANSIENT_MASK 0x74
-#define UART_DCD 0x01
-#define UART_DSR 0x02
-#define UART_BREAK_ERROR 0x04
-#define UART_RING 0x08
-#define UART_FRAME_ERROR 0x10
-#define UART_PARITY_ERROR 0x20
-#define UART_OVERRUN_ERROR 0x40
-#define UART_CTS 0x80
-
struct f81232_private {
struct mutex lock;
u8 modem_control;
--
1.9.1
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 ee9a2a2..23f0a17 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -691,5 +691,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
module_usb_serial_driver(serial_drivers, id_table);
MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
-MODULE_AUTHOR("Greg Kroah-Hartman <[email protected]");
+MODULE_AUTHOR("Greg Kroah-Hartman <[email protected]>");
+MODULE_AUTHOR("Peter Hong <[email protected]>");
MODULE_LICENSE("GPL v2");
--
1.9.1
On Tue, Feb 24, 2015 at 06:16:17PM +0800, Peter Hung wrote:
> We add f81232_get_register()/f81232_set_register() and necessary defines
> for future patch with communication with F81232 USB control endpoint.
>
> Because of this is a preparatory patch, there are unused function warning
> with compiling. The functions will used with following patches.
Ok, adding these separately facilitates review, but please add them in
the patches that first use them after addressing the comments below (to
avoid those build warning). Sorry about the extra work.
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..1f29b95 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -30,6 +30,11 @@ static const struct usb_device_id id_table[] = {
> };
> MODULE_DEVICE_TABLE(usb, id_table);
>
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST 0xA0
> +#define F81232_GET_REGISTER 0xc0
> +#define F81232_SET_REGISTER 0x40
> +
> #define CONTROL_DTR 0x01
> #define CONTROL_RTS 0x02
>
> @@ -50,6 +55,80 @@ struct f81232_private {
> u8 modem_status;
> };
>
> +static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> +{
Please rename "data" to "val" here (and in set_register).
> + int status;
> + u8 *tmp;
> + struct usb_device *dev = port->serial->dev;
> +
> + if (!data)
> + return -EFAULT;
No need to test for NULL here.
> +
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
Use sizeof(*val) throughout.
> +
> + status = usb_control_msg(dev,
> + usb_rcvctrlpipe(dev, 0),
> + F81232_REGISTER_REQUEST,
> + F81232_GET_REGISTER,
> + reg,
> + 0,
> + tmp,
> + sizeof(u8),
> + USB_CTRL_GET_TIMEOUT);
> + if (status <= 0) {
Test for status != sizeof(*val) here.
> + /* show something with failed */
Drop the comment.
> + dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> +
> + if (status == 0)
> + status = -EIO;
> + else
> + status = usb_translate_errors(status);
> + } else {
> + status = 0; /* on success */
Comment not needed.
> + memcpy((void*) data, (void*) tmp, sizeof(u8));
Just do
*val = *tmp;
here.
> + }
> +
> + kfree(tmp);
> + return status;
> +}
> +
> +static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
Use val instead of data here as well.
> +{
> + int status;
> + u8 *tmp;
> + struct usb_device *dev = port->serial->dev;
> +
> + tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + memcpy((void*) tmp, (void*) &data, sizeof(u8));
*tmp = *val
> +
> + status = usb_control_msg(dev,
> + usb_sndctrlpipe(dev, 0),
> + F81232_REGISTER_REQUEST,
> + F81232_SET_REGISTER,
> + reg,
> + 0,
> + tmp,
> + sizeof(u8),
> + USB_CTRL_SET_TIMEOUT);
> + if (status <= 0) {
> + /* show something with failed */
> + dev_err(&port->dev, "%s failed status: %d\n", __func__, status);
> +
> + if (status == 0)
> + status = -EIO;
> + else
> + status = usb_translate_errors(status);
> + } else
> + status = 0; /* on success */
Missing brackets on else branch. No need for comment.
> +
> + kfree(tmp);
> + return status;
> +}
Most comments to get_register apply here as well.
> static void f81232_update_line_status(struct usb_serial_port *port,
> unsigned char *data,
> unsigned int actual_length)
Thanks,
Johan
On Tue, Feb 24, 2015 at 06:16:18PM +0800, Peter Hung wrote:
> The F81232 bulk-in is RX data + LSR channel, data format is
> [LSR+Data][LSR+Data]..... , We had implemented in f81232_process_read_urb().
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 69 +++++++++++++++++++++++----------------------
> 1 file changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 1f29b95..419e2d6 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,6 +23,7 @@
> #include <linux/uaccess.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
>
> static const struct usb_device_id id_table[] = {
> { USB_DEVICE(0x1934, 0x0706) },
> @@ -183,44 +184,46 @@ exit:
> static void f81232_process_read_urb(struct urb *urb)
> {
> struct usb_serial_port *port = urb->context;
> - struct f81232_private *priv = usb_get_serial_port_data(port);
> unsigned char *data = urb->transfer_buffer;
> - char tty_flag = TTY_NORMAL;
> - unsigned long flags;
> - u8 line_status;
> + char tty_flag;
> int i;
Use an unsigned type for i.
> + u8 lsr;
>
> - /* update line status */
> - spin_lock_irqsave(&priv->lock, flags);
> - line_status = priv->modem_status;
> - priv->modem_status &= ~UART_STATE_TRANSIENT_MASK;
> - spin_unlock_irqrestore(&priv->lock, flags);
> -
> - if (!urb->actual_length)
> + if (urb->actual_length < 2)
> return;
>
> - /* break takes precedence over parity, */
> - /* which takes precedence over framing errors */
> - if (line_status & UART_BREAK_ERROR)
> - tty_flag = TTY_BREAK;
> - else if (line_status & UART_PARITY_ERROR)
> - tty_flag = TTY_PARITY;
> - else if (line_status & UART_FRAME_ERROR)
> - tty_flag = TTY_FRAME;
> - dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> -
> - /* overrun is special, not associated with a char */
> - if (line_status & UART_OVERRUN_ERROR)
> - tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> -
> - if (port->port.console && port->sysrq) {
> - for (i = 0; i < urb->actual_length; ++i)
> - if (!usb_serial_handle_sysrq_char(port, data[i]))
> - tty_insert_flip_char(&port->port, data[i],
> - tty_flag);
> - } else {
> - tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
> - urb->actual_length);
> + /* bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]... */
> +
> + for (i = 0; i < urb->actual_length; i += 2) {
Again, what if the length is not a multiple of 2?
> + tty_flag = TTY_NORMAL;
> + lsr = data[i + 0];
Just use data[i] here.
> +
> + if (lsr & UART_LSR_BRK_ERROR_BITS) {
> + if (lsr & UART_LSR_BI) {
> + tty_flag = TTY_BREAK;
> + port->icount.brk++;
> + usb_serial_handle_break(port);
> + } else if (lsr & UART_LSR_PE) {
> + tty_flag = TTY_PARITY;
> + port->icount.parity++;
> + } else if (lsr & UART_LSR_FE) {
> + tty_flag = TTY_FRAME;
> + port->icount.frame++;
> + }
> +
> + if (lsr & UART_LSR_OE) {
> + port->icount.overrun++;
> + tty_insert_flip_char(&port->port, 0,
> + TTY_OVERRUN);
Use at least two tabs indentation for continuation lines.
> + }
> + }
> +
> + if (port->port.console && port->sysrq) {
> + if (usb_serial_handle_sysrq_char(port, data[i + 1]))
> + continue;
> + }
> +
> + tty_insert_flip_char(&port->port, data[i + 1], tty_flag);
> }
>
> tty_flip_buffer_push(&port->port);
Looks good now, otherwise.
Thanks,
Johan
On Tue, Feb 24, 2015 at 06:16:20PM +0800, Peter Hung wrote:
> The interrupt endpoint will report current IIR. If we got IIR with MSR changed
> , We will do read MSR with interrupt_work worker to do f81232_read_msr()
> function.
>
> We also confirmd MSR strange delta value is not locking-issue. The issue
> is set MCR & get MSR before IIR notice with MSR changed (Loopback only).
But you are still not doing the actual register access under the mutex,
so this still sounds like it could be due to the shadow registers not
matching the hardware registers.
You need to update dtr_rts as well.
> When we use RS232 loopback, assume doing RTS change will cause
> CTS change, DTR change will cause DCD/DSR change too.
>
> Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of
> MSR(delta) maybe not changed when set & get MCR rapidly.
>
> So we add more check not only UART_MSR_ANY_DELTA but also with
> comparing DCD/RI/DSR/CTS change with old value. Because of the state
> bit is always correct, we direct save msr when read.
>
> The following step to reproduce this problem with while loop step 1~4:
> 1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
> 2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
> 3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
> 4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 98 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index bf072fe..339be30 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define F81232_GET_REGISTER 0xc0
> #define F81232_SET_REGISTER 0x40
>
> +#define SERIAL_BASE_ADDRESS 0x0120
> +#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
> +
> #define CONTROL_DTR 0x01
> #define CONTROL_RTS 0x02
>
> @@ -54,6 +57,8 @@ struct f81232_private {
> struct mutex lock;
> u8 line_control;
> u8 modem_status;
> + struct work_struct interrupt_work;
> + struct usb_serial_port *port;
> };
>
> static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> @@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
> kfree(tmp);
> return status;
> }
> +
> +static void f81232_read_msr(struct usb_serial_port *port)
> +{
> + int status;
> + u8 current_msr, prev_msr;
> + u8 msr_mask = ~UART_MSR_ANY_DELTA;
> + u8 msr_changed_bit;
> + struct tty_struct *tty;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + status = f81232_get_register(port, MODEM_STATUS_REGISTER,
> + ¤t_msr);
> + if (status) {
> + dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
> + return;
> + }
> +
> + /*
> + * The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
> + * maybe not change when set MCR & get MSR rapidly.
> + *
> + * So we add more check with comparing DCD/RI/DSR/CTS
> + * change. and direct save msr when read.
> + */
The asterisks should be aligned.
> +
> + mutex_lock(&priv->lock);
> + prev_msr = priv->modem_status;
> + priv->modem_status = current_msr;
> + mutex_unlock(&priv->lock);
> +
> + if (!(current_msr & UART_MSR_ANY_DELTA) &&
> + !((prev_msr ^ current_msr) & msr_mask))
> + return;
> +
> + /* find checked delta bits set */
> + msr_changed_bit =
> + (current_msr & UART_MSR_ANY_DELTA) << 4;
> +
> + /* append with not delta but changed bits */
> + msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
> +
> + if (msr_changed_bit & UART_MSR_CTS)
> + port->icount.cts++;
> + if (msr_changed_bit & UART_MSR_DSR)
> + port->icount.dsr++;
> + if (msr_changed_bit & UART_MSR_RI)
> + port->icount.rng++;
> + if (msr_changed_bit & UART_MSR_DCD) {
> +
No empty line.
> + port->icount.dcd++;
> + tty = tty_port_tty_get(&port->port);
> + if (tty) {
> +
No empty line.
> + usb_serial_handle_dcd_change(port, tty,
> + current_msr & UART_MSR_DCD);
> +
> + tty_kref_put(tty);
> + }
> + }
> +
> + wake_up_interruptible(&port->port.delta_msr_wait);
> +}
Johan
On Tue, Feb 24, 2015 at 06:16:21PM +0800, Peter Hung wrote:
> This patch implement relative MCR/MSR function, such like
> tiocmget()/tiocmset()/dtr_rts()/carrier_raised()
>
> original f81232_carrier_raised() compared with wrong value UART_DCD.
> It's should compared with UART_MSR_DCD.
>
> Signed-off-by: Peter Hung <[email protected]>
This patch as well as a few of the other patches in v7 now have
checkpatch warnings and errors.
> ---
> drivers/usb/serial/f81232.c | 103 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 81 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 339be30..21f606f 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define F81232_SET_REGISTER 0x40
>
> #define SERIAL_BASE_ADDRESS 0x0120
> +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> #define CONTROL_DTR 0x01
> @@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>
> struct f81232_private {
> struct mutex lock;
> - u8 line_control;
> + u8 modem_control;
> u8 modem_status;
> struct work_struct interrupt_work;
> struct usb_serial_port *port;
> @@ -198,6 +199,52 @@ static void f81232_read_msr(struct usb_serial_port *port)
> wake_up_interruptible(&port->port.delta_msr_wait);
> }
>
> +static int f81232_set_mctrl(struct usb_serial_port *port,
> + unsigned int set, unsigned int clear)
> +{
> + u8 urb_value;
> + int status;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
> + return 0; /* no change */
> +
> + /* 'set' takes precedence over 'clear' */
> + clear &= ~set;
> +
> + /* force enable interrupt with OUT2 */
> + mutex_lock(&priv->lock);
> + urb_value = UART_MCR_OUT2 | priv->modem_control;
> + mutex_unlock(&priv->lock);
So this is one of the places where the port mute should protect the
whole operation.
> +
> + if (clear & TIOCM_DTR)
> + urb_value &= ~UART_MCR_DTR;
> +
> + if (clear & TIOCM_RTS)
> + urb_value &= ~UART_MCR_RTS;
> +
> + if (set & TIOCM_DTR)
> + urb_value |= UART_MCR_DTR;
> +
> + if (set & TIOCM_RTS)
> + urb_value |= UART_MCR_RTS;
> +
> + dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
> + urb_value, priv->modem_control);
> +
> + status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
> + if (status) {
> + dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
> + return status;
> + } else {
> + mutex_lock(&priv->lock);
> + priv->modem_control = urb_value;
> + mutex_unlock(&priv->lock);
> + }
No need for else-branch as you return on errors.
> +
> + return 0;
> +}
Johan
On Tue, Feb 24, 2015 at 06:16:22PM +0800, Peter Hung wrote:
> We put FCR/IER initial step to f81232_port_enable(). When port is open,
> it set MSR interrupt on. Otherwise set it off.
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 39 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 21f606f..f5c9060 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -37,6 +37,8 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define F81232_SET_REGISTER 0x40
>
> #define SERIAL_BASE_ADDRESS 0x0120
> +#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
> #define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> @@ -367,6 +369,33 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> */
> }
>
> +static int f81232_port_enable(struct usb_serial_port *port, int enable)
Split this in two functions, enable and disable, since you don't need to
set FCR on close.
> +{
> + u8 data = 0;
> + int status;
> +
> + /* fifo on, trigger8, clear TX/RX*/
> + data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
> + | UART_FCR_CLEAR_XMIT;
Put the final | before the line break.
> +
> + status = f81232_set_register(port, FIFO_CONTROL_REGISTER, data);
> + if (status) {
> + dev_err(&port->dev, "%s failed to set FCR: %d\n", __func__, status);
> + return status;
> + }
> +
> + /* MSR Interrupt only, LSR will read from Bulk-in odd byte */
> + data = enable ? UART_IER_MSI : 0;
> +
> + status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);
> + if (status) {
> + dev_err(&port->dev, "%s failed to set IER: %d\n", __func__, status);
> + return status;
> + }
> +
> + return 0;
> +}
> +
> static void f81232_set_termios(struct tty_struct *tty,
> struct usb_serial_port *port, struct ktermios *old_termios)
> {
> @@ -418,6 +447,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
> {
> int result;
>
> + result = f81232_port_enable(port, 1);
> + if (result) {
> + dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
> + return result;
You already log the error in port_enable.
> + }
> +
> /* Setup termios */
> if (tty)
> f81232_set_termios(tty, port, NULL);
> @@ -440,6 +475,10 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>
> static void f81232_close(struct usb_serial_port *port)
> {
> + int result = f81232_port_enable(port, 0);
Separate the declaration from the function call.
> + if (result)
> + dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
> +
Already logged as well.
> usb_serial_generic_close(port);
> usb_kill_urb(port->interrupt_in_urb);
> }
Johan
On Tue, Feb 24, 2015 at 06:16:23PM +0800, Peter Hung wrote:
> The original driver had do not any h/w change in driver.
> This patch implements with configure H/W for
> baud/parity/word length/stop bits functional in f81232_set_termios().
>
> This patch also implement DTR/RTS control when baudrate B0.
> We drop DTR/RTS when B0, otherwise enable it.
>
> Signed-off-by: Peter Hung <[email protected]>
> ---
> drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 102 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index f5c9060..0c96b9a 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = {
> };
> MODULE_DEVICE_TABLE(usb, id_table);
>
> +/* Maximum baudrate for F81232 */
> +#define F81232_MAX_BAUDRATE 115200
> +
> /* USB Control EP parameter */
> #define F81232_REGISTER_REQUEST 0xA0
> #define F81232_GET_REGISTER 0xc0
> #define F81232_SET_REGISTER 0x40
>
> #define SERIAL_BASE_ADDRESS 0x0120
> +#define RECEIVE_BUFFER_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
> #define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
> #define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
> #define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> @@ -64,6 +69,14 @@ struct f81232_private {
> struct usb_serial_port *port;
> };
>
> +static int calc_baud_divisor(u32 baudrate)
> +{
> + if (!baudrate)
> + return 0;
Check no longer needed since you added the check at the call site. Just
add a comment. In fact it look like you can get rid of this function
now.
> + else
> + return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
> +}
> +
> static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> {
> int status;
> @@ -369,6 +382,52 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> */
> }
>
> +static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
> +{
> + u8 divisor, lcr;
> + int status = 0;
> +
> + if (!baudrate)
> + return;
> +
> + divisor = calc_baud_divisor(baudrate);
> +
> + status = f81232_get_register(port, LINE_CONTROL_REGISTER,
> + &lcr); /* get LCR */
> + if (status) {
> + dev_err(&port->dev, "%s failed to get LCR: %d\n",
> + __func__, status);
> + }
> +
> + status = f81232_set_register(port, LINE_CONTROL_REGISTER,
> + lcr | UART_LCR_DLAB); /* Enable DLAB */
> + if (status) {
> + dev_err(&port->dev, "%s failed to set DLAB: %d\n",
> + __func__, status);
> + }
> +
> + status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
> + divisor & 0x00ff); /* low */
> + if (status) {
> + dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n",
> + __func__, status);
> + }
> +
> + status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
> + (divisor & 0xff00) >> 8); /* high */
> + if (status) {
> + dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n",
> + __func__, status);
> + }
> +
> + status = f81232_set_register(port, LINE_CONTROL_REGISTER,
> + lcr & ~UART_LCR_DLAB);
> + if (status) {
> + dev_err(&port->dev, "%s failed to set DLAB: %d\n",
> + __func__, status);
> + }
> +}
Again, what about the error handling? No need to continue setting the
other registers should the first one fail, right? And you always want to
restore LCR to its previous value.
> +
> static int f81232_port_enable(struct usb_serial_port *port, int enable)
> {
> u8 data = 0;
> @@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable)
> static void f81232_set_termios(struct tty_struct *tty,
> struct usb_serial_port *port, struct ktermios *old_termios)
> {
> - /* FIXME - Stubbed out for now */
> + u8 new_lcr = 0;
> + int status = 0;
> +
>
> /* Don't change anything if nothing has changed */
> if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> return;
>
> - /* Do the real work here... */
> - if (old_termios)
> - tty_termios_copy_hw(&tty->termios, old_termios);
> + if (C_BAUD(tty) == B0)
> + f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
> + else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
> + f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
> +
> + f81232_set_baudrate(port, tty_get_baud_rate(tty));
Check for B0 here instead of in the helper.
And what about unsupported baudrates (e.g. > 115200 baud)? You should
return the baudrate actually used in that case using
tty_termios_encode_baud_rate.
Johan