Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1393878 > unrolled thread

[PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

Started byKonstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
First post2016-05-04 03:00 +0200
Last post2016-05-04 15:20 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code. Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com> - 2016-05-04 03:00 +0200
    Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in  CRTSCTS flag code. Johan Hovold <johan@kernel.org> - 2016-05-04 09:30 +0200
      Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic  numbers in CRTSCTS flag code. Johan Hovold <johan@kernel.org> - 2016-05-04 15:00 +0200
        RE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic  numbers in CRTSCTS flag code. Konstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com> - 2016-05-04 15:20 +0200
          Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic  numbers in CRTSCTS flag code. Johan Hovold <johan@kernel.org> - 2016-05-04 15:30 +0200
      RE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic  numbers in CRTSCTS flag code. Konstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com> - 2016-05-04 15:20 +0200

#1393878 — [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

FromKonstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
Date2016-05-04 03:00 +0200
Subject[PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
Message-ID<ruTku-1Li-7@gated-at.bofh.it>
Replaced magic numbers used in the CRTSCTS flag code with symbolic names
from the chip specification.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
v4:
Same series of patches, fixed names and defines by feedback.
v3:
Regenerated the patches correctly against the latest usb-next branch.
v2
Improved CRTSCTS fix by feedback. Dropped get_termios error handling fix.

 drivers/usb/serial/cp210x.c | 109 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 84 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index fef7a51..9857d0c 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -327,6 +327,42 @@ struct cp210x_comm_status {
  */
 #define PURGE_ALL		0x000f
 
+/* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
+struct cp210x_flow_ctl {
+	__le32	ulControlHandshake;
+	__le32	ulFlowReplace;
+	__le32	ulXonLimit;
+	__le32	ulXoffLimit;
+} __packed;
+
+/* cp210x_flow_ctl::ulControlHandshake */
+#define CP210X_SERIAL_DTR_MASK		GENMASK(1, 0)
+#define CP210X_SERIAL_DTR_SHIFT(_mode)	(_mode)
+#define CP210X_SERIAL_CTS_HANDSHAKE	BIT(3)
+#define CP210X_SERIAL_DSR_HANDSHAKE	BIT(4)
+#define CP210X_SERIAL_DCD_HANDSHAKE	BIT(5)
+#define CP210X_SERIAL_DSR_SENSITIVITY	BIT(6)
+
+/* values for cp210x_flow_ctl::ulControlHandshake::CP210X_SERIAL_DTR_MASK */
+#define CP210X_SERIAL_DTR_INACTIVE	0
+#define CP210X_SERIAL_DTR_ACTIVE	1
+#define CP210X_SERIAL_DTR_FLOW_CTL	2
+
+/* cp210x_flow_ctl::ulFlowReplace */
+#define CP210X_SERIAL_AUTO_TRANSMIT	BIT(0)
+#define CP210X_SERIAL_AUTO_RECEIVE	BIT(1)
+#define CP210X_SERIAL_ERROR_CHAR	BIT(2)
+#define CP210X_SERIAL_NULL_STRIPPING	BIT(3)
+#define CP210X_SERIAL_BREAK_CHAR	BIT(4)
+#define CP210X_SERIAL_RTS_MASK		GENMASK(7, 6)
+#define CP210X_SERIAL_RTS_SHIFT(_mode)	(_mode << 6)
+#define CP210X_SERIAL_XOFF_CONTINUE	BIT(31)
+
+/* values for cp210x_flow_ctl::ulFlowReplace::CP210X_SERIAL_RTS_MASK */
+#define CP210X_SERIAL_RTS_INACTIVE	0
+#define CP210X_SERIAL_RTS_ACTIVE	1
+#define CP210X_SERIAL_RTS_FLOW_CTL	2
+
 /*
  * Reads a variable-sized block of CP210X_ registers, identified by req.
  * Returns data into buf in native USB byte order.
@@ -694,9 +730,10 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 {
 	struct device *dev = &port->dev;
 	unsigned int cflag;
-	u8 modem_ctl[16];
+	struct cp210x_flow_ctl flow_ctl;
 	u32 baud;
 	u16 bits;
+	u32 ctl_hs;
 
 	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
 
@@ -792,9 +829,10 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		break;
 	}
 
-	cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
-			sizeof(modem_ctl));
-	if (modem_ctl[0] & 0x08) {
+	cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
+			sizeof(flow_ctl));
+	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
+	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
 		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
 		cflag |= CRTSCTS;
 	} else {
@@ -863,7 +901,6 @@ static void cp210x_set_termios(struct tty_struct *tty,
 	struct device *dev = &port->dev;
 	unsigned int cflag, old_cflag;
 	u16 bits;
-	u8 modem_ctl[16];
 
 	cflag = tty->termios.c_cflag;
 	old_cflag = old_termios->c_cflag;
@@ -947,34 +984,56 @@ static void cp210x_set_termios(struct tty_struct *tty,
 	}
 
 	if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
+		struct cp210x_flow_ctl flow_ctl;
+		u32 ctl_hs;
+		u32 flow_repl;
 
-		/* 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]);
+		cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
+				sizeof(flow_ctl));
+		ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
+		flow_repl      = le32_to_cpu(flow_ctl.ulFlowReplace);
+		dev_dbg(dev, "%s - read ulControlHandshake=0x%08x, ulFlowReplace=0x%08x\n",
+			__func__, ctl_hs, flow_repl);
 
 		if (cflag & CRTSCTS) {
-			modem_ctl[0] &= ~0x7B;
-			modem_ctl[0] |= 0x09;
-			modem_ctl[4] = 0x80;
-			/* FIXME - why clear reserved bits just read? */
-			modem_ctl[5] = 0;
-			modem_ctl[6] = 0;
-			modem_ctl[7] = 0;
+			ctl_hs &= ~(CP210X_SERIAL_DTR_MASK |
+					CP210X_SERIAL_CTS_HANDSHAKE |
+					CP210X_SERIAL_DSR_HANDSHAKE |
+					CP210X_SERIAL_DCD_HANDSHAKE |
+					CP210X_SERIAL_DSR_SENSITIVITY);
+			ctl_hs |= CP210X_SERIAL_DTR_SHIFT(
+					CP210X_SERIAL_DTR_ACTIVE);
+			ctl_hs |= CP210X_SERIAL_CTS_HANDSHAKE;
+			/*
+			 * FIXME: Why clear bits unrelated to flow control.
+			 * Why clear CP210X_SERIAL_XOFF_CONTINUE which is
+			 * never set
+			 */
+			flow_repl = 0;
+			flow_repl |= CP210X_SERIAL_RTS_SHIFT(
+					CP210X_SERIAL_RTS_FLOW_CTL);
 			dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
 		} else {
-			modem_ctl[0] &= ~0x7B;
-			modem_ctl[0] |= 0x01;
-			modem_ctl[4] = 0x40;
+			ctl_hs &= ~(CP210X_SERIAL_DTR_MASK |
+					CP210X_SERIAL_CTS_HANDSHAKE |
+					CP210X_SERIAL_DSR_HANDSHAKE |
+					CP210X_SERIAL_DCD_HANDSHAKE |
+					CP210X_SERIAL_DSR_SENSITIVITY);
+			ctl_hs |= CP210X_SERIAL_DTR_SHIFT(
+					CP210X_SERIAL_DTR_ACTIVE;
+			/* FIXME: Why clear bits unrelated to flow control */
+			((u8)flow_repl) = 0;
+			flow_repl |= CP210X_SERIAL_RTS_SHIFT(
+					CP210X_SERIAL_RTS_ACTIVE);
 			dev_dbg(dev, "%s - flow control = NONE\n", __func__);
 		}
 
-		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));
+		dev_dbg(dev, "%s - write ulControlHandshake=0x%08x, ulFlowReplace=0x%08x\n",
+			__func__, ctl_hs, flow_repl);
+		flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);
+		flow_ctl.ulFlowReplace      = cpu_to_le32(flow_repl);
+		cp210x_write_reg_block(port, CP210X_SET_FLOW, &flow_ctl,
+				sizeof(flow_ctl));
 	}
 
 }
