From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756662AbcANSWs (ORCPT ); Thu, 14 Jan 2016 13:22:48 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:42570 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754616AbcANSWr (ORCPT ); Thu, 14 Jan 2016 13:22:47 -0500 Message-ID: <5697E771.3050809@collabora.co.uk> Date: Thu, 14 Jan 2016 18:22:41 +0000 From: Martyn Welch User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.8.0 MIME-Version: 1.0 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 References: <1451704360-12011-1-git-send-email-konstantin.shkolnyy@gmail.com> <5697E03D.9040209@collabora.co.uk> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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