From: Johan Hovold <johan@kernel.org>
To: Peter Hung <hpeter@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw,
Peter Hung <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH 1/1] usb: serial: Fintek F81232 driver improvement
Date: Mon, 19 Jan 2015 12:29:04 +0100 [thread overview]
Message-ID: <20150119112904.GQ30960@localhost> (raw)
In-Reply-To: <1421632495-26260-1-git-send-email-hpeter+linux_kernel@gmail.com>
On Mon, Jan 19, 2015 at 09:54:55AM +0800, Peter Hung wrote:
> The original driver completed with TX function, but RX/MSR/MCR/LSR is not
> workable with this driver. So we rewrite it to make this device workable.
>
> This patch is tested with PassMark BurnInTest with Cycle-to-115200 +
> MCR/MSR check for 15mins & checked with Suspend-To-RAM/DISK
>
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
Thanks for the patch.
You need to break these changes up into several patches adding the
various features and submit it as a series. The rule of thumb is one
self-contained, logical change per patch (e.g. "fix x", "refactor y",
"add function z").
A few initial comments follow inline below.
> ---
> drivers/usb/serial/f81232.c | 528 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 440 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index c5dc233..5ae6bc9 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,9 +23,16 @@
> #include <linux/uaccess.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/version.h>
> +
> +#define FINTEK_MAGIC 'F'
> +#define FINTEK_GET_ID _IOR(FINTEK_MAGIC, 3, int)
Adding new ioctls is hardly ever accepted, and definitely not for
retrieving static information that is already provided through sysfs
(idVendor, idProduct).
> +#define FINTEK_VENDOR_ID 0x1934
> +#define FINTEK_DEVICE_ID 0x0706 /* RS232 1 port */
>
> static const struct usb_device_id id_table[] = {
> - { USB_DEVICE(0x1934, 0x0706) },
> + { USB_DEVICE(FINTEK_VENDOR_ID, FINTEK_DEVICE_ID) },
So just drop these changes.
> { } /* Terminating entry */
> };
> MODULE_DEVICE_TABLE(usb, id_table);
> @@ -37,30 +44,257 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define UART_STATE_TRANSIENT_MASK 0x74
> #define UART_DCD 0x01
> #define UART_DSR 0x02
> -#define UART_BREAK_ERROR 0x04
> #define UART_RING 0x08
> -#define UART_FRAME_ERROR 0x10
> -#define UART_PARITY_ERROR 0x20
> -#define UART_OVERRUN_ERROR 0x40
> #define UART_CTS 0x80
>
> +
> +#define UART_BREAK_ERROR 0x10
> +#define UART_FRAME_ERROR 0x08
> +#define UART_PARITY_ERROR 0x04
> +#define UART_OVERRUN_ERROR 0x02
> +
> +
> +#define SERIAL_EVEN_PARITY (UART_LCR_PARITY | UART_LCR_EPAR)
> +
> +
> +#define REGISTER_REQUEST 0xA0
> +#define F81232_USB_TIMEOUT 1000
> +#define F81232_USB_RETRY 20
> +
> +
> +#define SERIAL_BASE_ADDRESS ((__u16)0x0120)
> +#define RECEIVE_BUFFER_REGISTER ((__u16)(0x00) + SERIAL_BASE_ADDRESS)
> +#define TRANSMIT_HOLDING_REGISTER ((__u16)(0x00) + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_ENABLE_REGISTER ((__u16)(0x01) + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_IDENT_REGISTER ((__u16)(0x02) + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER ((__u16)(0x02) + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER ((__u16)(0x03) + SERIAL_BASE_ADDRESS)
> +#define MODEM_CONTROL_REGISTER ((__u16)(0x04) + SERIAL_BASE_ADDRESS)
> +#define LINE_STATUS_REGISTER ((__u16)(0x05) + SERIAL_BASE_ADDRESS)
> +#define MODEM_STATUS_REGISTER ((__u16)(0x06) + SERIAL_BASE_ADDRESS)
No need for casts.
> +
> +static int m_enable_debug;
> +
> +module_param(m_enable_debug, int, S_IRUGO);
> +MODULE_PARM_DESC(m_enable_debug, "Debugging mode enabled or not");
Don't add module parameters, use dynamic debugging.
> +
> +#define LOG_MESSAGE(x, y, ...) \
> + printk(x y, ##__VA_ARGS__)
> +
> +#define LOG_DEBUG_MESSAGE(level, y, ...) \
> + do { if (unlikely(m_enable_debug)) \
> + printk(level y, ##__VA_ARGS__); } while (0)
Don't add your own debug macros, use dev_dbg.
> +
> +
> struct f81232_private {
> spinlock_t lock;
> - u8 line_control;
> - u8 line_status;
> + u8 modem_control;
> + u8 modem_status;
> + struct usb_device *dev;
> +
> + struct work_struct int_worker;
> + struct usb_serial_port *port;
> };
>
> -static void f81232_update_line_status(struct usb_serial_port *port,
> - unsigned char *data,
> - unsigned int actual_length)
> +
> +static inline int calc_baud_divisor(u32 baudrate)
> {
> - /*
> - * FIXME: Update port->icount, and call
> - *
> - * wake_up_interruptible(&port->port.delta_msr_wait);
> - *
> - * on MSR changes.
> - */
> + u32 divisor, rem;
> +
> + divisor = 115200L / baudrate;
> + rem = 115200L % baudrate;
> +
> + /* Round to nearest divisor */
> + if (((rem * 2) >= baudrate) && (baudrate != 110))
> + divisor++;
> +
> + return divisor;
> +}
> +
> +
> +static inline int f81232_get_register(struct usb_device *dev,
> + u16 reg, u8 *data)
> +{
> + int status;
> + int i = 0;
> +timeout_get_repeat:
> +
> + status = usb_control_msg(dev,
> + usb_rcvctrlpipe(dev, 0),
> + REGISTER_REQUEST,
> + 0xc0,
Avoid magic constants, use defines with descriptive names.
> + reg,
> + 0,
> + data,
> + sizeof(*data),
> + F81232_USB_TIMEOUT);
> + if (status < 0) {
> + i++;
> +
> + if (i < F81232_USB_RETRY) {
> + mdelay(1);
> + goto timeout_get_repeat;
Why do you need to retry? You should probably just fail, otherwise
implement this a proper loop.
> + }
> + }
> + return status;
> +}
> +
> +
> +static inline int f81232_set_register(struct usb_device *dev,
> + u16 reg, u8 data)
> +{
> + int status;
> + int i = 0;
> +
> +timeout_set_repeat:
> + status = 0;
> +
> + status = usb_control_msg(dev,
> + usb_sndctrlpipe(dev, 0),
> + REGISTER_REQUEST,
> + 0x40,
> + reg,
> + 0,
> + &data,
> + 1,
> + F81232_USB_TIMEOUT);
> +
> + if (status < 0) {
> + i++;
> + if (i < F81232_USB_RETRY) {
> + mdelay(1);
> + goto timeout_set_repeat;
Same as above.
> + }
> + }
> +
> + return status;
> +}
[...]
> -static void f81232_process_read_urb(struct urb *urb)
> +static void f81232_read_bulk_callback(struct urb *urb)
Why are you renaming this function (hint: you shouldn't).
> {
> struct usb_serial_port *port = urb->context;
> - struct f81232_private *priv = usb_get_serial_port_data(port);
> unsigned char *data = urb->transfer_buffer;
> char tty_flag = TTY_NORMAL;
> - unsigned long flags;
> - u8 line_status;
> + u8 line_status = 0;
> int i;
>
> - /* update line status */
> - spin_lock_irqsave(&priv->lock, flags);
> - line_status = priv->line_status;
> - priv->line_status &= ~UART_STATE_TRANSIENT_MASK;
> - spin_unlock_irqrestore(&priv->lock, flags);
>
> if (!urb->actual_length)
> return;
>
> /* break takes precedence over parity, */
> /* which takes precedence over framing errors */
> +
> +#if 0
> if (line_status & UART_BREAK_ERROR)
> tty_flag = TTY_BREAK;
> else if (line_status & UART_PARITY_ERROR)
> @@ -129,28 +358,22 @@ static void f81232_process_read_urb(struct urb *urb)
> else if (line_status & UART_FRAME_ERROR)
> tty_flag = TTY_FRAME;
> dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> +#endif
Either remove or fix code, don't keep it unless used.
> - /* overrun is special, not associated with a char */
> - if (line_status & UART_OVERRUN_ERROR)
> - tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> + if (urb->actual_length >= 2) {
>
> - 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);
> - }
> + for (i = 0 ; i < urb->actual_length ; i += 2) {
> + line_status |= data[i+0];
> + tty_insert_flip_string_fixed_flag(&port->port,
> + &data[i+1], tty_flag, 1);
> + }
>
> - tty_flip_buffer_push(&port->port);
> -}
> + if (unlikely(line_status & UART_OVERRUN_ERROR))
> + tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> +
> + 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)
> @@ -165,30 +388,117 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> }
>
> static void f81232_set_termios(struct tty_struct *tty,
> - struct usb_serial_port *port, struct ktermios *old_termios)
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> {
> - /* FIXME - Stubbed out for now */
> + u16 divisor;
> + u16 new_lcr = 0;
> +/*
> +The following comment is for legacy 3.7.0- kernel, You
> +can uncomment and build it if toy need
> +*/
>
> - /* Don't change anything if nothing has changed */
> - if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> - return;
> +/*
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 7, 0)
> + struct ktermios *termios = &tty->termios;
> +#else
> + struct ktermios *termios = tty->termios;
> +#endif
> +*/
We don't want this. Don't use conditional compilation, and especially
not support older kernels like this.
> + struct ktermios *termios = &tty->termios;
> +
> + unsigned int cflag = termios->c_cflag;
> + int status;
> +
> + struct usb_device *dev = port->serial->dev;
> +
> + divisor = calc_baud_divisor(tty_get_baud_rate(tty));
> +
> + status = f81232_set_register(dev, LINE_CONTROL_REGISTER,
> + UART_LCR_DLAB); /* DLAB */
> + mdelay(1);
Why are you adding these delays?
> + status = f81232_set_register(dev, RECEIVE_BUFFER_REGISTER,
> + divisor & 0x00ff); /* low */
> + mdelay(1);
> + status = f81232_set_register(dev, INTERRUPT_ENABLE_REGISTER,
> + (divisor & 0xff00) >> 8); /* high */
> + mdelay(1);
> + status = f81232_set_register(dev, LINE_CONTROL_REGISTER, 0x00);
> + mdelay(1);
> +
> + if (cflag & PARENB) {
> + if (cflag & PARODD)
> + new_lcr |= UART_LCR_PARITY; /* odd */
> + else
> + new_lcr |= SERIAL_EVEN_PARITY; /* even */
> + }
> +
> +
> + if (cflag & CSTOPB)
> + new_lcr |= UART_LCR_STOP;
> + else
> + new_lcr &= ~UART_LCR_STOP;
> +
> + switch (cflag & CSIZE) {
> + case CS5:
> + new_lcr |= UART_LCR_WLEN5;
> + break;
> + case CS6:
> + new_lcr |= UART_LCR_WLEN6;
> + break;
> + case CS7:
> + new_lcr |= UART_LCR_WLEN7;
> + break;
> + default:
> + case CS8:
> + new_lcr |= UART_LCR_WLEN8;
> + break;
> + }
> +
> + status |= f81232_set_register(dev, LINE_CONTROL_REGISTER, new_lcr);
> +
> + status |= f81232_set_register(dev, FIFO_CONTROL_REGISTER,
> + 0x87); /* fifo, trigger8 */
> + status |= f81232_set_register(dev,
> + INTERRUPT_ENABLE_REGISTER, 0xf); /* IER */
> +
> + if (status < 0) {
> + LOG_MESSAGE(KERN_INFO,
> + "[Fintek]: LINE_CONTROL_REGISTER set error: %d\n"
> + , status);
> + }
>
> - /* Do the real work here... */
> - if (old_termios)
> - tty_termios_copy_hw(&tty->termios, old_termios);
> }
>
> static int f81232_tiocmget(struct tty_struct *tty)
> {
> - /* 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;
> +
> + LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_tiocmget in\n");
> + spin_lock_irqsave(&port_priv->lock, flags);
> + r = (port_priv->modem_control & UART_MCR_DTR ? TIOCM_DTR : 0) |
> + (port_priv->modem_control & UART_MCR_RTS ? TIOCM_RTS : 0) |
> + (port_priv->modem_status & UART_MSR_CTS ? TIOCM_CTS : 0) |
> + (port_priv->modem_status & UART_MSR_DCD ? TIOCM_CAR : 0) |
> + (port_priv->modem_status & UART_MSR_RI ? TIOCM_RI : 0) |
> + (port_priv->modem_status & UART_MSR_DSR ? TIOCM_DSR : 0);
> + spin_unlock_irqrestore(&port_priv->lock, flags);
Use a temporary variable for the status.
> + LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_tiocmget out\n");
> + return r;
> }
>
> static int f81232_tiocmset(struct tty_struct *tty,
> - unsigned int set, unsigned int clear)
> + unsigned int set,
> + unsigned int clear)
> {
> - /* FIXME - Stubbed out for now */
> - return 0;
> + struct usb_serial_port *port = tty->driver_data;
> + struct f81232_private *port_priv =
> + usb_get_serial_port_data(port);
> +
> + return update_mctrl(port_priv, set, clear);
> }
>
> static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
> @@ -201,12 +511,14 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>
> result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
> if (result) {
> - dev_err(&port->dev, "%s - failed submitting interrupt urb,"
> - " error %d\n", __func__, result);
> + dev_err(&port->dev,
> + "%s - failed submitting interrupt urb, error %d\n"
> + , __func__, result);
Fix this separately as well.
> return result;
> }
>
> result = usb_serial_generic_open(tty, port);
> +
Don't do random whitespace changes (here or elsewhere).
> if (result) {
> usb_kill_urb(port->interrupt_in_urb);
> return result;
> @@ -217,6 +529,7 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>
> static void f81232_close(struct usb_serial_port *port)
> {
> +
> usb_serial_generic_close(port);
> usb_kill_urb(port->interrupt_in_urb);
> }
> @@ -224,52 +537,89 @@ static void f81232_close(struct usb_serial_port *port)
> static void f81232_dtr_rts(struct usb_serial_port *port, int on)
> {
> struct f81232_private *priv = usb_get_serial_port_data(port);
> - unsigned long flags;
> - u8 control;
>
> - spin_lock_irqsave(&priv->lock, flags);
> - /* Change DTR and RTS */
> if (on)
> - priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
> + update_mctrl(priv, TIOCM_DTR | TIOCM_RTS, 0);
> else
> - priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
> - control = priv->line_control;
> - spin_unlock_irqrestore(&priv->lock, flags);
> - set_control_lines(port->serial->dev, control);
> + update_mctrl(priv, 0, TIOCM_DTR | TIOCM_RTS);
> }
>
> static int f81232_carrier_raised(struct usb_serial_port *port)
> {
> struct f81232_private *priv = usb_get_serial_port_data(port);
> - if (priv->line_status & UART_DCD)
> + unsigned long flags;
> + int modem_status;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + modem_status = priv->modem_status;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + if (modem_status & UART_DCD)
> return 1;
> return 0;
> }
>
> +static int f81232_get_id(struct usb_serial_port *port, int __user *arg)
> +{
> + /* 0x19340706 */
> + int data = (FINTEK_VENDOR_ID << 16) | FINTEK_DEVICE_ID;
> +
> + if (copy_to_user((int __user *) arg, &data, sizeof(int)))
> + return -EFAULT;
> +
> + return 0;
> +}
So drop this.
> +
> +
> static int f81232_ioctl(struct tty_struct *tty,
> - unsigned int cmd, unsigned long arg)
> + 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;
> + memset(&ser, 0, sizeof(ser));
> + ser.flags = ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
> + ser.xmit_fifo_size = port->bulk_out_size;
> + ser.close_delay = 5*HZ;
> + ser.closing_wait = 30*HZ;
> +
> + ser.type = PORT_16550A;
> ser.line = port->minor;
> ser.port = port->port_number;
> - ser.baud_base = 460800;
> + ser.baud_base = 115200;
>
> - if (copy_to_user((void __user *)arg, &ser, sizeof ser))
> + if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
> return -EFAULT;
>
> return 0;
> +
> + case FINTEK_GET_ID:
> + return f81232_get_id(port, (int __user *)arg);
> +
> default:
> break;
> }
> return -ENOIOCTLCMD;
> }
>
> +
> +
> +
> +static void f81232_int_work_wq(struct work_struct *work)
> +{
> + struct f81232_private *priv =
> + container_of(work, struct f81232_private, int_worker);
> +
> +
> + LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_int_work_wq\n");
> + f81232_read_msr(priv);
> +
> +
> +}
> +
> static int f81232_port_probe(struct usb_serial_port *port)
> {
> struct f81232_private *priv;
> @@ -279,10 +629,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
> return -ENOMEM;
>
> spin_lock_init(&priv->lock);
> + INIT_WORK(&priv->int_worker, f81232_int_work_wq);
>
> usb_set_serial_port_data(port, priv);
>
> - port->port.drain_delay = 256;
> + priv->dev = port->serial->dev;
> + priv->port = port;
No need to store either of these in the private data.
> return 0;
> }
> @@ -304,22 +656,21 @@ static struct usb_serial_driver f81232_device = {
> },
> .id_table = id_table,
> .num_ports = 1,
> - .bulk_in_size = 256,
> - .bulk_out_size = 256,
> + .bulk_in_size = 64,
> + .bulk_out_size = 64,
Why are you reducing the buffer sizes?
> .open = f81232_open,
> .close = f81232_close,
> - .dtr_rts = f81232_dtr_rts,
> + .dtr_rts = f81232_dtr_rts,
Again, don't include random whitespace changes.
> .carrier_raised = f81232_carrier_raised,
> .ioctl = f81232_ioctl,
> .break_ctl = f81232_break_ctl,
> .set_termios = f81232_set_termios,
> .tiocmget = f81232_tiocmget,
> .tiocmset = f81232_tiocmset,
> - .tiocmiwait = usb_serial_generic_tiocmiwait,
> - .process_read_urb = f81232_process_read_urb,
> + .process_read_urb = f81232_read_bulk_callback,
> .read_int_callback = f81232_read_int_callback,
> .port_probe = f81232_port_probe,
> - .port_remove = f81232_port_remove,
> + .port_remove = f81232_port_remove,
Ditto.
Thanks,
Johan
prev parent reply other threads:[~2015-01-19 11:28 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-19 1:54 [PATCH 1/1] usb: serial: Fintek F81232 driver improvement Peter Hung
2015-01-19 11:29 ` Johan Hovold [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150119112904.GQ30960@localhost \
--to=johan@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hpeter+linux_kernel@gmail.com \
--cc=hpeter@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=peter_hong@fintek.com.tw \
--cc=tom_tsai@fintek.com.tw \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.