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

["Gary Guo" <gary@garyguo.net>]

On Sun Aug 2, 2026 at 2:05 AM BST, Alexandre Courbot wrote:
> 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).

This seems to be exactly matching my issue with the current API design:
https://lore.kernel.org/rust-for-linux/DKEO7DXQ8604.1CB09QFM2EO68@garyguo.net/

i.e. the type state is used for two things.

Best,
Gary

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