[PATCH] USB: serial: cp210x: Cleaned up USB access functions.

[Konstantin Shkolnyy]

cp210x_get_config and cp210x_set_config were hard to use. They required
the buffer as an array of 32-bit values even for smaller values, and did
endian conversions on per-32-bit value basis, which is wrong for some
cp210x data structures (although not for any that were actually used.)
This change introduces separate register accessor functions for single
8, 16 and 32-bit values, with endian conversion, as well as "block"
access functions without conversion.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@xxxxxxxxx>
---
 drivers/usb/serial/cp210x.c | 314 ++++++++++++++++++++++++++------------------
 1 file changed, 186 insertions(+), 128 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index fd67958..ce80d5f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -323,113 +323,169 @@ struct cp210x_comm_status {
 #define PURGE_ALL		0x000f
 
 /*
- * cp210x_get_config
- * Reads from the CP210x configuration registers
- * 'size' is specified in bytes.
- * 'data' is a pointer to a pre-allocated array of integers large
- * enough to hold 'size' bytes (with 4 bytes to each integer)
+ * Reads a variable-sized block of CP210X_ registers, identified by req.
+ * Returns data into buf in native USB byte order.
  */
-static int cp210x_get_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
+static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
+		void *buf, int bufsize)
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	__le32 *buf;
-	int result, i, length;
-
-	/* Number of integers required to contain the array */
-	length = (((size - 1) | 3) + 1) / 4;
+	void *dmabuf;
+	int result;
 
-	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
-	if (!buf)
+	dmabuf = kmalloc(bufsize, GFP_KERNEL);
+	if (!dmabuf) {
+		/*
+		 * FIXME Some callers don't bother to check for error,
+		 * at least give them consistent junk until they are fixed
+		 */
+		memset(buf, 0, bufsize);
 		return -ENOMEM;
+	}
 
-	/* Issue the request, attempting to read 'size' bytes */
 	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-				request, REQTYPE_INTERFACE_TO_HOST, 0x0000,
-				port_priv->bInterfaceNumber, buf, size,
-				USB_CTRL_GET_TIMEOUT);
+			req, REQTYPE_INTERFACE_TO_HOST, 0,
+			port_priv->bInterfaceNumber, dmabuf, bufsize,
+			USB_CTRL_SET_TIMEOUT);
+	if (result == bufsize) {
+		memcpy(buf, dmabuf, bufsize);
+		result = 0;
+	} else {
+		dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
+				req, bufsize, result);
+		if (result >= 0)
+			result = -EPROTO;
 
-	/* Convert data into an array of integers */
-	for (i = 0; i < length; i++)
-		data[i] = le32_to_cpu(buf[i]);
+		/*
+		 * FIXME Some callers don't bother to check for error,
+		 * at least give them consistent junk until they are fixed
+		 */
+		memset(buf, 0, bufsize);
+	}
 
-	kfree(buf);
+	kfree(dmabuf);
 
-	if (result != size) {
-		dev_dbg(&port->dev, "%s - Unable to send config request, request=0x%x size=%d result=%d\n",
-			__func__, request, size, result);
-		if (result > 0)
-			result = -EPROTO;
+	return result;
+}
+
+/*
+ * Reads any 32-bit CP210X_ register identified by req.
+ */
+static int cp210x_read_u32_reg(struct usb_serial_port *port, u8 req, u32 *val)
+{
+	__le32 le32_val;
+	int err;
 
-		return result;
+	err = cp210x_read_reg_block(port, req, &le32_val, sizeof(le32_val));
+	if (err) {
+		/*
+		 * FIXME Some callers don't bother to check for error,
+		 * at least give them consistent junk until they are fixed
+		 */
+		*val = 0;
+		return err;
 	}
 
+	*val = le32_to_cpu(le32_val);
+
 	return 0;
 }
 
 /*
- * cp210x_set_config
- * Writes to the CP210x configuration registers
- * Values less than 16 bits wide are sent directly
- * 'size' is specified in bytes.
+ * Reads any 16-bit CP210X_ register identified by req.
  */
-static int cp210x_set_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
+static int cp210x_read_u16_reg(struct usb_serial_port *port, u8 req, u16 *val)
+{
+	__le16 le16_val;
+	int err;
+
+	err = cp210x_read_reg_block(port, req, &le16_val, sizeof(le16_val));
+	if (err)
+		return err;
+
+	*val = le16_to_cpu(le16_val);
+
+	return 0;
+}
+
+/*
+ * Reads any 8-bit CP210X_ register identified by req.
+ */
+static int cp210x_read_u8_reg(struct usb_serial_port *port, u8 req, u8 *val)
+{
+	return cp210x_read_reg_block(port, req, val, sizeof(*val));
+}
+
+/*
+ * Writes any 16-bit CP210X_ register (req) whose value is passed
+ * entirely in the wValue field of the USB request.
+ */
+static int cp210x_write_u16_reg(struct usb_serial_port *port, u8 req, u16 val)
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	__le32 *buf;
-	int result, i, length;
+	int result;
 
