Re: [PATCH] tty: serial: samsung: Correct clock selection logic

[Krzysztof Kozlowski <krzk@kernel.org>]

On Fri, May 08, 2020 at 06:34:33PM -0700, Jonathan Bakker wrote:
> Some variants of the samsung tty driver can pick which clock
> to use for their baud rate generation.  In the DT conversion,
> a default clock was selected to be used if a specific one wasn't
> assigned and then a comparison of which clock rate worked better
> was done.  Unfortunately, the comparison was implemented in such
> a way that only the default clock was ever actually compared.
> Fix this by iterating through all possible clocks, except when a
> specific clock has already been picked via clk_sel (which is
> only possible via board files).
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/tty/serial/samsung_tty.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..9d2b4be44209 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1281,14 +1281,14 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
>  	struct s3c24xx_uart_info *info = ourport->info;
>  	struct clk *clk;
>  	unsigned long rate;
> -	unsigned int cnt, baud, quot, clk_sel, best_quot = 0;
> +	unsigned int cnt, baud, quot, best_quot = 0;
>  	char clkname[MAX_CLK_NAME_LENGTH];
>  	int calc_deviation, deviation = (1 << 30) - 1;
>  
> -	clk_sel = (ourport->cfg->clk_sel) ? ourport->cfg->clk_sel :
> -			ourport->info->def_clk_sel;
>  	for (cnt = 0; cnt < info->num_clks; cnt++) {
> -		if (!(clk_sel & (1 << cnt)))
> +		/* Keep selected clock if provided */

Makes sense and good catch.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

I wonder about the s3c24xx_serial_enable_baudclk() which has similar
pattern - is there
testing only def_clk_sel on purpose?

Best regards,
Krzysztof

> +		if (ourport->cfg->clk_sel &&
> +			!(ourport->cfg->clk_sel & (1 << cnt)))
>  			continue;
>  
>  		sprintf(clkname, "clk_uart_baud%d", cnt);
> -- 
> 2.20.1
>