Message-Id: <14A9284F-A773-4F21-A5FC-9762AE5A5390@collabora.com>
Date: Mon, 19 Jan 2026 09:13:43 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Danilo Krummrich <dakr@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Drew Fustini <fustini@...nel.org>,
 Guo Ren <guoren@...nel.org>,
 Fu Wei <wefu@...hat.com>,
 Uwe Kleine-König <ukleinek@...nel.org>,
 Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>,
 Miguel Ojeda <ojeda@...nel.org>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Trevor Gross <tmgross@...ch.edu>,
 linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,
 linux-riscv@...ts.infradead.org,
 linux-pwm@...r.kernel.org,
 linux-clk@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/3] rust: clk: use the type-state pattern



> On 19 Jan 2026, at 07:45, Maxime Ripard <mripard@...nel.org> wrote:
> 
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> Hi Maxime :)
>> 
>>> 
>>> 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
>> 
>> Minor correction, devm_clk_get_enabled is not handled by the typestate
>> pattern. The next patch does include this function for convenience, but
>> you get a Result<()>. The typestate pattern is used when you want more
>> control.
>> 
>>> 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
>> 
>> IIUC, your main point seems to be about mutating the state at runtime? This is
>> possible with this code. You can just have an enum, for example:
>> 
>> enum MyClocks {
>> Unprepared(Clk<Unprepared>),
>>        Prepared(Clk<Prepared>),
>> Enabled(Clk<Enabled>), 
>> }
>> 
>> In fact, I specifically wanted to ensure that this was possible when writing
>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>> the examples, no worries.
> 
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?

The problem with keeping it the way it is that you’re back to manual
prepare/unprepare and enable/disable, as the type-state is what’s enforcing
the correct order of calls. This is also the case when the type is dropped.

In fact, one of the aims of this patch is to get rid of the current Clk type
before we have more users. The current series fixes this by enforcing a sane
order of operations. Most importantly, it enforces that the refcounts to get(),
enable() and etc are handled correctly using the type system.

It rids us of this problem, which is possible today:

clk.enable();
clk.prepare();
clk.prepare();
clk.disable_unprepare();
clk.set_rate();


> 
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.


But that's how it works in this series. The typestate pattern is opt-in. If you
need to "set and forget" there's the devm API that's introduced by the next
patch. I can expose more devm_* APIs if you want.

I'm not sure the boilerplate is significant, by the way. You can just do:


Clk::<Enabled>::get();


As a starting point, and have the enum thing (which is also simple) _if_ you
need to manually enable/disable at runtime. Most of the time, you will only
need to mention the type state once, like I did in the call above, and then
the type system will figure out the rest when transitions take place.

What boilerplate did you have in mind?

— Daniel

Powered by blists - more mailing lists