-	/* Number of integers required to contain the array */
-	length = (((size - 1) | 3) + 1) / 4;
+	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
+			req, REQTYPE_HOST_TO_INTERFACE, val,
+			port_priv->bInterfaceNumber, NULL, 0,
+			USB_CTRL_SET_TIMEOUT);
+	if (result < 0) {
+		dev_err(&port->dev, "failed set request 0x%x status: %d\n",
+				req, result);
+	}
 
-	buf = kmalloc(length * sizeof(__le32), GFP_KERNEL);
-	if (!buf)
+	return result;
+}
+
+/*
+ * Writes a variable-sized block of CP210X_ registers, identified by req.
+ * Data in buf must be in native USB byte order.
+ */
+static int cp210x_write_reg_block(struct usb_serial_port *port, u8 req,
+		void *buf, int bufsize)
+{
+	struct usb_serial *serial = port->serial;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	void *dmabuf;
+	int result;
+
+	dmabuf = kmalloc(bufsize, GFP_KERNEL);
+	if (!dmabuf)
 		return -ENOMEM;
 
-	/* Array of integers into bytes */
-	for (i = 0; i < length; i++)
-		buf[i] = cpu_to_le32(data[i]);
+	memcpy(dmabuf, buf, bufsize);
 
-	if (size > 2) {
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_INTERFACE, 0x0000,
-				port_priv->bInterfaceNumber, buf, size,
-				USB_CTRL_SET_TIMEOUT);
-	} else {
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_INTERFACE, data[0],
-				port_priv->bInterfaceNumber, NULL, 0,
-				USB_CTRL_SET_TIMEOUT);
-	}
+	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
+			req, REQTYPE_HOST_TO_INTERFACE, 0,
+			port_priv->bInterfaceNumber, dmabuf, bufsize,
+			USB_CTRL_SET_TIMEOUT);
 
-	kfree(buf);
+	kfree(dmabuf);
 
-	if ((size > 2 && result != size) || result < 0) {
-		dev_dbg(&port->dev, "%s - Unable to send request, request=0x%x size=%d result=%d\n",
-			__func__, request, size, result);
-		if (result > 0)
+	if (result == bufsize) {
+		result = 0;
+	} else {
+		dev_err(&port->dev, "failed set req 0x%x size %d status: %d\n",
+				req, bufsize, result);
+		if (result >= 0)
 			result = -EPROTO;
-
-		return result;
 	}
 
-	return 0;
+	return result;
 }
 
 /*
- * cp210x_set_config_single
- * Convenience function for calling cp210x_set_config on single data values
- * without requiring an integer pointer
+ * Writes any 32-bit CP210X_ register identified by req.
  */
-static inline int cp210x_set_config_single(struct usb_serial_port *port,
-		u8 request, unsigned int data)
+static int cp210x_write_u32_reg(struct usb_serial_port *port, u8 req, u32 val)
 {
-	return cp210x_set_config(port, request, &data, 2);
+	__le32 le32_val;
+
+	le32_val = cpu_to_le32(val);
+
+	return cp210x_write_reg_block(port, req, &le32_val, sizeof(le32_val));
 }
 
 /*
@@ -441,48 +497,47 @@ static inline int cp210x_set_config_single(struct usb_serial_port *port,
 static int cp210x_detect_swapped_line_ctl(struct usb_serial_port *port)
 {
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-	unsigned int line_ctl_save;
-	unsigned int line_ctl_test;
+	u16 line_ctl_save;
+	u16 line_ctl_test;
 	int err;
 
-	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_save, 2);
+	err = cp210x_read_u16_reg(port, CP210X_GET_LINE_CTL, &line_ctl_save);
 	if (err)
 		return err;
 
-	line_ctl_test = 0x800;
-	err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_test, 2);
+	err = cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, 0x800);
 	if (err)
 		return err;
 
-	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_test, 2);
+	err = cp210x_read_u16_reg(port, CP210X_GET_LINE_CTL, &line_ctl_test);
 	if (err)
 		return err;
 
 	/* has_swapped_line_ctl is 0 here because port_priv was kzalloced */
 	if (line_ctl_test == 8) {
 		port_priv->has_swapped_line_ctl = true;
-		line_ctl_save = swab16((u16)line_ctl_save);
+		line_ctl_save = swab16(line_ctl_save);
 	}
 
