On Sun, May 26, 2013 at 06:04:24PM +0200, Johan Hovold wrote:
> 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.
And thanks for the quick review.
> 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.
I will take another look. I was not happy with cut/pasting code from
the generic driver, so tried to minimize it. I will take a deeper look
at your suggestions with the endpoints.
> 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.
Sure, no problem.
> > 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.
O.K.
Also, most of the paranoid checks for NULL pointers and closed ports
is from the original MOXA driver. Maybe in the past they have had
problems. I've already take out a lot of dead code, and converted to
generic where i could. I'm happy to continue this.
> > +/* Global variables */
> > +static bool debug;
>
> Please drop the debug module parameter. usb_serial_debug_data is
> supposed to be controlled using dynamic debugging.
O.K.
> > +
> > +/* 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.
I didn't think about initializing them for the first port. Yes, that
could help. Thanks.
> > +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.
I suspect that is from the original MOXA driver. I will go through
them all and make them uniform.
> > +/*
> > + * 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.
Sure, i can take another look at this now you gave me the idea.
>
> > +
> > +/*
> > + * 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.
Ah, new functionality. I did the development work with v3.9 and just
rebased onto v3.10 without looking to see what had changed. I will
take a look at the commit logs for v3.10.
> 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.
Ah, O.K. I developed on ARM, and it was fine. Is it necessary to pass
GFP_DMA? I don't use any users of this flag in drivers/usb/serial, so
i guess not.
> > +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.
O.K, i can weed out the unused ones.
> > +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?
Lack of locking was a general issue with the original MOXA
driver. mcr_state is not protected by a lock. I can add one.
> > +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.
O.K.
> > +/*
> > + * 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.
I need to look deeper, but one issues is the size of the write buffer
in the device itself. It can be very large so you do need to wait
around for it to flush stuff out, before making changes. There is a
microprocessor inside the box and it seems like all the space RAM is
dedicated to buffers. When using 9600 it can take many multiple of
seconds to send out what it has buffered.
> > +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.
Is new in v3.10?
>
> > +
> > +/*
> > + * 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.
Will do.
>
> > + 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.
The output buffer can be large, so flushing it probably is needed.
> > + 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?
Won't the compiler do that anyway?
>
> > + 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?
Can do. It is clear more than a vt100 screenfull.
> > + 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?
I can do. But i noticed dev_info is not used very much in
drivers/usb/serial so i avoided it. Also, 'Installed' is maybe the
wrong word. Downloaded might be better, since it goes into RAM and
will be lost on power cycle.
> > +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.
O.K.
> > 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.
O.K.
> > @@ -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.
Strange. I would of though checkpatch would of found that.
>
> > +#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.
Sure. Easy to see once in an email :-)
Thanks for the feedback.
Andrew
--
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
