[PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Michal Wilczynski]

This patch series introduces Rust support for the T-HEAD TH1520 PWM
controller and demonstrates its use for fan control on the Sipeed Lichee
Pi 4A board.

The primary goal of this patch series is to introduce a basic set of
Rust abstractions for the Linux PWM subsystem. As a first user and
practical demonstration of these abstractions, the series also provides
a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
of its PWM channels and ultimately enables temperature controlled fan
support for the Lichee Pi 4A board. This work aims to explore the use of
Rust for PWM drivers and lay a foundation for potential future
Rust based PWM drivers.

The series is structured as follows:

Patch 1/6: Introduce basic PWM abstractions
This patch lays the groundwork by adding a Kconfig option for Rust PWM
abstractions, necessary C helper functions, and a new Rust module
(rust/kernel/pwm.rs). This module provides initial safe wrappers for
core PWM data structures (Chip, Device, State, Args, Polarity) and
functions (devm_chip_alloc, devm_chip_add), along with a basic PwmOps
trait focusing on the .apply callback needed by PWM chip providers.

Patch 2/6: Add PWM driver for TH1520 SoC
This introduces the Rust based PWM driver for the T-HEAD TH1520 SoC.
It implements the PwmOps trait using the abstractions from the first
patch and handles the specifics of the TH1520 hardware for configuring
period, duty cycle, and polarity. Resource management leverages devm
for the PWM chip and Rust DevRes for I/O memory, and RAII for clock
handling.

Patch 3/6: dt-bindings: Add PWM T-HEAD controller dt-binding
This patch adds the Device Tree binding documentation for the T-HEAD
TH1520 PWM controller.

Patch 4/6: riscv: dts: thead:: Add PWM controller node
This patch adds the actual Device Tree node for the TH1520 PWM controller.

Patch 5/6: riscv: dts: thead: Add PVT node
Add pvt node for thermal sensor.

Patch 6/6: riscv: dts: thead: Add PWM fan and thermal control
This final patch adds the Device Tree configuration for a PWM controlled
fan to the Sipeed Lichee Pi 4A board DTS file. 

Testing:
Tested on the TH1520 SoC. The fan works correctly.

Points for Discussion:
The rust/kernel/pwm.rs abstraction layer is currently minimal,
focusing on the immediate needs of this driver. Feedback on its design,
scope, and potential for generalization would be highly appreciated.
General feedback on the Rust implementation, FFI wrapping patterns, and
adherence to kernel development practices is very welcome.

The patches are based on rust-next, with some dependencies which are not
merged yet - platform Io support [1] and clk abstractions [2]. 

Reference repository with all the patches together can be found on
github [3].

[1] - https://lore.kernel.org/rust-for-linux/20250509-topics-tyr-platform_iomem-v8-0-e9f1725a40da@collabora.com/
[2] - https://lore.kernel.org/rust-for-linux/0ec0250c1170a8a6efb2db7a6cb49ae974d7ce05.1747634382.git.viresh.kumar@linaro.org/ 
[3] - https://github.com/mwilczy/linux/commits/rust-next-pwm-working-fan-for-sending/

---
Michal Wilczynski (6):
      rust: Add basic PWM abstractions
      pwm: Add Rust driver for T-HEAD TH1520 SoC
      dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
      riscv: dts: thead: Add PWM controller node
      riscv: dts: thead: Add PVT node
      riscv: dts: thead: Add PWM fan and thermal control

 .../devicetree/bindings/pwm/thead,th1520-pwm.yaml  |  48 +++
 MAINTAINERS                                        |   8 +
 arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts  |  67 ++++
 arch/riscv/boot/dts/thead/th1520.dtsi              |  18 +
 drivers/pwm/Kconfig                                |  14 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm_th1520.rs                          | 272 +++++++++++++++
 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                                 | 376 +++++++++++++++++++++
 12 files changed, 828 insertions(+)
---
base-commit: 9416c85e2767adcf72a21bce15f9c56ed085c5d4
change-id: 20250524-rust-next-pwm-working-fan-for-sending-552ad2d1b193

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

[PATCH RFC 1/6] rust: Add basic PWM abstractions

[Michal Wilczynski]

Introduce initial Rust abstractions for the Linux PWM subsystem. These
abstractions provide safe wrappers around the core C data structures and
functions, enabling the development of PWM chip drivers in Rust.

The main components added are:
- A Kconfig option RUST_PWM_ABSTRACTIONS
- C helper functions in rust/helpers/pwm.c to provide stable callable
  interfaces for Rust, for pwmchip_parent, pwmchip_get_drvdata, and
  pwmchip_set_drvdata
- A new Rust module rust/kernel/pwm.rs containing:
    - Safe wrappers for struct pwm_chip, struct pwm_device,
      struct pwm_state, and struct pwm_args
    - An enum Polarity for type safe polarity handling
    - Functions devm_chip_alloc and devm_chip_add which wrap the
      kernel's device-managed APIs for PWM chip allocation and
      registration.
    - A PwmOps trait and create_pwm_ops function to allow Rust
      drivers to define their PWM operations, initially supporting the
      .apply callback.

This foundational layer will be used by subsequent patches to implement
a specific PWM chip driver in Rust. It focuses on the pwm_chip provider
APIs necessary for such a driver.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS                     |   6 +
 drivers/pwm/Kconfig             |   8 +
 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              | 376 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 414 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b6126bccf17c899cbbf5b729e1f426ff38d04a8e..2b080e8f3d873b1e401b3a2fe1207c224c4591fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19678,6 +19678,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 4731d5b90d7edcc61138e4a5bf7e98906953ece4..b5bd5c13b3a5e5a575a0fbfb2e285f5665b7a671 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -755,4 +755,12 @@ 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
+	  Adds support needed for PWM drivers written in Rust. It provides a
+          wrapper around the C pwm core.
+
 endif
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 43aabecc1d6161398acf3bb402d1f67b48bfcd6f..24a2498f17a2fc56f5dc012a20d621007be28b5e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -32,6 +32,7 @@
 #include <linux/platform_device.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 34f040e5f260b892fc421a6e55de32cbf90c8c22..7dda45d1794c957d83227362d9483eb90543f053 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -26,6 +26,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 1b0b6790a7f33c30af16a056d6afcca3d15a2a0d..72b60afe5f51d8a62b3c95b1b320a48386d14e61 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -92,6 +92,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..357fda46faa99c4462149658951ec53bf9cc2d1e
--- /dev/null
+++ b/rust/kernel/pwm.rs
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! PWM (Pulse Width Modulator) abstractions.
+//!
+//! This module provides safe Rust abstractions for working with the Linux
+//! kernel's PWM subsystem, leveraging types generated by `bindgen`
+//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
+
+use crate::{
+    bindings,
+    device::Device as CoreDevice,
+    error::*,
+    prelude::*,
+    str::CStr,
+    types::{ForeignOwnable, Opaque},
+};
+use core::marker::PhantomData;
+
+/// PWM polarity. Mirrors `enum pwm_polarity`.
+#[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 From<bindings::pwm_polarity> for Polarity {
+    fn from(polarity: bindings::pwm_polarity) -> Self {
+        match polarity {
+            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
+            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
+            _ => {
+                pr_warn!(
+                    "Unknown pwm_polarity value {}, defaulting to Normal\n",
+                    polarity
+                );
+                Polarity::Normal
+            }
+        }
+    }
+}
+
+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,
+        }
+    }
+}
+
+/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
+#[repr(transparent)]
+pub struct Args(Opaque<bindings::pwm_args>);
+
+impl Args {
+    /// Creates an `Args` wrapper from the C struct reference.
+    fn from_c_ref(c_args: &bindings::pwm_args) -> Self {
+        // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
+        Args(Opaque::new(*c_args))
+    }
+
+    /// Returns the period of the PWM signal in nanoseconds.
+    pub fn period(&self) -> u64 {
+        // SAFETY: Reading from the valid pointer obtained by `get()`.
+        unsafe { (*self.0.get()).period }
+    }
+
+    /// Returns the polarity of the PWM signal.
+    pub fn polarity(&self) -> Polarity {
+        // SAFETY: Reading from the valid pointer obtained by `get()`.
+        Polarity::from(unsafe { (*self.0.get()).polarity })
+    }
+}
+
+/// Wrapper for PWM state (`struct pwm_state`).
+#[repr(transparent)]
+pub struct State(Opaque<bindings::pwm_state>);
+
+impl State {
+    /// Creates a new zeroed `State`.
+    pub fn new() -> Self {
+        State(Opaque::new(bindings::pwm_state::default()))
+    }
+
+    /// Creates a `State` wrapper around a copy of a C `pwm_state`.
+    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
+        State(Opaque::new(c_state))
+    }
+
+    /// Creates a `State` wrapper around a reference to a C `pwm_state`.
+    fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
+        // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
+        unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
+    }
+
+    /// Gets the period of the PWM signal in nanoseconds.
+    pub fn period(&self) -> u64 {
+        unsafe { (*self.0.get()).period }
+    }
+
+    /// Sets the period of the PWM signal in nanoseconds.
+    pub fn set_period(&mut self, period_ns: u64) {
+        unsafe {
+            (*self.0.get()).period = period_ns;
+        }
+    }
+
+    /// Gets the duty cycle of the PWM signal in nanoseconds.
+    pub fn duty_cycle(&self) -> u64 {
+        unsafe { (*self.0.get()).duty_cycle }
+    }
+
+    /// Sets the duty cycle of the PWM signal in nanoseconds.
+    pub fn set_duty_cycle(&mut self, duty_ns: u64) {
+        unsafe {
+            (*self.0.get()).duty_cycle = duty_ns;
+        }
+    }
+
+    /// Returns `true` if the PWM signal is enabled.
+    pub fn enabled(&self) -> bool {
+        unsafe { (*self.0.get()).enabled }
+    }
+
+    /// Sets the enabled state of the PWM signal.
+    pub fn set_enabled(&mut self, enabled: bool) {
+        unsafe {
+            (*self.0.get()).enabled = enabled;
+        }
+    }
+
+    /// Gets the polarity of the PWM signal.
+    pub fn polarity(&self) -> Polarity {
+        Polarity::from(unsafe { (*self.0.get()).polarity })
+    }
+
+    /// Sets the polarity of the PWM signal.
+    pub fn set_polarity(&mut self, polarity: Polarity) {
+        unsafe {
+            (*self.0.get()).polarity = polarity.into();
+        }
+    }
+
+    /// Returns `true` if the PWM signal is configured for power usage hint.
+    pub fn usage_power(&self) -> bool {
+        unsafe { (*self.0.get()).usage_power }
+    }
+
+    /// Sets the power usage hint for the PWM signal.
+    pub fn set_usage_power(&mut self, usage_power: bool) {
+        unsafe {
+            (*self.0.get()).usage_power = usage_power;
+        }
+    }
+}
+
+/// Wrapper for a PWM device/channel (`struct pwm_device`).
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::pwm_device>);
+
+impl Device {
+    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
+        unsafe { &mut *ptr.cast::<Self>() }
+    }
+
+    fn as_ptr(&self) -> *mut bindings::pwm_device {
+        self.0.get()
+    }
+
+    /// Gets the hardware PWM index for this device within its chip.
+    pub fn hwpwm(&self) -> u32 {
+        unsafe { (*self.as_ptr()).hwpwm }
+    }
+
+    /// Gets a reference to the parent `Chip` that this device belongs to.
+    pub fn chip(&self) -> &Chip {
+        unsafe { Chip::from_ptr((*self.as_ptr()).chip) }
+    }
+
+    /// Gets the label for this PWM device, if any.
+    pub fn label(&self) -> Option<&CStr> {
+        let label_ptr = unsafe { (*self.as_ptr()).label };
+        if label_ptr.is_null() {
+            None
+        } else {
+            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 {
+        Args::from_c_ref(unsafe { &(*self.as_ptr()).args })
+    }
+
+    /// Gets a copy of the current state of this PWM device.
+    pub fn state(&self) -> State {
+        State::from_c(unsafe { (*self.as_ptr()).state })
+    }
+
+    /// Returns `true` if the PWM signal is currently enabled based on its state.
+    pub fn is_enabled(&self) -> bool {
+        self.state().enabled()
+    }
+}
+
+/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
+#[repr(transparent)]
+pub struct Chip(Opaque<bindings::pwm_chip>);
+
+impl Chip {
+    /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
+    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
+        unsafe { &mut *ptr.cast::<Self>() }
+    }
+
+    /// Returns a raw pointer to the underlying `pwm_chip`.
+    pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
+        self.0.get()
+    }
+
+    /// Gets the number of PWM channels (hardware PWMs) on this chip.
+    pub fn npwm(&self) -> u32 {
+        unsafe { (*self.as_ptr()).npwm }
+    }
+
+    /// Returns `true` if the chip supports atomic operations for configuration.
+    pub fn is_atomic(&self) -> bool {
+        unsafe { (*self.as_ptr()).atomic }
+    }
+
+    /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
+    pub fn device(&self) -> &CoreDevice {
+        // SAFETY: `dev` field exists and points to the embedded device.
+        let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
+        unsafe { &*(dev_ptr as *mut CoreDevice) }
+    }
+
+    /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
+    pub fn parent_device(&self) -> Option<&CoreDevice> {
+        // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
+        let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
+        if parent_ptr.is_null() {
+            None
+        } else {
+            // SAFETY: Pointer is non-null, assume valid device managed by kernel.
+            Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
+        }
+    }
+
+    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
+    pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {
+        let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };
+        if ptr.is_null() {
+            None
+        } else {
+            unsafe { Some(&*(ptr as *const T)) }
+        }
+    }
+
+    /// Sets the *typed* driver-specific data associated with this chip's embedded device.
+    pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
+        unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
+    }
+}
+
+/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
+pub fn devm_chip_alloc<'a>(
+    parent: &'a CoreDevice,
+    npwm: u32,
+    sizeof_priv: usize,
+) -> Result<&'a mut Chip> {
+    // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
+    let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
+    let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
+    if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
+        let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
+        pr_err!("devm_pwmchip_alloc failed: {}\n", err);
+        Err(Error::from_errno(err as i32))
+    } else {
+        // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
+        Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
+    }
+}
+
+/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
+pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
+    // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
+    unsafe {
+        let chip_ptr = chip.as_ptr();
+        // Assign the ops pointer directly to the C struct field
+        (*chip_ptr).ops = ops.as_ptr();
+        to_result(bindings::__pwmchip_add(
+            chip_ptr,
+            core::ptr::null_mut()
+        ))
+    }
+}
+
+/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
+pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
+    // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
+    unsafe {
+        let chip_ptr = chip.as_ptr();
+        // Assign the ops pointer directly to the C struct field
+        (*chip_ptr).ops = ops.as_ptr();
+        let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
+        to_result(bindings::__devm_pwmchip_add(
+            parent_ptr,
+            chip_ptr,
+            core::ptr::null_mut()
+        ))
+    }
+}
+
+/// Trait defining the operations for a PWM driver. Mirrors relevant parts of `struct pwm_ops`.
+pub trait PwmOps: 'static {
+    /// Atomically apply a new state to the PWM device. Mirrors `pwm_ops->apply`.
+    fn apply(chip: &mut Chip, pwm: &mut Device, state: &State) -> Result;
+
+    // TODO: Add other ops like request, free, capture, waveform ops if needed.
+}
+
+/// Holds the vtable for PwmOps implementations.
+struct Adapter<T: PwmOps> {
+    _p: PhantomData<T>,
+}
+
+impl<T: PwmOps> Adapter<T> {
+    // Trampoline for `apply`.
+    unsafe extern "C" fn apply_callback(
+        chip: *mut bindings::pwm_chip,
+        pwm: *mut bindings::pwm_device,
+        state: *const bindings::pwm_state, // Input state is const
+    ) -> core::ffi::c_int {
+        // SAFETY: Pointers from core are valid. Create temporary wrappers.
+        let chip_ref = unsafe { Chip::from_ptr(chip) };
+        let pwm_ref = unsafe { Device::from_ptr(pwm) };
+        // Use the reference wrapper for the const C state
+        let state_ref = State::from_c_ref(unsafe { &*state });
+
+        match T::apply(chip_ref, pwm_ref, state_ref) {
+            Ok(()) => 0,
+            Err(e) => e.to_errno(),
+        }
+    }
+}
+
+/// VTable structure wrapper for PWM operations. Mirrors `struct pwm_ops`.
+#[repr(transparent)]
+pub struct PwmOpsVTable(Opaque<bindings::pwm_ops>);
+
+// SAFETY: Holds function pointers, no accessible mutable state via &self.
+unsafe impl Sync for PwmOpsVTable {}
+
+impl PwmOpsVTable {
+    /// Returns a raw pointer to the underlying `pwm_ops` struct.
+    pub(crate) fn as_ptr(&self) -> *const bindings::pwm_ops {
+        self.0.get()
+    }
+}
+
+/// Creates a PWM operations vtable for a type `T` that implements `PwmOps`.
+///
+/// This is used to bridge Rust trait implementations to the C `struct pwm_ops`
+/// expected by the kernel.
+pub const fn create_pwm_ops<T: PwmOps>() -> PwmOpsVTable {
+    let mut ops: bindings::pwm_ops = unsafe { core::mem::zeroed() };
+
+    ops.apply = Some(Adapter::<T>::apply_callback);
+
+    PwmOpsVTable(Opaque::new(ops))
+}

