* [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
@ 2016-05-04 0:52 Konstantin Shkolnyy
2016-05-04 7:28 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Shkolnyy @ 2016-05-04 0:52 UTC (permalink / raw)
To: johan; +Cc: linux-usb, linux-kernel, Konstantin Shkolnyy
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
2016-05-04 0:52 [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code Konstantin Shkolnyy
@ 2016-05-04 7:28 ` Johan Hovold
2016-05-04 12:46 ` [EXT] " Konstantin Shkolnyy
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2016-05-04 7:28 UTC (permalink / raw)
To: Konstantin Shkolnyy; +Cc: johan, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
2016-05-04 7:28 ` Johan Hovold
@ 2016-05-04 12:46 ` Konstantin Shkolnyy
2016-05-04 12:55 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Shkolnyy @ 2016-05-04 12:46 UTC (permalink / raw)
To: Johan Hovold, Konstantin Shkolnyy
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
> -----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
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
2016-05-04 12:46 ` [EXT] " Konstantin Shkolnyy
@ 2016-05-04 12:55 ` Johan Hovold
2016-05-04 13:17 ` Konstantin Shkolnyy
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2016-05-04 12:55 UTC (permalink / raw)
To: Konstantin Shkolnyy
Cc: Johan Hovold, Konstantin Shkolnyy, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
2016-05-04 12:55 ` Johan Hovold
@ 2016-05-04 13:17 ` Konstantin Shkolnyy
2016-05-04 13:22 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Shkolnyy @ 2016-05-04 13:17 UTC (permalink / raw)
To: Johan Hovold
Cc: Konstantin Shkolnyy, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org
> -----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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code.
2016-05-04 13:17 ` Konstantin Shkolnyy
@ 2016-05-04 13:22 ` Johan Hovold
0 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2016-05-04 13:22 UTC (permalink / raw)
To: Konstantin Shkolnyy
Cc: Johan Hovold, Konstantin Shkolnyy, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-04 13:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-04 0:52 [PATCH v4 2/3] USB: serial: cp210x: Got rid of magic numbers in CRTSCTS flag code Konstantin Shkolnyy
2016-05-04 7:28 ` Johan Hovold
2016-05-04 12:46 ` [EXT] " Konstantin Shkolnyy
2016-05-04 12:55 ` Johan Hovold
2016-05-04 13:17 ` Konstantin Shkolnyy
2016-05-04 13:22 ` Johan Hovold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox