Re: [PATCH v2 4/6] USB: serial: ch341: Name prescaler, divisor registers

From: Johan Hovold <johan@kernel.org>
To: Michael Hanselmann <public@hansmi.ch>
Cc: Johan Hovold <johan@kernel.org>,
	linux-usb@vger.kernel.org, Michael Dreher <michael@5dot1.de>,
	Jonathan Olds <jontio@i4free.co.nz>
Subject: Re: [PATCH v2 4/6] USB: serial: ch341: Name prescaler, divisor registers
Date: Mon, 29 Jun 2020 11:51:20 +0200	[thread overview]
Message-ID: <20200629095120.GR3334@localhost> (raw)
In-Reply-To: <2e80916d-1be8-dc0f-abf9-adc0feea1803@msgid.hansmi.ch>

On Wed, May 27, 2020 at 10:59:53PM +0200, Michael Hanselmann wrote:
> On 14.05.20 16:24, Johan Hovold wrote:
> > On Tue, Mar 31, 2020 at 11:37:20PM +0000, Michael Hanselmann wrote:

> >> The 0x25 register is only used by CH341 chips before version 0x30 and is
> >> involved in configuring the line control parameters. It's not known to
> >> the author whether there any such chips in the wild, and the driver
> >> never supported them (other registers are also treated differently). The
> >> alternative would've been to not set the register, but that may have
> >> unintended effects.
> > 
> > How did you come to those conclusions? I see this register being written
> > the value zero in some older version of the vendor driver, but not in
> > more recent ones.
> >
> > Are you sure it's at all related to LCR?
> 
> I am looking at version 3.50.2014.8 from 2019-03-04 of CH341S64.SYS, the
> vendor driver. The function configuring registers 0x18 and 0x25 starts
> at offset 0x119f0.
> 
> For chip versions 0x30 and above 0x25 is always set to 0. For versions
> before 0x30 the values stored in register 0x25 depend on stop bit,
> partity and word length settings.

Great, thanks for confirming.

> From aa597afe1e9cf5641b94b377ce63248c2d0d677a Mon Sep 17 00:00:00 2001
> From: Michael Hanselmann <public@hansmi.ch>
> Date: Wed, 27 May 2020 22:56:54 +0200
> Subject: [PATCH] USB: serial: ch341: Name prescaler, divisor registers
> 
> Add constants for the prescaler and divisor registers. Document and
> name register 0x25, and put the LCR define to more use.
> 
> The 0x25 register (CH341_REG_LCR2) is only used by CH341 chips before
> version 0x30 and is involved in configuring the line control parameters.
> It's not known to the author whether there any such chips in the wild,
> and Linux' ch341 driver never supported them. For chip version 0x30 and
> above the 0x25 register is always set to zero. The alternative would've
> been to not set the register at all, but that may have unintended
> effects.
> 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 89675ee29645..97214e1ec5d2 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -59,7 +59,11 @@
>  #define CH341_REQ_MODEM_CTRL   0xA4
>  
>  #define CH341_REG_BREAK        0x05
> +#define CH341_REG_PRESCALER    0x12
> +#define CH341_REG_DIVISOR      0x13
>  #define CH341_REG_LCR          0x18
> +#define CH341_REG_LCR2         0x25
> +
>  #define CH341_NBREAK_BITS      0x01
>  
>  #define CH341_LCR_ENABLE_RX    0x80
> @@ -246,11 +250,20 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  	 */
>  	val |= BIT(7);
>  
> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
> +			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
> +			      val);
>  	if (r)
>  		return r;
>  
> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
> +	/*
> +	 * Chip versions before version 0x30 as read using
> +	 * CH341_REQ_READ_VERSION used separate registers for line control
> +	 * (word length, parity and word length). Version 0x30 and above use

I replaced the first "word length" with "stop bits" here before applying.

> +	 * CH341_REG_LCR only and CH341_REG_LCR2 is always set to zero.
> +	 */
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
> +			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
>  	if (r)
>  		return r;

Johan