-- 
2.34.1

Re: [PATCH RFC 1/6] rust: Add basic PWM abstractions

[Danilo Krummrich]

Hi Michal,

On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..357fda46faa99c4462149658951ec53bf9cc2d1e
> --- /dev/null
> +++ b/rust/kernel/pwm.rs
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! PWM (Pulse Width Modulator) abstractions.
> +//!
> +//! This module provides safe Rust abstractions for working with the Linux
> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
> +
> +use crate::{
> +    bindings,
> +    device::Device as CoreDevice,

I recommend referring the the base device as just device::Device, like we do it
everywhere else where we have this name conflict.

I'm not a huge fan of such aliases, since it's confusing when looking at patches
that do not have the alias as context later on.

> +    error::*,
> +    prelude::*,
> +    str::CStr,
> +    types::{ForeignOwnable, Opaque},
> +};
> +use core::marker::PhantomData;
> +
> +/// PWM polarity. Mirrors `enum pwm_polarity`.
> +#[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 From<bindings::pwm_polarity> for Polarity {
> +    fn from(polarity: bindings::pwm_polarity) -> Self {
> +        match polarity {
> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
> +            _ => {
> +                pr_warn!(
> +                    "Unknown pwm_polarity value {}, defaulting to Normal\n",
> +                    polarity
> +                );
> +                Polarity::Normal

Either Polarity::Normal is the correct thing to return in such a case, but then
we do not need the pr_warn!(), or it is not, but then this should be a TryFrom
impl instead.

> +            }
> +        }
> +    }
> +}
> +
> +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,
> +        }
> +    }
> +}
> +
> +/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
> +#[repr(transparent)]
> +pub struct Args(Opaque<bindings::pwm_args>);
> +
> +impl Args {
> +    /// Creates an `Args` wrapper from the C struct reference.
> +    fn from_c_ref(c_args: &bindings::pwm_args) -> Self {

I'd make this a pointer type instead, rather than having conversion to a
reference at the caller's side.

> +        // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
> +        Args(Opaque::new(*c_args))
> +    }
> +
> +    /// Returns the period of the PWM signal in nanoseconds.
> +    pub fn period(&self) -> u64 {
> +        // SAFETY: Reading from the valid pointer obtained by `get()`.

Here and below, you should explain why this pointer is guaranteed to be valid
instead.

> +        unsafe { (*self.0.get()).period }
> +    }
> +
> +    /// Returns the polarity of the PWM signal.
> +    pub fn polarity(&self) -> Polarity {
> +        // SAFETY: Reading from the valid pointer obtained by `get()`.
> +        Polarity::from(unsafe { (*self.0.get()).polarity })
> +    }
> +}
> +
> +/// Wrapper for PWM state (`struct pwm_state`).
> +#[repr(transparent)]
> +pub struct State(Opaque<bindings::pwm_state>);
> +
> +impl State {
> +    /// Creates a new zeroed `State`.
> +    pub fn new() -> Self {
> +        State(Opaque::new(bindings::pwm_state::default()))
> +    }
> +
> +    /// Creates a `State` wrapper around a copy of a C `pwm_state`.
> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> +        State(Opaque::new(c_state))
> +    }

You probably don't need Opaque here, given that you have instances of
bindings::pwm_state already.

> +
> +    /// Creates a `State` wrapper around a reference to a C `pwm_state`.
> +    fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
> +        // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
> +        unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
> +    }
> +
> +    /// Gets the period of the PWM signal in nanoseconds.
> +    pub fn period(&self) -> u64 {
> +        unsafe { (*self.0.get()).period }

This and all the methods below lack SAFETY comments, did you compile with
CLIPPY=1? You should get lots of warnings reminding you to add them.

<snip>

> +
> +/// Wrapper for a PWM device/channel (`struct pwm_device`).
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::pwm_device>);
> +
> +impl Device {
> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {

We usually call this kind of function as_ref(), it'd be nice to stick to that
for consistency.

> +        unsafe { &mut *ptr.cast::<Self>() }

A mutable reference -- why?

<snip>

> +/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
> +#[repr(transparent)]
> +pub struct Chip(Opaque<bindings::pwm_chip>);
> +
> +impl Chip {
> +    /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {

Same here, for the name...

> +        unsafe { &mut *ptr.cast::<Self>() }

and the mutability.

> +    }
> +
> +    /// Returns a raw pointer to the underlying `pwm_chip`.
> +    pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
> +        self.0.get()
> +    }
> +
> +    /// Gets the number of PWM channels (hardware PWMs) on this chip.
> +    pub fn npwm(&self) -> u32 {
> +        unsafe { (*self.as_ptr()).npwm }
> +    }
> +
> +    /// Returns `true` if the chip supports atomic operations for configuration.
> +    pub fn is_atomic(&self) -> bool {
> +        unsafe { (*self.as_ptr()).atomic }
> +    }
> +
> +    /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
> +    pub fn device(&self) -> &CoreDevice {
> +        // SAFETY: `dev` field exists and points to the embedded device.
> +        let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
> +        unsafe { &*(dev_ptr as *mut CoreDevice) }

Missing SAFETY comment, also please use cast() and cast_{const,mut}() instead.

> +    }
> +
> +    /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
> +    pub fn parent_device(&self) -> Option<&CoreDevice> {
> +        // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
> +        let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
> +        if parent_ptr.is_null() {
> +            None
> +        } else {
> +            // SAFETY: Pointer is non-null, assume valid device managed by kernel.
> +            Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
> +        }

This can just be

	self.device().parent() // [1]

which lands through the DRM tree in the upcoming merge window.

[1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/device.rs?ref_type=heads#L72

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

I will soon send a patch series that adds drvdata accessors to the generic
Device abstraction.

Anyways, no need for you to wait for this.

> +        let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };

Obviously, the C side calls this pwmchip_get_drvdata() and dev_get_drvdata(),
however, I suggest not to use get_drvdata() as a method name, since 'get' is
used in the context of reference counting (get/put) instead, and hence may be
confusing. Let's just name this drvdata().

> +        if ptr.is_null() {
> +            None
> +        } else {
> +            unsafe { Some(&*(ptr as *const T)) }
> +        }
> +    }
> +
> +    /// Sets the *typed* driver-specific data associated with this chip's embedded device.
> +    pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
> +        unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
> +    }
> +}
> +
> +/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
> +pub fn devm_chip_alloc<'a>(

This should be a function of pwm::Chip rather than standalone.

> +    parent: &'a CoreDevice,

Since you're using devres, this must be a bound device, i.e. the parameter must
be &'a device::Device<device::Bound>, see also [2], which lands in the upcoming
merge window through the driver-core tree.

But I think this shouldn't use devm_pwmchip_alloc() anyways, see below.

[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n237

> +    npwm: u32,
> +    sizeof_priv: usize,
> +) -> Result<&'a mut Chip> {

Besides the above, this solution seems a bit half-baked, since it means that you
can only ever access the pwm::Chip as long as you have the &Device<Bound>,
which, given that you call this function typically from probe(), not beyond the
scope of probe().

This is because you return a reference which you can't save in the driver's
private data.

Instead you should use pwmchip_alloc() instead and make this return a real
instance of pwm::Chip and implement AlwaysRefCounted [3] for pwm::Chip.

You can then store the pwm::Chip instance in the Driver's private data, which
will only live until the driver is unbound, so it gives the same guarantees.

[3] https://rust.docs.kernel.org/kernel/types/trait.AlwaysRefCounted.html

> +    // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
> +    let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
> +    let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
> +    if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
> +        let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
> +        pr_err!("devm_pwmchip_alloc failed: {}\n", err);

You have the parent device, please use dev_err!(). But I don't think this needs
to error print at all.

> +        Err(Error::from_errno(err as i32))
> +    } else {
> +        // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
> +        Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
> +    }
> +}
> +
> +/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
> +pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
> +    // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
> +    unsafe {
> +        let chip_ptr = chip.as_ptr();
> +        // Assign the ops pointer directly to the C struct field
> +        (*chip_ptr).ops = ops.as_ptr();
> +        to_result(bindings::__pwmchip_add(
> +            chip_ptr,
> +            core::ptr::null_mut()
> +        ))
> +    }
> +}

How do you ensure to unregister the chip, once it was registered through this
function? I think this can cause UAF bugs. Instead you should wrap this in a
'Registration' structure, like we do everywhere else, see for example [4].

The structure should look like this:

	pub struct Registration(ARef<Chip>);

Registration::new() should register the chip and Registration::drop() should
unregister the chip.

[4] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/drm/driver.rs?ref_type=heads#L121

> +/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
> +pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
> +    // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
> +    unsafe {
> +        let chip_ptr = chip.as_ptr();
> +        // Assign the ops pointer directly to the C struct field
> +        (*chip_ptr).ops = ops.as_ptr();
> +        let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
> +        to_result(bindings::__devm_pwmchip_add(
> +            parent_ptr,
> +            chip_ptr,
> +            core::ptr::null_mut()
> +        ))
> +    }
> +}

This won't work anymore when creating a real pwm::Chip instance, since you can't
guarantee that the pwm::Chip still exists when devres will clean this up.

If you want devres to clean this up, you should make Registration::new() return
a Result<Devres<Registration>> instance.

This way the Registration keeps a reference to the pwm::Chip (giving the
guarantee of no potential UAF), and the Devres container ensures to drop the
Registration when the parent device is unbound internally.

If you want the exact same semantics as your current devm_chip_add(), but
without potential UAF, there is also Devres::new_foreign_owned() [5]. This way
Registration::new() will only return a Result.

[5] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html#method.new_foreign_owned

Re: [PATCH RFC 1/6] rust: Add basic PWM abstractions

[Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics]

