On Sun, May 26, 2013 at 01:00:51PM +0200, Andrew Lunn wrote:
> Add a driver which supports the following Moxa USB to serial converters:
> * 2 ports : UPort 1250, UPort 1250I
> * 4 ports : UPort 1410, UPort 1450, UPort 1450I
> * 8 ports : UPort 1610-8, UPort 1650-8
> * 16 ports : UPort 1610-16, UPort 1650-16
>
> The UPORT devices don't directy fit the USB serial model. USB serial
> assumes a bulk in/out endpoint pair per serial port. Thus a dual port
> USB serial device is expected to have two bulk in/out pairs. The Moxa
> UPORT only has one pair for data transfer and places a header on each
> transfer over the endpoint indicating for which port the transfer
> relates to. There is a second endpoint pair for events, just as modem
> control lines changing state, setting baud rates etc. Again, a
> multiplexing header is used on these endpoints.
>
> This difference to the model results in some additional code which
> other drivers don't have:
>
> Some ports need to have a kfifo explicitily allocated since the
> framework does not allocate one if there is no associated endpoints.
> The framework will however free it on unload of the module.
>
> All data transfers are made on port0, yet the locks are taken on PortN.
> urb->context points to PortN, even though the URL is for port0.
First, thanks for submitting the driver. As you mention, it does not fit
the usb-serial model directly, but it seems that there's still quite a
bit more of generic code that could be reused rather than copied or
reimplemented.
There are also some more severe issues related to possible memory leaks
which I comment on below.
Consider my comments as a first round of review, there's probably some
things I've missed and more minor things I'll get back to later.
> The driver will attempt to load firmware from userspace and compare
> the available version and the running version. If the available
> version is newer, it will be download into RAM of the device and
> started. This is optional and currently the firmware blobs are not yet
> in linux-firmware. However MOXA have agreed that the blobs can be
> distributed.
>
> This driver is based on the MOXA driver and retains MOXAs copyright.
The driver is also quite heavily based on the generic driver and much
code is copied more or less verbatim. Please make sure to mention in
this in the header as well.
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
> ---
> drivers/usb/serial/Kconfig | 30 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/mxuport.c | 2005 ++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/serial/mxuport.h | 184 ++++
> 4 files changed, 2220 insertions(+)
> create mode 100644 drivers/usb/serial/mxuport.c
> create mode 100644 drivers/usb/serial/mxuport.h
>
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 1d55762..9d5ad95 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -471,6 +471,36 @@ config USB_SERIAL_MOTOROLA
> To compile this driver as a module, choose M here: the
> module will be called moto_modem. If unsure, choose N.
>
> +config USB_SERIAL_MOXA_UPORT
> + tristate "USB Moxa UPORT USB Serial Driver"
> + select USB_SERIAL_GENERIC
> + ---help---
> + Say Y here if you want to use a MOXA UPort Serial hub.
> +
> + This driver supports:
> +
> + [2 Port]
> + - UPort 1250 : 2 Port RS-232/422/485 USB to Serial Hub
> + - UPort 1250I : 2 Port RS-232/422/485 USB to Serial Hub with
> + Isolation
> +
> + [4 Port]
> + - UPort 1410 : 4 Port RS-232 USB to Serial Hub
> + - UPort 1450 : 4 Port RS-232/422/485 USB to Serial Hub
> + - UPort 1450I : 4 Port RS-232/422/485 USB to Serial Hub with
> + Isolation
> +
> + [8 Port]
> + - UPort 1610-8 : 8 Port RS-232 USB to Serial Hub
> + - UPort 1650-8 : 8 Port RS-232/422/485 USB to Serial Hub
> +
> + [16 Port]
> + - UPort 1610-16 : 16 Port RS-232 USB to Serial Hub
> + - UPort 1650-16 : 16 Port RS-232/422/485 USB to Serial Hub
> +
> + To compile this driver as a module, choose M here: the
> + module will be called mx_uport.
> +
> config USB_SERIAL_NAVMAN
> tristate "USB Navman GPS device"
> help
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index cec63fa..e26b8d0 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_USB_SERIAL_METRO) += metro-usb.o
> obj-$(CONFIG_USB_SERIAL_MOS7720) += mos7720.o
> obj-$(CONFIG_USB_SERIAL_MOS7840) += mos7840.o
> obj-$(CONFIG_USB_SERIAL_MOTOROLA) += moto_modem.o
> +obj-$(CONFIG_USB_SERIAL_MOXA_UPORT) += mxuport.o
> obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o
> obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o
> obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> new file mode 100644
> index 0000000..230cb3e
> --- /dev/null
> +++ b/drivers/usb/serial/mxuport.c
> @@ -0,0 +1,2005 @@
> +/*
> + * mx-uport.c - MOXA UPort series driver
> + *
> + * Copyright (c) 2006 Moxa Technologies Co., Ltd.
> + * Copyright (c) 2013 Andrew Lunn <andrew@xxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Supports the following Moxa USB to serial converters:
> + * 2 ports : UPort 1250, UPort 1250I
> + * 4 ports : UPort 1410, UPort 1450, UPort 1450I
> + * 8 ports : UPort 1610-8, UPort 1650-8
> + * 16 ports : UPort 1610-16, UPort 1650-16
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +#include <asm/unaligned.h>
> +#include "mxuport.h"
> +
> +/* Global variables */
> +static bool debug;
Please drop the debug module parameter. usb_serial_debug_data is
supposed to be controlled using dynamic debugging.
> +
> +/* Table of devices that work with this driver */
> +static struct usb_device_id mxuport_idtable[] = {
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1250_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1251_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID)},
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxuport_idtable);
> +
> +/*
> + * mxuport_prepare_write_buffer - fill in the buffer, ready for
> + * sending
> + *
> + * Add a four byte header containing the port number and the number of
> + * bytes of data in the message. Return the number of bytes in the
> + * buffer.
> + */
> +static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
> + void *dest, size_t size)
> +{
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + unsigned char *buf = dest;
> + int count;
> +
> + count = kfifo_out_locked(&port->write_fifo, buf + 4, size - 4,
> + &port->lock);
> +
> + put_unaligned_be16(mx_port->portno, buf);
> + put_unaligned_be16(count, buf + 2);
> +
> + dev_dbg(&port->dev, "%s - port %d, size %d count %d", __func__,
> + mx_port->portno, size, count);
> +
> + return count + 4;
> +}
> +
> +/*
> + * mxuport_write_start - kick off an URB write
> + * @port: Pointer to the &struct usb_serial_port data
> + *
> + * Returns zero on success, or a negative errno value
> + */
> +static int mxuport_write_start(struct usb_serial_port *port)
> +{
> + struct usb_serial *serial = port->serial;
> + struct urb *urb;
> + int count, result;
> + unsigned long flags;
> + int i;
> + struct usb_serial_port *port0;
> +
> + if (test_and_set_bit_lock(USB_SERIAL_WRITE_BUSY, &port->flags))
> + return 0;
> +
> + /* All data goes over port 0 endpoints */
> + port0 = serial->port[0];
> +
> +retry:
> + spin_lock_irqsave(&port->lock, flags);
> + if (!port0->write_urbs_free || !kfifo_len(&port->write_fifo)) {
> + clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags);
> + spin_unlock_irqrestore(&port->lock, flags);
> + return 0;
> + }
> + i = (int)find_first_bit(&port0->write_urbs_free,
> + ARRAY_SIZE(port0->write_urbs));
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + urb = port0->write_urbs[i];
> + count = mxuport_prepare_write_buffer(port, urb->transfer_buffer,
> + port0->bulk_out_size);
> + urb->transfer_buffer_length = count;
> + if (debug)
> + usb_serial_debug_data(&port->dev, __func__, count,
> + urb->transfer_buffer);
> + spin_lock_irqsave(&port->lock, flags);
> + port->tx_bytes += count;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + clear_bit(i, &port0->write_urbs_free);
> +
> + /* Set the context to point to the real port, not port 0 */
> + urb->context = port;
> +
> + result = usb_submit_urb(urb, GFP_ATOMIC);
> + if (result) {
> + dev_err_console(port, "%s - error submitting urb: %d\n",
> + __func__, result);
> + set_bit(i, &port0->write_urbs_free);
> + spin_lock_irqsave(&port->lock, flags);
> + port->tx_bytes -= count;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags);
> + return result;
> + }
> +
> + /* Try sending off another urb, unless in irq context (in which case
> + * there will be no free urb). */
> + if (!in_irq())
> + goto retry;
> +
> + clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags);
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_write - write function
> + * @tty: Pointer to &struct tty_struct for the device
> + * @port: Pointer to the &usb_serial_port structure for the device
> + * @buf: Pointer to the data to write
> + * @count: Number of bytes to write
> + *
> + * Returns the number of characters actually written, which may be anything
> + * from zero to @count. If an error occurs, it returns the negative errno
> + * value.
> + */
> +static int mxuport_write(struct tty_struct *tty, struct usb_serial_port *port,
> + const unsigned char *buf, int count)
> +{
> + int result;
> +
> + if (!count)
> + return 0;
> +
> + count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
> + result = mxuport_write_start(port);
> + if (result)
> + return result;
> +
> + return count;
> +}
> +
> +/*
> + * mxuport_write_bulk_callback - A write has completed
> + *
> + * Now that the write is complete, update the count of transmitted
> + * characters and make the URB is available to be reused. Start the
> + * next transmission.
> + */
> +
> +static void mxuport_write_bulk_callback(struct urb *urb)
> +{
> + unsigned long flags;
> + struct usb_serial_port *port = urb->context;
> + struct usb_serial_port *port0 = port->serial->port[0];
> +
> + int err = urb->status;
> + int i;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + for (i = 0; i < ARRAY_SIZE(port0->write_urbs); ++i)
> + if (port0->write_urbs[i] == urb)
> + break;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + port->tx_bytes -= urb->transfer_buffer_length;
> + set_bit(i, &port0->write_urbs_free);
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + if (err) {
> + dev_dbg(&port->dev, "%s - non-zero urb status: %d\n",
> + __func__, err);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + kfifo_reset_out(&port->write_fifo);
> + spin_unlock_irqrestore(&port->lock, flags);
> + } else {
> + mxuport_write_start(port);
> + }
> +
> + usb_serial_port_softint(port);
> +}
> +
> +/*
> + * mxuport_write_room
> + *
> + * Return how much space is available in the buffer.
> + */
> +static int mxuport_write_room(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + unsigned long flags;
> + int room;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + room = kfifo_avail(&port->write_fifo);
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + dev_dbg(&port->dev, "%s - returns %d\n", __func__, room);
> + return room;
> +}
> +
> +/*
> + * mxuport_chars_in_buffer
> + *
> + * Return how many charactors are currenty in the write buffer
> + */
> +static int mxuport_chars_in_buffer(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + unsigned long flags;
> + int chars;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + chars = kfifo_len(&port->write_fifo) + port->tx_bytes;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
> + return chars;
> +}
It seems it could be possible to reuse the generic write implementations
of all functions but prepare_write_buffer above rather than the modified
copies.
When you set up the "fake ports", simply allocate the port write urbs
along with the write fifos and initialise them to use the first (and
only) bulk-out end point.
> +/*
> + * mxuport_control_callback
> + *
> + * This is the callback function for when we have finished sending
> + * control urb on the control pipe.
> + */
> +static void mxuport_control_callback(struct urb *urb)
> +{
> + struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
> + struct device *dev = &urb->dev->dev;
> +
> + switch (urb->status) {
> + case 0:
> + break;
> +
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* This urb is terminated, clean up */
> + dev_dbg(dev, "%s - urb shutting down with status: %d",
The newline in the debug message is missing. Same problem in most (or
all) debug an errors messages below. I won't comment on this again.
> + __func__, urb->status);
> + break;
> +
> + default:
> + dev_err(dev, "%s - nonzero control status received: %d\n",
> + __func__, urb->status);
> + break;
> + }
> +
> + kfree(req);
> + usb_free_urb(urb);
> +}
> +
> +/*
> + * mxuport_send_aysnc_ctrl_urb - send asynchronously a vendor request with
> + * data
> + *
> + * This function writes the given buffer out to the control pipe, but
> + * does not wait around for it to complete.
> + */
> +static void mxuport_send_async_ctrl_urb(struct usb_device *udev,
> + __u8 request,
> + __u16 value,
> + __u16 index, u8 *data, int size)
> +{
> + struct usb_ctrlrequest *req;
> + int err;
> + struct urb *urb = usb_alloc_urb(0, GFP_ATOMIC);
> +
> + if (!urb) {
> + dev_dbg(&udev->dev,
> + "Error allocating URB in write_cmd_async!");
No need to log failed allocation (has already been done).
But generally, why not use same style of error and debug message
through-out, e.g
"%s - <message in lowercase>\n", __func__
Exclamation-points aren't needed.
> + return;
> + }
> +
> + req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_ATOMIC);
> + if (!req) {
> + dev_dbg(&udev->dev,
> + "Failed to allocate memory for control request");
No need to log failed allocation here or elsewhere (has already been
done).
> + usb_free_urb(urb);
> + return;
> + }
> +
> + req->bRequestType = USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> + req->bRequest = request;
> + req->wValue = cpu_to_le16(value);
> + req->wIndex = cpu_to_le16(index);
> + req->wLength = cpu_to_le16(size);
> +
> + usb_fill_control_urb(urb,
> + udev,
> + usb_sndctrlpipe(udev, 0),
> + (void *)req,
> + data, size, mxuport_control_callback, req);
> +
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> + if (err < 0) {
> + dev_dbg(&udev->dev,
> + "Error submitting the control message: status=%d",
> + err);
> + kfree(req);
> + usb_free_urb(urb);
> + }
> +}
> +
> +/*
> + * mxuport_recv_ctrl_urb - receive vendor request
> + *
> + * This function reads the given buffer in from the control pipe.
> + */
> +static int mxuport_recv_ctrl_urb(struct usb_device *udev,
> + __u8 request,
> + __u16 value, __u16 index, u8 *data, int size)
> +{
> + int status;
> +
> + status = usb_control_msg(udev,
> + usb_rcvctrlpipe(udev, 0),
> + request,
> + (USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index, data, size,
> + HZ * USB_CTRL_GET_TIMEOUT);
> +
> + if (status < 0) {
> + dev_dbg(&udev->dev, "%s - recv usb_control_msg failed. (%d)",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_dbg(&udev->dev,
> + "%s - wanted to recv %d, but only read %d",
> + __func__, size, status);
> + return -ECOMM;
> + }
> +
> + return status;
> +}
> +
> +/*
> + * mxuport_send_ctrl_data_urb - send vendor request with data
> + *
> + * This function writes the given buffer out to the control pipe.
> + */
> +static int mxuport_send_ctrl_data_urb(struct usb_device *udev,
> + __u8 request,
> + __u16 value, __u16 index,
> + u8 *data, int size)
> +{
> + int status = usb_control_msg(udev,
> + usb_sndctrlpipe(udev, 0),
> + request,
> + (USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + HZ * USB_CTRL_SET_TIMEOUT);
> + if (status < 0) {
> + dev_dbg(&udev->dev, "%s - send usb_control_msg failed. (%d)",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_dbg(&udev->dev,
> + "%s - wanted to write %d, but only wrote %d",
> + __func__, size, status);
> + return -ECOMM;
> + }
> +
> + return status;
> +}
> +
> +/*
> + * mxuport_send_ctrl_urb - send vendor request without any data
> + *
> + * This function writes the given request out to the control pipe.
> + */
> +static int mxuport_send_ctrl_urb(struct usb_device *udev,
> + __u8 request, __u16 value, __u16 index)
> +{
> + return mxuport_send_ctrl_data_urb(udev, request, value, index,
> + NULL, 0);
> +}
> +
> +/*
> + * mxuport_throttle - throttle function of driver
> + *
> + * This function is called by the tty driver when it wants to stop the
> + * data being read from the port.
> + */
> +static void mxuport_throttle(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct usb_serial *serial = port->serial;
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, mx_port->portno);
> +
> + if (mx_port == NULL)
> + return;
Isn't this overly defensive? In fact, you've already dereferenced the
pointer in the debug message above. Just remove the test.
> + if (!mx_port->opened)
> + return;
You should not be getting this callback unless the port is open.
> + spin_lock_irqsave(&mx_port->lock, flags);
> + mx_port->hold_reason |= MX_WAIT_FOR_UNTHROTTLE;
> +
> + mxuport_send_async_ctrl_urb(serial->dev,
> + RQ_VENDOR_SET_RX_HOST_EN,
> + 0, mx_port->portno, NULL, 0);
> +
> + spin_unlock_irqrestore(&mx_port->lock, flags);
> +}
> +
> +/*
> + * mxuport_unthrottle - unthrottle function of driver
> + *
> + * This function is called by the tty driver when it wants to resume
> + * the data being read from the port.
> + */
> +static void mxuport_unthrottle(struct tty_struct *tty)
> +{
> +
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, mx_port->portno);
> +
> + if (mx_port == NULL)
> + return;
> + if (!mx_port->opened)
> + return;
See above.
> + spin_lock_irqsave(&mx_port->lock, flags);
> + mx_port->hold_reason &= ~MX_WAIT_FOR_UNTHROTTLE;
> +
> + mxuport_send_async_ctrl_urb(serial->dev,
> + RQ_VENDOR_SET_RX_HOST_EN,
> + 1, mx_port->portno, NULL, 0);
> + spin_unlock_irqrestore(&mx_port->lock, flags);
> +}
> +
> +/*
> + * mxuport_read_process_data
> + *
> + * Processes one block of data received for a port. If the port is not
> + * open, discard the data. Otherwise pass it onto the tty layer.
> + */
> +static void mxuport_read_process_data(struct usb_serial_port *port,
> + char *ch, int rcv_len)
> +{
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + int i;
> +
> + if (!mx_port->opened)
> + return;
> +
> + if (debug)
> + usb_serial_debug_data(&port->dev, __func__, rcv_len, ch);
> +
> + if (!port->port.console || !port->sysrq)
> + tty_insert_flip_string(&port->port, ch, rcv_len);
> + else {
> + for (i = 0; i < rcv_len; i++, ch++) {
> + if (!usb_serial_handle_sysrq_char(port, *ch))
> + tty_insert_flip_char(&port->port, *ch,
> + TTY_NORMAL);
> + }
> + }
> + tty_flip_buffer_push(&port->port);
> +}
> +
> +/*
> + * mxuport_read_data_caLlback
> + *
> + * This is the callback function for when we have received data on the
> + * bulk in endpoint. Forward it to the correct port
> + */
> +static void mxuport_read_data_callback(struct urb *urb)
> +{
> + struct usb_serial_port *port01 = urb->context;
> + struct usb_serial *serial = port01->serial;
> + unsigned char *data = urb->transfer_buffer;
> + unsigned char *end = data + urb->actual_length;
> + struct device *dev = &urb->dev->dev;
> + struct usb_serial_port *port;
> + __u16 rcv_port, rcv_len;
> + char *ch;
> +
> + if (urb->actual_length < HEADER_SIZE) {
> + dev_dbg(dev, "%s - read bulk callback with no data",
> + __func__);
> + return;
> + }
> +
> + while (data < end) {
> + rcv_port = get_unaligned_be16(data);
> + rcv_len = get_unaligned_be16(data + 2);
> + ch = data + HEADER_SIZE;
> +
> + if (rcv_port >= serial->num_ports) {
> + dev_err(dev, "%s - Invalid port %d\n",
> + __func__, rcv_port);
> + return;
> + }
> +
> + port = serial->port[rcv_port];
> + mxuport_read_process_data(port, ch, rcv_len);
> +
> + data += (HEADER_SIZE + rcv_len);
> + }
> +}
> +
> +/*
> + * mxuport_update_recv_event - Process received events
> + *
> + * When something interesting happens, modem control lines XON/XOFF
> + * etc, the device sends an event. Process these events.
> + */
> +static void mxuport_update_recv_event(struct usb_serial_port *port,
> + struct mxuport_port *mxport,
> + unsigned long event, unsigned char *buf)
> +{
> + unsigned long rcv_msr_event, rcv_msr_hold;
> + unsigned long lsr_event, flags;
> + unsigned long mcr_event;
> + struct async_icount *icount;
> +
> + if (!mxport->opened)
> + return;
> +
> + dev_dbg(&port->dev, "%s - port %d receive event : %ld",
> + __func__, mxport->portno, event);
> +
> + switch (event) {
> + case UPORT_EVNET_SEND_NEXT:
> + spin_lock_irqsave(&mxport->lock, flags);
> + if (mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT)
> + mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
> + spin_unlock_irqrestore(&mxport->lock, flags);
> + break;
> +
> + case UPORT_EVNET_MSR:
> + rcv_msr_event = get_unaligned_be16(buf);
> + rcv_msr_hold = ((unsigned long)buf[2] & 0xF0);
> + icount = &mxport->icount;
> +
> + dev_dbg(&port->dev, "%s - current MSR status = 0x%x",
> + __func__, mxport->msr_state);
> +
> + /* Update hold reason */
> + if (rcv_msr_event != 0) {
> + spin_lock_irqsave(&mxport->lock, flags);
> +
> + if (!(rcv_msr_hold & UART_MSR_CTS)) {
> + mxport->hold_reason |= MX_WAIT_FOR_CTS;
> + mxport->msr_state &= ~UART_MSR_CTS;
> + dev_dbg(&port->dev, "CTS Low");
> + } else {
> + mxport->hold_reason &= ~MX_WAIT_FOR_CTS;
> + mxport->msr_state |= UART_MSR_CTS;
> + dev_dbg(&port->dev, "CTS High");
> + }
> +
> + if (!(rcv_msr_hold & UART_MSR_DSR)) {
> + mxport->hold_reason |= MX_WAIT_FOR_DSR;
> + mxport->msr_state &= ~UART_MSR_DSR;
> + dev_dbg(&port->dev, "DSR Low");
> + } else {
> + mxport->msr_state |= UART_MSR_DSR;
> + mxport->hold_reason &= ~MX_WAIT_FOR_DSR;
> + dev_dbg(&port->dev, "DSR High");
> + }
> +
> + if (!(rcv_msr_hold & UART_MSR_DCD)) {
> + mxport->hold_reason |= MX_WAIT_FOR_DCD;
> + mxport->msr_state &= ~UART_MSR_DCD;
> + dev_dbg(&port->dev, "DCD Low");
> + } else {
> + mxport->msr_state |= UART_MSR_DCD;
> + mxport->hold_reason &= ~MX_WAIT_FOR_DCD;
> + dev_dbg(&port->dev, "DCD High");
> + }
> + spin_unlock_irqrestore(&mxport->lock, flags);
> + }
> +
> + /* Update MSR status */
> + if (rcv_msr_event &
> + (SERIAL_EV_CTS | SERIAL_EV_DSR | SERIAL_EV_RLSD)) {
> +
> + if (rcv_msr_event & SERIAL_EV_CTS) {
> + icount->cts++;
> + dev_dbg(&port->dev, "CTS change");
> + }
> +
> + if (rcv_msr_event & SERIAL_EV_DSR) {
> + icount->dsr++;
> + dev_dbg(&port->dev, "DSR change");
> + }
> +
> + if (rcv_msr_event & SERIAL_EV_RLSD) {
> + icount->dcd++;
> + dev_dbg(&port->dev, "DCD change");
> + }
> +
> + if (atomic_read(&mxport->wait_msr)) {
> + atomic_set(&mxport->wait_msr, 0);
> + wake_up_interruptible
> + (&mxport->delta_msr_wait);
> + }
> + }
> + break;
> +
> + case UPORT_EVNET_LSR:
> + lsr_event = (unsigned long)buf[2] & 0xFF;
> + icount = &mxport->icount;
> +
> + if (lsr_event & UART_LSR_BI) {
> + icount->brk++;
> + dev_dbg(&port->dev, "Break error");
> + }
> +
> + if (lsr_event & UART_LSR_FE) {
> + icount->frame++;
> + dev_dbg(&port->dev, "Frame error");
> + }
> +
> + if (lsr_event & UART_LSR_PE) {
> + icount->parity++;
> + dev_dbg(&port->dev, "Parity error");
> + }
> +
> + if (lsr_event & UART_LSR_OE) {
> + icount->overrun++;
> + dev_dbg(&port->dev, "Overrun error");
> + }
> +
> + mxport->lsr_state = lsr_event;
> +
> + break;
> +
> + case UPORT_EVNET_MCR:
> + mcr_event = (unsigned long)buf[0] & 0xFF;
> +
> + spin_lock_irqsave(&mxport->lock, flags);
> + if (mcr_event & 0x40)
> + mxport->hold_reason |= MX_WAIT_FOR_XON;
> + else
> + mxport->hold_reason &= ~MX_WAIT_FOR_XON;
> + spin_unlock_irqrestore(&mxport->lock, flags);
> +
> +
> + break;
> +
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * mxuport_read_event_callback
> + *
> + * This is the callback function for when we have received event on
> + * the bulk in endpoint.
> + */
> +static void mxuport_read_event_callback(struct urb *urb)
> +{
> + struct usb_serial_port *port01 = urb->context;
> + struct usb_serial *serial = port01->serial;
> + unsigned char *data = urb->transfer_buffer;
> + unsigned char *end = data + urb->actual_length;
> + struct usb_serial_port *port;
> + struct device *dev = &urb->dev->dev;
> + __u16 rcv_port, rcv_event;
> + struct mxuport_port *mx_port;
> +
> + if (urb->actual_length == 0) {
> + dev_dbg(dev, "%s - event bulk callback with no data",
> + __func__);
> + return;
> + }
> +
> + data = urb->transfer_buffer;
> +
> + while (data < end) {
> + rcv_port = get_unaligned_be16(data);
> + rcv_event = get_unaligned_be16(data + 2);
> +
> + if (rcv_port >= serial->num_ports) {
> + dev_err(dev, "%s - Invalid port %d\n",
> + __func__, rcv_port);
> + return;
> + }
> +
> + port = serial->port[rcv_port];
> + mx_port = usb_get_serial_port_data(port);
> + mxuport_update_recv_event(port, mx_port, rcv_event, data + 4);
> +
> + data += MAX_EVENT_LENGTH;
> + }
> +}
> +
> +/*
> + * mxuport_read_bulk_callback
> + *
> + * This is the callback function for when we have received data on the
> + * bulk in endpoint, either with data or events.
> + */
> +static void mxuport_read_bulk_callback(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct usb_serial *serial = port->serial;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(port->read_urbs); ++i) {
> + if (urb == port->read_urbs[i])
> + break;
> + }
> + set_bit(i, &port->read_urbs_free);
> +
> + if (urb->status) {
> + dev_dbg(&port->dev, "%s - non-zero urb status: %d\n",
> + __func__, urb->status);
> + return;
> + }
> +
> + if (port == serial->port[0])
> + mxuport_read_data_callback(urb);
> +
> + if (port == serial->port[1])
> + mxuport_read_event_callback(urb);
> +
> + usb_serial_generic_submit_read_urb(port, i, GFP_ATOMIC);
> +}
Again, it seems you could get rid of most functions above and reuse the
generic implementation while doing the port demultiplexing in
process_read_urb.
This will also make the first patch in the series (exporting
usb_serial_generic_submit_read_urb) unnecessary.
> +
> +/*
> + * mxuport_wait_until_sent - wait while transmit buffer empties
> + *
> + * Parameter timeout is a flag (1 or 0).
> + * If timeout = 1, using a best effort transmit approach
> + * = 0, wait until firmware transmit buffer really empties
> + */
> +static void mxuport_wait_until_sent(struct usb_serial_port *port, int timeout)
Hmmm. You're defining your own wait_until_sent semantics here. Why no use
the generic implementation instead (in v3.10-rc3)? Then all you need to
do is implement tx_empty using RQ_VENDOR_GET_OUTQUEUE.
The implementation below might never return...
> +{
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int err, count;
> + int equal_cnt, pass_cnt;
> + unsigned long txlen;
> + unsigned char len_buf[4];
You need to allocate the buffer using kmalloc as some systems can't do
DMA from the stack.
> +
> + count = 0;
> + pass_cnt = 0;
> + equal_cnt = 0;
> +
> + while (1) {
> + /* get remaining data length of transmit buffer */
> + err = mxuport_recv_ctrl_urb(serial->dev,
> + RQ_VENDOR_GET_OUTQUEUE,
> + 0, mx_port->portno,
> + len_buf, sizeof(len_buf));
> +
> + if (err < 0) {
> + dev_dbg(&port->dev,
> + "%s - receive control URB failed. (status = %d)",
> + __func__, err);
> + return;
> + }
> +
> + txlen = get_unaligned_be32(len_buf);
> + dev_dbg(&port->dev, "%s - tx len = %ld\n", __func__, txlen);
> +
> + if (txlen == 0)
> + break;
> +
> + if (timeout) {
> + if (count != txlen) {
> + count = txlen;
> + pass_cnt++;
> + equal_cnt = 0;
> + } else {
> + equal_cnt++;
> + }
> +
> + if ((pass_cnt > 20) || (equal_cnt > 30))
> + break;
> + }
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(HZ); /* delay 1 second */
Fairly high polling period...
> + }
> +}
> +
> +/*
> + * mxuport_init_port - Initialize the port
> + *
> + * Initialize one port, by enabling its FIFO, set to RS-232 mode, open
> + * the port and enable reception.
> + */
> +static int mxuport_init_port(struct usb_serial_port *port)
> +{
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int err = 0;
> +
> + dev_dbg(&port->dev, "%s - Port number : %d", __func__,
> + mx_port->portno);
> +
> + if (mx_port == NULL)
> + return -ENODEV;
Not needed.
> +
> + /* Send vendor request - set FIFO (Enable) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_FIFO_DISABLE,
> + 0, mx_port->portno);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set transmition mode (Hi-Performance) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_HIGH_PERFOR,
> + 0, mx_port->portno);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set interface (RS-232) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_INTERFACE,
> + mx_port->uart_cfg->interface,
> + mx_port->portno);
> + if (err)
> + return err;
> +
> + /* Send vendor request - port open */
> +
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_OPEN,
> + 1, mx_port->portno);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set receive host (enable) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RX_HOST_EN,
> + 1, mx_port->portno);
> + return err;
> +}
> +
> +/*
> + * mxuport_change_port_settings - Reconfigure the port based on the tty
> + * settings
> + *
> + * Based on the ttys termios settings, send commands to configure the
> + * port as required.
> + */
> +static int mxuport_change_port_settings(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct mxuport_port *mxport)
> +{
> + struct usb_serial *serial = port->serial;
> + struct mx_uart_config config;
It seems some of the mx_uart_config fields are never used (e.g.
baudrate) and/or could be retrieved from the tty struct if needed.
> + int baud, err, status;
> + unsigned int cflag;
> + unsigned char buf[4];
DMA from stack again.
> +
> + dev_dbg(&port->dev, "%s - port %d enter", __func__, mxport->portno);
> +
> + if (!tty) {
> + dev_dbg(&port->dev, "%s - no tty structures", __func__);
> + return 0;
> + }
> +
> + cflag = tty->termios.c_cflag;
> + memset(&config, 0, sizeof(config));
> +
> + /* initial local uart config */
> + config.xon = mxport->uart_cfg->xon;
> + config.xoff = mxport->uart_cfg->xoff;
> + config.interface = mxport->uart_cfg->interface;
> +
> + /* Set data bit of termios */
> + switch (cflag & CSIZE) {
> + case CS5:
> + config.data_bits = MX_WORDLENGTH_5;
> + dev_dbg(&port->dev, "%s - data bits = 5", __func__);
> + break;
> + case CS6:
> + config.data_bits = MX_WORDLENGTH_6;
> + dev_dbg(&port->dev, "%s - data bits = 6", __func__);
> + break;
> + case CS7:
> + config.data_bits = MX_WORDLENGTH_7;
> + dev_dbg(&port->dev, "%s - data bits = 7", __func__);
> + break;
> + case CS8:
> + default:
> + config.data_bits = MX_WORDLENGTH_8;
> + dev_dbg(&port->dev, "%s - data bits = 8", __func__);
> + break;
> + }
> +
> + /* Set parity of termios */
> + if (cflag & PARENB) {
> + if (cflag & CMSPAR) {
> + if (cflag & PARODD) {
> + config.parity = MX_PARITY_MARK;
> + dev_dbg(&port->dev, "%s - parity = mark",
> + __func__);
> + } else {
> + config.parity = MX_PARITY_SPACE;
> + dev_dbg(&port->dev, "%s - parity = space",
> + __func__);
> + }
> + } else if (cflag & PARODD) {
> + config.parity = MX_PARITY_ODD;
> + dev_dbg(&port->dev, "%s - parity = odd", __func__);
> + } else {
> + config.parity = MX_PARITY_EVEN;
> + dev_dbg(&port->dev, "%s - parity = even", __func__);
> + }
> + } else {
> + config.parity = MX_PARITY_NONE;
> + dev_dbg(&port->dev, "%s - parity = none", __func__);
> + }
> +
> + /* Set stop bit of termios */
> + if (cflag & CSTOPB) {
> + config.stop_bits = MX_STOP_BITS_2;
> + dev_dbg(&port->dev, "%s - stop bits = 2", __func__);
> + } else {
> + config.stop_bits = MX_STOP_BITS_1;
> + dev_dbg(&port->dev, "%s - stop bits = 1", __func__);
> + }
> +
> + /* Submit the vendor request */
> + buf[0] = (unsigned char)config.data_bits;
> + buf[1] = (unsigned char)config.parity;
> + buf[2] = (unsigned char)config.stop_bits;
> + buf[3] = 0;
> +
> + status = mxuport_send_ctrl_data_urb(serial->dev, RQ_VENDOR_SET_LINE,
> + 0, mxport->portno,
> + buf, sizeof(buf));
> +
> + if (status != sizeof(buf))
> + return status;
> +
> + /* Flow control settings */
> + config.flow_ctrl = 0;
> +
> + /* H/W flow control settings */
> + if ((cflag & CRTSCTS) && (cflag & CBAUD)) {
> + config.flow_ctrl |= MX_HW_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - RTS/CTS is enabled", __func__);
> + } else {
> + config.flow_ctrl &= ~MX_HW_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - RTS/CTS is disabled", __func__);
> + }
> +
> + /* Submit the vendor request */
> + if (config.flow_ctrl & MX_HW_FLOWCTRL) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 0x2, mxport->portno);
> + if (err)
> + return err;
> + } else {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 0x1, mxport->portno);
> + }
> +
> + /* if baud rate is B0, drop DTR and RTS */
> + if (!(cflag & CBAUD)) {
> + mxport->set_B0 = 1;
> + mxport->mcr_state &= ~(UART_MCR_DTR | UART_MCR_RTS);
> +
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_MCR,
> + mxport->mcr_state, mxport->portno);
> + if (err)
> + return err;
> + } else {
> + if (mxport->set_B0) {
> + mxport->set_B0 = 0;
> + err = mxuport_send_ctrl_urb(serial->dev,
> + RQ_VENDOR_SET_DTR,
> + 1, mxport->portno);
> + if (err)
> + return err;
> + }
> + }
> +
> + /* S/W flow control settings */
> + if (I_IXOFF(tty) || I_IXON(tty)) {
> + config.xon = START_CHAR(tty);
> + config.xoff = STOP_CHAR(tty);
> +
> + /* Submit the vendor request */
> + buf[0] = (unsigned char)config.xon;
> + buf[1] = (unsigned char)config.xoff;
> +
> + status = mxuport_send_ctrl_data_urb(serial->dev,
> + RQ_VENDOR_SET_CHARS,
> + 0, mxport->portno, buf, 2);
> +
> + if (status != 2)
> + return status;
> + }
> +
> + /* if we are implementing OUTBOUND XON/XOFF */
> + if (I_IXON(tty)) {
> + config.flow_ctrl |= MX_XON_FLOWCTRL;
> + dev_dbg(&port->dev,
> + "%s - OUTBOUND XON/XOFF is enabled, XON = 0x%2x, XOFF = 0x%2x",
> + __func__, config.xon, config.xoff);
> + } else {
> + config.flow_ctrl &= ~MX_XON_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - OUTBOUND XON/XOFF is disabled",
> + __func__);
> + }
> +
> + /* if we are implementing INBOUND XON/XOFF */
> + if (I_IXOFF(tty)) {
> + config.flow_ctrl |= MX_XOFF_FLOWCTRL;
> + dev_dbg(&port->dev,
> + "%s - INBOUND XON/XOFF is enabled, XON = 0x%2x, XOFF = 0x%2x",
> + __func__, config.xon, config.xoff);
> + } else {
> + config.flow_ctrl &= ~MX_XOFF_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - INBOUND XON/XOFF is disabled",
> + __func__);
> + }
> +
> + /* Submit the vendor request */
> + if (config.flow_ctrl & (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL)) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 1, mxport->portno);
> + if (err)
> + return err;
> + } else {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 0, mxport->portno);
> + if (err)
> + return err;
> + }
> +
> + /* Set baud rate of termios */
> + baud = tty_get_baud_rate(tty);
> + if (!baud) {
> + /* pick a default using 9600 */
> + baud = 9600;
> + }
> + config.baud_rate = (long)baud;
> +
> + /* Submit the vendor request - Little Endian!! */
> + put_unaligned_le32(config.baud_rate, buf);
> +
> + status = mxuport_send_ctrl_data_urb(serial->dev, RQ_VENDOR_SET_BAUD,
> + 0, mxport->portno,
> + buf, sizeof(buf));
> +
> + if (status != sizeof(buf))
> + return status;
> +
> + /* Finally, store the uart settings to private structure */
> + memcpy(mxport->uart_cfg, &config, sizeof(struct mx_uart_config));
> +
> + /* Dump serial settings */
> + dev_dbg(&port->dev, "baud_rate : %ld", mxport->uart_cfg->baud_rate);
> + dev_dbg(&port->dev, "data_bits : %d", mxport->uart_cfg->data_bits);
> + dev_dbg(&port->dev, "parity : %d", mxport->uart_cfg->parity);
> + dev_dbg(&port->dev, "stop_bits : %d", mxport->uart_cfg->stop_bits);
> + dev_dbg(&port->dev, "flow_ctrl : %d", mxport->uart_cfg->flow_ctrl);
> + dev_dbg(&port->dev, "xon : 0x%x", mxport->uart_cfg->xon);
> + dev_dbg(&port->dev, "xoff : 0x%x", mxport->uart_cfg->xoff);
> + dev_dbg(&port->dev, "Interface : %d", mxport->uart_cfg->interface);
> + dev_dbg(&port->dev, "%s - port %d exit", __func__, mxport->portno);
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_tiocmset - Set the modem control registers
> + *
> + * Sets or clears RTS & DTR.
> + */
> +static int mxuport_tiocmset(struct tty_struct *tty, unsigned int set,
> + unsigned int clear)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int err = 0;
> + unsigned int mcr;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, mx_port->portno);
> +
> + mcr = mx_port->mcr_state;
Locking here and elsewhere?
> +
> + /* set MCR status */
> + if (set & TIOCM_RTS) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 1, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to set port %d RTS",
> + __func__, mx_port->portno);
> + return err;
> + }
> + mcr |= UART_MCR_RTS;
> + }
> +
> + if (set & TIOCM_DTR) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_DTR,
> + 1, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to set port %d DTR",
> + __func__, mx_port->portno);
> + return err;
> + }
> + mcr |= UART_MCR_DTR;
> + }
> +
> + /* clear MCR status */
> + if (clear & TIOCM_RTS) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 0, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to clear port %d RTS",
> + __func__, mx_port->portno);
> + return err;
> + }
> + mcr &= ~UART_MCR_RTS;
> + }
> +
> + if (clear & TIOCM_DTR) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_DTR,
> + 0, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to clear port %d DTR",
> + __func__, mx_port->portno);
> + return err;
> + }
> + mcr &= ~UART_MCR_DTR;
> + }
> +
> + mx_port->mcr_state = mcr;
> +
> + return err;
> +}
> +
> +/*
> + * mxuport_get_serial_info - Return information about the port
> + *
> + */
> +static int mxuport_get_serial_info(struct mxuport_port *mxport,
> + struct serial_struct __user *ret_arg)
> +{
> + struct usb_serial_port *port = mxport->port;
> + struct usb_serial *serial = port->serial;
> + struct serial_struct tmp_serial;
> +
> + if (!ret_arg)
> + return -EFAULT;
> +
> + memset(&tmp_serial, 0, sizeof(tmp_serial));
> +
> + tmp_serial.type = PORT_16550A;
> + tmp_serial.line = serial->minor;
> + tmp_serial.port = mxport->uart_cfg->interface;
> + tmp_serial.flags = mxport->flags;
> + tmp_serial.xmit_fifo_size = MAX_QUEUE_SIZE;
> + tmp_serial.baud_base = 921600;
> +
> + if (copy_to_user(ret_arg, &tmp_serial, sizeof(*ret_arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_set_serial_info
> + *
> + * Set the interface type, e.g. RS-232, 2W RS-484, etc...
> + * Also set the alternative baud rates, 57600 and above
> + */
> +static int mxuport_set_serial_info(struct tty_struct *tty,
> + struct mxuport_port *mxport,
> + struct serial_struct __user *new_arg)
> +{
> + int err;
> + struct usb_serial_port *port = mxport->port;
> + struct usb_device *udev = port->serial->dev;
> + u16 productid = le16_to_cpu(udev->descriptor.idProduct);
> + struct mxuport_port old_cfg;
> + struct serial_struct new_serial;
> +
> + if (copy_from_user(&new_serial, new_arg, sizeof(new_serial)))
> + return -EFAULT;
> +
> + old_cfg = *mxport;
> +
> + if (!capable(CAP_SYS_ADMIN)) {
> + if (((new_serial.flags & ~ASYNC_USR_MASK) !=
> + (mxport->flags & ~ASYNC_USR_MASK)))
> + return -EPERM;
> + mxport->flags = ((mxport->flags & ~ASYNC_USR_MASK) |
> + (new_serial.flags & ASYNC_USR_MASK));
> + }
> + mxport->flags = ((mxport->flags & ~ASYNC_FLAGS) |
> + (new_serial.flags & ASYNC_FLAGS));
> +
> +
> + /* set port interface */
> + if ((productid == MX_UPORT1250_PID) ||
> + (productid == MX_UPORT1251_PID) ||
> + (productid == MX_UPORT1450_PID) ||
> + (productid == MX_UPORT1451_PID) ||
> + (productid == MX_UPORT1658_PID) ||
> + (productid == MX_UPORT1653_PID)) {
I would be better to do this capability test in probe and store a flag
in the usb-serial data.
> + if ((new_serial.port >= 0) && (new_serial.port <= 3)) {
> + if (old_cfg.uart_cfg->interface != new_serial.port) {
> + mxport->uart_cfg->interface = new_serial.port;
> + err = mxuport_send_ctrl_urb(
> + udev,
> + RQ_VENDOR_SET_INTERFACE,
> + mxport->uart_cfg->
> + interface, mxport->portno);
> + if (err)
> + return err;
> + }
> + }
> + } else {
> + if (old_cfg.uart_cfg->interface != new_serial.port)
> + return -EINVAL;
> + }
> +
> + /* set alternative baud rate */
> + if ((old_cfg.flags & ASYNC_SPD_MASK) !=
> + (mxport->flags & ASYNC_SPD_MASK)) {
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> + tty->alt_speed = 57600;
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> + tty->alt_speed = 115200;
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_SHI)
> + tty->alt_speed = 230400;
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_WARP)
> + tty->alt_speed = 460800;
> + mxuport_change_port_settings(tty, port, mxport);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_set_termios
> + *
> + * Change the port configuration based on termios. Check there is
> + * something to change, and call the lower level function if there is.
> + */
> +static void mxuport_set_termios(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> +{
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct ktermios *termios = &tty->termios;
> + unsigned int cflag, iflag;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, mx_port->portno);
> +
> + if (!tty) {
> + dev_dbg(&port->dev, "%s - no tty", __func__);
> + return;
> + }
> +
> + mxuport_wait_until_sent(port, 0);
If you use the generic wait_until_sent implementation (or implement the
standard semantics) then this has already been handled by the tty layer.
> +
> + /* check that they really want us to change something */
> +
> + cflag = termios->c_cflag;
> + iflag = termios->c_iflag;
> +
> + if (old_termios) {
> + if (cflag == old_termios->c_cflag &&
> + iflag == old_termios->c_iflag) {
> + dev_dbg(&port->dev, "%s - nothing to change",
> + __func__);
> + return;
> + }
> + }
> +
> + if (old_termios) {
> + dev_dbg(&port->dev, "%s - old clfag %08x old iflag %08x",
> + __func__, old_termios->c_cflag,
> + old_termios->c_iflag);
> + }
> +
> + if (mx_port == NULL)
> + return;
Remove (dereferenced above either way).
> +
> + /* change the port settings to the new ones specified */
> + mxuport_change_port_settings(tty, port, mx_port);
> +
> + return;
> +}
> +
> +static int mxuport_get_icount(struct tty_struct *tty,
> + struct serial_icounter_struct *icount)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct async_icount *ic = &mx_port->icount;
> +
> + icount->cts = ic->cts;
> + icount->dsr = ic->dsr;
> + icount->rng = ic->rng;
> + icount->dcd = ic->dcd;
> + icount->tx = ic->tx;
> + icount->rx = ic->rx;
> + icount->frame = ic->frame;
> + icount->parity = ic->parity;
> + icount->overrun = ic->overrun;
> + icount->brk = ic->brk;
> + icount->buf_overrun = ic->buf_overrun;
> + return 0;
> +}
You should remove this one and reuse the port icount struct and the
generic implementation.
> +
> +/*
> + * mxuport_ioctl - I/O control function of driver
> + *
> + * This function handles any ioctl calls to the driver.
> + */
> +static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct usb_serial *serial = port->serial;
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + int err;
> + struct async_icount cnow;
> + struct async_icount cprev;
> +
> + if (mx_port == NULL)
> + return -ENODEV;
> +
> + dev_dbg(&port->dev, "%s - port %d, cmd = 0x%x", __func__,
> + mx_port->portno, cmd);
> +
> + switch (cmd) {
> +
> + case TIOCMIWAIT:
> + dev_dbg(&port->dev, "%s - port %d : TIOCMIWAIT", __func__,
> + mx_port->portno);
> +
> + cprev = mx_port->icount;
> + atomic_set(&mx_port->wait_msr, 1);
> +
> + wait_event_interruptible_timeout(mx_port->delta_msr_wait,
> + (atomic_read
> + (&mx_port->wait_msr) == 0),
> + WAIT_MSR_TIMEOUT);
> +
> + /* if no change, return error */
> + cnow = mx_port->icount;
> + if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
> + cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
> + return -EIO;
> +
> + /* status change, return 0 */
> + if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
> + ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
> + ((arg & TIOCM_CD) && (cnow.dcd != cprev.dcd)) ||
> + ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
> + return 0;
> + }
> + break;
You should remove this one as well and reuse the generic implementation.
Simply use the port counters and wait queue, instead.
> +
> + case TIOCGSERIAL:
> + dev_dbg(&port->dev, "%s - port %d : TIOCGSERIAL", __func__,
> + mx_port->portno);
> + return mxuport_get_serial_info(
> + mx_port, (struct serial_struct __user *)arg);
> + break;
> +
> + case TIOCSSERIAL:
> + dev_dbg(&port->dev, "%s - port %d : TIOCSSERIAL", __func__,
> + mx_port->portno);
> + return mxuport_set_serial_info(
> + tty, mx_port, (struct serial_struct __user *)arg);
> + break;
> +
> + case TCFLSH:
> + switch (arg) {
> + case TCIFLUSH:
> + err = mxuport_send_ctrl_urb(serial->dev,
> + RQ_VENDOR_PURGE,
> + 0x8, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev,
> + "%s - fail to flush port %d input buffer",
> + __func__, mx_port->portno);
> + return err;
> + }
> + break;
> +
> + case TCOFLUSH:
> + if (tty->driver->ops->flush_buffer)
> + tty->driver->ops->flush_buffer(tty);
> + err = mxuport_send_ctrl_urb(serial->dev,
> + RQ_VENDOR_PURGE,
> + 0x4, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev,
> + "%s - fail to flush port %d output buffer",
> + __func__, mx_port->portno);
> + return err;
> + }
> + break;
> +
> + case TCIOFLUSH:
> + err = mxuport_send_ctrl_urb(serial->dev,
> + RQ_VENDOR_PURGE,
> + 0xC, mx_port->portno);
> + if (err) {
> + dev_dbg(&port->dev,
> + "%s - fail to flush port %d input/output buffer",
> + __func__, mx_port->portno);
> + return err;
> + }
> + break;
> + }
> + break;
You probably just let the line discipline handle TCFLSH. If you need the
hardware output buffer to be flushed we can add the flush_buffer operation
to usb-serial.
> +
> + default:
> + dev_dbg(&port->dev, "%s - not support. (code : 0x%x)",
> + __func__, cmd);
> + return -ENOIOCTLCMD;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_check_close_port
> + *
> + * Wait a while until the devices buffer is empty and then signal it
> + * to close the port.
> + */
> +static int mxuport_check_close_port(struct mxuport_port *mxport, int portnum)
> +{
> + int err;
> + struct usb_serial *serial = mxport->port->serial;
> + struct usb_serial_port *port = mxport->port;
> +
> + dev_dbg(&port->dev, "%s - port number %d", __func__, portnum);
> +
> + /* wait firmware to transmit data (best effort) */
> + mxuport_wait_until_sent(port, 0);
> +
> + /* Build the vendor request */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_OPEN, 0,
> + portnum);
> +
> + return err;
> +}
> +
> +/*
> + * mxuport_calc_num_port
> + *
> + * Determine how many ports this device has dynamically. It will be
> + * called after the probe() callback is called, but before attach().
> + */
> +static int mxuport_calc_num_ports(struct usb_serial *serial)
> +{
> + u16 product = le16_to_cpu(serial->dev->descriptor.idProduct);
> +
> + switch (product) {
> + case MX_UPORT1250_PID:
> + case MX_UPORT1251_PID:
> + return 2;
> + case MX_UPORT1410_PID:
> + case MX_UPORT1450_PID:
> + case MX_UPORT1451_PID:
> + return 4;
> + case MX_UPORT1618_PID:
> + case MX_UPORT1658_PID:
> + return 8;
> + case MX_UPORT1613_PID:
> + case MX_UPORT1653_PID:
> + return 16;
> +
> + }
> + return 0;
> +}
> +
> +/*
> + * mxuport_probe - device has been found, can we driver it?
> + *
> + * This will be called when the device is inserted into the system,
> + * but before the device has been fully initialized by the usb_serial
> + * subsystem. Check the version of the firmware running on the
> + * device. If there is a newer version available, download it.
> + */
> +static int mxuport_probe(struct usb_serial *serial,
> + const struct usb_device_id *id)
> +{
> + u16 productid = le16_to_cpu(serial->dev->descriptor.idProduct);
> + size_t txlen, fwidx;
> + struct usb_device *udev = serial->dev;
> + unsigned long dev_ver, local_ver;
> + unsigned char *fw_buf = NULL;
> + unsigned char ver_buf[4];
DMA from stack.
> + const struct firmware *fw_p;
> + char buf[32];
> + int err;
> +
> + dev_dbg(&udev->dev, "%s", __func__);
> +
> + /* Load our firmware */
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_QUERY_FW_CONFIG, 0, 0);
> + if (err) {
> + mxuport_send_ctrl_urb(udev, RQ_VENDOR_RESET_DEVICE, 0, 0);
> + return err;
> + }
> +
> + /* Send vendor request - Get firmware version from SDRAM */
> + err = mxuport_recv_ctrl_urb(udev, RQ_VENDOR_GET_VERSION, 0, 0,
> + ver_buf, sizeof(ver_buf));
> + if (err < 0) {
> + dev_dbg(&udev->dev,
> + "%s - receive control URB failed. (status = %d)",
> + __func__, err);
> + return err;
> + }
> +
> + dev_ver = ver_buf[0] * 65536 + ver_buf[1] * 256 + ver_buf[2];
> +
> + dev_dbg(&udev->dev, "Device firmware version v%x.%x.%x\n",
> + ver_buf[0], ver_buf[1], ver_buf[2]);
> +
> + sprintf(buf, "moxa/moxa-%04x.fw", productid);
snprintf
> +
> + err = request_firmware(&fw_p, buf, &udev->dev);
> + if (err) {
> + dev_warn(&udev->dev, "Firmware %s not found\n", buf);
> + /* Use the firmware already in the device */
> + return 0;
> + } else {
> + local_ver = (fw_p->data[VER_ADDR_1] * 65536 +
> + fw_p->data[VER_ADDR_2] * 256 +
> + fw_p->data[VER_ADDR_3]);
Use bitshift and or?
> + dev_dbg(&udev->dev, "Available firmware version v%x.%x.%x\n",
> + fw_p->data[VER_ADDR_1], fw_p->data[VER_ADDR_2],
> + fw_p->data[VER_ADDR_3]);
> + if (local_ver <= dev_ver) {
> + release_firmware(fw_p);
> + return 0;
> + }
> + }
How about splitting the actual firmware download below into a separate
function?
> +
> + dev_dbg(&udev->dev, "Starting download firmware ...");
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_START_FW_DOWN, 0, 0);
> + if (err)
> + goto out;
> +
> + /* Download firmware to device. */
> + fw_buf = kmalloc(sizeof(unsigned char) * DOWN_BLOCK_SIZE, GFP_KERNEL);
sizeof not needed
> + if (fw_buf == NULL) {
> + dev_dbg(&udev->dev, "%s - out of memory", __func__);
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + fwidx = 0;
> + do {
> + txlen = min((fw_p->size - fwidx), (size_t)DOWN_BLOCK_SIZE);
use min_t
> +
> + memcpy(fw_buf, &fw_p->data[fwidx], txlen);
> + err = mxuport_send_ctrl_data_urb(udev, RQ_VENDOR_FW_DATA, 0, 0,
> + fw_buf, txlen);
> + if (err != txlen)
> + goto out;
> +
> + fwidx += txlen;
> + mdelay(1);
> +
> + } while (fwidx < fw_p->size);
> +
> + mdelay(1000);
msleep
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_STOP_FW_DOWN, 0, 0);
> + if (err)
> + goto out;
> +
> + mdelay(1000);
msleep
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_QUERY_FW_READY, 0, 0);
> + if (err)
> + goto out;
> +
> + dev_dbg(&udev->dev, "Download firmware completely.");
> +
> + err = mxuport_recv_ctrl_urb(udev, RQ_VENDOR_GET_VERSION, 0, 0,
> + ver_buf, sizeof(ver_buf));
> + if (err < 0)
> + dev_dbg(&udev->dev,
> + "%s - receive control URB failed. (err = %d)",
> + __func__, err);
> + err = 0;
> +
> + dev_ver = ver_buf[0] * 65536 + ver_buf[1] * 256 + ver_buf[2];
> +
> + dev_dbg(&udev->dev, "Installed device firmware version v%x.%x.%x\n",
> + ver_buf[0], ver_buf[1], ver_buf[2]);
dev_info?
> +out:
> + kfree(fw_buf);
> + release_firmware(fw_p);
> + return err;
> +}
> +
> +/*
> + * mxuport_attach - attach function of driver
> + *
> + * This will be called when the struct usb_serial structure is fully
> + * set up. Create private structures per port, and set the device into
> + * a default configuration.
> + */
> +static int mxuport_attach(struct usb_serial *serial)
> +{
> +
> + struct usb_serial_port *port0 = serial->port[0];
> + struct usb_serial_port *port1 = serial->port[1];
> + struct usb_serial_port *port;
> + struct mxuport_port *mx_port;
> + struct usb_device *udev;
> + int err = -ENOMEM;
> + int i;
> +
> + /* Get product info from device */
> + udev = serial->dev;
> + dev_dbg(&udev->dev, "%s detected\n", serial->type->description);
> +
> + /* Setup our port private structures */
> + for (i = 0; i < serial->num_ports; ++i) {
> + mx_port = kzalloc(sizeof(struct mxuport_port), GFP_KERNEL);
> + if (mx_port == NULL) {
> + dev_err(&udev->dev, "%s - Out of memory\n",
> + __func__);
> + goto out;
> + }
> +
> + /* Initial local port spin lock */
> + spin_lock_init(&mx_port->lock);
> +
> + /* Initial serial settings */
> + mx_port->portno = i;
> +
> + /* Loop back to the owner of this object */
> + mx_port->port = serial->port[i];
> +
> + /* Initial UART configuration */
> + mx_port->uart_cfg =
> + kzalloc(sizeof(struct mx_uart_config), GFP_KERNEL);
> + if (mx_port->uart_cfg == NULL) {
> + dev_err(&udev->dev, "%s - Out of memory\n",
> + __func__);
> + goto out;
> + }
> +
> + mx_port->uart_cfg->baud_rate = 9600;
> + mx_port->uart_cfg->data_bits = MX_WORDLENGTH_8;
> + mx_port->uart_cfg->parity = MX_PARITY_NONE;
> + mx_port->uart_cfg->stop_bits = MX_STOP_BITS_1;
> + mx_port->uart_cfg->flow_ctrl = MX_NO_FLOWCTRL;
> + mx_port->uart_cfg->xon = MX_START_CHAR;
> + mx_port->uart_cfg->xoff = MX_STOP_CHAR;
> + mx_port->uart_cfg->interface = MX_INT_RS232;
> +
> + /* Set the port private data */
> + usb_set_serial_port_data(serial->port[i], mx_port);
> + }
> +
> + /* Start to read data and receive event from the device */
> + err = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
> + if (err) {
> + dev_dbg(&port0->dev,
> + "%s - USB submit read bulk urb failed. (status = %d)",
> + __func__, err);
> + goto out;
> + }
> +
> + /* Endpoints on Port1 is used for events */
> + err = usb_serial_generic_submit_read_urbs(port1, GFP_KERNEL);
> + if (err) {
> + dev_dbg(&port1->dev,
> + "%s - USB submit read bulk urb failed. (status = %d)",
> + __func__, err);
> + goto out;
> + }
> +
> + /*
> + * Ensure each port has a fifo. The framework only allocates a
> + * fifo to ports with a bulk out endpoint, where as we need
> + * one for every port.
> + */
> + for (i = 0; i < serial->num_ports; ++i) {
> + port = serial->port[i];
> + if (!kfifo_initialized(&port->write_fifo)) {
> + err = kfifo_alloc(&port->write_fifo, PAGE_SIZE,
> + GFP_KERNEL);
> + if (err)
> + goto out;
> + }
> + }
> + return 0;
> +
> +out:
> + /* shutdown any bulk transfers that might be going on */
> + usb_serial_generic_close(port1);
> + usb_serial_generic_close(port0);
> +
> + for (; i >= 0; i--) {
> + mx_port = usb_get_serial_port_data(serial->port[i]);
> + if (mx_port == NULL)
> + continue;
> +
> + if (mx_port->uart_cfg != NULL)
> + kfree(mx_port->uart_cfg);
> +
> + kfree(mx_port);
> + usb_set_serial_port_data(serial->port[i], NULL);
> + }
> + usb_set_serial_data(serial, NULL);
Has never been set.
> +
> + return err;
> +}
You should allocate the port data for each port in port_probe rather
than in attach. The device port number can be retrieved from the tty
port number (i.e. port->number - port->serial->minor).
As mentioned above you should allocate and initialise the write_urbs for
the "fake ports", so that you can reuse the generic write
implementation.
You could submit the read urbs using usb_serial_generic_open or
explicitly
if (port->bulk_in_size)
result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
as only port 0 and 1 have bulk_in_size set.
> +/*
> + * mxuport_release - release function of driver
> + *
> + * This will be called when the usb_serial data structure is about to
> + * be destroyed.
> + */
> +static void mxuport_release(struct usb_serial *serial)
> +{
> + int i;
> + struct mxuport_port *mx_port;
> +
> + dev_dbg(&serial->dev->dev, "%s", __func__);
> +
> + /* If disconnected, exit directly. */
> + if (!usb_get_intfdata(serial->interface))
> + return;
> +
> + /* Stop reads and writes on all ports */
> + for (i = 0; i < serial->num_ports; ++i) {
> + mx_port = usb_get_serial_port_data(serial->port[i]);
> +
> + if (mx_port == NULL)
> + continue;
> +
> + if (mx_port->uart_cfg != NULL)
> + kfree(mx_port->uart_cfg);
> +
> + if (mx_port->opened)
> + mxuport_check_close_port(mx_port, i);
No need for this. Port has already been closed.
> +
> + kfree(usb_get_serial_port_data(serial->port[i]));
> + usb_set_serial_port_data(serial->port[i], NULL);
> + }
> +
> + kfree(usb_get_serial_data(serial));
> + usb_set_serial_data(serial, NULL);
Has never been set.
> +}
This should be done per port in port_remove, or you may even leak
memory.
> +
> +/*
> + * mxuport_open - open function of driver
> + *
> + * This function is called by the tty driver when a port is opened
> + * If successful, we return 0.
> + * Otherwise, we return a negative error number.
> + */
> +static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + int err = 0;
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> +
> + if (mx_port == NULL)
> + return -ENODEV;
Remove.
> +
> + /* Initial port settings */
> + err = mxuport_init_port(port);
> + if (err)
> + return err;
> +
> + /* Initial port termios */
> + mxuport_set_termios(tty, port, NULL);
> +
> + /* Initial private parameters of port */
> + mx_port->opened = true;
> + mx_port->hold_reason = 0;
> + mx_port->lsr_state = 0;
> + mx_port->msr_state = 0;
> + mx_port->set_B0 = 0;
> + mx_port->mcr_state = UART_MCR_DTR | UART_MCR_RTS;
You should implement drt_rts() instead.
> +
> + init_waitqueue_head(&mx_port->delta_msr_wait);
> +
> + memset(&(mx_port->icount), 0x0, sizeof(mx_port->icount));
You should use the port wait queue and interrupt counters.
> +
> + return err;
> +}
> +
> +/*
> + * mxuport_close - close function of driver
> + *
> + * This function is called by the tty driver when a port is closed.
> + */
> +static void mxuport_close(struct usb_serial_port *port)
> +{
> + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> +
> + /* If disconnected, exit directly. */
> + if (!usb_get_intfdata(serial->interface))
> + return;
Remove. You shouldn't have to worry about disconnected interfaces in
close.
> +
> + if (mx_port == NULL)
> + return;
Remove.
> + dev_dbg(&port->dev, "%s - port %d", __func__, mx_port->portno);
> +
> + /* Check the port has already closed */
> + if (mxuport_check_close_port(mx_port, mx_port->portno))
> + return;
Close is called exactly once and only for opened ports, so remove.
> +
> + /*
> + * Mark the port closed, so any data we may receive gets
> + * discarded
> + */
> + mx_port->opened = false;
You might not need this at all, but let's look at that later.
> +
> + /*
> + * Reenable the S/W flow control to prevent cannot write
> + * data out that receive XOFF char before.
> + */
> + mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 0, mx_port->portno);
> + if (mx_port->uart_cfg->flow_ctrl &
> + (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL)) {
> + mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 1, mx_port->portno);
> + }
> +
> + /* Shutdown our urb */
Remove comment (copy-paste-error).
> + dev_dbg(&port->dev, "%s exited", __func__);
Not needed.
> +
> + return;
Likewise.
> +}
> +
> +static int mxuport_tiocmget(struct tty_struct *tty)
> +{
> + struct mxuport_port *mx_port;
> + struct usb_serial_port *port = tty->driver_data;
> + unsigned int result = 0;
> + unsigned int msr;
> + unsigned int mcr;
> + unsigned long flags;
> +
> + mx_port = usb_get_serial_port_data(port);
> + if (mx_port == NULL)
> + return -ENODEV;
> +
> + spin_lock_irqsave(&mx_port->lock, flags);
> +
> + msr = mx_port->msr_state;
> +
> + spin_unlock_irqrestore(&mx_port->lock, flags);
> +
> + mcr = mx_port->mcr_state;
> +
> + result = (((mcr & UART_MCR_DTR) ? TIOCM_DTR : 0) | /* 0x002 */
> + ((mcr & UART_MCR_RTS) ? TIOCM_RTS : 0) | /* 0x004 */
> + ((msr & UART_MSR_CTS) ? TIOCM_CTS : 0) | /* 0x020 */
> + ((msr & UART_MSR_DCD) ? TIOCM_CAR : 0) | /* 0x040 */
> + ((msr & UART_MSR_RI) ? TIOCM_RI : 0) | /* 0x080 */
> + ((msr & UART_MSR_DSR) ? TIOCM_DSR : 0)); /* 0x100 */
> +
> + dev_dbg(&port->dev, "%s - MSR return 0x%x", __func__, result);
> +
> + return result;
> +}
> +
> +/*
> + * mxuport_break_ctl - break function of driver
> + *
> + * This function sends a break to the port.
> + */
> +static void mxuport_break_ctl(struct tty_struct *tty, int break_state)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct usb_serial *serial = port->serial;
> + struct mxuport_port *mx_port;
> + int err;
> +
> + mx_port = usb_get_serial_port_data(port);
> + if (mx_port == NULL)
> + return;
> +
> + if (break_state == -1) {
> + dev_dbg(&port->dev, "%s - Sending break to port %d",
> + __func__, mx_port->portno);
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_BREAK,
> + 1, mx_port->portno);
> + } else {
> + dev_dbg(&port->dev, "%s - Clearing break of port %d",
> + __func__, mx_port->portno);
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_BREAK,
> + 0, mx_port->portno);
> + }
> + if (err) {
> + dev_dbg(&port->dev,
> + "%s - error sending break set/clear command.",
> + __func__);
> + }
> +
> + return;
> +}
> +
> +static struct usb_serial_driver mxuport_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "mx-uport",
> + },
> + .description = "MOXA UPort",
> + .id_table = mxuport_idtable,
> + .num_ports = 0,
> + .probe = mxuport_probe,
> + .attach = mxuport_attach,
> + .calc_num_ports = mxuport_calc_num_ports,
> + .release = mxuport_release,
> + .open = mxuport_open,
> + .close = mxuport_close,
> + .write = mxuport_write,
> + .write_room = mxuport_write_room,
> + .ioctl = mxuport_ioctl,
> + .set_termios = mxuport_set_termios,
> + .break_ctl = mxuport_break_ctl,
> + .get_icount = mxuport_get_icount,
> + .throttle = mxuport_throttle,
> + .chars_in_buffer = mxuport_chars_in_buffer,
> + .unthrottle = mxuport_unthrottle,
> + .tiocmget = mxuport_tiocmget,
> + .tiocmset = mxuport_tiocmset,
> + .read_bulk_callback = mxuport_read_bulk_callback,
> + .write_bulk_callback = mxuport_write_bulk_callback,
> + .prepare_write_buffer = mxuport_prepare_write_buffer,
Please align the values using tabs.
> +};
> +
> +static struct usb_serial_driver *const serial_drivers[] = {
> + &mxuport_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, mxuport_idtable);
> +
> +/*
> + * Module Information
> + */
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_LICENSE("GPL");
> +
> +module_param(debug, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(debug, "Enable debugging, 0=OFF, 1=ON, Default is OFF");
Remove.
> diff --git a/drivers/usb/serial/mxuport.h b/drivers/usb/serial/mxuport.h
> new file mode 100644
> index 0000000..a0e1eae
> --- /dev/null
> +++ b/drivers/usb/serial/mxuport.h
These defines aren't used outside of the driver and should probably
simply be included in the c-file directly.
> @@ -0,0 +1,184 @@
> +/*
> + * mx-uport.h - MOXA UPort series drvier definitions
> + *
> + * Copyright(c)2006 Moxa Technologies Co., Ltd.
> + * Copyright(c)2013 Andrew Lunn.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +#ifndef __MX_USB_SERIAL_H
> +#define __MX_USB_SERIAL_H
> +
> +/* Definitions for driver info */
> +#define DRIVER_AUTHOR \
> + "<support@xxxxxxxx>" \
> + "Andrew Lunn <andrew@xxxxxxx>"
> +
> +/* Definitions for the vendor ID and device ID */
> +#define MX_USBSERIAL_VID 0x110A
> +#define MX_UPORT1250_PID 0x1250
> +#define MX_UPORT1251_PID 0x1251
> +#define MX_UPORT1410_PID 0x1410
> +#define MX_UPORT1450_PID 0x1450
> +#define MX_UPORT1451_PID 0x1451
> +#define MX_UPORT1618_PID 0x1618
> +#define MX_UPORT1658_PID 0x1658
> +#define MX_UPORT1613_PID 0x1613
> +#define MX_UPORT1653_PID 0x1653
> +
> +/* Definitions for USB info */
> +#define HEADER_SIZE 4
> +#define MAX_EVENT_LENGTH 8
> +#define MAX_NAME_LEN 64
Indentation? Please use tabs through-out.
> +#define DOWN_BLOCK_SIZE 64
> +#define TTY_THRESHOLD_THROTTLE 128
> +#define TRIGGER_SEND_NEXT 4096
> +#define MAX_QUEUE_SIZE (1024 * 32)
> +#define HIGH_WATER_SIZE (1024 * 5)
> +#define LOW_WATER_SIZE (1024 * 2)
> +#define WAIT_MSR_TIMEOUT (5*HZ)
> +
> +/* Definitions for firmware info */
> +#define VER_ADDR_1 0x20
> +#define VER_ADDR_2 0x24
> +#define VER_ADDR_3 0x28
> +
> +/* Definitions for USB vendor request */
> +#define RQ_VENDOR_NONE 0x00
> +#define RQ_VENDOR_SET_BAUD 0x01 /* set baud rate */
> +#define RQ_VENDOR_SET_LINE 0x02 /* set line status */
> +#define RQ_VENDOR_SET_CHARS 0x03 /* set Xon/Xoff chars */
> +#define RQ_VENDOR_SET_RTS 0x04 /* set RTS */
> +#define RQ_VENDOR_SET_DTR 0x05 /* set DTR */
> +#define RQ_VENDOR_SET_XONXOFF 0x06 /* set auto Xon/Xoff */
> +#define RQ_VENDOR_SET_RX_HOST_EN 0x07 /* set RX host enable */
> +#define RQ_VENDOR_SET_OPEN 0x08 /* set open/close port */
> +#define RQ_VENDOR_PURGE 0x09 /* purge Rx/Tx buffer */
> +#define RQ_VENDOR_SET_MCR 0x0A /* set MCR register */
> +#define RQ_VENDOR_SET_BREAK 0x0B /* set Break signal */
> +
> +#define RQ_VENDOR_START_FW_DOWN 0x0C /* start firmware download */
> +#define RQ_VENDOR_STOP_FW_DOWN 0x0D /* stop firmware download */
> +#define RQ_VENDOR_QUERY_FW_READY 0x0E /* query if new firmware ready */
> +
> +#define RQ_VENDOR_SET_FIFO_DISABLE 0x0F /* set fifo disable */
> +#define RQ_VENDOR_SET_INTERFACE 0x10 /* set interface */
> +#define RQ_VENDOR_SET_HIGH_PERFOR 0x11 /* set hi-performance */
> +
> +#define RQ_VENDOR_ERASE_BLOCK 0x12 /* erase flash block */
> +#define RQ_VENDOR_WRITE_PAGE 0x13 /* write flash page */
> +#define RQ_VENDOR_PREPARE_WRITE 0x14 /* prepare write flash */
> +#define RQ_VENDOR_CONFIRM_WRITE 0x15 /* confirm write flash */
> +#define RQ_VENDOR_LOCATE 0x16 /* locate the device */
> +
> +#define RQ_VENDOR_START_ROM_DOWN 0x17 /* start firmware download */
> +#define RQ_VENDOR_ROM_DATA 0x18 /* rom file data */
> +#define RQ_VENDOR_STOP_ROM_DOWN 0x19 /* stop firmware download */
> +#define RQ_VENDOR_FW_DATA 0x20 /* firmware data */
> +
> +#define RQ_VENDOR_RESET_DEVICE 0x23 /* Try to reset the device */
> +#define RQ_VENDOR_QUERY_FW_CONFIG 0x24
> +
> +#define RQ_VENDOR_GET_VERSION 0x81 /* get firmware version */
> +#define RQ_VENDOR_GET_PAGE 0x82 /* read flash page */
> +#define RQ_VENDOR_GET_ROM_PROC 0x83 /* get ROM process state */
> +
> +#define RQ_VENDOR_GET_INQUEUE 0x84 /* data remaining in input buffer */
> +#define RQ_VENDOR_GET_OUTQUEUE 0x85 /* data remaining in output buffer */
> +
> +#define RQ_VENDOR_GET_MSR 0x86
> +
> +/* Definitions for UPort event type */
> +#define UPORT_EVENT_NONE 0 /* None */
Remove the tab after #define here and elsewhere.
> +#define UPORT_EVNET_TXBUF_THRESHOLD 1 /* Tx buffer threshold */
> +#define UPORT_EVNET_SEND_NEXT 2 /* Send next */
> +#define UPORT_EVNET_MSR 3 /* Modem status */
> +#define UPORT_EVNET_LSR 4 /* Line status */
> +#define UPORT_EVNET_MCR 5 /* Modem control */
> +
> +/* Definitions for serial event type */
> +#define SERIAL_EV_CTS 0x0008 /* CTS changed state */
> +#define SERIAL_EV_DSR 0x0010 /* DSR changed state */
> +#define SERIAL_EV_RLSD 0x0020 /* RLSD changed state */
> +
> +/* Definitions for line control of communication */
> +#define MX_WORDLENGTH_5 5
> +#define MX_WORDLENGTH_6 6
> +#define MX_WORDLENGTH_7 7
> +#define MX_WORDLENGTH_8 8
> +
> +#define MX_PARITY_NONE 0
> +#define MX_PARITY_ODD 1
> +#define MX_PARITY_EVEN 2
> +#define MX_PARITY_MARK 3
> +#define MX_PARITY_SPACE 4
> +
> +#define MX_STOP_BITS_1 0
> +#define MX_STOP_BITS_1_5 1
> +#define MX_STOP_BITS_2 2
> +
> +#define MX_NO_FLOWCTRL 0x0
> +#define MX_HW_FLOWCTRL 0x1
> +#define MX_XON_FLOWCTRL 0x2
> +#define MX_XOFF_FLOWCTRL 0x4
> +
> +#define MX_INT_RS232 0
> +#define MX_INT_2W_RS485 1
> +#define MX_INT_RS422 2
> +#define MX_INT_4W_RS485 3
> +
> +#define MX_START_CHAR 0x11
> +#define MX_STOP_CHAR 0x13
> +
> +/* Definitions for holding reason */
> +#define MX_WAIT_FOR_CTS 0x0001
> +#define MX_WAIT_FOR_DSR 0x0002
> +#define MX_WAIT_FOR_DCD 0x0004
> +#define MX_WAIT_FOR_XON 0x0008
> +#define MX_WAIT_FOR_START_TX 0x0010
> +#define MX_WAIT_FOR_UNTHROTTLE 0x0020
> +#define MX_WAIT_FOR_LOW_WATER 0x0040
> +#define MX_WAIT_FOR_SEND_NEXT 0x0080
> +
> +/* This structure holds all of the local port information */
> +struct mxuport_port {
> + int portno; /* indicate the actual port number */
> + bool opened; /* port open status */
> + int flags; /* for async_struct and serial_struct flags
> + field */
> + int set_B0;
> +
> + __u8 mcr_state; /* last MCR state */
You should u8 here and below.
> +
> + spinlock_t lock; /* protects msr_state and lsr_state and
> + hold reason */
> + __u8 msr_state; /* last MSR state */
> + __u8 lsr_state; /* last LSR state */
> + unsigned long hold_reason;
> +
> + struct async_icount icount;
Could be removed if you use the port counters.
> +
> + struct usb_serial_port *port; /* loop back to the owner of this
> + object */
> + struct mx_uart_config *uart_cfg; /* configuration of UART */
> +
> + atomic_t wait_msr; /* flag for waiting MSR event */
> + wait_queue_head_t delta_msr_wait; /* waiting for MSR change */
These two can be removed if you use the generic implementation.
> +};
> +
> +/* Configuration of UART */
> +struct mx_uart_config {
> + long baud_rate; /* baud rate */
> + __u8 data_bits; /* 5..8 - data bits per character */
> + __u8 parity; /* parity settings */
> + __u8 stop_bits; /* stop bits settings */
> + __u8 flow_ctrl; /* flow control settings */
> + char xon; /* XON character */
> + char xoff; /* XOFF character */
> + int interface; /* interface is defined */
> +};
As mentioned elsewhere, do you really need all of these fields?
> +#endif
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