-- 
1.8.4.5

[toc] | [next] | [standalone]

#1394055 — Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

FromJohan Hovold <johan@kernel.org>
Date2016-05-04 09:30 +0200
SubjectRe: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
Message-ID<ruZpU-7Yn-29@gated-at.bofh.it>
In reply to#1393878
On Tue, May 03, 2016 at 07:52:23PM -0500, Konstantin Shkolnyy wrote:
> Replaced magic numbers used in the CRTSCTS flag code with symbolic names
> from the chip specification.
> 
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>

This patch does not even compile. Please be more careful when
resubmitting. There are at least two compilation errors below.

> ---
> v4:
> Same series of patches, fixed names and defines by feedback.
> v3:
> Regenerated the patches correctly against the latest usb-next branch.
> v2
> Improved CRTSCTS fix by feedback. Dropped get_termios error handling fix.

In the future, if you have an aggregate changelog for a series, put it
in a cover letter. A cover letter is good to have anyway and should
provide a high-level summary of what the series does.

Thanks,
Johan

[toc] | [prev] | [next] | [standalone]

#1394266 — Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

FromJohan Hovold <johan@kernel.org>
Date2016-05-04 15:00 +0200
SubjectRe: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
Message-ID<rv4zg-3Wc-29@gated-at.bofh.it>
In reply to#1394055
On Wed, May 04, 2016 at 12:46:17PM +0000, Konstantin Shkolnyy wrote:
> > -----Original Message----- From: linux-usb-owner@vger.kernel.org
> > [mailto:linux-usb- owner@vger.kernel.org] On Behalf Of Johan Hovold
> > Sent: Wednesday, May 04, 2016 02:29 To: Konstantin Shkolnyy Cc:
> > johan@kernel.org; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org Subject: [EXT] Re: [PATCH v4 2/3] USB:
> > serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
> > 
> > On Tue, May 03, 2016 at 07:52:23PM -0500, Konstantin Shkolnyy wrote:
> > > Replaced magic numbers used in the CRTSCTS flag code with symbolic
> > names
> > > from the chip specification.
> > >
> > > Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> > 
> > This patch does not even compile. Please be more careful when
> > resubmitting. There are at least two compilation errors below.
> 
> Sorry about that.
> 
> It's a couple of dumb syntax errors that don't really matter for
> change review purpose. Otherwise, do the patches look good? 

