Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Johan Hovold <johan@kernel.org>]

On Sat, Jun 08, 2019 at 05:13:09PM +1200, jontio wrote:
> For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
> value such that it produces a significantly different baud rate than the
> desired one. This means some hardware can't communicate with the CH34x
> chip. Particularly obvious wrong baud rates are 256000 and 921600 which
> deviate by 2.3% and 7.4% respectively. This proposed patch will bring the
> errors for these baud rates to below 0.5%. This patch will significantly
> improve the error of some other unusual baud rates too (such as 1333333
> from 10% error to 0% error). Currently ch341_set_baudrate_lcr() will
> accept any baud rate and can produce a practically arbitrary large error
> (for example a 40% error for 5000000) this patch does not address this
> issue.

It doesn't hurt to expand the commit message with further details from
your other mail.

> Signed-off-by: jontio <jontio@i4free.co.nz>

You need to sign off with your full name. It should also match the From
line (author).

> ---
>  drivers/usb/serial/ch341.c | 45 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 3bb1fff02bed..7cd1d6f70b56 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -54,6 +54,9 @@
>  #define CH341_BAUDBASE_FACTOR 1532620800
>  #define CH341_BAUDBASE_DIVMAX 3
>  
> +/* Chip frequency is 12Mhz. not quite the same as (CH341_BAUDBASE_FACTOR>>7) */
> +#define CH341_OSC_FREQUENCY 12000000
> +
>  /* Break support - the information used to implement this was gleaned from
>   * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
>   */
> @@ -168,6 +171,48 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  	factor = 0x10000 - factor;
>  	a = (factor & 0xff00) | divisor;
>  
> +	/*
> +	 * Calculate baud error using the 0,1,2,3 LSB and
> +	 * also the error without the divisor (LSB==7).
> +	 * Decide whether the divisor should be used.

Wrap also comments at 72 cols or so.

> +	 */
> +	uint32_t msB = (a>>8) & 0xFF;
> +	uint32_t lsB = a & 0xFF;
> +	int32_t baud_wanted = priv->baud_rate;
> +	uint32_t denom = ((1<<(10-3*lsB))*(256-msB));

It's not obvious from just looking at the above chunk that 3*lsB < 10.

And some style issues:

 - declare variables at the start of the function (or possibly start of
   block), and defer non-trivial initialisation
 - use the kernel types u32, s32 or plain (unsigned) int instead of the
   c99 types.
 - no camel case, msb, lsb is fine
 - add a space on both sides of operators (also in your comments)
 - drop the denom outmost parenthesis (also in some expressions below)
 - please use lowercase hex notation for consistency with the rest of
   the driver (function)

> +	/*
> +	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no divisor
> +	 * the 100 is for rounding.
> +	 */
> +	if (denom && ((baud_wanted+100) >= (((uint32_t)CH341_OSC_FREQUENCY)>>8))) {
> +
> +		/* Calculate error for divisor */
> +		int32_t baud_expected = ((uint32_t)CH341_OSC_FREQUENCY) / denom;
> +		uint32_t baud_error_difference = abs(baud_expected-baud_wanted);
> +
> +		/* Calculate a for no divisor */
> +		uint32_t a_no_divisor = ((0x10000-(((uint32_t)CH341_OSC_FREQUENCY)<<8) /
> +			baud_wanted+128) & 0xFF00) | 0x07;
> +
> +		/* a_no_divisor is only valid for MSB<248 */
> +		if ((a_no_divisor>>8) < 248) {
> +
> +			/* Calculate error for no divisor */
> +			int32_t baud_expected_no_divisor = ((uint32_t)CH341_OSC_FREQUENCY) /
> +				(256-(a_no_divisor>>8));
> +			uint32_t baud_error_difference_no_divisor =
> +				abs(baud_expected_no_divisor-baud_wanted);
> +
> +			/*
> +			 * If error using no divisor is less than using
> +			 * a divisor then use it instead for the "a" word.
> +			 */
> +			if (baud_error_difference_no_divisor < baud_error_difference)
> +				a = a_no_divisor;
> +		}
> +

Stray newline.

> +	}
> +
>  	/*
>  	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
>  	 * has been received unless bit 7 is set.

Ok, I'm gonna have to look at this again, but perhaps you can consider
the style input meanwhile.

Johan