From: Johan Hovold <johan@kernel.org>
To: jontio <jontio@i4free.co.nz>
Cc: johan@kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation
Date: Thu, 20 Jun 2019 15:43:18 +0200 [thread overview]
Message-ID: <20190620134318.GL6241@localhost> (raw)
In-Reply-To: <20190608051309.4689-1-jontio@i4free.co.nz>
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
next prev parent reply other threads:[~2019-06-20 13:43 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-08 5:13 [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation jontio
2019-06-20 13:43 ` Johan Hovold [this message]
2019-06-29 0:05 ` Jonathan Olds
2019-10-29 19:18 ` Jonathan Olds
2019-10-30 9:47 ` Johan Hovold
2019-11-01 0:17 ` Jonathan Olds
2019-11-01 9:57 ` Johan Hovold
2019-11-01 19:18 ` Jonathan Olds
2019-11-01 9:25 Michael Dreher
2019-11-01 14:20 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190620134318.GL6241@localhost \
--to=johan@kernel.org \
--cc=jontio@i4free.co.nz \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).