Hi Boris, > On 3 Feb 2026, at 06:17, Boris Brezillon <[email protected]> > wrote: > > Hello Daniel, > > On Wed, 07 Jan 2026 12:09:52 -0300 > Daniel Almeida <[email protected]> wrote: > >> - /// 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). > >> @@ -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
