Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Alex Elder]

On 4/11/25 2:44 PM, Andy Shevchenko wrote:
> On Fri, Apr 11, 2025 at 10:36 PM Alex Elder <elder@xxxxxxxxxxxx> wrote:
> > On 4/11/25 2:30 PM, Andy Shevchenko wrote:
> > > Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti:
> > > > Save the bus clock pointer in the of_serial_info structure, and use
> > > > that to disable the bus clock on suspend and re-enable it on resume.
>
> ...
>
> > > >       if (!port->uartclk) {
> > > > -            struct clk *bus_clk;
> > > > -
> > > > -            bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> > > > -            if (IS_ERR(bus_clk)) {
> > > > -                    ret = dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
> > > > +            info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> > > > +            if (IS_ERR(info->bus_clk)) {
> > > > +                    ret = dev_err_probe(dev, PTR_ERR(info->bus_clk),
> > > > +                                        "failed to get bus clock\n");
> > > >                       goto err_pmruntime;
> > > >               }
> > > >
> > > >               /* If the bus clock is required, core clock must be named */
> > > > -            info->clk = devm_clk_get_enabled(dev, bus_clk ? "core" : NULL);
> > > > +            info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL);
> > > >               if (IS_ERR(info->clk)) {
> > > >                       ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> > >
> > > While the first patch against this file looks okay now, this one inherits the
> > > same problem (seems like not enought thinking about the code representation).
> > >
> > > Instead of rewritting half of the lines you just introduced (which is also a
> > > bad practice), add a one-liner that assigns a field to the local variable.
> >
> > So you want me to re-spin this again so that I use the local variable?
>
> Yes.
>
> > I understand what you're saying based on ease of review,
>
> No, not only review. Here the main issue is ping-pong between patches.
> If you know ahead that these lines should be changed, do it from the
> start. But I understand the need to have separate changes for
> logically different pieces. That's why

I did do it from the start (v1 of the series).  Then I reworked
it based on your feedback and concluded there was no need for
keeping a copy of the bus clock in the of_serial_info structure.
In the interest of keeping the code simple I dropped it.

Yixun asked me to disable/enable the bus clock in suspend/resume,
so I was preparing the patches for that, when Greg informed me
he had already accepted the first two patches.  So I added this
new feature on the end.  This is the reason for this "ping pong."
I would have done it from the start in this series otherwise in v3.

Anyway, I'm sending v4 shortly.  I just have to run it through
another test.  It is indeed a much simpler change.

					-Alex

> > but this
> > is a simple patch
>
> It doesn't matter how simple it is.
>
> > and the change is very understandable, and the
> > code is no more or less clear when using the local variable.
>
> See above.
>
> > > >                       goto err_pmruntime;