W dniu 25.05.2025 o 13:49, Danilo Krummrich pisze:
> Hi Michal,
>
> On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..357fda46faa99c4462149658951ec53bf9cc2d1e
>> --- /dev/null
>> +++ b/rust/kernel/pwm.rs
>> @@ -0,0 +1,376 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! PWM (Pulse Width Modulator) abstractions.
>> +//!
>> +//! This module provides safe Rust abstractions for working with the Linux
>> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
>> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
>> +
>> +use crate::{
>> +    bindings,
>> +    device::Device as CoreDevice,
> I recommend referring the the base device as just device::Device, like we do it
> everywhere else where we have this name conflict.
>
> I'm not a huge fan of such aliases, since it's confusing when looking at patches
> that do not have the alias as context later on.
>
>> +    error::*,
>> +    prelude::*,
>> +    str::CStr,
>> +    types::{ForeignOwnable, Opaque},
>> +};
>> +use core::marker::PhantomData;
>> +
>> +/// PWM polarity. Mirrors `enum pwm_polarity`.
>> +#[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 From<bindings::pwm_polarity> for Polarity {
>> +    fn from(polarity: bindings::pwm_polarity) -> Self {
>> +        match polarity {
>> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
>> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
>> +            _ => {
>> +                pr_warn!(
>> +                    "Unknown pwm_polarity value {}, defaulting to Normal\n",
>> +                    polarity
>> +                );
>> +                Polarity::Normal
> Either Polarity::Normal is the correct thing to return in such a case, but then
> we do not need the pr_warn!(), or it is not, but then this should be a TryFrom
> impl instead.
>
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +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,
>> +        }
>> +    }
>> +}
>> +
>> +/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
>> +#[repr(transparent)]
>> +pub struct Args(Opaque<bindings::pwm_args>);
>> +
>> +impl Args {
>> +    /// Creates an `Args` wrapper from the C struct reference.
>> +    fn from_c_ref(c_args: &bindings::pwm_args) -> Self {
> I'd make this a pointer type instead, rather than having conversion to a
> reference at the caller's side.
>
>> +        // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
>> +        Args(Opaque::new(*c_args))
>> +    }
>> +
>> +    /// Returns the period of the PWM signal in nanoseconds.
>> +    pub fn period(&self) -> u64 {
>> +        // SAFETY: Reading from the valid pointer obtained by `get()`.
> Here and below, you should explain why this pointer is guaranteed to be valid
> instead.
>
>> +        unsafe { (*self.0.get()).period }
>> +    }
>> +
>> +    /// Returns the polarity of the PWM signal.
>> +    pub fn polarity(&self) -> Polarity {
>> +        // SAFETY: Reading from the valid pointer obtained by `get()`.
>> +        Polarity::from(unsafe { (*self.0.get()).polarity })
>> +    }
>> +}
>> +
>> +/// Wrapper for PWM state (`struct pwm_state`).
>> +#[repr(transparent)]
>> +pub struct State(Opaque<bindings::pwm_state>);
>> +
>> +impl State {
>> +    /// Creates a new zeroed `State`.
>> +    pub fn new() -> Self {
>> +        State(Opaque::new(bindings::pwm_state::default()))
>> +    }
>> +
>> +    /// Creates a `State` wrapper around a copy of a C `pwm_state`.
>> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
>> +        State(Opaque::new(c_state))
>> +    }
> You probably don't need Opaque here, given that you have instances of
> bindings::pwm_state already.
>
>> +
>> +    /// Creates a `State` wrapper around a reference to a C `pwm_state`.
>> +    fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
>> +        // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
>> +        unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
>> +    }
>> +
>> +    /// Gets the period of the PWM signal in nanoseconds.
>> +    pub fn period(&self) -> u64 {
>> +        unsafe { (*self.0.get()).period }
> This and all the methods below lack SAFETY comments, did you compile with
> CLIPPY=1? You should get lots of warnings reminding you to add them.
>
> <snip>
>
>> +
>> +/// Wrapper for a PWM device/channel (`struct pwm_device`).
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::pwm_device>);
>> +
>> +impl Device {
>> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
> We usually call this kind of function as_ref(), it'd be nice to stick to that
> for consistency.
>
>> +        unsafe { &mut *ptr.cast::<Self>() }
> A mutable reference -- why?
>
> <snip>
>
>> +/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
>> +#[repr(transparent)]
>> +pub struct Chip(Opaque<bindings::pwm_chip>);
>> +
>> +impl Chip {
>> +    /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
>> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
> Same here, for the name...
>
>> +        unsafe { &mut *ptr.cast::<Self>() }
> and the mutability.
>
>> +    }
>> +
>> +    /// Returns a raw pointer to the underlying `pwm_chip`.
>> +    pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
>> +        self.0.get()
>> +    }
>> +
>> +    /// Gets the number of PWM channels (hardware PWMs) on this chip.
>> +    pub fn npwm(&self) -> u32 {
>> +        unsafe { (*self.as_ptr()).npwm }
>> +    }
>> +
>> +    /// Returns `true` if the chip supports atomic operations for configuration.
>> +    pub fn is_atomic(&self) -> bool {
>> +        unsafe { (*self.as_ptr()).atomic }
>> +    }
>> +
>> +    /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
>> +    pub fn device(&self) -> &CoreDevice {
>> +        // SAFETY: `dev` field exists and points to the embedded device.
>> +        let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
>> +        unsafe { &*(dev_ptr as *mut CoreDevice) }
> Missing SAFETY comment, also please use cast() and cast_{const,mut}() instead.
>
>> +    }
>> +
>> +    /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
>> +    pub fn parent_device(&self) -> Option<&CoreDevice> {
>> +        // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
>> +        let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
>> +        if parent_ptr.is_null() {
>> +            None
>> +        } else {
>> +            // SAFETY: Pointer is non-null, assume valid device managed by kernel.
>> +            Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
>> +        }
> This can just be
>
> 	self.device().parent() // [1]
>
> which lands through the DRM tree in the upcoming merge window.
>
> [1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/device.rs?ref_type=heads#L72
>
>> +    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
>> +    pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {
> I will soon send a patch series that adds drvdata accessors to the generic
> Device abstraction.
>
> Anyways, no need for you to wait for this.
>
>> +        let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };
> Obviously, the C side calls this pwmchip_get_drvdata() and dev_get_drvdata(),
> however, I suggest not to use get_drvdata() as a method name, since 'get' is
> used in the context of reference counting (get/put) instead, and hence may be
> confusing. Let's just name this drvdata().
>
>> +        if ptr.is_null() {
>> +            None
>> +        } else {
>> +            unsafe { Some(&*(ptr as *const T)) }
>> +        }
>> +    }
>> +
>> +    /// Sets the *typed* driver-specific data associated with this chip's embedded device.
>> +    pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
>> +        unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
>> +    }
>> +}
>> +
>> +/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
>> +pub fn devm_chip_alloc<'a>(
> This should be a function of pwm::Chip rather than standalone.
>
>> +    parent: &'a CoreDevice,
> Since you're using devres, this must be a bound device, i.e. the parameter must
> be &'a device::Device<device::Bound>, see also [2], which lands in the upcoming
> merge window through the driver-core tree.
>
> But I think this shouldn't use devm_pwmchip_alloc() anyways, see below.
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n237
>
>> +    npwm: u32,
>> +    sizeof_priv: usize,
>> +) -> Result<&'a mut Chip> {
> Besides the above, this solution seems a bit half-baked, since it means that you
> can only ever access the pwm::Chip as long as you have the &Device<Bound>,
> which, given that you call this function typically from probe(), not beyond the
> scope of probe().
>
> This is because you return a reference which you can't save in the driver's
> private data.
>
> Instead you should use pwmchip_alloc() instead and make this return a real
> instance of pwm::Chip and implement AlwaysRefCounted [3] for pwm::Chip.
>
> You can then store the pwm::Chip instance in the Driver's private data, which
> will only live until the driver is unbound, so it gives the same guarantees.
>
> [3] https://rust.docs.kernel.org/kernel/types/trait.AlwaysRefCounted.html
>
>> +    // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
>> +    let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
>> +    let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
>> +    if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
>> +        let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
>> +        pr_err!("devm_pwmchip_alloc failed: {}\n", err);
> You have the parent device, please use dev_err!(). But I don't think this needs
> to error print at all.
>
>> +        Err(Error::from_errno(err as i32))
>> +    } else {
>> +        // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
>> +        Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
>> +    }
>> +}
>> +
>> +/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
>> +pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
>> +    // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
>> +    unsafe {
>> +        let chip_ptr = chip.as_ptr();
>> +        // Assign the ops pointer directly to the C struct field
>> +        (*chip_ptr).ops = ops.as_ptr();
>> +        to_result(bindings::__pwmchip_add(
>> +            chip_ptr,
>> +            core::ptr::null_mut()
>> +        ))
>> +    }
>> +}
> How do you ensure to unregister the chip, once it was registered through this
> function? I think this can cause UAF bugs. Instead you should wrap this in a
> 'Registration' structure, like we do everywhere else, see for example [4].
>
> The structure should look like this:
>
> 	pub struct Registration(ARef<Chip>);
>
> Registration::new() should register the chip and Registration::drop() should
> unregister the chip.
>
> [4] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/drm/driver.rs?ref_type=heads#L121
>
>> +/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
>> +pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
>> +    // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
>> +    unsafe {
>> +        let chip_ptr = chip.as_ptr();
>> +        // Assign the ops pointer directly to the C struct field
>> +        (*chip_ptr).ops = ops.as_ptr();
>> +        let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
>> +        to_result(bindings::__devm_pwmchip_add(
>> +            parent_ptr,
>> +            chip_ptr,
>> +            core::ptr::null_mut()
>> +        ))
>> +    }
>> +}
> This won't work anymore when creating a real pwm::Chip instance, since you can't
> guarantee that the pwm::Chip still exists when devres will clean this up.
>
> If you want devres to clean this up, you should make Registration::new() return
> a Result<Devres<Registration>> instance.
>
> This way the Registration keeps a reference to the pwm::Chip (giving the
> guarantee of no potential UAF), and the Devres container ensures to drop the
> Registration when the parent device is unbound internally.
>
> If you want the exact same semantics as your current devm_chip_add(), but
> without potential UAF, there is also Devres::new_foreign_owned() [5]. This way
> Registration::new() will only return a Result.
>
> [5] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html#method.new_foreign_owned

Hi Danilo,
Thank you very much for the detailed review. I took some time to digest 
it and everything you wrote
makes perfect sense. Will also make sure to compile with CLIPPY=1 next time.

Thanks !
Michał

>

Re: [PATCH RFC 1/6] rust: Add basic PWM abstractions

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

Hello Michal,

On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
> Introduce initial Rust abstractions for the Linux PWM subsystem. These
> abstractions provide safe wrappers around the core C data structures and
> functions, enabling the development of PWM chip drivers in Rust.

Oh wow, thanks for rustifying PWM. That might be my chance to actually
learn Rust.

I don't know when I will find the time to look into that in detail but
one thing I'd like to have for Rust support is that the drivers use the
new abstraction (i.e. .round_waveform_tohw() + .round_waveform_fromhw()
+ .read_waveform() + .write_waveform()) instead of the older .apply()
callback.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC 1/6] rust: Add basic PWM abstractions

[Michal Wilczynski]

On 5/26/25 09:53, Uwe Kleine-König wrote:
> Hello Michal,
> 
> On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
>> Introduce initial Rust abstractions for the Linux PWM subsystem. These
>> abstractions provide safe wrappers around the core C data structures and
>> functions, enabling the development of PWM chip drivers in Rust.
> 
> Oh wow, thanks for rustifying PWM. That might be my chance to actually
> learn Rust.
> 
> I don't know when I will find the time to look into that in detail but
> one thing I'd like to have for Rust support is that the drivers use the
> new abstraction (i.e. .round_waveform_tohw() + .round_waveform_fromhw()
> + .read_waveform() + .write_waveform()) instead of the older .apply()
> callback.

Hi Uwe,

Thanks for the valuable feedback. You're right, building on the newer
waveform abstractions is a correct approach.

I'll rework the patches to use .round_waveform_tohw(),
.round_waveform_fromhw(), .read_waveform(), and .write_waveform() as you
suggested, instead of the .apply() callback.

I appreciate you steering me in the right direction.

> 
> Best regards
> Uwe

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

[PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski]

Introduce a PWM driver for the T-HEAD TH1520 SoC written in Rust. It
utilizes the Rust PWM abstractions added in the previous commit.

The driver implements the standard PwmOps for the PWM framework,
supporting configuration of period, duty cycle, and polarity for the
TH1520's PWM channels. It uses devm managed resources for the PWM chip
itself and Rust DevRes for I/O memory. Clock management is handled using
Rust's RAII pattern.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS               |   1 +
 drivers/pwm/Kconfig       |   6 +
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm_th1520.rs | 272 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 280 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2b080e8f3d873b1e401b3a2fe1207c224c4591fc..0cfac73aea65076c5ccb50a25ea686fb86b472b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20986,6 +20986,7 @@ F:	drivers/mailbox/mailbox-th1520.c
 F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
 F:	drivers/pinctrl/pinctrl-th1520.c
 F:	drivers/pmdomain/thead/
+F:	drivers/pwm/pwm_th1520.rs
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 F:	include/dt-bindings/power/thead,th1520-power.h
 F:	include/linux/firmware/thead/thead,th1520-aon.h
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b5bd5c13b3a5e5a575a0fbfb2e285f5665b7a671..796fcd8343b7c8e30f62edc2e0fecf0e9b1ae20e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -684,6 +684,12 @@ config PWM_TEGRA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-tegra.
 
+config PWM_TH1520_RUST
+	tristate "TH1520 PWM support (Rust)"
+	depends on RUST_PWM_ABSTRACTIONS
+	help
+	  Generic PWM framework driver for TH1520 SoC.
+
 config PWM_TIECAP
 	tristate "ECAP PWM support"
 	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 539e0def3f82fcb866ab83a0346a15f7efdd7127..6890f860ada6f1a6ed43dd3a3a9584cd2fa877f3 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -70,3 +70,4 @@ obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
 obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
+obj-$(CONFIG_PWM_TH1520_RUST)	+= pwm_th1520.o
diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
--- /dev/null
+++ b/drivers/pwm/pwm_th1520.rs
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! Rust T-HEAD TH1520 PWM driver
+use kernel::{c_str, clk::Clk, device, io::mem::IoMem, of, platform, prelude::*, pwm, time};
+
+const MAX_PWM_NUM: u32 = 6;
+
+const fn th1520_pwm_chn_base(n: u32) -> u32 {
+    n * 0x20
+}
+const fn th1520_pwm_ctrl(n: u32) -> u32 {
+    th1520_pwm_chn_base(n) + 0x00
+}
+const fn th1520_pwm_per(n: u32) -> u32 {
+    th1520_pwm_chn_base(n) + 0x08
+}
+const fn th1520_pwm_fp(n: u32) -> u32 {
+    th1520_pwm_chn_base(n) + 0x0c
+}
+
+const PWM_START: u32 = 1 << 0;
+const PWM_CFG_UPDATE: u32 = 1 << 2;
+const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
+const PWM_FPOUT: u32 = 1 << 8;
+const PWM_INFACTOUT: u32 = 1 << 9;
+
+struct Th1520PwmChipData {
+    clk: Clk,
+    iomem: kernel::devres::Devres<IoMem<0>>,
+}
+
+impl Th1520PwmChipData {
+    fn _config(
+        &self,
+        hwpwm: u32,
+        duty_ns: u64,
+        period_ns: u64,
+        target_polarity: pwm::Polarity,
+    ) -> Result<u32> {
+        let regs = self.iomem.try_access().ok_or_else(|| {
+            pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm);
+            EBUSY
+        })?;
+
+        // Calculate cycle values
+        let rate_hz_u64 = self.clk.rate().as_hz() as u64;
+
+        if duty_ns > period_ns {
+            pr_err!(
+                "PWM-{}: Duty {}ns > period {}ns\n",
+                hwpwm,
+                duty_ns,
+                period_ns
+            );
+            return Err(EINVAL);
+        }
+        if period_ns == 0 {
+            pr_err!("PWM-{}: Period is zero\n", hwpwm);
+            return Err(EINVAL);
+        }
+
+        let mut period_cycle = mul_div_u64(period_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
+        if period_cycle > u32::MAX as u64 {
+            period_cycle = u32::MAX as u64;
+        }
+        if period_cycle == 0 {
+            pr_err!(
+                "PWM-{}: Calculated period_cycle is zero, not allowed by HW\n",
+                hwpwm
+            );
+            return Err(EINVAL);
+        }
+
+        let mut duty_cycle = mul_div_u64(duty_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
+        if duty_cycle > u32::MAX as u64 {
+            duty_cycle = u32::MAX as u64;
+        }
+
+        let mut base_ctrl_val = PWM_INFACTOUT | PWM_CONTINUOUS_MODE;
+        if target_polarity == pwm::Polarity::Normal {
+            // FPOUT=1 for Normal
+            base_ctrl_val |= PWM_FPOUT;
+        } else {
+            // Inversed, FPOUT=0
+            base_ctrl_val &= !PWM_FPOUT;
+        }
+        regs.try_write32(base_ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
+        pr_debug!(
+            "PWM-{}: _config: Initial CTRL write (polarity, mode): 0x{:x}\n",
+            hwpwm,
+            base_ctrl_val
+        );
+
+        // Write period and duty registers
+        regs.try_write32(period_cycle as u32, th1520_pwm_per(hwpwm) as usize)?;
+        regs.try_write32(duty_cycle as u32, th1520_pwm_fp(hwpwm) as usize)?;
+        pr_debug!(
+            "PWM-{}: _config: Period_cyc={}, Duty_cyc={}\n",
+            hwpwm,
+            period_cycle,
+            duty_cycle
+        );
+
+        // Apply period/duty by toggling CFG_UPDATE from 0 to 1.
+        // The `base_ctrl_val` (just written to HW) has CFG_UPDATE=0. Now set it.
+        let ctrl_val_for_update = base_ctrl_val | PWM_CFG_UPDATE;
+        regs.try_write32(ctrl_val_for_update, th1520_pwm_ctrl(hwpwm) as usize)?;
+        pr_debug!(
+            "PWM-{}: _config: CTRL write with CFG_UPDATE: 0x{:x}\n",
+            hwpwm,
+            ctrl_val_for_update
+        );
+
+        Ok(ctrl_val_for_update)
+    }
+
+    fn _enable(&self, hwpwm: u32, ctrl_val_after_config: u32) -> Result {
+        let regs = self.iomem.try_access().ok_or_else(|| {
+            pr_err!("PWM-{}: Failed to access I/O memory in _enable\n", hwpwm);
+            EBUSY
+        })?;
+
+        // ctrl_val_after_config already has mode, polarity, and CFG_UPDATE correctly set.
+        // Now add the START bit. START bit auto-clears.
+        let ctrl_to_start = ctrl_val_after_config | PWM_START;
+        regs.try_write32(ctrl_to_start, th1520_pwm_ctrl(hwpwm) as usize)?;
+        pr_debug!(
+            "PWM-{}: _enable: CTRL write with START: 0x{:x}\n",
+            hwpwm,
+            ctrl_to_start
+        );
+        Ok(())
+    }
+
+    fn _disable(&self, hwpwm: u32) -> Result<()> {
+        let regs = self.iomem.try_access().ok_or_else(|| {
+            pr_err!("PWM-{}: Failed to access I/O memory in _disable\n", hwpwm);
+            EINVAL
+        })?;
+
+        let mut ctrl_val = regs.try_read32(th1520_pwm_ctrl(hwpwm) as usize)?;
+        pr_debug!("PWM-{}: _disable: Read CTRL: 0x{:x}\n", hwpwm, ctrl_val);
+
+        // Ensure CFG_UPDATE is 0 before updating duty (Limitation #4)
+        if (ctrl_val & PWM_CFG_UPDATE) != 0 {
+            ctrl_val &= !PWM_CFG_UPDATE;
+            regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
+            pr_debug!(
+                "PWM-{}: _disable: Cleared CFG_UPDATE, wrote CTRL: 0x{:x}\n",
+                hwpwm,
+                ctrl_val
+            );
+        }
+
+        // Set duty cycle to 0
+        regs.try_write32(0, th1520_pwm_fp(hwpwm) as usize)?;
+        pr_debug!("PWM-{}: _disable: Wrote 0 to DUTY (FP) register\n", hwpwm);
+
+        // Apply the 0% duty by toggling CFG_UPDATE from 0 to 1
+        // Use the ctrl_val that has CFG_UPDATE cleared (or was already clear)
+        ctrl_val |= PWM_CFG_UPDATE;
+        regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
+        pr_debug!(
+            "PWM-{}: _disable: Set CFG_UPDATE, wrote CTRL: 0x{:x}\n",
+            hwpwm,
+            ctrl_val
+        );
+
+        Ok(())
+    }
+}
+
+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(())
+    }
+}
+
+impl Drop for Th1520PwmChipData {
+    fn drop(&mut self) {
+        self.clk.disable_unprepare();
+    }
+}
+
+fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
+    if c == 0 {
+        return 0;
+    }
+    a.wrapping_mul(b) / c
+}
+
+static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmChipData>();
+
+struct Th1520PwmPlatformDriver;
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
+    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
+);
+
+impl platform::Driver for Th1520PwmPlatformDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(
+        pdev: &platform::Device<device::Core>,
+        _id_info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
+        let resource = pdev.resource(0).ok_or(ENODEV)?;
+        let iomem = pdev.ioremap_resource(&resource)?;
+
+        let clk = Clk::get(pdev.as_ref(), None)?;
+
+        clk.prepare_enable()?;
+        let driver_data = KBox::new(Th1520PwmChipData { clk, iomem }, GFP_KERNEL)?;
+        let pwm_chip = pwm::devm_chip_alloc(pdev.as_ref(), MAX_PWM_NUM, 0)?;
+
+        let result = pwm::devm_chip_add(pdev.as_ref(), pwm_chip, &TH1520_PWM_OPS);
+        if result.is_err() {
+            pr_err!("Failed to add PWM chip: {:?}\n", result);
+            return Err(EIO);
+        }
+
+        pwm_chip.set_drvdata(driver_data);
+        pr_info!("T-HEAD TH1520 PWM probed correctly\n");
+
+        Ok(KBox::new(Self, GFP_KERNEL)?.into())
+    }
+}
+
+kernel::module_platform_driver! {
+    type: Th1520PwmPlatformDriver,
+    name: "pwm_th1520",
+    author: "Michal Wilczynski",
+    description: "T-HEAD TH1520 PWM driver",
+    license: "GPL v2",
+}

-- 
2.34.1

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Danilo Krummrich]

On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +use kernel::{c_st
> +
> +struct Th1520PwmChipData {
> +    clk: Clk,
> +    iomem: kernel::devres::Devres<IoMem<0>>,

Why IoMem<0>? If you put the expected memory region size for this chip instead
all your subsequent accesses can be iomem.write() / iomem.read() rather than the
fallible try_{read,write}() variants.

> +impl Th1520PwmChipData {
> +    fn _config(
> +        &self,
> +        hwpwm: u32,
> +        duty_ns: u64,
> +        period_ns: u64,
> +        target_polarity: pwm::Polarity,
> +    ) -> Result<u32> {
> +        let regs = self.iomem.try_access().ok_or_else(|| {
> +            pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm);

Here and throughout the whole driver, please use the dev_*!() print macros.
Drivers have no reason to use the pr_*!() macros.

> +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 {

I assume those callbacks can't race with pwmchip_remove() called from driver
remove()? I.e. the callbacks are guaranteed to complete before pwmchip_remove()
completes?

If so, this function signature can provide the parent device of the pwm::Chip as
device::Device<device::Bound> reference.

This would allow you to access iomem more efficiently.

Instead of

	data.iomem.try_access()

you could do

	data.iomem.access(parent) // [1]

which does get you rid of the atomic check and the RCU read side critical
section implied by try_access().

Actually, I should have added this comment and explanation to the abstraction
patch, but forgot about it. :)

[1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/devres.rs?ref_type=heads#L213

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics]

W dniu 25.05.2025 o 14:03, Danilo Krummrich pisze:
> On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,272 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>> +use kernel::{c_st
>> +
>> +struct Th1520PwmChipData {
>> +    clk: Clk,
>> +    iomem: kernel::devres::Devres<IoMem<0>>,
> Why IoMem<0>? If you put the expected memory region size for this chip instead
> all your subsequent accesses can be iomem.write() / iomem.read() rather than the
> fallible try_{read,write}() variants.
The size of the memory region is not known at the compile time. Instead 
it's configured
via Device Tree. I'm not sure why it should work differently in Rust ?
>
>> +impl Th1520PwmChipData {
>> +    fn _config(
>> +        &self,
>> +        hwpwm: u32,
>> +        duty_ns: u64,
>> +        period_ns: u64,
>> +        target_polarity: pwm::Polarity,
>> +    ) -> Result<u32> {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm);
> Here and throughout the whole driver, please use the dev_*!() print macros.
> Drivers have no reason to use the pr_*!() macros.
>
>> +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 {
> I assume those callbacks can't race with pwmchip_remove() called from driver
> remove()? I.e. the callbacks are guaranteed to complete before pwmchip_remove()
> completes?

Yeah this is my understanding as well - this is something that the PWM 
core should
guarantee. Fairly recently there was a commit adding even more locking
1cc2e1faafb3 ("pwm: Add more locking")

>
> If so, this function signature can provide the parent device of the pwm::Chip as
> device::Device<device::Bound> reference.
>
> This would allow you to access iomem more efficiently.
>
> Instead of
>
> 	data.iomem.try_access()
>
> you could do
>
> 	data.iomem.access(parent) // [1]
>
> which does get you rid of the atomic check and the RCU read side critical
> section implied by try_access().
>
> Actually, I should have added this comment and explanation to the abstraction
> patch, but forgot about it. :)

Thanks ! Appreciate your review !
Michał

>
> [1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/devres.rs?ref_type=heads#L213
>

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Danilo Krummrich]

On Tue, May 27, 2025 at 02:44:57PM +0200, Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics wrote:
> W dniu 25.05.2025 o 14:03, Danilo Krummrich pisze:
> > On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
> >> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
> >> --- /dev/null
> >> +++ b/drivers/pwm/pwm_th1520.rs
> >> @@ -0,0 +1,272 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> >> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> >> +
> >> +//! Rust T-HEAD TH1520 PWM driver
> >> +use kernel::{c_st
> >> +
> >> +struct Th1520PwmChipData {
> >> +    clk: Clk,
> >> +    iomem: kernel::devres::Devres<IoMem<0>>,
> > Why IoMem<0>? If you put the expected memory region size for this chip instead
> > all your subsequent accesses can be iomem.write() / iomem.read() rather than the
> > fallible try_{read,write}() variants.
> The size of the memory region is not known at the compile time. Instead 
> it's configured
> via Device Tree. I'm not sure why it should work differently in Rust ?

There are two sizes:

  (1) The size of the actual MMIO region, which comes from the device-tree.
  (2) The size of the MMIO region that the driver knows it requires to work.

Let's say your driver uses registers with the following offsets.

REG0_OFFSET = 0x0
REG1_OFFSET = 0x4
REG2_OFFSET = 0x100

This means that the size of (2) is 0x100 + width of REG2 (let's say 0x4), which
means that you can safely define your MMIO memory type as IoMem<0x104>.

If you subsequently call pdev.ioremap_resource_sized() it will fail on runtime
if the MMIO region defined in the device-tree does not have a size of at least
0x104, i.e. (1) < (2). That's not a problem because if (1) < (2) your driver
can't work anyways.

In return, you can call the non-try variant of the read / write methods of
IoMem, which do boundary checks on compile time and hence are infallible.

Note that this does not prevent you to still call the try variants of read /
write in case you also have to deal with dynamic offsets that are not known at
compile time.

I hope this helps.

- Danilo

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

Hello,

On Tue, May 27, 2025 at 02:44:57PM +0200, Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics wrote:
> W dniu 25.05.2025 o 14:03, Danilo Krummrich pisze:
> > On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
> >> +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 {
> > I assume those callbacks can't race with pwmchip_remove() called from driver
> > remove()? I.e. the callbacks are guaranteed to complete before pwmchip_remove()
> > completes?
> 
> Yeah this is my understanding as well - this is something that the PWM 
> core should
> guarantee. Fairly recently there was a commit adding even more locking
> 1cc2e1faafb3 ("pwm: Add more locking")

I confirm that starting with that commit pwmchip_remove() and the driver
callbacks are properly serialized. Before that this an issue, even
though it was hard to hit. (Because if you triggered the callbacks via
sysfs the locking in sysfs code implicitly worked around the problems.)

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 13397 bytes --]

Hello,

I don't speak Rust, so please double-check all my feedback if it really
applies.

On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC written in Rust. It
> utilizes the Rust PWM abstractions added in the previous commit.
> 
> The driver implements the standard PwmOps for the PWM framework,
> supporting configuration of period, duty cycle, and polarity for the
> TH1520's PWM channels. It uses devm managed resources for the PWM chip
> itself and Rust DevRes for I/O memory. Clock management is handled using
> Rust's RAII pattern.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |   6 +
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm_th1520.rs | 272 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 280 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2b080e8f3d873b1e401b3a2fe1207c224c4591fc..0cfac73aea65076c5ccb50a25ea686fb86b472b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20986,6 +20986,7 @@ F:	drivers/mailbox/mailbox-th1520.c
>  F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>  F:	drivers/pinctrl/pinctrl-th1520.c
>  F:	drivers/pmdomain/thead/
> +F:	drivers/pwm/pwm_th1520.rs
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>  F:	include/dt-bindings/power/thead,th1520-power.h
>  F:	include/linux/firmware/thead/thead,th1520-aon.h
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b5bd5c13b3a5e5a575a0fbfb2e285f5665b7a671..796fcd8343b7c8e30f62edc2e0fecf0e9b1ae20e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -684,6 +684,12 @@ config PWM_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tegra.
>  
> +config PWM_TH1520_RUST
> +	tristate "TH1520 PWM support (Rust)"
> +	depends on RUST_PWM_ABSTRACTIONS
> +	help
> +	  Generic PWM framework driver for TH1520 SoC.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 539e0def3f82fcb866ab83a0346a15f7efdd7127..6890f860ada6f1a6ed43dd3a3a9584cd2fa877f3 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -70,3 +70,4 @@ obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
>  obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> +obj-$(CONFIG_PWM_TH1520_RUST)	+= pwm_th1520.o
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +use kernel::{c_str, clk::Clk, device, io::mem::IoMem, of, platform, prelude::*, pwm, time};
> +
> +const MAX_PWM_NUM: u32 = 6;
> +
> +const fn th1520_pwm_chn_base(n: u32) -> u32 {
> +    n * 0x20
> +}
> +const fn th1520_pwm_ctrl(n: u32) -> u32 {
> +    th1520_pwm_chn_base(n) + 0x00
> +}
> +const fn th1520_pwm_per(n: u32) -> u32 {
> +    th1520_pwm_chn_base(n) + 0x08
> +}
> +const fn th1520_pwm_fp(n: u32) -> u32 {
> +    th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +const PWM_START: u32 = 1 << 0;
> +const PWM_CFG_UPDATE: u32 = 1 << 2;
> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const PWM_FPOUT: u32 = 1 << 8;
> +const PWM_INFACTOUT: u32 = 1 << 9;
> +
> +struct Th1520PwmChipData {
> +    clk: Clk,
> +    iomem: kernel::devres::Devres<IoMem<0>>,
> +}
> +
> +impl Th1520PwmChipData {
> +    fn _config(
> +        &self,
> +        hwpwm: u32,
> +        duty_ns: u64,
> +        period_ns: u64,
> +        target_polarity: pwm::Polarity,

Why "target_polarity"? duty_ns and period_ns also don't contain
"target_"?

> +    ) -> Result<u32> {
> +        let regs = self.iomem.try_access().ok_or_else(|| {
> +            pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm);
> +            EBUSY
> +        })?;
> +
> +        // Calculate cycle values
> +        let rate_hz_u64 = self.clk.rate().as_hz() as u64;
> +
> +        if duty_ns > period_ns {
> +            pr_err!(
> +                "PWM-{}: Duty {}ns > period {}ns\n",
> +                hwpwm,
> +                duty_ns,
> +                period_ns
> +            );
> +            return Err(EINVAL);
> +        }
> +        if period_ns == 0 {
> +            pr_err!("PWM-{}: Period is zero\n", hwpwm);
> +            return Err(EINVAL);
> +        }

You don't need to check for period_ns == 0 explicitly. This case then
hits period_cycle == 0 below.

> +
> +        let mut period_cycle = mul_div_u64(period_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);

if period_ns is big and rate_hz_u64 > NSEC_PER_SEC this might overflow.

Typically refuse to probe if rate_hz_u64 > NSEC_PER_SEC and call
clk_rate_exclusive_get().

> +        if period_cycle > u32::MAX as u64 {
> +            period_cycle = u32::MAX as u64;
> +        }
> +        if period_cycle == 0 {
> +            pr_err!(
> +                "PWM-{}: Calculated period_cycle is zero, not allowed by HW\n",
> +                hwpwm
> +            );
> +            return Err(EINVAL);
> +        }
> +
> +        let mut duty_cycle = mul_div_u64(duty_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
> +        if duty_cycle > u32::MAX as u64 {
> +            duty_cycle = u32::MAX as u64;
> +        }
> +
> +        let mut base_ctrl_val = PWM_INFACTOUT | PWM_CONTINUOUS_MODE;
> +        if target_polarity == pwm::Polarity::Normal {
> +            // FPOUT=1 for Normal
> +            base_ctrl_val |= PWM_FPOUT;
> +        } else {
> +            // Inversed, FPOUT=0
> +            base_ctrl_val &= !PWM_FPOUT;
> +        }
> +        regs.try_write32(base_ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
> +        pr_debug!(
> +            "PWM-{}: _config: Initial CTRL write (polarity, mode): 0x{:x}\n",
> +            hwpwm,
> +            base_ctrl_val
> +        );
> +
> +        // Write period and duty registers
> +        regs.try_write32(period_cycle as u32, th1520_pwm_per(hwpwm) as usize)?;
> +        regs.try_write32(duty_cycle as u32, th1520_pwm_fp(hwpwm) as usize)?;
> +        pr_debug!(
> +            "PWM-{}: _config: Period_cyc={}, Duty_cyc={}\n",
> +            hwpwm,
> +            period_cycle,
> +            duty_cycle
> +        );
> +
> +        // Apply period/duty by toggling CFG_UPDATE from 0 to 1.
> +        // The `base_ctrl_val` (just written to HW) has CFG_UPDATE=0. Now set it.
> +        let ctrl_val_for_update = base_ctrl_val | PWM_CFG_UPDATE;
> +        regs.try_write32(ctrl_val_for_update, th1520_pwm_ctrl(hwpwm) as usize)?;
> +        pr_debug!(
> +            "PWM-{}: _config: CTRL write with CFG_UPDATE: 0x{:x}\n",
> +            hwpwm,
> +            ctrl_val_for_update
> +        );
> +
> +        Ok(ctrl_val_for_update)
> +    }
> +
> +    fn _enable(&self, hwpwm: u32, ctrl_val_after_config: u32) -> Result {
> +        let regs = self.iomem.try_access().ok_or_else(|| {
> +            pr_err!("PWM-{}: Failed to access I/O memory in _enable\n", hwpwm);
> +            EBUSY
> +        })?;
> +
> +        // ctrl_val_after_config already has mode, polarity, and CFG_UPDATE correctly set.
> +        // Now add the START bit. START bit auto-clears.
> +        let ctrl_to_start = ctrl_val_after_config | PWM_START;
> +        regs.try_write32(ctrl_to_start, th1520_pwm_ctrl(hwpwm) as usize)?;
> +        pr_debug!(
> +            "PWM-{}: _enable: CTRL write with START: 0x{:x}\n",
> +            hwpwm,
> +            ctrl_to_start
> +        );
> +        Ok(())
> +    }
> +
> +    fn _disable(&self, hwpwm: u32) -> Result<()> {
> +        let regs = self.iomem.try_access().ok_or_else(|| {
> +            pr_err!("PWM-{}: Failed to access I/O memory in _disable\n", hwpwm);
> +            EINVAL
> +        })?;
> +
> +        let mut ctrl_val = regs.try_read32(th1520_pwm_ctrl(hwpwm) as usize)?;
> +        pr_debug!("PWM-{}: _disable: Read CTRL: 0x{:x}\n", hwpwm, ctrl_val);
> +
> +        // Ensure CFG_UPDATE is 0 before updating duty (Limitation #4)
> +        if (ctrl_val & PWM_CFG_UPDATE) != 0 {
> +            ctrl_val &= !PWM_CFG_UPDATE;
> +            regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
> +            pr_debug!(
> +                "PWM-{}: _disable: Cleared CFG_UPDATE, wrote CTRL: 0x{:x}\n",
> +                hwpwm,
> +                ctrl_val
> +            );
> +        }
> +
> +        // Set duty cycle to 0
> +        regs.try_write32(0, th1520_pwm_fp(hwpwm) as usize)?;
> +        pr_debug!("PWM-{}: _disable: Wrote 0 to DUTY (FP) register\n", hwpwm);
> +
> +        // Apply the 0% duty by toggling CFG_UPDATE from 0 to 1
> +        // Use the ctrl_val that has CFG_UPDATE cleared (or was already clear)
> +        ctrl_val |= PWM_CFG_UPDATE;
> +        regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
> +        pr_debug!(
> +            "PWM-{}: _disable: Set CFG_UPDATE, wrote CTRL: 0x{:x}\n",
> +            hwpwm,
> +            ctrl_val
> +        );
> +
> +        Ok(())
> +    }
> +}
> +
> +impl pwm::PwmOps for Th1520PwmChipData {
> +    // This driver implements get_state

I don't spot the get_state implementation?!

> +    fn apply(
> +        pwm_chip_ref: &mut pwm::Chip,
> +        pwm_dev: &mut pwm::Device,
> +        target_state: &pwm::State,

In C code I like these variables be called "chip", "pwm" and "state"
respectively. Is that possible here, too?

> +    ) -> 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(())
> +    }
> +}
> +
> +impl Drop for Th1520PwmChipData {
> +    fn drop(&mut self) {
> +        self.clk.disable_unprepare();
> +    }
> +}
> +
> +fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
> +    if c == 0 {
> +        return 0;
> +    }
> +    a.wrapping_mul(b) / c
> +}

Is this save if a * b > U64_MAX? I would have expected such a function
to already exist in generic code.

> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmChipData>();
> +
> +struct Th1520PwmPlatformDriver;
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
> +    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
> +);
> +
> +impl platform::Driver for Th1520PwmPlatformDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<device::Core>,
> +        _id_info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let resource = pdev.resource(0).ok_or(ENODEV)?;
> +        let iomem = pdev.ioremap_resource(&resource)?;
> +
> +        let clk = Clk::get(pdev.as_ref(), None)?;
> +
> +        clk.prepare_enable()?;

Is there something like devm_clk_get_enabled() such that the drop
function becomes redundant?

> +        let driver_data = KBox::new(Th1520PwmChipData { clk, iomem }, GFP_KERNEL)?;
> +        let pwm_chip = pwm::devm_chip_alloc(pdev.as_ref(), MAX_PWM_NUM, 0)?;
> +
> +        let result = pwm::devm_chip_add(pdev.as_ref(), pwm_chip, &TH1520_PWM_OPS);
> +        if result.is_err() {
> +            pr_err!("Failed to add PWM chip: {:?}\n", result);
> +            return Err(EIO);
> +        }
> +
> +        pwm_chip.set_drvdata(driver_data);
> +        pr_info!("T-HEAD TH1520 PWM probed correctly\n");

Please degrade to pr_debug. Each driver emitting a message is an
annoyance.

> +        Ok(KBox::new(Self, GFP_KERNEL)?.into())
> +    }
> +}
> +
> +kernel::module_platform_driver! {
> +    type: Th1520PwmPlatformDriver,
> +    name: "pwm_th1520",
> +    author: "Michal Wilczynski",
> +    description: "T-HEAD TH1520 PWM driver",
> +    license: "GPL v2",
> +}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski]

On 6/5/25 12:39, Uwe Kleine-König wrote:
> Hello,
> 
> I don't speak Rust, so please double-check all my feedback if it really
> applies.

No problem, appreciate your time and experience with PWM subsystem !

> 
> On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC written in Rust. It
>> utilizes the Rust PWM abstractions added in the previous commit.
>>
>> The driver implements the standard PwmOps for the PWM framework,
>> supporting configuration of period, duty cycle, and polarity for the
>> TH1520's PWM channels. It uses devm managed resources for the PWM chip
>> itself and Rust DevRes for I/O memory. Clock management is handled using
>> Rust's RAII pattern.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  MAINTAINERS               |   1 +
>>  drivers/pwm/Kconfig       |   6 +
>>  drivers/pwm/Makefile      |   1 +
>>  drivers/pwm/pwm_th1520.rs | 272 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 280 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2b080e8f3d873b1e401b3a2fe1207c224c4591fc..0cfac73aea65076c5ccb50a25ea686fb86b472b8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20986,6 +20986,7 @@ F:	drivers/mailbox/mailbox-th1520.c
>>  F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>>  F:	drivers/pinctrl/pinctrl-th1520.c
>>  F:	drivers/pmdomain/thead/
>> +F:	drivers/pwm/pwm_th1520.rs
>>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>>  F:	include/dt-bindings/power/thead,th1520-power.h
>>  F:	include/linux/firmware/thead/thead,th1520-aon.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index b5bd5c13b3a5e5a575a0fbfb2e285f5665b7a671..796fcd8343b7c8e30f62edc2e0fecf0e9b1ae20e 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -684,6 +684,12 @@ config PWM_TEGRA
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-tegra.
>>  
>> +config PWM_TH1520_RUST
>> +	tristate "TH1520 PWM support (Rust)"
>> +	depends on RUST_PWM_ABSTRACTIONS
>> +	help
>> +	  Generic PWM framework driver for TH1520 SoC.
>> +
>>  config PWM_TIECAP
>>  	tristate "ECAP PWM support"
>>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 539e0def3f82fcb866ab83a0346a15f7efdd7127..6890f860ada6f1a6ed43dd3a3a9584cd2fa877f3 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -70,3 +70,4 @@ obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
>>  obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
>> +obj-$(CONFIG_PWM_TH1520_RUST)	+= pwm_th1520.o
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,272 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>> +use kernel::{c_str, clk::Clk, device, io::mem::IoMem, of, platform, prelude::*, pwm, time};
>> +
>> +const MAX_PWM_NUM: u32 = 6;
>> +
>> +const fn th1520_pwm_chn_base(n: u32) -> u32 {
>> +    n * 0x20
>> +}
>> +const fn th1520_pwm_ctrl(n: u32) -> u32 {
>> +    th1520_pwm_chn_base(n) + 0x00
>> +}
>> +const fn th1520_pwm_per(n: u32) -> u32 {
>> +    th1520_pwm_chn_base(n) + 0x08
>> +}
>> +const fn th1520_pwm_fp(n: u32) -> u32 {
>> +    th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +const PWM_START: u32 = 1 << 0;
>> +const PWM_CFG_UPDATE: u32 = 1 << 2;
>> +const PWM_CONTINUOUS_MODE: u32 = 1 << 5;
>> +const PWM_FPOUT: u32 = 1 << 8;
>> +const PWM_INFACTOUT: u32 = 1 << 9;
>> +
>> +struct Th1520PwmChipData {
>> +    clk: Clk,
>> +    iomem: kernel::devres::Devres<IoMem<0>>,
>> +}
>> +
>> +impl Th1520PwmChipData {
>> +    fn _config(
>> +        &self,
>> +        hwpwm: u32,
>> +        duty_ns: u64,
>> +        period_ns: u64,
>> +        target_polarity: pwm::Polarity,
> 
> Why "target_polarity"? duty_ns and period_ns also don't contain
> "target_"?

You're right, that was an inconsistent name. I've since re-worked the
driver to use the modern waveform API as you suggested in the other
thread. In the new version, this function no longer exists, and the new
round_waveform_tohw op takes a &pwm::Waveform parameter, which resolves
this naming issue. Thank you for pointing it out.

> 
>> +    ) -> Result<u32> {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm);
>> +            EBUSY
>> +        })?;
>> +
>> +        // Calculate cycle values
>> +        let rate_hz_u64 = self.clk.rate().as_hz() as u64;
>> +
>> +        if duty_ns > period_ns {
>> +            pr_err!(
>> +                "PWM-{}: Duty {}ns > period {}ns\n",
>> +                hwpwm,
>> +                duty_ns,
>> +                period_ns
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +        if period_ns == 0 {
>> +            pr_err!("PWM-{}: Period is zero\n", hwpwm);
>> +            return Err(EINVAL);
>> +        }
> 
> You don't need to check for period_ns == 0 explicitly. This case then
> hits period_cycle == 0 below.
> 
>> +
>> +        let mut period_cycle = mul_div_u64(period_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
> 
> if period_ns is big and rate_hz_u64 > NSEC_PER_SEC this might overflow.
> 
> Typically refuse to probe if rate_hz_u64 > NSEC_PER_SEC and call
> clk_rate_exclusive_get().
> 
>> +        if period_cycle > u32::MAX as u64 {
>> +            period_cycle = u32::MAX as u64;
>> +        }
>> +        if period_cycle == 0 {
>> +            pr_err!(
>> +                "PWM-{}: Calculated period_cycle is zero, not allowed by HW\n",
>> +                hwpwm
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let mut duty_cycle = mul_div_u64(duty_ns, rate_hz_u64, time::NSEC_PER_SEC as u64);
>> +        if duty_cycle > u32::MAX as u64 {
>> +            duty_cycle = u32::MAX as u64;
>> +        }
>> +
>> +        let mut base_ctrl_val = PWM_INFACTOUT | PWM_CONTINUOUS_MODE;
>> +        if target_polarity == pwm::Polarity::Normal {
>> +            // FPOUT=1 for Normal
>> +            base_ctrl_val |= PWM_FPOUT;
>> +        } else {
>> +            // Inversed, FPOUT=0
>> +            base_ctrl_val &= !PWM_FPOUT;
>> +        }
>> +        regs.try_write32(base_ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _config: Initial CTRL write (polarity, mode): 0x{:x}\n",
>> +            hwpwm,
>> +            base_ctrl_val
>> +        );
>> +
>> +        // Write period and duty registers
>> +        regs.try_write32(period_cycle as u32, th1520_pwm_per(hwpwm) as usize)?;
>> +        regs.try_write32(duty_cycle as u32, th1520_pwm_fp(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _config: Period_cyc={}, Duty_cyc={}\n",
>> +            hwpwm,
>> +            period_cycle,
>> +            duty_cycle
>> +        );
>> +
>> +        // Apply period/duty by toggling CFG_UPDATE from 0 to 1.
>> +        // The `base_ctrl_val` (just written to HW) has CFG_UPDATE=0. Now set it.
>> +        let ctrl_val_for_update = base_ctrl_val | PWM_CFG_UPDATE;
>> +        regs.try_write32(ctrl_val_for_update, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _config: CTRL write with CFG_UPDATE: 0x{:x}\n",
>> +            hwpwm,
>> +            ctrl_val_for_update
>> +        );
>> +
>> +        Ok(ctrl_val_for_update)
>> +    }
>> +
>> +    fn _enable(&self, hwpwm: u32, ctrl_val_after_config: u32) -> Result {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _enable\n", hwpwm);
>> +            EBUSY
>> +        })?;
>> +
>> +        // ctrl_val_after_config already has mode, polarity, and CFG_UPDATE correctly set.
>> +        // Now add the START bit. START bit auto-clears.
>> +        let ctrl_to_start = ctrl_val_after_config | PWM_START;
>> +        regs.try_write32(ctrl_to_start, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _enable: CTRL write with START: 0x{:x}\n",
>> +            hwpwm,
>> +            ctrl_to_start
>> +        );
>> +        Ok(())
>> +    }
>> +
>> +    fn _disable(&self, hwpwm: u32) -> Result<()> {
>> +        let regs = self.iomem.try_access().ok_or_else(|| {
>> +            pr_err!("PWM-{}: Failed to access I/O memory in _disable\n", hwpwm);
>> +            EINVAL
>> +        })?;
>> +
>> +        let mut ctrl_val = regs.try_read32(th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!("PWM-{}: _disable: Read CTRL: 0x{:x}\n", hwpwm, ctrl_val);
>> +
>> +        // Ensure CFG_UPDATE is 0 before updating duty (Limitation #4)
>> +        if (ctrl_val & PWM_CFG_UPDATE) != 0 {
>> +            ctrl_val &= !PWM_CFG_UPDATE;
>> +            regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +            pr_debug!(
>> +                "PWM-{}: _disable: Cleared CFG_UPDATE, wrote CTRL: 0x{:x}\n",
>> +                hwpwm,
>> +                ctrl_val
>> +            );
>> +        }
>> +
>> +        // Set duty cycle to 0
>> +        regs.try_write32(0, th1520_pwm_fp(hwpwm) as usize)?;
>> +        pr_debug!("PWM-{}: _disable: Wrote 0 to DUTY (FP) register\n", hwpwm);
>> +
>> +        // Apply the 0% duty by toggling CFG_UPDATE from 0 to 1
>> +        // Use the ctrl_val that has CFG_UPDATE cleared (or was already clear)
>> +        ctrl_val |= PWM_CFG_UPDATE;
>> +        regs.try_write32(ctrl_val, th1520_pwm_ctrl(hwpwm) as usize)?;
>> +        pr_debug!(
>> +            "PWM-{}: _disable: Set CFG_UPDATE, wrote CTRL: 0x{:x}\n",
>> +            hwpwm,
>> +            ctrl_val
>> +        );
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl pwm::PwmOps for Th1520PwmChipData {
>> +    // This driver implements get_state
> 
> I don't spot the get_state implementation?!

That's a good catch, thank you. You are correct, the comment was ahead
of the implementation in this RFC version. My intention was to keep the
initial patch focused. I have now added the full get_state
implementation for v2.

> 
>> +    fn apply(
>> +        pwm_chip_ref: &mut pwm::Chip,
>> +        pwm_dev: &mut pwm::Device,
>> +        target_state: &pwm::State,
> 
> In C code I like these variables be called "chip", "pwm" and "state"
> respectively. Is that possible here, too?

Absolutely. That makes sense for consistency with the C codebase. In the
new waveform based version, I've ensured the PwmOps parameters are named
simply chip, pwm to align with standard practice.

> 
>> +    ) -> 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(())
>> +    }
>> +}
>> +
>> +impl Drop for Th1520PwmChipData {
>> +    fn drop(&mut self) {
>> +        self.clk.disable_unprepare();
>> +    }
>> +}
>> +
>> +fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
>> +    if c == 0 {
>> +        return 0;
>> +    }
>> +    a.wrapping_mul(b) / c
>> +}
> 
> Is this save if a * b > U64_MAX? I would have expected such a function
> to already exist in generic code.

You're right, thank you. The wrapping_mul is unsafe due to the overflow
risk you pointed out.

The ideal solution would be to use the kernel's own mul_u64_u64_div_u64
helper function.

Rust maintainers: This binding doesn't seem to be available yet. Would a
preparatory patch introducing a minimal rust/kernel/math.rs with  only
this binding be the best way to proceed? It seems like a useful utility
for more than just this driver.

> 
>> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmChipData>();
>> +
>> +struct Th1520PwmPlatformDriver;
>> +
>> +kernel::of_device_table!(
>> +    OF_TABLE,
>> +    MODULE_OF_TABLE,
>> +    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
>> +    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
>> +);
>> +
>> +impl platform::Driver for Th1520PwmPlatformDriver {
>> +    type IdInfo = ();
>> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
>> +
>> +    fn probe(
>> +        pdev: &platform::Device<device::Core>,
>> +        _id_info: Option<&Self::IdInfo>,
>> +    ) -> Result<Pin<KBox<Self>>> {
>> +        let resource = pdev.resource(0).ok_or(ENODEV)?;
>> +        let iomem = pdev.ioremap_resource(&resource)?;
>> +
>> +        let clk = Clk::get(pdev.as_ref(), None)?;
>> +
>> +        clk.prepare_enable()?;
> 
> Is there something like devm_clk_get_enabled() such that the drop
> function becomes redundant?

I've reviewed the current clk.rs abstractions that were recently merged,
and while they provide Clk::get() (which is refcounted), a combined
devm_clk_get_enabled() doesn't exist yet. Because of this, the driver
must manually call enable and disable. I've achieved it through Rust
RAII.

> 
>> +        let driver_data = KBox::new(Th1520PwmChipData { clk, iomem }, GFP_KERNEL)?;
>> +        let pwm_chip = pwm::devm_chip_alloc(pdev.as_ref(), MAX_PWM_NUM, 0)?;
>> +
>> +        let result = pwm::devm_chip_add(pdev.as_ref(), pwm_chip, &TH1520_PWM_OPS);
>> +        if result.is_err() {
>> +            pr_err!("Failed to add PWM chip: {:?}\n", result);
>> +            return Err(EIO);
>> +        }
>> +
>> +        pwm_chip.set_drvdata(driver_data);
>> +        pr_info!("T-HEAD TH1520 PWM probed correctly\n");
> 
> Please degrade to pr_debug. Each driver emitting a message is an
> annoyance.
> 
>> +        Ok(KBox::new(Self, GFP_KERNEL)?.into())
>> +    }
>> +}
>> +
>> +kernel::module_platform_driver! {
>> +    type: Th1520PwmPlatformDriver,
>> +    name: "pwm_th1520",
>> +    author: "Michal Wilczynski",
>> +    description: "T-HEAD TH1520 PWM driver",
>> +    license: "GPL v2",
>> +}
> 
> Best regards
> Uwe

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

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Miguel Ojeda]

On Fri, Jun 6, 2025 at 4:08 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> You're right, thank you. The wrapping_mul is unsafe due to the overflow
> risk you pointed out.

What do you mean by "unsafe"? `wrapping_mul` does not trigger UB and
it intentionally provides wrapping arithmetic.

> The ideal solution would be to use the kernel's own mul_u64_u64_div_u64
> helper function.
>
> Rust maintainers: This binding doesn't seem to be available yet. Would a
> preparatory patch introducing a minimal rust/kernel/math.rs with  only
> this binding be the best way to proceed? It seems like a useful utility
> for more than just this driver.

Sounds good to me. We also recently had related discussions about
64-bit divisions in 32-bit architectures and `const fn`s, you may want
to take a look:

    https://lore.kernel.org/rust-for-linux/CANiq72ke45eOwckMhWHvmwxc03dxr4rnxxKvx+HvWdBLopZfrQ@mail.gmail.com/
    https://lore.kernel.org/rust-for-linux/20250502004524.230553-1-fujita.tomonori@gmail.com/
    https://lore.kernel.org/rust-for-linux/20250501015818.226376-1-fujita.tomonori@gmail.com/

I would also be careful choosing the names: if they are supposed the
same behavior, then please pick the same name as C. Otherwise, we
should avoid it. Either way, we should document them.

For instance, is this supposed to be `mul_u64_u64_div_u64`? I guess
not, given e.g. the `c == 0` case.

I hope that helps & thanks!

Cheers,
Miguel

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski]

On 6/6/25 17:21, Miguel Ojeda wrote:
> On Fri, Jun 6, 2025 at 4:08 PM Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> You're right, thank you. The wrapping_mul is unsafe due to the overflow
>> risk you pointed out.
> 
> What do you mean by "unsafe"? `wrapping_mul` does not trigger UB and
> it intentionally provides wrapping arithmetic.

You're right, I should have said "mathematically incorrect" due to the
overflow, not unsafe

> 
>> The ideal solution would be to use the kernel's own mul_u64_u64_div_u64
>> helper function.
>>
>> Rust maintainers: This binding doesn't seem to be available yet. Would a
>> preparatory patch introducing a minimal rust/kernel/math.rs with  only
>> this binding be the best way to proceed? It seems like a useful utility
>> for more than just this driver.
> 
> Sounds good to me. We also recently had related discussions about
> 64-bit divisions in 32-bit architectures and `const fn`s, you may want
> to take a look:
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72ke45eOwckMhWHvmwxc03dxr4rnxxKvx+HvWdBLopZfrQ@mail.gmail.com/
>     https://lore.kernel.org/rust-for-linux/20250502004524.230553-1-fujita.tomonori@gmail.com/
>     https://lore.kernel.org/rust-for-linux/20250501015818.226376-1-fujita.tomonori@gmail.com/
> 
> I would also be careful choosing the names: if they are supposed the
> same behavior, then please pick the same name as C. Otherwise, we
> should avoid it. Either way, we should document them.
> 
> For instance, is this supposed to be `mul_u64_u64_div_u64`? I guess
> not, given e.g. the `c == 0` case.

Thanks ! Seems like providing a safe Rust wrapper for
mul_u64_u64_div_u64 is the correct path forward. Since it's a standard
helper in the PWM subsystem, using it would allow me to remove the
temporary math implementation from this driver in favor of the kernel's
optimized version.

> 
> I hope that helps & thanks!
> 
> Cheers,
> Miguel
> 

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

Re: [PATCH RFC 2/6] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Benno Lossin]

On Fri Jun 6, 2025 at 4:08 PM CEST, Michal Wilczynski wrote:
> On 6/5/25 12:39, Uwe Kleine-König wrote:
>> On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
>>> +impl Drop for Th1520PwmChipData {
>>> +    fn drop(&mut self) {
>>> +        self.clk.disable_unprepare();
>>> +    }
>>> +}
>>> +
>>> +fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
>>> +    if c == 0 {
>>> +        return 0;
>>> +    }
>>> +    a.wrapping_mul(b) / c
>>> +}
>> 
>> Is this save if a * b > U64_MAX? I would have expected such a function
>> to already exist in generic code.
>
> You're right, thank you. The wrapping_mul is unsafe due to the overflow
> risk you pointed out.

`wrapping_mul` is not `unsafe`, but the above code will result in an
incorrect result if `a * b` overflows (it won't be UB, since
`wrapping_mul` just wraps around to 0).

> The ideal solution would be to use the kernel's own mul_u64_u64_div_u64
> helper function.
>
> Rust maintainers: This binding doesn't seem to be available yet. Would a
> preparatory patch introducing a minimal rust/kernel/math.rs with  only
> this binding be the best way to proceed? It seems like a useful utility
> for more than just this driver.

Sounds good, but I would separate it into its own patch series, since
then you can iterate on both at the same time.

Are there also similar functions for signed and other sizes? If yes,
then we should add them via an extension trait on the primitive types.

---
Cheers,
Benno

[PATCH RFC 3/6] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller

[Michal Wilczynski]

Add the Device Tree binding documentation for the T-HEAD
TH1520 SoC PWM controller.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../devicetree/bindings/pwm/thead,th1520-pwm.yaml  | 48 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..855aec59ac53c430adc849271235686e87b10e6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/thead,th1520-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 PWM controller
+
+maintainers:
+  - Michal Wilczynski <m.wilczynski@samsung.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: thead,th1520-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: SoC PWM clock
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      pwm@ffec01c000 {
+          compatible = "thead,th1520-pwm";
+          reg = <0xff 0xec01c000 0x0 0x4000>;
+          clocks = <&clk CLK_PWM>;
+          #pwm-cells = <3>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 0cfac73aea65076c5ccb50a25ea686fb86b472b8..2ae758a14e8f5a1e7fab7d85831b571551a49ee9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20979,6 +20979,7 @@ F:	Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
 F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
 F:	Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
 F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
+F:	Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c
 F:	drivers/firmware/thead,th1520-aon.c

-- 
2.34.1

[PATCH RFC 4/6] riscv: dts: thead: Add PWM controller node

[Michal Wilczynski]

Add the Device Tree node for the T-HEAD TH1520 SoC's PWM controller.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 527336417765d8470426f2985e1bc22eeafb31aa..f24e12d7259fabcfbdc2dfa966d759db06684ab4 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -482,6 +482,13 @@ uart2: serial@ffec010000 {
 			status = "disabled";
 		};
 
+		pwm: pwm@ffec01c000 {
+			compatible = "thead,th1520-pwm";
+			reg = <0xff 0xec01c000 0x0 0x4000>;
+			clocks = <&clk CLK_PWM>;
+			#pwm-cells = <3>;
+		};
+
 		clk: clock-controller@ffef010000 {
 			compatible = "thead,th1520-clk-ap";
 			reg = <0xff 0xef010000 0x0 0x1000>;

-- 
2.34.1

[PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Michal Wilczynski]

Add PVT DT node for thermal sensor.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
 			thead,pad-group = <1>;
 		};
 
+		pvt: pvt@fffff4e000 {
+			compatible = "moortec,mr75203";
+			reg = <0xff 0xfff4e000 0x0 0x80>,
+			      <0xff 0xfff4e080 0x0 0x100>,
+			      <0xff 0xfff4e180 0x0 0x680>,
+			      <0xff 0xfff4e800 0x0 0x600>;
+			reg-names = "common", "ts", "pd", "vm";
+			clocks = <&aonsys_clk>;
+			#thermal-sensor-cells = <1>;
+		};
+
 		gpio@fffff52000 {
 			compatible = "snps,dw-apb-gpio";
 			reg = <0xff 0xfff52000 0x0 0x1000>;

-- 
2.34.1

Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Drew Fustini]

On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
> Add PVT DT node for thermal sensor.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
>  			thead,pad-group = <1>;
>  		};
>  
> +		pvt: pvt@fffff4e000 {
> +			compatible = "moortec,mr75203";
> +			reg = <0xff 0xfff4e000 0x0 0x80>,
> +			      <0xff 0xfff4e080 0x0 0x100>,
> +			      <0xff 0xfff4e180 0x0 0x680>,
> +			      <0xff 0xfff4e800 0x0 0x600>;
> +			reg-names = "common", "ts", "pd", "vm";
> +			clocks = <&aonsys_clk>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
>  		gpio@fffff52000 {
>  			compatible = "snps,dw-apb-gpio";
>  			reg = <0xff 0xfff52000 0x0 0x1000>;
> 
> -- 
> 2.34.1
> 

I found that on my lpi4a that boot while hang after applying this patch.
I think that it is related to clocks as boot finished okay when using
clk_ignore_unused on the kernel cmdline. Do you happen have that in your
kernel cmdline?

I need to investigate further to understand which clocks are causing the
problem.

Thanks,
Drew

Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics]

W dniu 27.05.2025 o 10:00, Drew Fustini pisze:
> On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
>> Add PVT DT node for thermal sensor.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>   arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
>>   			thead,pad-group = <1>;
>>   		};
>>   
>> +		pvt: pvt@fffff4e000 {
>> +			compatible = "moortec,mr75203";
>> +			reg = <0xff 0xfff4e000 0x0 0x80>,
>> +			      <0xff 0xfff4e080 0x0 0x100>,
>> +			      <0xff 0xfff4e180 0x0 0x680>,
>> +			      <0xff 0xfff4e800 0x0 0x600>;
>> +			reg-names = "common", "ts", "pd", "vm";
>> +			clocks = <&aonsys_clk>;
>> +			#thermal-sensor-cells = <1>;
>> +		};
>> +
>>   		gpio@fffff52000 {
>>   			compatible = "snps,dw-apb-gpio";
>>   			reg = <0xff 0xfff52000 0x0 0x1000>;
>>
>> -- 
>> 2.34.1
>>
> I found that on my lpi4a that boot while hang after applying this patch.
> I think that it is related to clocks as boot finished okay when using
> clk_ignore_unused on the kernel cmdline. Do you happen have that in your
> kernel cmdline?

Right I had that option enabled, that's why I've missed this. Thanks for

letting me know ! I'll remove this option for future testing.

>
> I need to investigate further to understand which clocks are causing the
> problem.


No problem I can look into that as well, most likely towards the end

of the week as I'm travelling and don't have access to the board.


>
> Thanks,
> Drew
>

Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Michal Wilczynski]

On 5/27/25 10:00, Drew Fustini wrote:
> On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
>> Add PVT DT node for thermal sensor.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
>>  			thead,pad-group = <1>;
>>  		};
>>  
>> +		pvt: pvt@fffff4e000 {
>> +			compatible = "moortec,mr75203";
>> +			reg = <0xff 0xfff4e000 0x0 0x80>,
>> +			      <0xff 0xfff4e080 0x0 0x100>,
>> +			      <0xff 0xfff4e180 0x0 0x680>,
>> +			      <0xff 0xfff4e800 0x0 0x600>;
>> +			reg-names = "common", "ts", "pd", "vm";
>> +			clocks = <&aonsys_clk>;
>> +			#thermal-sensor-cells = <1>;
>> +		};
>> +
>>  		gpio@fffff52000 {
>>  			compatible = "snps,dw-apb-gpio";
>>  			reg = <0xff 0xfff52000 0x0 0x1000>;
>>
>> -- 
>> 2.34.1
>>
> 
> I found that on my lpi4a that boot while hang after applying this patch.
> I think that it is related to clocks as boot finished okay when using
> clk_ignore_unused on the kernel cmdline. Do you happen have that in your
> kernel cmdline?
> 
> I need to investigate further to understand which clocks are causing the
> problem.
> 
> Thanks,
> Drew
> 

Thanks for your earlier message. I've investigated, and you were right
about the clocks – the specific one causing the hang is CLK_CPU2AON_X2H.

This appears to be an AHB bus clock required for CPU access to the AON
domain. My proposed solution is to make the pvt node a child of a new
parent bus node in the Device Tree. This new "AON bus" node would then
explicitly request and manage CLK_CPU2AON_X2H, ensuring it's enabled
when its children are accessed.

What are your thoughts on this approach?

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

Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Drew Fustini]

On Sun, Jun 01, 2025 at 09:50:52AM +0200, Michal Wilczynski wrote:
> 
> 
> On 5/27/25 10:00, Drew Fustini wrote:
> > On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
> >> Add PVT DT node for thermal sensor.
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
> >>  			thead,pad-group = <1>;
> >>  		};
> >>  
> >> +		pvt: pvt@fffff4e000 {
> >> +			compatible = "moortec,mr75203";
> >> +			reg = <0xff 0xfff4e000 0x0 0x80>,
> >> +			      <0xff 0xfff4e080 0x0 0x100>,
> >> +			      <0xff 0xfff4e180 0x0 0x680>,
> >> +			      <0xff 0xfff4e800 0x0 0x600>;
> >> +			reg-names = "common", "ts", "pd", "vm";
> >> +			clocks = <&aonsys_clk>;
> >> +			#thermal-sensor-cells = <1>;
> >> +		};
> >> +
> >>  		gpio@fffff52000 {
> >>  			compatible = "snps,dw-apb-gpio";
> >>  			reg = <0xff 0xfff52000 0x0 0x1000>;
> >>
> >> -- 
> >> 2.34.1
> >>
> > 
> > I found that on my lpi4a that boot while hang after applying this patch.
> > I think that it is related to clocks as boot finished okay when using
> > clk_ignore_unused on the kernel cmdline. Do you happen have that in your
> > kernel cmdline?
> > 
> > I need to investigate further to understand which clocks are causing the
> > problem.
> > 
> > Thanks,
> > Drew
> > 
> 
> Thanks for your earlier message. I've investigated, and you were right
> about the clocks – the specific one causing the hang is CLK_CPU2AON_X2H.

Thanks for tracking down the clk causing the hang. I can confirm that
this fixes the boot hang:

diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index ebfb1d59401d..4d0179b8c17c 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -792,7 +792,7 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
                0x134, BIT(8), 0);
 static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
                0x134, BIT(7), 0);
-static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
+static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), CLK_IGNORE_UNUSED);
 static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
                0x140, BIT(9), CLK_IGNORE_UNUSED);
 static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,

