From: Martyn Welch <martyn.welch@collabora.co.uk>
To: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>, johan@kernel.org
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/4] USB: serial: cp210x: Switch to new 16-bit register access functions.
Date: Thu, 14 Jan 2016 17:43:33 +0000 [thread overview]
Message-ID: <5697DE45.7000204@collabora.co.uk> (raw)
In-Reply-To: <1451704323-11941-1-git-send-email-konstantin.shkolnyy@gmail.com>
On 02/01/16 03:12, Konstantin Shkolnyy wrote:
> Change to use new 16-bit register access functions instead of
> cp210x_get_config and cp210x_set_config.
>
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
> change in v3: Presented new function addition as a separate patch #1,
> to simplify code review.
>
> drivers/usb/serial/cp210x.c | 80 ++++++++++++++++++---------------------------
> 1 file changed, 31 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 324eb7e..7a91c6c 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -588,17 +588,6 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
> }
>
> /*
> - * cp210x_set_config_single
> - * Convenience function for calling cp210x_set_config on single data values
> - * without requiring an integer pointer
> - */
> -static inline int cp210x_set_config_single(struct usb_serial_port *port,
> - u8 request, unsigned int data)
> -{
> - return cp210x_set_config(port, request, &data, 2);
> -}
> -
> -/*
> * 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.
> @@ -607,48 +596,47 @@ static inline int cp210x_set_config_single(struct usb_serial_port *port,
> static int cp210x_detect_swapped_line_ctl(struct usb_serial_port *port)
> {
> struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> - unsigned int line_ctl_save;
> - unsigned int line_ctl_test;
> + u16 line_ctl_save;
> + u16 line_ctl_test;
> int err;
>
> - err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_save, 2);
> + err = cp210x_read_u16_reg(port, CP210X_GET_LINE_CTL, &line_ctl_save);
> if (err)
> return err;
>
> - line_ctl_test = 0x800;
> - err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_test, 2);
> + err = cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, 0x800);
As mentioned in the comments to the first patch.
> if (err)
> return err;
>
> - err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_test, 2);
> + err = cp210x_read_u16_reg(port, CP210X_GET_LINE_CTL, &line_ctl_test);
> if (err)
> return err;
>
> /* has_swapped_line_ctl is 0 here because port_priv was kzalloced */
> if (line_ctl_test == 8) {
> port_priv->has_swapped_line_ctl = true;
> - line_ctl_save = swab16((u16)line_ctl_save);
> + line_ctl_save = swab16(line_ctl_save);
> }
>
> - return cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_save, 2);
> + return cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, line_ctl_save);
> }
>
> /*
> - * Must always be called instead of cp210x_get_config(CP210X_GET_LINE_CTL)
> + * Must always be called instead of cp210x_read_u16_reg(CP210X_GET_LINE_CTL)
> * to workaround cp2108 bug and get correct value.
> */
> -static int cp210x_get_line_ctl(struct usb_serial_port *port, unsigned int *ctl)
> +static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
> {
> struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> int err;
>
> - err = cp210x_get_config(port, CP210X_GET_LINE_CTL, ctl, 2);
> + err = cp210x_read_u16_reg(port, CP210X_GET_LINE_CTL, ctl);
> if (err)
> return err;
>
> /* Workaround swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
> if (port_priv->has_swapped_line_ctl)
> - *ctl = swab16((u16)(*ctl));
> + *ctl = swab16(*ctl);
>
> return 0;
> }
> @@ -697,14 +685,11 @@ static unsigned int cp210x_quantise_baudrate(unsigned int baud)
>
> static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
> {
> - int result;
> + int err;
>
> - result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
> - UART_ENABLE);
> - if (result) {
> - dev_err(&port->dev, "%s - Unable to enable UART\n", __func__);
> - return result;
> - }
> + err = cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_ENABLE);
> + if (err)
> + return err;
Any reason for dropping the error message?
>
> /* Configure the termios structure */
> cp210x_get_termios(tty, port);
> @@ -718,15 +703,12 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
>
> static void cp210x_close(struct usb_serial_port *port)
> {
> - unsigned int purge_ctl;
> -
> usb_serial_generic_close(port);
>
> /* Clear both queues; cp2108 needs this to avoid an occasional hang */
> - purge_ctl = PURGE_ALL;
> - cp210x_set_config(port, CP210X_PURGE, &purge_ctl, 2);
> + cp210x_write_u16_reg(port, CP210X_PURGE, PURGE_ALL);
>
> - cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
> + cp210x_write_u16_reg(port, CP210X_IFC_ENABLE, UART_DISABLE);
> }
>
> /*
> @@ -806,7 +788,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> struct device *dev = &port->dev;
> unsigned int cflag, modem_ctl[4];
> unsigned int baud;
> - unsigned int bits;
> + u16 bits;
>
> cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
>
> @@ -839,14 +821,14 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> cflag |= CS8;
> bits &= ~BITS_DATA_MASK;
> bits |= BITS_DATA_8;
> - cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
> + cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
> break;
> default:
> dev_dbg(dev, "%s - Unknown number of data bits, using 8\n", __func__);
> cflag |= CS8;
> bits &= ~BITS_DATA_MASK;
> bits |= BITS_DATA_8;
> - cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
> + cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
> break;
> }
>
> @@ -877,7 +859,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> dev_dbg(dev, "%s - Unknown parity mode, disabling parity\n", __func__);
> cflag &= ~PARENB;
> bits &= ~BITS_PARITY_MASK;
> - cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
> + cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
> break;
> }
>
> @@ -889,7 +871,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> case BITS_STOP_1_5:
> dev_dbg(dev, "%s - stop bits = 1.5 (not supported, using 1 stop bit)\n", __func__);
> bits &= ~BITS_STOP_MASK;
> - cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
> + cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
> break;
> case BITS_STOP_2:
> dev_dbg(dev, "%s - stop bits = 2\n", __func__);
> @@ -898,7 +880,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> default:
> dev_dbg(dev, "%s - Unknown number of stop bits, using 1 stop bit\n", __func__);
> bits &= ~BITS_STOP_MASK;
> - cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
> + cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits);
> break;
> }
>
> @@ -972,7 +954,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
> {
> struct device *dev = &port->dev;
> unsigned int cflag, old_cflag;
> - unsigned int bits;
> + u16 bits;
> unsigned int modem_ctl[4];
>
> cflag = tty->termios.c_cflag;
> @@ -1011,7 +993,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
> bits |= BITS_DATA_8;
> break;
> }
> - if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
> + if (cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits))
> dev_dbg(dev, "Number of data bits requested not supported by device\n");
> }
>
> @@ -1038,7 +1020,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
> }
> }
> }
> - if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
> + if (cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits))
> dev_dbg(dev, "Parity mode not supported by device\n");
> }
>
> @@ -1052,7 +1034,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
> bits |= BITS_STOP_1;
> dev_dbg(dev, "%s - stop bits = 1\n", __func__);
> }
> - if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
> + if (cp210x_write_u16_reg(port, CP210X_SET_LINE_CTL, bits))
> dev_dbg(dev, "Number of stop bits requested not supported by device\n");
> }
>
> @@ -1092,7 +1074,7 @@ static int cp210x_tiocmset(struct tty_struct *tty,
> static int cp210x_tiocmset_port(struct usb_serial_port *port,
> unsigned int set, unsigned int clear)
> {
> - unsigned int control = 0;
> + u16 control = 0;
>
> if (set & TIOCM_RTS) {
> control |= CONTROL_RTS;
> @@ -1113,7 +1095,7 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port,
>
> dev_dbg(&port->dev, "%s - control = 0x%.4x\n", __func__, control);
>
> - return cp210x_set_config(port, CP210X_SET_MHS, &control, 2);
> + return cp210x_write_u16_reg(port, CP210X_SET_MHS, control);
> }
>
> static void cp210x_dtr_rts(struct usb_serial_port *p, int on)
> @@ -1147,7 +1129,7 @@ static int cp210x_tiocmget(struct tty_struct *tty)
> static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
> {
> struct usb_serial_port *port = tty->driver_data;
> - unsigned int state;
> + u16 state;
>
> if (break_state == 0)
> state = BREAK_OFF;
> @@ -1155,7 +1137,7 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
> state = BREAK_ON;
> dev_dbg(&port->dev, "%s - turning break %s\n", __func__,
> state == BREAK_OFF ? "off" : "on");
> - cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
> + cp210x_write_u16_reg(port, CP210X_SET_BREAK, state);
> }
>
> static int cp210x_port_probe(struct usb_serial_port *port)
>
next prev parent reply other threads:[~2016-01-14 17:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-02 3:12 [PATCH v3 2/4] USB: serial: cp210x: Switch to new 16-bit register access functions Konstantin Shkolnyy
2016-01-14 17:43 ` Martyn Welch [this message]
2016-01-14 18:22 ` Konstantin Shkolnyy
2016-01-14 18:23 ` Martyn Welch
2016-01-18 17:42 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5697DE45.7000204@collabora.co.uk \
--to=martyn.welch@collabora.co.uk \
--cc=johan@kernel.org \
--cc=konstantin.shkolnyy@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.