Messages in this thread | | | Date | Sun, 07 Apr 2024 09:54:19 +0000 | From | Benno Lossin <> | Subject | Re: [RFC PATCH 1/3] rust: Add bindings for OPP framework |
| |
Hi,
I took a quick look and left some comments from the Rust side of view.
On 05.04.24 13:09, Viresh Kumar wrote: > +/// Equivalent to `struct dev_pm_opp_config` in the C Code. > +pub struct Config<T: ConfigOps> { > + token: Option<i32>, > + clk_names: Option<Pin<Vec<CString>>>,
Why are you using `Pin<Vec<_>>`? The vector may reallocate the backing storage at any point in time.
> + prop_name: Option<Pin<CString>>, > + regulator_names: Option<Pin<Vec<CString>>>, > + genpd_names: Option<Pin<Vec<CString>>>, > + supported_hw: Option<Pin<Vec<u32>>>, > + required_devs: Option<Pin<Vec<Device>>>, > + _data: PhantomData<T>, > +}
[...]
> + /// Sets the configuration with the OPP core. > + pub fn set(&mut self, dev: &Device) -> Result<()> { > + // Already configured. > + if self.token.is_some() {
Why does the config hold onto this token? Would it make sense to consume the config and return a `Handle` or `Token` abstraction? Then you don't need to check if the config has been "used" before.
> + return Err(EBUSY); > + } > + > + let (_clk_list, clk_names) = match &self.clk_names { > + Some(x) => { > + let list = to_c_str_array(x)?; > + let ptr = list.as_ptr(); > + (Some(list), ptr) > + } > + None => (None, ptr::null()), > + };
[...]
> +/// Operating performance point (OPP). > +/// > +/// # Invariants > +/// > +/// `ptr` is valid, non-null, and has a non-zero reference count. One of the references is owned by > +/// `self`, and will be decremented when `self` is dropped. > +#[repr(transparent)] > +pub struct OPP(*mut bindings::dev_pm_opp);
I think you should use the `ARef` pattern instead:
#[repr(transparent)] pub struct OPP(Opaque<bindings::dev_pm_opp>);
unsafe impl AlwaysRefCounted for OPP { // ... }
Then you can use `ARef<OPP>` everywhere you use `OPP` currently.
-- Cheers, Benno
| |