> 
> This appears to be an AHB bus clock required for CPU access to the AON
> domain. My proposed solution is to make the pvt node a child of a new
> parent bus node in the Device Tree. This new "AON bus" node would then
> explicitly request and manage CLK_CPU2AON_X2H, ensuring it's enabled
> when its children are accessed.
> 
> What are your thoughts on this approach?

I think that is a good approach. The alternative would be to just add
CLK_IGNORE_UNUSED like above. I've done it before but it is a bit of a
hack.

Thanks,
Drew

Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Michal Wilczynski]

On 6/1/25 19:32, Drew Fustini wrote:
> On Sun, Jun 01, 2025 at 09:50:52AM +0200, Michal Wilczynski wrote:
>>
>>
>> On 5/27/25 10:00, Drew Fustini wrote:
>>> On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
>>>> Add PVT DT node for thermal sensor.
>>>>
>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>>> ---
>>>>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
>>>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>>>> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
>>>>  			thead,pad-group = <1>;
>>>>  		};
>>>>  
>>>> +		pvt: pvt@fffff4e000 {
>>>> +			compatible = "moortec,mr75203";
>>>> +			reg = <0xff 0xfff4e000 0x0 0x80>,
>>>> +			      <0xff 0xfff4e080 0x0 0x100>,
>>>> +			      <0xff 0xfff4e180 0x0 0x680>,
>>>> +			      <0xff 0xfff4e800 0x0 0x600>;
>>>> +			reg-names = "common", "ts", "pd", "vm";
>>>> +			clocks = <&aonsys_clk>;
>>>> +			#thermal-sensor-cells = <1>;
>>>> +		};
>>>> +
>>>>  		gpio@fffff52000 {
>>>>  			compatible = "snps,dw-apb-gpio";
>>>>  			reg = <0xff 0xfff52000 0x0 0x1000>;
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>> I found that on my lpi4a that boot while hang after applying this patch.
>>> I think that it is related to clocks as boot finished okay when using
>>> clk_ignore_unused on the kernel cmdline. Do you happen have that in your
>>> kernel cmdline?
>>>
>>> I need to investigate further to understand which clocks are causing the
>>> problem.
>>>
>>> Thanks,
>>> Drew
>>>
>>
>> Thanks for your earlier message. I've investigated, and you were right
>> about the clocks – the specific one causing the hang is CLK_CPU2AON_X2H.
> 
> Thanks for tracking down the clk causing the hang. I can confirm that
> this fixes the boot hang:
> 
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index ebfb1d59401d..4d0179b8c17c 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -792,7 +792,7 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
>                 0x134, BIT(8), 0);
>  static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
>                 0x134, BIT(7), 0);
> -static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
> +static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), CLK_IGNORE_UNUSED);
>  static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
>                 0x140, BIT(9), CLK_IGNORE_UNUSED);
>  static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,
> 
>>
>> This appears to be an AHB bus clock required for CPU access to the AON
>> domain. My proposed solution is to make the pvt node a child of a new
>> parent bus node in the Device Tree. This new "AON bus" node would then
>> explicitly request and manage CLK_CPU2AON_X2H, ensuring it's enabled
>> when its children are accessed.
>>
>> What are your thoughts on this approach?
> 
> I think that is a good approach. The alternative would be to just add
> CLK_IGNORE_UNUSED like above. I've done it before but it is a bit of a
> hack.

