On Wed, Jul 15, 2015 at 01:35:58PM +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 using 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 #2 or too fast due to #2 and #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>
I took another look at this one and discovered an issue that should be
addressed.
Pointing out two style issues I had fixed locally as well.
> ---
> drivers/usb/serial/pl2303.c | 32 +++++++++++++++++++++++---------
> 1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index f5257af..3f9a808 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -362,22 +362,36 @@ 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;
> + u32 baseline, mantissa, exponent;
Please keep these as unsigned int.
> /*
> * 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;
> + baseline = 12000000 * 32;
> + mantissa = baseline / baud;
> + exponent = 0;
> + while (mantissa >= 512) {
> + if (exponent < 7) {
> + 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;
>
> + /* Calculate and return the exact baud rate. */
> + baud = (baseline / mantissa) >> (exponent << 1);
You should handle division by zero here. It cannot currently happen as
we cap the baudrate, but you should handle it here nonetheless. You can
still assume a non-zero baudrate, though.
Please add a newline before returning as well.
> return baud;
> }
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
