[PATCH v16 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Uwe Kleine-König]

Hello Michael,

On Wed, Oct 22, 2025 at 10:34:42AM +0200, Michal Wilczynski wrote:
> Since you mentioned last time that you were happy with the code, would
> you please consider picking up this series for linux-next? It would be
> great to get it in for wider testing to ensure everything is solid.

I took another look, and just being able to compile and understanding
very little Rust, I came up with:

diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
index 79fbb13cd47f..c8f9f5b61bfa 100644
--- a/rust/kernel/pwm.rs
+++ b/rust/kernel/pwm.rs
@@ -15,38 +15,7 @@
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
-use core::{convert::TryFrom, marker::PhantomData, ptr::NonNull};
+use core::{marker::PhantomData, ptr::NonNull};
-
-/// 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).
@@ -89,22 +58,6 @@ fn from(wf: Waveform) -> Self {
     }
 }
 
-/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
-#[repr(transparent)]
-pub struct State(bindings::pwm_state);
-
-impl State {
-    /// 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)
-    }
-
-    /// Returns `true` if the PWM signal is enabled.
-    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 {
@@ -164,13 +117,6 @@ pub fn label(&self) -> Option<&CStr> {
         Some(unsafe { CStr::from_char_ptr(label_ptr) })
     }
 
-    /// 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);

Does that make sense?

I consider applying your series and put the above space on top, to
create my first Rust patch :-)

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20251024/372b4a83/attachment.sig>