On Thu, Oct 22, 2015 at 04:12:00PM -0500, Konstantin Shkolnyy wrote:
> cp2108 GET_LINE_CTL returns the 16-bit value with the 2 bytes swapped.
> However, SET_LINE_CTL functions properly. When the driver tries to modify
> the register, it reads it, modifies some bits and writes back. Because the
> read bytes were swapped, this often results in an invalid value to be
> written. In turn, this causes cp2108 respond with a stall. The stall
> sometimes doesn't clear properly and cp2108 starts responding to following
> valid commands also with stalls, effectively failing.
>
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@xxxxxxxxx>
> ---
> drivers/usb/serial/cp210x.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 69 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 352fe63..eb02184 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -199,6 +199,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>
> struct cp210x_port_private {
> __u8 bInterfaceNumber;
> + bool cp2108_with_get_line_ctl_swap;
Odd indentation. Either match the previous line or use a single space.
Please also drop the cp2108 prefix and rename the flag something like
has_swapped_line_ctl.
> };
>
> static struct usb_serial_driver cp210x_device = {
> @@ -419,6 +420,63 @@ static inline int cp210x_set_config_single(struct usb_serial_port *port,
> }
>
> /*
> + * 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.
> + * Preserve the original register value.
> + */
> +static int cp210x_activate_workarounds(struct usb_serial_port *port)
Rename this to something more specific like
cp210x_detect_swapped_line_ctl.
> +{
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + unsigned int line_ctl_save;
> + unsigned int line_ctl_test;
> + int err;
> +
> + port_priv->cp2108_with_get_line_ctl_swap = false;
You can drop this and rely on the private data being kzalloced.
> +
> + err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_save, 2);
> + if (err)
> + return err;
> +
> + line_ctl_test = 0x800;
> + err = cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_test, 2);
> + if (err)
> + return err;
> +
> + err = cp210x_get_config(port, CP210X_GET_LINE_CTL, &line_ctl_test, 2);
> + if (err)
> + return err;
> +
> + if (line_ctl_test == 8) {
> + port_priv->cp2108_with_get_line_ctl_swap = true;
> + swab16s((u16 *)(&line_ctl_save));
This will not work on a big-endian machine. Just use swab16 here.
> + }
> +
> + return cp210x_set_config(port, CP210X_SET_LINE_CTL, &line_ctl_save, 2);
> +}
> +
> +/*
> + * Must always be called instead of cp210x_get_config(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 *pctrl)
Drop the pointer prefix (p) from the parameter name.
> +{
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + int err;
> +
> + err = cp210x_get_config(port, CP210X_GET_LINE_CTL, pctrl, 2);
> + if (err)
> + return err;
> +
> + /* Workaround swapped bytes in 16-bit value from CP210X_GET_LINE_CTL */
> + if (port_priv->cp2108_with_get_line_ctl_swap)
> + swab16s((u16 *) pctrl);
Same as above.
> +
> + return 0;
> +}
> +
> +/*
> * cp210x_quantise_baudrate
> * Quantises the baud rate as per AN205 Table 1
> */
> @@ -535,7 +593,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>
> cflag = *cflagp;
>
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
> + cp210x_get_line_ctl(port, &bits);
> cflag &= ~CSIZE;
> switch (bits & BITS_DATA_MASK) {
> case BITS_DATA_5:
> @@ -703,7 +761,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
>
> /* If the number of data bits is to be updated */
> if ((cflag & CSIZE) != (old_cflag & CSIZE)) {
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
> + cp210x_get_line_ctl(port, &bits);
> bits &= ~BITS_DATA_MASK;
> switch (cflag & CSIZE) {
> case CS5:
> @@ -737,7 +795,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
>
> if ((cflag & (PARENB|PARODD|CMSPAR)) !=
> (old_cflag & (PARENB|PARODD|CMSPAR))) {
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
> + cp210x_get_line_ctl(port, &bits);
> bits &= ~BITS_PARITY_MASK;
> if (cflag & PARENB) {
> if (cflag & CMSPAR) {
> @@ -763,7 +821,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
> }
>
> if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
> - cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
> + cp210x_get_line_ctl(port, &bits);
> bits &= ~BITS_STOP_MASK;
> if (cflag & CSTOPB) {
> bits |= BITS_STOP_2;
> @@ -883,16 +941,23 @@ static int cp210x_port_probe(struct usb_serial_port *port)
> struct usb_serial *serial = port->serial;
> struct usb_host_interface *cur_altsetting;
> struct cp210x_port_private *port_priv;
> + int err;
>
> port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
> if (!port_priv)
> return -ENOMEM;
>
> + /* register access functions use this port_priv field */
That is true, but you could do away with bInterfaceNumber in a follow up
patch and just access it through the usb_serial struct.
Comment is not needed here either way.
> cur_altsetting = serial->interface->cur_altsetting;
> port_priv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
>
> + /* must be set before calling register access functions */
No need to be explicit about having to initialise the port private data
before using it. Just drop this comment as well.
> usb_set_serial_port_data(port, port_priv);
>
> + err = cp210x_activate_workarounds(port);
> + if (err)
> + kfree(port_priv);
> +
> return 0;
> }
The first two patches look good. Care to fix up the above and send a v4
(you forgot the patch revision in Subject)?
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
