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.

 - High-Level `Device` API: Exposes safe wrappers for the modern
   `waveform` API, allowing consumers to apply, read, and pre-validate
   hardware configurations.

 - 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 | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 268 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
index 3fad101406eac728d9b12083fad7abf7b7f89b25..3b383b66c241ac68213924c3aa7bc933a817bc46 100644
--- a/rust/kernel/pwm.rs
+++ b/rust/kernel/pwm.rs
@@ -7,11 +7,12 @@
 //! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
 
 use crate::{
-    bindings,
+    bindings, device,
+    error::{self, to_result},
     prelude::*,
-    types::Opaque,
+    types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
 };
-use core::convert::TryFrom;
+use core::{convert::TryFrom, ptr::NonNull};
 
 /// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
@@ -135,3 +136,267 @@ pub fn enabled(&self) -> bool {
         self.0.enabled
     }
 }
+
+/// 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,
+}
+
+/// 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 })
+    }
+
+    /// Sets the PWM waveform configuration and enables the PWM signal.
+    pub fn set_waveform(&self, wf: &Waveform, exact: bool) -> Result {
+        let c_wf = bindings::pwm_waveform::from(*wf);
+
+        // SAFETY: `self.as_raw()` provides a valid `*mut pwm_device` pointer.
+        // `&c_wf` is a valid pointer to a `pwm_waveform` struct. The C function
+        // handles all necessary internal locking.
+        let ret = unsafe { bindings::pwm_set_waveform_might_sleep(self.as_raw(), &c_wf, exact) };
+        to_result(ret)
+    }
+
+    /// Queries the hardware for the configuration it would apply for a given
+    /// request.
+    pub fn round_waveform(&self, wf: &mut Waveform) -> Result<RoundingOutcome> {
+        let mut c_wf = bindings::pwm_waveform::from(*wf);
+
+        // SAFETY: `self.as_raw()` provides a valid `*mut pwm_device` pointer.
+        // `&mut c_wf` is a valid pointer to a mutable `pwm_waveform` struct that
+        // the C function will update.
+        let ret = unsafe { bindings::pwm_round_waveform_might_sleep(self.as_raw(), &mut c_wf) };
+
+        to_result(ret)?;
+
+        *wf = Waveform::from(c_wf);
+
+        if ret == 1 {
+            Ok(RoundingOutcome::RoundedUp)
+        } else {
+            Ok(RoundingOutcome::ExactOrRoundedDown)
+        }
+    }
+
+    /// Reads the current waveform configuration directly from the hardware.
+    pub fn get_waveform(&self) -> Result<Waveform> {
+        let mut c_wf = bindings::pwm_waveform::default();
+
+        // SAFETY: `self.as_raw()` is a valid pointer. We provide a valid pointer
+        // to a stack-allocated `pwm_waveform` struct for the kernel to fill.
+        let ret = unsafe { bindings::pwm_get_waveform_might_sleep(self.as_raw(), &mut c_wf) };
+
+        to_result(ret)?;
+
+        Ok(Waveform::from(c_wf))
+    }
+}
+
+/// 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>()) }
+    }
+
+    /// 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>() }
+    }
+
+    /// 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 };
+        // SAFETY: `chip_ref` points to a valid chip from `pwmchip_alloc` and `drvdata` is a valid,
+        // owned pointer from `ForeignOwnable` to be stored in the chip's private data.
+        unsafe { bindings::pwmchip_set_drvdata(chip_ref.as_raw(), drvdata.into_foreign().cast()) }
+
+        // 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)) })
+    }
+
+    /// Returns a reference to the parent device of this PWM chip's device.
+    ///
+    /// # Safety
+    ///
+    /// The caller must guarantee that the parent device exists and is bound.
+    /// This is guaranteed by the PWM core during `PwmOps` callbacks.
+    unsafe fn bound_parent_device(&self) -> &device::Device<Bound> {
+        // SAFETY: Per the function's safety contract, the parent device exists.
+        let parent = unsafe { self.device().parent().unwrap_unchecked() };
+
+        // SAFETY: Per the function's safety contract, the parent device is bound.
+        // The pointer is cast from `&Device` to `&Device<Bound>`.
+        unsafe { &*core::ptr::from_ref(parent).cast::<device::Device<Bound>>() }
+    }
+}
+
+// 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

On Wed, Jul 02, 2025 at 03:45:31PM +0200, Michal Wilczynski wrote:
> 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.
> 
>  - High-Level `Device` API: Exposes safe wrappers for the modern
>    `waveform` API, allowing consumers to apply, read, and pre-validate
>    hardware configurations.
> 
>  - 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>

Few more comments below, with those fixed:

	Reviewed-by: Danilo Krummrich <dakr@kernel.org>

> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::pwm_device>);
> +
> +impl Device {

<snip>

> +    /// 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) }

I assume the C API does guarantee that a struct pwm_device *always* holds a
valid pointer to a struct pwm_chip?

> +
> +/// 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) };

I think you can use `&raw` instead.

> +        // 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>()) }

Please use Device::as_ref() instead.

> +    }
> +
> +    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
> +    pub fn drvdata<T: 'static>(&self) -> &T {

