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

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

[Konstantin Shkolnyy]

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@gmail.com>
---
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 */
+			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));
 	}
 
 }
-- 
1.8.4.5

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

[Martyn Welch]

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@public.gmane.org>
> ---
> 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));
>   	}
>
>   }
>

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

[Konstantin Shkolnyy]

> -----Original Message-----
> From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
> Sent: Thursday, January 14, 2016 11:52
> To: Konstantin Shkolnyy; Johan Hovold
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access
> functions for large registers
...

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

The patching procedure enforced by maintainers dictates that each separate patch addresses 1 issue.
It's much easier to review changes this way. So this particular patch just converts from one register access function to another.
The bugfix patch will come later.

While doing this cleanup, I also noticed another bug - this function will always set the low 2 bits of byte 0 to  01b: "modem_ctl[0] |= 0x01".
This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held inactive"). The function will only write it when CRTSCTS changes.
So the device will start with 0, then, if CRTSCTS ever changes, it'll become 1 and stay 1 forever. Looks wrong to me.
I'm still researching the subject when and how it should be set.

	 * Wikipedia: "DTR and DSR are usually on all the time and, per the
	 * RS-232 standard and its successors, are used to signal from each
	 * end that the other equipment is actually present and powered-up."
	 * So perhaps DTR should be turned ON in open() and OFF in close()?

I'm waiting on this patch series to be accepted, then submit other improvements. Or it may be better to submit a longer patch series that has further improvements appended... I'm new here and not really sure.

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

[Martyn Welch]

On 14/01/16 18:15, Konstantin Shkolnyy wrote:
>> -----Original Message-----
>> From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
>> Sent: Thursday, January 14, 2016 11:52
>> To: Konstantin Shkolnyy; Johan Hovold
>> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access
>> functions for large registers
> ...
>
>>> @@ -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?
>
> The patching procedure enforced by maintainers dictates that each separate patch addresses 1 issue.
> It's much easier to review changes this way. So this particular patch just converts from one register access function to another.
> The bugfix patch will come later.
>
> While doing this cleanup, I also noticed another bug - this function will always set the low 2 bits of byte 0 to  01b: "modem_ctl[0] |= 0x01".
> This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held inactive"). The function will only write it when CRTSCTS changes.
> So the device will start with 0, then, if CRTSCTS ever changes, it'll become 1 and stay 1 forever. Looks wrong to me.
> I'm still researching the subject when and how it should be set.
>
> 	 * Wikipedia: "DTR and DSR are usually on all the time and, per the
> 	 * RS-232 standard and its successors, are used to signal from each
> 	 * end that the other equipment is actually present and powered-up."
> 	 * So perhaps DTR should be turned ON in open() and OFF in close()?
>
> I'm waiting on this patch series to be accepted, then submit other improvements. Or it may be better to submit a longer patch series that has further improvements appended... I'm new here and not really sure.
>

Given there's a typo that needs correcting, I'd probably extend the 
patch series if you have the work ready.

Martyn

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

[Konstantin Shkolnyy]

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Konstantin Shkolnyy
> Sent: Thursday, January 14, 2016 12:16
> To: Martyn Welch; Johan Hovold
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access
> functions for large registers
> 
> > -----Original Message-----
> > From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
> > Sent: Thursday, January 14, 2016 11:52
> > To: Konstantin Shkolnyy; Johan Hovold
> > Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register
> access
> > functions for large registers
> ...
> 
> > > @@ -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?
> 
> The patching procedure enforced by maintainers dictates that each separate
> patch addresses 1 issue.
> It's much easier to review changes this way. So this particular patch just
> converts from one register access function to another.
> The bugfix patch will come later.
> 
> While doing this cleanup, I also noticed another bug - this function will always
> set the low 2 bits of byte 0 to  01b: "modem_ctl[0] |= 0x01".
> This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held
> inactive"). The function will only write it when CRTSCTS changes.
> So the device will start with 0, then, if CRTSCTS ever changes, it'll become 1
> and stay 1 forever. Looks wrong to me.
> I'm still researching the subject when and how it should be set.
> 
> 	 * Wikipedia: "DTR and DSR are usually on all the time and, per the
> 	 * RS-232 standard and its successors, are used to signal from each
> 	 * end that the other equipment is actually present and powered-
> up."
> 	 * So perhaps DTR should be turned ON in open() and OFF in close()?

