From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756671AbcANRwE (ORCPT ); Thu, 14 Jan 2016 12:52:04 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:42535 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756632AbcANRwB (ORCPT ); Thu, 14 Jan 2016 12:52:01 -0500 Message-ID: <5697E03D.9040209@collabora.co.uk> Date: Thu, 14 Jan 2016 17:51:57 +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> In-Reply-To: <1451704360-12011-1-git-send-email-konstantin.shkolnyy@gmail.com> 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 02/01/16 03:12, Konstantin Shkolnyy wrote: > Change to use new large register access functions instead of > cp210x_get_config and cp210x_set_config and remove the old functions since > they are now unused. > > Signed-off-by: Konstantin Shkolnyy > --- > change in v3: Presented new function addition as a separate patch #1, > to simplify code review. > > drivers/usb/serial/cp210x.c | 137 ++++++++------------------------------------ > 1 file changed, 24 insertions(+), 113 deletions(-) > > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c > index 1339d77..ce80d5f 100644 > --- a/drivers/usb/serial/cp210x.c > +++ b/drivers/usb/serial/cp210x.c > @@ -489,105 +489,6 @@ static int cp210x_write_u32_reg(struct usb_serial_port *port, u8 req, u32 val) > } > > /* > - * cp210x_get_config > - * Reads from the CP210x configuration registers > - * 'size' is specified in bytes. > - * 'data' is a pointer to a pre-allocated array of integers large > - * enough to hold 'size' bytes (with 4 bytes to each integer) > - */ > -static int cp210x_get_config(struct usb_serial_port *port, u8 request, > - unsigned int *data, int size) > -{ > - struct usb_serial *serial = port->serial; > - struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); > - __le32 *buf; > - int result, i, length; > - > - /* Number of integers required to contain the array */ > - length = (((size - 1) | 3) + 1) / 4; > - > - buf = kcalloc(length, sizeof(__le32), GFP_KERNEL); > - if (!buf) > - return -ENOMEM; > - > - /* Issue the request, attempting to read 'size' bytes */ > - result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), > - request, REQTYPE_INTERFACE_TO_HOST, 0x0000, > - port_priv->bInterfaceNumber, buf, size, > - USB_CTRL_GET_TIMEOUT); > - > - /* Convert data into an array of integers */ > - for (i = 0; i < length; i++) > - data[i] = le32_to_cpu(buf[i]); > - > - kfree(buf); > - > - if (result != size) { > - dev_dbg(&port->dev, "%s - Unable to send config request, request=0x%x size=%d result=%d\n", > - __func__, request, size, result); > - if (result > 0) > - result = -EPROTO; > - > - return result; > - } > - > - return 0; > -} > - > -/* > - * cp210x_set_config > - * Writes to the CP210x configuration registers > - * Values less than 16 bits wide are sent directly > - * 'size' is specified in bytes. > - */ > -static int cp210x_set_config(struct usb_serial_port *port, u8 request, > - unsigned int *data, int size) > -{ > - struct usb_serial *serial = port->serial; > - struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); > - __le32 *buf; > - int result, i, length; > - > - /* Number of integers required to contain the array */ > - length = (((size - 1) | 3) + 1) / 4; > - > - buf = kmalloc(length * sizeof(__le32), GFP_KERNEL); > - if (!buf) > - return -ENOMEM; > - > - /* Array of integers into bytes */ > - for (i = 0; i < length; i++) > - buf[i] = cpu_to_le32(data[i]); > - > - if (size > 2) { > - result = usb_control_msg(serial->dev, > - usb_sndctrlpipe(serial->dev, 0), > - request, REQTYPE_HOST_TO_INTERFACE, 0x0000, > - port_priv->bInterfaceNumber, buf, size, > - USB_CTRL_SET_TIMEOUT); > - } else { > - result = usb_control_msg(serial->dev, > - usb_sndctrlpipe(serial->dev, 0), > - request, REQTYPE_HOST_TO_INTERFACE, data[0], > - port_priv->bInterfaceNumber, NULL, 0, > - USB_CTRL_SET_TIMEOUT); > - } > - > - kfree(buf); > - > - if ((size > 2 && result != size) || result < 0) { > - dev_dbg(&port->dev, "%s - Unable to send request, request=0x%x size=%d result=%d\n", > - __func__, request, size, result); > - if (result > 0) > - result = -EPROTO; > - > - return result; > - } > - > - return 0; > -} > - > -/* > * Detect CP2108 GET_LINE_CTL bug and activate workaround. > * Write a known good value 0x800, read it back. > * If it comes back swapped the bug is detected. > @@ -786,7 +687,8 @@ static void cp210x_get_termios_port(struct usb_serial_port *port, > unsigned int *cflagp, unsigned int *baudp) > { > struct device *dev = &port->dev; > - unsigned int cflag, modem_ctl[4]; > + unsigned int cflag; > + u8 modem_ctl[16]; > u32 baud; > u16 bits; > > @@ -884,8 +786,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) { > dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__); > cflag |= CRTSCTS; > } else { > @@ -954,7 +857,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; > @@ -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? Martyn > + modem_ctl[4] |= 0x40; > dev_dbg(dev, "%s - flow control = NONE\n", __func__); > } > > - dev_dbg(dev, "%s - write modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n", > - __func__, modem_ctl[0], modem_ctl[1], > - modem_ctl[2], modem_ctl[3]); > - cp210x_set_config(port, CP210X_SET_FLOW, modem_ctl, 16); > + dev_dbg(dev, "%s - write modem controls = %02x .. .. .. %02x .. .. %02x\n", > + __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]); > + cp210x_write_reg_block(port, CP210X_SET_FLOW, modem_ctl, > + sizeof(modem_ctl)); > } > > } >