You need to make the whole Chip structure generic over T, i.e.
Chip<T: ForeignOwnable>.

Otherwise the API is unsafe, since the caller can pass in any T when calling
`chip.drvdata()` regardless of whether you actually stored as private data
through Chip::new().

Also, given that `T: ForeignOwnable`, you should return `T::Borrowed`.

> +        // 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>() }
> +    }
> +
> +    /// 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,

As mentioned above, the whole Chip structure needs to be generic over T,
otherwise you can't guarantee that this T is the same T as the one in drvdata().

> +// 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));

I think you can use `&raw mut` instead.

Also, if you move the semicolon at the end of the unsafe block, this goes in one
line.

> +        }
> +    }
> +
> +    #[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));
> +        }

Same here.

> +    }
> +}

On 7/2/25 17:13, Danilo Krummrich wrote:
> On Wed, Jul 02, 2025 at 03:45:31PM +0200, Michal Wilczynski wrote:
>> 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.
>>
>>  - High-Level `Device` API: Exposes safe wrappers for the modern
>>    `waveform` API, allowing consumers to apply, read, and pre-validate
>>    hardware configurations.
>>
>>  - 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>
> 
> Few more comments below, with those fixed:
> 
> 	Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> 
>> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::pwm_device>);
>> +
>> +impl Device {
> 
> <snip>
> 
>> +    /// 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) }
> 
> I assume the C API does guarantee that a struct pwm_device *always* holds a
> valid pointer to a struct pwm_chip?
> 
>> +
>> +/// 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) };
> 
> I think you can use `&raw` instead.
> 
>> +        // 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>()) }
> 
> Please use Device::as_ref() instead.
> 
>> +    }
>> +
>> +    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
>> +    pub fn drvdata<T: 'static>(&self) -> &T {
> 
> You need to make the whole Chip structure generic over T, i.e.
> Chip<T: ForeignOwnable>.
> 
> Otherwise the API is unsafe, since the caller can pass in any T when calling
> `chip.drvdata()` regardless of whether you actually stored as private data
> through Chip::new().

You were right that the original drvdata<T>() method was unsafe. The
most direct fix, making Chip generic to Chip<T>, unfortunately creates a
significant cascade effect:

- If Chip becomes Chip<T>, then anything holding it, like ARef, must
  become ARef<Chip<T>>.

- This in turn forces container structs like Registration to become
  generic (Registration<T>).

- Finally, the PwmOps trait itself needs to be aware of T, which
  complicates the trait and all driver implementations.

This chain reaction adds a lot of complexity. To avoid it, I've
figured an alternative:

The new idea keeps Chip simple and non generic but ensures type safety
through two main improvements to the abstraction layer:

1. A Thread Safe DriverData Wrapper

The pwm.rs module now provides a generic pwm::DriverData<T> struct. Its
only job is to wrap the driver's private data and provide the necessary
unsafe impl Send + Sync.

// In `rust/kernel/pwm.rs`
// SAFETY: The contained data is guaranteed by the kernel to have
// synchronized access during callbacks.
pub struct DriverData<T>(T);
unsafe impl<T: Send> Send for DriverData<T> {}
unsafe impl<T: Sync> Sync for DriverData<T> {}

// In the driver's `probe` function
let safe_data = pwm::DriverData::new(Th1520PwmDriverData{ });

2. A More Ergonomic PwmOps Trait

The PwmOps trait methods now receive the driver's data directly as
&self, which is much more intuitive. We achieve this by providing a
default associated type for the data owner, which removes boilerplate
from the driver.

// In `rust/kernel/pwm.rs`
pub trait PwmOps: 'static + Sized {
    type Owner: Deref<Target = DriverData<Self>> + ForeignOwnable =
        Pin<KBox<DriverData<Self>>>;
    /// For now I'm getting compiler error here: `associated type defaults are unstable`
    /// So the driver would need to specify this for now, until this feature
    /// is stable


    // Methods now receive `&self`, making them much cleaner to implement.
    fn round_waveform_tohw(&self, chip: &Chip, pwm: &Device, wf: &Waveform) -> Result<...>;
}

// In the driver
impl pwm::PwmOps for Th1520PwmDriverData {
    type WfHw = Th1520WfHw;

    fn round_waveform_tohw(&self, chip: &pwm::Chip, ...) -> Result<...> {
        // no drvdata() call here :-)
        let rate_hz = self.clk.rate().as_hz();
        // ...
    }
}

This solution seem to address to issue you've pointed (as the user of
the API never deals with drvdata directly at this point), while making
it easier to develop PWM drivers in Rust.

Please let me know what you think.

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

