Re: [PATCH v2 5/6] USB: serial: ch341: Compute minimum baud rate

From: Johan Hovold <johan@kernel.org>
To: Michael Hanselmann <public@hansmi.ch>
Cc: linux-usb@vger.kernel.org, Johan Hovold <johan@kernel.org>,
	Michael Dreher <michael@5dot1.de>,
	Jonathan Olds <jontio@i4free.co.nz>
Subject: Re: [PATCH v2 5/6] USB: serial: ch341: Compute minimum baud rate
Date: Tue, 30 Jun 2020 11:57:56 +0200	[thread overview]
Message-ID: <20200630095756.GZ3334@localhost> (raw)
In-Reply-To: <de39436b-ba4d-73be-513f-72ccff0fec1b@msgid.hansmi.ch>

On Thu, May 28, 2020 at 12:19:30AM +0200, Michael Hanselmann wrote:
> The minimum baud rate was hardcoded, not computed from first principles.
> The break condition simulation added in a later patch will also need to
> make use of the minimum baud rate.
> 
> The (1 + ((x - 1) / y)) pattern is to force rounding up (mathematically
> the minimum rate is about 45.78bps).
> 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
> Rebase on top of usb-next and wording change in commit message.
> 
>  drivers/usb/serial/ch341.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 97214e1ec5d2..202cfa4ef6c7 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -147,6 +147,8 @@ static int ch341_control_in(struct usb_device *dev,
>  
>  #define CH341_CLKRATE		48000000
>  #define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * (ps) - (fact)))
> +#define CH341_MIN_BPS \
> +	(unsigned int)(1 + (((CH341_CLKRATE) - 1) / (CH341_CLK_DIV(0, 0) * 256)))

We have DIV_ROUND_UP(), and the cast can be avoided by using
clamp_val().

>  #define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV((ps), 1) * 512))
>  
>  static const speed_t ch341_min_rates[] = {
> @@ -174,10 +176,10 @@ static int ch341_get_divisor(struct ch341_private *priv)
>  	int ps;
>  
>  	/*
> -	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
> -	 * sanity checks below redundant.
> +	 * Clamp to supported range, making the later range sanity checks
> +	 * redundant.

This change seems uncalled for.

>  	 */
> -	speed = clamp(speed, 46U, 3000000U);
> +	speed = clamp(speed, CH341_MIN_BPS, 3000000U);
>  
>  	/*
>  	 * Start with highest possible base clock (fact = 1) that will give a

I replaced this patch with the below one adding both MIN and MAX macros
instead.

Johan


From fd1d3198e26696ba66e8ac2111acadd360eb86b3 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 29 Jun 2020 14:18:48 +0200
Subject: [PATCH] USB: serial: ch341: add min and max line-speed macros

The line-speed algorithm clamps the requested value to the supported
range instead of bailing out on unsupported values.

Provide min and max macros and indicate how they are derived instead of
hardcoding the limits.

Note that the algorithm depends on the minimum rate (45.78 bps)
being rounded up (and the maximum rate being rounded down) to avoid
special casing.

Suggested-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 684d595e7630..55a1c6dbeeb2 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -156,6 +156,10 @@ static const speed_t ch341_min_rates[] = {
 	CH341_MIN_RATE(3),
 };
 
+/* Supported range is 46 to 3000000 bps. */
+#define CH341_MIN_BPS	DIV_ROUND_UP(CH341_CLKRATE, CH341_CLK_DIV(0, 0) * 256)
+#define CH341_MAX_BPS	(CH341_CLKRATE / (CH341_CLK_DIV(3, 0) * 2))
+
 /*
  * The device line speed is given by the following equation:
  *
@@ -177,7 +181,7 @@ static int ch341_get_divisor(struct ch341_private *priv)
 	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
 	 * sanity checks below redundant.
 	 */
-	speed = clamp(speed, 46U, 3000000U);
+	speed = clamp_val(speed, CH341_MIN_BPS, CH341_MAX_BPS);
 
 	/*
 	 * Start with highest possible base clock (fact = 1) that will give a
-- 
2.26.2

