[PATCH] clk: Disambiguate comment about clk_get_rate() for disabled clocks

[PATCH] clk: Disambiguate comment about clk_get_rate() for disabled clocks

[Uwe Kleine-König]

The sentence "[clk_get_rate()] is only valid once the clock source has
been enabled." can be understood in two ways:

a) When called for a disabled clock the return value might be wrong; or
b) The disabled clock must be enabled before it runs at the returned
rate.

It's hard to find evidence what is actually meant, but given that the
clock tree can change between the call to clk_get_rate() and the return
of a later clk_enable() call, it's prudent to assume a).

Adapt the comment accordingly to be unambiguous.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

while archiving my old mail I stumbled over
https://lore.kernel.org/linux-clk/20210213165406.GQ1463@shell.armlinux.org.uk
which supports semantic a).

Clearify the documentation accordingly.

Best regards
Uwe

 include/linux/clk.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1ef013324237..72f90d4df433 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -676,8 +676,10 @@ void clk_bulk_disable(int num_clks, const struct clk_bulk_data *clks);
 
 /**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
- *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
+ *
+ * Note that the return value for disabled clks is unreliable. It might or
+ * might not match the actual rate of the clock once it's enabled.
  */
 unsigned long clk_get_rate(struct clk *clk);
 

base-commit: 58706f7fb045b7019bada81fa17f372189315fe5
-- 
2.39.0

Re: [PATCH] clk: Disambiguate comment about clk_get_rate() for disabled clocks

[Russell King (Oracle)]

On Wed, Feb 01, 2023 at 09:23:09AM +0100, Uwe Kleine-König wrote:
> The sentence "[clk_get_rate()] is only valid once the clock source has
> been enabled." can be understood in two ways:
> 
> a) When called for a disabled clock the return value might be wrong; or
> b) The disabled clock must be enabled before it runs at the returned
> rate.
> 
> It's hard to find evidence what is actually meant, but given that the
> clock tree can change between the call to clk_get_rate() and the return
> of a later clk_enable() call, it's prudent to assume a).
> 
> Adapt the comment accordingly to be unambiguous.

Sorry for the late reply, I've been suffering with Covid for the last
nine days.

From the API perspective, it's both. Essentially, if the clock isn't
enabled, then the return value is completely undefined by the API and
no one should trust it.

It's one of the reasons why:

	clk_set_rate(clk, r);
	v = clk_get_rate(clk);

should not be used when what is actually required is:

	v = clk_round_rate(clk, r);

	...

	clk_set_rate(clk, r);

Note: r to clk_set_rate() not v.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] clk: Disambiguate comment about clk_get_rate() for disabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

Hello Russell,

On Thu, Feb 09, 2023 at 03:57:15PM +0000, Russell King (Oracle) wrote:
> On Wed, Feb 01, 2023 at 09:23:09AM +0100, Uwe Kleine-König wrote:
> > The sentence "[clk_get_rate()] is only valid once the clock source has
> > been enabled." can be understood in two ways:
> > 
> > a) When called for a disabled clock the return value might be wrong; or
> > b) The disabled clock must be enabled before it runs at the returned
> > rate.
> > 
> > It's hard to find evidence what is actually meant, but given that the
> > clock tree can change between the call to clk_get_rate() and the return
> > of a later clk_enable() call, it's prudent to assume a).
> > 
> > Adapt the comment accordingly to be unambiguous.
> 
> From the API perspective, it's both.

I documented a) now which is weaker than b), so b) is (only somewhat?)
implied.

I wonder if your mail is essentialy an Ack, or if you are unhappy with
my change. Can you suggest an alternative wording?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: Disambiguate comment about clk_get_rate() for disabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

Hello Russell,

On Tue, Feb 14, 2023 at 10:06:52AM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 09, 2023 at 03:57:15PM +0000, Russell King (Oracle) wrote:
> > On Wed, Feb 01, 2023 at 09:23:09AM +0100, Uwe Kleine-König wrote:
> > > The sentence "[clk_get_rate()] is only valid once the clock source has
> > > been enabled." can be understood in two ways:
> > > 
> > > a) When called for a disabled clock the return value might be wrong; or
> > > b) The disabled clock must be enabled before it runs at the returned
> > > rate.
> > > 
> > > It's hard to find evidence what is actually meant, but given that the
> > > clock tree can change between the call to clk_get_rate() and the return
> > > of a later clk_enable() call, it's prudent to assume a).
> > > 
> > > Adapt the comment accordingly to be unambiguous.
> > 
> > From the API perspective, it's both.
> 
> I documented a) now which is weaker than b), so b) is (only somewhat?)
> implied.
> 
> I wonder if your mail is essentialy an Ack, or if you are unhappy with
> my change. Can you suggest an alternative wording?

Would you pleas care to share your thoughts about this patch? I think
the patch is fine, but I didn't understood your reply if you're in
favour of it or you'd prefer a different wording.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]