| Message ID | 20200506233136.11842-4-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: 8250_dw: Fix ref clock usage | expand |
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;
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 > >
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 --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;
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(-)