Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

Boris Brezillon
2026-02-03T19:44:06Z

On Tue, 3 Feb 2026 16:26:22 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> >
> > I think it's fine to have all of these:
> > * `Clone` impl
> > * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> > * `with_enabled` that gives `&Clk<Enabled>`
> >
> > This way, if you only want to enable in short time, you can do `with_enabled`.
> > If the closure callback wants to keep clock enabled for longer, it can just do
> > `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> >
> > If the user just have a reference and want to enable the callback they can do
> > `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> >
> > Best,
> > Gary
>
>
> I’m ok with what you proposed above. The only problem is that implementing
> clone() is done through an Arc<*mut bindings::clk> in Boris’ current
> design,

It's actually Arc<RawClk> with

struct RawClk(*mut bindings::clk);

impl Drop for RawClk {
fn drop(&mut self) {
// SAFETY: By the type invariants, self.as_raw() is a valid argument for // [`clk_put`].
unsafe { bindings::clk_put(self.0) };
}
}

This is because struct clk is not refcounted, so cloning
implies wrapping this object in an Arc, and only calling
clk_put() when the Arc refcnt reaches zero.

> so this requires an extra allocation.

That's true. But the memory overhead should be pretty negligible,
and I don't think the extra indirection makes any noticeable
difference for an actual clk implementation (one that's not a NOP),
since we have indirections all over the place already (clk -> clk_hw,
clk_ops, ...). So I think I'd value ease of use over this small
perfs/mem-usage hit.