On Mon, Dec 28, 2015 at 09:21:25PM +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.othac...@gmail.com> > --- > > Hi Johan, > > Here is the v6 of the driver. > > I understand the problems with the TIOCSRS485/TIOCGRS485 ioctls in > this driver. > For now, I prefer dropping mode switching support from the driver as > you suggested. > > So UPORT 1110 and 1150 only support RS232 and UPORT 1130 only support > RS-485-2W. > If a new interface is developped, I'll restore mode switching code.
Sounds good. > About firmware images, I just sent a patch to linux-firmware. Here is the > link : > https://lkml.org/lkml/2015/12/28/239 Thanks, I've done some quick tests using a 1150 now. When looking through the code one last time I found a few issues that I fixed up. I'll submit patches for these to the USB list. But there are couple of things you could consider to do as follow ups as well. Details below. > +static int mxu1_port_probe(struct usb_serial_port *port) > +{ > + struct mxu1_port *mxport; > + struct mxu1_device *mxdev; > + struct urb *urb; > + > + mxport = kzalloc(sizeof(struct mxu1_port), GFP_KERNEL); > + if (!mxport) > + return -ENOMEM; > + > + spin_lock_init(&mxport->spinlock); > + mutex_init(&mxport->mutex); > + > + mxdev = usb_get_serial_data(port->serial); > + > + urb = port->interrupt_in_urb; > + if (!urb) { You are leaking the port private data here (fixed). > + dev_err(&port->dev, "%s - no interrupt urb\n", __func__); > + return -EINVAL; > + } > + > + switch (mxdev->mxd_model) { > + case MXU1_1110_PRODUCT_ID: > + case MXU1_1150_PRODUCT_ID: > + case MXU1_1151_PRODUCT_ID: > + mxport->uart_mode = MXU1_UART_232; > + break; > + case MXU1_1130_PRODUCT_ID: > + case MXU1_1131_PRODUCT_ID: > + mxport->uart_mode = MXU1_UART_485_RECEIVER_DISABLED; > + break; > + } > + > + usb_set_serial_port_data(port, mxport); > + > + port->port.closing_wait = > + msecs_to_jiffies(MXU1_DEFAULT_CLOSING_WAIT * 10); > + port->port.drain_delay = 1; > + > + 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(&serial->interface->dev, "%s - product 0x%04X, num > configurations %d, configuration value %d\n", > + __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) > + return -ENOMEM; > + > + 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, "failed to request > firmware: %d\n", > + err); > + kfree(mxdev); > + return err; > + } > + > + err = mxu1_download_firmware(serial, fw_p); > + if (err) { > + release_firmware(fw_p); > + kfree(mxdev); > + return err; > + } > + > + status = -ENODEV; > + release_firmware(fw_p); And you're leaking the interface private data here as well (also fixed). What you could consider doing as a follow up it so move both the interrupt-in urb test in port_probe and the firmware download to a new probe callback. That way you avoid the unnecessary memory allocations done by core before attach is called, and you can verify and refuse to bind to a device in case the expected endpoints are missing. > + } > + > + return status; > +} > + if (C_BAUD(tty) == B0) > + mcr &= ~(MXU1_MCR_DTR | MXU1_MCR_RTS); > + else if (old_termios && (old_termios->c_cflag & CBAUD) == B0) > + mcr |= ~(MXU1_MCR_DTR | MXU1_MCR_RTS); This should have been mcr |= MXU1_MCR_DTR | MXU1_MCR_RTS; Also fixed. > +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port) > +{ > + struct mxu1_port *mxport = usb_get_serial_port_data(port); > + struct usb_serial *serial = port->serial; > + int status; > + u16 open_settings; > + > + open_settings = (MXU1_PIPE_MODE_CONTINUOUS | > + MXU1_PIPE_TIMEOUT_ENABLE | > + (MXU1_TRANSFER_TIMEOUT << 2)); > + > + mxport->msr = 0; > + > + dev_dbg(&port->dev, "%s - start interrupt in urb\n", __func__); > + status = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL); > + if (status) { > + dev_err(&port->dev, "failed to submit interrupt urb: %d\n", > + status); > + return status; > + } > + > + if (tty) > + mxu1_set_termios(tty, port, NULL); > + > + status = mxu1_send_ctrl_urb(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(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(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(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(serial->dev, port->write_urb->pipe); > + usb_clear_halt(serial->dev, port->read_urb->pipe); > + > + if (tty) > + mxu1_set_termios(tty, port, NULL); > + > + status = mxu1_send_ctrl_urb(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(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; > + } I noticed that you send OPEN and START twice during probe -- is that really necessary? > + 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; > + > + 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); And should there not be a matching STOP request at close? > +static struct usb_serial_driver mxuport11_device = { > + .driver = { > + .owner = THIS_MODULE, > + .name = "mxuport11", I also decided to rename the usb-serial driver mxu11x0 to match the module and USB driver name. I have applied the patch now and will post my fixes and a couple of clean ups to the list. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
