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@xxxxxxxxxxxx>
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
