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

[Alex Elder]

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?

I understand what you're saying based on ease of review, but this
is a simple patch and the change is very understandable, and the
code is no more or less clear when using the local variable.

					-Alex


> >   			goto err_pmruntime;