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

[Alex Elder <elder@riscstar.com>]

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