It's worse than that as when the code doesn't even compile it's obvious
that it has never been tested. That is just not acceptable, and the code
does not deserve review.

Johan

[toc] | [prev] | [next] | [standalone]

#1394282 — RE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

FromKonstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com>
Date2016-05-04 15:20 +0200
SubjectRE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
Message-ID<rv4SD-4va-27@gated-at.bofh.it>
In reply to#1394266
> -----Original Message-----
> From: Johan Hovold [mailto:jhovold@gmail.com] On Behalf Of Johan Hovold
> Sent: Wednesday, May 04, 2016 07:55
> To: Konstantin Shkolnyy
> Cc: Johan Hovold; Konstantin Shkolnyy; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic
> numbers in CRTSCTS flag code.
> 
> On Wed, May 04, 2016 at 12:46:17PM +0000, Konstantin Shkolnyy wrote:
> > > -----Original Message----- From: linux-usb-owner@vger.kernel.org
> > > [mailto:linux-usb- owner@vger.kernel.org] On Behalf Of Johan Hovold
> > > Sent: Wednesday, May 04, 2016 02:29 To: Konstantin Shkolnyy Cc:
> > > johan@kernel.org; linux-usb@vger.kernel.org; linux-
> > > kernel@vger.kernel.org Subject: [EXT] Re: [PATCH v4 2/3] USB:
> > > serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
> > >
> > > On Tue, May 03, 2016 at 07:52:23PM -0500, Konstantin Shkolnyy wrote:
> > > > Replaced magic numbers used in the CRTSCTS flag code with symbolic
> > > names
> > > > from the chip specification.
> > > >
> > > > Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> > >
> > > This patch does not even compile. Please be more careful when
> > > resubmitting. There are at least two compilation errors below.
> >
> > Sorry about that.
> >
> > It's a couple of dumb syntax errors that don't really matter for
> > change review purpose. Otherwise, do the patches look good?
> 
> It's worse than that as when the code doesn't even compile it's obvious
> that it has never been tested. That is just not acceptable, and the code
> does not deserve review.

