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

[Johan Hovold]

On Thu, Jan 14, 2016 at 06:15:42PM +0000, Konstantin Shkolnyy wrote:
> > -----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]);s
> > >
> > >   		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;
> > > +		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.

If possible you should fix any bugs before reworking the register
helpers as that will make backporting the fix much easier.

And the CRTSCTS handling indeed seems broken.

> 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()?

That is already being taken care of by the tty layer through the dtr_rts
callback which makes sure to raise DTR and RTS at open (and lower them
at close if HUPCL is set).

That said, you should probably still fix the CRTSCTS handling to honour
whatever DTR state has been set by the tty layer and/or user. Perhaps
it's enough just not to touch these bits after they are read back from
the device.

> 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.

If you can fix the above before modifying the register accessors that
would be preferred. You can submit it all in one series.

Thanks,
Johan