On Thu, Jul 03, 2025 at 01:37:43PM +0200, Michal Wilczynski wrote:
> 
> 
> On 7/2/25 17:13, Danilo Krummrich wrote:
> > On Wed, Jul 02, 2025 at 03:45:31PM +0200, Michal Wilczynski wrote:
> >> 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.
> >>
> >>  - High-Level `Device` API: Exposes safe wrappers for the modern
> >>    `waveform` API, allowing consumers to apply, read, and pre-validate
> >>    hardware configurations.
> >>
> >>  - 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>
> > 
> > Few more comments below, with those fixed:
> > 
> > 	Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> > 
> >> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
> >> +#[repr(transparent)]
> >> +pub struct Device(Opaque<bindings::pwm_device>);
> >> +
> >> +impl Device {
> > 
> > <snip>
> > 
> >> +    /// 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) }
> > 
> > I assume the C API does guarantee that a struct pwm_device *always* holds a
> > valid pointer to a struct pwm_chip?
> > 
> >> +
> >> +/// 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) };
> > 
> > I think you can use `&raw` instead.
> > 
> >> +        // 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>()) }
> > 
> > Please use Device::as_ref() instead.
> > 
> >> +    }
> >> +
> >> +    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
> >> +    pub fn drvdata<T: 'static>(&self) -> &T {
> > 
> > You need to make the whole Chip structure generic over T, i.e.
> > Chip<T: ForeignOwnable>.
> > 
> > Otherwise the API is unsafe, since the caller can pass in any T when calling
> > `chip.drvdata()` regardless of whether you actually stored as private data
> > through Chip::new().
> 
> You were right that the original drvdata<T>() method was unsafe. The
> most direct fix, making Chip generic to Chip<T>, unfortunately creates a
> significant cascade effect:
> 
> - If Chip becomes Chip<T>, then anything holding it, like ARef, must
>   become ARef<Chip<T>>.
> 
> - This in turn forces container structs like Registration to become
>   generic (Registration<T>).
> 
> - Finally, the PwmOps trait itself needs to be aware of T, which
>   complicates the trait and all driver implementations.
> 
> This chain reaction adds a lot of complexity. To avoid it, I've
> figured an alternative:
> 
> The new idea keeps Chip simple and non generic but ensures type safety
> through two main improvements to the abstraction layer:
> 
> 1. A Thread Safe DriverData Wrapper
> 
> The pwm.rs module now provides a generic pwm::DriverData<T> struct. Its
> only job is to wrap the driver's private data and provide the necessary
> unsafe impl Send + Sync.
> 
> // In `rust/kernel/pwm.rs`
> // SAFETY: The contained data is guaranteed by the kernel to have
> // synchronized access during callbacks.
> pub struct DriverData<T>(T);
> unsafe impl<T: Send> Send for DriverData<T> {}
> unsafe impl<T: Sync> Sync for DriverData<T> {}

I think you don't need to implement them explicitly, it's automatically derived
from T.

> 
> // In the driver's `probe` function
> let safe_data = pwm::DriverData::new(Th1520PwmDriverData{ });
> 
> 2. A More Ergonomic PwmOps Trait
> 
> The PwmOps trait methods now receive the driver's data directly as
> &self, which is much more intuitive. We achieve this by providing a
> default associated type for the data owner, which removes boilerplate
> from the driver.
> 
> // In `rust/kernel/pwm.rs`
> pub trait PwmOps: 'static + Sized {
>     type Owner: Deref<Target = DriverData<Self>> + ForeignOwnable =
>         Pin<KBox<DriverData<Self>>>;
>     /// For now I'm getting compiler error here: `associated type defaults are unstable`
>     /// So the driver would need to specify this for now, until this feature
>     /// is stable
> 
> 
>     // Methods now receive `&self`, making them much cleaner to implement.
>     fn round_waveform_tohw(&self, chip: &Chip, pwm: &Device, wf: &Waveform) -> Result<...>;
> }
> 
> // In the driver
> impl pwm::PwmOps for Th1520PwmDriverData {
>     type WfHw = Th1520WfHw;

Shouldn't this be:

	type Owner = Pin<KBox<DriverData<Self>>>;

> 
>     fn round_waveform_tohw(&self, chip: &pwm::Chip, ...) -> Result<...> {

If you accept any ForeignOwnable, I think this has to be Owner::Borrowed.

>         // no drvdata() call here :-)
>         let rate_hz = self.clk.rate().as_hz();
>         // ...
>     }
> }
> 
> This solution seem to address to issue you've pointed (as the user of
> the API never deals with drvdata directly at this point), while making
> it easier to develop PWM drivers in Rust.
> 
> Please let me know what you think.

In DRM [1][2] we used the approach I proposed, but at a first glance what you
propose seems like it should work as well.

Drivers having to set the Owner type seems a bit unfortunate, but otherwise it
seems like a matter of personal preference.

Although, I just notice, isn't this broken if a driver sets Owner to something
else than Self?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/drm/device.rs
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/drm/driver.rs

Hello Danilo,

On Wed, Jul 02, 2025 at 05:13:34PM +0200, Danilo Krummrich wrote:
> I assume the C API does guarantee that a struct pwm_device *always* holds a
> valid pointer to a struct pwm_chip?

Yes, a pwm_device holds a reference to the chip's dev. So until pwm_put
is called the chip won't go away.

Best regards
Uwe