Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux