Message-Id: <518D8B09-B9A1-4DB4-85CD-37A2DD3D5FB1@collabora.com>
Date: Mon, 19 Jan 2026 09:54:10 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Maxime Ripard <mripard@...nel.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Danilo Krummrich <dakr@...nel.org>,
 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 09:35, Alice Ryhl <aliceryhl@...gle.com> wrote:
> 
> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>>>> 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);


Not sure I understand. You can get a Clk<Enabled>, do what you need, and then
consume Clk<Enabled> to go back to Clk<Prepared>. I think I added this, but if
I didn’t, it’s a trivial thing to do.

> 
> 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>.
> 
>>>> 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>), 
>>> }
> 
> I believe you need an extra state if the state is not bound to the scope
> of a function:
> 
> enum MyClocks {
>    Unprepared(Clk<Unprepared>),
>    Prepared(Clk<Prepared>),
>    Enabled(Clk<Enabled>), 
>    Transitioning,
> }
> 
> since mem::replace() needs a new value before you can take ownership of
> the existing Clk value.

Right, I need to update the docs to account for this, as they imply that you
can do this with only two states.

> 
>>> 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?
>> 
>> That way, we do have the typestate API for whoever wants to, without
>> creating too much boilerplate for everybody else.
> 
> I think that if you still want an API where you just call enable/disable
> directly on it with no protection against unbalanced calls, then that
> should be the special API. Probably called RawClk and functions marked
> unsafe. Unbalanced calls seem really dangerous and use should not be
> encouraged.

I think we should discourage RawClk if at all possible. But if the consensus
is that we *really* need this easily-abused thing, I can provide a follow-up.

> 
> The current type-state based API is the least-boilerplate option for
> drivers that exist today.
> 
> Alice

Powered by blists - more mailing lists