diff for duplicates of <20260618081037.35784-1-work@onurozkan.dev> diff --git a/a/1.txt b/N1/1.txt index aef65c1..ee3bb02 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,180 +1,185 @@ -On Thu, 18 Jun 2026 00:46:35 -0300\r -Daniel Almeida <daniel.almeida@collabora.com> wrote:\r -\r -> The current Clk abstraction can still be improved on the following issues:\r -> \r -> a) It only keeps track of a count to clk_get(), which means that users have\r -> to manually call disable() and unprepare(), or a variation of those, like\r -> disable_unprepare().\r -> \r -> b) It allows repeated calls to prepare() or enable(), but it keeps no track\r -> of how often these were called, i.e., it's currently legal to write the\r -> following:\r -> \r -> clk.prepare();\r -> clk.prepare();\r -> clk.enable();\r -> clk.enable();\r -> \r -> And nothing gets undone on drop().\r -> \r -> c) It adds a OptionalClk type that is probably not needed. There is no\r -> "struct optional_clk" in C and we should probably not add one.\r -> \r -> d) It does not let a user express the state of the clk through the\r -> type system. For example, there is currently no way to encode that a Clk is\r -> enabled via the type system alone.\r -> \r -> In light of the Regulator abstraction that was recently merged, switch this\r -> abstraction to use the type-state pattern instead. It solves both a) and b)\r -> by establishing a number of states and the valid ways to transition between\r -> them. It also automatically undoes any call to clk_get(), clk_prepare() and\r -> clk_enable() as applicable on drop(), so users do not have to do anything\r -> special before Clk goes out of scope.\r -> \r -> It solves c) by removing the OptionalClk type, which is now simply encoded\r -> as a Clk whose inner pointer is NULL.\r -> \r -> It solves d) by directly encoding the state of the Clk into the type, e.g.:\r -> Clk<Enabled> is now known to be a Clk that is enabled.\r -> \r -> The INVARIANTS section for Clk is expanded to highlight the relationship\r -> between the states and the respective reference counts that are owned by\r -> each of them.\r -> \r -> The examples are expanded to highlight how a user can transition between\r -> states, as well as highlight some of the shortcuts built into the API.\r -> \r -> The current implementation is also more flexible, in the sense that it\r -> allows for more states to be added in the future. This lets us implement\r -> different strategies for handling clocks, including one that mimics the\r -> current API, allowing for multiple calls to prepare() and enable().\r -> \r -> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not\r -> a separate one) to reflect the new changes. This is needed, because\r -> otherwise this patch would break the build.\r -> \r -> Link: https://crates.io/crates/sealed [1]\r -> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>\r -> ---\r -> drivers/cpufreq/rcpufreq_dt.rs | 2 +-\r -> drivers/gpu/drm/tyr/driver.rs | 31 +--\r -> drivers/pwm/pwm_th1520.rs | 17 +-\r -> rust/kernel/clk.rs | 512 ++++++++++++++++++++++++++++++-----------\r -> rust/kernel/cpufreq.rs | 8 +-\r -> 5 files changed, 396 insertions(+), 174 deletions(-)\r -> \r -> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs\r -> index f17bf64c22e2..9d2ec7df4bac 100644\r -> --- a/drivers/cpufreq/rcpufreq_dt.rs\r -> +++ b/drivers/cpufreq/rcpufreq_dt.rs\r -> @@ -40,7 +40,7 @@ struct CPUFreqDTDevice {\r -> freq_table: opp::FreqTable,\r -> _mask: CpumaskVar,\r -> _token: Option<opp::ConfigToken>,\r -> - _clk: Clk,\r -> + _clk: Clk<kernel::clk::Unprepared>,\r -> }\r -> \r -> #[derive(Default)]\r -> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs\r -> index 279710b36a10..a2230aebfea2 100644\r -> --- a/drivers/gpu/drm/tyr/driver.rs\r -> +++ b/drivers/gpu/drm/tyr/driver.rs\r -> @@ -3,7 +3,7 @@\r -> use kernel::{\r -> clk::{\r -> Clk,\r -> - OptionalClk, //\r -> + Enabled, //\r -> },\r -> device::{\r -> Bound,\r -> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {\r -\r -[...]\r -\r -> - /// Disable and unprepare the clock.\r -> - ///\r -> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].\r -> + /// Behaves the same as [`Self::get`], except when there is no clock\r -> + /// producer. In this case, instead of returning [`ENOENT`], it returns\r -> + /// a dummy [`Clk`].\r -> #[inline]\r -> - pub fn disable_unprepare(&self) {\r -> - // SAFETY: By the type invariants, self.as_raw() is a valid argument for\r -> - // [`clk_disable_unprepare`].\r -> - unsafe { bindings::clk_disable_unprepare(self.as_raw()) };\r -> + pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {\r -> + Clk::<Prepared>::get_optional(dev, name)?\r -> + .enable()\r -> + .map_err(|error| error.error)\r -> + }\r -> +\r -> + /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]\r -\r -nit: I wouldn't use the word "Attempts" for an infallible function.\r -\r -> + /// state.\r -> + #[inline]\r -> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {\r -\r -This is an infallible function, you can return Clk<Prepared> directly.\r -\r -Thanks,\r -Onur\r -\r -> + // We will be transferring the ownership of our `clk_get()` and\r -> + // `clk_enable()` counts to `Clk<Prepared>`.\r -> + let clk = ManuallyDrop::new(self);\r -> +\r -> + // SAFETY: By the type invariants, `self.0` is a valid argument for\r -> + // [`clk_disable`].\r -> + unsafe { bindings::clk_disable(clk.as_raw()) };\r -> +\r -> + Ok(Clk {\r -> + inner: clk.inner,\r -> + _phantom: PhantomData,\r -> + })\r -> }\r -> \r -> /// Get clock's rate.\r -> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {\r -> + // [`clk_unprepare`].\r -\r -[...]\r -\r -> + unsafe { bindings::clk_unprepare(self.as_raw()) };\r -> + }\r -> \r -> - fn deref(&self) -> &Clk {\r -> - &self.0\r -> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for\r -> + // [`clk_put`].\r -> + unsafe { bindings::clk_put(self.as_raw()) };\r -> }\r -> }\r -> }\r -> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs\r -> index d8d26870bea2..e837bb1010e0 100644\r -> --- a/rust/kernel/cpufreq.rs\r -> +++ b/rust/kernel/cpufreq.rs\r -> @@ -553,8 +553,12 @@ pub fn cpus(&mut self) -> &mut cpumask::Cpumask {\r -> /// The caller must guarantee that the returned [`Clk`] is not dropped while it is getting used\r -> /// by the C code.\r -> #[cfg(CONFIG_COMMON_CLK)]\r -> - pub unsafe fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<Clk> {\r -> - let clk = Clk::get(dev, name)?;\r -> + pub unsafe fn set_clk(\r -> + &mut self,\r -> + dev: &Device,\r -> + name: Option<&CStr>,\r -> + ) -> Result<Clk<crate::clk::Unprepared>> {\r -> + let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;\r -> self.as_mut_ref().clk = clk.as_raw();\r -> Ok(clk)\r -> }\r -> \r -> -- \r -> 2.54.0\r -> +On Thu, 18 Jun 2026 00:46:35 -0300 +Daniel Almeida <daniel.almeida@collabora.com> wrote: + +> The current Clk abstraction can still be improved on the following issues: +> +> a) It only keeps track of a count to clk_get(), which means that users have +> to manually call disable() and unprepare(), or a variation of those, like +> disable_unprepare(). +> +> b) It allows repeated calls to prepare() or enable(), but it keeps no track +> of how often these were called, i.e., it's currently legal to write the +> following: +> +> clk.prepare(); +> clk.prepare(); +> clk.enable(); +> clk.enable(); +> +> And nothing gets undone on drop(). +> +> c) It adds a OptionalClk type that is probably not needed. There is no +> "struct optional_clk" in C and we should probably not add one. +> +> d) It does not let a user express the state of the clk through the +> type system. For example, there is currently no way to encode that a Clk is +> enabled via the type system alone. +> +> In light of the Regulator abstraction that was recently merged, switch this +> abstraction to use the type-state pattern instead. It solves both a) and b) +> by establishing a number of states and the valid ways to transition between +> them. It also automatically undoes any call to clk_get(), clk_prepare() and +> clk_enable() as applicable on drop(), so users do not have to do anything +> special before Clk goes out of scope. +> +> It solves c) by removing the OptionalClk type, which is now simply encoded +> as a Clk whose inner pointer is NULL. +> +> It solves d) by directly encoding the state of the Clk into the type, e.g.: +> Clk<Enabled> is now known to be a Clk that is enabled. +> +> The INVARIANTS section for Clk is expanded to highlight the relationship +> between the states and the respective reference counts that are owned by +> each of them. +> +> The examples are expanded to highlight how a user can transition between +> states, as well as highlight some of the shortcuts built into the API. +> +> The current implementation is also more flexible, in the sense that it +> allows for more states to be added in the future. This lets us implement +> different strategies for handling clocks, including one that mimics the +> current API, allowing for multiple calls to prepare() and enable(). +> +> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not +> a separate one) to reflect the new changes. This is needed, because +> otherwise this patch would break the build. +> +> Link: https://crates.io/crates/sealed [1] +> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> +> --- +> drivers/cpufreq/rcpufreq_dt.rs | 2 +- +> drivers/gpu/drm/tyr/driver.rs | 31 +-- +> drivers/pwm/pwm_th1520.rs | 17 +- +> rust/kernel/clk.rs | 512 ++++++++++++++++++++++++++++++----------- +> rust/kernel/cpufreq.rs | 8 +- +> 5 files changed, 396 insertions(+), 174 deletions(-) +> +> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs +> index f17bf64c22e2..9d2ec7df4bac 100644 +> --- a/drivers/cpufreq/rcpufreq_dt.rs +> +++ b/drivers/cpufreq/rcpufreq_dt.rs +> @@ -40,7 +40,7 @@ struct CPUFreqDTDevice { +> freq_table: opp::FreqTable, +> _mask: CpumaskVar, +> _token: Option<opp::ConfigToken>, +> - _clk: Clk, +> + _clk: Clk<kernel::clk::Unprepared>, +> } +> +> #[derive(Default)] +> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs +> index 279710b36a10..a2230aebfea2 100644 +> --- a/drivers/gpu/drm/tyr/driver.rs +> +++ b/drivers/gpu/drm/tyr/driver.rs +> @@ -3,7 +3,7 @@ +> use kernel::{ +> clk::{ +> Clk, +> - OptionalClk, // +> + Enabled, // +> }, +> device::{ +> Bound, +> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData { + +[...] + +> - /// Disable and unprepare the clock. +> - /// +> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`]. +> + /// 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 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_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`] + +nit: I wouldn't use the word "Attempts" for an infallible function. + +> + /// state. +> + #[inline] +> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> { + +This is an infallible function, you can return Clk<Prepared> directly. + +Thanks, +Onur + +> + // 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. +> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result { +> + // [`clk_unprepare`]. + +[...] + +> + unsafe { bindings::clk_unprepare(self.as_raw()) }; +> + } +> +> - fn deref(&self) -> &Clk { +> - &self.0 +> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for +> + // [`clk_put`]. +> + unsafe { bindings::clk_put(self.as_raw()) }; +> } +> } +> } +> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs +> index d8d26870bea2..e837bb1010e0 100644 +> --- a/rust/kernel/cpufreq.rs +> +++ b/rust/kernel/cpufreq.rs +> @@ -553,8 +553,12 @@ pub fn cpus(&mut self) -> &mut cpumask::Cpumask { +> /// The caller must guarantee that the returned [`Clk`] is not dropped while it is getting used +> /// by the C code. +> #[cfg(CONFIG_COMMON_CLK)] +> - pub unsafe fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<Clk> { +> - let clk = Clk::get(dev, name)?; +> + pub unsafe fn set_clk( +> + &mut self, +> + dev: &Device, +> + name: Option<&CStr>, +> + ) -> Result<Clk<crate::clk::Unprepared>> { +> + let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?; +> self.as_mut_ref().clk = clk.as_raw(); +> Ok(clk) +> } +> +> -- +> 2.54.0 +> + +_______________________________________________ +linux-riscv mailing list +linux-riscv@lists.infradead.org +http://lists.infradead.org/mailman/listinfo/linux-riscv diff --git a/a/content_digest b/N1/content_digest index 5314376..5a3edc0 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -38,185 +38,190 @@ " Boris Brezillon <boris.brezillon@collabora.com>\0" "\00:1\0" "b\0" - "On Thu, 18 Jun 2026 00:46:35 -0300\r\n" - "Daniel Almeida <daniel.almeida@collabora.com> wrote:\r\n" - "\r\n" - "> The current Clk abstraction can still be improved on the following issues:\r\n" - "> \r\n" - "> a) It only keeps track of a count to clk_get(), which means that users have\r\n" - "> to manually call disable() and unprepare(), or a variation of those, like\r\n" - "> disable_unprepare().\r\n" - "> \r\n" - "> b) It allows repeated calls to prepare() or enable(), but it keeps no track\r\n" - "> of how often these were called, i.e., it's currently legal to write the\r\n" - "> following:\r\n" - "> \r\n" - "> clk.prepare();\r\n" - "> clk.prepare();\r\n" - "> clk.enable();\r\n" - "> clk.enable();\r\n" - "> \r\n" - "> And nothing gets undone on drop().\r\n" - "> \r\n" - "> c) It adds a OptionalClk type that is probably not needed. There is no\r\n" - "> \"struct optional_clk\" in C and we should probably not add one.\r\n" - "> \r\n" - "> d) It does not let a user express the state of the clk through the\r\n" - "> type system. For example, there is currently no way to encode that a Clk is\r\n" - "> enabled via the type system alone.\r\n" - "> \r\n" - "> In light of the Regulator abstraction that was recently merged, switch this\r\n" - "> abstraction to use the type-state pattern instead. It solves both a) and b)\r\n" - "> by establishing a number of states and the valid ways to transition between\r\n" - "> them. It also automatically undoes any call to clk_get(), clk_prepare() and\r\n" - "> clk_enable() as applicable on drop(), so users do not have to do anything\r\n" - "> special before Clk goes out of scope.\r\n" - "> \r\n" - "> It solves c) by removing the OptionalClk type, which is now simply encoded\r\n" - "> as a Clk whose inner pointer is NULL.\r\n" - "> \r\n" - "> It solves d) by directly encoding the state of the Clk into the type, e.g.:\r\n" - "> Clk<Enabled> is now known to be a Clk that is enabled.\r\n" - "> \r\n" - "> The INVARIANTS section for Clk is expanded to highlight the relationship\r\n" - "> between the states and the respective reference counts that are owned by\r\n" - "> each of them.\r\n" - "> \r\n" - "> The examples are expanded to highlight how a user can transition between\r\n" - "> states, as well as highlight some of the shortcuts built into the API.\r\n" - "> \r\n" - "> The current implementation is also more flexible, in the sense that it\r\n" - "> allows for more states to be added in the future. This lets us implement\r\n" - "> different strategies for handling clocks, including one that mimics the\r\n" - "> current API, allowing for multiple calls to prepare() and enable().\r\n" - "> \r\n" - "> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not\r\n" - "> a separate one) to reflect the new changes. This is needed, because\r\n" - "> otherwise this patch would break the build.\r\n" - "> \r\n" - "> Link: https://crates.io/crates/sealed [1]\r\n" - "> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>\r\n" - "> ---\r\n" - "> drivers/cpufreq/rcpufreq_dt.rs | 2 +-\r\n" - "> drivers/gpu/drm/tyr/driver.rs | 31 +--\r\n" - "> drivers/pwm/pwm_th1520.rs | 17 +-\r\n" - "> rust/kernel/clk.rs | 512 ++++++++++++++++++++++++++++++-----------\r\n" - "> rust/kernel/cpufreq.rs | 8 +-\r\n" - "> 5 files changed, 396 insertions(+), 174 deletions(-)\r\n" - "> \r\n" - "> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs\r\n" - "> index f17bf64c22e2..9d2ec7df4bac 100644\r\n" - "> --- a/drivers/cpufreq/rcpufreq_dt.rs\r\n" - "> +++ b/drivers/cpufreq/rcpufreq_dt.rs\r\n" - "> @@ -40,7 +40,7 @@ struct CPUFreqDTDevice {\r\n" - "> freq_table: opp::FreqTable,\r\n" - "> _mask: CpumaskVar,\r\n" - "> _token: Option<opp::ConfigToken>,\r\n" - "> - _clk: Clk,\r\n" - "> + _clk: Clk<kernel::clk::Unprepared>,\r\n" - "> }\r\n" - "> \r\n" - "> #[derive(Default)]\r\n" - "> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs\r\n" - "> index 279710b36a10..a2230aebfea2 100644\r\n" - "> --- a/drivers/gpu/drm/tyr/driver.rs\r\n" - "> +++ b/drivers/gpu/drm/tyr/driver.rs\r\n" - "> @@ -3,7 +3,7 @@\r\n" - "> use kernel::{\r\n" - "> clk::{\r\n" - "> Clk,\r\n" - "> - OptionalClk, //\r\n" - "> + Enabled, //\r\n" - "> },\r\n" - "> device::{\r\n" - "> Bound,\r\n" - "> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {\r\n" - "\r\n" - "[...]\r\n" - "\r\n" - "> - /// Disable and unprepare the clock.\r\n" - "> - ///\r\n" - "> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].\r\n" - "> + /// Behaves the same as [`Self::get`], except when there is no clock\r\n" - "> + /// producer. In this case, instead of returning [`ENOENT`], it returns\r\n" - "> + /// a dummy [`Clk`].\r\n" - "> #[inline]\r\n" - "> - pub fn disable_unprepare(&self) {\r\n" - "> - // SAFETY: By the type invariants, self.as_raw() is a valid argument for\r\n" - "> - // [`clk_disable_unprepare`].\r\n" - "> - unsafe { bindings::clk_disable_unprepare(self.as_raw()) };\r\n" - "> + pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {\r\n" - "> + Clk::<Prepared>::get_optional(dev, name)?\r\n" - "> + .enable()\r\n" - "> + .map_err(|error| error.error)\r\n" - "> + }\r\n" - "> +\r\n" - "> + /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]\r\n" - "\r\n" - "nit: I wouldn't use the word \"Attempts\" for an infallible function.\r\n" - "\r\n" - "> + /// state.\r\n" - "> + #[inline]\r\n" - "> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {\r\n" - "\r\n" - "This is an infallible function, you can return Clk<Prepared> directly.\r\n" - "\r\n" - "Thanks,\r\n" - "Onur\r\n" - "\r\n" - "> + // We will be transferring the ownership of our `clk_get()` and\r\n" - "> + // `clk_enable()` counts to `Clk<Prepared>`.\r\n" - "> + let clk = ManuallyDrop::new(self);\r\n" - "> +\r\n" - "> + // SAFETY: By the type invariants, `self.0` is a valid argument for\r\n" - "> + // [`clk_disable`].\r\n" - "> + unsafe { bindings::clk_disable(clk.as_raw()) };\r\n" - "> +\r\n" - "> + Ok(Clk {\r\n" - "> + inner: clk.inner,\r\n" - "> + _phantom: PhantomData,\r\n" - "> + })\r\n" - "> }\r\n" - "> \r\n" - "> /// Get clock's rate.\r\n" - "> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {\r\n" - "> + // [`clk_unprepare`].\r\n" - "\r\n" - "[...]\r\n" - "\r\n" - "> + unsafe { bindings::clk_unprepare(self.as_raw()) };\r\n" - "> + }\r\n" - "> \r\n" - "> - fn deref(&self) -> &Clk {\r\n" - "> - &self.0\r\n" - "> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for\r\n" - "> + // [`clk_put`].\r\n" - "> + unsafe { bindings::clk_put(self.as_raw()) };\r\n" - "> }\r\n" - "> }\r\n" - "> }\r\n" - "> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs\r\n" - "> index d8d26870bea2..e837bb1010e0 100644\r\n" - "> --- a/rust/kernel/cpufreq.rs\r\n" - "> +++ b/rust/kernel/cpufreq.rs\r\n" - "> @@ -553,8 +553,12 @@ pub fn cpus(&mut self) -> &mut cpumask::Cpumask {\r\n" - "> /// The caller must guarantee that the returned [`Clk`] is not dropped while it is getting used\r\n" - "> /// by the C code.\r\n" - "> #[cfg(CONFIG_COMMON_CLK)]\r\n" - "> - pub unsafe fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<Clk> {\r\n" - "> - let clk = Clk::get(dev, name)?;\r\n" - "> + pub unsafe fn set_clk(\r\n" - "> + &mut self,\r\n" - "> + dev: &Device,\r\n" - "> + name: Option<&CStr>,\r\n" - "> + ) -> Result<Clk<crate::clk::Unprepared>> {\r\n" - "> + let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;\r\n" - "> self.as_mut_ref().clk = clk.as_raw();\r\n" - "> Ok(clk)\r\n" - "> }\r\n" - "> \r\n" - "> -- \r\n" - "> 2.54.0\r\n" - > + "On Thu, 18 Jun 2026 00:46:35 -0300\n" + "Daniel Almeida <daniel.almeida@collabora.com> wrote:\n" + "\n" + "> The current Clk abstraction can still be improved on the following issues:\n" + "> \n" + "> a) It only keeps track of a count to clk_get(), which means that users have\n" + "> to manually call disable() and unprepare(), or a variation of those, like\n" + "> disable_unprepare().\n" + "> \n" + "> b) It allows repeated calls to prepare() or enable(), but it keeps no track\n" + "> of how often these were called, i.e., it's currently legal to write the\n" + "> following:\n" + "> \n" + "> clk.prepare();\n" + "> clk.prepare();\n" + "> clk.enable();\n" + "> clk.enable();\n" + "> \n" + "> And nothing gets undone on drop().\n" + "> \n" + "> c) It adds a OptionalClk type that is probably not needed. There is no\n" + "> \"struct optional_clk\" in C and we should probably not add one.\n" + "> \n" + "> d) It does not let a user express the state of the clk through the\n" + "> type system. For example, there is currently no way to encode that a Clk is\n" + "> enabled via the type system alone.\n" + "> \n" + "> In light of the Regulator abstraction that was recently merged, switch this\n" + "> abstraction to use the type-state pattern instead. It solves both a) and b)\n" + "> by establishing a number of states and the valid ways to transition between\n" + "> them. It also automatically undoes any call to clk_get(), clk_prepare() and\n" + "> clk_enable() as applicable on drop(), so users do not have to do anything\n" + "> special before Clk goes out of scope.\n" + "> \n" + "> It solves c) by removing the OptionalClk type, which is now simply encoded\n" + "> as a Clk whose inner pointer is NULL.\n" + "> \n" + "> It solves d) by directly encoding the state of the Clk into the type, e.g.:\n" + "> Clk<Enabled> is now known to be a Clk that is enabled.\n" + "> \n" + "> The INVARIANTS section for Clk is expanded to highlight the relationship\n" + "> between the states and the respective reference counts that are owned by\n" + "> each of them.\n" + "> \n" + "> The examples are expanded to highlight how a user can transition between\n" + "> states, as well as highlight some of the shortcuts built into the API.\n" + "> \n" + "> The current implementation is also more flexible, in the sense that it\n" + "> allows for more states to be added in the future. This lets us implement\n" + "> different strategies for handling clocks, including one that mimics the\n" + "> current API, allowing for multiple calls to prepare() and enable().\n" + "> \n" + "> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not\n" + "> a separate one) to reflect the new changes. This is needed, because\n" + "> otherwise this patch would break the build.\n" + "> \n" + "> Link: https://crates.io/crates/sealed [1]\n" + "> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>\n" + "> ---\n" + "> drivers/cpufreq/rcpufreq_dt.rs | 2 +-\n" + "> drivers/gpu/drm/tyr/driver.rs | 31 +--\n" + "> drivers/pwm/pwm_th1520.rs | 17 +-\n" + "> rust/kernel/clk.rs | 512 ++++++++++++++++++++++++++++++-----------\n" + "> rust/kernel/cpufreq.rs | 8 +-\n" + "> 5 files changed, 396 insertions(+), 174 deletions(-)\n" + "> \n" + "> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs\n" + "> index f17bf64c22e2..9d2ec7df4bac 100644\n" + "> --- a/drivers/cpufreq/rcpufreq_dt.rs\n" + "> +++ b/drivers/cpufreq/rcpufreq_dt.rs\n" + "> @@ -40,7 +40,7 @@ struct CPUFreqDTDevice {\n" + "> freq_table: opp::FreqTable,\n" + "> _mask: CpumaskVar,\n" + "> _token: Option<opp::ConfigToken>,\n" + "> - _clk: Clk,\n" + "> + _clk: Clk<kernel::clk::Unprepared>,\n" + "> }\n" + "> \n" + "> #[derive(Default)]\n" + "> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs\n" + "> index 279710b36a10..a2230aebfea2 100644\n" + "> --- a/drivers/gpu/drm/tyr/driver.rs\n" + "> +++ b/drivers/gpu/drm/tyr/driver.rs\n" + "> @@ -3,7 +3,7 @@\n" + "> use kernel::{\n" + "> clk::{\n" + "> Clk,\n" + "> - OptionalClk, //\n" + "> + Enabled, //\n" + "> },\n" + "> device::{\n" + "> Bound,\n" + "> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {\n" + "\n" + "[...]\n" + "\n" + "> - /// Disable and unprepare the clock.\n" + "> - ///\n" + "> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].\n" + "> + /// Behaves the same as [`Self::get`], except when there is no clock\n" + "> + /// producer. In this case, instead of returning [`ENOENT`], it returns\n" + "> + /// a dummy [`Clk`].\n" + "> #[inline]\n" + "> - pub fn disable_unprepare(&self) {\n" + "> - // SAFETY: By the type invariants, self.as_raw() is a valid argument for\n" + "> - // [`clk_disable_unprepare`].\n" + "> - unsafe { bindings::clk_disable_unprepare(self.as_raw()) };\n" + "> + pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {\n" + "> + Clk::<Prepared>::get_optional(dev, name)?\n" + "> + .enable()\n" + "> + .map_err(|error| error.error)\n" + "> + }\n" + "> +\n" + "> + /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]\n" + "\n" + "nit: I wouldn't use the word \"Attempts\" for an infallible function.\n" + "\n" + "> + /// state.\n" + "> + #[inline]\n" + "> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {\n" + "\n" + "This is an infallible function, you can return Clk<Prepared> directly.\n" + "\n" + "Thanks,\n" + "Onur\n" + "\n" + "> + // We will be transferring the ownership of our `clk_get()` and\n" + "> + // `clk_enable()` counts to `Clk<Prepared>`.\n" + "> + let clk = ManuallyDrop::new(self);\n" + "> +\n" + "> + // SAFETY: By the type invariants, `self.0` is a valid argument for\n" + "> + // [`clk_disable`].\n" + "> + unsafe { bindings::clk_disable(clk.as_raw()) };\n" + "> +\n" + "> + Ok(Clk {\n" + "> + inner: clk.inner,\n" + "> + _phantom: PhantomData,\n" + "> + })\n" + "> }\n" + "> \n" + "> /// Get clock's rate.\n" + "> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {\n" + "> + // [`clk_unprepare`].\n" + "\n" + "[...]\n" + "\n" + "> + unsafe { bindings::clk_unprepare(self.as_raw()) };\n" + "> + }\n" + "> \n" + "> - fn deref(&self) -> &Clk {\n" + "> - &self.0\n" + "> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for\n" + "> + // [`clk_put`].\n" + "> + unsafe { bindings::clk_put(self.as_raw()) };\n" + "> }\n" + "> }\n" + "> }\n" + "> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs\n" + "> index d8d26870bea2..e837bb1010e0 100644\n" + "> --- a/rust/kernel/cpufreq.rs\n" + "> +++ b/rust/kernel/cpufreq.rs\n" + "> @@ -553,8 +553,12 @@ pub fn cpus(&mut self) -> &mut cpumask::Cpumask {\n" + "> /// The caller must guarantee that the returned [`Clk`] is not dropped while it is getting used\n" + "> /// by the C code.\n" + "> #[cfg(CONFIG_COMMON_CLK)]\n" + "> - pub unsafe fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<Clk> {\n" + "> - let clk = Clk::get(dev, name)?;\n" + "> + pub unsafe fn set_clk(\n" + "> + &mut self,\n" + "> + dev: &Device,\n" + "> + name: Option<&CStr>,\n" + "> + ) -> Result<Clk<crate::clk::Unprepared>> {\n" + "> + let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;\n" + "> self.as_mut_ref().clk = clk.as_raw();\n" + "> Ok(clk)\n" + "> }\n" + "> \n" + "> -- \n" + "> 2.54.0\n" + "> \n" + "\n" + "_______________________________________________\n" + "linux-riscv mailing list\n" + "linux-riscv@lists.infradead.org\n" + http://lists.infradead.org/mailman/listinfo/linux-riscv -f0623169db7f41bfe8b787509d3cccdaf4f81f6277b662a760295d9e8e59e587 +29f0cafd23f88c679976b13cab08ce842a2efb4221f3af3a1909fa24a253e375
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.