| Message ID | 20230201082309.233348-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: Disambiguate comment about clk_get_rate() for disabled clocks | expand |
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.
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
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
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);
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(-) base-commit: 58706f7fb045b7019bada81fa17f372189315fe5