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

Alex Elder
2025-04-08T20:11:13Z

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