Re: [PATCH 2/2] serial: 8250_of: add support for an optional bus clock

[Alex Elder]

On 4/8/25 2:52 PM, Andy Shevchenko wrote:
> On Tue, Apr 08, 2025 at 12:51:43PM -0500, Alex Elder wrote:
> > The SpacemiT UART requires a bus clock to be enabled, in addition to
> > it's "normal" core clock.  Look up the core clock by name, and if
> > that's found, look up the bus clock.  If named clocks are used, both
> > are required.
> >
> > Supplying a bus clock is optional.  If no bus clock is needed, the clocks
> > aren't named and we only look up the first clock.
>
> Code and description are not aligned. And What you are described above make
> more sense to me than what's done below.

I want to do this the right way.

My explanation says:
- look up the core clock by name
    - if that is found, look up the bus clock
    - both clocks are required in this case (error otherwise)
- If the "core" clock is not found:
    - look up the first clock.

And my code does:
- look up the core clock by name (not found is OK)
    - if it is found, look up the bus clock by name
    - If that is not found or error, it's an error
- if the "core" clock is not found
    - look up the first clock

What is not aligned?


> Also can we simply not not touch this conditional at all, but just add
> a new one before? Like
>
> 	/* Get clk rate through clk driver if present */
>
> 	/* Try named clock first */
> 	if (!port->uartclk) {
> 		...
> 	}
>
> 	/* Try unnamed core clock */
> // the below is just an existing code.

That's easy enough.  I think it might be a little more code
but I have no problem with that.

> 	if (!port->uartclk) {
> 		...
> 	}
>
> ...
>
> >   	/* Get clk rate through clk driver if present */
> >   	if (!port->uartclk) {
> > -		info->clk = devm_clk_get_enabled(dev, NULL);
> > +		info->clk = devm_clk_get_optional_enabled(dev, "core");
> >   		if (IS_ERR(info->clk)) {
> > -			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> > +			ret = dev_err_probe(dev, PTR_ERR(info->clk),
> > +					    "failed to get core clock\n");
>
> Can be still one line.
>
> >   			goto err_pmruntime;
> >   		}
> >   
> > +		/* If we got the core clock, look for a bus clock */
> > +		if (info->clk) {
> > +			info->bus_clk = devm_clk_get_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");
>
> Something with indentation happened here and below.
>
> > +				goto err_pmruntime;
> > +			}
> > +		} else {
> > +			/* Fall back to getting the one unnamed clock */
> > +			info->clk = devm_clk_get_enabled(dev, NULL);
> > +			if (IS_ERR(info->clk)) {
> > +				ret = dev_err_probe(dev, PTR_ERR(info->clk),
> > +						"failed to get clock\n");
> > +				goto err_pmruntime;
> > +			}
> > +		}
> > +
> >   		port->uartclk = clk_get_rate(info->clk);
> >   	}
>
> > +
>
> Stray change.

Sorry, I didn't notice that.  Next time it won't be there.

Thanks for your (quick) review.

					-Alex


> >   	/* If current-speed was set, then try not to change it. */
> >   	if (of_property_read_u32(np, "current-speed", &spd) == 0)
> >   		port->custom_divisor = port->uartclk / (16 * spd);