Re: [PATCH v3 5/7] USB: serial: ftdi_sio: Fix baud rate rounding for ASYNC_SPD_CUST

[Johan Hovold <johan@kernel.org>]

On Sat, Sep 24, 2022 at 12:27:16PM +0200, Pali Rohár wrote:
> To compute more accurate baud rate when user uses ASYNC_SPD_CUST API,
> use DIV_ROUND_CLOSEST() instead of just rounding down.
> 
> Rationale:
>   Application uses old API, so it computes divisor D for baud rate B.

s/old/deprecated/

>   The driver then tries to compute back the requested baud rate B, but
>   rounds down in the division.
> 
>   Using rounding to closest value instead should increate accuracy here.

typo: increase

> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 1ab6bf48516f..718c86db2900 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1333,7 +1333,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  	if (baud == 38400 &&
>  	    ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) &&
>  	     (priv->custom_divisor)) {
> -		baud = priv->baud_base / priv->custom_divisor;
> +		baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor);
>  		dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n",
>  			__func__, priv->custom_divisor, baud);
>  	}

I'm having second thoughts about this one. The SPD_CUST hack should not
be used anymore, but it was supposed to be used to set the hardware
divisor directly. Someone was creative and reinterpreted this for for
the FTDI driver to mean software divisor instead. So instead of
understanding how the hardware determines rates from divisors, you know
needed knowledge of how the FTDI driver happens to work, including that
it rounds down. And now you're changing that again.

Perhaps we should just leave this as is. This interface has been
deprecated for decades and comes with a deprecated warning since several
years. Would be nice to drop it completely instead.

Johan