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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_YhwJ1ZGSodMcMH@smile.fi.intel.com>
Date: Wed, 9 Apr 2025 10:29:04 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Alex Elder <elder@...cstar.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 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?

> > 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.

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

-- 
With Best Regards,
Andy Shevchenko