Re: [PATCH v3 2/2] USB: serial: cp210x: Proper RTS control when buffers fill

[Johan Hovold <johan@kernel.org>]

On Fri, Jun 26, 2020 at 09:42:06AM +0200, Johan Hovold wrote:
> On Fri, Jun 26, 2020 at 04:24:20AM +0000, Phu Luu wrote:
> > From: Brant Merryman <brant.merryman@silabs.com>
> > 
> > CP210x hardware disables auto-RTS but leaves auto-CTS when
> > in hardware flow control mode and UART on cp210x hardware
> > is disabled. This allows data to flow out, but new data
> > will not come into the port. When re-opening the port, if
> > auto-CTS is enabled on the cp210x, then auto-RTS must be
> > re-enabled in the driver.
> > 
> > Signed-off-by: Phu Luu <phu.luu@silabs.com>
> > Signed-off-by: Brant Merryman <brant.merryman@silabs.com>
> 
> Please revisit these tags as well.
> 
> > ---
> > 06/09/2020: Patch v3 2/2 Modified based on feedback from Johan Hovold <johan@kernel.org>
> > 12/18/2019: Patch v2 Broken into two patches and modified based on feedback from Johan Hovold <johan@kernel.org>
> > 12/09/2019: Initial submission of patch "Proper RTS control when buffers fill"
> > 
> >  drivers/usb/serial/cp210x.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index bcceb4ad8be0..091441b1c5b9 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -917,6 +917,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> >  	u32 baud;
> >  	u16 bits;
> >  	u32 ctl_hs;
> > +	u32 flow_repl;
> >  
> >  	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
> >  
> > @@ -1017,6 +1018,23 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> >  	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> >  	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
> >  		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> > +		/*
> > +		 * When the port is closed, the CP210x hardware disables
> > +		 * auto-RTS and RTS is deasserted but it leaves auto-CTS when
> > +		 * in hardware flow control mode. This prevents new data from
> > +		 * being received by the port. When re-opening the port, if
> > +		 * auto-CTS is enabled on the cp210x, then auto-RTS must be
> > +		 * re-enabled in the driver.
> > +		 */
> 
> I already asked this of Brant, but could you please be more specific
> about which state the RTS-line end up in when disabling the UART (e.g.
> 0x00: statically inactive)?
> 
> The reason I ask is that after open() returns, the tty layer would raise
> RTS, which should again allow data to flow in in contrast to what the
> comment and changelog text claims.

I had another reason to look at this driver so I could determine that
the patch description and comment were indeed incorrect; the tty layer
asserts RTS when reopening the port just fine and data flows in.

> We still want to enable auto-RTS of course so the patch is otherwise
> correct.

I've fixed up the commit message and comment, reordered the SOB tags
and added your Co-developed-by tag before applying both of these now:

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-next

Johan