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

Daniel Almeida
2026-02-03T13:36:11Z

Hi Boris,

> On 3 Feb 2026, at 06:17, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> Hello Daniel,
>
> On Wed, 07 Jan 2026 12:09:52 -0300
> Daniel Almeida <daniel.almeida@collabora.com> 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