Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752143AbbL1OGA (ORCPT ); Mon, 28 Dec 2015 09:06:00 -0500 Received: from mail-lf0-f41.google.com ([209.85.215.41]:35877 "EHLO mail-lf0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750912AbbL1OF5 (ORCPT ); Mon, 28 Dec 2015 09:05:57 -0500 Date: Mon, 28 Dec 2015 15:06:30 +0100 From: Johan Hovold To: Mathieu OTHACEHE Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH v5] USB: serial: add Moxa UPORT 11x0 driver Message-ID: <20151228140630.GA18146@localhost> References: <1449401383-10047-1-git-send-email-m.othacehe@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449401383-10047-1-git-send-email-m.othacehe@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15570 Lines: 542 On Sun, Dec 06, 2015 at 12:29:43PM +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 > --- > > Hi Johan, > > Thanks for your patience, here is v5, Thanks for the update. The code itself is in a state ready to be merged, but I have some concerns about the transceiver-mode interface and also a question about firmware availability. > +/* Operation modes */ > +#define MXU1_UART_232 0x00 > +#define MXU1_UART_485_RECEIVER_DISABLED 0x01 > +#define MXU1_UART_485_RECEIVER_ENABLED 0x02 These modes correspond to RS-232, RS-485-2W and RS-422/RS-485-4W. It is known that the TIOCSRS485-interface is not generic enough as it is essentially limited to switching between full and half-duplex mode. There has been been discussions about extending it, see for example this thread from 2011: https://lkml.kernel.org/r/201102171210.03474.frode@tennebo.com However, the TIOCSRS485-ioctl might not even be the right interface for changing transceiver modes, and instead a sysfs-based usb-serial-specific interface could be more appropriate for example. This is a can that we've kicked down the road for a very long time now, and we really need to reach some sort of conclusion on this. There are a few usb-serial drivers in mainline for devices that supports other modes than RS232 and there are out-of-tree drivers that have cooked up their own interfaces (e.g. Moxa). Before we have resolved this, we could merge this driver without support for switching modes as we did for mxuport. If you prefer this, then just set RS232 for 1110 and 1150, pick a good default mode for 1130 and drop the RS485 ioctls for now. > +/* Supported operation types */ > +#define MXU1_TYPE_RS232 BIT(0) > +#define MXU1_TYPE_RS422 BIT(1) > +#define MXU1_TYPE_RS485 BIT(2) > +/* Config struct */ > +struct mxu1_uart_config { > + __be16 wBaudRate; > + __be16 wFlags; > + u8 bDataBits; > + u8 bParity; > + u8 bStopBits; > + char cXon; > + char cXoff; > + u8 bUartMode; > +} __packed; > +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) { > + dev_err(&port->dev, "%s - no interrupt urb\n", __func__); > + return -EINVAL; > + } > + > + switch (mxdev->mxd_model) { > + case MXU1_1110_PRODUCT_ID: > + mxport->uart_types = MXU1_TYPE_RS232; > + mxport->uart_mode = MXU1_UART_232; > + break; > + case MXU1_1130_PRODUCT_ID: > + case MXU1_1131_PRODUCT_ID: > + mxport->uart_types = MXU1_TYPE_RS422 | MXU1_TYPE_RS485; > + mxport->uart_mode = MXU1_UART_485_RECEIVER_DISABLED; > + break; > + case MXU1_1150_PRODUCT_ID: > + case MXU1_1151_PRODUCT_ID: > + mxport->uart_types = > + MXU1_TYPE_RS232 | MXU1_TYPE_RS422 | MXU1_TYPE_RS485; > + mxport->uart_mode = MXU1_UART_232; > + 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) { > + kfree(mxdev); > + release_firmware(fw_p); Please release the firmware before the state container to reverse allocation. > + return err; > + } > + > + 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; > + > + 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; > + > + /* these flags must be set */ > + config->wFlags |= MXU1_UART_ENABLE_MS_INTS; > + config->wFlags |= MXU1_UART_ENABLE_AUTO_START_DMA; > + if (mxport->send_break) > + config->wFlags |= MXU1_UART_SEND_BREAK_SIGNAL; > + config->bUartMode = mxport->uart_mode; > + > + switch (C_CSIZE(tty)) { > + case CS5: > + config->bDataBits = MXU1_UART_5_DATA_BITS; > + break; > + case CS6: > + config->bDataBits = MXU1_UART_6_DATA_BITS; > + break; > + case CS7: > + config->bDataBits = MXU1_UART_7_DATA_BITS; > + break; > + default: > + case CS8: > + config->bDataBits = MXU1_UART_8_DATA_BITS; > + break; > + } > + > + if (C_PARENB(tty)) { > + config->wFlags |= MXU1_UART_ENABLE_PARITY_CHECKING; > + if (C_CMSPAR(tty)) { > + if (C_PARODD(tty)) > + config->bParity = MXU1_UART_MARK_PARITY; > + else > + config->bParity = MXU1_UART_SPACE_PARITY; > + } else { > + if (C_PARODD(tty)) > + config->bParity = MXU1_UART_ODD_PARITY; > + else > + config->bParity = MXU1_UART_EVEN_PARITY; > + } > + } else { > + config->bParity = MXU1_UART_NO_PARITY; > + } > + > + if (C_CSTOPB(tty)) > + config->bStopBits = MXU1_UART_2_STOP_BITS; > + else > + config->bStopBits = MXU1_UART_1_STOP_BITS; > + > + if (C_CRTSCTS(tty)) { > + /* RTS flow control must be off to drop RTS for baud rate B0 */ > + if (C_BAUD(tty) != B0) > + config->wFlags |= MXU1_UART_ENABLE_RTS_IN; > + config->wFlags |= MXU1_UART_ENABLE_CTS_OUT; > + } > + > + if (I_IXOFF(tty) || I_IXON(tty)) { > + config->cXon = START_CHAR(tty); > + config->cXoff = STOP_CHAR(tty); > + > + if (I_IXOFF(tty)) > + config->wFlags |= MXU1_UART_ENABLE_X_IN; > + > + if (I_IXON(tty)) > + config->wFlags |= MXU1_UART_ENABLE_X_OUT; > + } > + > + baud = tty_get_baud_rate(tty); > + if (!baud) > + baud = 9600; > + config->wBaudRate = MXU1_BAUD_BASE / baud; > + > + dev_dbg(&port->dev, "%s - BaudRate=%d, wBaudRate=%d, wFlags=0x%04X, bDataBits=%d, bParity=%d, bStopBits=%d, cXon=%d, cXoff=%d, bUartMode=%d\n", > + __func__, baud, config->wBaudRate, config->wFlags, > + config->bDataBits, config->bParity, config->bStopBits, > + config->cXon, config->cXoff, config->bUartMode); > + > + cpu_to_be16s(&config->wBaudRate); > + cpu_to_be16s(&config->wFlags); > + > + status = mxu1_send_ctrl_data_urb(port->serial, MXU1_SET_CONFIG, 0, > + MXU1_UART1_PORT, config, > + sizeof(*config)); > + if (status) > + dev_err(&port->dev, "cannot set config: %d\n", status); > + > + mutex_lock(&mxport->mutex); > + mcr = mxport->mcr; > + > + 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); > + > + status = mxu1_set_mcr(port, mcr); > + if (status) > + dev_err(&port->dev, "cannot set modem control: %d\n", status); > + else > + mxport->mcr = mcr; > + > + mutex_unlock(&mxport->mutex); > + > + kfree(config); > +} > + > +static int mxu1_ioctl_get_rs485(struct mxu1_port *mxport, > + struct serial_rs485 __user *rs485) > +{ > + struct serial_rs485 aux; > + > + memset(&aux, 0, sizeof(aux)); > + > + if (mxport->uart_mode == MXU1_UART_485_RECEIVER_ENABLED) > + aux.flags = SER_RS485_ENABLED; With the current ioctl it's not possible to discern RS422 from RS232 mode. > + > + if (copy_to_user(rs485, &aux, sizeof(aux))) > + return -EFAULT; > + > + return 0; > +} > + > +static int mxu1_ioctl_set_rs485(struct usb_serial_port *port, > + struct mxu1_port *mxport, > + struct serial_rs485 __user *rs485_user) > +{ > + struct serial_rs485 rs485; > + int status = 0; > + > + if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user))) > + return -EFAULT; > + > + if (mxport->uart_types & (MXU1_TYPE_RS422 | MXU1_TYPE_RS485)) { > + > + if (rs485.flags & SER_RS485_ENABLED) { > + mxport->uart_mode = MXU1_UART_485_RECEIVER_ENABLED; > + } else { > + if (mxport->uart_types & MXU1_TYPE_RS232) > + mxport->uart_mode = MXU1_UART_232; > + else > + mxport->uart_mode = > + MXU1_UART_485_RECEIVER_DISABLED; > + } > + } else { > + dev_err(&port->dev, "RS485 not supported by this device\n"); > + status = -EINVAL; > + } And there's currently no way to set RS485-2W for 1150 using this interface. > + > + memset(&rs485, 0, sizeof(rs485)); > + if (mxport->uart_mode & MXU1_UART_485_RECEIVER_ENABLED) > + rs485.flags = SER_RS485_ENABLED; > + > + if (copy_to_user(rs485_user, &rs485, sizeof(rs485))) > + return -EFAULT; > + > + return status; > +} > +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; > + struct urb *urb; > + 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__); > + urb = port->interrupt_in_urb; > + status = usb_submit_urb(urb, GFP_KERNEL); Just use port->interrupt_in_urb directly here and drop the urb pointer. > + 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; > + } > + > + 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_handle_new_msr(struct usb_serial_port *port, > + struct mxu1_port *mxport, u8 msr) Just pass the usb-serial port and use usb_get_serial_port_data to access the state container. >+{ > + struct async_icount *icount; > + unsigned long flags; > + > + dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr); > + > + spin_lock_irqsave(&mxport->spinlock, flags); > + mxport->msr = msr & MXU1_MSR_MASK; > + spin_unlock_irqrestore(&mxport->spinlock, flags); > + > + if (msr & MXU1_MSR_DELTA_MASK) { > + icount = &port->icount; > + 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); > + } > +} > + > +static void mxu1_interrupt_callback(struct urb *urb) > +{ > + struct usb_serial_port *port = urb->context; > + struct mxu1_port *mxport; > + unsigned char *data = urb->transfer_buffer; > + int length = urb->actual_length; > + int function; > + int status; > + u8 msr; > + > + mxport = usb_get_serial_port_data(port); Then you can drop this too. > +MODULE_AUTHOR("Mathieu Othacehe "); > +MODULE_DESCRIPTION("MOXA UPort 11x0 USB to Serial Hub Driver"); > +MODULE_LICENSE("GPL"); > +MODULE_FIRMWARE("moxa/moxa-1110.fw"); > +MODULE_FIRMWARE("moxa/moxa-1130.fw"); > +MODULE_FIRMWARE("moxa/moxa-1131.fw"); > +MODULE_FIRMWARE("moxa/moxa-1150.fw"); > +MODULE_FIRMWARE("moxa/moxa-1151.fw"); Where can I find these firmware images? Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/