Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/01/16 03:12, Konstantin Shkolnyy wrote:
Change to use new large register access functions instead of
cp210x_get_config and cp210x_set_config and remove the old functions since
they are now unused.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy-Re5JQEeQqe8AvxtiuMwx3w@xxxxxxxxxxxxxxxx>
---
change in v3: Presented new function addition as a separate patch #1,
to simplify code review.

  drivers/usb/serial/cp210x.c | 137 ++++++++------------------------------------
  1 file changed, 24 insertions(+), 113 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 1339d77..ce80d5f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -489,105 +489,6 @@ static int cp210x_write_u32_reg(struct usb_serial_port *port, u8 req, u32 val)
  }

  /*
- * 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)
- */
-static int cp210x_get_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
-{
-	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;
-
-	buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
-	if (!buf)
-		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);
-
-	/* Convert data into an array of integers */
-	for (i = 0; i < length; i++)
-		data[i] = le32_to_cpu(buf[i]);
-
-	kfree(buf);
-
-	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;
-	}
-
-	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.
- */
-static int cp210x_set_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
-{
-	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;
-
-	buf = kmalloc(length * sizeof(__le32), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	/* Array of integers into bytes */
-	for (i = 0; i < length; i++)
-		buf[i] = cpu_to_le32(data[i]);
-
-	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);
-	}
-
-	kfree(buf);
-
-	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)
-			result = -EPROTO;
-
-		return result;
-	}
-
-	return 0;
-}
-
-/*
   * Detect CP2108 GET_LINE_CTL bug and activate workaround.
   * Write a known good value 0x800, read it back.
   * If it comes back swapped the bug is detected.
@@ -786,7 +687,8 @@ 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 cflag;
+	u8 modem_ctl[16];
  	u32 baud;
  	u16 bits;

@@ -884,8 +786,9 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
  		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 {
@@ -954,7 +857,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
  	struct device *dev = &port->dev;
  	unsigned int cflag, old_cflag;
  	u16 bits;
-	unsigned int modem_ctl[4];
+	u8 modem_ctl[16];

  	cflag = tty->termios.c_cflag;
  	old_cflag = old_termios->c_cflag;
@@ -1038,27 +941,35 @@ static void cp210x_set_termios(struct tty_struct *tty,
  	}

  	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 */

s/istead/instead/

I'm a little unsure about FIXME comments being added rather than the issue being addressed. If I'm reading this right, then this is the control for the RTS/CTS lines, could the operation without these bits being cleared/ORed be checked by using a serial cable (connected to another serial port) and writing data with and without flow control enabled through a terminal emulator?

Martyn

+			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));
  	}

  }


--
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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux