Message-ID: <0ca876ea-2607-407f-a0e8-98bb4bd94135@riscstar.com>
Date: Wed, 9 Apr 2025 07:10:15 -0500
From: Alex Elder <elder@...cstar.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, benjamin.larsson@...exis.eu,
 bastien.curutchet@...tlin.com, u.kleine-koenig@...libre.com, lkundrak@...sk,
 devicetree@...r.kernel.org, linux-serial@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] serial: 8250_of: add support for an optional bus
 clock

On 4/9/25 2:29 AM, Andy Shevchenko wrote:
> On Tue, Apr 08, 2025 at 03:11:10PM -0500, Alex Elder wrote:
>> 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?
> 
> That you are telling that bus is optional and core is not, the code does the
> opposite (and yes, I understand the logic behind, but why not doing the same in
> the code, i.e. check for the *optional* bus clock first?

Ahh, now I see what you mean.  The result will be the same,
but if it "reads better" that way to you then I'm all for it.

One of the reasons I did it this way was that I wasn't sure
how to express a "don't care" clock name as a DTS binding,
so I just tried to avoid that.

In other words, I thought about adding the "bus" clock as an
optional first lookup, and then leaving the existing code to
look up the core clock by position--without caring about the
name.  But I assume named clocks aren't guaranteed to be in
any particular order ("core" clock *could* be listed second).

So I look up the "core" clock by (optional) name, and if not
found look it up by position.  If it is found, I look up the
bus clock--required in this case.

Now that I write that I understand why you felt the logic was
a bit inverted.

I'll send v2 today and will rearrange the logic to match what
you're talking about.

>>> 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.
> 
> I;m fine with a little more code, but it will be much cleaner in my point of
> view and as a bonus the diff will be easier to review.

I understand that completely.  Thanks a lot for clarifying.

					-Alex

>>> 	if (!port->uartclk) {
>>> 		...
>>> 	}
> 

Powered by blists - more mailing lists