-	return cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_save, 2);
+	return cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, line_ctl_save);
 }
 
 /*
- * Must always be called instead of cp210x_get_config(CP210X_GET_LINE_CTL)
+ * Must always be called instead of cp210x_read_u16_reg(CP210X_GET_LINE_CTL)
  * to workaround cp2108 bug and get correct value.
  */
-static int cp210x_get_line_ctl(struct usb_serial_port *port, unsigned int *ctl)
+static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
 {
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
 	int err;
 
-	err = cp210x_get_config(port, CP210X_GET_LINE_CTL, ctl, 2);
+	err = cp210x_read_u16_reg(port, CP210X_GET_LINE_CTL, ctl);
 	if (err)
 		return err;
 
 	/* Workaround swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
 	if (port_priv->has_swapped_line_ctl)
-		*ctl = swab16((u16)(*ctl));
+		*ctl = swab16(*ctl);
 
 	return 0;
 }
@@ -531,14 +586,11 @@ static unsigned int cp210x_quantise_baudrate(unsigned int baud)
 
 static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
-	int result;
+	int err;
 
-	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
-								UART_ENABLE);
-	if (result) {
-		dev_err(&port->dev, "%s - Unable to enable UART\n", __func__);
-		return result;
-	}
+	err = cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_ENABLE);
+	if (err)
+		return err;
 
 	/* Configure the termios structure */
 	cp210x_get_termios(tty, port);
@@ -552,15 +604,12 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void cp210x_close(struct usb_serial_port *port)
 {
-	unsigned int purge_ctl;
-
 	usb_serial_generic_close(port);
 
 	/* Clear both queues; cp2108 needs this to avoid an occasional hang */
-	purge_ctl = PURGE_ALL;
-	cp210x_set_config(port, CP210X_PURGE, &purge_ctl, 2);
+	cp210x_write_u16_reg(port, CP210X_PURGE, PURGE_ALL);
 
-	cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
+	cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_DISABLE);
 }
 
 /*
@@ -638,11 +687,12 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	unsigned int *cflagp, unsigned int *baudp)
 {
 	struct device *dev = &port->dev;
-	unsigned int cflag, modem_ctl[4];
-	unsigned int baud;
-	unsigned int bits;
+	unsigned int cflag;
+	u8 modem_ctl[16];
+	u32 baud;
+	u16 bits;
 
-	cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
+	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
 
 	dev_dbg(dev, "%s - baud rate = %d\n", __func__, baud);
 	*baudp = baud;
@@ -673,14 +723,14 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		cflag |= CS8;
 		bits &= ~BITS_DATA_MASK;
 		bits |= BITS_DATA_8;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
 		break;
 	default:
 		dev_dbg(dev, "%s - Unknown number of data bits, using 8\n", __func__);
 		cflag |= CS8;
 		bits &= ~BITS_DATA_MASK;
 		bits |= BITS_DATA_8;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
 		break;
 	}
 
@@ -711,7 +761,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		dev_dbg(dev, "%s - Unknown parity mode, disabling parity\n", __func__);
 		cflag &= ~PARENB;
 		bits &= ~BITS_PARITY_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
 		break;
 	}
 
@@ -723,7 +773,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	case BITS_STOP_1_5:
 		dev_dbg(dev, "%s - stop bits = 1.5 (not supported, using 1 stop bit)\n", __func__);
 		bits &= ~BITS_STOP_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
 		break;
 	case BITS_STOP_2:
 		dev_dbg(dev, "%s - stop bits = 2\n", __func__);
@@ -732,12 +782,13 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	default:
 		dev_dbg(dev, "%s - Unknown number of stop bits, using 1 stop bit\n", __func__);
 		bits &= ~BITS_STOP_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
 		break;
 	}
 
-	cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16);
-	if (modem_ctl[0] & 0x0008) {
+	cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
+			sizeof(modem_ctl));
+	if (modem_ctl[0] & 8) {
 		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
 		cflag |= CRTSCTS;
 	} else {
@@ -789,8 +840,7 @@ static void cp210x_change_speed(struct tty_struct *tty,
 	baud = cp210x_quantise_baudrate(baud);
 
 	dev_dbg(&port->dev, "%s - setting baud rate to %u\n", __func__, baud);
-	if (cp210x_set_config(port, CP210X_SET_BAUDRATE, &baud,
-							sizeof(baud))) {
+	if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
 		dev_warn(&port->dev, "failed to set baud rate to %u\n", baud);
 		if (old_termios)
 			baud = old_termios->c_ospeed;
@@ -806,8 +856,8 @@ static void cp210x_set_termios(struct tty_struct *tty,
 {
 	struct device *dev = &port->dev;
 	unsigned int cflag, old_cflag;
-	unsigned int bits;
-	unsigned int modem_ctl[4];
+	u16 bits;
+	u8 modem_ctl[16];
 
 	cflag = tty->termios.c_cflag;
 	old_cflag = old_termios->c_cflag;
@@ -845,7 +895,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 			bits |= BITS_DATA_8;
 			break;
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits))
 			dev_dbg(dev, "Number of data bits requested not supported by device\n");
 	}
 
@@ -872,7 +922,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 				}
 			}
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits))
 			dev_dbg(dev, "Parity mode not supported by device\n");
 	}
 
@@ -886,32 +936,40 @@ static void cp210x_set_termios(struct tty_struct *tty,
 			bits |= BITS_STOP_1;
 			dev_dbg(dev, "%s - stop bits = 1\n", __func__);
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits))
 			dev_dbg(dev, "Number of stop bits requested not supported by device\n");
 	}
 
 	if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
-		cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16);
-		dev_dbg(dev, "%s - read modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n",
-			__func__, modem_ctl[0], modem_ctl[1],
-			modem_ctl[2], modem_ctl[3]);
+
+		/* Only bytes 0, 4 and 7 out of first 8 have functional bits */
+
+		cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
+				sizeof(modem_ctl));
+		dev_dbg(dev, "%s - read modem controls = %02x .. .. .. %02x .. .. %02x\n",
+			__func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
 
 		if (cflag & CRTSCTS) {
 			modem_ctl[0] &= ~0x7B;
 			modem_ctl[0] |= 0x09;
-			modem_ctl[1] = 0x80;
+			modem_ctl[4] = 0x80;
+			/* FIXME - why clear reserved bits just read? */
+			modem_ctl[5] = 0;
+			modem_ctl[6] = 0;
+			modem_ctl[7] = 0;
 			dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
 		} else {
 			modem_ctl[0] &= ~0x7B;
 			modem_ctl[0] |= 0x01;
-			modem_ctl[1] |= 0x40;
+			/* FIXME - OR here istead of assignment looks wrong */
+			modem_ctl[4] |= 0x40;
 			dev_dbg(dev, "%s - flow control = NONE\n", __func__);
 		}
 
-		dev_dbg(dev, "%s - write modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n",
-			__func__, modem_ctl[0], modem_ctl[1],
-			modem_ctl[2], modem_ctl[3]);
-		cp210x_set_config(port, CP210X_SET_FLOW, modem_ctl, 16);
+		dev_dbg(dev, "%s - write modem controls = %02x .. .. .. %02x .. .. %02x\n",
+			__func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
+		cp210x_write_reg_block(port, CP210X_SET_FLOW, modem_ctl,
+				sizeof(modem_ctl));
 	}
 
 }
