Re: [PATCH v5 2/4] rust: clk: implement Clone for Clk<T>

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Jul 6, 2026 at 11:37 PM JST, Daniel Almeida wrote:
> The type-state pattern makes state transitions consume the Clk value,
> which means a single Clk cannot be shared between users that need to
> hold the clock in different states,

Sharing a clock can be done by acquiring it several times though - this
actually seems cleaner to me when concurrent setting of a given clock is
needed within the same driver.

> nor can a driver keep a long-lived
> Clk<Prepared> around while temporarily enabling it for scoped sections.

Something like an `EnabledGuard` for a `Clk<Prepared>` should be a good
fit for this? If you make it generic over `Deref<Target =
Clk<Prepared>>` you could even make it work with `&Clk<Prepared>` (for
short-lived enables) and `Arc<Clk<Prepared>>` (for enable periods longer
than a code block).

>
> Implement Clone for Clk<T>: each clone is an independent view of the
> same underlying clock, in the same state, owning its own
> clk_prepare()/clk_enable() counts, e.g.:
>
> 	let enabled_clk = prepared_clk.clone().enable()?;
>
> 	// Do stuff that requires the clock to be enabled.
>
> 	// enabled_clk goes out of scope and releases the counts it
> 	// owns; the clock remains prepared through prepared_clk.

Looks indeed like something an `EnabledGuard` would cover.

>
> Since struct clk is not refcounted on the C side, share it between
> clones by wrapping the pointer in an Arc'd RawClk, whose drop
> implementation calls clk_put() exactly once, when the last clone goes
> out of scope. This costs one small allocation per clk_get(), which was
> deemed negligible when compared against the pre-existing indirections
> in the clk framework.
>
> Cloning a prepared (or enabled) Clk calls clk_prepare() (and
> clk_enable()) on the underlying clock. These calls cannot fail here:
> the value being cloned already holds a count of each, so the C side
> only increments the respective counts. This is what makes an infallible
> Clone implementation possible.

Another point to consider is that `clk_prepare` (and thus `clone()`, as
you mentioned in the doccomment of the patch) can sleep. An
`EnabledGuard`, otoh, would only need to call `clk_enable` and thus
wouldn't sleep, making it usable in atomic context.

>
> The DISABLE_ON_DROP and UNPREPARE_ON_DROP constants are renamed to
> ENABLED and PREPARED respectively, since they now describe the state

Let's give them their final name in patch 1 then, since it's also
accurate (and arguably better) even before this patch.

I am not quite familiar with the client code that will make use of this;
a pointer would be welcome. But I strongly suspect that multiple calls
to `get` and an `EnabledGuard` can cover most (if not all) use-cases.
Introducing an `Arc` here forces a storage decision upon users while
hiding it.

It sets us up for some dilemmas in the future: for instance, Maurice's
series exposing `clk_rate_exclusive_get` turns a `Clk` into an
`ExclusiveClk`, but its potential clones are still around as regular
`Clk`s. Moreover, `ExclusiveClk` derefs to `Clk<Enabled>`, which can be
cloned as per this patch; so is it really exclusive?

So I'd like to keep discussing this design a bit more, with more
visibility over user code. Maybe it should be moved to the tail of the
series, or even extracted into its own series so as to not block the
other patches.