* [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-27 15:10 ` Uwe Kleine-König
2025-06-23 18:08 ` [PATCH v5 2/9] rust: pwm: Add core 'Device' and 'Chip' object wrappers Michal Wilczynski
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Introduce the foundational support for PWM abstractions in Rust.
This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
the feature, along with the necessary build-system support and C
helpers.
It also introduces the first set of safe wrappers for the PWM
subsystem, covering the basic data carrying C structs and enums:
- `Polarity`: A safe wrapper for `enum pwm_polarity`.
- `Waveform`: A wrapper for `struct pwm_waveform`.
- `Args`: A wrapper for `struct pwm_args`.
- `State`: A wrapper for `struct pwm_state`.
These types provide memory safe, idiomatic Rust representations of the
core PWM data structures and form the building blocks for the
abstractions that will follow.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
MAINTAINERS | 6 ++
drivers/pwm/Kconfig | 13 +++
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/pwm.c | 20 ++++
rust/kernel/lib.rs | 2 +
rust/kernel/pwm.rs | 198 ++++++++++++++++++++++++++++++++++++++++
7 files changed, 241 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 0c1d245bf7b84f8a78b811e0c9c5a3edc09edc22..a575622454a2ef57ce055c8a8c4765fa4fddc490 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20073,6 +20073,12 @@ F: include/linux/pwm.h
F: include/linux/pwm_backlight.h
K: pwm_(config|apply_might_sleep|apply_atomic|ops)
+PWM SUBSYSTEM BINDINGS [RUST]
+M: Michal Wilczynski <m.wilczynski@samsung.com>
+S: Maintained
+F: rust/helpers/pwm.c
+F: rust/kernel/pwm.rs
+
PXA GPIO DRIVER
M: Robert Jarzmik <robert.jarzmik@free.fr>
L: linux-gpio@vger.kernel.org
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index d9bcd1e8413eaed1602d6686873e263767c58f5f..cfddeae0eab3523f04f361fb41ccd1345c0c937b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -790,4 +790,17 @@ config PWM_XILINX
To compile this driver as a module, choose M here: the module
will be called pwm-xilinx.
+ config RUST_PWM_ABSTRACTIONS
+ bool "Rust PWM abstractions support"
+ depends on RUST
+ depends on PWM=y
+ help
+ This option enables the safe Rust abstraction layer for the PWM
+ subsystem. It provides idiomatic wrappers and traits necessary for
+ writing PWM controller drivers in Rust.
+
+ The abstractions handle resource management (like memory and reference
+ counting) and provide safe interfaces to the underlying C core,
+ allowing driver logic to be written in safe Rust.
+
endif
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -64,6 +64,7 @@
#include <linux/pm_opp.h>
#include <linux/poll.h>
#include <linux/property.h>
+#include <linux/pwm.h>
#include <linux/refcount.h>
#include <linux/sched.h>
#include <linux/security.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 16fa9bca5949b85e8d4cdcfe8e6886124f72d8d8..60879e6d794ce0f87e39caafc5495bf5e8acf8f0 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -31,6 +31,7 @@
#include "platform.c"
#include "pci.c"
#include "pid_namespace.c"
+#include "pwm.c"
#include "rbtree.c"
#include "rcu.c"
#include "refcount.c"
diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
new file mode 100644
index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
--- /dev/null
+++ b/rust/helpers/pwm.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+#include <linux/pwm.h>
+
+struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
+{
+ return pwmchip_parent(chip);
+}
+
+void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
+{
+ return pwmchip_get_drvdata(chip);
+}
+
+void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
+{
+ pwmchip_set_drvdata(chip, data);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..ce1d08b14e456905dbe7b625bbb8ca8b08deae2a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -105,6 +105,8 @@
pub mod seq_file;
pub mod sizes;
mod static_assert;
+#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
+pub mod pwm;
#[doc(hidden)]
pub mod std_vendor;
pub mod str;
diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
new file mode 100644
index 0000000000000000000000000000000000000000..ed681b228c414e7ae8bf80ca649ad497c9dc4ec3
--- /dev/null
+++ b/rust/kernel/pwm.rs
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! PWM subsystem abstractions.
+//!
+//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
+
+use crate::{
+ bindings,
+ prelude::*,
+ types::Opaque,
+};
+use core::convert::TryFrom;
+
+/// Maximum size for the hardware-specific waveform representation buffer.
+///
+/// From C: `#define WFHWSIZE 20`
+pub const WFHW_MAX_SIZE: usize = 20;
+
+/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub enum Polarity {
+ /// Normal polarity (duty cycle defines the high period of the signal).
+ Normal,
+
+ /// Inversed polarity (duty cycle defines the low period of the signal).
+ Inversed,
+}
+
+impl TryFrom<bindings::pwm_polarity> for Polarity {
+ type Error = Error;
+
+ fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
+ match polarity {
+ bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
+ bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
+ _ => Err(EINVAL),
+ }
+ }
+}
+
+impl From<Polarity> for bindings::pwm_polarity {
+ fn from(polarity: Polarity) -> Self {
+ match polarity {
+ Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
+ Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
+ }
+ }
+}
+
+/// Represents a PWM waveform configuration.
+/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub struct Waveform {
+ /// Total duration of one complete PWM cycle, in nanoseconds.
+ pub period_length_ns: u64,
+
+ /// Duty-cycle active time, in nanoseconds.
+ ///
+ /// For a typical normal polarity configuration (active-high) this is the
+ /// high time of the signal.
+ pub duty_length_ns: u64,
+
+ /// Duty-cycle start offset, in nanoseconds.
+ ///
+ /// Delay from the beginning of the period to the first active edge.
+ /// In most simple PWM setups this is `0`, so the duty cycle starts
+ /// immediately at each period’s start.
+ pub duty_offset_ns: u64,
+}
+
+impl From<bindings::pwm_waveform> for Waveform {
+ fn from(wf: bindings::pwm_waveform) -> Self {
+ Waveform {
+ period_length_ns: wf.period_length_ns,
+ duty_length_ns: wf.duty_length_ns,
+ duty_offset_ns: wf.duty_offset_ns,
+ }
+ }
+}
+
+impl From<Waveform> for bindings::pwm_waveform {
+ fn from(wf: Waveform) -> Self {
+ bindings::pwm_waveform {
+ period_length_ns: wf.period_length_ns,
+ duty_length_ns: wf.duty_length_ns,
+ duty_offset_ns: wf.duty_offset_ns,
+ }
+ }
+}
+
+/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct Args(Opaque<bindings::pwm_args>);
+
+impl Args {
+ /// Creates an `Args` wrapper from a C struct pointer.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
+ /// to `bindings::pwm_args` and that the pointed-to data is valid
+ /// for the duration of this function call (as data is copied).
+ unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
+ // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
+ Args(Opaque::new(unsafe { *c_args_ptr }))
+ }
+
+ /// Returns the period of the PWM signal in nanoseconds.
+ pub fn period(&self) -> u64 {
+ // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
+ // managed by the `Opaque` wrapper. This pointer is guaranteed to be
+ // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
+ unsafe { (*self.0.get()).period }
+ }
+
+ /// Returns the polarity of the PWM signal.
+ pub fn polarity(&self) -> Result<Polarity, Error> {
+ // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
+ // managed by the `Opaque` wrapper. This pointer is guaranteed to be
+ // valid and aligned for the lifetime of `self`.
+ let raw_polarity = unsafe { (*self.0.get()).polarity };
+ Polarity::try_from(raw_polarity)
+ }
+}
+
+/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct State(bindings::pwm_state);
+
+impl Default for State {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl State {
+ /// Creates a new zeroed `State`.
+ pub fn new() -> Self {
+ State(bindings::pwm_state::default())
+ }
+
+ /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
+ pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
+ State(c_state)
+ }
+
+ /// Gets the period of the PWM signal in nanoseconds.
+ pub fn period(&self) -> u64 {
+ self.0.period
+ }
+
+ /// Sets the period of the PWM signal in nanoseconds.
+ pub fn set_period(&mut self, period_ns: u64) {
+ self.0.period = period_ns;
+ }
+
+ /// Gets the duty cycle of the PWM signal in nanoseconds.
+ pub fn duty_cycle(&self) -> u64 {
+ self.0.duty_cycle
+ }
+
+ /// Sets the duty cycle of the PWM signal in nanoseconds.
+ pub fn set_duty_cycle(&mut self, duty_ns: u64) {
+ self.0.duty_cycle = duty_ns;
+ }
+
+ /// Returns `true` if the PWM signal is enabled.
+ pub fn enabled(&self) -> bool {
+ self.0.enabled
+ }
+
+ /// Sets the enabled state of the PWM signal.
+ pub fn set_enabled(&mut self, enabled: bool) {
+ self.0.enabled = enabled;
+ }
+
+ /// Gets the polarity of the PWM signal.
+ pub fn polarity(&self) -> Result<Polarity, Error> {
+ Polarity::try_from(self.0.polarity)
+ }
+
+ /// Sets the polarity of the PWM signal.
+ pub fn set_polarity(&mut self, polarity: Polarity) {
+ self.0.polarity = polarity.into();
+ }
+
+ /// Returns `true` if the PWM signal is configured for power usage hint.
+ pub fn usage_power(&self) -> bool {
+ self.0.usage_power
+ }
+
+ /// Sets the power usage hint for the PWM signal.
+ pub fn set_usage_power(&mut self, usage_power: bool) {
+ self.0.usage_power = usage_power;
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-23 18:08 ` [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures Michal Wilczynski
@ 2025-06-27 15:10 ` Uwe Kleine-König
2025-06-27 15:37 ` Miguel Ojeda
2025-06-28 14:38 ` Michal Wilczynski
0 siblings, 2 replies; 29+ messages in thread
From: Uwe Kleine-König @ 2025-06-27 15:10 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 12744 bytes --]
On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
> Introduce the foundational support for PWM abstractions in Rust.
>
> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
> the feature, along with the necessary build-system support and C
> helpers.
>
> It also introduces the first set of safe wrappers for the PWM
> subsystem, covering the basic data carrying C structs and enums:
> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
> - `Waveform`: A wrapper for `struct pwm_waveform`.
> - `Args`: A wrapper for `struct pwm_args`.
> - `State`: A wrapper for `struct pwm_state`.
>
> These types provide memory safe, idiomatic Rust representations of the
> core PWM data structures and form the building blocks for the
> abstractions that will follow.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 6 ++
> drivers/pwm/Kconfig | 13 +++
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/helpers.c | 1 +
> rust/helpers/pwm.c | 20 ++++
> rust/kernel/lib.rs | 2 +
> rust/kernel/pwm.rs | 198 ++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 241 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0c1d245bf7b84f8a78b811e0c9c5a3edc09edc22..a575622454a2ef57ce055c8a8c4765fa4fddc490 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20073,6 +20073,12 @@ F: include/linux/pwm.h
> F: include/linux/pwm_backlight.h
> K: pwm_(config|apply_might_sleep|apply_atomic|ops)
>
> +PWM SUBSYSTEM BINDINGS [RUST]
> +M: Michal Wilczynski <m.wilczynski@samsung.com>
> +S: Maintained
> +F: rust/helpers/pwm.c
> +F: rust/kernel/pwm.rs
> +
> PXA GPIO DRIVER
> M: Robert Jarzmik <robert.jarzmik@free.fr>
> L: linux-gpio@vger.kernel.org
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index d9bcd1e8413eaed1602d6686873e263767c58f5f..cfddeae0eab3523f04f361fb41ccd1345c0c937b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -790,4 +790,17 @@ config PWM_XILINX
> To compile this driver as a module, choose M here: the module
> will be called pwm-xilinx.
>
> + config RUST_PWM_ABSTRACTIONS
> + bool "Rust PWM abstractions support"
> + depends on RUST
> + depends on PWM=y
Currently CONFIG_PWM is a bool, so it cannot be =m. But I considered
making PWM a tristate variable. How would that interfere with Rust
support?
> + help
> + This option enables the safe Rust abstraction layer for the PWM
> + subsystem. It provides idiomatic wrappers and traits necessary for
> + writing PWM controller drivers in Rust.
> +
> + The abstractions handle resource management (like memory and reference
> + counting) and provide safe interfaces to the underlying C core,
> + allowing driver logic to be written in safe Rust.
> +
> endif
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -64,6 +64,7 @@
> #include <linux/pm_opp.h>
> #include <linux/poll.h>
> #include <linux/property.h>
> +#include <linux/pwm.h>
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/security.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 16fa9bca5949b85e8d4cdcfe8e6886124f72d8d8..60879e6d794ce0f87e39caafc5495bf5e8acf8f0 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -31,6 +31,7 @@
> #include "platform.c"
> #include "pci.c"
> #include "pid_namespace.c"
> +#include "pwm.c"
> #include "rbtree.c"
> #include "rcu.c"
> #include "refcount.c"
> diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
> --- /dev/null
> +++ b/rust/helpers/pwm.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +#include <linux/pwm.h>
> +
> +struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
> +{
> + return pwmchip_parent(chip);
> +}
> +
> +void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
> +{
> + pwmchip_set_drvdata(chip, data);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c37f4da1866e993be6230bc6715841..ce1d08b14e456905dbe7b625bbb8ca8b08deae2a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -105,6 +105,8 @@
> pub mod seq_file;
> pub mod sizes;
> mod static_assert;
> +#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
> +pub mod pwm;
> #[doc(hidden)]
> pub mod std_vendor;
> pub mod str;
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..ed681b228c414e7ae8bf80ca649ad497c9dc4ec3
> --- /dev/null
> +++ b/rust/kernel/pwm.rs
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! PWM subsystem abstractions.
> +//!
> +//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
> +
> +use crate::{
> + bindings,
> + prelude::*,
> + types::Opaque,
> +};
> +use core::convert::TryFrom;
> +
> +/// Maximum size for the hardware-specific waveform representation buffer.
> +///
> +/// From C: `#define WFHWSIZE 20`
> +pub const WFHW_MAX_SIZE: usize = 20;
Can we somehow enforce that this doesn't diverge if the C define is
increased?
> +/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> +pub enum Polarity {
> + /// Normal polarity (duty cycle defines the high period of the signal).
> + Normal,
> +
> + /// Inversed polarity (duty cycle defines the low period of the signal).
> + Inversed,
> +}
> +
> +impl TryFrom<bindings::pwm_polarity> for Polarity {
> + type Error = Error;
> +
> + fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
> + match polarity {
> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
> + _ => Err(EINVAL),
> + }
> + }
> +}
> +
> +impl From<Polarity> for bindings::pwm_polarity {
> + fn from(polarity: Polarity) -> Self {
> + match polarity {
> + Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
> + Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
> + }
> + }
> +}
> +
> +/// Represents a PWM waveform configuration.
> +/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
> +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
> +pub struct Waveform {
> + /// Total duration of one complete PWM cycle, in nanoseconds.
> + pub period_length_ns: u64,
> +
> + /// Duty-cycle active time, in nanoseconds.
> + ///
> + /// For a typical normal polarity configuration (active-high) this is the
> + /// high time of the signal.
> + pub duty_length_ns: u64,
> +
> + /// Duty-cycle start offset, in nanoseconds.
> + ///
> + /// Delay from the beginning of the period to the first active edge.
> + /// In most simple PWM setups this is `0`, so the duty cycle starts
> + /// immediately at each period’s start.
> + pub duty_offset_ns: u64,
> +}
> +
> +impl From<bindings::pwm_waveform> for Waveform {
> + fn from(wf: bindings::pwm_waveform) -> Self {
> + Waveform {
> + period_length_ns: wf.period_length_ns,
> + duty_length_ns: wf.duty_length_ns,
> + duty_offset_ns: wf.duty_offset_ns,
> + }
> + }
> +}
> +
> +impl From<Waveform> for bindings::pwm_waveform {
> + fn from(wf: Waveform) -> Self {
> + bindings::pwm_waveform {
> + period_length_ns: wf.period_length_ns,
> + duty_length_ns: wf.duty_length_ns,
> + duty_offset_ns: wf.duty_offset_ns,
> + }
> + }
> +}
> +
> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct Args(Opaque<bindings::pwm_args>);
> +
> +impl Args {
> + /// Creates an `Args` wrapper from a C struct pointer.
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
> + /// to `bindings::pwm_args` and that the pointed-to data is valid
> + /// for the duration of this function call (as data is copied).
> + unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
> + // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
> + Args(Opaque::new(unsafe { *c_args_ptr }))
> + }
> +
> + /// Returns the period of the PWM signal in nanoseconds.
> + pub fn period(&self) -> u64 {
> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
> + // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
> + unsafe { (*self.0.get()).period }
> + }
> +
> + /// Returns the polarity of the PWM signal.
> + pub fn polarity(&self) -> Result<Polarity, Error> {
> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
> + // valid and aligned for the lifetime of `self`.
> + let raw_polarity = unsafe { (*self.0.get()).polarity };
> + Polarity::try_from(raw_polarity)
> + }
> +}
> +
> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct State(bindings::pwm_state);
> +
> +impl Default for State {
> + fn default() -> Self {
> + Self::new()
> + }
> +}
> +
> +impl State {
> + /// Creates a new zeroed `State`.
> + pub fn new() -> Self {
> + State(bindings::pwm_state::default())
> + }
> +
> + /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
> + pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> + State(c_state)
> + }
> +
> + /// Gets the period of the PWM signal in nanoseconds.
> + pub fn period(&self) -> u64 {
> + self.0.period
> + }
> +
> + /// Sets the period of the PWM signal in nanoseconds.
> + pub fn set_period(&mut self, period_ns: u64) {
> + self.0.period = period_ns;
> + }
> +
> + /// Gets the duty cycle of the PWM signal in nanoseconds.
> + pub fn duty_cycle(&self) -> u64 {
> + self.0.duty_cycle
> + }
> +
> + /// Sets the duty cycle of the PWM signal in nanoseconds.
> + pub fn set_duty_cycle(&mut self, duty_ns: u64) {
> + self.0.duty_cycle = duty_ns;
> + }
> +
> + /// Returns `true` if the PWM signal is enabled.
> + pub fn enabled(&self) -> bool {
> + self.0.enabled
> + }
> +
> + /// Sets the enabled state of the PWM signal.
> + pub fn set_enabled(&mut self, enabled: bool) {
> + self.0.enabled = enabled;
> + }
> +
> + /// Gets the polarity of the PWM signal.
> + pub fn polarity(&self) -> Result<Polarity, Error> {
> + Polarity::try_from(self.0.polarity)
> + }
> +
> + /// Sets the polarity of the PWM signal.
> + pub fn set_polarity(&mut self, polarity: Polarity) {
> + self.0.polarity = polarity.into();
> + }
Please don't expose these non-atomic callbacks. pwm_disable() would be
fine.
Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
exposed to/by Rust.
> + /// Returns `true` if the PWM signal is configured for power usage hint.
> + pub fn usage_power(&self) -> bool {
> + self.0.usage_power
> + }
> +
> + /// Sets the power usage hint for the PWM signal.
> + pub fn set_usage_power(&mut self, usage_power: bool) {
> + self.0.usage_power = usage_power;
> + }
I would prefer to not expose usage_power, too.
> +}
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-27 15:10 ` Uwe Kleine-König
@ 2025-06-27 15:37 ` Miguel Ojeda
2025-06-28 14:38 ` Michal Wilczynski
1 sibling, 0 replies; 29+ messages in thread
From: Miguel Ojeda @ 2025-06-27 15:37 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Michal Wilczynski, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Marek Szyprowski,
Benno Lossin, Michael Turquette, Stephen Boyd, linux-kernel,
linux-pwm, rust-for-linux, linux-riscv, devicetree, linux-clk
On Fri, Jun 27, 2025 at 5:10 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
>
> Currently CONFIG_PWM is a bool, so it cannot be =m. But I considered
> making PWM a tristate variable. How would that interfere with Rust
> support?
At the moment, the requirement would still need to be `PWM=y` until
the `kernel` crate is split, which I guess is why this was here (I
assume copied from elsewhere).
Cheers,
Miguel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-27 15:10 ` Uwe Kleine-König
2025-06-27 15:37 ` Miguel Ojeda
@ 2025-06-28 14:38 ` Michal Wilczynski
2025-06-28 19:47 ` Michal Wilczynski
2025-06-29 9:23 ` Uwe Kleine-König
1 sibling, 2 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-28 14:38 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/27/25 17:10, Uwe Kleine-König wrote:
> On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
>> Introduce the foundational support for PWM abstractions in Rust.
>>
>> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
>> the feature, along with the necessary build-system support and C
>> helpers.
>>
>> It also introduces the first set of safe wrappers for the PWM
>> subsystem, covering the basic data carrying C structs and enums:
>> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
>> - `Waveform`: A wrapper for `struct pwm_waveform`.
>> - `Args`: A wrapper for `struct pwm_args`.
>> - `State`: A wrapper for `struct pwm_state`.
>>
>> These types provide memory safe, idiomatic Rust representations of the
>> core PWM data structures and form the building blocks for the
>> abstractions that will follow.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>> MAINTAINERS | 6 ++
>> drivers/pwm/Kconfig | 13 +++
>> rust/bindings/bindings_helper.h | 1 +
>> rust/helpers/helpers.c | 1 +
>> rust/helpers/pwm.c | 20 ++++
>> rust/kernel/lib.rs | 2 +
>> rust/kernel/pwm.rs | 198 ++++++++++++++++++++++++++++++++++++++++
>> 7 files changed, 241 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0c1d245bf7b84f8a78b811e0c9c5a3edc09edc22..a575622454a2ef57ce055c8a8c4765fa4fddc490 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20073,6 +20073,12 @@ F: include/linux/pwm.h
>> F: include/linux/pwm_backlight.h
>> K: pwm_(config|apply_might_sleep|apply_atomic|ops)
>>
>> +PWM SUBSYSTEM BINDINGS [RUST]
>> +M: Michal Wilczynski <m.wilczynski@samsung.com>
>> +S: Maintained
>> +F: rust/helpers/pwm.c
>> +F: rust/kernel/pwm.rs
>> +
>> PXA GPIO DRIVER
>> M: Robert Jarzmik <robert.jarzmik@free.fr>
>> L: linux-gpio@vger.kernel.org
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index d9bcd1e8413eaed1602d6686873e263767c58f5f..cfddeae0eab3523f04f361fb41ccd1345c0c937b 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -790,4 +790,17 @@ config PWM_XILINX
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-xilinx.
>>
>> + config RUST_PWM_ABSTRACTIONS
>> + bool "Rust PWM abstractions support"
>> + depends on RUST
>> + depends on PWM=y
>
> Currently CONFIG_PWM is a bool, so it cannot be =m. But I considered
> making PWM a tristate variable. How would that interfere with Rust
> support?
>
>> + help
>> + This option enables the safe Rust abstraction layer for the PWM
>> + subsystem. It provides idiomatic wrappers and traits necessary for
>> + writing PWM controller drivers in Rust.
>> +
>> + The abstractions handle resource management (like memory and reference
>> + counting) and provide safe interfaces to the underlying C core,
>> + allowing driver logic to be written in safe Rust.
>> +
>> endif
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -64,6 +64,7 @@
>> #include <linux/pm_opp.h>
>> #include <linux/poll.h>
>> #include <linux/property.h>
>> +#include <linux/pwm.h>
>> #include <linux/refcount.h>
>> #include <linux/sched.h>
>> #include <linux/security.h>
>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>> index 16fa9bca5949b85e8d4cdcfe8e6886124f72d8d8..60879e6d794ce0f87e39caafc5495bf5e8acf8f0 100644
>> --- a/rust/helpers/helpers.c
>> +++ b/rust/helpers/helpers.c
>> @@ -31,6 +31,7 @@
>> #include "platform.c"
>> #include "pci.c"
>> #include "pid_namespace.c"
>> +#include "pwm.c"
>> #include "rbtree.c"
>> #include "rcu.c"
>> #include "refcount.c"
>> diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
>> --- /dev/null
>> +++ b/rust/helpers/pwm.c
>> @@ -0,0 +1,20 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +#include <linux/pwm.h>
>> +
>> +struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
>> +{
>> + return pwmchip_parent(chip);
>> +}
>> +
>> +void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
>> +{
>> + return pwmchip_get_drvdata(chip);
>> +}
>> +
>> +void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
>> +{
>> + pwmchip_set_drvdata(chip, data);
>> +}
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c37f4da1866e993be6230bc6715841..ce1d08b14e456905dbe7b625bbb8ca8b08deae2a 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -105,6 +105,8 @@
>> pub mod seq_file;
>> pub mod sizes;
>> mod static_assert;
>> +#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
>> +pub mod pwm;
>> #[doc(hidden)]
>> pub mod std_vendor;
>> pub mod str;
>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..ed681b228c414e7ae8bf80ca649ad497c9dc4ec3
>> --- /dev/null
>> +++ b/rust/kernel/pwm.rs
>> @@ -0,0 +1,198 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! PWM subsystem abstractions.
>> +//!
>> +//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
>> +
>> +use crate::{
>> + bindings,
>> + prelude::*,
>> + types::Opaque,
>> +};
>> +use core::convert::TryFrom;
>> +
>> +/// Maximum size for the hardware-specific waveform representation buffer.
>> +///
>> +/// From C: `#define WFHWSIZE 20`
>> +pub const WFHW_MAX_SIZE: usize = 20;
>
> Can we somehow enforce that this doesn't diverge if the C define is
> increased?
You are absolutely right. The hardcoded value is a maintenance risk. The
#define is in core.c, so bindgen cannot see it.
I can address this by submitting a patch to move the #define WFHWSIZE to
include/linux/pwm.h. This will make it part of the public API, allow
bindgen to generate a binding for it, and ensure the Rust code can never
diverge. Is this fine ?
>
>> +/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
>> +pub enum Polarity {
>> + /// Normal polarity (duty cycle defines the high period of the signal).
>> + Normal,
>> +
>> + /// Inversed polarity (duty cycle defines the low period of the signal).
>> + Inversed,
>> +}
>> +
>> +impl TryFrom<bindings::pwm_polarity> for Polarity {
>> + type Error = Error;
>> +
>> + fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
>> + match polarity {
>> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
>> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
>> + _ => Err(EINVAL),
>> + }
>> + }
>> +}
>> +
>> +impl From<Polarity> for bindings::pwm_polarity {
>> + fn from(polarity: Polarity) -> Self {
>> + match polarity {
>> + Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
>> + Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
>> + }
>> + }
>> +}
>> +
>> +/// Represents a PWM waveform configuration.
>> +/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
>> +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
>> +pub struct Waveform {
>> + /// Total duration of one complete PWM cycle, in nanoseconds.
>> + pub period_length_ns: u64,
>> +
>> + /// Duty-cycle active time, in nanoseconds.
>> + ///
>> + /// For a typical normal polarity configuration (active-high) this is the
>> + /// high time of the signal.
>> + pub duty_length_ns: u64,
>> +
>> + /// Duty-cycle start offset, in nanoseconds.
>> + ///
>> + /// Delay from the beginning of the period to the first active edge.
>> + /// In most simple PWM setups this is `0`, so the duty cycle starts
>> + /// immediately at each period’s start.
>> + pub duty_offset_ns: u64,
>> +}
>> +
>> +impl From<bindings::pwm_waveform> for Waveform {
>> + fn from(wf: bindings::pwm_waveform) -> Self {
>> + Waveform {
>> + period_length_ns: wf.period_length_ns,
>> + duty_length_ns: wf.duty_length_ns,
>> + duty_offset_ns: wf.duty_offset_ns,
>> + }
>> + }
>> +}
>> +
>> +impl From<Waveform> for bindings::pwm_waveform {
>> + fn from(wf: Waveform) -> Self {
>> + bindings::pwm_waveform {
>> + period_length_ns: wf.period_length_ns,
>> + duty_length_ns: wf.duty_length_ns,
>> + duty_offset_ns: wf.duty_offset_ns,
>> + }
>> + }
>> +}
>> +
>> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
>> +#[repr(transparent)]
>> +pub struct Args(Opaque<bindings::pwm_args>);
>> +
>> +impl Args {
>> + /// Creates an `Args` wrapper from a C struct pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
>> + /// to `bindings::pwm_args` and that the pointed-to data is valid
>> + /// for the duration of this function call (as data is copied).
>> + unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
>> + // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
>> + Args(Opaque::new(unsafe { *c_args_ptr }))
>> + }
>> +
>> + /// Returns the period of the PWM signal in nanoseconds.
>> + pub fn period(&self) -> u64 {
>> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
>> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
>> + // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
>> + unsafe { (*self.0.get()).period }
>> + }
>> +
>> + /// Returns the polarity of the PWM signal.
>> + pub fn polarity(&self) -> Result<Polarity, Error> {
>> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
>> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
>> + // valid and aligned for the lifetime of `self`.
>> + let raw_polarity = unsafe { (*self.0.get()).polarity };
>> + Polarity::try_from(raw_polarity)
>> + }
>> +}
>> +
>> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
>> +#[repr(transparent)]
>> +pub struct State(bindings::pwm_state);
>> +
>> +impl Default for State {
>> + fn default() -> Self {
>> + Self::new()
>> + }
>> +}
>> +
>> +impl State {
>> + /// Creates a new zeroed `State`.
>> + pub fn new() -> Self {
>> + State(bindings::pwm_state::default())
>> + }
>> +
>> + /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
>> + pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
>> + State(c_state)
>> + }
>> +
>> + /// Gets the period of the PWM signal in nanoseconds.
>> + pub fn period(&self) -> u64 {
>> + self.0.period
>> + }
>> +
>> + /// Sets the period of the PWM signal in nanoseconds.
>> + pub fn set_period(&mut self, period_ns: u64) {
>> + self.0.period = period_ns;
>> + }
>> +
>> + /// Gets the duty cycle of the PWM signal in nanoseconds.
>> + pub fn duty_cycle(&self) -> u64 {
>> + self.0.duty_cycle
>> + }
>> +
>> + /// Sets the duty cycle of the PWM signal in nanoseconds.
>> + pub fn set_duty_cycle(&mut self, duty_ns: u64) {
>> + self.0.duty_cycle = duty_ns;
>> + }
>> +
>> + /// Returns `true` if the PWM signal is enabled.
>> + pub fn enabled(&self) -> bool {
>> + self.0.enabled
>> + }
>> +
>> + /// Sets the enabled state of the PWM signal.
>> + pub fn set_enabled(&mut self, enabled: bool) {
>> + self.0.enabled = enabled;
>> + }
>> +
>> + /// Gets the polarity of the PWM signal.
>> + pub fn polarity(&self) -> Result<Polarity, Error> {
>> + Polarity::try_from(self.0.polarity)
>> + }
>> +
>> + /// Sets the polarity of the PWM signal.
>> + pub fn set_polarity(&mut self, polarity: Polarity) {
>> + self.0.polarity = polarity.into();
>> + }
>
> Please don't expose these non-atomic callbacks. pwm_disable() would be
> fine.
>
> Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
> exposed to/by Rust.
OK, I'll remove all the setters from the State, while will keep the
getters, as they would be useful in apply callbacks. Will implement
additional functions for Device i.e set_waveform, round_waveform and
get_waveform, and the new enum to expose the result of the
round_waveform more idiomatically.
/// Describes the outcome of a `round_waveform` operation.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum RoundingOutcome {
/// The requested waveform was achievable exactly or by rounding values down.
ExactOrRoundedDown,
/// The requested waveform could only be achieved by rounding up.
RoundedUp,
}
>
>> + /// Returns `true` if the PWM signal is configured for power usage hint.
>> + pub fn usage_power(&self) -> bool {
>> + self.0.usage_power
>> + }
>> +
>> + /// Sets the power usage hint for the PWM signal.
>> + pub fn set_usage_power(&mut self, usage_power: bool) {
>> + self.0.usage_power = usage_power;
>> + }
>
> I would prefer to not expose usage_power, too.
>
>> +}
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-28 14:38 ` Michal Wilczynski
@ 2025-06-28 19:47 ` Michal Wilczynski
2025-06-29 10:29 ` Uwe Kleine-König
2025-06-29 9:23 ` Uwe Kleine-König
1 sibling, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-28 19:47 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/28/25 16:38, Michal Wilczynski wrote:
>
>
> On 6/27/25 17:10, Uwe Kleine-König wrote:
>> On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
>>> Introduce the foundational support for PWM abstractions in Rust.
>>>
>>> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
>>> the feature, along with the necessary build-system support and C
>>> helpers.
>>>
>>> It also introduces the first set of safe wrappers for the PWM
>>> subsystem, covering the basic data carrying C structs and enums:
>>> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
>>> - `Waveform`: A wrapper for `struct pwm_waveform`.
>>> - `Args`: A wrapper for `struct pwm_args`.
>>> - `State`: A wrapper for `struct pwm_state`.
>>>
>>> These types provide memory safe, idiomatic Rust representations of the
>>> core PWM data structures and form the building blocks for the
>>> abstractions that will follow.
>>>
>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>> ---
>>> MAINTAINERS | 6 ++
>>> drivers/pwm/Kconfig | 13 +++
>>> rust/bindings/bindings_helper.h | 1 +
>>> rust/helpers/helpers.c | 1 +
>>> rust/helpers/pwm.c | 20 ++++
>>> rust/kernel/lib.rs | 2 +
>>> rust/kernel/pwm.rs | 198 ++++++++++++++++++++++++++++++++++++++++
>>> 7 files changed, 241 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 0c1d245bf7b84f8a78b811e0c9c5a3edc09edc22..a575622454a2ef57ce055c8a8c4765fa4fddc490 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -20073,6 +20073,12 @@ F: include/linux/pwm.h
>>> F: include/linux/pwm_backlight.h
>>> K: pwm_(config|apply_might_sleep|apply_atomic|ops)
>>>
>>> +PWM SUBSYSTEM BINDINGS [RUST]
>>> +M: Michal Wilczynski <m.wilczynski@samsung.com>
>>> +S: Maintained
>>> +F: rust/helpers/pwm.c
>>> +F: rust/kernel/pwm.rs
>>> +
>>> PXA GPIO DRIVER
>>> M: Robert Jarzmik <robert.jarzmik@free.fr>
>>> L: linux-gpio@vger.kernel.org
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index d9bcd1e8413eaed1602d6686873e263767c58f5f..cfddeae0eab3523f04f361fb41ccd1345c0c937b 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -790,4 +790,17 @@ config PWM_XILINX
>>> To compile this driver as a module, choose M here: the module
>>> will be called pwm-xilinx.
>>>
>>> + config RUST_PWM_ABSTRACTIONS
>>> + bool "Rust PWM abstractions support"
>>> + depends on RUST
>>> + depends on PWM=y
>>
>> Currently CONFIG_PWM is a bool, so it cannot be =m. But I considered
>> making PWM a tristate variable. How would that interfere with Rust
>> support?
>>
>>> + help
>>> + This option enables the safe Rust abstraction layer for the PWM
>>> + subsystem. It provides idiomatic wrappers and traits necessary for
>>> + writing PWM controller drivers in Rust.
>>> +
>>> + The abstractions handle resource management (like memory and reference
>>> + counting) and provide safe interfaces to the underlying C core,
>>> + allowing driver logic to be written in safe Rust.
>>> +
>>> endif
>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>> index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
>>> --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -64,6 +64,7 @@
>>> #include <linux/pm_opp.h>
>>> #include <linux/poll.h>
>>> #include <linux/property.h>
>>> +#include <linux/pwm.h>
>>> #include <linux/refcount.h>
>>> #include <linux/sched.h>
>>> #include <linux/security.h>
>>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>>> index 16fa9bca5949b85e8d4cdcfe8e6886124f72d8d8..60879e6d794ce0f87e39caafc5495bf5e8acf8f0 100644
>>> --- a/rust/helpers/helpers.c
>>> +++ b/rust/helpers/helpers.c
>>> @@ -31,6 +31,7 @@
>>> #include "platform.c"
>>> #include "pci.c"
>>> #include "pid_namespace.c"
>>> +#include "pwm.c"
>>> #include "rbtree.c"
>>> #include "rcu.c"
>>> #include "refcount.c"
>>> diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
>>> --- /dev/null
>>> +++ b/rust/helpers/pwm.c
>>> @@ -0,0 +1,20 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>>> +
>>> +#include <linux/pwm.h>
>>> +
>>> +struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
>>> +{
>>> + return pwmchip_parent(chip);
>>> +}
>>> +
>>> +void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
>>> +{
>>> + return pwmchip_get_drvdata(chip);
>>> +}
>>> +
>>> +void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
>>> +{
>>> + pwmchip_set_drvdata(chip, data);
>>> +}
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 6b4774b2b1c37f4da1866e993be6230bc6715841..ce1d08b14e456905dbe7b625bbb8ca8b08deae2a 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -105,6 +105,8 @@
>>> pub mod seq_file;
>>> pub mod sizes;
>>> mod static_assert;
>>> +#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
>>> +pub mod pwm;
>>> #[doc(hidden)]
>>> pub mod std_vendor;
>>> pub mod str;
>>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..ed681b228c414e7ae8bf80ca649ad497c9dc4ec3
>>> --- /dev/null
>>> +++ b/rust/kernel/pwm.rs
>>> @@ -0,0 +1,198 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>>> +
>>> +//! PWM subsystem abstractions.
>>> +//!
>>> +//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
>>> +
>>> +use crate::{
>>> + bindings,
>>> + prelude::*,
>>> + types::Opaque,
>>> +};
>>> +use core::convert::TryFrom;
>>> +
>>> +/// Maximum size for the hardware-specific waveform representation buffer.
>>> +///
>>> +/// From C: `#define WFHWSIZE 20`
>>> +pub const WFHW_MAX_SIZE: usize = 20;
>>
>> Can we somehow enforce that this doesn't diverge if the C define is
>> increased?
>
> You are absolutely right. The hardcoded value is a maintenance risk. The
> #define is in core.c, so bindgen cannot see it.
>
> I can address this by submitting a patch to move the #define WFHWSIZE to
> include/linux/pwm.h. This will make it part of the public API, allow
> bindgen to generate a binding for it, and ensure the Rust code can never
> diverge. Is this fine ?
>
>>
>>> +/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
>>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
>>> +pub enum Polarity {
>>> + /// Normal polarity (duty cycle defines the high period of the signal).
>>> + Normal,
>>> +
>>> + /// Inversed polarity (duty cycle defines the low period of the signal).
>>> + Inversed,
>>> +}
>>> +
>>> +impl TryFrom<bindings::pwm_polarity> for Polarity {
>>> + type Error = Error;
>>> +
>>> + fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
>>> + match polarity {
>>> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
>>> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
>>> + _ => Err(EINVAL),
>>> + }
>>> + }
>>> +}
>>> +
>>> +impl From<Polarity> for bindings::pwm_polarity {
>>> + fn from(polarity: Polarity) -> Self {
>>> + match polarity {
>>> + Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
>>> + Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
>>> + }
>>> + }
>>> +}
>>> +
>>> +/// Represents a PWM waveform configuration.
>>> +/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
>>> +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
>>> +pub struct Waveform {
>>> + /// Total duration of one complete PWM cycle, in nanoseconds.
>>> + pub period_length_ns: u64,
>>> +
>>> + /// Duty-cycle active time, in nanoseconds.
>>> + ///
>>> + /// For a typical normal polarity configuration (active-high) this is the
>>> + /// high time of the signal.
>>> + pub duty_length_ns: u64,
>>> +
>>> + /// Duty-cycle start offset, in nanoseconds.
>>> + ///
>>> + /// Delay from the beginning of the period to the first active edge.
>>> + /// In most simple PWM setups this is `0`, so the duty cycle starts
>>> + /// immediately at each period’s start.
>>> + pub duty_offset_ns: u64,
>>> +}
>>> +
>>> +impl From<bindings::pwm_waveform> for Waveform {
>>> + fn from(wf: bindings::pwm_waveform) -> Self {
>>> + Waveform {
>>> + period_length_ns: wf.period_length_ns,
>>> + duty_length_ns: wf.duty_length_ns,
>>> + duty_offset_ns: wf.duty_offset_ns,
>>> + }
>>> + }
>>> +}
>>> +
>>> +impl From<Waveform> for bindings::pwm_waveform {
>>> + fn from(wf: Waveform) -> Self {
>>> + bindings::pwm_waveform {
>>> + period_length_ns: wf.period_length_ns,
>>> + duty_length_ns: wf.duty_length_ns,
>>> + duty_offset_ns: wf.duty_offset_ns,
>>> + }
>>> + }
>>> +}
>>> +
>>> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
>>> +#[repr(transparent)]
>>> +pub struct Args(Opaque<bindings::pwm_args>);
>>> +
>>> +impl Args {
>>> + /// Creates an `Args` wrapper from a C struct pointer.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
>>> + /// to `bindings::pwm_args` and that the pointed-to data is valid
>>> + /// for the duration of this function call (as data is copied).
>>> + unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
>>> + // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
>>> + Args(Opaque::new(unsafe { *c_args_ptr }))
>>> + }
>>> +
>>> + /// Returns the period of the PWM signal in nanoseconds.
>>> + pub fn period(&self) -> u64 {
>>> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
>>> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
>>> + // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
>>> + unsafe { (*self.0.get()).period }
>>> + }
>>> +
>>> + /// Returns the polarity of the PWM signal.
>>> + pub fn polarity(&self) -> Result<Polarity, Error> {
>>> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
>>> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
>>> + // valid and aligned for the lifetime of `self`.
>>> + let raw_polarity = unsafe { (*self.0.get()).polarity };
>>> + Polarity::try_from(raw_polarity)
>>> + }
>>> +}
>>> +
>>> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
>>> +#[repr(transparent)]
>>> +pub struct State(bindings::pwm_state);
>>> +
>>> +impl Default for State {
>>> + fn default() -> Self {
>>> + Self::new()
>>> + }
>>> +}
>>> +
>>> +impl State {
>>> + /// Creates a new zeroed `State`.
>>> + pub fn new() -> Self {
>>> + State(bindings::pwm_state::default())
>>> + }
>>> +
>>> + /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
>>> + pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
>>> + State(c_state)
>>> + }
>>> +
>>> + /// Gets the period of the PWM signal in nanoseconds.
>>> + pub fn period(&self) -> u64 {
>>> + self.0.period
>>> + }
>>> +
>>> + /// Sets the period of the PWM signal in nanoseconds.
>>> + pub fn set_period(&mut self, period_ns: u64) {
>>> + self.0.period = period_ns;
>>> + }
>>> +
>>> + /// Gets the duty cycle of the PWM signal in nanoseconds.
>>> + pub fn duty_cycle(&self) -> u64 {
>>> + self.0.duty_cycle
>>> + }
>>> +
>>> + /// Sets the duty cycle of the PWM signal in nanoseconds.
>>> + pub fn set_duty_cycle(&mut self, duty_ns: u64) {
>>> + self.0.duty_cycle = duty_ns;
>>> + }
>>> +
>>> + /// Returns `true` if the PWM signal is enabled.
>>> + pub fn enabled(&self) -> bool {
>>> + self.0.enabled
>>> + }
>>> +
>>> + /// Sets the enabled state of the PWM signal.
>>> + pub fn set_enabled(&mut self, enabled: bool) {
>>> + self.0.enabled = enabled;
>>> + }
>>> +
>>> + /// Gets the polarity of the PWM signal.
>>> + pub fn polarity(&self) -> Result<Polarity, Error> {
>>> + Polarity::try_from(self.0.polarity)
>>> + }
>>> +
>>> + /// Sets the polarity of the PWM signal.
>>> + pub fn set_polarity(&mut self, polarity: Polarity) {
>>> + self.0.polarity = polarity.into();
>>> + }
>>
>> Please don't expose these non-atomic callbacks. pwm_disable() would be
>> fine.
Hmm, I've just realized that without those setters it would most likely
impossible to correctly implement the get_state callback. Shall I keep
them ? The meaning of those setters is to update the State struct, not
change polarity of the running PWM channel
>>
>> Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
>> exposed to/by Rust.
>
>
> OK, I'll remove all the setters from the State, while will keep the
> getters, as they would be useful in apply callbacks. Will implement
> additional functions for Device i.e set_waveform, round_waveform and
> get_waveform, and the new enum to expose the result of the
> round_waveform more idiomatically.
>
> /// Describes the outcome of a `round_waveform` operation.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub enum RoundingOutcome {
> /// The requested waveform was achievable exactly or by rounding values down.
> ExactOrRoundedDown,
>
> /// The requested waveform could only be achieved by rounding up.
> RoundedUp,
> }
>
>>
>>> + /// Returns `true` if the PWM signal is configured for power usage hint.
>>> + pub fn usage_power(&self) -> bool {
>>> + self.0.usage_power
>>> + }
>>> +
>>> + /// Sets the power usage hint for the PWM signal.
>>> + pub fn set_usage_power(&mut self, usage_power: bool) {
>>> + self.0.usage_power = usage_power;
>>> + }
>>
>> I would prefer to not expose usage_power, too.
>>
>>> +}
>>
>> Best regards
>> Uwe
>
> Best regards,
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-28 19:47 ` Michal Wilczynski
@ 2025-06-29 10:29 ` Uwe Kleine-König
2025-07-01 8:24 ` Michal Wilczynski
0 siblings, 1 reply; 29+ messages in thread
From: Uwe Kleine-König @ 2025-06-29 10:29 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 14392 bytes --]
On Sat, Jun 28, 2025 at 09:47:19PM +0200, Michal Wilczynski wrote:
>
>
> On 6/28/25 16:38, Michal Wilczynski wrote:
> >
> >
> > On 6/27/25 17:10, Uwe Kleine-König wrote:
> >> On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
> >>> Introduce the foundational support for PWM abstractions in Rust.
> >>>
> >>> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
> >>> the feature, along with the necessary build-system support and C
> >>> helpers.
> >>>
> >>> It also introduces the first set of safe wrappers for the PWM
> >>> subsystem, covering the basic data carrying C structs and enums:
> >>> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
> >>> - `Waveform`: A wrapper for `struct pwm_waveform`.
> >>> - `Args`: A wrapper for `struct pwm_args`.
> >>> - `State`: A wrapper for `struct pwm_state`.
> >>>
> >>> These types provide memory safe, idiomatic Rust representations of the
> >>> core PWM data structures and form the building blocks for the
> >>> abstractions that will follow.
> >>>
> >>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >>> ---
> >>> MAINTAINERS | 6 ++
> >>> drivers/pwm/Kconfig | 13 +++
> >>> rust/bindings/bindings_helper.h | 1 +
> >>> rust/helpers/helpers.c | 1 +
> >>> rust/helpers/pwm.c | 20 ++++
> >>> rust/kernel/lib.rs | 2 +
> >>> rust/kernel/pwm.rs | 198 ++++++++++++++++++++++++++++++++++++++++
> >>> 7 files changed, 241 insertions(+)
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 0c1d245bf7b84f8a78b811e0c9c5a3edc09edc22..a575622454a2ef57ce055c8a8c4765fa4fddc490 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -20073,6 +20073,12 @@ F: include/linux/pwm.h
> >>> F: include/linux/pwm_backlight.h
> >>> K: pwm_(config|apply_might_sleep|apply_atomic|ops)
> >>>
> >>> +PWM SUBSYSTEM BINDINGS [RUST]
> >>> +M: Michal Wilczynski <m.wilczynski@samsung.com>
> >>> +S: Maintained
> >>> +F: rust/helpers/pwm.c
> >>> +F: rust/kernel/pwm.rs
> >>> +
> >>> PXA GPIO DRIVER
> >>> M: Robert Jarzmik <robert.jarzmik@free.fr>
> >>> L: linux-gpio@vger.kernel.org
> >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >>> index d9bcd1e8413eaed1602d6686873e263767c58f5f..cfddeae0eab3523f04f361fb41ccd1345c0c937b 100644
> >>> --- a/drivers/pwm/Kconfig
> >>> +++ b/drivers/pwm/Kconfig
> >>> @@ -790,4 +790,17 @@ config PWM_XILINX
> >>> To compile this driver as a module, choose M here: the module
> >>> will be called pwm-xilinx.
> >>>
> >>> + config RUST_PWM_ABSTRACTIONS
> >>> + bool "Rust PWM abstractions support"
> >>> + depends on RUST
> >>> + depends on PWM=y
> >>
> >> Currently CONFIG_PWM is a bool, so it cannot be =m. But I considered
> >> making PWM a tristate variable. How would that interfere with Rust
> >> support?
> >>
> >>> + help
> >>> + This option enables the safe Rust abstraction layer for the PWM
> >>> + subsystem. It provides idiomatic wrappers and traits necessary for
> >>> + writing PWM controller drivers in Rust.
> >>> +
> >>> + The abstractions handle resource management (like memory and reference
> >>> + counting) and provide safe interfaces to the underlying C core,
> >>> + allowing driver logic to be written in safe Rust.
> >>> +
> >>> endif
> >>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> >>> index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
> >>> --- a/rust/bindings/bindings_helper.h
> >>> +++ b/rust/bindings/bindings_helper.h
> >>> @@ -64,6 +64,7 @@
> >>> #include <linux/pm_opp.h>
> >>> #include <linux/poll.h>
> >>> #include <linux/property.h>
> >>> +#include <linux/pwm.h>
> >>> #include <linux/refcount.h>
> >>> #include <linux/sched.h>
> >>> #include <linux/security.h>
> >>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> >>> index 16fa9bca5949b85e8d4cdcfe8e6886124f72d8d8..60879e6d794ce0f87e39caafc5495bf5e8acf8f0 100644
> >>> --- a/rust/helpers/helpers.c
> >>> +++ b/rust/helpers/helpers.c
> >>> @@ -31,6 +31,7 @@
> >>> #include "platform.c"
> >>> #include "pci.c"
> >>> #include "pid_namespace.c"
> >>> +#include "pwm.c"
> >>> #include "rbtree.c"
> >>> #include "rcu.c"
> >>> #include "refcount.c"
> >>> diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
> >>> --- /dev/null
> >>> +++ b/rust/helpers/pwm.c
> >>> @@ -0,0 +1,20 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> >>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> >>> +
> >>> +#include <linux/pwm.h>
> >>> +
> >>> +struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
> >>> +{
> >>> + return pwmchip_parent(chip);
> >>> +}
> >>> +
> >>> +void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
> >>> +{
> >>> + return pwmchip_get_drvdata(chip);
> >>> +}
> >>> +
> >>> +void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
> >>> +{
> >>> + pwmchip_set_drvdata(chip, data);
> >>> +}
> >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >>> index 6b4774b2b1c37f4da1866e993be6230bc6715841..ce1d08b14e456905dbe7b625bbb8ca8b08deae2a 100644
> >>> --- a/rust/kernel/lib.rs
> >>> +++ b/rust/kernel/lib.rs
> >>> @@ -105,6 +105,8 @@
> >>> pub mod seq_file;
> >>> pub mod sizes;
> >>> mod static_assert;
> >>> +#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
> >>> +pub mod pwm;
> >>> #[doc(hidden)]
> >>> pub mod std_vendor;
> >>> pub mod str;
> >>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..ed681b228c414e7ae8bf80ca649ad497c9dc4ec3
> >>> --- /dev/null
> >>> +++ b/rust/kernel/pwm.rs
> >>> @@ -0,0 +1,198 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> >>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> >>> +
> >>> +//! PWM subsystem abstractions.
> >>> +//!
> >>> +//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
> >>> +
> >>> +use crate::{
> >>> + bindings,
> >>> + prelude::*,
> >>> + types::Opaque,
> >>> +};
> >>> +use core::convert::TryFrom;
> >>> +
> >>> +/// Maximum size for the hardware-specific waveform representation buffer.
> >>> +///
> >>> +/// From C: `#define WFHWSIZE 20`
> >>> +pub const WFHW_MAX_SIZE: usize = 20;
> >>
> >> Can we somehow enforce that this doesn't diverge if the C define is
> >> increased?
> >
> > You are absolutely right. The hardcoded value is a maintenance risk. The
> > #define is in core.c, so bindgen cannot see it.
> >
> > I can address this by submitting a patch to move the #define WFHWSIZE to
> > include/linux/pwm.h. This will make it part of the public API, allow
> > bindgen to generate a binding for it, and ensure the Rust code can never
> > diverge. Is this fine ?
> >
> >>
> >>> +/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
> >>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> >>> +pub enum Polarity {
> >>> + /// Normal polarity (duty cycle defines the high period of the signal).
> >>> + Normal,
> >>> +
> >>> + /// Inversed polarity (duty cycle defines the low period of the signal).
> >>> + Inversed,
> >>> +}
> >>> +
> >>> +impl TryFrom<bindings::pwm_polarity> for Polarity {
> >>> + type Error = Error;
> >>> +
> >>> + fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
> >>> + match polarity {
> >>> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
> >>> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
> >>> + _ => Err(EINVAL),
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> +impl From<Polarity> for bindings::pwm_polarity {
> >>> + fn from(polarity: Polarity) -> Self {
> >>> + match polarity {
> >>> + Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
> >>> + Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> +/// Represents a PWM waveform configuration.
> >>> +/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
> >>> +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
> >>> +pub struct Waveform {
> >>> + /// Total duration of one complete PWM cycle, in nanoseconds.
> >>> + pub period_length_ns: u64,
> >>> +
> >>> + /// Duty-cycle active time, in nanoseconds.
> >>> + ///
> >>> + /// For a typical normal polarity configuration (active-high) this is the
> >>> + /// high time of the signal.
> >>> + pub duty_length_ns: u64,
> >>> +
> >>> + /// Duty-cycle start offset, in nanoseconds.
> >>> + ///
> >>> + /// Delay from the beginning of the period to the first active edge.
> >>> + /// In most simple PWM setups this is `0`, so the duty cycle starts
> >>> + /// immediately at each period’s start.
> >>> + pub duty_offset_ns: u64,
> >>> +}
> >>> +
> >>> +impl From<bindings::pwm_waveform> for Waveform {
> >>> + fn from(wf: bindings::pwm_waveform) -> Self {
> >>> + Waveform {
> >>> + period_length_ns: wf.period_length_ns,
> >>> + duty_length_ns: wf.duty_length_ns,
> >>> + duty_offset_ns: wf.duty_offset_ns,
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> +impl From<Waveform> for bindings::pwm_waveform {
> >>> + fn from(wf: Waveform) -> Self {
> >>> + bindings::pwm_waveform {
> >>> + period_length_ns: wf.period_length_ns,
> >>> + duty_length_ns: wf.duty_length_ns,
> >>> + duty_offset_ns: wf.duty_offset_ns,
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
> >>> +#[repr(transparent)]
> >>> +pub struct Args(Opaque<bindings::pwm_args>);
> >>> +
> >>> +impl Args {
> >>> + /// Creates an `Args` wrapper from a C struct pointer.
> >>> + ///
> >>> + /// # Safety
> >>> + ///
> >>> + /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
> >>> + /// to `bindings::pwm_args` and that the pointed-to data is valid
> >>> + /// for the duration of this function call (as data is copied).
> >>> + unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
> >>> + // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
> >>> + Args(Opaque::new(unsafe { *c_args_ptr }))
> >>> + }
> >>> +
> >>> + /// Returns the period of the PWM signal in nanoseconds.
> >>> + pub fn period(&self) -> u64 {
> >>> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
> >>> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
> >>> + // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
> >>> + unsafe { (*self.0.get()).period }
> >>> + }
> >>> +
> >>> + /// Returns the polarity of the PWM signal.
> >>> + pub fn polarity(&self) -> Result<Polarity, Error> {
> >>> + // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
> >>> + // managed by the `Opaque` wrapper. This pointer is guaranteed to be
> >>> + // valid and aligned for the lifetime of `self`.
> >>> + let raw_polarity = unsafe { (*self.0.get()).polarity };
> >>> + Polarity::try_from(raw_polarity)
> >>> + }
> >>> +}
> >>> +
> >>> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
> >>> +#[repr(transparent)]
> >>> +pub struct State(bindings::pwm_state);
> >>> +
> >>> +impl Default for State {
> >>> + fn default() -> Self {
> >>> + Self::new()
> >>> + }
> >>> +}
> >>> +
> >>> +impl State {
> >>> + /// Creates a new zeroed `State`.
> >>> + pub fn new() -> Self {
> >>> + State(bindings::pwm_state::default())
> >>> + }
> >>> +
> >>> + /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
> >>> + pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> >>> + State(c_state)
> >>> + }
> >>> +
> >>> + /// Gets the period of the PWM signal in nanoseconds.
> >>> + pub fn period(&self) -> u64 {
> >>> + self.0.period
> >>> + }
> >>> +
> >>> + /// Sets the period of the PWM signal in nanoseconds.
> >>> + pub fn set_period(&mut self, period_ns: u64) {
> >>> + self.0.period = period_ns;
> >>> + }
> >>> +
> >>> + /// Gets the duty cycle of the PWM signal in nanoseconds.
> >>> + pub fn duty_cycle(&self) -> u64 {
> >>> + self.0.duty_cycle
> >>> + }
> >>> +
> >>> + /// Sets the duty cycle of the PWM signal in nanoseconds.
> >>> + pub fn set_duty_cycle(&mut self, duty_ns: u64) {
> >>> + self.0.duty_cycle = duty_ns;
> >>> + }
> >>> +
> >>> + /// Returns `true` if the PWM signal is enabled.
> >>> + pub fn enabled(&self) -> bool {
> >>> + self.0.enabled
> >>> + }
> >>> +
> >>> + /// Sets the enabled state of the PWM signal.
> >>> + pub fn set_enabled(&mut self, enabled: bool) {
> >>> + self.0.enabled = enabled;
> >>> + }
> >>> +
> >>> + /// Gets the polarity of the PWM signal.
> >>> + pub fn polarity(&self) -> Result<Polarity, Error> {
> >>> + Polarity::try_from(self.0.polarity)
> >>> + }
> >>> +
> >>> + /// Sets the polarity of the PWM signal.
> >>> + pub fn set_polarity(&mut self, polarity: Polarity) {
> >>> + self.0.polarity = polarity.into();
> >>> + }
> >>
> >> Please don't expose these non-atomic callbacks. pwm_disable() would be
> >> fine.
>
> Hmm, I've just realized that without those setters it would most likely
> impossible to correctly implement the get_state callback.
You shouldn't implement the get_state callback for a waveform driver.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-29 10:29 ` Uwe Kleine-König
@ 2025-07-01 8:24 ` Michal Wilczynski
2025-07-01 13:47 ` Uwe Kleine-König
0 siblings, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-07-01 8:24 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/29/25 12:29, Uwe Kleine-König wrote:
> On Sat, Jun 28, 2025 at 09:47:19PM +0200, Michal Wilczynski wrote:
>>>>> + /// Sets the polarity of the PWM signal.
>>>>> + pub fn set_polarity(&mut self, polarity: Polarity) {
>>>>> + self.0.polarity = polarity.into();
>>>>> + }
>>>>
>>>> Please don't expose these non-atomic callbacks. pwm_disable() would be
>>>> fine.
>>
>> Hmm, I've just realized that without those setters it would most likely
>> impossible to correctly implement the get_state callback.
>
> You shouldn't implement the get_state callback for a waveform driver.
You're right that a new driver using the waveform API shouldn't
implement .get_state.
My goal for the abstraction layer, however, is to be flexible enough to
support writing both modern waveform drivers and legacy style drivers
that use the .apply and .get_state callbacks.
To implement the .get_state callback, a driver needs the ability to
construct a State struct and populate its fields from hardware values
before returning it to the PWM core. Without this ability there is no
way to implement get_state callback.
I think the cleaner way, without the setters would be to update the
`new` like so:
pub fn new(
period: u64,
duty_cycle: u64,
polarity: Polarity,
enabled: bool,
usage_power: bool,
) -> Self {
let raw_c_state = bindings::pwm_state {
period,
duty_cycle,
polarity: polarity.into(),
enabled,
usage_power,
};
State(raw_c_state)
}
This way the get_state callback would be responsible for creating new
state and initializing it, instead of passing the mutable State to
get_state.
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-07-01 8:24 ` Michal Wilczynski
@ 2025-07-01 13:47 ` Uwe Kleine-König
0 siblings, 0 replies; 29+ messages in thread
From: Uwe Kleine-König @ 2025-07-01 13:47 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
Hello Michal,
On Tue, Jul 01, 2025 at 10:24:54AM +0200, Michal Wilczynski wrote:
> On 6/29/25 12:29, Uwe Kleine-König wrote:
> > On Sat, Jun 28, 2025 at 09:47:19PM +0200, Michal Wilczynski wrote:
>
> >>>>> + /// Sets the polarity of the PWM signal.
> >>>>> + pub fn set_polarity(&mut self, polarity: Polarity) {
> >>>>> + self.0.polarity = polarity.into();
> >>>>> + }
> >>>>
> >>>> Please don't expose these non-atomic callbacks. pwm_disable() would be
> >>>> fine.
> >>
> >> Hmm, I've just realized that without those setters it would most likely
> >> impossible to correctly implement the get_state callback.
> >
> > You shouldn't implement the get_state callback for a waveform driver.
>
> You're right that a new driver using the waveform API shouldn't
> implement .get_state.
>
> My goal for the abstraction layer, however, is to be flexible enough to
> support writing both modern waveform drivers and legacy style drivers
> that use the .apply and .get_state callbacks.
No, please don't. New C drivers should implement the waveform API. The
same holds true for Rust drivers. So don't create a door that isn't
supposed to be used.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-28 14:38 ` Michal Wilczynski
2025-06-28 19:47 ` Michal Wilczynski
@ 2025-06-29 9:23 ` Uwe Kleine-König
2025-07-01 8:52 ` Michal Wilczynski
1 sibling, 1 reply; 29+ messages in thread
From: Uwe Kleine-König @ 2025-06-29 9:23 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]
Hello Michal,
On Sat, Jun 28, 2025 at 04:38:15PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:10, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
> >> +/// From C: `#define WFHWSIZE 20`
> >> +pub const WFHW_MAX_SIZE: usize = 20;
> >
> > Can we somehow enforce that this doesn't diverge if the C define is
> > increased?
>
> You are absolutely right. The hardcoded value is a maintenance risk. The
> #define is in core.c, so bindgen cannot see it.
>
> I can address this by submitting a patch to move the #define WFHWSIZE to
> include/linux/pwm.h. This will make it part of the public API, allow
> bindgen to generate a binding for it, and ensure the Rust code can never
> diverge. Is this fine ?
I wonder if that is the opportunity to create a file
include/linux/pwm-provider.h. In that file we could collect all the bits
that are only relevant for drivers (pwm_ops, pwm_chip, pwmchip_parent,
pwmchip_alloc ...). (Some inline functions depend on some of these, so
some might have to stay in pwm.h)
I can address that in parallel, don't add this quest to your series. So
yes, move WFHWSIZE to include/linux/pwm.h (and rename it to PWM_WFHWSIZE
to not pollute the global namespace).
> > Please don't expose these non-atomic callbacks. pwm_disable() would be
> > fine.
> >
> > Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
> > exposed to/by Rust.
>
>
> OK, I'll remove all the setters from the State, while will keep the
> getters, as they would be useful in apply callbacks.
How so? They might be useful for consumers, but my preferred idiom for
them is that they know at each point in time what they want completely
and don't make that dependant on the previous setting.
> Will implement additional functions for Device i.e set_waveform,
> round_waveform and get_waveform, and the new enum to expose the result
> of the round_waveform more idiomatically.
>
> /// Describes the outcome of a `round_waveform` operation.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub enum RoundingOutcome {
> /// The requested waveform was achievable exactly or by rounding values down.
> ExactOrRoundedDown,
>
> /// The requested waveform could only be achieved by rounding up.
> RoundedUp,
> }
Sounds good. Hoever I have some doubts about the C semantic here, too.
Is it really helpful to provide that info? A user of that return value
has to check anyhow which parameter got rounded up. If you have an
opinion here, please share.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures
2025-06-29 9:23 ` Uwe Kleine-König
@ 2025-07-01 8:52 ` Michal Wilczynski
0 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-07-01 8:52 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/29/25 11:23, Uwe Kleine-König wrote:
> Hello Michal,
>
> On Sat, Jun 28, 2025 at 04:38:15PM +0200, Michal Wilczynski wrote:
>> On 6/27/25 17:10, Uwe Kleine-König wrote:
>>> On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
>>>> +/// From C: `#define WFHWSIZE 20`
>>>> +pub const WFHW_MAX_SIZE: usize = 20;
>>>
>>> Can we somehow enforce that this doesn't diverge if the C define is
>>> increased?
>>
>> You are absolutely right. The hardcoded value is a maintenance risk. The
>> #define is in core.c, so bindgen cannot see it.
>>
>> I can address this by submitting a patch to move the #define WFHWSIZE to
>> include/linux/pwm.h. This will make it part of the public API, allow
>> bindgen to generate a binding for it, and ensure the Rust code can never
>> diverge. Is this fine ?
>
> I wonder if that is the opportunity to create a file
> include/linux/pwm-provider.h. In that file we could collect all the bits
> that are only relevant for drivers (pwm_ops, pwm_chip, pwmchip_parent,
> pwmchip_alloc ...). (Some inline functions depend on some of these, so
> some might have to stay in pwm.h)
>
> I can address that in parallel, don't add this quest to your series. So
> yes, move WFHWSIZE to include/linux/pwm.h (and rename it to PWM_WFHWSIZE
> to not pollute the global namespace).
>
>>> Please don't expose these non-atomic callbacks. pwm_disable() would be
>>> fine.
>>>
>>> Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
>>> exposed to/by Rust.
>>
>>
>> OK, I'll remove all the setters from the State, while will keep the
>> getters, as they would be useful in apply callbacks.
>
> How so? They might be useful for consumers, but my preferred idiom for
> them is that they know at each point in time what they want completely
> and don't make that dependant on the previou setting.
Oh, this is not just to check the previous state, let me bring my
implementation of apply from the v1 of the series:
impl pwm::PwmOps for Th1520PwmChipData {
// This driver implements get_state
fn apply(
pwm_chip_ref: &mut pwm::Chip,
pwm_dev: &mut pwm::Device,
target_state: &pwm::State,
) -> Result {
let data: &Th1520PwmChipData = pwm_chip_ref.get_drvdata().ok_or(EINVAL)?;
let hwpwm = pwm_dev.hwpwm();
if !target_state.enabled() {
if pwm_dev.state().enabled() {
data._disable(hwpwm)?;
}
return Ok(());
}
// Configure period, duty, and polarity.
// This function also latches period/duty with CFG_UPDATE.
// It returns the control value that was written with CFG_UPDATE set.
let ctrl_val_after_config = data._config(
hwpwm,
target_state.duty_cycle(),
target_state.period(),
target_state.polarity(),
)?;
// Enable by setting START bit if it wasn't enabled before this apply call
if !pwm_dev.state().enabled() {
data._enable(hwpwm, ctrl_val_after_config)?;
}
Ok(())
}
}
So the target state values are also accessed by those getters, not just
previous state.
>
>> Will implement additional functions for Device i.e set_waveform,
>> round_waveform and get_waveform, and the new enum to expose the result
>> of the round_waveform more idiomatically.
>>
>> /// Describes the outcome of a `round_waveform` operation.
>> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> pub enum RoundingOutcome {
>> /// The requested waveform was achievable exactly or by rounding values down.
>> ExactOrRoundedDown,
>>
>> /// The requested waveform could only be achieved by rounding up.
>> RoundedUp,
>> }
>
> Sounds good. Hoever I have some doubts about the C semantic here, too.
> Is it really helpful to provide that info? A user of that return value
> has to check anyhow which parameter got rounded up. If you have an
> opinion here, please share.
FWIW; In my opinion, it is helpful.
The 1 (rounded up) vs. 0 (rounded down/exact) return value provides a
simple summary flag for the most common validation case: ensuring a
strict requirement, like a minimum frequency, is not violated by
rounding.
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 2/9] rust: pwm: Add core 'Device' and 'Chip' object wrappers
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
2025-06-23 18:08 ` [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-27 12:12 ` Danilo Krummrich
2025-06-23 18:08 ` [PATCH v5 3/9] rust: pwm: Add driver operations trait and registration support Michal Wilczynski
` (7 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Building on the basic data types, this commit introduces the central
object abstractions for the PWM subsystem: Device and Chip. It also
includes the core trait implementations that make the Chip wrapper a
complete, safe, and managed object.
The main components of this change are:
- Device and Chip Structs: These structs wrap the underlying struct
pwm_device and struct pwm_chip C objects, providing safe, idiomatic
methods to access their fields.
- Core Trait Implementations for Chip:
- AlwaysRefCounted: Links the Chip's lifetime to its embedded
struct device reference counter. This enables automatic lifetime
management via ARef.
- Send and Sync: Marks the Chip wrapper as safe for use across
threads. This is sound because the C core handles all necessary
locking for the underlying object's state.
These wrappers and traits form a robust foundation for building PWM
drivers in Rust.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
rust/kernel/pwm.rs | 216 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 214 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
index ed681b228c414e7ae8bf80ca649ad497c9dc4ec3..3865b43ec47df6cb0c09bc74a228535512b6b1a8 100644
--- a/rust/kernel/pwm.rs
+++ b/rust/kernel/pwm.rs
@@ -8,10 +8,12 @@
use crate::{
bindings,
+ device,
+ error,
prelude::*,
- types::Opaque,
+ types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
};
-use core::convert::TryFrom;
+use core::{convert::TryFrom, ptr::NonNull};
/// Maximum size for the hardware-specific waveform representation buffer.
///
@@ -196,3 +198,213 @@ pub fn set_usage_power(&mut self, usage_power: bool) {
self.0.usage_power = usage_power;
}
}
+
+/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::pwm_device>);
+
+impl Device {
+ /// Creates a reference to a [`Device`] from a valid C pointer.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+ /// returned [`Device`] reference.
+ pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::pwm_device) -> &'a Self {
+ // SAFETY: The safety requirements guarantee the validity of the dereference, while the
+ // `Device` type being transparent makes the cast ok.
+ unsafe { &*ptr.cast::<Self>() }
+ }
+
+ /// Returns a raw pointer to the underlying `pwm_device`.
+ fn as_raw(&self) -> *mut bindings::pwm_device {
+ self.0.get()
+ }
+
+ /// Gets the hardware PWM index for this device within its chip.
+ pub fn hwpwm(&self) -> u32 {
+ // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+ unsafe { (*self.as_raw()).hwpwm }
+ }
+
+ /// Gets a reference to the parent `Chip` that this device belongs to.
+ pub fn chip(&self) -> &Chip {
+ // SAFETY: `self.as_raw()` provides a valid pointer. (*self.as_raw()).chip
+ // is assumed to be a valid pointer to `pwm_chip` managed by the kernel.
+ // Chip::as_ref's safety conditions must be met.
+ unsafe { Chip::as_ref((*self.as_raw()).chip) }
+ }
+
+ /// Gets the label for this PWM device, if any.
+ pub fn label(&self) -> Option<&CStr> {
+ // SAFETY: self.as_raw() provides a valid pointer.
+ let label_ptr = unsafe { (*self.as_raw()).label };
+ if label_ptr.is_null() {
+ None
+ } else {
+ // SAFETY: label_ptr is non-null and points to a C string
+ // managed by the kernel, valid for the lifetime of the PWM device.
+ Some(unsafe { CStr::from_char_ptr(label_ptr) })
+ }
+ }
+
+ /// Gets a copy of the board-dependent arguments for this PWM device.
+ pub fn args(&self) -> Args {
+ // SAFETY: self.as_raw() gives a valid pointer to `pwm_device`.
+ // The `args` field is a valid `pwm_args` struct embedded within `pwm_device`.
+ // `Args::from_c_ptr`'s safety conditions are met by providing this pointer.
+ unsafe { Args::from_c_ptr(&(*self.as_raw()).args) }
+ }
+
+ /// Gets a copy of the current state of this PWM device.
+ pub fn state(&self) -> State {
+ // SAFETY: `self.as_raw()` gives a valid pointer. `(*self.as_raw()).state`
+ // is a valid `pwm_state` struct. `State::from_c` copies this data.
+ State::from_c(unsafe { (*self.as_raw()).state })
+ }
+
+ /// Returns `true` if the PWM signal is currently enabled based on its state.
+ pub fn is_enabled(&self) -> bool {
+ self.state().enabled()
+ }
+}
+
+/// Wrapper for a PWM chip/controller ([`struct pwm_chip`](srctree/include/linux/pwm.h)).
+#[repr(transparent)]
+pub struct Chip(Opaque<bindings::pwm_chip>);
+
+impl Chip {
+ /// Creates a reference to a [`Chip`] from a valid pointer.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+ /// returned [`Chip`] reference.
+ pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::pwm_chip) -> &'a Self {
+ // SAFETY: The safety requirements guarantee the validity of the dereference, while the
+ // `Chip` type being transparent makes the cast ok.
+ unsafe { &*ptr.cast::<Self>() }
+ }
+
+ /// Returns a raw pointer to the underlying `pwm_chip`.
+ pub(crate) fn as_raw(&self) -> *mut bindings::pwm_chip {
+ self.0.get()
+ }
+
+ /// Gets the number of PWM channels (hardware PWMs) on this chip.
+ pub fn npwm(&self) -> u32 {
+ // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+ unsafe { (*self.as_raw()).npwm }
+ }
+
+ /// Returns `true` if the chip supports atomic operations for configuration.
+ pub fn is_atomic(&self) -> bool {
+ // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+ unsafe { (*self.as_raw()).atomic }
+ }
+
+ /// Returns a reference to the embedded `struct device` abstraction.
+ pub fn device(&self) -> &device::Device {
+ // SAFETY: `self.as_raw()` provides a valid pointer to `bindings::pwm_chip`.
+ // The `dev` field is an instance of `bindings::device` embedded within `pwm_chip`.
+ // Taking a pointer to this embedded field is valid.
+ // `device::Device` is `#[repr(transparent)]`.
+ // The lifetime of the returned reference is tied to `self`.
+ let dev_field_ptr = unsafe { core::ptr::addr_of!((*self.as_raw()).dev) };
+ // SAFETY: `dev_field_ptr` is a valid pointer to `bindings::device`.
+ // Casting and dereferencing is safe due to `repr(transparent)` and lifetime.
+ unsafe { &*(dev_field_ptr.cast::<device::Device>()) }
+ }
+
+ /// Returns a reference to the parent device of this PWM chip's device.
+ pub fn parent_device(&self) -> Option<&device::Device> {
+ self.device().parent()
+ }
+
+ /// Gets the *typed* driver-specific data associated with this chip's embedded device.
+ pub fn drvdata<T: 'static>(&self) -> &T {
+ // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
+ // `bindings::pwmchip_get_drvdata` is the C function to retrieve driver data.
+ let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_raw()) };
+
+ // SAFETY: The only way to create a chip is through Chip::new, which initializes
+ // this pointer.
+ unsafe { &*ptr.cast::<T>() }
+ }
+
+ /// Sets the *typed* driver-specific data associated with this chip's embedded device.
+ pub fn set_drvdata<T: 'static + ForeignOwnable>(&self, data: T) {
+ // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
+ // `bindings::pwmchip_set_drvdata` is the C function to set driver data.
+ // `data.into_foreign()` provides a valid `*mut c_void`.
+ unsafe { bindings::pwmchip_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
+ }
+
+ /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
+ ///
+ /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
+ /// on its embedded `struct device`.
+ pub fn new<T: 'static + ForeignOwnable>(
+ parent_dev: &device::Device,
+ npwm: u32,
+ sizeof_priv: usize,
+ drvdata: T,
+ ) -> Result<ARef<Self>> {
+ // SAFETY: `parent_device_for_dev_field.as_raw()` is valid.
+ // `bindings::pwmchip_alloc` returns a valid `*mut bindings::pwm_chip` (refcount 1)
+ // or an ERR_PTR.
+ let c_chip_ptr_raw =
+ unsafe { bindings::pwmchip_alloc(parent_dev.as_raw(), npwm, sizeof_priv) };
+
+ let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
+
+ // Cast the `*mut bindings::pwm_chip` to `*mut Chip`. This is valid because
+ // `Chip` is `repr(transparent)` over `Opaque<bindings::pwm_chip>`, and
+ // `Opaque<T>` is `repr(transparent)` over `T`.
+ let chip_ptr_as_self = c_chip_ptr.cast::<Self>();
+
+ // SAFETY: The pointer is valid, so we can create a temporary ref to set data.
+ let chip_ref = unsafe { &*chip_ptr_as_self };
+ chip_ref.set_drvdata(drvdata);
+
+ // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
+ // `bindings::pwm_chip`) whose embedded device has refcount 1.
+ // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
+ Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
+ }
+}
+
+// SAFETY: Implements refcounting for `Chip` using the embedded `struct device`.
+unsafe impl AlwaysRefCounted for Chip {
+ #[inline]
+ fn inc_ref(&self) {
+ // SAFETY: `self.0.get()` points to a valid `pwm_chip` because `self` exists.
+ // The embedded `dev` is valid. `get_device` increments its refcount.
+ unsafe {
+ bindings::get_device(core::ptr::addr_of_mut!((*self.0.get()).dev));
+ }
+ }
+
+ #[inline]
+ unsafe fn dec_ref(obj: NonNull<Chip>) {
+ let c_chip_ptr = obj.cast::<bindings::pwm_chip>().as_ptr();
+
+ // SAFETY: `obj` is a valid pointer to a `Chip` (and thus `bindings::pwm_chip`)
+ // with a non-zero refcount. `put_device` handles decrement and final release.
+ unsafe {
+ bindings::put_device(core::ptr::addr_of_mut!((*c_chip_ptr).dev));
+ }
+ }
+}
+
+// SAFETY: `Chip` is a wrapper around `*mut bindings::pwm_chip`. The underlying C
+// structure's state is managed and synchronized by the kernel's device model
+// and PWM core locking mechanisms. Therefore, it is safe to move the `Chip`
+// wrapper (and the pointer it contains) across threads.
+unsafe impl Send for Chip {}
+
+// SAFETY: It is safe for multiple threads to have shared access (`&Chip`) because
+// the `Chip` data is immutable from the Rust side without holding the appropriate
+// kernel locks, which the C core is responsible for. Any interior mutability is
+// handled and synchronized by the C kernel code.
+unsafe impl Sync for Chip {}
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 2/9] rust: pwm: Add core 'Device' and 'Chip' object wrappers
2025-06-23 18:08 ` [PATCH v5 2/9] rust: pwm: Add core 'Device' and 'Chip' object wrappers Michal Wilczynski
@ 2025-06-27 12:12 ` Danilo Krummrich
2025-06-28 14:59 ` Michal Wilczynski
0 siblings, 1 reply; 29+ messages in thread
From: Danilo Krummrich @ 2025-06-27 12:12 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On Mon, Jun 23, 2025 at 08:08:50PM +0200, Michal Wilczynski wrote:
> + /// Gets the *typed* driver-specific data associated with this chip's embedded device.
> + pub fn drvdata<T: 'static>(&self) -> &T {
> + // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
> + // `bindings::pwmchip_get_drvdata` is the C function to retrieve driver data.
> + let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_raw()) };
> +
> + // SAFETY: The only way to create a chip is through Chip::new, which initializes
> + // this pointer.
> + unsafe { &*ptr.cast::<T>() }
> + }
> +
> + /// Sets the *typed* driver-specific data associated with this chip's embedded device.
> + pub fn set_drvdata<T: 'static + ForeignOwnable>(&self, data: T) {
> + // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
> + // `bindings::pwmchip_set_drvdata` is the C function to set driver data.
> + // `data.into_foreign()` provides a valid `*mut c_void`.
> + unsafe { bindings::pwmchip_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> + }
I think this is unsound, e.g. what happens if someone calls set_drvdata() twice?
Then you leak the ForeignOwnable from the first call.
Anyways, this does not need to be public, you should just call
bindings::pwmchip_set_drvdata() once in Self::new().
Please also see [1], where I introduce generic accessors for drvdata for Device.
[1] https://lore.kernel.org/lkml/20250621195118.124245-3-dakr@kernel.org/
> + /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
> + ///
> + /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
> + /// on its embedded `struct device`.
> + pub fn new<T: 'static + ForeignOwnable>(
> + parent_dev: &device::Device,
> + npwm: u32,
> + sizeof_priv: usize,
> + drvdata: T,
> + ) -> Result<ARef<Self>> {
> + // SAFETY: `parent_device_for_dev_field.as_raw()` is valid.
> + // `bindings::pwmchip_alloc` returns a valid `*mut bindings::pwm_chip` (refcount 1)
> + // or an ERR_PTR.
> + let c_chip_ptr_raw =
> + unsafe { bindings::pwmchip_alloc(parent_dev.as_raw(), npwm, sizeof_priv) };
> +
> + let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
> +
> + // Cast the `*mut bindings::pwm_chip` to `*mut Chip`. This is valid because
> + // `Chip` is `repr(transparent)` over `Opaque<bindings::pwm_chip>`, and
> + // `Opaque<T>` is `repr(transparent)` over `T`.
> + let chip_ptr_as_self = c_chip_ptr.cast::<Self>();
> +
> + // SAFETY: The pointer is valid, so we can create a temporary ref to set data.
> + let chip_ref = unsafe { &*chip_ptr_as_self };
> + chip_ref.set_drvdata(drvdata);
> +
> + // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
> + // `bindings::pwm_chip`) whose embedded device has refcount 1.
> + // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
> + Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
> + }
> +}
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 2/9] rust: pwm: Add core 'Device' and 'Chip' object wrappers
2025-06-27 12:12 ` Danilo Krummrich
@ 2025-06-28 14:59 ` Michal Wilczynski
0 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-28 14:59 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/27/25 14:12, Danilo Krummrich wrote:
> On Mon, Jun 23, 2025 at 08:08:50PM +0200, Michal Wilczynski wrote:
>> + /// Gets the *typed* driver-specific data associated with this chip's embedded device.
>> + pub fn drvdata<T: 'static>(&self) -> &T {
>> + // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
>> + // `bindings::pwmchip_get_drvdata` is the C function to retrieve driver data.
>> + let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_raw()) };
>> +
>> + // SAFETY: The only way to create a chip is through Chip::new, which initializes
>> + // this pointer.
>> + unsafe { &*ptr.cast::<T>() }
>> + }
>> +
>> + /// Sets the *typed* driver-specific data associated with this chip's embedded device.
>> + pub fn set_drvdata<T: 'static + ForeignOwnable>(&self, data: T) {
>> + // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
>> + // `bindings::pwmchip_set_drvdata` is the C function to set driver data.
>> + // `data.into_foreign()` provides a valid `*mut c_void`.
>> + unsafe { bindings::pwmchip_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
>> + }
>
> I think this is unsound, e.g. what happens if someone calls set_drvdata() twice?
> Then you leak the ForeignOwnable from the first call.
>
> Anyways, this does not need to be public, you should just call
> bindings::pwmchip_set_drvdata() once in Self::new().
>
> Please also see [1], where I introduce generic accessors for drvdata for Device.
Thanks, it would be a great idea to update the code after below patchset
is merged.
>
> [1] https://lore.kernel.org/lkml/20250621195118.124245-3-dakr@kernel.org/
>
>> + /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
>> + ///
>> + /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
>> + /// on its embedded `struct device`.
>> + pub fn new<T: 'static + ForeignOwnable>(
>> + parent_dev: &device::Device,
>> + npwm: u32,
>> + sizeof_priv: usize,
>> + drvdata: T,
>> + ) -> Result<ARef<Self>> {
>> + // SAFETY: `parent_device_for_dev_field.as_raw()` is valid.
>> + // `bindings::pwmchip_alloc` returns a valid `*mut bindings::pwm_chip` (refcount 1)
>> + // or an ERR_PTR.
>> + let c_chip_ptr_raw =
>> + unsafe { bindings::pwmchip_alloc(parent_dev.as_raw(), npwm, sizeof_priv) };
>> +
>> + let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
>> +
>> + // Cast the `*mut bindings::pwm_chip` to `*mut Chip`. This is valid because
>> + // `Chip` is `repr(transparent)` over `Opaque<bindings::pwm_chip>`, and
>> + // `Opaque<T>` is `repr(transparent)` over `T`.
>> + let chip_ptr_as_self = c_chip_ptr.cast::<Self>();
>> +
>> + // SAFETY: The pointer is valid, so we can create a temporary ref to set data.
>> + let chip_ref = unsafe { &*chip_ptr_as_self };
>> + chip_ref.set_drvdata(drvdata);
>> +
>> + // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
>> + // `bindings::pwm_chip`) whose embedded device has refcount 1.
>> + // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
>> + Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
>> + }
>> +}
>
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 3/9] rust: pwm: Add driver operations trait and registration support
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
2025-06-23 18:08 ` [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures Michal Wilczynski
2025-06-23 18:08 ` [PATCH v5 2/9] rust: pwm: Add core 'Device' and 'Chip' object wrappers Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-27 18:51 ` Danilo Krummrich
2025-06-23 18:08 ` [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC Michal Wilczynski
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Complete the PWM abstraction layer by adding the final components
required to implement and register a driver.
The main additions are:
- PwmOps Trait: An interface that drivers can implement to provide
their hardware specific logic. It mirrors the C pwm_ops interface,
providing hooks for standard PWM operations like apply, request, and
waveform handling.
- FFI VTable and Adapter: The Adapter struct, PwmOpsVTable wrapper, and
create_pwm_ops function are introduced. This scaffolding handles the
unsafe FFI translation, bridging the gap between the idiomatic PwmOps
trait and the C kernel's function-pointer-based vtable.
- Registration Guard: A final RAII guard that uses the vtable to safely
register a Chip with the PWM core via pwmchip_add. Its Drop
implementation guarantees that pwmchip_remove is always called,
preventing resource leaks.
With this patch, the PWM abstraction layer is now complete and ready to
be used for writing PWM chip drivers in Rust.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
rust/kernel/pwm.rs | 486 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 483 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
index 3865b43ec47df6cb0c09bc74a228535512b6b1a8..25bc07a3df1d43467a3a6ec8f2362ae8f770360a 100644
--- a/rust/kernel/pwm.rs
+++ b/rust/kernel/pwm.rs
@@ -8,12 +8,13 @@
use crate::{
bindings,
- device,
- error,
+ device::{self, Bound},
+ devres::Devres,
+ error::{self, to_result},
prelude::*,
types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
};
-use core::{convert::TryFrom, ptr::NonNull};
+use core::{convert::TryFrom, marker::PhantomData, ptr::NonNull};
/// Maximum size for the hardware-specific waveform representation buffer.
///
@@ -408,3 +409,482 @@ unsafe impl Send for Chip {}
// kernel locks, which the C core is responsible for. Any interior mutability is
// handled and synchronized by the C kernel code.
unsafe impl Sync for Chip {}
+
+/// A resource guard that ensures `pwmchip_remove` is called on drop.
+///
+/// This struct is intended to be managed by the `devres` framework by transferring its ownership
+/// via [`Devres::new_foreign_owned`]. This ties the lifetime of the PWM chip registration
+/// to the lifetime of the underlying device.
+pub struct Registration {
+ chip: ARef<Chip>,
+}
+
+impl Registration {
+ /// Registers a PWM chip with the PWM subsystem.
+ ///
+ /// Transfers its ownership to the `devres` framework, which ties its lifetime
+ /// to the parent device.
+ /// On unbind of the parent device, the `devres` entry will be dropped, automatically
+ /// calling `pwmchip_remove`. This function should be called from the driver's `probe`.
+ pub fn new_foreign_owned(
+ dev: &device::Device<Bound>,
+ chip: ARef<Chip>,
+ ops_vtable: &'static PwmOpsVTable,
+ ) -> Result {
+ let c_chip_ptr = chip.as_raw();
+
+ // SAFETY: `c_chip_ptr` is valid because the `ARef<Chip>` that owns it exists.
+ // The vtable pointer is also valid. This sets the `.ops` field on the C struct.
+ unsafe {
+ (*c_chip_ptr).ops = ops_vtable.as_raw();
+ }
+
+ // SAFETY: `c_chip_ptr` points to a valid chip with its ops initialized.
+ // `__pwmchip_add` is the C function to register the chip with the PWM core.
+ unsafe {
+ to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
+ }
+
+ let registration = Registration { chip };
+
+ Devres::new_foreign_owned(dev, registration, GFP_KERNEL)?;
+
+ Ok(())
+ }
+}
+
+impl Drop for Registration {
+ fn drop(&mut self) {
+ let chip_raw = self.chip.as_raw();
+
+ // SAFETY: `chip_raw` points to a chip that was successfully registered.
+ // `bindings::pwmchip_remove` is the correct C function to unregister it.
+ // This `drop` implementation is called automatically by `devres` on driver unbind.
+ unsafe {
+ bindings::pwmchip_remove(chip_raw);
+ }
+ }
+}
+
+/// Trait defining the operations for a PWM driver.
+pub trait PwmOps: 'static + Sized {
+ /// The driver-specific hardware representation of a waveform.
+ ///
+ /// This type must be [`Copy`], [`Default`], and fit within [`WFHW_MAX_SIZE`].
+ type WfHw: Copy + Default;
+
+ /// Optional hook to atomically apply a new PWM config.
+ fn apply(
+ _chip: &Chip,
+ _pwm: &Device,
+ _state: &State,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Optional hook for when a PWM device is requested.
+ fn request(_chip: &Chip, _pwm: &Device, _parent_dev: &device::Device<Bound>) -> Result {
+ Ok(())
+ }
+
+ /// Optional hook for when a PWM device is freed.
+ fn free(_chip: &Chip, _pwm: &Device, _parent_dev: &device::Device<Bound>) {}
+
+ /// Optional hook for capturing a PWM signal.
+ fn capture(
+ _chip: &Chip,
+ _pwm: &Device,
+ _result: &mut bindings::pwm_capture,
+ _timeout: usize,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Optional hook to get the current hardware state.
+ fn get_state(
+ _chip: &Chip,
+ _pwm: &Device,
+ _state: &mut State,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Convert a generic waveform to the hardware-specific representation.
+ /// This is typically a pure calculation and does not perform I/O.
+ fn round_waveform_tohw(
+ _chip: &Chip,
+ _pwm: &Device,
+ _wf: &Waveform,
+ ) -> Result<(c_int, Self::WfHw)> {
+ Err(ENOTSUPP)
+ }
+
+ /// Convert a hardware-specific representation back to a generic waveform.
+ /// This is typically a pure calculation and does not perform I/O.
+ fn round_waveform_fromhw(
+ _chip: &Chip,
+ _pwm: &Device,
+ _wfhw: &Self::WfHw,
+ _wf: &mut Waveform,
+ ) -> Result<c_int> {
+ Err(ENOTSUPP)
+ }
+
+ /// Read the current hardware configuration into the hardware-specific representation.
+ fn read_waveform(
+ _chip: &Chip,
+ _pwm: &Device,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result<Self::WfHw> {
+ Err(ENOTSUPP)
+ }
+
+ /// Write a hardware-specific waveform configuration to the hardware.
+ fn write_waveform(
+ _chip: &Chip,
+ _pwm: &Device,
+ _wfhw: &Self::WfHw,
+ _parent_dev: &device::Device<Bound>,
+ ) -> Result {
+ Err(ENOTSUPP)
+ }
+}
+/// Bridges Rust `PwmOps` to the C `pwm_ops` vtable.
+struct Adapter<T: PwmOps> {
+ _p: PhantomData<T>,
+}
+
+impl<T: PwmOps> Adapter<T> {
+ /// # Safety
+ ///
+ /// `wfhw_ptr` must be valid for writes of `size_of::<T::WfHw>()` bytes.
+ unsafe fn serialize_wfhw(wfhw: &T::WfHw, wfhw_ptr: *mut c_void) -> Result {
+ let size = core::mem::size_of::<T::WfHw>();
+ if size > WFHW_MAX_SIZE {
+ return Err(EINVAL);
+ }
+
+ // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
+ unsafe {
+ core::ptr::copy_nonoverlapping(wfhw as *const _ as *const u8, wfhw_ptr.cast(), size);
+ }
+
+ Ok(())
+ }
+
+ /// # Safety
+ ///
+ /// `wfhw_ptr` must be valid for reads of `size_of::<T::WfHw>()` bytes.
+ unsafe fn deserialize_wfhw(wfhw_ptr: *const c_void) -> Result<T::WfHw> {
+ let size = core::mem::size_of::<T::WfHw>();
+ if size > WFHW_MAX_SIZE {
+ return Err(EINVAL);
+ }
+
+ let mut wfhw = T::WfHw::default();
+ // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
+ unsafe {
+ core::ptr::copy_nonoverlapping(wfhw_ptr.cast(), &mut wfhw as *mut _ as *mut u8, size);
+ }
+
+ Ok(wfhw)
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn apply_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ s: *const bindings::pwm_state,
+ ) -> c_int {
+ // SAFETY: This block relies on the function's safety contract: the C caller
+ // provides valid pointers. `Chip::as_ref` and `Device::as_ref` are `unsafe fn`
+ // whose preconditions are met by this contract.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ // SAFETY: The PWM core guarantees callbacks only happen on a live, bound device.
+ let bound_parent =
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+
+ // SAFETY: The state provided by the callback is guaranteed to be valid
+ let state = State::from_c(unsafe { *s });
+ match T::apply(chip, pwm, &state, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn request_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ ) -> c_int {
+ // SAFETY: PWM core guarentees `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ match T::request(chip, pwm, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn free_callback(c: *mut bindings::pwm_chip, p: *mut bindings::pwm_device) {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return;
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ T::free(chip, pwm, bound_parent);
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn capture_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ res: *mut bindings::pwm_capture,
+ timeout: usize,
+ ) -> c_int {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm, result) = unsafe { (Chip::as_ref(c), Device::as_ref(p), &mut *res) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ match T::capture(chip, pwm, result, timeout, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn get_state_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ s: *mut bindings::pwm_state,
+ ) -> c_int {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ let mut rust_state = State::new();
+ match T::get_state(chip, pwm, &mut rust_state, bound_parent) {
+ Ok(()) => {
+ // SAFETY: `s` is guaranteed valid by the C caller.
+ unsafe {
+ *s = rust_state.0;
+ };
+ 0
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn round_waveform_tohw_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ w: *const bindings::pwm_waveform,
+ wh: *mut c_void,
+ ) -> c_int {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm, wf) = unsafe { (Chip::as_ref(c), Device::as_ref(p), Waveform::from(*w)) };
+ match T::round_waveform_tohw(chip, pwm, &wf) {
+ Ok((status, wfhw)) => {
+ // SAFETY: `wh` is valid per this function's safety contract.
+ if unsafe { Self::serialize_wfhw(&wfhw, wh) }.is_err() {
+ return EINVAL.to_errno();
+ }
+ status
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn round_waveform_fromhw_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ wh: *const c_void,
+ w: *mut bindings::pwm_waveform,
+ ) -> c_int {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ // SAFETY: `deserialize_wfhw`'s safety contract is met by this function's contract.
+ let wfhw = match unsafe { Self::deserialize_wfhw(wh) } {
+ Ok(v) => v,
+ Err(e) => return e.to_errno(),
+ };
+
+ let mut rust_wf = Waveform::default();
+ match T::round_waveform_fromhw(chip, pwm, &wfhw, &mut rust_wf) {
+ Ok(ret) => {
+ // SAFETY: `w` is guaranteed valid by the C caller.
+ unsafe {
+ *w = rust_wf.into();
+ };
+ ret
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn read_waveform_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ wh: *mut c_void,
+ ) -> c_int {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ match T::read_waveform(chip, pwm, bound_parent) {
+ // SAFETY: `wh` is valid per this function's safety contract.
+ Ok(wfhw) => match unsafe { Self::serialize_wfhw(&wfhw, wh) } {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ },
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// Pointers from C must be valid.
+ unsafe extern "C" fn write_waveform_callback(
+ c: *mut bindings::pwm_chip,
+ p: *mut bindings::pwm_device,
+ wh: *const c_void,
+ ) -> c_int {
+ // SAFETY: Relies on the function's contract that `c` and `p` are valid pointers.
+ let (chip, pwm) = unsafe { (Chip::as_ref(c), Device::as_ref(p)) };
+ let parent_dev = match chip.parent_device() {
+ Some(dev) => dev,
+ None => {
+ return EINVAL.to_errno();
+ }
+ };
+
+ let bound_parent =
+ // SAFETY: The PWM core guarantees the device is bound during callbacks.
+ unsafe { &*(parent_dev as *const device::Device as *const device::Device<Bound>) };
+ // SAFETY: `wh` is valid per this function's safety contract.
+ let wfhw = match unsafe { Self::deserialize_wfhw(wh) } {
+ Ok(v) => v,
+ Err(e) => return e.to_errno(),
+ };
+ match T::write_waveform(chip, pwm, &wfhw, bound_parent) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+}
+
+/// VTable structure wrapper for PWM operations.
+/// Mirrors [`struct pwm_ops`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct PwmOpsVTable(Opaque<bindings::pwm_ops>);
+
+// SAFETY: PwmOpsVTable is Send. The vtable contains only function pointers
+// and a size, which are simple data types that can be safely moved across
+// threads. The thread-safety of calling these functions is handled by the
+// kernel's locking mechanisms.
+unsafe impl Send for PwmOpsVTable {}
+
+// SAFETY: PwmOpsVTable is Sync. The vtable is immutable after it is created,
+// so it can be safely referenced and accessed concurrently by multiple threads
+// e.g. to read the function pointers.
+unsafe impl Sync for PwmOpsVTable {}
+
+impl PwmOpsVTable {
+ /// Returns a raw pointer to the underlying `pwm_ops` struct.
+ pub(crate) fn as_raw(&self) -> *const bindings::pwm_ops {
+ self.0.get()
+ }
+}
+
+/// Creates a PWM operations vtable for a type `T` that implements `PwmOps`.
+///
+/// This is used to bridge Rust trait implementations to the C `struct pwm_ops`
+/// expected by the kernel.
+pub const fn create_pwm_ops<T: PwmOps>() -> PwmOpsVTable {
+ // SAFETY: `core::mem::zeroed()` is unsafe. For `pwm_ops`, all fields are
+ // `Option<extern "C" fn(...)>` or data, so a zeroed pattern (None/0) is valid initially.
+ let mut ops: bindings::pwm_ops = unsafe { core::mem::zeroed() };
+
+ ops.apply = Some(Adapter::<T>::apply_callback);
+ ops.request = Some(Adapter::<T>::request_callback);
+ ops.free = Some(Adapter::<T>::free_callback);
+ ops.capture = Some(Adapter::<T>::capture_callback);
+ ops.get_state = Some(Adapter::<T>::get_state_callback);
+
+ ops.round_waveform_tohw = Some(Adapter::<T>::round_waveform_tohw_callback);
+ ops.round_waveform_fromhw = Some(Adapter::<T>::round_waveform_fromhw_callback);
+ ops.read_waveform = Some(Adapter::<T>::read_waveform_callback);
+ ops.write_waveform = Some(Adapter::<T>::write_waveform_callback);
+ ops.sizeof_wfhw = core::mem::size_of::<T::WfHw>();
+
+ PwmOpsVTable(Opaque::new(ops))
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 3/9] rust: pwm: Add driver operations trait and registration support
2025-06-23 18:08 ` [PATCH v5 3/9] rust: pwm: Add driver operations trait and registration support Michal Wilczynski
@ 2025-06-27 18:51 ` Danilo Krummrich
0 siblings, 0 replies; 29+ messages in thread
From: Danilo Krummrich @ 2025-06-27 18:51 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/23/25 8:08 PM, Michal Wilczynski wrote:
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> index 3865b43ec47df6cb0c09bc74a228535512b6b1a8..25bc07a3df1d43467a3a6ec8f2362ae8f770360a 100644
> --- a/rust/kernel/pwm.rs
> +++ b/rust/kernel/pwm.rs
> @@ -8,12 +8,13 @@
>
> use crate::{
> bindings,
> - device,
> - error,
> + device::{self, Bound},
> + devres::Devres,
> + error::{self, to_result},
> prelude::*,
> types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
> };
> -use core::{convert::TryFrom, ptr::NonNull};
> +use core::{convert::TryFrom, marker::PhantomData, ptr::NonNull};
>
> /// Maximum size for the hardware-specific waveform representation buffer.
> ///
> @@ -408,3 +409,482 @@ unsafe impl Send for Chip {}
> // kernel locks, which the C core is responsible for. Any interior mutability is
> // handled and synchronized by the C kernel code.
> unsafe impl Sync for Chip {}
> +
> +/// A resource guard that ensures `pwmchip_remove` is called on drop.
> +///
> +/// This struct is intended to be managed by the `devres` framework by transferring its ownership
> +/// via [`Devres::new_foreign_owned`]. This ties the lifetime of the PWM chip registration
> +/// to the lifetime of the underlying device.
> +pub struct Registration {
> + chip: ARef<Chip>,
> +}
> +
> +impl Registration {
> + /// Registers a PWM chip with the PWM subsystem.
> + ///
> + /// Transfers its ownership to the `devres` framework, which ties its lifetime
> + /// to the parent device.
> + /// On unbind of the parent device, the `devres` entry will be dropped, automatically
> + /// calling `pwmchip_remove`. This function should be called from the driver's `probe`.
> + pub fn new_foreign_owned(
> + dev: &device::Device<Bound>,
> + chip: ARef<Chip>,
> + ops_vtable: &'static PwmOpsVTable,
> + ) -> Result {
> + let c_chip_ptr = chip.as_raw();
> +
> + // SAFETY: `c_chip_ptr` is valid because the `ARef<Chip>` that owns it exists.
> + // The vtable pointer is also valid. This sets the `.ops` field on the C struct.
> + unsafe {
> + (*c_chip_ptr).ops = ops_vtable.as_raw();
> + }
> +
> + // SAFETY: `c_chip_ptr` points to a valid chip with its ops initialized.
> + // `__pwmchip_add` is the C function to register the chip with the PWM core.
> + unsafe {
> + to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
> + }
> +
> + let registration = Registration { chip };
> +
> + Devres::new_foreign_owned(dev, registration, GFP_KERNEL)?;
> +
> + Ok(())
This can just be:
Devres::new_foreign_owned(dev, registration, GFP_KERNEL)
I.e. no need for the `Ok(())` below.
With that,
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
for the Registration bits.
@Uwe: Just a head-up if you plan to pick this up for the upcoming merge window,
Devres::new_foreign_owned() will be replaced with devres::register() --
semantics and arguments do not change.
> + }
> +}
> +
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + let chip_raw = self.chip.as_raw();
> +
> + // SAFETY: `chip_raw` points to a chip that was successfully registered.
> + // `bindings::pwmchip_remove` is the correct C function to unregister it.
> + // This `drop` implementation is called automatically by `devres` on driver unbind.
> + unsafe {
> + bindings::pwmchip_remove(chip_raw);
> + }
NIT: You can write this in one line as:
unsafe { bindings::pwmchip_remove(chip_raw) };
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (2 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 3/9] rust: pwm: Add driver operations trait and registration support Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-27 15:28 ` Uwe Kleine-König
2025-06-23 18:08 ` [PATCH v5 5/9] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED Michal Wilczynski
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
utilizing the safe PWM abstractions from the preceding commit.
The driver implements the pwm::PwmOps trait using the modern waveform
API (round_waveform_tohw, write_waveform, etc.) to support configuration
of period, duty cycle, and polarity for the TH1520's PWM channels.
Resource management is handled using idiomatic Rust patterns. The PWM
chip object is allocated via pwm::Chip::new and its registration with
the PWM core is managed by the pwm::Registration RAII guard. This
ensures pwmchip_remove is always called when the driver unbinds,
preventing resource leaks. Device managed resources are used for the
MMIO region, and the clock lifecycle is correctly managed in the
driver's private data Drop implementation.
The driver's core logic is written entirely in safe Rust, with no unsafe
blocks.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
MAINTAINERS | 1 +
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm_th1520.rs | 318 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 330 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21402,6 +21402,7 @@ F: drivers/mailbox/mailbox-th1520.c
F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
F: drivers/pinctrl/pinctrl-th1520.c
F: drivers/pmdomain/thead/
+F: drivers/pwm/pwm_th1520.rs
F: drivers/reset/reset-th1520.c
F: include/dt-bindings/clock/thead,th1520-clk-ap.h
F: include/dt-bindings/power/thead,th1520-power.h
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -719,6 +719,16 @@ config PWM_TEGRA
To compile this driver as a module, choose M here: the module
will be called pwm-tegra.
+config PWM_TH1520
+ tristate "TH1520 PWM support"
+ depends on RUST_PWM_ABSTRACTIONS
+ help
+ This option enables the driver for the PWM controller found on the
+ T-HEAD TH1520 SoC.
+
+ To compile this driver as a module, choose M here; the module
+ will be called pwm-th1520. If you are unsure, say N.
+
config PWM_TIECAP
tristate "ECAP PWM support"
depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
+obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL) += pwm-twl.o
diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
new file mode 100644
index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
--- /dev/null
+++ b/drivers/pwm/pwm_th1520.rs
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! Rust T-HEAD TH1520 PWM driver
+//!
+//! Limitations:
+//! - The period and duty cycle are controlled by 32-bit hardware registers,
+//! limiting the maximum resolution.
+//! - The driver supports continuous output mode only; one-shot mode is not
+//! implemented.
+//! - The controller hardware provides up to 6 PWM channels.
+//!
+
+use core::ops::Deref;
+use kernel::{
+ c_str,
+ clk::Clk,
+ device::{Bound, Core, Device},
+ devres,
+ io::mem::IoMem,
+ of, platform,
+ prelude::*,
+ pwm, time,
+};
+
+const MAX_PWM_NUM: u32 = 6;
+
+// Register offsets
+const fn th1520_pwm_chn_base(n: u32) -> usize {
+ (n * 0x20) as usize
+}
+
+const fn th1520_pwm_ctrl(n: u32) -> usize {
+ th1520_pwm_chn_base(n)
+}
+
+const fn th1520_pwm_per(n: u32) -> usize {
+ th1520_pwm_chn_base(n) + 0x08
+}
+
+const fn th1520_pwm_fp(n: u32) -> usize {
+ th1520_pwm_chn_base(n) + 0x0c
+}
+
+// Control register bits
+const PWM_START: u32 = 1 << 0;
+const PWM_CFG_UPDATE: u32 = 1 << 2;
+const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
+const PWM_FPOUT: u32 = 1 << 8;
+
+const TH1520_PWM_REG_SIZE: usize = 0xB0;
+
+fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
+ const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
+
+ match ns.checked_mul(rate_hz) {
+ Some(product) => product / NSEC_PER_SEC_U64,
+ None => u64::MAX,
+ }
+}
+
+fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
+ const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
+
+ // Round up
+ let Some(numerator) = cycles
+ .checked_mul(NSEC_PER_SEC_U64)
+ .and_then(|p| p.checked_add(rate_hz - 1))
+ else {
+ return u64::MAX;
+ };
+
+ numerator / rate_hz
+}
+
+/// Hardware-specific waveform representation for TH1520.
+#[derive(Copy, Clone, Debug, Default)]
+struct Th1520WfHw {
+ period_cycles: u32,
+ duty_cycles: u32,
+ ctrl_val: u32,
+ enabled: bool,
+}
+
+/// The driver's private data struct. It holds all necessary devres managed resources.
+struct Th1520PwmDriverData {
+ iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
+ clk: Clk,
+}
+
+impl pwm::PwmOps for Th1520PwmDriverData {
+ type WfHw = Th1520WfHw;
+
+ fn round_waveform_tohw(
+ chip: &pwm::Chip,
+ _pwm: &pwm::Device,
+ wf: &pwm::Waveform,
+ ) -> Result<(c_int, Self::WfHw)> {
+ let data: &Self = chip.drvdata();
+
+ if wf.period_length_ns == 0 {
+ return Ok((
+ 0,
+ Th1520WfHw {
+ enabled: false,
+ ..Default::default()
+ },
+ ));
+ }
+
+ let rate_hz = data.clk.rate().as_hz() as u64;
+
+ let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
+ let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
+
+ let mut ctrl_val = PWM_CONTINUOUS_MODE;
+
+ if wf.duty_offset_ns == 0 {
+ ctrl_val |= PWM_FPOUT;
+ } else {
+ duty_cycles = period_cycles - duty_cycles;
+ }
+
+ let wfhw = Th1520WfHw {
+ period_cycles: period_cycles as u32,
+ duty_cycles: duty_cycles as u32,
+ ctrl_val,
+ enabled: true,
+ };
+
+ dev_dbg!(
+ chip.device(),
+ "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
+ wf.period_length_ns,
+ wf.duty_length_ns,
+ wf.duty_offset_ns,
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ wfhw.ctrl_val
+ );
+
+ Ok((0, wfhw))
+ }
+
+ fn round_waveform_fromhw(
+ chip: &pwm::Chip,
+ _pwm: &pwm::Device,
+ wfhw: &Self::WfHw,
+ wf: &mut pwm::Waveform,
+ ) -> Result<c_int> {
+ let data: &Self = chip.drvdata();
+ let rate_hz = data.clk.rate().as_hz() as u64;
+
+ wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
+
+ let duty_cycles = wfhw.duty_cycles as u64;
+
+ if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
+ wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
+ wf.duty_offset_ns = 0;
+ } else {
+ let period_cycles = wfhw.period_cycles as u64;
+ let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
+
+ wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
+ // We can't recover the original non-zero offset, so we just set it
+ // to a representative non-zero value.
+ wf.duty_offset_ns = 1;
+ }
+
+ Ok(0)
+ }
+
+ fn read_waveform(
+ chip: &pwm::Chip,
+ pwm: &pwm::Device,
+ parent_dev: &Device<Bound>,
+ ) -> Result<Self::WfHw> {
+ let data: &Self = chip.drvdata();
+ let hwpwm = pwm.hwpwm();
+ let iomem_accessor = data.iomem.access(parent_dev)?;
+ let iomap = iomem_accessor.deref();
+
+ let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
+ let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
+ let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
+
+ let wfhw = Th1520WfHw {
+ period_cycles,
+ duty_cycles,
+ ctrl_val: ctrl,
+ enabled: duty_cycles != 0,
+ };
+
+ dev_dbg!(
+ chip.device(),
+ "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
+ hwpwm,
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ wfhw.ctrl_val,
+ wfhw.enabled
+ );
+
+ Ok(wfhw)
+ }
+
+ fn write_waveform(
+ chip: &pwm::Chip,
+ pwm: &pwm::Device,
+ wfhw: &Self::WfHw,
+ parent_dev: &Device<Bound>,
+ ) -> Result {
+ let data: &Self = chip.drvdata();
+ let hwpwm = pwm.hwpwm();
+ let iomem_accessor = data.iomem.access(parent_dev)?;
+ let iomap = iomem_accessor.deref();
+ let was_enabled = pwm.state().enabled();
+
+ if !wfhw.enabled {
+ if was_enabled {
+ iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
+ iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
+ iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
+ }
+ return Ok(());
+ }
+
+ iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
+ iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
+ iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
+ iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
+
+ // The `PWM_START` bit must be written in a separate, final transaction, and
+ // only when enabling the channel from a disabled state.
+ if !was_enabled {
+ iomap.try_write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm))?;
+ }
+
+ dev_dbg!(
+ chip.device(),
+ "PWM-{}: Wrote (per: {}, duty: {})",
+ hwpwm,
+ wfhw.period_cycles,
+ wfhw.duty_cycles,
+ );
+
+ Ok(())
+ }
+}
+
+impl Drop for Th1520PwmDriverData {
+ fn drop(&mut self) {
+ self.clk.disable_unprepare();
+ }
+}
+
+static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
+
+struct Th1520PwmPlatformDriver;
+
+kernel::of_device_table!(
+ OF_TABLE,
+ MODULE_OF_TABLE,
+ <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
+ [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
+);
+
+impl platform::Driver for Th1520PwmPlatformDriver {
+ type IdInfo = ();
+ const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+ fn probe(
+ pdev: &platform::Device<Core>,
+ _id_info: Option<&Self::IdInfo>,
+ ) -> Result<Pin<KBox<Self>>> {
+ let dev = pdev.as_ref();
+ let resource = pdev.resource(0).ok_or(ENODEV)?;
+ let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
+ let clk = Clk::get(pdev.as_ref(), None)?;
+
+ clk.prepare_enable()?;
+
+ // TODO: Get exclusive ownership of the clock to prevent rate changes.
+ // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
+ // This should be updated once it is implemented.
+ let rate_hz = clk.rate().as_hz();
+ if rate_hz == 0 {
+ dev_err!(dev, "Clock rate is zero\n");
+ return Err(EINVAL);
+ }
+
+ if rate_hz > time::NSEC_PER_SEC as usize {
+ dev_err!(
+ dev,
+ "Clock rate {} Hz is too high, not supported.\n",
+ rate_hz
+ );
+ return Err(ERANGE);
+ }
+
+ let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
+ let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0, drvdata)?;
+
+ pwm::Registration::new_foreign_owned(dev, chip, &TH1520_PWM_OPS)?;
+
+ Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
+ }
+}
+
+kernel::module_platform_driver! {
+ type: Th1520PwmPlatformDriver,
+ name: "pwm-th1520",
+ authors: ["Michal Wilczynski <m.wilczynski@samsung.com>"],
+ description: "T-HEAD TH1520 PWM driver",
+ license: "GPL v2",
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-23 18:08 ` [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC Michal Wilczynski
@ 2025-06-27 15:28 ` Uwe Kleine-König
2025-06-28 18:14 ` Michal Wilczynski
0 siblings, 1 reply; 29+ messages in thread
From: Uwe Kleine-König @ 2025-06-27 15:28 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 9404 bytes --]
On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
> utilizing the safe PWM abstractions from the preceding commit.
>
> The driver implements the pwm::PwmOps trait using the modern waveform
> API (round_waveform_tohw, write_waveform, etc.) to support configuration
> of period, duty cycle, and polarity for the TH1520's PWM channels.
>
> Resource management is handled using idiomatic Rust patterns. The PWM
> chip object is allocated via pwm::Chip::new and its registration with
> the PWM core is managed by the pwm::Registration RAII guard. This
> ensures pwmchip_remove is always called when the driver unbinds,
> preventing resource leaks. Device managed resources are used for the
> MMIO region, and the clock lifecycle is correctly managed in the
> driver's private data Drop implementation.
>
> The driver's core logic is written entirely in safe Rust, with no unsafe
> blocks.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 1 +
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm_th1520.rs | 318 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 330 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21402,6 +21402,7 @@ F: drivers/mailbox/mailbox-th1520.c
> F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
> F: drivers/pinctrl/pinctrl-th1520.c
> F: drivers/pmdomain/thead/
> +F: drivers/pwm/pwm_th1520.rs
> F: drivers/reset/reset-th1520.c
> F: include/dt-bindings/clock/thead,th1520-clk-ap.h
> F: include/dt-bindings/power/thead,th1520-power.h
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -719,6 +719,16 @@ config PWM_TEGRA
> To compile this driver as a module, choose M here: the module
> will be called pwm-tegra.
>
> +config PWM_TH1520
> + tristate "TH1520 PWM support"
> + depends on RUST_PWM_ABSTRACTIONS
RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
user's POV it shouldn't matter if the driver is written in Rust or not.
> + help
> + This option enables the driver for the PWM controller found on the
> + T-HEAD TH1520 SoC.
> +
> + To compile this driver as a module, choose M here; the module
> + will be called pwm-th1520. If you are unsure, say N.
> +
> config PWM_TIECAP
> tristate "ECAP PWM support"
> depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
> obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> +obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> obj-$(CONFIG_PWM_TWL) += pwm-twl.o
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +//!
> +//! Limitations:
> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
> +//! limiting the maximum resolution.
> +//! - The driver supports continuous output mode only; one-shot mode is not
> +//! implemented.
> +//! - The controller hardware provides up to 6 PWM channels.
Important questions to answer here are:
- How does the hardware behave on disable? (Does it stop immediately
(or at all)? Does it emit a constant output? Which?)
- How does the hardware behave on reconfiguration? (Does it switch
immidiately or does it complete the current period? Can there be
glitches?
> +//!
> +
> +use core::ops::Deref;
> +use kernel::{
> + c_str,
> + clk::Clk,
> + device::{Bound, Core, Device},
> + devres,
> + io::mem::IoMem,
> + of, platform,
> + prelude::*,
> + pwm, time,
> +};
> +
> +const MAX_PWM_NUM: u32 = 6;
> +
> +// Register offsets
> +const fn th1520_pwm_chn_base(n: u32) -> usize {
> + (n * 0x20) as usize
> +}
> +
> +const fn th1520_pwm_ctrl(n: u32) -> usize {
> + th1520_pwm_chn_base(n)
> +}
> +
> +const fn th1520_pwm_per(n: u32) -> usize {
> + th1520_pwm_chn_base(n) + 0x08
> +}
> +
> +const fn th1520_pwm_fp(n: u32) -> usize {
> + th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +// Control register bits
> +const PWM_START: u32 = 1 << 0;
> +const PWM_CFG_UPDATE: u32 = 1 << 2;
> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const PWM_FPOUT: u32 = 1 << 8;
Can you please add a driver specific prefix to these?
> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> +
> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> + const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> + match ns.checked_mul(rate_hz) {
> + Some(product) => product / NSEC_PER_SEC_U64,
> + None => u64::MAX,
> + }
The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
* rate_hz / NSEC_PER_SEC, right?
If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
it would be more prudent do make this:
match ns.checked_mul(rate_hz) {
Some(product) => product,
None => u64::MAX,
} / NSEC_PER_SEC_U64
> +}
> +
> [...]
> +impl pwm::PwmOps for Th1520PwmDriverData {
> + type WfHw = Th1520WfHw;
> +
> + fn round_waveform_tohw(
> + chip: &pwm::Chip,
> + _pwm: &pwm::Device,
> + wf: &pwm::Waveform,
> + ) -> Result<(c_int, Self::WfHw)> {
> + let data: &Self = chip.drvdata();
> +
> + if wf.period_length_ns == 0 {
> + return Ok((
> + 0,
> + Th1520WfHw {
> + enabled: false,
> + ..Default::default()
> + },
> + ));
> + }
> +
> + let rate_hz = data.clk.rate().as_hz() as u64;
> +
> + let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
> + let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
> +
> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
> +
> + if wf.duty_offset_ns == 0 {
> + ctrl_val |= PWM_FPOUT;
> + } else {
> + duty_cycles = period_cycles - duty_cycles;
Huh, this looks wrong. Your hardware only supports the two polarities,
right? Then configure inversed polarity if
wf->duty_length_ns && wf->duty_offset_ns && wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns
(i.e. how the pwm-stm32 driver does it).
> + }
> +
> + let wfhw = Th1520WfHw {
> + period_cycles: period_cycles as u32,
> + duty_cycles: duty_cycles as u32,
> + ctrl_val,
> + enabled: true,
> + };
> +
> + dev_dbg!(
> + chip.device(),
> + "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
Would it be helpful to also emit the clkrate here?
> + wf.period_length_ns,
> + wf.duty_length_ns,
> + wf.duty_offset_ns,
> + wfhw.period_cycles,
> + wfhw.duty_cycles,
> + wfhw.ctrl_val
> + );
> +
> + Ok((0, wfhw))
> + }
> +
> + fn round_waveform_fromhw(
> + chip: &pwm::Chip,
> + _pwm: &pwm::Device,
> + wfhw: &Self::WfHw,
> + wf: &mut pwm::Waveform,
> + ) -> Result<c_int> {
> + let data: &Self = chip.drvdata();
> + let rate_hz = data.clk.rate().as_hz() as u64;
> +
> + wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
> +
> + let duty_cycles = wfhw.duty_cycles as u64;
> +
> + if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
> + wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
> + wf.duty_offset_ns = 0;
> + } else {
> + let period_cycles = wfhw.period_cycles as u64;
> + let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
> +
> + wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> + // We can't recover the original non-zero offset, so we just set it
> + // to a representative non-zero value.
> + wf.duty_offset_ns = 1;
For an inversed polarity signal the duty_offset is polarity - duty_cycle.
> + }
> +
> + Ok(0)
> + }
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-27 15:28 ` Uwe Kleine-König
@ 2025-06-28 18:14 ` Michal Wilczynski
2025-06-29 9:08 ` Uwe Kleine-König
0 siblings, 1 reply; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-28 18:14 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
On 6/27/25 17:28, Uwe Kleine-König wrote:
> On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
>> utilizing the safe PWM abstractions from the preceding commit.
>>
>> The driver implements the pwm::PwmOps trait using the modern waveform
>> API (round_waveform_tohw, write_waveform, etc.) to support configuration
>> of period, duty cycle, and polarity for the TH1520's PWM channels.
>>
>> Resource management is handled using idiomatic Rust patterns. The PWM
>> chip object is allocated via pwm::Chip::new and its registration with
>> the PWM core is managed by the pwm::Registration RAII guard. This
>> ensures pwmchip_remove is always called when the driver unbinds,
>> preventing resource leaks. Device managed resources are used for the
>> MMIO region, and the clock lifecycle is correctly managed in the
>> driver's private data Drop implementation.
>>
>> The driver's core logic is written entirely in safe Rust, with no unsafe
>> blocks.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm_th1520.rs | 318 ++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 330 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a575622454a2ef57ce055c8a8c4765fa4fddc490..879870471e86dcec4a0e8f5c45d2cc3409411fdd 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21402,6 +21402,7 @@ F: drivers/mailbox/mailbox-th1520.c
>> F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>> F: drivers/pinctrl/pinctrl-th1520.c
>> F: drivers/pmdomain/thead/
>> +F: drivers/pwm/pwm_th1520.rs
>> F: drivers/reset/reset-th1520.c
>> F: include/dt-bindings/clock/thead,th1520-clk-ap.h
>> F: include/dt-bindings/power/thead,th1520-power.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -719,6 +719,16 @@ config PWM_TEGRA
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-tegra.
>>
>> +config PWM_TH1520
>> + tristate "TH1520 PWM support"
>> + depends on RUST_PWM_ABSTRACTIONS
>
> RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
> user's POV it shouldn't matter if the driver is written in Rust or not.
You make an excellent point about user experience. My initial thought
was to follow the depends on pattern that I saw in other Rust drivers.
I can see how using select would be cleaner for the end user. My only
hesitation was that it differs from the current convention for Rust
drivers, and I wanted to be careful about changing an established
pattern.
If you are comfortable with setting this direction for the PWM
subsystem, I am happy to make the change to use select
RUST_PWM_ABSTRACTIONS (gated by depends on RUST). Please let me know.
>
>> + help
>> + This option enables the driver for the PWM controller found on the
>> + T-HEAD TH1520 SoC.
>> +
>> + To compile this driver as a module, choose M here; the module
>> + will be called pwm-th1520. If you are unsure, say N.
>> +
>> config PWM_TIECAP
>> tristate "ECAP PWM support"
>> depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 96160f4257fcb0e0951581af0090615c0edf5260..a410747095327a315a6bcd24ae343ce7857fe323 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -66,6 +66,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
>> obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
>> +obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
>> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
>> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
>> obj-$(CONFIG_PWM_TWL) += pwm-twl.o
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..a77c45cef9cf8f02a25db9d42c45cd0df565b0ec
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>> +//!
>> +//! Limitations:
>> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
>> +//! limiting the maximum resolution.
>> +//! - The driver supports continuous output mode only; one-shot mode is not
>> +//! implemented.
>> +//! - The controller hardware provides up to 6 PWM channels.
>
> Important questions to answer here are:
>
> - How does the hardware behave on disable? (Does it stop immediately
> (or at all)? Does it emit a constant output? Which?)
> - How does the hardware behave on reconfiguration? (Does it switch
> immidiately or does it complete the current period? Can there be
> glitches?
Sure, I have investigated and will add comments explaining that
reconfiguration is glitch free and disabling is done by setting the duty
cycle to zero
>
>> +//!
>> +
>> +use core::ops::Deref;
>> +use kernel::{
>> + c_str,
>> + clk::Clk,
>> + device::{Bound, Core, Device},
>> + devres,
>> + io::mem::IoMem,
>> + of, platform,
>> + prelude::*,
>> + pwm, time,
>> +};
>> +
>> +const MAX_PWM_NUM: u32 = 6;
>> +
>> +// Register offsets
>> +const fn th1520_pwm_chn_base(n: u32) -> usize {
>> + (n * 0x20) as usize
>> +}
>> +
>> +const fn th1520_pwm_ctrl(n: u32) -> usize {
>> + th1520_pwm_chn_base(n)
>> +}
>> +
>> +const fn th1520_pwm_per(n: u32) -> usize {
>> + th1520_pwm_chn_base(n) + 0x08
>> +}
>> +
>> +const fn th1520_pwm_fp(n: u32) -> usize {
>> + th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +// Control register bits
>> +const PWM_START: u32 = 1 << 0;
>> +const PWM_CFG_UPDATE: u32 = 1 << 2;
>> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
>> +const PWM_FPOUT: u32 = 1 << 8;
>
> Can you please add a driver specific prefix to these?
OK
>
>> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
>> + const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
>> +
>> + match ns.checked_mul(rate_hz) {
>> + Some(product) => product / NSEC_PER_SEC_U64,
>> + None => u64::MAX,
>> + }
>
> The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
> * rate_hz / NSEC_PER_SEC, right?
>
> If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
> it would be more prudent do make this:
>
> match ns.checked_mul(rate_hz) {
> Some(product) => product,
> None => u64::MAX,
> } / NSEC_PER_SEC_U64
Thank you for the feedback on the calculation. I analyzed the two
approaches and found that on overflow, my version saturates to u64::MAX,
while the suggested version would result in u64::MAX / NSEC_PER_SEC. I
believe my original implementation's saturation behavior is more
predictable. With this in mind, would you be comfortable with me
retaining the original implementation?
>
>> +}
>> +
>> [...]
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> + type WfHw = Th1520WfHw;
>> +
>> + fn round_waveform_tohw(
>> + chip: &pwm::Chip,
>> + _pwm: &pwm::Device,
>> + wf: &pwm::Waveform,
>> + ) -> Result<(c_int, Self::WfHw)> {
>> + let data: &Self = chip.drvdata();
>> +
>> + if wf.period_length_ns == 0 {
>> + return Ok((
>> + 0,
>> + Th1520WfHw {
>> + enabled: false,
>> + ..Default::default()
>> + },
>> + ));
>> + }
>> +
>> + let rate_hz = data.clk.rate().as_hz() as u64;
>> +
>> + let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u32::MAX as u64);
>> + let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u32::MAX as u64);
>> +
>> + let mut ctrl_val = PWM_CONTINUOUS_MODE;
>> +
>> + if wf.duty_offset_ns == 0 {
>> + ctrl_val |= PWM_FPOUT;
>> + } else {
>> + duty_cycles = period_cycles - duty_cycles;
>
> Huh, this looks wrong. Your hardware only supports the two polarities,
> right? Then configure inversed polarity if
>
> wf->duty_length_ns && wf->duty_offset_ns && wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns
>
> (i.e. how the pwm-stm32 driver does it).
OK, will fix
>
>> + }
>> +
>> + let wfhw = Th1520WfHw {
>> + period_cycles: period_cycles as u32,
>> + duty_cycles: duty_cycles as u32,
>> + ctrl_val,
>> + enabled: true,
>> + };
>> +
>> + dev_dbg!(
>> + chip.device(),
>> + "Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
>
> Would it be helpful to also emit the clkrate here?
OK will update
>
>> + wf.period_length_ns,
>> + wf.duty_length_ns,
>> + wf.duty_offset_ns,
>> + wfhw.period_cycles,
>> + wfhw.duty_cycles,
>> + wfhw.ctrl_val
>> + );
>> +
>> + Ok((0, wfhw))
>> + }
>> +
>> + fn round_waveform_fromhw(
>> + chip: &pwm::Chip,
>> + _pwm: &pwm::Device,
>> + wfhw: &Self::WfHw,
>> + wf: &mut pwm::Waveform,
>> + ) -> Result<c_int> {
>> + let data: &Self = chip.drvdata();
>> + let rate_hz = data.clk.rate().as_hz() as u64;
>> +
>> + wf.period_length_ns = cycles_to_ns(wfhw.period_cycles as u64, rate_hz);
>> +
>> + let duty_cycles = wfhw.duty_cycles as u64;
>> +
>> + if (wfhw.ctrl_val & PWM_FPOUT) != 0 {
>> + wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
>> + wf.duty_offset_ns = 0;
>> + } else {
>> + let period_cycles = wfhw.period_cycles as u64;
>> + let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
>> +
>> + wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
>> + // We can't recover the original non-zero offset, so we just set it
>> + // to a representative non-zero value.
>> + wf.duty_offset_ns = 1;
>
> For an inversed polarity signal the duty_offset is polarity - duty_cycle.
I believe there was a typo in your suggestion and you meant period
instead of polarity. Based on that, my understanding is that for an
inverted signal, the generic pwm_waveform struct expects duty_offset_ns
to represent the duration of the initial low time, while duty_length_ns
represents the high time.
so the code would look like this:
// For an inverted signal, `duty_length_ns` is the high time (period - low_time).
wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
// The offset is the initial low time, which is what the hardware register provides.
wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
>
>> + }
>> +
>> + Ok(0)
>> + }
>
> Best regards
> Uwe
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC
2025-06-28 18:14 ` Michal Wilczynski
@ 2025-06-29 9:08 ` Uwe Kleine-König
0 siblings, 0 replies; 29+ messages in thread
From: Uwe Kleine-König @ 2025-06-29 9:08 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Marek Szyprowski, Benno Lossin,
Michael Turquette, Stephen Boyd, linux-kernel, linux-pwm,
rust-for-linux, linux-riscv, devicetree, linux-clk
[-- Attachment #1: Type: text/plain, Size: 4313 bytes --]
Hello Michal,
On Sat, Jun 28, 2025 at 08:14:59PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:28, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -719,6 +719,16 @@ config PWM_TEGRA
> >> To compile this driver as a module, choose M here: the module
> >> will be called pwm-tegra.
> >>
> >> +config PWM_TH1520
> >> + tristate "TH1520 PWM support"
> >> + depends on RUST_PWM_ABSTRACTIONS
> >
> > RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
> > user's POV it shouldn't matter if the driver is written in Rust or not.
>
> You make an excellent point about user experience. My initial thought
> was to follow the depends on pattern that I saw in other Rust drivers.
>
> I can see how using select would be cleaner for the end user. My only
> hesitation was that it differs from the current convention for Rust
> drivers, and I wanted to be careful about changing an established
> pattern.
>
> If you are comfortable with setting this direction for the PWM
> subsystem, I am happy to make the change to use select
> RUST_PWM_ABSTRACTIONS (gated by depends on RUST). Please let me know.
Sounds good.
> >> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> >> +
> >> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> >> + const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> >> +
> >> + match ns.checked_mul(rate_hz) {
> >> + Some(product) => product / NSEC_PER_SEC_U64,
> >> + None => u64::MAX,
> >> + }
> >
> > The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
> > * rate_hz / NSEC_PER_SEC, right?
> >
> > If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
> > it would be more prudent do make this:
> >
> > match ns.checked_mul(rate_hz) {
> > Some(product) => product,
> > None => u64::MAX,
> > } / NSEC_PER_SEC_U64
>
> Thank you for the feedback on the calculation. I analyzed the two
> approaches and found that on overflow, my version saturates to u64::MAX,
> while the suggested version would result in u64::MAX / NSEC_PER_SEC. I
> believe my original implementation's saturation behavior is more
> predictable. With this in mind, would you be comfortable with me
> retaining the original implementation?
I'm convinced that my alternative is better. Consider the implemented
mapping: Assuming rate_hz = 160000000 you have:
ns | cycles
-------------+---------------------
... |
115292150452 | 18446744072
115292150453 | 18446744072
115292150454 | 18446744072
115292150455 | 18446744072
115292150456 | 18446744072
115292150457 | 18446744073
115292150458 | 18446744073
115292150459 | 18446744073
115292150460 | 18446744073
115292150461 | 18446744073709551615
115292150462 | 18446744073709551615
...
that's strange, isn't it?
> >> + wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> >> + // We can't recover the original non-zero offset, so we just set it
> >> + // to a representative non-zero value.
> >> + wf.duty_offset_ns = 1;
> >
> > For an inversed polarity signal the duty_offset is polarity - duty_cycle.
>
> I believe there was a typo in your suggestion and you meant period
> instead of polarity.
ack.
> Based on that, my understanding is that for an
> inverted signal, the generic pwm_waveform struct expects duty_offset_ns
> to represent the duration of the initial low time, while duty_length_ns
> represents the high time.
right.
> so the code would look like this:
> // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
> wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> // The offset is the initial low time, which is what the hardware register provides.
> wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
Looks correct
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 5/9] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (3 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-23 18:08 ` [PATCH v5 6/9] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller Michal Wilczynski
` (4 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Probing peripherals in the AON and PERI domains, such as the PVT thermal
sensor and the PWM controller, can lead to boot hangs or unresponsive
devices on the LPi4A board. The root cause is that their parent bus
clocks ('CLK_CPU2AON_X2H' and the 'CLK_PERISYS_APB' clocks) are
automatically gated by the kernel's power-saving mechanisms when the bus
is perceived as idle.
Alternative solutions were investigated, including modeling the parent
bus in the Device Tree with 'simple-pm-bus' or refactoring the clock
driver's parentage. The 'simple-pm-bus' approach is not viable due to
the lack of defined bus address ranges in the hardware manual and its
creation of improper dependencies on the 'pm_runtime' API for consumer
drivers.
Therefore, applying the'`CLK_IGNORE_UNUSED' flag directly to the
essential bus clocks is the most direct and targeted fix. This prevents
the kernel from auto-gating these buses and ensures peripherals remain
accessible.
This change fixes the boot hang associated with the PVT sensor and
resolves the functional issues with the PWM controller.
Link: https://lore.kernel.org/all/9e8a12db-236d-474c-b110-b3be96edf057@samsung.com/ [1]
Reviewed-by: Drew Fustini <drew@pdp7.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
drivers/clk/thead/clk-th1520-ap.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index ebfb1d59401d05443716eb0029403b01775e8f73..cf7f6bd428a0faa4611b3fc61edbbc6690e565d9 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -792,11 +792,12 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
0x134, BIT(8), 0);
static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
0x134, BIT(7), 0);
-static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
+static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd,
+ 0x138, BIT(8), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
0x140, BIT(9), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,
- 0x150, BIT(9), 0);
+ 0x150, BIT(9), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_PERISYS_APB2_HCLK, perisys_apb2_hclk, "perisys-apb2-hclk", perisys_ahb_hclk_pd,
0x150, BIT(10), CLK_IGNORE_UNUSED);
static CCU_GATE(CLK_PERISYS_APB3_HCLK, perisys_apb3_hclk, "perisys-apb3-hclk", perisys_ahb_hclk_pd,
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v5 6/9] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (4 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 5/9] clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-23 18:08 ` [PATCH v5 7/9] riscv: dts: thead: Add PWM controller node Michal Wilczynski
` (3 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk, Krzysztof Kozlowski
Add the Device Tree binding documentation for the T-HEAD
TH1520 SoC PWM controller.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
.../devicetree/bindings/pwm/thead,th1520-pwm.yaml | 48 ++++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 49 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..855aec59ac53c430adc849271235686e87b10e6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/thead,th1520-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 PWM controller
+
+maintainers:
+ - Michal Wilczynski <m.wilczynski@samsung.com>
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ const: thead,th1520-pwm
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: SoC PWM clock
+
+ "#pwm-cells":
+ const: 3
+
+required:
+ - compatible
+ - reg
+ - clocks
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ pwm@ffec01c000 {
+ compatible = "thead,th1520-pwm";
+ reg = <0xff 0xec01c000 0x0 0x4000>;
+ clocks = <&clk CLK_PWM>;
+ #pwm-cells = <3>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 879870471e86dcec4a0e8f5c45d2cc3409411fdd..8a55e8d88394a269aa152e80a03c439a36e6062e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21394,6 +21394,7 @@ F: Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
+F: Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
F: Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
F: arch/riscv/boot/dts/thead/
F: drivers/clk/thead/clk-th1520-ap.c
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v5 7/9] riscv: dts: thead: Add PWM controller node
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (5 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 6/9] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-23 18:08 ` [PATCH v5 8/9] riscv: dts: thead: Add PVT node Michal Wilczynski
` (2 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add the Device Tree node for the T-HEAD TH1520 SoC's PWM controller.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 1db0054c4e093400e9dbebcee5fcfa5b5cae6e32..26996422e1efe5d2dde68819c2cec1c3fa782a23 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -490,6 +490,13 @@ uart2: serial@ffec010000 {
status = "disabled";
};
+ pwm: pwm@ffec01c000 {
+ compatible = "thead,th1520-pwm";
+ reg = <0xff 0xec01c000 0x0 0x4000>;
+ clocks = <&clk CLK_PWM>;
+ #pwm-cells = <3>;
+ };
+
clk: clock-controller@ffef010000 {
compatible = "thead,th1520-clk-ap";
reg = <0xff 0xef010000 0x0 0x1000>;
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v5 8/9] riscv: dts: thead: Add PVT node
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (6 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 7/9] riscv: dts: thead: Add PWM controller node Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-30 20:27 ` Drew Fustini
2025-07-25 1:17 ` Stephen Boyd
2025-06-23 18:08 ` [PATCH v5 9/9] riscv: dts: thead: Add PWM fan and thermal control Michal Wilczynski
2025-06-27 8:25 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
9 siblings, 2 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add PVT DT node for thermal sensor.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 26996422e1efe5d2dde68819c2cec1c3fa782a23..bef30780034e06b07aa29b27b0225ea891a4b531 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -669,6 +669,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
thead,pad-group = <1>;
};
+ pvt: pvt@fffff4e000 {
+ compatible = "moortec,mr75203";
+ reg = <0xff 0xfff4e000 0x0 0x80>,
+ <0xff 0xfff4e080 0x0 0x100>,
+ <0xff 0xfff4e180 0x0 0x680>,
+ <0xff 0xfff4e800 0x0 0x600>;
+ reg-names = "common", "ts", "pd", "vm";
+ clocks = <&aonsys_clk>;
+ #thermal-sensor-cells = <1>;
+ };
+
gpio@fffff52000 {
compatible = "snps,dw-apb-gpio";
reg = <0xff 0xfff52000 0x0 0x1000>;
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 8/9] riscv: dts: thead: Add PVT node
2025-06-23 18:08 ` [PATCH v5 8/9] riscv: dts: thead: Add PVT node Michal Wilczynski
@ 2025-06-30 20:27 ` Drew Fustini
2025-07-25 1:17 ` Stephen Boyd
1 sibling, 0 replies; 29+ messages in thread
From: Drew Fustini @ 2025-06-30 20:27 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Marek Szyprowski,
Benno Lossin, Michael Turquette, Stephen Boyd, linux-kernel,
linux-pwm, rust-for-linux, linux-riscv, devicetree, linux-clk
On Mon, Jun 23, 2025 at 08:08:56PM +0200, Michal Wilczynski wrote:
> Add PVT DT node for thermal sensor.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 26996422e1efe5d2dde68819c2cec1c3fa782a23..bef30780034e06b07aa29b27b0225ea891a4b531 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -669,6 +669,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
> thead,pad-group = <1>;
> };
>
> + pvt: pvt@fffff4e000 {
> + compatible = "moortec,mr75203";
> + reg = <0xff 0xfff4e000 0x0 0x80>,
> + <0xff 0xfff4e080 0x0 0x100>,
> + <0xff 0xfff4e180 0x0 0x680>,
> + <0xff 0xfff4e800 0x0 0x600>;
> + reg-names = "common", "ts", "pd", "vm";
> + clocks = <&aonsys_clk>;
> + #thermal-sensor-cells = <1>;
> + };
> +
> gpio@fffff52000 {
> compatible = "snps,dw-apb-gpio";
> reg = <0xff 0xfff52000 0x0 0x1000>;
>
> --
> 2.34.1
>
The PVT sensor is useful for more than just the fan so I'm okay with
taking this even though the PWM driver has yet to be accepted. I have
applied this patch to thead-dt-for-next [1] as commit c31f289 [2].
The required clk driver fix has been applied to thead-clk-for-next [3]
as commit 0370395 [4], so PVT sensor will be able to be tested in next.
Thanks,
Drew
[1] https://github.com/pdp7/linux/commits/thead-dt-for-next/
[2] https://github.com/pdp7/linux/commit/c31f2899eab084b3557e9f9e10fc7898113ef18d
[3] https://github.com/pdp7/linux/commits/thead-clk-for-next/
[4] https://github.com/pdp7/linux/commit/0370395d45ca6dd53bb931978f0e91ac8dd6f1c5
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 8/9] riscv: dts: thead: Add PVT node
2025-06-23 18:08 ` [PATCH v5 8/9] riscv: dts: thead: Add PVT node Michal Wilczynski
2025-06-30 20:27 ` Drew Fustini
@ 2025-07-25 1:17 ` Stephen Boyd
2025-07-26 16:56 ` Drew Fustini
1 sibling, 1 reply; 29+ messages in thread
From: Stephen Boyd @ 2025-07-25 1:17 UTC (permalink / raw)
To: Albert Ou, Alex Gaynor, Alexandre Ghiti, Alice Ryhl,
Andreas Hindborg, Benno Lossin, Björn Roy Baron, Boqun Feng,
Conor Dooley, Danilo Krummrich, Drew Fustini, Fu Wei, Gary Guo,
Guo Ren, Krzysztof Kozlowski, Marek Szyprowski, Michael Turquette,
Michal Wilczynski, Miguel Ojeda, Palmer Dabbelt, Paul Walmsley,
Rob Herring, Trevor Gross, Uwe Kleine-König
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Quoting Michal Wilczynski (2025-06-23 11:08:56)
> Add PVT DT node for thermal sensor.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 26996422e1efe5d2dde68819c2cec1c3fa782a23..bef30780034e06b07aa29b27b0225ea891a4b531 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -669,6 +669,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
> thead,pad-group = <1>;
> };
>
> + pvt: pvt@fffff4e000 {
Node name should probably be 'thermal-sensor@fffff4e000' then.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 8/9] riscv: dts: thead: Add PVT node
2025-07-25 1:17 ` Stephen Boyd
@ 2025-07-26 16:56 ` Drew Fustini
0 siblings, 0 replies; 29+ messages in thread
From: Drew Fustini @ 2025-07-26 16:56 UTC (permalink / raw)
To: Stephen Boyd
Cc: Albert Ou, Alex Gaynor, Alexandre Ghiti, Alice Ryhl,
Andreas Hindborg, Benno Lossin, Björn Roy Baron, Boqun Feng,
Conor Dooley, Danilo Krummrich, Drew Fustini, Fu Wei, Gary Guo,
Guo Ren, Krzysztof Kozlowski, Marek Szyprowski, Michael Turquette,
Michal Wilczynski, Miguel Ojeda, Palmer Dabbelt, Paul Walmsley,
Rob Herring, Trevor Gross, Uwe Kleine-König, linux-kernel,
linux-pwm, rust-for-linux, linux-riscv, devicetree, linux-clk
On Thu, Jul 24, 2025 at 06:17:15PM -0700, Stephen Boyd wrote:
> Quoting Michal Wilczynski (2025-06-23 11:08:56)
> > Add PVT DT node for thermal sensor.
> >
> > Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> > ---
> > arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > index 26996422e1efe5d2dde68819c2cec1c3fa782a23..bef30780034e06b07aa29b27b0225ea891a4b531 100644
> > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > @@ -669,6 +669,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
> > thead,pad-group = <1>;
> > };
> >
> > + pvt: pvt@fffff4e000 {
>
> Node name should probably be 'thermal-sensor@fffff4e000' then.
Thanks for pointing this out. It does seem like 'pvt@' is unusual:
$ git grep -l 'thermal-sensor@' arch/*/boot/dts | wc -l
57
$ git grep -l 'pvt@' arch/*/boot/dts | wc -l
1
$ git grep -l 'thermal-sensor@' Documentation/devicetree/bindings/ | wc -l
14
$ git grep -l 'pvt@' Documentation/devicetree/bindings/ | wc -l
2
The 6.17 PR for dts has already happened, so I'll do a fix once I can
update my thead-dt-fixes branch to 6.17-rc1.
thanks,
drew
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 9/9] riscv: dts: thead: Add PWM fan and thermal control
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (7 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 8/9] riscv: dts: thead: Add PVT node Michal Wilczynski
@ 2025-06-23 18:08 ` Michal Wilczynski
2025-06-27 8:25 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
9 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-23 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Michal Wilczynski, Drew Fustini,
Guo Ren, Fu Wei, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Marek Szyprowski, Benno Lossin, Michael Turquette, Stephen Boyd,
Benno Lossin
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk
Add Device Tree nodes to enable a PWM controlled fan and it's associated
thermal management for the Lichee Pi 4A board.
This enables temperature-controlled active cooling for the Lichee Pi 4A
board based on SoC temperature.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts | 67 +++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts b/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
index 4020c727f09e8e2286fdc7fecd79dbd8eba69556..c58c2085ca92a3234f1350500cedae4157f0c35f 100644
--- a/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
+++ b/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
@@ -28,9 +28,76 @@ aliases {
chosen {
stdout-path = "serial0:115200n8";
};
+
+ thermal-zones {
+ cpu-thermal {
+ polling-delay = <1000>;
+ polling-delay-passive = <1000>;
+ thermal-sensors = <&pvt 0>;
+
+ trips {
+ fan_config0: fan-trip0 {
+ temperature = <39000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+
+ fan_config1: fan-trip1 {
+ temperature = <50000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+
+ fan_config2: fan-trip2 {
+ temperature = <60000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+ };
+
+ cooling-maps {
+ map-active-0 {
+ cooling-device = <&fan 1 1>;
+ trip = <&fan_config0>;
+ };
+
+ map-active-1 {
+ cooling-device = <&fan 2 2>;
+ trip = <&fan_config1>;
+ };
+
+ map-active-2 {
+ cooling-device = <&fan 3 3>;
+ trip = <&fan_config2>;
+ };
+ };
+ };
+ };
+
+ fan: pwm-fan {
+ pinctrl-names = "default";
+ pinctrl-0 = <&fan_pins>;
+ compatible = "pwm-fan";
+ #cooling-cells = <2>;
+ pwms = <&pwm 1 10000000 0>;
+ cooling-levels = <0 66 196 255>;
+ };
+
};
&padctrl0_apsys {
+ fan_pins: fan-0 {
+ pwm1-pins {
+ pins = "GPIO3_3"; /* PWM1 */
+ function = "pwm";
+ bias-disable;
+ drive-strength = <25>;
+ input-disable;
+ input-schmitt-disable;
+ slew-rate = <0>;
+ };
+ };
+
uart0_pins: uart0-0 {
tx-pins {
pins = "UART0_TXD";
--
2.34.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver
2025-06-23 18:08 ` [PATCH v5 0/9] Rust Abstractions for PWM subsystem with TH1520 PWM driver Michal Wilczynski
` (8 preceding siblings ...)
2025-06-23 18:08 ` [PATCH v5 9/9] riscv: dts: thead: Add PWM fan and thermal control Michal Wilczynski
@ 2025-06-27 8:25 ` Michal Wilczynski
9 siblings, 0 replies; 29+ messages in thread
From: Michal Wilczynski @ 2025-06-27 8:25 UTC (permalink / raw)
To: Uwe Kleine-König, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Drew Fustini, Guo Ren, Fu Wei,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Marek Szyprowski,
Benno Lossin, Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-pwm, rust-for-linux, linux-riscv, devicetree,
linux-clk, Krzysztof Kozlowski
On 6/23/25 20:08, Michal Wilczynski wrote:
> This patch series introduces Rust support for the T-HEAD TH1520 PWM
> controller and demonstrates its use for fan control on the Sipeed Lichee
> Pi 4A board.
>
> The primary goal of this patch series is to introduce a basic set of
> Rust abstractions for the Linux PWM subsystem. As a first user and
> practical demonstration of these abstractions, the series also provides
> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
> of its PWM channels and ultimately enables temperature controlled fan
> support for the Lichee Pi 4A board. This work aims to explore the use of
> Rust for PWM drivers and lay a foundation for potential future
> Rust based PWM drivers.
>
> The core of this series is a new rust/kernel/pwm.rs module that provides
> abstractions for writing PWM chip provider drivers in Rust. This has
> been significantly reworked from v1 based on extensive feedback. The key
> features of the new abstraction layer include:
>
> - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
> by ARef, correctly tying its lifetime to its embedded struct device
> reference counter. Chip registration is handled by a pwm::Registration
> RAII guard, which guarantees that pwmchip_add is always paired with
> pwmchip_remove, preventing resource leaks.
>
> - Modern and Safe API: The PwmOps trait is now based on the modern
> waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
> by the subsystem maintainer. It is generic over a driver's
> hardware specific data structure, moving all unsafe serialization logic
> into the abstraction layer and allowing drivers to be written in 100%
> safe Rust.
>
> - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
> types (State, Args, Device, etc.) and uses standard kernel error
> handling patterns.
>
> The series is structured as follows:
> - Rust PWM Abstractions: The new safe abstraction layer.
> - TH1520 PWM Driver: A new Rust driver for the TH1520 SoC, built on
> top of the new abstractions.
> - Clock Fix: A necessary fix to the TH1520 clock driver to ensure bus
> clocks remain enabled.
> - Device Tree Bindings & Nodes: The remaining patches add the necessary
> DT bindings and nodes for the TH1520 PWM controller, a thermal
> sensor, and the PWM fan configuration for the Lichee Pi 4A board.
>
> Testing:
> Tested on the TH1520 SoC. The fan works correctly. The duty/period
> calculations are correct. Fan starts slow when the chip is not hot and
> gradually increases the speed when PVT reports higher temperatures.
>
> The patches are based on mainline, with some dependencies which are not
> merged yet - platform Io support [1].
>
> Reference repository with all the patches together can be found on
> github [2].
>
> [1] - https://lore.kernel.org/rust-for-linux/20250509-topics-tyr-platform_iomem-v8-0-e9f1725a40da@collabora.com/
> [2] - https://github.com/mwilczy/linux/commits/rust-next-pwm-working-fan-for-sending-v9/
>
> ---
> Changes in v5:
> - Reworked `pwm::Chip` creation to take driver data directly, which
> allowed making the `chip.drvdata()` accessor infallible
> - added missing `pwm.c` file lost during the commit split (sorry !)
> - Link to v4: https://lore.kernel.org/r/20250618-rust-next-pwm-working-fan-for-sending-v4-0-a6a28f2b6d8a@samsung.com
>
> Changes in v4:
> - Reworked the pwm::Registration API to use the devres framework,
> addressing lifetime issue.
> - Corrected the PwmOps trait and its callbacks to use immutable references
> (&Chip, &Device) for improved safety.
> - Applied various code style and naming cleanups based on feedback
>
> - Link to v3: https://lore.kernel.org/r/20250617-rust-next-pwm-working-fan-for-sending-v3-0-1cca847c6f9f@samsung.com
>
> Changes in v3:
> - Addressed feedback from Uwe by making multiple changes to the TH1520
> driver and the abstraction layer.
> - Split the core PWM abstractions into three focused commits to ease
> review per Benno request.
> - Confirmed the driver now works correctly with CONFIG_PWM_DEBUG enabled
> by implementing the full waveform API, which correctly reads the
> hardware state.
> - Refactored the Rust code to build cleanly with
> CONFIG_RUST_BUILD_ASSERT_ALLOW=n, primarily by using the try_* family of
> functions for IoMem access.
> - Included several cosmetic changes and cleanups to the abstractions
> per Miguel review.
>
> - Link to v2: https://lore.kernel.org/r/20250610-rust-next-pwm-working-fan-for-sending-v2-0-753e2955f110@samsung.com
>
> Changes in v2:
> - Reworked the PWM abstraction layer based on extensive feedback.
> - Replaced initial devm allocation with a proper ARef<Chip> lifetime model
> using AlwaysRefCounted.
> - Implemented a Registration RAII guard to ensure safe chip add/remove.
> - Migrated the PwmOps trait from the legacy .apply callback to the modern
> waveform API.
> - Refactored the TH1520 driver to use the new, safer abstractions.
> - Added a patch to mark essential bus clocks as CLK_IGNORE_UNUSED to fix
> boot hangs when the PWM and thermal sensors are enabled.
> - Link to v1: https://lore.kernel.org/r/20250524-rust-next-pwm-working-fan-for-sending-v1-0-bdd2d5094ff7@samsung.com
>
> ---
> Michal Wilczynski (9):
> rust: pwm: Add Kconfig and basic data structures
> rust: pwm: Add core 'Device' and 'Chip' object wrappers
> rust: pwm: Add driver operations trait and registration support
> pwm: Add Rust driver for T-HEAD TH1520 SoC
> clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED
> dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
> riscv: dts: thead: Add PWM controller node
> riscv: dts: thead: Add PVT node
> riscv: dts: thead: Add PWM fan and thermal control
>
> .../devicetree/bindings/pwm/thead,th1520-pwm.yaml | 48 ++
> MAINTAINERS | 8 +
> arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts | 67 ++
> arch/riscv/boot/dts/thead/th1520.dtsi | 18 +
> drivers/clk/thead/clk-th1520-ap.c | 5 +-
> drivers/pwm/Kconfig | 23 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm_th1520.rs | 318 ++++++++
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/helpers.c | 1 +
> rust/helpers/pwm.c | 20 +
> rust/kernel/lib.rs | 2 +
> rust/kernel/pwm.rs | 890 +++++++++++++++++++++
> 13 files changed, 1400 insertions(+), 2 deletions(-)
> ---
> base-commit: 79b01ff21368605a62b76535ab1ab5f1f726de60
> change-id: 20250524-rust-next-pwm-working-fan-for-sending-552ad2d1b193
>
> Best regards,
Hi,
Since this v5 patchset hasn't received further comments, I'd like to
inquire about the merge strategy.
The first three patches containing the Rust PWM abstractions have no
outstanding dependencies. Could these be considered for the next merge
window independently?
The fourth patch (the TH1520 driver) depends on the platform IoMem
support [1], which is now at v10. If that series is merged, could the
rest of this series also be considered?
Thanks for your guidance.
[1] - https://lore.kernel.org/rust-for-linux/20250623-topics-tyr-platform_iomem-v10-0-ed860a562940@collabora.com/
Best regards,
--
Michal Wilczynski <m.wilczynski@samsung.com>
^ permalink raw reply [flat|nested] 29+ messages in thread