And the same problem with SERIAL_RTS_MASK. Here is what this piece of code turned it into after fixing the FIXME you mentioned and assigning names to all the bits.

	if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
		struct cp210x_flow_ctl flow_ctl;
	
		/* Only bytes 0, 4 and 7 out of first 8 have functional bits */

		cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
				sizeof(flow_ctl));

		if (cflag & CRTSCTS) {
			/* "DTR is held active" */
			flow_ctl.SERIAL_DTR_MASK        = 1;
			/* "CTS is a handshake line" */
			flow_ctl.SERIAL_CTS_HANDSHAKE   = 1;
			flow_ctl.SERIAL_DSR_HANDSHAKE   = 0;
			flow_ctl.SERIAL_DCD_HANDSHAKE   = 0;
			flow_ctl.SERIAL_DSR_SENSITIVITY = 0;

			flow_ctl.SERIAL_AUTO_TRANSMIT   = 0;
			flow_ctl.SERIAL_AUTO_RECEIVE    = 0;
			flow_ctl.SERIAL_ERROR_CHAR      = 0;
			flow_ctl.SERIAL_NULL_STRIPPING  = 0;
			flow_ctl.SERIAL_BREAK_CHAR      = 0;
			/* "RTS is used for receive flow control" */
			flow_ctl.SERIAL_RTS_MASK        = 2;

			dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
		} else {
			/* "DTR is held active" */
			flow_ctl.SERIAL_DTR_MASK        = 1;
			/* "CTS is simply a status input" */
			flow_ctl.SERIAL_CTS_HANDSHAKE   = 0;
			flow_ctl.SERIAL_DSR_HANDSHAKE   = 0;
			flow_ctl.SERIAL_DCD_HANDSHAKE   = 0;
			flow_ctl.SERIAL_DSR_SENSITIVITY = 0;

			flow_ctl.SERIAL_AUTO_TRANSMIT   = 0;
			flow_ctl.SERIAL_AUTO_RECEIVE    = 0;
			flow_ctl.SERIAL_ERROR_CHAR      = 0;
			flow_ctl.SERIAL_NULL_STRIPPING  = 0;
			flow_ctl.SERIAL_BREAK_CHAR      = 0;
			/* "RTS is statically active" */
			flow_ctl.SERIAL_RTS_MASK        = 1;

			dev_dbg(dev, "%s - flow control = NONE\n", __func__);
		}
		flow_ctl.SERIAL_XOFF_CONTINUE = 0;

		cp210x_write_reg_block(port, CP210X_SET_FLOW, flow_ctl,
				sizeof(flow_ctl));
	}

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

[Johan Hovold]

On Thu, Jan 14, 2016 at 06:15:42PM +0000, Konstantin Shkolnyy wrote:

> The patching procedure enforced by maintainers dictates that each
> separate patch addresses 1 issue.
> It's much easier to review changes this way. So this particular patch
> just converts from one register access function to another.
> The bugfix patch will come later.

Do one logical change per patch is a good rule of thumb, yes. But we
must never introduce regression by taking that rule too far.

I haven't had time to review your patches yet, but I remember thinking
that those FIXMEs looked odd. How about adding the missing error
handling before you introduce the new helpers?

> While doing this cleanup, I also noticed another bug - this function
> will always set the low 2 bits of byte 0 to  01b: "modem_ctl[0] |=
> 0x01".
> This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held
> inactive"). The function will only write it when CRTSCTS changes.
> So the device will start with 0, then, if CRTSCTS ever changes, it'll
> become 1 and stay 1 forever. Looks wrong to me.
> I'm still researching the subject when and how it should be set.
> 
> 	 * Wikipedia: "DTR and DSR are usually on all the time and, per the
> 	 * RS-232 standard and its successors, are used to signal from each
> 	 * end that the other equipment is actually present and powered-up."
> 	 * So perhaps DTR should be turned ON in open() and OFF in close()?
> 
> I'm waiting on this patch series to be accepted, then submit other
> improvements. Or it may be better to submit a longer patch series that
> has further improvements appended... I'm new here and not really sure.

Generally, you should wait for a series to be reviewed before sending
(too many) follow ups. Unless you find any issues with it and ask for it
to be dropped, that is.

Thanks,
Johan

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

[Johan Hovold]

On Thu, Jan 14, 2016 at 06:15:42PM +0000, Konstantin Shkolnyy wrote:
> > -----Original Message-----
> > From: Martyn Welch [mailto:martyn.welch@collabora.co.uk]
> > Sent: Thursday, January 14, 2016 11:52
> > To: Konstantin Shkolnyy; Johan Hovold
> > Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access
> > functions for large registers
> ...
> 
> > > @@ -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]);s
> > >
> > >   		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;
> > > +		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?
> 
> The patching procedure enforced by maintainers dictates that each
> separate patch addresses 1 issue.
> It's much easier to review changes this way. So this particular patch
> just converts from one register access function to another.
> The bugfix patch will come later.

If possible you should fix any bugs before reworking the register
helpers as that will make backporting the fix much easier.

And the CRTSCTS handling indeed seems broken.

> While doing this cleanup, I also noticed another bug - this function
> will always set the low 2 bits of byte 0 to  01b: "modem_ctl[0] |=
> 0x01".
> This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held
> inactive"). The function will only write it when CRTSCTS changes.
> So the device will start with 0, then, if CRTSCTS ever changes, it'll
> become 1 and stay 1 forever. Looks wrong to me.
> I'm still researching the subject when and how it should be set.
> 
> 	 * Wikipedia: "DTR and DSR are usually on all the time and, per the
> 	 * RS-232 standard and its successors, are used to signal from each
> 	 * end that the other equipment is actually present and powered-up."

> 	 * So perhaps DTR should be turned ON in open() and OFF in close()?

That is already being taken care of by the tty layer through the dtr_rts
callback which makes sure to raise DTR and RTS at open (and lower them
at close if HUPCL is set).

That said, you should probably still fix the CRTSCTS handling to honour
whatever DTR state has been set by the tty layer and/or user. Perhaps
it's enough just not to touch these bits after they are read back from
the device.

> I'm waiting on this patch series to be accepted, then submit other
> improvements. Or it may be better to submit a longer patch series that
> has further improvements appended... I'm new here and not really sure.

If you can fix the above before modifying the register accessors that
would be preferred. You can submit it all in one series.

Thanks,
Johan