On Wed, Jul 08, 2015 at 12:51:03PM +0200, Michał Pecio wrote:
>
> This commit fixes the following issues:
>
> 1. The 9th bit of buf was believed to be the LSB of divisor's
> exponent, but the hardware interprets it as MSB (9th bit) of the
> mantissa. The exponent is actually one bit shorter and applies
> to base 4, not 2 as previously believed.
>
> 2. Loop iterations doubled the exponent instead of incrementing.
>
> 3. The exponent wasn't checked for overflow.
>
> 4. The function returned requested rate instead of actual rate.
>
> Due to issue #2, the old code deviated from the wrong formula
> described in #1 and actually yielded correct rates when divisor
> was lower than 4096 by giving exponents of 0, 2 or 4 base-2,
> interpreted as 0, 1, 2 base-4 with the 9th mantissa bit clear.
> However, at 93.75 kbaud or less the rate turned out too slow
> due to #1 and #2 or too fast due to #2 with #3.
>
> I tested this patch by sending and validating 0x00,0x01,..,0xff
> to an FTDI dongle at 234, 987, 2401, 9601, 31415, 115199, 250k,
> 500k, 750k, 1M, 1.5M, 3M+1 baud. All rates passed.
>
> I also used pv to check speed at some rates unsupported by FTDI:
> 45 (the lowest possible), 2M, 4M, 5M and 6M-1. Looked sane.
>
> Signed-off-by: Michal Pecio <michal.pecio@xxxxxxxxx>
> ---
> drivers/usb/serial/pl2303.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index f5257af..8141ccc 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -362,22 +362,34 @@ static speed_t pl2303_encode_baud_rate_direct(unsigned char buf[4],
> static speed_t pl2303_encode_baud_rate_divisor(unsigned char buf[4],
> speed_t baud)
> {
> - unsigned int tmp;
> -
> /*
> * Apparently the formula is:
> - * baudrate = 12M * 32 / (2^buf[1]) / buf[0]
> + * baudrate = 12M * 32 / (mantissa * 4^exponent)
> + * where
> + * mantissa = buf[8:0]
> + * exponent = buf[11:9]
> */
> - tmp = 12000000 * 32 / baud;
> + u32 baseline = 12000000 * 32;
> + u32 mantissa = baseline / baud;
> + u32 exponent = 0;
You'd get a checkpatch warning here about a missing new line after the
declarations (always run checkpatch.pl before submitting).
Please declare the variables at the top of the function and initialise
after the comment.
> + while (mantissa >= 512) {
> + if (likely(exponent < 7)) {
Don't use likely().
> + mantissa >>= 2; /* divide by 4 */
> + exponent++;
> + } else {
> + /* Exponent is maxed. Trim mantissa and leave. */
> + mantissa = 511;
> + break;
> + }
> + }
> +
> buf[3] = 0x80;
> buf[2] = 0;
> - buf[1] = (tmp >= 256);
> - while (tmp >= 256) {
> - tmp >>= 2;
> - buf[1] <<= 1;
> - }
> - buf[0] = tmp;
> + buf[1] = (exponent << 1) | (mantissa >> 8);
> + buf[0] = (mantissa & 0xff);
No parentheses.
>
> + /* Calculate and return the exact baud rate. */
> + baud = (baseline / mantissa) >> (exponent << 1);
> return baud;
> }
Looks nice and clean otherwise.
Were you already going to send a v2 or was this version complete?
Care to fix up the above style issues and resend so I can apply this for
4.2?
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
