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: linux-usb@vger.kernel.org, Johan Hovold <johan@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: Thu, 14 May 2020 16:24:34 +0200	[thread overview]
Message-ID: <20200514142434.GF25962@localhost> (raw)
In-Reply-To: <65cfdf4a0600e86e89b3a3fb839e733ecebee688.1585697281.git.public@hansmi.ch>

On Tue, Mar 31, 2020 at 11:37:20PM +0000, Michael Hanselmann wrote:
> Add constants for the prescaler and divisor registers.

...and document and name register 0x25, and put the LCR define to more
use. I still thinks this should go in its own patch as your not
replacing all magic register constants (or at least be mentioned in the
commit message).

> 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?

> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 67a5d4c3df42..9407e12d9fbc 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -61,7 +61,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
> @@ -294,11 +298,19 @@ 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 (read using
> +	 * CH341_REQ_READ_VERSION) used separate registers for line control.
> +	 * 0x30 and above use CH341_REG_LCR only.
> +	 */
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
> +			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);

Keeping "0x25" here to indicate that it's purpose is still not known
should be ok too.

>  	if (r)
>  		return r;

Johan