Well, I did test the final code. I assumed, perhaps incorrectly, that the point of a patch series is that it gets applied or rejected entirely, so nobody would run the code in the middle of it.

[toc] | [prev] | [next] | [standalone]

#1394285 — Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

FromJohan Hovold <johan@kernel.org>
Date2016-05-04 15:30 +0200
SubjectRe: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
Message-ID<rv52i-4BT-13@gated-at.bofh.it>
In reply to#1394282
On Wed, May 04, 2016 at 01:17:26PM +0000, Konstantin Shkolnyy wrote:
> > -----Original Message-----
> > From: Johan Hovold [mailto:jhovold@gmail.com] On Behalf Of Johan Hovold
> > Sent: Wednesday, May 04, 2016 07:55
> > To: Konstantin Shkolnyy
> > Cc: Johan Hovold; Konstantin Shkolnyy; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic
> > numbers in CRTSCTS flag code.
> > 
> > On Wed, May 04, 2016 at 12:46:17PM +0000, Konstantin Shkolnyy wrote:
> > > > -----Original Message----- From: linux-usb-owner@vger.kernel.org
> > > > [mailto:linux-usb- owner@vger.kernel.org] On Behalf Of Johan Hovold
> > > > Sent: Wednesday, May 04, 2016 02:29 To: Konstantin Shkolnyy Cc:
> > > > johan@kernel.org; linux-usb@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org Subject: [EXT] Re: [PATCH v4 2/3] USB:
> > > > serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
> > > >
> > > > On Tue, May 03, 2016 at 07:52:23PM -0500, Konstantin Shkolnyy wrote:
> > > > > Replaced magic numbers used in the CRTSCTS flag code with symbolic
> > > > names
> > > > > from the chip specification.
> > > > >
> > > > > Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> > > >
> > > > This patch does not even compile. Please be more careful when
> > > > resubmitting. There are at least two compilation errors below.
> > >
> > > Sorry about that.
> > >
> > > It's a couple of dumb syntax errors that don't really matter for
> > > change review purpose. Otherwise, do the patches look good?
> > 
> > It's worse than that as when the code doesn't even compile it's obvious
> > that it has never been tested. That is just not acceptable, and the code
> > does not deserve review.
> 
> Well, I did test the final code. I assumed, perhaps incorrectly, that
> the point of a patch series is that it gets applied or rejected
> entirely, so nobody would run the code in the middle of it.

No, every patch in a series should be correct, and must specifically not
break bisectability by failing to compile.

Johan

[toc] | [prev] | [next] | [standalone]

#1394281 — RE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.

FromKonstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com>
Date2016-05-04 15:20 +0200
SubjectRE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
Message-ID<rv4zg-3Wc-31@gated-at.bofh.it>
In reply to#1394055
> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Johan Hovold
> Sent: Wednesday, May 04, 2016 02:29
> To: Konstantin Shkolnyy
> Cc: johan@kernel.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic
> numbers in CRTSCTS flag code.
> 
> On Tue, May 03, 2016 at 07:52:23PM -0500, Konstantin Shkolnyy wrote:
> > Replaced magic numbers used in the CRTSCTS flag code with symbolic
> names
> > from the chip specification.
> >
> > Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> 
> This patch does not even compile. Please be more careful when
> resubmitting. There are at least two compilation errors below.

Sorry about that.

It's a couple of dumb syntax errors that don't really matter for change review purpose. Otherwise, do the patches look good? I 

[...]

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web