On Wed, Nov 11, 2015 at 10:35:47AM +0100, Mathieu OTHACEHE wrote:
> Add a driver which supports :
>
> - UPort 1110 : 1 port RS-232 USB to Serial Hub.
> - UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
> - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
> - UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
> - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
>
> This driver is based on GPL MOXA driver written by Hen Huang and available
> on MOXA website. The original driver was based on io_ti serial driver.
>
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@xxxxxxxxx>
> ---
Thanks for the v4, and sorry about the late review.
> +struct mxu1_port {
> + u8 mxp_msr;
> + u8 mxp_mcr;
> + u8 mxp_uart_types;
> + u8 mxp_uart_mode;
> + struct usb_serial_port *mxp_port;
You should not need a back pointer here. Pass the usb_serial_port as an
argument where needed instead.
> + spinlock_t mxp_lock; /* Protects mxp_msr */
> + struct mutex mxp_mutex; /* Protects mxp_mcr */
> + int mxp_send_break;
Why is this not a bool? You never use the value you assign to
mxp_send_break other than to compare it with a constant.
> +};
Please drop the mxp_ prefix from the field names.
> +struct mxu1_device {
> + struct mutex mxd_lock;
> + struct usb_serial *mxd_serial;
This lock and back pointer are not needed. You can access the usb_serial
from usb_serial_port.
> + u16 mxd_model;
> +};
> +
> +static const struct usb_device_id mxu1_idtable[] = {
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1110_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1130_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1150_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1151_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1131_PRODUCT_ID) },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxu1_idtable);
> +
> +/* Write the given buffer out to the control pipe. */
> +static int mxu1_send_ctrl_data_urb(struct usb_serial *serial,
> + u8 request,
> + u16 value, u16 index,
> + u8 *data, size_t size)
Use void * for the buffer and drop the casts at the call sites.
> +{
> + int status;
> +
> + status = usb_control_msg(serial->dev,
> + usb_sndctrlpipe(serial->dev, 0),
> + request,
> + (USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + USB_CTRL_SET_TIMEOUT);
> + if (status < 0) {
> + dev_err(&serial->interface->dev,
> + "%s - usb_control_msg failed: %d\n",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_err(&serial->interface->dev,
> + "%s - short write (%d / %zd)\n",
> + __func__, status, size);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +/* Send a vendor request without any data */
> +static int mxu1_send_ctrl_urb(struct usb_serial *serial,
> + u8 request, u16 value, u16 index)
> +{
> + return mxu1_send_ctrl_data_urb(serial, request, value, index,
> + NULL, 0);
> +}
> +
> +static int mxu1_download_firmware(struct usb_serial *serial,
> + const struct firmware *fw_p)
> +{
> + int status = 0;
> + int buffer_size;
> + int pos;
> + int len;
> + int done;
> + u8 cs = 0;
> + u8 *buffer;
> + struct usb_device *dev = serial->dev;
> + struct mxu1_firmware_header *header;
> + unsigned int pipe;
> +
> + pipe = usb_sndbulkpipe(dev, serial->port[0]->
> + bulk_out_endpointAddress);
Odd line break that is not even needed.
> +
> + buffer_size = fw_p->size + sizeof(*header);
> + buffer = kmalloc(buffer_size, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + memcpy(buffer, fw_p->data, fw_p->size);
> + memset(buffer + fw_p->size, 0xff, buffer_size - fw_p->size);
> +
> + for (pos = sizeof(*header); pos < buffer_size; pos++)
> + cs = (u8)(cs + buffer[pos]);
> +
> + header = (struct mxu1_firmware_header *)buffer;
> + header->wLength = cpu_to_le16(buffer_size - sizeof(*header));
> + header->bCheckSum = cs;
> +
> + dev_dbg(&dev->dev, "%s - downloading firmware\n", __func__);
> +
> + for (pos = 0; pos < buffer_size; pos += done) {
> + len = min(buffer_size - pos, MXU1_DOWNLOAD_MAX_PACKET_SIZE);
> +
> + status = usb_bulk_msg(dev, pipe, buffer + pos, len, &done,
> + MXU1_DOWNLOAD_TIMEOUT);
> + if (status)
> + break;
> + }
> +
> + kfree(buffer);
> +
> + if (status) {
> + dev_err(&dev->dev, "failed to download firmware: %d\n", status);
> + return status;
> + }
> +
> + msleep_interruptible(100);
> + usb_reset_device(dev);
> +
> + dev_dbg(&dev->dev, "%s - download successful (%d)\n", __func__, status);
You can drop status from the message, it's always zero.
> +
> + return 0;
> +}
> +static int mxu1_startup(struct usb_serial *serial)
> +{
> + struct mxu1_device *mxdev;
> + struct usb_device *dev = serial->dev;
> + struct usb_host_interface *cur_altsetting;
> + char fw_name[32];
> + const struct firmware *fw_p = NULL;
> + int err;
> + int status = 0;
> +
> + dev_dbg(&dev->dev, "%s - product 0x%4X, num configurations %d, configuration value %d\n",
Use &serial->interface->dev here too, and you want 0x%04x zero-padded.
> + __func__, le16_to_cpu(dev->descriptor.idProduct),
> + dev->descriptor.bNumConfigurations,
> + dev->actconfig->desc.bConfigurationValue);
> +
> + /* create device structure */
> + mxdev = kzalloc(sizeof(struct mxu1_device), GFP_KERNEL);
> + if (mxdev == NULL)
Use !mxdev.
> + return -ENOMEM;
> +
> + mutex_init(&mxdev->mxd_lock);
> + mxdev->mxd_serial = serial;
These two are not needed as mentioned above.
> + usb_set_serial_data(serial, mxdev);
> +
> + mxdev->mxd_model = le16_to_cpu(dev->descriptor.idProduct);
> +
> + cur_altsetting = serial->interface->cur_altsetting;
> +
> + /* if we have only 1 configuration, download firmware */
> + if (cur_altsetting->desc.bNumEndpoints == 1) {
> +
> + snprintf(fw_name,
> + sizeof(fw_name),
> + "moxa/moxa-%04x.fw",
> + mxdev->mxd_model);
> +
> + err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
> + if (err) {
> + dev_err(&serial->interface->dev, "firmware %s not found\n",
> + fw_name);
Be a little less specific here (e.g. "failed to request firmware: %d\n" and
include the errno.
> + kfree(mxdev);
> + return err;
> + }
> +
> + err = mxu1_download_firmware(serial, fw_p);
> + if (err) {
> + kfree(mxdev);
> + return err;
You're leaking the firmware memory here.
> + }
> +
> + status = -ENODEV;
> + release_firmware(fw_p);
> + }
> +
> + return status;
> +}
> +static void mxu1_set_termios(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> +{
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> + struct mxu1_uart_config *config;
> + tcflag_t cflag, iflag;
> + speed_t baud;
> + int status;
> + unsigned int mcr;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + cflag = tty->termios.c_cflag;
> + iflag = tty->termios.c_iflag;
> +
> + if (old_termios &&
> + !tty_termios_hw_change(&tty->termios, old_termios) &&
> + tty->termios.c_iflag == old_termios->c_iflag) {
> + dev_dbg(&port->dev, "%s - nothing to change\n", __func__);
> + return;
> + }
> +
> + dev_dbg(&port->dev,
> + "%s - clfag %08x, iflag %08x\n", __func__, cflag, iflag);
> +
> + if (old_termios) {
> + dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x\n",
> + __func__,
> + old_termios->c_cflag,
> + old_termios->c_iflag);
> + }
> +
> + config = kzalloc(sizeof(*config), GFP_KERNEL);
> + if (!config)
> + return;
> +
> + config->wFlags = 0;
You can rely on your kzalloc just above.
> +
> + /* these flags must be set */
> + config->wFlags |= MXU1_UART_ENABLE_MS_INTS;
> + config->wFlags |= MXU1_UART_ENABLE_AUTO_START_DMA;
> + if (mxport->mxp_send_break == MXU1_LCR_BREAK)
> + config->wFlags |= MXU1_UART_SEND_BREAK_SIGNAL;
> + config->bUartMode = (u8)(mxport->mxp_uart_mode);
No need to cast.
> +static int mxu1_ioctl_set_rs485(struct mxu1_port *mxport,
> + struct serial_rs485 __user *rs485_user)
> +{
> + struct serial_rs485 rs485;
> + struct usb_serial_port *port = mxport->mxp_port;
> + struct mxu1_device *mxdev;
> +
> + mxdev = usb_get_serial_data(port->serial);
> +
> + if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
> + return -EFAULT;
> +
> + if (mxport->mxp_uart_types & (MXU1_TYPE_RS422 | MXU1_TYPE_RS485)) {
> +
> + if (rs485.flags & SER_RS485_ENABLED) {
> + mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_ENABLED;
> + } else {
> + if (mxport->mxp_uart_types & MXU1_TYPE_RS232)
> + mxport->mxp_uart_mode = MXU1_UART_232;
> + else
> + mxport->mxp_uart_mode =
> + MXU1_UART_485_RECEIVER_DISABLED;
> + }
> + } else {
> + dev_err(&port->dev, "%s not handled by MOXA UPort %04x\n",
> + __func__, mxdev->mxd_model);
Could you just spell this out as something like "RS485 not supported by
this device\n"?
> + return -EINVAL;
> + }
You also need to revert any bits not supported and return the new config
to user space. Take a look at uart_set_rs485_config in serial_core.
> +
> + return 0;
> +}
> +static int mxu1_tiocmget(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> + unsigned int result;
> + unsigned int msr;
> + unsigned int mcr;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
Drop this.
> +
> + mutex_lock(&mxport->mxp_mutex);
> + spin_lock_irqsave(&mxport->mxp_lock, flags);
> +
> + msr = mxport->mxp_msr;
> + mcr = mxport->mxp_mcr;
> +
> + spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> + mutex_unlock(&mxport->mxp_mutex);
> +
> + result = ((mcr & MXU1_MCR_DTR) ? TIOCM_DTR : 0) |
> + ((mcr & MXU1_MCR_RTS) ? TIOCM_RTS : 0) |
> + ((mcr & MXU1_MCR_LOOP) ? TIOCM_LOOP : 0) |
> + ((msr & MXU1_MSR_CTS) ? TIOCM_CTS : 0) |
> + ((msr & MXU1_MSR_CD) ? TIOCM_CAR : 0) |
> + ((msr & MXU1_MSR_RI) ? TIOCM_RI : 0) |
> + ((msr & MXU1_MSR_DSR) ? TIOCM_DSR : 0);
> +
> + dev_dbg(&port->dev, "%s - 0x%04X\n", __func__, result);
> +
> + return result;
> +}
> +
> +static int mxu1_tiocmset(struct tty_struct *tty,
> + unsigned int set, unsigned int clear)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> + int err;
> + unsigned int mcr;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
Ditto.
> +
> + mutex_lock(&mxport->mxp_mutex);
> + mcr = mxport->mxp_mcr;
> +
> + if (set & TIOCM_RTS)
> + mcr |= MXU1_MCR_RTS;
> + if (set & TIOCM_DTR)
> + mcr |= MXU1_MCR_DTR;
> + if (set & TIOCM_LOOP)
> + mcr |= MXU1_MCR_LOOP;
> +
> + if (clear & TIOCM_RTS)
> + mcr &= ~MXU1_MCR_RTS;
> + if (clear & TIOCM_DTR)
> + mcr &= ~MXU1_MCR_DTR;
> + if (clear & TIOCM_LOOP)
> + mcr &= ~MXU1_MCR_LOOP;
> +
> + err = mxu1_set_mcr(port, mcr);
> + if (!err)
> + mxport->mxp_mcr = mcr;
> +
> + mutex_unlock(&mxport->mxp_mutex);
> +
> + return err;
> +}
> +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + struct mxu1_device *mxdev;
> + struct mxu1_port *mxport;
> + struct usb_device *dev;
> + struct urb *urb;
> + int status;
> + u16 open_settings;
> +
> + open_settings = (MXU1_PIPE_MODE_CONTINUOUS |
> + MXU1_PIPE_TIMEOUT_ENABLE |
> + (MXU1_TRANSFER_TIMEOUT << 2));
> +
> + mxdev = usb_get_serial_data(port->serial);
You don't need mxdev; you already have port->serial, and the interrupt
urb already has the usb_serial_port set as context.
> + mxport = usb_get_serial_port_data(port);
> +
> + dev = port->serial->dev;
> +
> + mxport->mxp_msr = 0;
> + mxport->mxp_mcr |= MXU1_MCR_RTS | MXU1_MCR_DTR;
These will be raised by tty core using dtr_rts, so drop this.
> +
> + dev_dbg(&port->dev, "%s - start interrupt in urb\n", __func__);
> + urb = port->interrupt_in_urb;
> + urb->context = mxdev;
> + status = usb_submit_urb(urb, GFP_KERNEL);
> + if (status) {
> + dev_err(&port->dev, "failed to submit interrupt urb: %d\n",
> + status);
> + return status;
> + }
> +
> + mxu1_set_termios(tty, port, NULL);
tty may be NULL if this port used as a console, so only call set_termios
if tty is non-NULL.
> +
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> + open_settings, MXU1_UART1_PORT);
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send open command: %d\n",
> + __func__, status);
> + goto unlink_int_urb;
> + }
> +
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> + 0, MXU1_UART1_PORT);
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send start command: %d\n",
> + __func__, status);
> + goto unlink_int_urb;
> + }
> +
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> + MXU1_PURGE_INPUT, MXU1_UART1_PORT);
> + if (status) {
> + dev_err(&port->dev, "%s - cannot clear input buffers: %d\n",
> + __func__, status);
> +
> + goto unlink_int_urb;
> + }
> +
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> + MXU1_PURGE_OUTPUT, MXU1_UART1_PORT);
> + if (status) {
> + dev_err(&port->dev, "%s - cannot clear output buffers: %d\n",
> + __func__, status);
> +
> + goto unlink_int_urb;
> + }
> +
> + /*
> + * reset the data toggle on the bulk endpoints to work around bug in
> + * host controllers where things get out of sync some times
> + */
> + usb_clear_halt(dev, port->write_urb->pipe);
> + usb_clear_halt(dev, port->read_urb->pipe);
> +
> + mxu1_set_termios(tty, port, NULL);
> +
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> + open_settings, MXU1_UART1_PORT);
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send open command: %d\n",
> + __func__, status);
> + goto unlink_int_urb;
> + }
> +
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> + 0, MXU1_UART1_PORT);
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send start command: %d\n",
> + __func__, status);
> + goto unlink_int_urb;
> + }
> +
> + status = usb_serial_generic_open(tty, port);
> + if (status) {
> + dev_err(&port->dev, "%s - submit read urb failed: %d\n",
> + __func__, status);
> + goto unlink_int_urb;
> + }
> +
> + return status;
> +
> +unlink_int_urb:
> + usb_kill_urb(port->interrupt_in_urb);
> +
> + return status;
> +}
> +
> +static void mxu1_close(struct usb_serial_port *port)
> +{
> + int status;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
Drop this.
> +
> + usb_serial_generic_close(port);
> + usb_kill_urb(port->interrupt_in_urb);
> +
> + status = mxu1_send_ctrl_urb(port->serial, MXU1_CLOSE_PORT,
> + 0, MXU1_UART1_PORT);
> + if (status)
> + dev_err(&port->dev, "failed to send close port command: %d\n",
> + status);
> +}
> +
> +static void mxu1_handle_new_msr(struct usb_serial_port *port,
> + struct mxu1_port *mxport, u8 msr)
> +{
> + struct async_icount *icount;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr);
> +
> + if (msr & MXU1_MSR_DELTA_MASK) {
> + icount = &mxport->mxp_port->icount;
You already have the port structure.
> + if (msr & MXU1_MSR_DELTA_CTS)
> + icount->cts++;
> + if (msr & MXU1_MSR_DELTA_DSR)
> + icount->dsr++;
> + if (msr & MXU1_MSR_DELTA_CD)
> + icount->dcd++;
> + if (msr & MXU1_MSR_DELTA_RI)
> + icount->rng++;
> +
> + wake_up_interruptible(&port->port.delta_msr_wait);
> + }
> +
> + spin_lock_irqsave(&mxport->mxp_lock, flags);
> + mxport->mxp_msr = msr & MXU1_MSR_MASK;
> + spin_unlock_irqrestore(&mxport->mxp_lock, flags);
You want to do this before testing the delta and waking up any waiters.
> +}
> +
> +static void mxu1_interrupt_callback(struct urb *urb)
> +{
> + struct mxu1_device *mxdev = urb->context;
> + struct usb_serial_port *port;
> + struct usb_serial *serial = mxdev->mxd_serial;
> + struct mxu1_port *mxport;
> + unsigned char *data = urb->transfer_buffer;
> + int length = urb->actual_length;
> + int function;
> + int status;
> + u8 msr;
> +
> + port = serial->port[0];
> + mxport = usb_get_serial_port_data(port);
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
Drop this.
> +
> + switch (urb->status) {
> + case 0:
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_dbg(&port->dev, "%s - urb shutting down: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
> + __func__, urb->status);
> + goto exit;
> + }
> +
> + if (length != 2) {
> + dev_dbg(&port->dev, "%s - bad packet size: %d\n",
> + __func__, length);
> + goto exit;
> + }
> +
> + if (data[0] == MXU1_CODE_HARDWARE_ERROR) {
> + dev_err(&port->dev, "%s - hardware error: %d\n",
> + __func__, data[1]);
> + goto exit;
> + }
> +
> + function = mxu1_get_func_from_code(data[0]);
> +
> + dev_dbg(&port->dev, "%s - function %d, data 0x%02X\n",
> + __func__, function, data[1]);
> +
> + switch (function) {
> + case MXU1_CODE_DATA_ERROR:
> + dev_dbg(&port->dev, "%s - DATA ERROR, data 0x%02X\n",
> + __func__, data[1]);
> + break;
> +
> + case MXU1_CODE_MODEM_STATUS:
> + msr = data[1];
> + dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr);
You print the same debug info in mxu1_handle_new_msr.
> + mxu1_handle_new_msr(port, mxport, msr);
> + break;
> +
> + default:
> + dev_err(&port->dev, "%s - unknown interrupt code: 0x%02X\n",
> + __func__, data[1]);
> + break;
> + }
> +
> +exit:
> + status = usb_submit_urb(urb, GFP_ATOMIC);
> + if (status)
> + dev_err(&port->dev, "%s - resubmit interrupt urb failed: %d\n",
> + __func__, status);
No need to include the function name.
> +}
Overall this looks really good now.
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
