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

[Martyn Welch <martyn.welch@collabora.co.uk>]

On 14/01/16 18:15, Konstantin Shkolnyy wrote:
>> -----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()?
>
> 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.
>

Given there's a typo that needs correcting, I'd probably extend the 
patch series if you have the work ready.

Martyn