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
>
>
