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.
--
With Best Regards,
Andy Shevchenko
