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

[Johan Hovold <johan@kernel.org>]

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