RE: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers

[Konstantin Shkolnyy]

> -----Original Message-----
> From: Martyn Welch [mailto:martyn.welch@xxxxxxxxxxxxxxx]
> Sent: Thursday, January 14, 2016 11:52
> To: Konstantin Shkolnyy; Johan Hovold
> Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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()?

I'm waiting on this patch series to be accepted, then submit other improvements. Or it may be better to submit a longer patch series that has further improvements appended... I'm new here and not really sure.