Re: [PATCH v7 2/3] serial: 8250_dw: Simplify the ref clock rate setting procedure

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Fri, Jun 19, 2020 at 11:02:50PM +0300, Serge Semin wrote:
> Really instead of twice checking the clk_round_rate() return value
> we could do it once, and if it isn't error the clock rate can be changed.
> By doing so we decrease a number of ret-value tests and remove a weird
> goto-based construction implemented in the dw8250_set_termios() method.

This doesn't look right to me - neither the before code nor the after
code.

>  	clk_disable_unprepare(d->clk);
>  	rate = clk_round_rate(d->clk, baud * 16);
> -	if (rate < 0)
> -		ret = rate;
> -	else if (rate == 0)
> -		ret = -ENOENT;
> -	else
> +	if (rate > 0) {
>  		ret = clk_set_rate(d->clk, rate);
> +		if (!ret)
> +			p->uartclk = rate;
> +	}
>  	clk_prepare_enable(d->clk);
>  
> -	if (ret)
> -		goto out;
> -
> -	p->uartclk = rate;

	newrate = baud * 16;

	clk_disable_unprepare(d->clk);
	rate = clk_round_rate(newrate);
	ret = clk_set_rate(d->clk, newrate);
	if (!ret)
		p->uartclk = rate;

	ret = elk_prepare_enable(d->clk);
	/* check ret for failure, means the clock is no longer running */

is all that should be necessary: note that clk_round_rate() is required
to return the rate that a successful call to clk_set_rate() would result
in for that clock.  It is equivalent to:

	ret = clk_set_rate(d->clk, newrate);
	if (ret == 0)
		p->uartclk = clk_get_rate(d->clk);

The other commonly misunderstood thing about the clk API is that the
rate you pass in to clk_round_rate() to discover the actual clock rate
and the value passed in to clk_set_rate() _should_ be the same value.
You should _not_ do clk_set_rate(clk, clk_round_rate(clk, newrate));

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!