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 <konstantin.shkolnyy@xxxxxxxxx>
> ---
> @@ -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
--
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