I've followed up on the idea of creating a parent bus node. My attempt
using simple-pm-bus ran into a couple of significant issues that suggest
it's not the correct path.

First, the TRM doesn't seem to specify an address range for this bus.
The range I used in my test was only for the PVT controller itself,
which would be an incorrect abstraction in the device tree.

Second, simple-pm-bus requires its child nodes to use the PM runtime API
(pm_runtime_resume_and_get, etc.). Forcing this on consumer drivers like
the PVT sensor seems like an inappropriate dependency.

Additionally, I discovered that the PWM driver has a similar problem,
silently failing because another clock, CLK_PERISYS_APB1_HCLK, is not
enabled.

The most correct solution likely involves refactoring the clock parent
relationships in clk-th1520-ap.c. However, as a more immediate and less
invasive fix, I propose we apply the CLK_IGNORE_UNUSED flag for both
CLK_CPU2AON_X2H and CLK_PERISYS_APB1_HCLK in the v2 patch. This will fix
the boot hang and the PWM issue while we consider the larger clock
driver changes separately.

Does that sound like a reasonable plan for the v2 series?

> 
> Thanks,
> Drew
> 

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

Re: [PATCH RFC 5/6] riscv: dts: thead: Add PVT node

[Drew Fustini]

On Mon, Jun 09, 2025 at 08:49:57PM +0200, Michal Wilczynski wrote:
> 
> 
> On 6/1/25 19:32, Drew Fustini wrote:
> > On Sun, Jun 01, 2025 at 09:50:52AM +0200, Michal Wilczynski wrote:
> >>
> >>
> >> On 5/27/25 10:00, Drew Fustini wrote:
> >>> On Sat, May 24, 2025 at 11:14:59PM +0200, Michal Wilczynski wrote:
> >>>> Add PVT DT node for thermal sensor.
> >>>>
> >>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >>>> ---
> >>>>  arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
> >>>>  1 file changed, 11 insertions(+)
> >>>>
> >>>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >>>> index f24e12d7259fabcfbdc2dfa966d759db06684ab4..faf5c3aaf209b24cd99ddc377a88e08a8cce24fe 100644
> >>>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >>>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >>>> @@ -648,6 +648,17 @@ padctrl_aosys: pinctrl@fffff4a000 {
> >>>>  			thead,pad-group = <1>;
> >>>>  		};
> >>>>  
> >>>> +		pvt: pvt@fffff4e000 {
> >>>> +			compatible = "moortec,mr75203";
> >>>> +			reg = <0xff 0xfff4e000 0x0 0x80>,
> >>>> +			      <0xff 0xfff4e080 0x0 0x100>,
> >>>> +			      <0xff 0xfff4e180 0x0 0x680>,
> >>>> +			      <0xff 0xfff4e800 0x0 0x600>;
> >>>> +			reg-names = "common", "ts", "pd", "vm";
> >>>> +			clocks = <&aonsys_clk>;
> >>>> +			#thermal-sensor-cells = <1>;
> >>>> +		};
> >>>> +
> >>>>  		gpio@fffff52000 {
> >>>>  			compatible = "snps,dw-apb-gpio";
> >>>>  			reg = <0xff 0xfff52000 0x0 0x1000>;
> >>>>
> >>>> -- 
> >>>> 2.34.1
> >>>>
> >>>
> >>> I found that on my lpi4a that boot while hang after applying this patch.
> >>> I think that it is related to clocks as boot finished okay when using
> >>> clk_ignore_unused on the kernel cmdline. Do you happen have that in your
> >>> kernel cmdline?
> >>>
> >>> I need to investigate further to understand which clocks are causing the
> >>> problem.
> >>>
> >>> Thanks,
> >>> Drew
> >>>
> >>
> >> Thanks for your earlier message. I've investigated, and you were right
> >> about the clocks – the specific one causing the hang is CLK_CPU2AON_X2H.
> > 
> > Thanks for tracking down the clk causing the hang. I can confirm that
> > this fixes the boot hang:
> > 
> > diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> > index ebfb1d59401d..4d0179b8c17c 100644
> > --- a/drivers/clk/thead/clk-th1520-ap.c
> > +++ b/drivers/clk/thead/clk-th1520-ap.c
> > @@ -792,7 +792,7 @@ static CCU_GATE(CLK_AON2CPU_A2X, aon2cpu_a2x_clk, "aon2cpu-a2x", axi4_cpusys2_ac
> >                 0x134, BIT(8), 0);
> >  static CCU_GATE(CLK_X2X_CPUSYS, x2x_cpusys_clk, "x2x-cpusys", axi4_cpusys2_aclk_pd,
> >                 0x134, BIT(7), 0);
> > -static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), 0);
> > +static CCU_GATE(CLK_CPU2AON_X2H, cpu2aon_x2h_clk, "cpu2aon-x2h", axi_aclk_pd, 0x138, BIT(8), CLK_IGNORE_UNUSED);
> >  static CCU_GATE(CLK_CPU2PERI_X2H, cpu2peri_x2h_clk, "cpu2peri-x2h", axi4_cpusys2_aclk_pd,
> >                 0x140, BIT(9), CLK_IGNORE_UNUSED);
> >  static CCU_GATE(CLK_PERISYS_APB1_HCLK, perisys_apb1_hclk, "perisys-apb1-hclk", perisys_ahb_hclk_pd,
> > 
> >>
> >> This appears to be an AHB bus clock required for CPU access to the AON
> >> domain. My proposed solution is to make the pvt node a child of a new
> >> parent bus node in the Device Tree. This new "AON bus" node would then
> >> explicitly request and manage CLK_CPU2AON_X2H, ensuring it's enabled
> >> when its children are accessed.
> >>
> >> What are your thoughts on this approach?
> > 
> > I think that is a good approach. The alternative would be to just add
> > CLK_IGNORE_UNUSED like above. I've done it before but it is a bit of a
> > hack.
> 
> I've followed up on the idea of creating a parent bus node. My attempt
> using simple-pm-bus ran into a couple of significant issues that suggest
> it's not the correct path.
> 
> First, the TRM doesn't seem to specify an address range for this bus.
> The range I used in my test was only for the PVT controller itself,
> which would be an incorrect abstraction in the device tree.
> 
> Second, simple-pm-bus requires its child nodes to use the PM runtime API
> (pm_runtime_resume_and_get, etc.). Forcing this on consumer drivers like
> the PVT sensor seems like an inappropriate dependency.
> 
> Additionally, I discovered that the PWM driver has a similar problem,
> silently failing because another clock, CLK_PERISYS_APB1_HCLK, is not
> enabled.
> 
> The most correct solution likely involves refactoring the clock parent
> relationships in clk-th1520-ap.c. However, as a more immediate and less
> invasive fix, I propose we apply the CLK_IGNORE_UNUSED flag for both
> CLK_CPU2AON_X2H and CLK_PERISYS_APB1_HCLK in the v2 patch. This will fix
> the boot hang and the PWM issue while we consider the larger clock
> driver changes separately.
> 
> Does that sound like a reasonable plan for the v2 series?

Yes, I think that sounds like a good plan. I am okay with adding
CLK_IGNORE_UNUSED for CLK_CPU2AON_X2H and CLK_PERISYS_APB1_HCLK until a
better solution is found.

I like the idea of revisting the parent relationships in the driver. I
added CLK_IGNORE_UNUSED to several clocks in order to fix boot hangs
when I removed clk_ignore_unused from the kernel cmdline. However, I
don't think that I addressed the root cause.

Thanks,
Drew
.

[PATCH RFC 6/6] riscv: dts: thead: Add PWM fan and thermal control

[Michal Wilczynski]

Add Device Tree nodes to enable a PWM controlled fan and it's associated
thermal management for the Lichee Pi 4A board.

This enables temperature-controlled active cooling for the Lichee Pi 4A
board based on SoC temperature.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts | 67 +++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts b/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
index 4020c727f09e8e2286fdc7fecd79dbd8eba69556..c58c2085ca92a3234f1350500cedae4157f0c35f 100644
--- a/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
+++ b/arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts
@@ -28,9 +28,76 @@ aliases {
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	thermal-zones {
+		cpu-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <1000>;
+			thermal-sensors = <&pvt 0>;
+
+			trips {
+				fan_config0: fan-trip0 {
+					temperature = <39000>;
+					hysteresis = <5000>;
+					type = "active";
+				};
+
+				fan_config1: fan-trip1 {
+					temperature = <50000>;
+					hysteresis = <5000>;
+					type = "active";
+				};
+
+				fan_config2: fan-trip2 {
+					temperature = <60000>;
+					hysteresis = <5000>;
+					type = "active";
+				};
+			};
+
+			cooling-maps {
+				map-active-0 {
+					cooling-device = <&fan 1 1>;
+					trip = <&fan_config0>;
+				};
+
+				map-active-1 {
+					cooling-device = <&fan 2 2>;
+					trip = <&fan_config1>;
+				};
+
+				map-active-2 {
+					cooling-device = <&fan 3 3>;
+					trip = <&fan_config2>;
+				};
+			};
+		};
+	};
+
+	fan: pwm-fan {
+		pinctrl-names = "default";
+		pinctrl-0 = <&fan_pins>;
+		compatible = "pwm-fan";
+		#cooling-cells = <2>;
+		pwms = <&pwm 1 10000000 0>;
+		cooling-levels = <0 66 196 255>;
+	};
+
 };
 
 &padctrl0_apsys {
+	fan_pins: fan-0 {
+		pwm1-pins {
+			pins = "GPIO3_3"; /* PWM1 */
+			function = "pwm";
+			bias-disable;
+			drive-strength = <25>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
 	uart0_pins: uart0-0 {
 		tx-pins {
 			pins = "UART0_TXD";

-- 
2.34.1

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Drew Fustini]

On Sat, May 24, 2025 at 11:14:54PM +0200, Michal Wilczynski wrote:
> This patch series introduces Rust support for the T-HEAD TH1520 PWM
> controller and demonstrates its use for fan control on the Sipeed Lichee
> Pi 4A board.
> 
> The primary goal of this patch series is to introduce a basic set of
> Rust abstractions for the Linux PWM subsystem. As a first user and
> practical demonstration of these abstractions, the series also provides
> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
> of its PWM channels and ultimately enables temperature controlled fan
> support for the Lichee Pi 4A board. This work aims to explore the use of
> Rust for PWM drivers and lay a foundation for potential future
> Rust based PWM drivers.
> 
> The series is structured as follows:
> 
> Patch 1/6: Introduce basic PWM abstractions
> This patch lays the groundwork by adding a Kconfig option for Rust PWM
> abstractions, necessary C helper functions, and a new Rust module
> (rust/kernel/pwm.rs). This module provides initial safe wrappers for
> core PWM data structures (Chip, Device, State, Args, Polarity) and
> functions (devm_chip_alloc, devm_chip_add), along with a basic PwmOps
> trait focusing on the .apply callback needed by PWM chip providers.
> 
> Patch 2/6: Add PWM driver for TH1520 SoC
> This introduces the Rust based PWM driver for the T-HEAD TH1520 SoC.
> It implements the PwmOps trait using the abstractions from the first
> patch and handles the specifics of the TH1520 hardware for configuring
> period, duty cycle, and polarity. Resource management leverages devm
> for the PWM chip and Rust DevRes for I/O memory, and RAII for clock
> handling.
> 
> Patch 3/6: dt-bindings: Add PWM T-HEAD controller dt-binding
> This patch adds the Device Tree binding documentation for the T-HEAD
> TH1520 PWM controller.
> 
> Patch 4/6: riscv: dts: thead:: Add PWM controller node
> This patch adds the actual Device Tree node for the TH1520 PWM controller.
> 
> Patch 5/6: riscv: dts: thead: Add PVT node
> Add pvt node for thermal sensor.
> 
> Patch 6/6: riscv: dts: thead: Add PWM fan and thermal control
> This final patch adds the Device Tree configuration for a PWM controlled
> fan to the Sipeed Lichee Pi 4A board DTS file. 
> 
> Testing:
> Tested on the TH1520 SoC. The fan works correctly.
> 
> Points for Discussion:
> The rust/kernel/pwm.rs abstraction layer is currently minimal,
> focusing on the immediate needs of this driver. Feedback on its design,
> scope, and potential for generalization would be highly appreciated.
> General feedback on the Rust implementation, FFI wrapping patterns, and
> adherence to kernel development practices is very welcome.
> 
> The patches are based on rust-next, with some dependencies which are not
> merged yet - platform Io support [1] and clk abstractions [2]. 
> 
> Reference repository with all the patches together can be found on
> github [3].
> 
> [1] - https://lore.kernel.org/rust-for-linux/20250509-topics-tyr-platform_iomem-v8-0-e9f1725a40da@collabora.com/
> [2] - https://lore.kernel.org/rust-for-linux/0ec0250c1170a8a6efb2db7a6cb49ae974d7ce05.1747634382.git.viresh.kumar@linaro.org/ 
> [3] - https://github.com/mwilczy/linux/commits/rust-next-pwm-working-fan-for-sending/

Thanks for the patch series. It will be great to have PWM working
upstream.

I've not built Linux with Rust before, so I'm going through the quick
start [1]. I've also never built Linux with LLVM before but clang seems
like the best compiler to use for Rust. Are you using LLVM?

Drew

[1] https://docs.kernel.org/rust/quick-start.html

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Michal Wilczynski]

On 5/25/25 00:21, Drew Fustini wrote:
> On Sat, May 24, 2025 at 11:14:54PM +0200, Michal Wilczynski wrote:
>> This patch series introduces Rust support for the T-HEAD TH1520 PWM
>> controller and demonstrates its use for fan control on the Sipeed Lichee
>> Pi 4A board.
>>
>> The primary goal of this patch series is to introduce a basic set of
>> Rust abstractions for the Linux PWM subsystem. As a first user and
>> practical demonstration of these abstractions, the series also provides
>> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
>> of its PWM channels and ultimately enables temperature controlled fan
>> support for the Lichee Pi 4A board. This work aims to explore the use of
>> Rust for PWM drivers and lay a foundation for potential future
>> Rust based PWM drivers.
>>
>> The series is structured as follows:
>>
>> Patch 1/6: Introduce basic PWM abstractions
>> This patch lays the groundwork by adding a Kconfig option for Rust PWM
>> abstractions, necessary C helper functions, and a new Rust module
>> (rust/kernel/pwm.rs). This module provides initial safe wrappers for
>> core PWM data structures (Chip, Device, State, Args, Polarity) and
>> functions (devm_chip_alloc, devm_chip_add), along with a basic PwmOps
>> trait focusing on the .apply callback needed by PWM chip providers.
>>
>> Patch 2/6: Add PWM driver for TH1520 SoC
>> This introduces the Rust based PWM driver for the T-HEAD TH1520 SoC.
>> It implements the PwmOps trait using the abstractions from the first
>> patch and handles the specifics of the TH1520 hardware for configuring
>> period, duty cycle, and polarity. Resource management leverages devm
>> for the PWM chip and Rust DevRes for I/O memory, and RAII for clock
>> handling.
>>
>> Patch 3/6: dt-bindings: Add PWM T-HEAD controller dt-binding
>> This patch adds the Device Tree binding documentation for the T-HEAD
>> TH1520 PWM controller.
>>
>> Patch 4/6: riscv: dts: thead:: Add PWM controller node
>> This patch adds the actual Device Tree node for the TH1520 PWM controller.
>>
>> Patch 5/6: riscv: dts: thead: Add PVT node
>> Add pvt node for thermal sensor.
>>
>> Patch 6/6: riscv: dts: thead: Add PWM fan and thermal control
>> This final patch adds the Device Tree configuration for a PWM controlled
>> fan to the Sipeed Lichee Pi 4A board DTS file. 
>>
>> Testing:
>> Tested on the TH1520 SoC. The fan works correctly.
>>
>> Points for Discussion:
>> The rust/kernel/pwm.rs abstraction layer is currently minimal,
>> focusing on the immediate needs of this driver. Feedback on its design,
>> scope, and potential for generalization would be highly appreciated.
>> General feedback on the Rust implementation, FFI wrapping patterns, and
>> adherence to kernel development practices is very welcome.
>>
>> The patches are based on rust-next, with some dependencies which are not
>> merged yet - platform Io support [1] and clk abstractions [2]. 
>>
>> Reference repository with all the patches together can be found on
>> github [3].
>>
>> [1] - https://lore.kernel.org/rust-for-linux/20250509-topics-tyr-platform_iomem-v8-0-e9f1725a40da@collabora.com/
>> [2] - https://lore.kernel.org/rust-for-linux/0ec0250c1170a8a6efb2db7a6cb49ae974d7ce05.1747634382.git.viresh.kumar@linaro.org/ 
>> [3] - https://protect2.fireeye.com/v1/url?k=53ce9a1b-32458f21-53cf1154-74fe4860008a-0c44c7bcb0c6b2a5&q=1&e=b41cbed0-2556-4543-be6a-a1333ab74001&u=https%3A%2F%2Fgithub.com%2Fmwilczy%2Flinux%2Fcommits%2Frust-next-pwm-working-fan-for-sending%2F
> 
> Thanks for the patch series. It will be great to have PWM working
> upstream.
> 
> I've not built Linux with Rust before, so I'm going through the quick
> start [1]. I've also never built Linux with LLVM before but clang seems
> like the best compiler to use for Rust. Are you using LLVM?

Hi Drew,
You're correct, Clang is the way to go for Rust in the kernel. I also
followed the official quick start guide. To answer your question
directly: yes, I'm using LLVM. This is the exact command I use for
cross-compilation:

make ARCH=riscv LLVM=1

CROSS_COMPILATION variable seems to be unnecessary for the LLVM
toolchain.

After the build, I load the kernel binary onto my Lichee Pi 4A board
(running Debian Trixie) via TFTP, which is the same process I used with
the GNU toolchain.

> 
> Drew
> 
> [1] https://docs.kernel.org/rust/quick-start.html
> 

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

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Benno Lossin]

On Mon May 26, 2025 at 10:22 AM CEST, Michal Wilczynski wrote:
> On 5/25/25 00:21, Drew Fustini wrote:
>> Thanks for the patch series. It will be great to have PWM working
>> upstream.
>> 
>> I've not built Linux with Rust before, so I'm going through the quick
>> start [1]. I've also never built Linux with LLVM before but clang seems
>> like the best compiler to use for Rust. Are you using LLVM?
>
> Hi Drew,
> You're correct, Clang is the way to go for Rust in the kernel. I also
> followed the official quick start guide. To answer your question
> directly: yes, I'm using LLVM.

Just to let you know, there is an effort to get rustc to work with a gcc
backend rustc_gcc_codegen [1]. And there also is the gccrs project [2]
trying to create a gnu Rust compiler.

[1]: https://rust-for-linux.com/rustc_codegen_gcc
[2]: https://rust-for-linux.com/gccrs

They have made a lot of progress over the last year, so we're hopeful
that they become usable in the near future. But for the moment,
Clang/LLVM is the way to go.

Hope this helps!

---
Cheers,
Benno

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Drew Fustini]

On Mon, May 26, 2025 at 11:01:58AM +0200, Benno Lossin wrote:
> On Mon May 26, 2025 at 10:22 AM CEST, Michal Wilczynski wrote:
> > On 5/25/25 00:21, Drew Fustini wrote:
> >> Thanks for the patch series. It will be great to have PWM working
> >> upstream.
> >> 
> >> I've not built Linux with Rust before, so I'm going through the quick
> >> start [1]. I've also never built Linux with LLVM before but clang seems
> >> like the best compiler to use for Rust. Are you using LLVM?
> >
> > Hi Drew,
> > You're correct, Clang is the way to go for Rust in the kernel. I also
> > followed the official quick start guide. To answer your question
> > directly: yes, I'm using LLVM.
> 
> Just to let you know, there is an effort to get rustc to work with a gcc
> backend rustc_gcc_codegen [1]. And there also is the gccrs project [2]
> trying to create a gnu Rust compiler.
> 
> [1]: https://rust-for-linux.com/rustc_codegen_gcc
> [2]: https://rust-for-linux.com/gccrs
> 
> They have made a lot of progress over the last year, so we're hopeful
> that they become usable in the near future. But for the moment,
> Clang/LLVM is the way to go.
> 
> Hope this helps!
> 
> ---
> Cheers,
> Benno

Thanks for letting me know about gccrs.

I was able to build linux okay with clang:

make LLVM=1 ARCH=riscv -j16

It booted okay on the lpi4a:

Linux version 6.15.0-next-20250606 (pdp7@thelio) (Ubuntu clang version 18.1.3 (1ubuntu1), Ubuntu LLD 18.1.3) 

I installed rust with:

 rustup default beta
 rustup component add rust-src

 $ make LLVM=1 rustavailable
 ***
 *** Rust bindings generator 'bindgen' versions 0.66.0 and 0.66.1 may not
 *** work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2567),
 *** unless patched (like Debian's).
 ***   Your version:     0.66.1
 ***
 ***
 *** Please see Documentation/rust/quick-start.rst for details
 *** on how to set up the Rust support.
 ***
 Rust is available!

I'm not sure if that bindgen warning matters?

Thanks,
Drew

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Miguel Ojeda]

On Sun, Jun 8, 2025 at 6:58 PM Drew Fustini <drew@pdp7.com> wrote:
>
> I'm not sure if that bindgen warning matters?

If you don't see the `FromBytesWithNulError` error, then it should be
fine, but I would recommend using a newer version anyway.

I hope that helps.

Cheers,
Miguel

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Drew Fustini]

On Sun, Jun 08, 2025 at 07:14:18PM +0200, Miguel Ojeda wrote:
> On Sun, Jun 8, 2025 at 6:58 PM Drew Fustini <drew@pdp7.com> wrote:
> >
> > I'm not sure if that bindgen warning matters?
> 
> If you don't see the `FromBytesWithNulError` error, then it should be
> fine, but I would recommend using a newer version anyway.
> 
> I hope that helps.
> 
> Cheers,
> Miguel

Thanks for the quick response. I seemed to have updated it with:

 cargo install bindgen-cli

And it seems Linux is now happy :)

 $ make LLVM=1 rustavailable
 Rust is available!

-Drew

Re: [PATCH RFC 0/6] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Miguel Ojeda]

On Sun, Jun 8, 2025 at 9:58 PM Drew Fustini <drew@pdp7.com> wrote:
>
> Thanks for the quick response. I seemed to have updated it with:
>
>  cargo install bindgen-cli
>
> And it seems Linux is now happy :)

You're welcome!

(By the way, I always recommend passing `--locked` to `cargo install`
unless there is a good reason not to -- sadly it is not the default
for that subcommand)

Cheers,
Miguel