From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753754AbcB2Kf6 (ORCPT ); Mon, 29 Feb 2016 05:35:58 -0500 Received: from mail-lf0-f66.google.com ([209.85.215.66]:33761 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752403AbcB2Kf4 (ORCPT ); Mon, 29 Feb 2016 05:35:56 -0500 Date: Mon, 29 Feb 2016 11:36:01 +0100 From: Johan Hovold To: Konstantin Shkolnyy Cc: johan@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 3/3] USB: serial: cp210x: New access functions for large registers Message-ID: <20160229103601.GE26559@localhost> References: <1456696316-29980-1-git-send-email-konstantin.shkolnyy@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1456696316-29980-1-git-send-email-konstantin.shkolnyy@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 28, 2016 at 03:51:56PM -0600, Konstantin Shkolnyy wrote: > cp210x_get_config and cp210x_set_config are cumbersome to use. This change > switches large register access to use new block functions. The old > functions are removed because now they become unused. > > Signed-off-by: Konstantin Shkolnyy > --- > @@ -886,8 +788,9 @@ static void cp210x_get_termios_port(struct usb_serial_port *port, > break; > } > > - cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16); > - if (modem_ctl[0] & 0x0008) { > + cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl, > + sizeof(modem_ctl)); > + if (modem_ctl[0] & 8) { I changed this to 0x08 as it's a bitmask. > dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__); > cflag |= CRTSCTS; > } else { > @@ -956,7 +859,7 @@ static void cp210x_set_termios(struct tty_struct *tty, > struct device *dev = &port->dev; > unsigned int cflag, old_cflag; > u16 bits; > - unsigned int modem_ctl[4]; > + u8 modem_ctl[16]; > > cflag = tty->termios.c_cflag; > old_cflag = old_termios->c_cflag; > @@ -1040,27 +943,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]); And here you're actually now leaving out XON/XOFF limits in the last 8 bytes. It would have been better to just dump the whole array and make any such changes explicitly in a separate patch, but I left this in this time. > 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 instead of assignment looks wrong */ > + modem_ctl[4] |= 0x40; These indeed looks like bugs, but I agree that fixing the accessor functions first made sense. Looking forward to the follow-up fixes. :) All three patches now applied. Thanks for sticking to this. Johan