diff mbox series

[v3,3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure

Message ID 20200506233136.11842-4-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State New, archived
Headers show
Series [v3,1/4] serial: 8250: Fix max baud limit in generic 8250 port | expand

Commit Message

Serge Semin May 6, 2020, 11:31 p.m. UTC 
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.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Long Cheng <long.cheng@mediatek.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-mips@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-mediatek@lists.infradead.org
---
 drivers/tty/serial/8250/8250_dw.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

Comments

Andy Shevchenko May 15, 2020, 2:05 p.m. UTC | #1 
On Thu, May 07, 2020 at 02:31:34AM +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.

>  	rate = clk_round_rate(d->clk, baud * 16);
> -	if (rate < 0)
> -		ret = rate;

> -	else if (rate == 0)
> -		ret = -ENOENT;

This case now handled differently.
I don't think it's good idea to change semantics.

So, I don't see how this, after leaving the rate==0 case, would be better than
original one.

> -	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;
> -
> -out:
>  	p->status &= ~UPSTAT_AUTOCTS;
>  	if (termios->c_cflag & CRTSCTS)
>  		p->status |= UPSTAT_AUTOCTS;
Serge Semin May 15, 2020, 2:50 p.m. UTC | #2 
On Fri, May 15, 2020 at 05:05:47PM +0300, Andy Shevchenko wrote:
> On Thu, May 07, 2020 at 02:31:34AM +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.
> 
> >  	rate = clk_round_rate(d->clk, baud * 16);
> > -	if (rate < 0)
> > -		ret = rate;
> 
> > -	else if (rate == 0)
> > -		ret = -ENOENT;
> 
> This case now handled differently.
> I don't think it's good idea to change semantics.
> 
> So, I don't see how this, after leaving the rate==0 case, would be better than
> original one.

Semantic doesn't change. The code does exactly the same as before. If it didn't
I either would have provided a comment about this or just didn't introduce the
change in the first place. I guess you just don't see the whole picture of the
method. Take a look in the code. The ret variable's been used to skip the
"p->uartclk = rate" assignment. That's it. So the (rate == 0) will still be
considered as error condition, which causes the clock rate left unchanged.
Here is the code diff so you wouldn't need to dive deep into the driver
sources:

<	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
<		ret = clk_set_rate(d->clk, rate);
<	clk_prepare_enable(d->clk);
<
<	if (ret)
<		goto out;
<
<	p->uartclk = rate;
<
<out:
---
>       clk_disable_unprepare(d->clk);
>       rate = clk_round_rate(d->clk, baud * 16);
>       if (rate > 0) {
>              ret = clk_set_rate(d->clk, rate);
>              if (!ret)
>                      p->uartclk = rate;
>       }
>       clk_prepare_enable(d->clk);

-Sergey

> 
> > -	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;
> > -
> > -out:
> >  	p->status &= ~UPSTAT_AUTOCTS;
> >  	if (termios->c_cflag & CRTSCTS)
> >  		p->status |= UPSTAT_AUTOCTS;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko May 15, 2020, 3:05 p.m. UTC | #3 
On Fri, May 15, 2020 at 05:50:07PM +0300, Serge Semin wrote:
> On Fri, May 15, 2020 at 05:05:47PM +0300, Andy Shevchenko wrote:
> > On Thu, May 07, 2020 at 02:31:34AM +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.
> > 
> > >  	rate = clk_round_rate(d->clk, baud * 16);
> > > -	if (rate < 0)
> > > -		ret = rate;
> > 
> > > -	else if (rate == 0)
> > > -		ret = -ENOENT;
> > 
> > This case now handled differently.
> > I don't think it's good idea to change semantics.
> > 
> > So, I don't see how this, after leaving the rate==0 case, would be better than
> > original one.
> 
> Semantic doesn't change. The code does exactly the same as before. If it didn't
> I either would have provided a comment about this or just didn't introduce the
> change in the first place. I guess you just don't see the whole picture of the
> method. Take a look in the code. The ret variable's been used to skip the
> "p->uartclk = rate" assignment. That's it. So the (rate == 0) will still be
> considered as error condition, which causes the clock rate left unchanged.
> Here is the code diff so you wouldn't need to dive deep into the driver
> sources:
> 
> <	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
> <		ret = clk_set_rate(d->clk, rate);
> <	clk_prepare_enable(d->clk);
> <
> <	if (ret)
> <		goto out;
> <
> <	p->uartclk = rate;
> <
> <out:
> ---
> >       clk_disable_unprepare(d->clk);
> >       rate = clk_round_rate(d->clk, baud * 16);
> >       if (rate > 0) {
> >              ret = clk_set_rate(d->clk, rate);
> >              if (!ret)
> >                      p->uartclk = rate;
> >       }
> >       clk_prepare_enable(d->clk);

Thanks.
Indeed, in the above it looks clear.
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index aab3cccc6789..12866083731d 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -282,20 +282,13 @@  static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 
 	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;
-
-out:
 	p->status &= ~UPSTAT_AUTOCTS;
 	if (termios->c_cflag & CRTSCTS)
 		p->status |= UPSTAT_AUTOCTS;