Thread (68 messages) 68 messages, 7 authors, 2026-02-12

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

From: Daniel Almeida <daniel.almeida@collabora.com>
Date: 2026-02-03 13:55:50
Also in: dri-devel, linux-clk, linux-pm, linux-pwm, linux-riscv, lkml

On 3 Feb 2026, at 10:42, Gary Guo [off-list ref] wrote:

On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:
quoted
Hi Boris,
quoted
On 3 Feb 2026, at 07:39, Boris Brezillon [off-list ref] wrote:

On Mon, 19 Jan 2026 12:35:21 +0000
Alice Ryhl [off-list ref] wrote:
quoted
On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
quoted
On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:  
quoted
quoted
For example, it's quite typical to have (at least) one clock for the bus
interface that drives the register, and one that drives the main
component logic. The former needs to be enabled only when you're
accessing the registers (and can be abstracted with
regmap_mmio_attach_clk for example), and the latter needs to be enabled
only when the device actually starts operating.

You have a similar thing for the prepare vs enable thing. The difference
between the two is that enable can be called into atomic context but
prepare can't.

So for drivers that would care about this, you would create your device
with an unprepared clock, and then at various times during the driver
lifetime, you would mutate that state.  
The case where you're doing it only while accessing registers is
interesting, because that means the Enable bit may be owned by a local
variable. We may imagine an:

  let enabled = self.prepared_clk.enable_scoped();
  ... use registers
  drop(enabled);

Now ... this doesn't quite work with the current API - the current
Enabled stated owns both a prepare and enable count, but the above keeps
the prepare count in `self` and the enabled count in a local variable.
But it could be done with a fourth state, or by a closure method:

  self.prepared_clk.with_enabled(|| {
      ... use registers
  });

All of this would work with an immutable variable of type Clk<Prepared>.
Hm, maybe it'd make sense to implement Clone so we can have a temporary
clk variable that has its own prepare/enable refs and releases them
as it goes out of scope. This implies wrapping *mut bindings::clk in an
Arc<> because bindings::clk is not ARef, but should be relatively easy
to do. Posting the quick experiment I did with this approach, in case
you're interested [1]

[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
The problem with what you have suggested is that the previous state is not
consumed if you can clone it, and consuming the previous state is a pretty key
element in ensuring you cannot misuse it. For example, here:

let enabled_clk = prepared_clk.clone().enable()?;
// do stuff
// enabled_clk goes out of scope and releases the enable
// ref it had

prepared_clk is still alive. Now, this may not be the end of the world in this
particular case, but for API consistency, I'd say we should probably avoid this
behavior.
Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
is not enabled, (and similarly for `Prepared`), and that should be sufficient.
It is not an issue. However, I just find it a bit confusing. With a typestate, one
usually expects state transitions where a new state fully consumes the previous
one, and that assumption is “broken” in a way when you add clone().
Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
as you can have another user do `Clk<Unprepared>::get().enable()`.
Although you’re right here, I find this less confusing than clone(). You
have to explicitly craft a new Clk<Enabled>, where a clone() is a shorter way
to basically get around the “state transition” idea on an _existing_ Clk
reference.

This is a bit pedantic on my side though, so I have no problem in adding
clone() if it's the consensus of the majority.
The only guarantee is that the state the clk have is going to be greater than or
equal to the type state, so allowing cloning an intermediate state is no
problem.

Best,
Gary
quoted
I see that Alice suggested a closure approach. IMHO, we should use that
instead.

— Daniel

Is there any pushback on the closure approach? If so, mind sharing why?

— Daniel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help