Re: [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration

Johan Hovold
2016-04-29T13:03:21Z

On Fri, Apr 15, 2016 at 11:14:08PM +0200, Grigori Goronzy wrote:
> Changing the LCR register after initialization does not seem to be
> reliable on all chips (particularly not on CH341A). Restructure
> initialization and configuration to always reinit the chip on
> configuration changes instead and pass the LCR register value directly
> to the initialization command.
>
> v2: get rid of unused variable, improve error handling.
>
> Tested-by: Ryan Barber <rfb@skyscraper.nu>
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
> drivers/usb/serial/ch341.c | 47 ++++++++++++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 95c8a40..6181616 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -61,6 +61,8 @@
> * the Net/FreeBSD uchcom.c driver by Takanori Watanabe. Domo arigato.
> */
>
> +#define CH341_SERIAL_INIT 0xA1
> +#define CH341_VERSION 0x5F

Use the CH341_REQ prefix keep the entries sorted.

> #define CH341_MODEM_CTRL 0xA4
> #define CH341_REQ_WRITE_REG 0x9A
> #define CH341_REQ_READ_REG 0x95
> @@ -129,10 +131,10 @@ static int ch341_control_in(struct usb_device *dev,
> return r;
> }
>
> -static int ch341_set_baudrate(struct usb_device *dev,
> - struct ch341_private *priv)
> +static int ch341_init_set_baudrate(struct usb_device *dev,
> + struct ch341_private *priv, unsigned ctrl)
> {
> - short a, b;
> + short a;
> int r;
> unsigned long factor;
> short divisor;
> @@ -152,11 +154,8 @@ static int ch341_set_baudrate(struct usb_device *dev,
>
> factor = 0x10000 - factor;
> a = (factor & 0xff00) | divisor;
> - b = factor & 0xff;
>
> - r = ch341_control_out(dev, 0x9a, 0x1312, a);
> - if (!r)
> - r = ch341_control_out(dev, 0x9a, 0x0f2c, b);
> + r = ch341_control_out(dev, CH341_SERIAL_INIT, 0x9c | (ctrl << 8), a | 0x80);
>
> return r;
> }
> @@ -177,7 +176,7 @@ static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
> if (!buffer)
> return -ENOMEM;
>
> - r = ch341_control_in(dev, 0x95, 0x0706, 0, buffer, size);
> + r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x0706, 0, buffer, size);

This looks like an unrelated change.

> if (r < 0)
> goto out;
>
> @@ -207,24 +206,20 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
> return -ENOMEM;
>
> /* expect two bytes 0x27 0x00 */
> - r = ch341_control_in(dev, 0x5f, 0, 0, buffer, size);
> + r = ch341_control_in(dev, CH341_VERSION, 0, 0, buffer, size);

Ditto.

> if (r < 0)
> goto out;
>
> - r = ch341_control_out(dev, 0xa1, 0, 0);
> - if (r < 0)
> - goto out;
> -
> - r = ch341_set_baudrate(dev, priv);
> + r = ch341_control_out(dev, CH341_SERIAL_INIT, 0, 0);
> if (r < 0)
> goto out;
>
> /* expect two bytes 0x56 0x00 */
> - r = ch341_control_in(dev, 0x95, 0x2518, 0, buffer, size);
> + r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x2518, 0, buffer, size);

So does this.

Please fix up all the magic constants in a preparatory patch, which
should make it easier to see what's really going on here.

Thanks,
Johan