@@ -926,7 +984,7 @@ static int cp210x_tiocmset(struct tty_struct *tty,
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
 		unsigned int set, unsigned int clear)
 {
-	unsigned int control = 0;
+	u16 control = 0;
 
 	if (set & TIOCM_RTS) {
 		control |= CONTROL_RTS;
@@ -947,7 +1005,7 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port,
 
 	dev_dbg(&port->dev, "%s - control = 0x%.4x\n", __func__, control);
 
-	return cp210x_set_config(port, CP210X_SET_MHS, &control, 2);
+	return cp210x_write_u16_reg(port, CP210X_SET_MHS, control);
 }
 
 static void cp210x_dtr_rts(struct usb_serial_port *p, int on)
@@ -961,10 +1019,10 @@ static void cp210x_dtr_rts(struct usb_serial_port *p, int on)
 static int cp210x_tiocmget(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	unsigned int control;
+	u8 control;
 	int result;
 
-	cp210x_get_config(port, CP210X_GET_MDMSTS, &control, 1);
+	cp210x_read_u8_reg(port, CP210X_GET_MDMSTS, &control);
 
 	result = ((control & CONTROL_DTR) ? TIOCM_DTR : 0)
 		|((control & CONTROL_RTS) ? TIOCM_RTS : 0)
@@ -981,7 +1039,7 @@ static int cp210x_tiocmget(struct tty_struct *tty)
 static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	unsigned int state;
+	u16 state;
 
 	if (break_state == 0)
 		state = BREAK_OFF;
@@ -989,7 +1047,7 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 		state = BREAK_ON;
 	dev_dbg(&port->dev, "%s - turning break %s\n", __func__,
 		state == BREAK_OFF ? "off" : "on");
-	cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
+	cp210x_write_u16_reg(port, CP210X_SET_BREAK, state);
 }
 
 static int cp210x_port_probe(struct usb_serial_port *port)
-- 
1.8.4.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/