Re: [PATCH] USB: serial: pl2303: PL2303HXN can support baud rate are set directly

[Johan Hovold <johan@kernel.org>]

On Wed, Dec 25, 2019 at 09:30:05PM +0800, Charles Yeh wrote:
> PL2303HXN (TYPE_HXN) can program form 1 bps to 12000000 bps and
> support standard & non-standard baud rates (Note 1) are set directly
> It doesn't need complicated baud rate division calculation.

What's the benefit of this over using divisors directly?

If there's something wrong with that algorithm I think we should fix
that instead so that all device types benefit.

> Note 1:
> Standard baud rate:
> 75, 150, 300, 600, 1200, 1800, 2400, 3600, 4800, 7200, 9600,14400,
> 19200, 28800, 38400, 57600, 115200, 230400, 460800,614400, 921600,
> 1228800, 2457600, 3000000, 6000000
> 
> Non-standard baud rate (1 ~ 12000000):
> 1, 2, 5, 22, 55, 60, 75, 80, 123, 130, 150, 180, 187, 200, 300, 340,
> 400,..... 115200, 230400, 460800, 474747, 515151, 614400, 921600,
> .. 1000000,.. 7000000,.. 12000000

Which rate does HXN pick if it's not in either of these lists (e.g.
next lower/higher, etc)?

> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
> ---
>  drivers/usb/serial/pl2303.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index aab737e1e7b6..63d354a92db9 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -565,17 +565,21 @@ static void pl2303_encode_baud_rate(struct tty_struct *tty,
>  	if (!baud)
>  		return;
>  
> -	if (spriv->type->max_baud_rate)
> -		baud = min_t(speed_t, baud, spriv->type->max_baud_rate);

You still need this for HXN I guess.

> -	/*
> -	 * Use direct method for supported baud rates, otherwise use divisors.
> -	 */
> -	baud_sup = pl2303_get_supported_baud_rate(baud);
> -
> -	if (baud == baud_sup)
> +	if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>  		baud = pl2303_encode_baud_rate_direct(buf, baud);
> -	else
> -		baud = pl2303_encode_baud_rate_divisor(buf, baud);
> +	} else {
> +		if (spriv->type->max_baud_rate)
> +			baud = min_t(speed_t, baud, spriv->type->max_baud_rate);
> +		/*
> +		 * Use direct method for supported baud rates, otherwise use divisors.
> +		 */
> +		baud_sup = pl2303_get_supported_baud_rate(baud);
> +
> +		if (baud == baud_sup)
> +			baud = pl2303_encode_baud_rate_direct(buf, baud);
> +		else
> +			baud = pl2303_encode_baud_rate_divisor(buf, baud);
> +	}

You need to find a better way to abstract this. We can't just add
conditionals like this throughout the code without reorganising it or it
will become very hard to read and maintain.

>  
>  	/* Save resulting baud rate */
>  	tty_encode_baud_rate(tty, baud, baud);

Johan