From: Konstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com>
To: Martyn Welch <martyn.welch@collabora.co.uk>,
Johan Hovold <johan@kernel.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-kernel@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
Date: Thu, 14 Jan 2016 18:29:54 +0000 [thread overview]
Message-ID: <BLUPR0701MB15722F41DA2CBDA9C155F07C91CC0@BLUPR0701MB1572.namprd07.prod.outlook.com> (raw)
In-Reply-To: <BLUPR0701MB157297A9CB514F414CCA583691CC0@BLUPR0701MB1572.namprd07.prod.outlook.com>
> -----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));
}
next prev parent reply other threads:[~2016-01-14 18:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-02 3:12 [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers Konstantin Shkolnyy
2016-01-14 17:51 ` Martyn Welch
2016-01-14 18:15 ` Konstantin Shkolnyy
2016-01-14 18:22 ` Martyn Welch
2016-01-14 18:29 ` Konstantin Shkolnyy [this message]
2016-01-14 18:53 ` Johan Hovold
2016-01-18 20:31 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BLUPR0701MB15722F41DA2CBDA9C155F07C91CC0@BLUPR0701MB1572.namprd07.prod.outlook.com \
--to=konstantin.shkolnyy@silabs.com \
--cc=johan@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=martyn.welch@collabora.co.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).