Re: [PATCH] rust: clk: use the type-state pattern

["Danilo Krummrich" <dakr@kernel.org>]

On Wed Jul 30, 2025 at 2:13 PM CEST, Daniel Almeida wrote:
>> On 30 Jul 2025, at 05:03, Danilo Krummrich <dakr@kernel.org> wrote:
>> On Tue Jul 29, 2025 at 11:38 PM CEST, Daniel Almeida wrote:
>>>     /// fn configure_clk(dev: &Device) -> Result {
>>> -    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
>>> +    ///     // The fastest way is to use a version of `Clk::get` for the desired
>>> +    ///     // state, i.e.:
>>> +    ///     let clk: Clk<Enabled> = Clk::<Enabled>::get(dev, Some(c_str!("apb_clk")))?;
>> 
>> Given that this is a driver API, why do we allow obtaining and configuring
>> clocks of any device, i.e. also unbound devices?
>> 
>> I think Clk::<T>::get() should take a &Device<Bound> instead.
>
> Ah, this was a question I had, but that I forgot to mention here.
>
> The same can probably be said of the regulator series? i.e.:
>
> impl Regulator<Disabled> {
>     /// Obtains a [`Regulator`] instance from the system.
>     pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
>         Regulator::get_internal(dev, name)
>     }

Yes, that's a device resource as well. We should only give it out to drivers
when they're actually bound to the device.

>> 
>>> -    ///     clk.prepare_enable()?;
>>> +    ///     // Any other state is also possible, e.g.:
>>> +    ///     let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c_str!("apb_clk")))?;
>>> +    ///
>>> +    ///     // Later:
>>> +    ///     let clk: Clk<Enabled> = clk.enable().map_err(|error| {
>>> +    ///         error.error
>>> +    ///     })?;
>>> +    ///
>>> +    ///     // Note that error.clk is the original `clk` if the operation
>>> +    ///     // failed. It is provided as a convenience so that the operation may be
>>> +    ///     // retried in case of errors.
>>>     ///
>>>     ///     let expected_rate = Hertz::from_ghz(1);
>>>     ///
>>> @@ -120,104 +200,172 @@ mod common_clk {
>>>     ///         clk.set_rate(expected_rate)?;
>>>     ///     }
>>>     ///
>>> -    ///     clk.disable_unprepare();
>>> +    ///     // Nothing is needed here. The drop implementation will undo any
>>> +    ///     // operations as appropriate.
>>> +    ///     Ok(())
>>> +    /// }
>>> +    ///
>>> +    /// fn shutdown(dev: &Device, clk: Clk<Enabled>) -> Result {
>> 
>> You don't need the dev argument here.
>> 
>>> +    ///     // The states can be traversed "in the reverse order" as well:
>>> +    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
>>> +    ///         error.error
>>> +    ///     })?;
>>> +    ///
>>> +    ///     let clk: Clk<Unprepared> = clk.unprepare();
>> 
>> I know you want to showcase the type state, yet I don't know if we should
>> explicitly declare the type if not necessary. People will likely just copy
>> things. Maybe a comment is better to emphasize it?
>
> Ok
>
>> 
>>> +    ///
>>>     ///     Ok(())
>>>     /// }
>>>     /// ```
>>>     ///
>>>     /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
>>>     #[repr(transparent)]
>>> -    pub struct Clk(*mut bindings::clk);
>>> +    pub struct Clk<T: ClkState> {
>>> +        inner: *mut bindings::clk,
>>> +        _phantom: core::marker::PhantomData<T>,
>>> +    }
>> 
>> <snip>
>> 
>>> +    impl<T: ClkState> Drop for Clk<T> {
>>> +        fn drop(&mut self) {
>>> +            if T::DISABLE_ON_DROP {
>>> +                // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>>> +                // [`clk_disable`].
>>> +                unsafe { bindings::clk_disable(self.as_raw()) };
>>> +            }
>>> +
>>> +            if T::UNPREPARE_ON_DROP {
>>> +                // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>>> +                // [`clk_unprepare`].
>>> +                unsafe { bindings::clk_unprepare(self.as_raw()) };
>>> +            }
>> 
>> Nice! I like this cleanup. However, don't you still need to call clk_put() to
>> drop the reference count?
>
> Right, clk_put() was totally forgotten.
>
>> 
>> Also, given that this is a device resource, don't we want to take it away from
>> drivers once the corresponding device has been unbound, i.e. use Devres?
>
> Do you mean to have the get() functions return Devres<Clk>?
>
> Also, is this applicable for Regulator as well?

Yes, drivers shouldn't be able to mess with device specific resources once they
are unbound from the device.