Date:	Mon, 28 Dec 2015 15:06:30 +0100
From:	Johan Hovold <johan@...nel.org>
To:	Mathieu OTHACEHE <m.othacehe@...il.com>
Cc:	johan@...nel.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v5] USB: serial: add Moxa UPORT 11x0 driver

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 <m.othacehe@...il.com>
> ---
> 
> 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 <m.othacehe@...il.com>");
> +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists