* [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.
@ 2016-04-24 17:09 Konstantin Shkolnyy
2016-04-25 10:16 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Shkolnyy @ 2016-04-24 17:09 UTC (permalink / raw)
To: johan; +Cc: linux-usb, linux-kernel, Konstantin Shkolnyy
Documented "magic numbers" used in the CRTSCT flag code in terms of
register bit names from the chip specification.
Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
drivers/usb/serial/cp210x.c | 38 ++++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index ede5c52..b2321a7 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -790,7 +790,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
sizeof(modem_ctl));
- if (modem_ctl[0] & 0x08) {
+ if (modem_ctl[0] & 0x08) { /* if SERIAL_CTS_HANDSHAKE */
dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
cflag |= CRTSCTS;
} else {
@@ -952,17 +952,47 @@ static void cp210x_set_termios(struct tty_struct *tty,
__func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
if (cflag & CRTSCTS) {
- modem_ctl[0] &= ~0x7B;
+ modem_ctl[0] &= ~0x7B; /* keep reserved D2 and D7 */
+ /*
+ * D1-D0 SERIAL_DTR_MASK =01b: DTR is held active
+ * D3 SERIAL_CTS_HANDSHAKE =1: CTS is a handshake line
+ * D4 SERIAL_DSR_HANDSHAKE =0
+ * D5 SERIAL_DCD_HANDSHAKE =0
+ * D6 SERIAL_DSR_SENSITIVITY =0
+ */
modem_ctl[0] |= 0x09;
+ /*
+ * D0 SERIAL_AUTO_TRANSMIT =0
+ * D1 SERIAL_AUTO_RECEIVE =0
+ * D2 SERIAL_ERROR_CHAR =0
+ * D3 SERIAL_NULL_STRIPPING =0
+ * D4 SERIAL_BREAK_CHAR =0
+ * D7-D6 SERIAL_RTS_MASK =10b: RTS for rcv flow control
+ */
modem_ctl[4] = 0x80;
dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
} else {
- modem_ctl[0] &= ~0x7B;
+ modem_ctl[0] &= ~0x7B; /* keep reserved D2 and D7 */
+ /*
+ * D1-D0 SERIAL_DTR_MASK = 01b: DTR is held active
+ * D3 SERIAL_CTS_HANDSHAKE = 0: CTS is a status input.
+ * D4 SERIAL_DSR_HANDSHAKE = 0
+ * D5 SERIAL_DCD_HANDSHAKE = 0
+ * D6 SERIAL_DSR_SENSITIVITY = 0
+ */
modem_ctl[0] |= 0x01;
+ /*
+ * D0 SERIAL_AUTO_TRANSMIT = 0
+ * D1 SERIAL_AUTO_RECEIVE = 0
+ * D2 SERIAL_ERROR_CHAR = 0
+ * D3 SERIAL_NULL_STRIPPING = 0
+ * D4 SERIAL_BREAK_CHAR = 0
+ * D7-D6 SERIAL_RTS_MASK =01b: RTS is statically active
+ */
modem_ctl[4] = 0x40;
dev_dbg(dev, "%s - flow control = NONE\n", __func__);
}
- modem_ctl[7] = 0;
+ modem_ctl[7] = 0; /* SERIAL_XOFF_CONTINUE = 0 */
dev_dbg(dev, "%s - write modem controls = %02x .. .. .. %02x .. .. %02x\n",
__func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.
2016-04-24 17:09 [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code Konstantin Shkolnyy
@ 2016-04-25 10:16 ` Johan Hovold
2016-04-25 18:09 ` [EXT] " Konstantin Shkolnyy
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2016-04-25 10:16 UTC (permalink / raw)
To: Konstantin Shkolnyy; +Cc: johan, linux-usb, linux-kernel
On Sun, Apr 24, 2016 at 12:09:21PM -0500, Konstantin Shkolnyy wrote:
> Documented "magic numbers" used in the CRTSCT flag code in terms of
> register bit names from the chip specification.
Documenting these is long overdue. I even started adding defines just to
be able to review the first patch in the series.
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
> drivers/usb/serial/cp210x.c | 38 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ede5c52..b2321a7 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -790,7 +790,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>
> cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
> sizeof(modem_ctl));
> - if (modem_ctl[0] & 0x08) {
> + if (modem_ctl[0] & 0x08) { /* if SERIAL_CTS_HANDSHAKE */
But instead of adding comments like this one and below, please add
proper defines for the various flags and masks. That will make the code
mostly self-explanatory.
> dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> cflag |= CRTSCTS;
> } else {
> @@ -952,17 +952,47 @@ static void cp210x_set_termios(struct tty_struct *tty,
> __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
>
> if (cflag & CRTSCTS) {
> - modem_ctl[0] &= ~0x7B;
> + modem_ctl[0] &= ~0x7B; /* keep reserved D2 and D7 */
> + /*
> + * D1-D0 SERIAL_DTR_MASK =01b: DTR is held active
> + * D3 SERIAL_CTS_HANDSHAKE =1: CTS is a handshake line
> + * D4 SERIAL_DSR_HANDSHAKE =0
> + * D5 SERIAL_DCD_HANDSHAKE =0
> + * D6 SERIAL_DSR_SENSITIVITY =0
> + */
> - modem_ctl[7] = 0;
> + modem_ctl[7] = 0; /* SERIAL_XOFF_CONTINUE = 0 */
And this would be better as a bit operation as well (as already
mentioned).
Thanks,
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.
2016-04-25 10:16 ` Johan Hovold
@ 2016-04-25 18:09 ` Konstantin Shkolnyy
2016-04-26 7:26 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Shkolnyy @ 2016-04-25 18:09 UTC (permalink / raw)
To: Johan Hovold, Konstantin Shkolnyy
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
I was planning to define all these bits in a separate future patch.
Would you rather prefer the magic numbers defined before fixing the bugs?
I guess I can do that. Is something like this acceptable?
/* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
struct cp210x_flow_ctl {
u8 SERIAL_DTR_MASK : 2; /* byte 0 */
u8 : 1;
u8 SERIAL_CTS_HANDSHAKE : 1;
u8 SERIAL_DSR_HANDSHAKE : 1;
u8 SERIAL_DCD_HANDSHAKE : 1;
u8 SERIAL_DSR_SENSITIVITY : 1;
u8 : 1;
u8; /* byte 1 */
u8; /* byte 2 */
u8; /* byte 3 */
u8 SERIAL_AUTO_TRANSMIT : 1; /* byte 4 */
u8 SERIAL_AUTO_RECEIVE : 1;
u8 SERIAL_ERROR_CHAR : 1;
u8 SERIAL_NULL_STRIPPING : 1;
u8 SERIAL_BREAK_CHAR : 1;
u8 : 1;
u8 SERIAL_RTS_MASK : 2;
u8; /* byte 5 */
u8; /* byte 6 */
u8 : 7; /* byte 7 */
u8 SERIAL_XOFF_CONTINUE : 1;
__le32 ulXonLimit;
__le32 ulXoffLimit;
} __packed;
> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Johan Hovold
> Sent: Monday, April 25, 2016 05:17
> To: Konstantin Shkolnyy
> Cc: johan@kernel.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments
> to CRTSCT flag code.
>
> On Sun, Apr 24, 2016 at 12:09:21PM -0500, Konstantin Shkolnyy wrote:
> > Documented "magic numbers" used in the CRTSCT flag code in terms of
> > register bit names from the chip specification.
>
> Documenting these is long overdue. I even started adding defines just to
> be able to review the first patch in the series.
>
> > Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> > ---
> > drivers/usb/serial/cp210x.c | 38
> ++++++++++++++++++++++++++++++++++----
> > 1 file changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index ede5c52..b2321a7 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -790,7 +790,7 @@ static void cp210x_get_termios_port(struct
> usb_serial_port *port,
> >
> > cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
> > sizeof(modem_ctl));
> > - if (modem_ctl[0] & 0x08) {
> > + if (modem_ctl[0] & 0x08) { /* if SERIAL_CTS_HANDSHAKE */
>
> But instead of adding comments like this one and below, please add
> proper defines for the various flags and masks. That will make the code
> mostly self-explanatory.
>
> > dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> > cflag |= CRTSCTS;
> > } else {
> > @@ -952,17 +952,47 @@ static void cp210x_set_termios(struct tty_struct
> *tty,
> > __func__, modem_ctl[0], modem_ctl[4],
> modem_ctl[7]);
> >
> > if (cflag & CRTSCTS) {
> > - modem_ctl[0] &= ~0x7B;
> > + modem_ctl[0] &= ~0x7B; /* keep reserved D2 and D7
> */
> > + /*
> > + * D1-D0 SERIAL_DTR_MASK =01b: DTR is held
> active
> > + * D3 SERIAL_CTS_HANDSHAKE =1: CTS is a
> handshake line
> > + * D4 SERIAL_DSR_HANDSHAKE =0
> > + * D5 SERIAL_DCD_HANDSHAKE =0
> > + * D6 SERIAL_DSR_SENSITIVITY =0
> > + */
>
> > - modem_ctl[7] = 0;
> > + modem_ctl[7] = 0; /* SERIAL_XOFF_CONTINUE = 0 */
>
> And this would be better as a bit operation as well (as already
> mentioned).
>
> Thanks,
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.
2016-04-25 18:09 ` [EXT] " Konstantin Shkolnyy
@ 2016-04-26 7:26 ` Johan Hovold
2016-04-27 20:06 ` Konstantin Shkolnyy
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2016-04-26 7:26 UTC (permalink / raw)
To: Konstantin Shkolnyy
Cc: Johan Hovold, Konstantin Shkolnyy, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Apr 25, 2016 at 06:09:01PM +0000, Konstantin Shkolnyy wrote:
> I was planning to define all these bits in a separate future patch.
> Would you rather prefer the magic numbers defined before fixing the bugs?
Fixing the RTS bug (patch 1), which is the only "real" bug, should be
done before adding defines, and fixing and cleaning up the rest.
> I guess I can do that. Is something like this acceptable?
>
> /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
> struct cp210x_flow_ctl {
> u8 SERIAL_DTR_MASK : 2; /* byte 0 */
> u8 : 1;
> u8 SERIAL_CTS_HANDSHAKE : 1;
> u8 SERIAL_DSR_HANDSHAKE : 1;
> u8 SERIAL_DCD_HANDSHAKE : 1;
> u8 SERIAL_DSR_SENSITIVITY : 1;
> u8 : 1;
> u8; /* byte 1 */
> u8; /* byte 2 */
> u8; /* byte 3 */
> u8 SERIAL_AUTO_TRANSMIT : 1; /* byte 4 */
> u8 SERIAL_AUTO_RECEIVE : 1;
> u8 SERIAL_ERROR_CHAR : 1;
> u8 SERIAL_NULL_STRIPPING : 1;
> u8 SERIAL_BREAK_CHAR : 1;
> u8 : 1;
> u8 SERIAL_RTS_MASK : 2;
> u8; /* byte 5 */
> u8; /* byte 6 */
> u8 : 7; /* byte 7 */
> u8 SERIAL_XOFF_CONTINUE : 1;
> __le32 ulXonLimit;
> __le32 ulXoffLimit;
> } __packed;
No, shouldn't rely on the layout of bitfields. Define masks and shifts
as needed and the message structure as
struct cp210x_flow_ctl {
__le32 ulControlHandshake;
__le32 ulFlowReplace;
__le32 ulXonLimit;
__le32 ulXoffLimit;
};
that is, as per AN571.
Thanks,
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.
2016-04-26 7:26 ` Johan Hovold
@ 2016-04-27 20:06 ` Konstantin Shkolnyy
2016-04-28 11:09 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Konstantin Shkolnyy @ 2016-04-27 20:06 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: Tuesday, April 26, 2016 02:26
> To: Konstantin Shkolnyy
> Cc: Johan Hovold; Konstantin Shkolnyy; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added
> comments to CRTSCT flag code.
>
> On Mon, Apr 25, 2016 at 06:09:01PM +0000, Konstantin Shkolnyy wrote:
> > I was planning to define all these bits in a separate future patch.
> > Would you rather prefer the magic numbers defined before fixing the
> bugs?
>
> Fixing the RTS bug (patch 1), which is the only "real" bug, should be
> done before adding defines, and fixing and cleaning up the rest.
>
> > I guess I can do that. Is something like this acceptable?
> >
> > /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
> > struct cp210x_flow_ctl {
> > u8 SERIAL_DTR_MASK : 2; /* byte 0 */
> > u8 : 1;
> > u8 SERIAL_CTS_HANDSHAKE : 1;
> > u8 SERIAL_DSR_HANDSHAKE : 1;
> > u8 SERIAL_DCD_HANDSHAKE : 1;
> > u8 SERIAL_DSR_SENSITIVITY : 1;
> > u8 : 1;
> > u8; /* byte 1 */
> > u8; /* byte 2 */
> > u8; /* byte 3 */
> > u8 SERIAL_AUTO_TRANSMIT : 1; /* byte 4 */
> > u8 SERIAL_AUTO_RECEIVE : 1;
> > u8 SERIAL_ERROR_CHAR : 1;
> > u8 SERIAL_NULL_STRIPPING : 1;
> > u8 SERIAL_BREAK_CHAR : 1;
> > u8 : 1;
> > u8 SERIAL_RTS_MASK : 2;
> > u8; /* byte 5 */
> > u8; /* byte 6 */
> > u8 : 7; /* byte 7 */
> > u8 SERIAL_XOFF_CONTINUE : 1;
> > __le32 ulXonLimit;
> > __le32 ulXoffLimit;
> > } __packed;
>
> No, shouldn't rely on the layout of bitfields. Define masks and shifts
> as needed and the message structure as
>
> struct cp210x_flow_ctl {
> __le32 ulControlHandshake;
> __le32 ulFlowReplace;
> __le32 ulXonLimit;
> __le32 ulXoffLimit;
> };
>
> that is, as per AN571.
>
OK, from searching www I see that bitfields have bad reputation for unclear reasons, so I guess it's now easier to avoid them.
But doing it like you suggest, instead of splitting it to bytes, would complicate the code with endian conversions.
Is there a reason for this other than making it identical to the spec?\
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.
2016-04-27 20:06 ` Konstantin Shkolnyy
@ 2016-04-28 11:09 ` Johan Hovold
0 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2016-04-28 11:09 UTC (permalink / raw)
To: Konstantin Shkolnyy
Cc: Johan Hovold, Konstantin Shkolnyy, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, Apr 27, 2016 at 08:06:32PM +0000, Konstantin Shkolnyy wrote:
> > -----Original Message-----
> > From: Johan Hovold [mailto:jhovold@gmail.com] On Behalf Of Johan Hovold
> > Sent: Tuesday, April 26, 2016 02:26
> > To: Konstantin Shkolnyy
> > Cc: Johan Hovold; Konstantin Shkolnyy; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added
> > comments to CRTSCT flag code.
> >
> > On Mon, Apr 25, 2016 at 06:09:01PM +0000, Konstantin Shkolnyy wrote:
> > > I was planning to define all these bits in a separate future patch.
> > > Would you rather prefer the magic numbers defined before fixing the
> > bugs?
> >
> > Fixing the RTS bug (patch 1), which is the only "real" bug, should be
> > done before adding defines, and fixing and cleaning up the rest.
> >
> > > I guess I can do that. Is something like this acceptable?
> > >
> > > /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
> > > struct cp210x_flow_ctl {
> > > u8 SERIAL_DTR_MASK : 2; /* byte 0 */
> > > u8 : 1;
> > > u8 SERIAL_CTS_HANDSHAKE : 1;
> > > u8 SERIAL_DSR_HANDSHAKE : 1;
> > > u8 SERIAL_DCD_HANDSHAKE : 1;
> > > u8 SERIAL_DSR_SENSITIVITY : 1;
> > > u8 : 1;
> > > u8; /* byte 1 */
> > > u8; /* byte 2 */
> > > u8; /* byte 3 */
> > > u8 SERIAL_AUTO_TRANSMIT : 1; /* byte 4 */
> > > u8 SERIAL_AUTO_RECEIVE : 1;
> > > u8 SERIAL_ERROR_CHAR : 1;
> > > u8 SERIAL_NULL_STRIPPING : 1;
> > > u8 SERIAL_BREAK_CHAR : 1;
> > > u8 : 1;
> > > u8 SERIAL_RTS_MASK : 2;
> > > u8; /* byte 5 */
> > > u8; /* byte 6 */
> > > u8 : 7; /* byte 7 */
> > > u8 SERIAL_XOFF_CONTINUE : 1;
> > > __le32 ulXonLimit;
> > > __le32 ulXoffLimit;
> > > } __packed;
> >
> > No, shouldn't rely on the layout of bitfields. Define masks and shifts
> > as needed and the message structure as
> >
> > struct cp210x_flow_ctl {
> > __le32 ulControlHandshake;
> > __le32 ulFlowReplace;
> > __le32 ulXonLimit;
> > __le32 ulXoffLimit;
> > };
> >
> > that is, as per AN571.
>
> OK, from searching www I see that bitfields have bad reputation for
> unclear reasons, so I guess it's now easier to avoid them.
> But doing it like you suggest, instead of splitting it to bytes, would
> complicate the code with endian conversions.
> Is there a reason for this other than making it identical to the spec?\
Staying aligned with the specification is usually a good idea. That also
became apparent when reviewing these patching and trying to match up the
magic constants with the spec.
The endian conversions should not need to complicate things that much.
Get the values using le32_to_cpu, manipulate the bits in a u32, and
store them back using cpu_to_le32.
Thanks,
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-28 11:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-24 17:09 [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code Konstantin Shkolnyy
2016-04-25 10:16 ` Johan Hovold
2016-04-25 18:09 ` [EXT] " Konstantin Shkolnyy
2016-04-26 7:26 ` Johan Hovold
2016-04-27 20:06 ` Konstantin Shkolnyy
2016-04-28 11:09 ` Johan Hovold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).