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

Maxime Ripard
2026-01-08T08:07:05Z

[-- Attachment #1: Type: text/plain, Size: 4040 bytes --]

Hi Daniel,

On Wed, Jan 07, 2026 at 12:09:52PM -0300, Daniel Almeida wrote:
> The current Clk abstraction can still be improved on the following issues:
>
> a) It only keeps track of a count to clk_get(), which means that users have
> to manually call disable() and unprepare(), or a variation of those, like
> disable_unprepare().
>
> b) It allows repeated calls to prepare() or enable(), but it keeps no track
> of how often these were called, i.e., it's currently legal to write the
> following:
>
> clk.prepare();
> clk.prepare();
> clk.enable();
> clk.enable();
>
> And nothing gets undone on drop().
>
> c) It adds a OptionalClk type that is probably not needed. There is no
> "struct optional_clk" in C and we should probably not add one.
>
> d) It does not let a user express the state of the clk through the
> type system. For example, there is currently no way to encode that a Clk is
> enabled via the type system alone.
>
> In light of the Regulator abstraction that was recently merged, switch this
> abstraction to use the type-state pattern instead. It solves both a) and b)
> by establishing a number of states and the valid ways to transition between
> them. It also automatically undoes any call to clk_get(), clk_prepare() and
> clk_enable() as applicable on drop(), so users do not have to do anything
> special before Clk goes out of scope.
>
> It solves c) by removing the OptionalClk type, which is now simply encoded
> as a Clk whose inner pointer is NULL.
>
> It solves d) by directly encoding the state of the Clk into the type, e.g.:
> Clk<Enabled> is now known to be a Clk that is enabled.
>
> The INVARIANTS section for Clk is expanded to highlight the relationship
> between the states and the respective reference counts that are owned by
> each of them.
>
> The examples are expanded to highlight how a user can transition between
> states, as well as highlight some of the shortcuts built into the API.
>
> The current implementation is also more flexible, in the sense that it
> allows for more states to be added in the future. This lets us implement
> different strategies for handling clocks, including one that mimics the
> current API, allowing for multiple calls to prepare() and enable().
>
> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not
> a separate one) to reflect the new changes. This is needed, because
> otherwise this patch would break the build.
>
> Link: https://crates.io/crates/sealed [1]
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

I don't know the typestate pattern that well, but I wonder if we don't
paint ourselves into a corner by introducing it.

While it's pretty common to get your clock from the get go into a state,
and then don't modify it (like what devm_clk_get_enabled provides for
example), and the typestate pattern indeed works great for those, we
also have a significant number of drivers that will have a finer-grained
control over the clock enablement for PM.

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.

AFAIU, encoding the state of the clock into the Clk type (and thus
forcing the structure that holds it) prevents that mutation. If not, we
should make it clearer (by expanding the doc maybe?) how such a pattern
can be supported.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]