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

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

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>

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

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>

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>

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

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>

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

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