Re: [PATCH v3 1/3] rust: clk: use the type-state pattern
From: Daniel Almeida <daniel.almeida@collabora.com>
Date: 2026-02-03 13:36:11
Also in:
dri-devel, linux-clk, linux-pm, linux-pwm, linux-riscv, lkml
Hi Boris,
On 3 Feb 2026, at 06:17, Boris Brezillon [off-list ref] wrote: Hello Daniel, On Wed, 07 Jan 2026 12:09:52 -0300 Daniel Almeida [off-list ref] wrote:quoted
- /// Disable and unprepare the clock. + impl Clk<Enabled> { + /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id + /// and then prepares and enables it. /// - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`]. + /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`], + /// followed by [`Clk::enable`]. #[inline] - pub fn disable_unprepare(&self) { - // SAFETY: By the type invariants, self.as_raw() is a valid argument for - // [`clk_disable_unprepare`]. - unsafe { bindings::clk_disable_unprepare(self.as_raw()) }; + pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> { + Clk::<Prepared>::get(dev, name)? + .enable() + .map_err(|error| error.error) + } + + /// Behaves the same as [`Self::get`], except when there is no clock + /// producer. In this case, instead of returning [`ENOENT`], it returns + /// a dummy [`Clk`]. + #[inline] + pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> { + Clk::<Prepared>::get_optional(dev, name)? + .enable() + .map_err(|error| error.error) + } + + /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`] + /// state. + #[inline] + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> { + // We will be transferring the ownership of our `clk_get()` and + // `clk_enable()` counts to `Clk<Prepared>`. + let clk = ManuallyDrop::new(self); + + // SAFETY: By the type invariants, `self.0` is a valid argument for + // [`clk_disable`]. + unsafe { bindings::clk_disable(clk.as_raw()) }; + + Ok(Clk { + inner: clk.inner, + _phantom: PhantomData, + }) } /// Get clock's rate.Dunno if this has been mentioned already, but I belive the rate getter/setter should be in the generic implementation. Indeed, it's quite common for clock users to change the rate when the clk is disabled to avoid unstable transitional state. The usual pattern for that is: - clk_set_parent(my_clk, secondary_parent) - clk_disable[_unprepare](primary_parent) // (usually a PLL) - clk_set_rate(primary_parent) - clk[_prepare]_enable(primary_parent) - clk_set_parent(my_clk, primary_parent) The case where the clk rate is changed while the clk is active is also valid (usually fine when it's just a divider that's changed, because there's no stabilization period).quoted
@@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result { } }
I’m ok with this. I just assumed that these operations were only valid on enabled clks. Will change this in the next version. — Daniel