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

[PATCH v14 0/7] 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 core of this series is a new rust/kernel/pwm.rs module that provides
abstractions for writing PWM chip provider drivers in Rust. This has
been significantly reworked from v1 based on extensive feedback. The key
features of the new abstraction layer include:

 - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
   by ARef, correctly tying its lifetime to its embedded struct device
   reference counter. Chip registration is handled by a pwm::Registration
   RAII guard, which guarantees that pwmchip_add is always paired with
   pwmchip_remove, preventing resource leaks.

 - Modern and Safe API: The PwmOps trait is now based on the modern
   waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
   by the subsystem maintainer. It is generic over a driver's
   hardware specific data structure, moving all unsafe serialization logic
   into the abstraction layer and allowing drivers to be written in 100%
   safe Rust.

 - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
   types (State, Args, Device, etc.) and uses standard kernel error
   handling patterns.

The series is structured as follows:
 - Expose static function pwmchip_release.
 - Rust PWM Abstractions: The new safe abstraction layer.
 - TH1520 PWM Driver: A new Rust driver for the TH1520 SoC, built on
   top of the new abstractions.
 - Device Tree Bindings & Nodes: The remaining patches add the necessary
   DT bindings and nodes for the TH1520 PWM controller, and the PWM fan
   configuration for the Lichee Pi 4A board.

Testing:
Tested on the TH1520 SoC. The fan works correctly. The duty/period
calculations are correct. Fan starts slow when the chip is not hot and
gradually increases the speed when PVT reports higher temperatures.

The patches doesn't contain any dependencies that are not currently in
the mainline kernel anymore.

---
Changes in v14:
- Re-base on top of 6.17-rc1.
- Cosmetic change in label function.
- Link to v13: https://lore.kernel.org/r/20250806-rust-next-pwm-working-fan-for-sending-v13-0-690b669295b6@samsung.com

Changes in v13:
- Re-add the T-HEAD TH1520 PWM driver and its device tree bindings, as
  Iomem series got merged into mainline kernel.
- Fix Args struct to be consistent with State - no Opaque needed for
  copies.
- Replace tuple retur type in the PwmOps trait with dedicated struct
  for improved clarity.
- Use build_assert for WfHw size, as it doesn't have to be runtime
  check.
- Various cosmetic changes.
- Link to v12: https://lore.kernel.org/r/20250717-rust-next-pwm-working-fan-for-sending-v12-0-40f73defae0c@samsung.com

Changes in v12:
 - Reworked the PWM abstractions to use the subclassing pattern as
   suggested by reviewers.
 - pwm::Chip and its driver data are now allocated in a single, contiguous
   memory block via pwmchip_alloc() sizeof_priv argument.
 - Chip::new() now uses the pin init API to construct the driver data
   in place, removing the need for a separate allocation.
 - The  PwmOps trait is now implemented directly by the driver data struct
   itself, removing the DrvData associated type and the ForeignOwnable
   trait.
 - The custom release handler has been updated to call drop_in_place on the driver
   data, ensuring destructors are run correctly before the underlying
   memory is freed.
 - Moved the pwmchip_release prototype in the C header to a separate
   section to clarify it is for FFI use only, as requested.
 - Added a Prerequisite-patch-id trailer to the cover letter to declare
   the dependency on the PWM_WFHWSIZE patch.

- Link to v11: https://lore.kernel.org/r/20250710-rust-next-pwm-working-fan-for-sending-v11-0-93824a16f9ec@samsung.com

Changes in v11:
- Dropped driver and DT commits, as they don't compile based on publicly
  known commit.
- Re-based on top of pwm/for-next.
- Reverted back to devres::Devres::new_foreign_owned, as pwm/for-next
  doesn't contain 'register' re-factor, which is present in linux-next,
  queued for the next merge window. The conflict is trivial, simply
  change 'new_foreign_owned' -> 'register'.
- Added list to MAINTAINERS entry as requested.
- Link to v10: https://lore.kernel.org/r/20250707-rust-next-pwm-working-fan-for-sending-v10-0-d0c5cf342004@samsung.com

Changes in v10:
 - Exported the C pwmchip_release function and called it from the custom
   Rust release_callback to fix a memory leak of the pwm_chip struct.
 - Removed the PwmOps::free callback, as it is not needed for idiomatic
   Rust resource management.
 - Removed the redundant is_null check for drvdata in the release handler,
   as the Rust API guarantees a valid pointer is always provided.

- Link to v9: https://lore.kernel.org/r/20250706-rust-next-pwm-working-fan-for-sending-v9-0-42b5ac2101c7@samsung.com

Changes in v9:
 - Encapsulated vtable setup in Chip::new(): The Chip::new() function is
   now generic over the PwmOps implementation. This allows it to create and
   assign the vtable internally, which simplifies the public API by
   removing the ops_vtable parameter from Registration::register().
 - Fixed memory leak with a release handler: A custom release_callback is
   now assigned to the embedded struct device's release hook. This
   guarantees that driver specific data is always freed when the chip is
   destroyed, even if registration fails.
 - The PwmOpsVTable is now defined as a const associated item to ensure
   it has a 'static lifetime.
 - Combined introductory commits: The Device, Chip, and PwmOps abstractions
   are now introduced in a single commit. This was necessary to resolve the
   circular dependencies between them and present a clean, compilable unit
   for review.

- Link to v8: https://lore.kernel.org/r/20250704-rust-next-pwm-working-fan-for-sending-v8-0-951e5482c9fd@samsung.com

Changes in v8:
 - Dropped already accepted commit, re-based on top of linux-next
 - Reworked the Chip and PwmOps APIs to address the drvdata() type-safety
   comment. Chip is now generic, and PwmOps uses an associated type
   to provide compile-time guarantees.
 - Added a parent device sanity check to Registration::register().
 - Updated drvdata() to return the idiomatic T::Borrowed<'_>.
 - added temporary unsafe blocks in the driver, as the current
   abstraction for Clk is neiter Safe nor Sync. I think eventually
   proper abstraction for Clk will be added as in a current state it's
   not very useful.

- Link to v7: https://lore.kernel.org/r/20250702-rust-next-pwm-working-fan-for-sending-v7-0-67ef39ff1d29@samsung.com

Changes in v7:
- Made parent_device function private and moved casts to Device<Bound>
  there as well.
- Link to v6: https://lore.kernel.org/r/20250701-rust-next-pwm-working-fan-for-sending-v6-0-2710932f6f6b@samsung.com

Changes in v6:
 - Re-based on top of linux-next, dropped two already accepted commits.
 - After re-basing the IoMem dependent patchset stopped working,
   reworked it to use similar API like the PCI subsystem (I think it
   will end up the same). Re-worked the driver for it as well.
 - Remove the apply and get_state callbacks, and most of the State as
   well, as the old way of implementing drivers should not be possible
   in Rust. Left only enabled(), since it's useful for my driver.
 - Removed the public set_drvdata() method from pwm::Chip
 - Moved WFHWSIZE to the public include/linux/pwm.h header and renamed it
   to PWM_WFHWSIZE, allowing bindgen to create safe FFI bindings.
 - Corrected the ns_to_cycles integer calculation in the TH1520 driver to
   handle overflow correctly.
 - Updated the Kconfig entry for the TH1520 driver to select the Rust
   abstractions for a better user experience.

- Link to v5: https://lore.kernel.org/r/20250623-rust-next-pwm-working-fan-for-sending-v5-0-0ca23747c23e@samsung.com

Changes in v5:
- Reworked `pwm::Chip` creation to take driver data directly, which
  allowed making the `chip.drvdata()` accessor infallible
- added missing `pwm.c` file lost during the commit split (sorry !)
- Link to v4: https://lore.kernel.org/r/20250618-rust-next-pwm-working-fan-for-sending-v4-0-a6a28f2b6d8a@samsung.com

Changes in v4:
 - Reworked the pwm::Registration API to use the devres framework,
   addressing lifetime issue.
 - Corrected the PwmOps trait and its callbacks to use immutable references
   (&Chip, &Device) for improved safety.
 - Applied various code style and naming cleanups based on feedback

- Link to v3: https://lore.kernel.org/r/20250617-rust-next-pwm-working-fan-for-sending-v3-0-1cca847c6f9f@samsung.com

Changes in v3:
 - Addressed feedback from Uwe by making multiple changes to the TH1520
   driver and the abstraction layer.
 - Split the core PWM abstractions into three focused commits to ease
   review per Benno request.
 - Confirmed the driver now works correctly with CONFIG_PWM_DEBUG enabled
   by implementing the full waveform API, which correctly reads the
   hardware state.
 - Refactored the Rust code to build cleanly with
   CONFIG_RUST_BUILD_ASSERT_ALLOW=n, primarily by using the try_* family of
   functions for IoMem access.
 - Included several cosmetic changes and cleanups to the abstractions
   per Miguel review.

- Link to v2: https://lore.kernel.org/r/20250610-rust-next-pwm-working-fan-for-sending-v2-0-753e2955f110@samsung.com

Changes in v2:
 - Reworked the PWM abstraction layer based on extensive feedback.
 - Replaced initial devm allocation with a proper ARef<Chip> lifetime model
   using AlwaysRefCounted.
 - Implemented a Registration RAII guard to ensure safe chip add/remove.
 - Migrated the PwmOps trait from the legacy .apply callback to the modern
   waveform API.
 - Refactored the TH1520 driver to use the new, safer abstractions.
 - Added a patch to mark essential bus clocks as CLK_IGNORE_UNUSED to fix
   boot hangs when the PWM and thermal sensors are enabled.
- Link to v1: https://lore.kernel.org/r/20250524-rust-next-pwm-working-fan-for-sending-v1-0-bdd2d5094ff7@samsung.com

---
Michal Wilczynski (7):
      pwm: Export `pwmchip_release` for external use
      rust: pwm: Add Kconfig and basic data structures
      rust: pwm: Add complete abstraction layer
      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 PWM fan and thermal control

 .../devicetree/bindings/pwm/thead,th1520-pwm.yaml  |  48 ++
 MAINTAINERS                                        |  10 +
 arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts  |  67 ++
 arch/riscv/boot/dts/thead/th1520.dtsi              |   7 +
 drivers/pwm/Kconfig                                |  24 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/core.c                                 |   3 +-
 drivers/pwm/pwm_th1520.rs                          | 355 +++++++++
 include/linux/pwm.h                                |   6 +
 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                                 | 790 +++++++++++++++++++++
 14 files changed, 1334 insertions(+), 1 deletion(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250524-rust-next-pwm-working-fan-for-sending-552ad2d1b193

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 1/7] pwm: Export `pwmchip_release` for external use

[Michal Wilczynski]

The upcoming Rust abstraction layer for the PWM subsystem uses a custom
`dev->release` handler to safely manage the lifetime of its driver
data.

To prevent leaking the memory of the `struct pwm_chip` (allocated by
`pwmchip_alloc`), this custom handler must also call the original
`pwmchip_release` function to complete the cleanup.

Make `pwmchip_release` a global, exported function so that it can be
called from the Rust FFI bridge. This involves removing the `static`
keyword, adding a prototype to the public header, and exporting the
symbol.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/pwm/core.c  | 3 ++-
 include/linux/pwm.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 0d66376a83ec350e0c3718959f4d794efd71595a..a33da3dff608fdff91251e5fd07b0dbd295be022 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1600,12 +1600,13 @@ void pwmchip_put(struct pwm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(pwmchip_put);
 
-static void pwmchip_release(struct device *pwmchip_dev)
+void pwmchip_release(struct device *pwmchip_dev)
 {
 	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
 
 	kfree(chip);
 }
+EXPORT_SYMBOL_GPL(pwmchip_release);
 
 struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv)
 {
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 8cafc483db53addf95591d1ac74287532c0fa0ee..d86061024b52172edf3845bf9252a966f120e365 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -485,6 +485,12 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner);
 #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE)
 void pwmchip_remove(struct pwm_chip *chip);
 
+/*
+ * For FFI wrapper use only:
+ * The Rust PWM abstraction needs this to properly free the pwm_chip.
+ */
+void pwmchip_release(struct device *dev);
+
 int __devm_pwmchip_add(struct device *dev, struct pwm_chip *chip, struct module *owner);
 #define devm_pwmchip_add(dev, chip) __devm_pwmchip_add(dev, chip, THIS_MODULE)
 

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 1/7] pwm: Export `pwmchip_release` for external use

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:36AM +0200, Michal Wilczynski wrote:
> The upcoming Rust abstraction layer for the PWM subsystem uses a custom
> `dev->release` handler to safely manage the lifetime of its driver
> data.
> 
> To prevent leaking the memory of the `struct pwm_chip` (allocated by
> `pwmchip_alloc`), this custom handler must also call the original
> `pwmchip_release` function to complete the cleanup.
> 
> Make `pwmchip_release` a global, exported function so that it can be
> called from the Rust FFI bridge. This involves removing the `static`
> keyword, adding a prototype to the public header, and exporting the
> symbol.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/pwm/core.c  | 3 ++-
>  include/linux/pwm.h | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 0d66376a83ec350e0c3718959f4d794efd71595a..a33da3dff608fdff91251e5fd07b0dbd295be022 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -1600,12 +1600,13 @@ void pwmchip_put(struct pwm_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(pwmchip_put);
>  
> -static void pwmchip_release(struct device *pwmchip_dev)
> +void pwmchip_release(struct device *pwmchip_dev)
>  {
>  	struct pwm_chip *chip = pwmchip_from_dev(pwmchip_dev);
>  
>  	kfree(chip);
>  }
> +EXPORT_SYMBOL_GPL(pwmchip_release);
>  
>  struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv)
>  {
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 8cafc483db53addf95591d1ac74287532c0fa0ee..d86061024b52172edf3845bf9252a966f120e365 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -485,6 +485,12 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner);
>  #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE)
>  void pwmchip_remove(struct pwm_chip *chip);
>  
> +/*
> + * For FFI wrapper use only:
> + * The Rust PWM abstraction needs this to properly free the pwm_chip.
> + */
> +void pwmchip_release(struct device *dev);
> +
>  int __devm_pwmchip_add(struct device *dev, struct pwm_chip *chip, struct module *owner);
>  #define devm_pwmchip_add(dev, chip) __devm_pwmchip_add(dev, chip, THIS_MODULE)
>  
> 
> -- 
> 2.34.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 1/7] pwm: Export `pwmchip_release` for external use

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1030 bytes --]

Hello,

On Wed, Aug 20, 2025 at 10:35:36AM +0200, Michal Wilczynski wrote:
> The upcoming Rust abstraction layer for the PWM subsystem uses a custom
> `dev->release` handler to safely manage the lifetime of its driver
> data.
> 
> To prevent leaking the memory of the `struct pwm_chip` (allocated by
> `pwmchip_alloc`), this custom handler must also call the original
> `pwmchip_release` function to complete the cleanup.
> 
> Make `pwmchip_release` a global, exported function so that it can be
> called from the Rust FFI bridge. This involves removing the `static`
> keyword, adding a prototype to the public header, and exporting the
> symbol.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>

I still somewhat dislike this patch. Isn't it possible to make the rust
abstraction use the pointer that .release is set to when it calls
pwmchip_alloc()?

(I wouldn't further delay this series for this discussion, this can be
handled just fine at a later point in time.)

Best regards
Uwe

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 1/7] pwm: Export `pwmchip_release` for external use

[Michal Wilczynski]

On 9/15/25 12:00, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Aug 20, 2025 at 10:35:36AM +0200, Michal Wilczynski wrote:
>> The upcoming Rust abstraction layer for the PWM subsystem uses a custom
>> `dev->release` handler to safely manage the lifetime of its driver
>> data.
>>
>> To prevent leaking the memory of the `struct pwm_chip` (allocated by
>> `pwmchip_alloc`), this custom handler must also call the original
>> `pwmchip_release` function to complete the cleanup.
>>
>> Make `pwmchip_release` a global, exported function so that it can be
>> called from the Rust FFI bridge. This involves removing the `static`
>> keyword, adding a prototype to the public header, and exporting the
>> symbol.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> 
> I still somewhat dislike this patch. Isn't it possible to make the rust
> abstraction use the pointer that .release is set to when it calls
> pwmchip_alloc()?

Hi Uwe,

Sorry for the delay - I was on vacation and I hope you received my
out of office message.

I agree your suggestion is technically feasible. My concern was that it
would make the abstraction less readable and more complex, as it would
require the FFI wrapper to read the C function pointer and store it as
state within the driver's private data.

The current approach of calling a known, exported function felt more
direct. However, I absolutely agree with your point on C API purity and,
as you suggested, this can be revisited later if needed.

> 
> (I wouldn't further delay this series for this discussion, this can be
> handled just fine at a later point in time.)
> 
> Best regards
> Uwe

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 2/7] rust: pwm: Add Kconfig and basic data structures

[Michal Wilczynski]

Introduce the foundational support for PWM abstractions in Rust.

This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
the feature, along with the necessary build-system support and C
helpers.

It also introduces the first set of safe wrappers for the PWM
subsystem, covering the basic data carrying C structs and enums:
- `Polarity`: A safe wrapper for `enum pwm_polarity`.
- `Waveform`: A wrapper for `struct pwm_waveform`.
- `Args`: A wrapper for `struct pwm_args`.
- `State`: A wrapper for `struct pwm_state`.

These types provide memory safe, idiomatic Rust representations of the
core PWM data structures and form the building blocks for the
abstractions that will follow.

Tested-by: Drew Fustini <fustini@kernel.org>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS                     |   8 +++
 drivers/pwm/Kconfig             |  13 +++++
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/pwm.c              |  20 +++++++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/pwm.rs              | 123 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 168 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe168477caa45799dfe07de2f54de6d6a1ce0615..5d7c0676c1d00a02b3d7db2de88b039c08c99c6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20387,6 +20387,14 @@ 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>
+L:	linux-pwm@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+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 f00ce973dddf651287168b44228574f4d5c28dc0..2b608f4378138775ee3ba4d53f682952e1914118 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -800,4 +800,17 @@ config PWM_XILINX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-xilinx.
 
+ config RUST_PWM_ABSTRACTIONS
+	bool "Rust PWM abstractions support"
+	depends on RUST
+	depends on PWM=y
+	help
+	  This option enables the safe Rust abstraction layer for the PWM
+	  subsystem. It provides idiomatic wrappers and traits necessary for
+	  writing PWM controller drivers in Rust.
+
+	  The abstractions handle resource management (like memory and reference
+	  counting) and provide safe interfaces to the underlying C core,
+	  allowing driver logic to be written in safe Rust.
+
 endif
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..7a06ee5781eadc9f21ccd456b574a9cb152cd58c 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -66,6 +66,7 @@
 #include <linux/pm_opp.h>
 #include <linux/poll.h>
 #include <linux/property.h>
+#include <linux/pwm.h>
 #include <linux/refcount.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sched.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41dd51717050648d6160bebebdf4b26..861052ffffaff60e9c2e8109e55f3b6158ff2281 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -35,6 +35,7 @@
 #include "platform.c"
 #include "poll.c"
 #include "property.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 ed53169e795c0badf548025a57f946fa18bc73e3..e339b552f9650803b1efa1eb8ecc6fe9d2c56563 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -117,6 +117,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..57675a3dfca12033355670deb210ca66f8469334
--- /dev/null
+++ b/rust/kernel/pwm.rs
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! PWM subsystem abstractions.
+//!
+//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
+
+use crate::{
+    bindings,
+    prelude::*,
+    types::Opaque,
+};
+use core::convert::TryFrom;
+
+/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub enum Polarity {
+    /// Normal polarity (duty cycle defines the high period of the signal).
+    Normal,
+
+    /// Inversed polarity (duty cycle defines the low period of the signal).
+    Inversed,
+}
+
+impl TryFrom<bindings::pwm_polarity> for Polarity {
+    type Error = Error;
+
+    fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
+        match polarity {
+            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
+            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
+            _ => Err(EINVAL),
+        }
+    }
+}
+
+impl From<Polarity> for bindings::pwm_polarity {
+    fn from(polarity: Polarity) -> Self {
+        match polarity {
+            Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
+            Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
+        }
+    }
+}
+
+/// Represents a PWM waveform configuration.
+/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub struct Waveform {
+    /// Total duration of one complete PWM cycle, in nanoseconds.
+    pub period_length_ns: u64,
+
+    /// Duty-cycle active time, in nanoseconds.
+    ///
+    /// For a typical normal polarity configuration (active-high) this is the
+    /// high time of the signal.
+    pub duty_length_ns: u64,
+
+    /// Duty-cycle start offset, in nanoseconds.
+    ///
+    /// Delay from the beginning of the period to the first active edge.
+    /// In most simple PWM setups this is `0`, so the duty cycle starts
+    /// immediately at each period’s start.
+    pub duty_offset_ns: u64,
+}
+
+impl From<bindings::pwm_waveform> for Waveform {
+    fn from(wf: bindings::pwm_waveform) -> Self {
+        Waveform {
+            period_length_ns: wf.period_length_ns,
+            duty_length_ns: wf.duty_length_ns,
+            duty_offset_ns: wf.duty_offset_ns,
+        }
+    }
+}
+
+impl From<Waveform> for bindings::pwm_waveform {
+    fn from(wf: Waveform) -> Self {
+        bindings::pwm_waveform {
+            period_length_ns: wf.period_length_ns,
+            duty_length_ns: wf.duty_length_ns,
+            duty_offset_ns: wf.duty_offset_ns,
+        }
+    }
+}
+
+/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct Args(bindings::pwm_args);
+
+impl Args {
+    /// Creates a `Args` wrapper by taking ownership of a C `pwm_args` value.
+    pub(crate) fn from_c(c_args: bindings::pwm_args) -> Self {
+        Args(c_args)
+    }
+
+    /// Returns the period of the PWM signal in nanoseconds.
+    pub fn period(&self) -> u64 {
+        self.0.period
+    }
+
+    /// Returns the polarity of the PWM signal.
+    pub fn polarity(&self) -> Result<Polarity, Error> {
+        Polarity::try_from(self.0.polarity)
+    }
+}
+
+/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct State(bindings::pwm_state);
+
+impl State {
+    /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
+    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
+        State(c_state)
+    }
+
+    /// Returns `true` if the PWM signal is enabled.
+    pub fn enabled(&self) -> bool {
+        self.0.enabled
+    }
+}

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 2/7] rust: pwm: Add Kconfig and basic data structures

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:37AM +0200, Michal Wilczynski wrote:
> Introduce the foundational support for PWM abstractions in Rust.
> 
> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
> the feature, along with the necessary build-system support and C
> helpers.
> 
> It also introduces the first set of safe wrappers for the PWM
> subsystem, covering the basic data carrying C structs and enums:
> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
> - `Waveform`: A wrapper for `struct pwm_waveform`.
> - `Args`: A wrapper for `struct pwm_args`.
> - `State`: A wrapper for `struct pwm_state`.
> 
> These types provide memory safe, idiomatic Rust representations of the
> core PWM data structures and form the building blocks for the
> abstractions that will follow.
> 
> Tested-by: Drew Fustini <fustini@kernel.org>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS                     |   8 +++
>  drivers/pwm/Kconfig             |  13 +++++
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/pwm.c              |  20 +++++++
>  rust/kernel/lib.rs              |   2 +
>  rust/kernel/pwm.rs              | 123 ++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 168 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe168477caa45799dfe07de2f54de6d6a1ce0615..5d7c0676c1d00a02b3d7db2de88b039c08c99c6e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20387,6 +20387,14 @@ 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>
> +L:	linux-pwm@vger.kernel.org
> +L:	rust-for-linux@vger.kernel.org
> +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 f00ce973dddf651287168b44228574f4d5c28dc0..2b608f4378138775ee3ba4d53f682952e1914118 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -800,4 +800,17 @@ config PWM_XILINX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-xilinx.
>  
> + config RUST_PWM_ABSTRACTIONS
> +	bool "Rust PWM abstractions support"
> +	depends on RUST
> +	depends on PWM=y
> +	help
> +	  This option enables the safe Rust abstraction layer for the PWM
> +	  subsystem. It provides idiomatic wrappers and traits necessary for
> +	  writing PWM controller drivers in Rust.
> +
> +	  The abstractions handle resource management (like memory and reference
> +	  counting) and provide safe interfaces to the underlying C core,
> +	  allowing driver logic to be written in safe Rust.
> +
>  endif
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..7a06ee5781eadc9f21ccd456b574a9cb152cd58c 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -66,6 +66,7 @@
>  #include <linux/pm_opp.h>
>  #include <linux/poll.h>
>  #include <linux/property.h>
> +#include <linux/pwm.h>
>  #include <linux/refcount.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/sched.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7cf7fe95e41dd51717050648d6160bebebdf4b26..861052ffffaff60e9c2e8109e55f3b6158ff2281 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -35,6 +35,7 @@
>  #include "platform.c"
>  #include "poll.c"
>  #include "property.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 ed53169e795c0badf548025a57f946fa18bc73e3..e339b552f9650803b1efa1eb8ecc6fe9d2c56563 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -117,6 +117,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..57675a3dfca12033355670deb210ca66f8469334
> --- /dev/null
> +++ b/rust/kernel/pwm.rs
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! PWM subsystem abstractions.
> +//!
> +//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
> +
> +use crate::{
> +    bindings,
> +    prelude::*,
> +    types::Opaque,
> +};
> +use core::convert::TryFrom;
> +
> +/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> +pub enum Polarity {
> +    /// Normal polarity (duty cycle defines the high period of the signal).
> +    Normal,
> +
> +    /// Inversed polarity (duty cycle defines the low period of the signal).
> +    Inversed,
> +}
> +
> +impl TryFrom<bindings::pwm_polarity> for Polarity {
> +    type Error = Error;
> +
> +    fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
> +        match polarity {
> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
> +            _ => Err(EINVAL),
> +        }
> +    }
> +}
> +
> +impl From<Polarity> for bindings::pwm_polarity {
> +    fn from(polarity: Polarity) -> Self {
> +        match polarity {
> +            Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
> +            Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
> +        }
> +    }
> +}
> +
> +/// Represents a PWM waveform configuration.
> +/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
> +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
> +pub struct Waveform {
> +    /// Total duration of one complete PWM cycle, in nanoseconds.
> +    pub period_length_ns: u64,
> +
> +    /// Duty-cycle active time, in nanoseconds.
> +    ///
> +    /// For a typical normal polarity configuration (active-high) this is the
> +    /// high time of the signal.
> +    pub duty_length_ns: u64,
> +
> +    /// Duty-cycle start offset, in nanoseconds.
> +    ///
> +    /// Delay from the beginning of the period to the first active edge.
> +    /// In most simple PWM setups this is `0`, so the duty cycle starts
> +    /// immediately at each period’s start.
> +    pub duty_offset_ns: u64,
> +}
> +
> +impl From<bindings::pwm_waveform> for Waveform {
> +    fn from(wf: bindings::pwm_waveform) -> Self {
> +        Waveform {
> +            period_length_ns: wf.period_length_ns,
> +            duty_length_ns: wf.duty_length_ns,
> +            duty_offset_ns: wf.duty_offset_ns,
> +        }
> +    }
> +}
> +
> +impl From<Waveform> for bindings::pwm_waveform {
> +    fn from(wf: Waveform) -> Self {
> +        bindings::pwm_waveform {
> +            period_length_ns: wf.period_length_ns,
> +            duty_length_ns: wf.duty_length_ns,
> +            duty_offset_ns: wf.duty_offset_ns,
> +        }
> +    }
> +}
> +
> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct Args(bindings::pwm_args);
> +
> +impl Args {
> +    /// Creates a `Args` wrapper by taking ownership of a C `pwm_args` value.
> +    pub(crate) fn from_c(c_args: bindings::pwm_args) -> Self {
> +        Args(c_args)
> +    }
> +
> +    /// Returns the period of the PWM signal in nanoseconds.
> +    pub fn period(&self) -> u64 {
> +        self.0.period
> +    }
> +
> +    /// Returns the polarity of the PWM signal.
> +    pub fn polarity(&self) -> Result<Polarity, Error> {
> +        Polarity::try_from(self.0.polarity)
> +    }
> +}
> +
> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct State(bindings::pwm_state);
> +
> +impl State {
> +    /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> +        State(c_state)
> +    }
> +
> +    /// Returns `true` if the PWM signal is enabled.
> +    pub fn enabled(&self) -> bool {
> +        self.0.enabled
> +    }
> +}
> 
> -- 
> 2.34.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 2/7] rust: pwm: Add Kconfig and basic data structures

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 957 bytes --]

Hello Michal,

On Wed, Aug 20, 2025 at 10:35:37AM +0200, Michal Wilczynski wrote:
> Introduce the foundational support for PWM abstractions in Rust.
> 
> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
> the feature, along with the necessary build-system support and C
> helpers.
> 
> It also introduces the first set of safe wrappers for the PWM
> subsystem, covering the basic data carrying C structs and enums:
> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
> - `Waveform`: A wrapper for `struct pwm_waveform`.
> - `Args`: A wrapper for `struct pwm_args`.
> - `State`: A wrapper for `struct pwm_state`.

Args, State and Polarity are only needed for the consumer side of the
PWM API in Rust, right?

I don't particularily like like pwm_args and wonder if this really has
to be exposed to Rust.

I think for State (and thus Polarity) we have to have it for the
forseeable future.

Best regards
Uwe

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 2/7] rust: pwm: Add Kconfig and basic data structures

[Michal Wilczynski]

On 9/15/25 12:13, Uwe Kleine-König wrote:
> Hello Michal,
> 
> On Wed, Aug 20, 2025 at 10:35:37AM +0200, Michal Wilczynski wrote:
>> Introduce the foundational support for PWM abstractions in Rust.
>>
>> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
>> the feature, along with the necessary build-system support and C
>> helpers.
>>
>> It also introduces the first set of safe wrappers for the PWM
>> subsystem, covering the basic data carrying C structs and enums:
>> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
>> - `Waveform`: A wrapper for `struct pwm_waveform`.
>> - `Args`: A wrapper for `struct pwm_args`.
>> - `State`: A wrapper for `struct pwm_state`.
> 
> Args, State and Polarity are only needed for the consumer side of the
> PWM API in Rust, right?
> 
> I don't particularily like like pwm_args and wonder if this really has
> to be exposed to Rust.

You're right that Args is primarily for the consumer side, and our
provider driver doesn't use it at all. I only included it for
completeness, so I'm happy to remove it from the abstractions.

I did use State on the provider side, however, to check if a channel is
enabled before writing new hardware values - I've seen your comment
about it that it's better to use the HW registers instead, I remember I
tried doing something like this back in May when started working on
this, but haven't succeeded - will refresh my memory and respond to the
driver comments.

> 
> I think for State (and thus Polarity) we have to have it for the
> forseeable future.
> 
> Best regards
> Uwe

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 3/7] rust: pwm: Add complete abstraction layer

[Michal Wilczynski]

Introduce a comprehensive abstraction layer for the PWM subsystem to
enable writing drivers in Rust.

Because `Device`, `Chip`, and `PwmOps` all refer to each other, they
form a single, indivisible unit with circular dependencies. They are
introduced together in this single commit to create a complete,
compilable abstraction layer.

The main components are:
 - Data Wrappers: Safe, idiomatic wrappers for core C types like
   `pwm_device`, and `pwm_chip`.

 - PwmOps Trait: An interface that drivers can implement to provide
   their hardware-specific logic, mirroring the C `pwm_ops` interface.

 - FFI VTable and Adapter: A bridge to connect the high-level PwmOps trait
   to the C kernel's pwm_ops vtable.

 - Allocation and Lifetime Management: A high-level `Chip::new()`
   API to safely allocate a chip and a `Registration` guard that integrates
   with `devres` to manage the chip's registration with the PWM core.
   An `AlwaysRefCounted` implementation and a custom release handler
   prevent memory leaks by managing the chip's lifetime and freeing
   driver data correctly.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 rust/kernel/pwm.rs | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 669 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
index 57675a3dfca12033355670deb210ca66f8469334..52ab267083f8ecc3090962603cf8b9e4a33dd0c0 100644
--- a/rust/kernel/pwm.rs
+++ b/rust/kernel/pwm.rs
@@ -8,10 +8,14 @@
 
 use crate::{
     bindings,
+    container_of,
+    device::{self, Bound},
+    devres,
+    error::{self, to_result},
     prelude::*,
-    types::Opaque,
+    types::{ARef, AlwaysRefCounted, Opaque},
 };
-use core::convert::TryFrom;
+use core::{convert::TryFrom, marker::PhantomData, ptr::NonNull};
 
 /// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
@@ -121,3 +125,666 @@ pub fn enabled(&self) -> bool {
         self.0.enabled
     }
 }
+
+/// Describes the outcome of a `round_waveform` operation.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum RoundingOutcome {
+    /// The requested waveform was achievable exactly or by rounding values down.
+    ExactOrRoundedDown,
+
+    /// The requested waveform could only be achieved by rounding up.
+    RoundedUp,
+}
+
+/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::pwm_device>);
+
+impl Device {
+    /// Creates a reference to a [`Device`] from a valid C pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+    /// returned [`Device`] reference.
+    pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::pwm_device) -> &'a Self {
+        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
+        // `Device` type being transparent makes the cast ok.
+        unsafe { &*ptr.cast::<Self>() }
+    }
+
+    /// Returns a raw pointer to the underlying `pwm_device`.
+    fn as_raw(&self) -> *mut bindings::pwm_device {
+        self.0.get()
+    }
+
+    /// Gets the hardware PWM index for this device within its chip.
+    pub fn hwpwm(&self) -> u32 {
+        // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+        unsafe { (*self.as_raw()).hwpwm }
+    }
+
+    /// Gets a reference to the parent `Chip` that this device belongs to.
+    pub fn chip<T: PwmOps>(&self) -> &Chip<T> {
+        // SAFETY: `self.as_raw()` provides a valid pointer. (*self.as_raw()).chip
+        // is assumed to be a valid pointer to `pwm_chip` managed by the kernel.
+        // Chip::from_raw's safety conditions must be met.
+        unsafe { Chip::<T>::from_raw((*self.as_raw()).chip) }
+    }
+
+    /// Gets the label for this PWM device, if any.
+    pub fn label(&self) -> Option<&CStr> {
+        // SAFETY: self.as_raw() provides a valid pointer.
+        let label_ptr = unsafe { (*self.as_raw()).label };
+        if label_ptr.is_null() {
+            return None
+        }
+
+        // SAFETY: label_ptr is non-null and points to a C string
+        // managed by the kernel, valid for the lifetime of the PWM device.
+        Some(unsafe { CStr::from_char_ptr(label_ptr) })
+    }
+
+    /// Gets a copy of the board-dependent arguments for this PWM device.
+    pub fn args(&self) -> Args {
+        // SAFETY: `self.as_raw()` gives a valid pointer. `(*self.as_raw()).args`
+        // is a valid `pwm_args` struct. `Args::from_c` copies this data.
+        unsafe { Args::from_c((*self.as_raw()).args) }
+    }
+
+    /// Gets a copy of the current state of this PWM device.
+    pub fn state(&self) -> State {
+        // SAFETY: `self.as_raw()` gives a valid pointer. `(*self.as_raw()).state`
+        // is a valid `pwm_state` struct. `State::from_c` copies this data.
+        State::from_c(unsafe { (*self.as_raw()).state })
+    }
+
+    /// Sets the PWM waveform configuration and enables the PWM signal.
+    pub fn set_waveform(&self, wf: &Waveform, exact: bool) -> Result {
+        let c_wf = bindings::pwm_waveform::from(*wf);
+
+        // SAFETY: `self.as_raw()` provides a valid `*mut pwm_device` pointer.
+        // `&c_wf` is a valid pointer to a `pwm_waveform` struct. The C function
+        // handles all necessary internal locking.
+        let ret = unsafe { bindings::pwm_set_waveform_might_sleep(self.as_raw(), &c_wf, exact) };
+        to_result(ret)
+    }
+
+    /// Queries the hardware for the configuration it would apply for a given
+    /// request.
+    pub fn round_waveform(&self, wf: &mut Waveform) -> Result<RoundingOutcome> {
+        let mut c_wf = bindings::pwm_waveform::from(*wf);
+
+        // SAFETY: `self.as_raw()` provides a valid `*mut pwm_device` pointer.
+        // `&mut c_wf` is a valid pointer to a mutable `pwm_waveform` struct that
+        // the C function will update.
+        let ret = unsafe { bindings::pwm_round_waveform_might_sleep(self.as_raw(), &mut c_wf) };
+
+        to_result(ret)?;
+
+        *wf = Waveform::from(c_wf);
+
+        if ret == 1 {
+            Ok(RoundingOutcome::RoundedUp)
+        } else {
+            Ok(RoundingOutcome::ExactOrRoundedDown)
+        }
+    }
+
+    /// Reads the current waveform configuration directly from the hardware.
+    pub fn get_waveform(&self) -> Result<Waveform> {
+        let mut c_wf = bindings::pwm_waveform::default();
+
+        // SAFETY: `self.as_raw()` is a valid pointer. We provide a valid pointer
+        // to a stack-allocated `pwm_waveform` struct for the kernel to fill.
+        let ret = unsafe { bindings::pwm_get_waveform_might_sleep(self.as_raw(), &mut c_wf) };
+
+        to_result(ret)?;
+
+        Ok(Waveform::from(c_wf))
+    }
+}
+
+/// The result of a `round_waveform_tohw` operation.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct RoundedWaveform<WfHw> {
+    /// A status code, 0 for success or 1 if values were rounded up.
+    pub status: c_int,
+    /// The driver-specific hardware representation of the waveform.
+    pub hardware_waveform: WfHw,
+}
+
+/// Trait defining the operations for a PWM driver.
+pub trait PwmOps: 'static + Sized {
+    /// The driver-specific hardware representation of a waveform.
+    ///
+    /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
+    type WfHw: Copy + Default;
+
+    /// Optional hook for when a PWM device is requested.
+    fn request(
+        _chip: &Chip<Self>,
+        _pwm: &Device,
+        _parent_dev: &device::Device<Bound>,
+    ) -> Result {
+        Ok(())
+    }
+
+    /// Optional hook for capturing a PWM signal.
+    fn capture(
+        _chip: &Chip<Self>,
+        _pwm: &Device,
+        _result: &mut bindings::pwm_capture,
+        _timeout: usize,
+        _parent_dev: &device::Device<Bound>,
+    ) -> Result {
+        Err(ENOTSUPP)
+    }
+
+    /// Convert a generic waveform to the hardware-specific representation.
+    /// This is typically a pure calculation and does not perform I/O.
+    fn round_waveform_tohw(
+        _chip: &Chip<Self>,
+        _pwm: &Device,
+        _wf: &Waveform,
+    ) -> Result<RoundedWaveform<Self::WfHw>> {
+        Err(ENOTSUPP)
+    }
+
+    /// Convert a hardware-specific representation back to a generic waveform.
+    /// This is typically a pure calculation and does not perform I/O.
+    fn round_waveform_fromhw(
+        _chip: &Chip<Self>,
+        _pwm: &Device,
+        _wfhw: &Self::WfHw,
+        _wf: &mut Waveform,
+    ) -> Result {
+        Err(ENOTSUPP)
+    }
+
+    /// Read the current hardware configuration into the hardware-specific representation.
+    fn read_waveform(
+        _chip: &Chip<Self>,
+        _pwm: &Device,
+        _parent_dev: &device::Device<Bound>,
+    ) -> Result<Self::WfHw> {
+        Err(ENOTSUPP)
+    }
+
+    /// Write a hardware-specific waveform configuration to the hardware.
+    fn write_waveform(
+        _chip: &Chip<Self>,
+        _pwm: &Device,
+        _wfhw: &Self::WfHw,
+        _parent_dev: &device::Device<Bound>,
+    ) -> Result {
+        Err(ENOTSUPP)
+    }
+}
+
+/// Bridges Rust `PwmOps` to the C `pwm_ops` vtable.
+struct Adapter<T: PwmOps> {
+    _p: PhantomData<T>,
+}
+
+impl<T: PwmOps> Adapter<T> {
+    const VTABLE: PwmOpsVTable = create_pwm_ops::<T>();
+
+    /// # Safety
+    ///
+    /// `wfhw_ptr` must be valid for writes of `size_of::<T::WfHw>()` bytes.
+    unsafe fn serialize_wfhw(wfhw: &T::WfHw, wfhw_ptr: *mut c_void) -> Result {
+        let size = core::mem::size_of::<T::WfHw>();
+
+        build_assert!(size <= bindings::PWM_WFHWSIZE as usize);
+
+        // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
+        unsafe {
+            core::ptr::copy_nonoverlapping(
+                core::ptr::from_ref::<T::WfHw>(wfhw).cast::<u8>(),
+                wfhw_ptr.cast::<u8>(),
+                size,
+            );
+        }
+
+        Ok(())
+    }
+
+    /// # Safety
+    ///
+    /// `wfhw_ptr` must be valid for reads of `size_of::<T::WfHw>()` bytes.
+    unsafe fn deserialize_wfhw(wfhw_ptr: *const c_void) -> Result<T::WfHw> {
+        let size = core::mem::size_of::<T::WfHw>();
+
+        build_assert!(size <= bindings::PWM_WFHWSIZE as usize);
+
+        let mut wfhw = T::WfHw::default();
+        // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
+        unsafe {
+            core::ptr::copy_nonoverlapping(
+                wfhw_ptr.cast::<u8>(),
+                core::ptr::from_mut::<T::WfHw>(&mut wfhw).cast::<u8>(),
+                size,
+            );
+        }
+
+        Ok(wfhw)
+    }
+
+    /// # Safety
+    ///
+    /// `dev` must be a valid pointer to a `bindings::device` embedded within a
+    /// `bindings::pwm_chip`. This function is called by the device core when the
+    /// last reference to the device is dropped.
+    unsafe extern "C" fn release_callback(dev: *mut bindings::device) {
+        // SAFETY: The function's contract guarantees that `dev` points to a `device`
+        // field embedded within a valid `pwm_chip`. `container_of!` can therefore
+        // safely calculate the address of the containing struct.
+        let c_chip_ptr = unsafe { container_of!(dev, bindings::pwm_chip, dev) };
+
+        // SAFETY: `c_chip_ptr` is a valid pointer to a `pwm_chip` as established
+        // above. Calling this FFI function is safe.
+        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
+
+        // SAFETY: The driver data was initialized in `new`. We run its destructor here.
+        unsafe { core::ptr::drop_in_place(drvdata_ptr.cast::<T>()) };
+
+        // Now, call the original release function to free the `pwm_chip` itself.
+        // SAFETY: `dev` is the valid pointer passed into this callback, which is
+        // the expected argument for `pwmchip_release`.
+        unsafe { bindings::pwmchip_release(dev); }
+    }
+
+    /// # Safety
+    ///
+    /// Pointers from C must be valid.
+    unsafe extern "C" fn request_callback(
+        chip_ptr: *mut bindings::pwm_chip,
+        pwm_ptr: *mut bindings::pwm_device,
+    ) -> c_int {
+        // SAFETY: PWM core guarentees `chip_ptr` and `pwm_ptr` are valid pointers.
+        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
+
+        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
+        let bound_parent = unsafe { chip.bound_parent_device() };
+        match T::request(chip, pwm, bound_parent) {
+            Ok(()) => 0,
+            Err(e) => e.to_errno(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// Pointers from C must be valid.
+    unsafe extern "C" fn capture_callback(
+        chip_ptr: *mut bindings::pwm_chip,
+        pwm_ptr: *mut bindings::pwm_device,
+        res: *mut bindings::pwm_capture,
+        timeout: usize,
+    ) -> c_int {
+        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
+        // pointers.
+        let (chip, pwm, result) = unsafe {
+            (
+                Chip::<T>::from_raw(chip_ptr),
+                Device::from_raw(pwm_ptr),
+                &mut *res,
+            )
+        };
+
+        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
+        let bound_parent = unsafe { chip.bound_parent_device() };
+        match T::capture(chip, pwm, result, timeout, bound_parent) {
+            Ok(()) => 0,
+            Err(e) => e.to_errno(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// Pointers from C must be valid.
+    unsafe extern "C" fn round_waveform_tohw_callback(
+        chip_ptr: *mut bindings::pwm_chip,
+        pwm_ptr: *mut bindings::pwm_device,
+        wf_ptr: *const bindings::pwm_waveform,
+        wfhw_ptr: *mut c_void,
+    ) -> c_int {
+        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
+        // pointers.
+        let (chip, pwm, wf) = unsafe {
+            (
+                Chip::<T>::from_raw(chip_ptr),
+                Device::from_raw(pwm_ptr),
+                Waveform::from(*wf_ptr),
+            )
+        };
+        match T::round_waveform_tohw(chip, pwm, &wf) {
+            Ok(rounded) => {
+                // SAFETY: `wfhw_ptr` is valid per this function's safety contract.
+                if unsafe { Self::serialize_wfhw(&rounded.hardware_waveform, wfhw_ptr) }.is_err() {
+                    return EINVAL.to_errno();
+                }
+                rounded.status
+            }
+            Err(e) => e.to_errno(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// Pointers from C must be valid.
+    unsafe extern "C" fn round_waveform_fromhw_callback(
+        chip_ptr: *mut bindings::pwm_chip,
+        pwm_ptr: *mut bindings::pwm_device,
+        wfhw_ptr: *const c_void,
+        wf_ptr: *mut bindings::pwm_waveform,
+    ) -> c_int {
+        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
+        // pointers.
+        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
+        // SAFETY: `deserialize_wfhw`'s safety contract is met by this function's contract.
+        let wfhw = match unsafe { Self::deserialize_wfhw(wfhw_ptr) } {
+            Ok(v) => v,
+            Err(e) => return e.to_errno(),
+        };
+
+        let mut rust_wf = Waveform::default();
+        match T::round_waveform_fromhw(chip, pwm, &wfhw, &mut rust_wf) {
+            Ok(()) => {
+                // SAFETY: `wf_ptr` is guaranteed valid by the C caller.
+                unsafe {
+                    *wf_ptr = rust_wf.into();
+                };
+                0
+            }
+            Err(e) => e.to_errno(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// Pointers from C must be valid.
+    unsafe extern "C" fn read_waveform_callback(
+        chip_ptr: *mut bindings::pwm_chip,
+        pwm_ptr: *mut bindings::pwm_device,
+        wfhw_ptr: *mut c_void,
+    ) -> c_int {
+        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
+        // pointers.
+        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
+
+        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
+        let bound_parent = unsafe { chip.bound_parent_device() };
+        match T::read_waveform(chip, pwm, bound_parent) {
+            // SAFETY: `wfhw_ptr` is valid per this function's safety contract.
+            Ok(wfhw) => match unsafe { Self::serialize_wfhw(&wfhw, wfhw_ptr) } {
+                Ok(()) => 0,
+                Err(e) => e.to_errno(),
+            },
+            Err(e) => e.to_errno(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// Pointers from C must be valid.
+    unsafe extern "C" fn write_waveform_callback(
+        chip_ptr: *mut bindings::pwm_chip,
+        pwm_ptr: *mut bindings::pwm_device,
+        wfhw_ptr: *const c_void,
+    ) -> c_int {
+        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
+        // pointers.
+        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
+
+        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
+        let bound_parent = unsafe { chip.bound_parent_device() };
+
+        // SAFETY: `wfhw_ptr` is valid per this function's safety contract.
+        let wfhw = match unsafe { Self::deserialize_wfhw(wfhw_ptr) } {
+            Ok(v) => v,
+            Err(e) => return e.to_errno(),
+        };
+        match T::write_waveform(chip, pwm, &wfhw, bound_parent) {
+            Ok(()) => 0,
+            Err(e) => e.to_errno(),
+        }
+    }
+}
+
+/// VTable structure wrapper for PWM operations.
+/// Mirrors [`struct pwm_ops`](srctree/include/linux/pwm.h).
+#[repr(transparent)]
+pub struct PwmOpsVTable(bindings::pwm_ops);
+
+// SAFETY: PwmOpsVTable is Send. The vtable contains only function pointers
+// and a size, which are simple data types that can be safely moved across
+// threads. The thread-safety of calling these functions is handled by the
+// kernel's locking mechanisms.
+unsafe impl Send for PwmOpsVTable {}
+
+// SAFETY: PwmOpsVTable is Sync. The vtable is immutable after it is created,
+// so it can be safely referenced and accessed concurrently by multiple threads
+// e.g. to read the function pointers.
+unsafe impl Sync for PwmOpsVTable {}
+
+impl PwmOpsVTable {
+    /// Returns a raw pointer to the underlying `pwm_ops` struct.
+    pub(crate) fn as_raw(&self) -> *const bindings::pwm_ops {
+        &self.0
+    }
+}
+
+/// Creates a PWM operations vtable for a type `T` that implements `PwmOps`.
+///
+/// This is used to bridge Rust trait implementations to the C `struct pwm_ops`
+/// expected by the kernel.
+pub const fn create_pwm_ops<T: PwmOps>() -> PwmOpsVTable {
+    // SAFETY: `core::mem::zeroed()` is unsafe. For `pwm_ops`, all fields are
+    // `Option<extern "C" fn(...)>` or data, so a zeroed pattern (None/0) is valid initially.
+    let mut ops: bindings::pwm_ops = unsafe { core::mem::zeroed() };
+
+    ops.request = Some(Adapter::<T>::request_callback);
+    ops.capture = Some(Adapter::<T>::capture_callback);
+
+    ops.round_waveform_tohw = Some(Adapter::<T>::round_waveform_tohw_callback);
+    ops.round_waveform_fromhw = Some(Adapter::<T>::round_waveform_fromhw_callback);
+    ops.read_waveform = Some(Adapter::<T>::read_waveform_callback);
+    ops.write_waveform = Some(Adapter::<T>::write_waveform_callback);
+    ops.sizeof_wfhw = core::mem::size_of::<T::WfHw>();
+
+    PwmOpsVTable(ops)
+}
+
+/// Wrapper for a PWM chip/controller ([`struct pwm_chip`](srctree/include/linux/pwm.h)).
+#[repr(transparent)]
+pub struct Chip<T: PwmOps>(Opaque<bindings::pwm_chip>, PhantomData<T>);
+
+impl<T: PwmOps> Chip<T> {
+    /// Creates a reference to a [`Chip`] from a valid pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+    /// returned [`Chip`] reference.
+    pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::pwm_chip) -> &'a Self {
+        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
+        // `Chip` type being transparent makes the cast ok.
+        unsafe { &*ptr.cast::<Self>() }
+    }
+
+    /// Returns a raw pointer to the underlying `pwm_chip`.
+    pub(crate) fn as_raw(&self) -> *mut bindings::pwm_chip {
+        self.0.get()
+    }
+
+    /// Gets the number of PWM channels (hardware PWMs) on this chip.
+    pub fn num_channels(&self) -> u32 {
+        // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+        unsafe { (*self.as_raw()).npwm }
+    }
+
+    /// Returns `true` if the chip supports atomic operations for configuration.
+    pub fn is_atomic(&self) -> bool {
+        // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
+        unsafe { (*self.as_raw()).atomic }
+    }
+
+    /// Returns a reference to the embedded `struct device` abstraction.
+    pub fn device(&self) -> &device::Device {
+        // SAFETY:
+        // - `self.as_raw()` provides a valid pointer to `bindings::pwm_chip`.
+        // - The `dev` field is an instance of `bindings::device` embedded
+        //   within `pwm_chip`.
+        // - Taking a pointer to this embedded field is valid.
+        // - `device::Device` is `#[repr(transparent)]`.
+        // - The lifetime of the returned reference is tied to `self`.
+        unsafe { device::Device::from_raw(&raw mut (*self.as_raw()).dev) }
+    }
+
+    /// Gets the typed driver specific data associated with this chip's embedded device.
+    pub fn drvdata(&self) -> &T {
+        // SAFETY: `pwmchip_get_drvdata` returns the pointer to the private data area,
+        // which we know holds our `T`. The pointer is valid for the lifetime of `self`.
+        unsafe { &*bindings::pwmchip_get_drvdata(self.as_raw()).cast::<T>() }
+    }
+
+    /// Returns a reference to the parent device of this PWM chip's device.
+    ///
+    /// # Safety
+    ///
+    /// The caller must guarantee that the parent device exists and is bound.
+    /// This is guaranteed by the PWM core during `PwmOps` callbacks.
+    unsafe fn bound_parent_device(&self) -> &device::Device<Bound> {
+        // SAFETY: Per the function's safety contract, the parent device exists.
+        let parent = unsafe { self.device().parent().unwrap_unchecked() };
+
+        // SAFETY: Per the function's safety contract, the parent device is bound.
+        // This is guaranteed by the PWM core during `PwmOps` callbacks.
+        unsafe { parent.as_bound() }
+    }
+
+    /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
+    ///
+    /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
+    /// on its embedded `struct device`.
+    pub fn new(
+        parent_dev: &device::Device,
+        num_channels: u32,
+        data: impl pin_init::PinInit<T, Error>,
+    ) -> Result<ARef<Self>> {
+        let sizeof_priv = core::mem::size_of::<T>();
+        // SAFETY: `pwmchip_alloc` allocates memory for the C struct and our private data.
+        let c_chip_ptr_raw = unsafe {
+            bindings::pwmchip_alloc(parent_dev.as_raw(), num_channels, sizeof_priv)
+        };
+
+        let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
+
+        // SAFETY: The `drvdata` pointer is the start of the private area, which is where
+        // we will construct our `T` object.
+        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
+
+        // SAFETY: We construct the `T` object in-place in the allocated private memory.
+        unsafe { data.__pinned_init(drvdata_ptr.cast())? };
+
+        // SAFETY: `c_chip_ptr` points to a valid chip.
+        unsafe { (*c_chip_ptr).dev.release = Some(Adapter::<T>::release_callback); }
+
+        // SAFETY: `c_chip_ptr` points to a valid chip.
+        // The `Adapter`'s `VTABLE` has a 'static lifetime, so the pointer
+        // returned by `as_raw()` is always valid.
+        unsafe { (*c_chip_ptr).ops = Adapter::<T>::VTABLE.as_raw(); }
+
+        // Cast the `*mut bindings::pwm_chip` to `*mut Chip`. This is valid because
+        // `Chip` is `repr(transparent)` over `Opaque<bindings::pwm_chip>`, and
+        // `Opaque<T>` is `repr(transparent)` over `T`.
+        let chip_ptr_as_self = c_chip_ptr.cast::<Self>();
+
+        // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
+        // `bindings::pwm_chip`) whose embedded device has refcount 1.
+        // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
+        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
+    }
+}
+
+// SAFETY: Implements refcounting for `Chip` using the embedded `struct device`.
+unsafe impl<T: PwmOps> AlwaysRefCounted for Chip<T> {
+    #[inline]
+    fn inc_ref(&self) {
+        // SAFETY: `self.0.get()` points to a valid `pwm_chip` because `self` exists.
+        // The embedded `dev` is valid. `get_device` increments its refcount.
+        unsafe { bindings::get_device(&raw mut (*self.0.get()).dev); }
+    }
+
+    #[inline]
+    unsafe fn dec_ref(obj: NonNull<Chip<T>>) {
+        let c_chip_ptr = obj.cast::<bindings::pwm_chip>().as_ptr();
+
+        // SAFETY: `obj` is a valid pointer to a `Chip` (and thus `bindings::pwm_chip`)
+        // with a non-zero refcount. `put_device` handles decrement and final release.
+        unsafe { bindings::put_device(&raw mut (*c_chip_ptr).dev); }
+    }
+}
+
+// SAFETY: `Chip` is a wrapper around `*mut bindings::pwm_chip`. The underlying C
+// structure's state is managed and synchronized by the kernel's device model
+// and PWM core locking mechanisms. Therefore, it is safe to move the `Chip`
+// wrapper (and the pointer it contains) across threads.
+unsafe impl<T: PwmOps + Send> Send for Chip<T> {}
+
+// SAFETY: It is safe for multiple threads to have shared access (`&Chip`) because
+// the `Chip` data is immutable from the Rust side without holding the appropriate
+// kernel locks, which the C core is responsible for. Any interior mutability is
+// handled and synchronized by the C kernel code.
+unsafe impl<T: PwmOps + Sync> Sync for Chip<T> {}
+
+/// A resource guard that ensures `pwmchip_remove` is called on drop.
+///
+/// This struct is intended to be managed by the `devres` framework by transferring its ownership
+/// via [`Devres::register`]. This ties the lifetime of the PWM chip registration
+/// to the lifetime of the underlying device.
+pub struct Registration<T: PwmOps> {
+    chip: ARef<Chip<T>>,
+}
+
+impl<T: 'static + PwmOps + Send + Sync> Registration<T> {
+    /// Registers a PWM chip with the PWM subsystem.
+    ///
+    /// Transfers its ownership to the `devres` framework, which ties its lifetime
+    /// to the parent device.
+    /// On unbind of the parent device, the `devres` entry will be dropped, automatically
+    /// calling `pwmchip_remove`. This function should be called from the driver's `probe`.
+    pub fn register(
+        dev: &device::Device<Bound>,
+        chip: ARef<Chip<T>>,
+    ) -> Result {
+	let chip_parent = chip.device().parent().ok_or(EINVAL)?;
+        if dev.as_raw() != chip_parent.as_raw() {
+            return Err(EINVAL);
+        }
+
+        let c_chip_ptr = chip.as_raw();
+
+        // SAFETY: `c_chip_ptr` points to a valid chip with its ops initialized.
+        // `__pwmchip_add` is the C function to register the chip with the PWM core.
+        unsafe {
+            to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
+        }
+
+        let registration = Registration { chip };
+
+        devres::register(dev, registration, GFP_KERNEL)
+    }
+}
+
+impl<T: PwmOps> Drop for Registration<T> {
+    fn drop(&mut self) {
+        let chip_raw = self.chip.as_raw();
+
+        // SAFETY: `chip_raw` points to a chip that was successfully registered.
+        // `bindings::pwmchip_remove` is the correct C function to unregister it.
+        // This `drop` implementation is called automatically by `devres` on driver unbind.
+        unsafe { bindings::pwmchip_remove(chip_raw); }
+    }
+}

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 3/7] rust: pwm: Add complete abstraction layer

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:38AM +0200, Michal Wilczynski wrote:
> Introduce a comprehensive abstraction layer for the PWM subsystem to
> enable writing drivers in Rust.
> 
> Because `Device`, `Chip`, and `PwmOps` all refer to each other, they
> form a single, indivisible unit with circular dependencies. They are
> introduced together in this single commit to create a complete,
> compilable abstraction layer.
> 
> The main components are:
>  - Data Wrappers: Safe, idiomatic wrappers for core C types like
>    `pwm_device`, and `pwm_chip`.
> 
>  - PwmOps Trait: An interface that drivers can implement to provide
>    their hardware-specific logic, mirroring the C `pwm_ops` interface.
> 
>  - FFI VTable and Adapter: A bridge to connect the high-level PwmOps trait
>    to the C kernel's pwm_ops vtable.
> 
>  - Allocation and Lifetime Management: A high-level `Chip::new()`
>    API to safely allocate a chip and a `Registration` guard that integrates
>    with `devres` to manage the chip's registration with the PWM core.
>    An `AlwaysRefCounted` implementation and a custom release handler
>    prevent memory leaks by managing the chip's lifetime and freeing
>    driver data correctly.
> 
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  rust/kernel/pwm.rs | 671 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 669 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> index 57675a3dfca12033355670deb210ca66f8469334..52ab267083f8ecc3090962603cf8b9e4a33dd0c0 100644
> --- a/rust/kernel/pwm.rs
> +++ b/rust/kernel/pwm.rs
> @@ -8,10 +8,14 @@
>  
>  use crate::{
>      bindings,
> +    container_of,
> +    device::{self, Bound},
> +    devres,
> +    error::{self, to_result},
>      prelude::*,
> -    types::Opaque,
> +    types::{ARef, AlwaysRefCounted, Opaque},
>  };
> -use core::convert::TryFrom;
> +use core::{convert::TryFrom, marker::PhantomData, ptr::NonNull};
>  
>  /// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
>  #[derive(Copy, Clone, Debug, PartialEq, Eq)]
> @@ -121,3 +125,666 @@ pub fn enabled(&self) -> bool {
>          self.0.enabled
>      }
>  }
> +
> +/// Describes the outcome of a `round_waveform` operation.
> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
> +pub enum RoundingOutcome {
> +    /// The requested waveform was achievable exactly or by rounding values down.
> +    ExactOrRoundedDown,
> +
> +    /// The requested waveform could only be achieved by rounding up.
> +    RoundedUp,
> +}
> +
> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::pwm_device>);
> +
> +impl Device {
> +    /// Creates a reference to a [`Device`] from a valid C pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
> +    /// returned [`Device`] reference.
> +    pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::pwm_device) -> &'a Self {
> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> +        // `Device` type being transparent makes the cast ok.
> +        unsafe { &*ptr.cast::<Self>() }
> +    }

This looks like the only constructor for `Device`, and it has restricted
visibility.

Would it be helpful to provide a doc-test here to show intended usage?

After looking at the C code, it looks like the raw `pwm_device` pointer
is extracted from a `device` struct, or stored in an array of
`pwm_device` pointers.

Is the intent that the `pwm_device` will always be allocated from C
code? For example, is a `pub fn new() -> &'a Self`-style constructor
not possible/desired?

> +
> +    /// Returns a raw pointer to the underlying `pwm_device`.
> +    fn as_raw(&self) -> *mut bindings::pwm_device {
> +        self.0.get()
> +    }
> +
> +    /// Gets the hardware PWM index for this device within its chip.
> +    pub fn hwpwm(&self) -> u32 {
> +        // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
> +        unsafe { (*self.as_raw()).hwpwm }
> +    }
> +
> +    /// Gets a reference to the parent `Chip` that this device belongs to.
> +    pub fn chip<T: PwmOps>(&self) -> &Chip<T> {
> +        // SAFETY: `self.as_raw()` provides a valid pointer. (*self.as_raw()).chip
> +        // is assumed to be a valid pointer to `pwm_chip` managed by the kernel.
> +        // Chip::from_raw's safety conditions must be met.
> +        unsafe { Chip::<T>::from_raw((*self.as_raw()).chip) }
> +    }
> +
> +    /// Gets the label for this PWM device, if any.
> +    pub fn label(&self) -> Option<&CStr> {
> +        // SAFETY: self.as_raw() provides a valid pointer.
> +        let label_ptr = unsafe { (*self.as_raw()).label };
> +        if label_ptr.is_null() {
> +            return None
> +        }
> +
> +        // SAFETY: label_ptr is non-null and points to a C string
> +        // managed by the kernel, valid for the lifetime of the PWM device.
> +        Some(unsafe { CStr::from_char_ptr(label_ptr) })
> +    }
> +
> +    /// Gets a copy of the board-dependent arguments for this PWM device.
> +    pub fn args(&self) -> Args {
> +        // SAFETY: `self.as_raw()` gives a valid pointer. `(*self.as_raw()).args`
> +        // is a valid `pwm_args` struct. `Args::from_c` copies this data.
> +        unsafe { Args::from_c((*self.as_raw()).args) }
> +    }
> +
> +    /// Gets a copy of the current state of this PWM device.
> +    pub fn state(&self) -> State {
> +        // SAFETY: `self.as_raw()` gives a valid pointer. `(*self.as_raw()).state`
> +        // is a valid `pwm_state` struct. `State::from_c` copies this data.
> +        State::from_c(unsafe { (*self.as_raw()).state })
> +    }
> +
> +    /// Sets the PWM waveform configuration and enables the PWM signal.
> +    pub fn set_waveform(&self, wf: &Waveform, exact: bool) -> Result {
> +        let c_wf = bindings::pwm_waveform::from(*wf);
> +
> +        // SAFETY: `self.as_raw()` provides a valid `*mut pwm_device` pointer.
> +        // `&c_wf` is a valid pointer to a `pwm_waveform` struct. The C function
> +        // handles all necessary internal locking.
> +        let ret = unsafe { bindings::pwm_set_waveform_might_sleep(self.as_raw(), &c_wf, exact) };
> +        to_result(ret)
> +    }
> +
> +    /// Queries the hardware for the configuration it would apply for a given
> +    /// request.
> +    pub fn round_waveform(&self, wf: &mut Waveform) -> Result<RoundingOutcome> {
> +        let mut c_wf = bindings::pwm_waveform::from(*wf);
> +
> +        // SAFETY: `self.as_raw()` provides a valid `*mut pwm_device` pointer.
> +        // `&mut c_wf` is a valid pointer to a mutable `pwm_waveform` struct that
> +        // the C function will update.
> +        let ret = unsafe { bindings::pwm_round_waveform_might_sleep(self.as_raw(), &mut c_wf) };
> +
> +        to_result(ret)?;
> +
> +        *wf = Waveform::from(c_wf);
> +
> +        if ret == 1 {
> +            Ok(RoundingOutcome::RoundedUp)
> +        } else {
> +            Ok(RoundingOutcome::ExactOrRoundedDown)
> +        }
> +    }
> +
> +    /// Reads the current waveform configuration directly from the hardware.
> +    pub fn get_waveform(&self) -> Result<Waveform> {
> +        let mut c_wf = bindings::pwm_waveform::default();
> +
> +        // SAFETY: `self.as_raw()` is a valid pointer. We provide a valid pointer
> +        // to a stack-allocated `pwm_waveform` struct for the kernel to fill.
> +        let ret = unsafe { bindings::pwm_get_waveform_might_sleep(self.as_raw(), &mut c_wf) };
> +
> +        to_result(ret)?;
> +
> +        Ok(Waveform::from(c_wf))
> +    }
> +}
> +
> +/// The result of a `round_waveform_tohw` operation.
> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
> +pub struct RoundedWaveform<WfHw> {
> +    /// A status code, 0 for success or 1 if values were rounded up.
> +    pub status: c_int,
> +    /// The driver-specific hardware representation of the waveform.
> +    pub hardware_waveform: WfHw,
> +}
> +
> +/// Trait defining the operations for a PWM driver.
> +pub trait PwmOps: 'static + Sized {
> +    /// The driver-specific hardware representation of a waveform.
> +    ///
> +    /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
> +    type WfHw: Copy + Default;
> +
> +    /// Optional hook for when a PWM device is requested.
> +    fn request(
> +        _chip: &Chip<Self>,
> +        _pwm: &Device,
> +        _parent_dev: &device::Device<Bound>,
> +    ) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Optional hook for capturing a PWM signal.
> +    fn capture(
> +        _chip: &Chip<Self>,
> +        _pwm: &Device,
> +        _result: &mut bindings::pwm_capture,
> +        _timeout: usize,
> +        _parent_dev: &device::Device<Bound>,
> +    ) -> Result {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Convert a generic waveform to the hardware-specific representation.
> +    /// This is typically a pure calculation and does not perform I/O.
> +    fn round_waveform_tohw(
> +        _chip: &Chip<Self>,
> +        _pwm: &Device,
> +        _wf: &Waveform,
> +    ) -> Result<RoundedWaveform<Self::WfHw>> {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Convert a hardware-specific representation back to a generic waveform.
> +    /// This is typically a pure calculation and does not perform I/O.
> +    fn round_waveform_fromhw(
> +        _chip: &Chip<Self>,
> +        _pwm: &Device,
> +        _wfhw: &Self::WfHw,
> +        _wf: &mut Waveform,
> +    ) -> Result {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Read the current hardware configuration into the hardware-specific representation.
> +    fn read_waveform(
> +        _chip: &Chip<Self>,
> +        _pwm: &Device,
> +        _parent_dev: &device::Device<Bound>,
> +    ) -> Result<Self::WfHw> {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Write a hardware-specific waveform configuration to the hardware.
> +    fn write_waveform(
> +        _chip: &Chip<Self>,
> +        _pwm: &Device,
> +        _wfhw: &Self::WfHw,
> +        _parent_dev: &device::Device<Bound>,
> +    ) -> Result {
> +        Err(ENOTSUPP)
> +    }
> +}
> +
> +/// Bridges Rust `PwmOps` to the C `pwm_ops` vtable.
> +struct Adapter<T: PwmOps> {
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: PwmOps> Adapter<T> {
> +    const VTABLE: PwmOpsVTable = create_pwm_ops::<T>();
> +
> +    /// # Safety
> +    ///
> +    /// `wfhw_ptr` must be valid for writes of `size_of::<T::WfHw>()` bytes.
> +    unsafe fn serialize_wfhw(wfhw: &T::WfHw, wfhw_ptr: *mut c_void) -> Result {
> +        let size = core::mem::size_of::<T::WfHw>();
> +
> +        build_assert!(size <= bindings::PWM_WFHWSIZE as usize);
> +
> +        // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
> +        unsafe {
> +            core::ptr::copy_nonoverlapping(
> +                core::ptr::from_ref::<T::WfHw>(wfhw).cast::<u8>(),
> +                wfhw_ptr.cast::<u8>(),
> +                size,
> +            );
> +        }
> +
> +        Ok(())
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `wfhw_ptr` must be valid for reads of `size_of::<T::WfHw>()` bytes.
> +    unsafe fn deserialize_wfhw(wfhw_ptr: *const c_void) -> Result<T::WfHw> {
> +        let size = core::mem::size_of::<T::WfHw>();
> +
> +        build_assert!(size <= bindings::PWM_WFHWSIZE as usize);
> +
> +        let mut wfhw = T::WfHw::default();
> +        // SAFETY: The caller ensures `wfhw_ptr` is valid for `size` bytes.
> +        unsafe {
> +            core::ptr::copy_nonoverlapping(
> +                wfhw_ptr.cast::<u8>(),
> +                core::ptr::from_mut::<T::WfHw>(&mut wfhw).cast::<u8>(),
> +                size,
> +            );
> +        }
> +
> +        Ok(wfhw)
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `dev` must be a valid pointer to a `bindings::device` embedded within a
> +    /// `bindings::pwm_chip`. This function is called by the device core when the
> +    /// last reference to the device is dropped.
> +    unsafe extern "C" fn release_callback(dev: *mut bindings::device) {
> +        // SAFETY: The function's contract guarantees that `dev` points to a `device`
> +        // field embedded within a valid `pwm_chip`. `container_of!` can therefore
> +        // safely calculate the address of the containing struct.
> +        let c_chip_ptr = unsafe { container_of!(dev, bindings::pwm_chip, dev) };
> +
> +        // SAFETY: `c_chip_ptr` is a valid pointer to a `pwm_chip` as established
> +        // above. Calling this FFI function is safe.
> +        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
> +
> +        // SAFETY: The driver data was initialized in `new`. We run its destructor here.
> +        unsafe { core::ptr::drop_in_place(drvdata_ptr.cast::<T>()) };
> +
> +        // Now, call the original release function to free the `pwm_chip` itself.
> +        // SAFETY: `dev` is the valid pointer passed into this callback, which is
> +        // the expected argument for `pwmchip_release`.
> +        unsafe { bindings::pwmchip_release(dev); }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Pointers from C must be valid.
> +    unsafe extern "C" fn request_callback(
> +        chip_ptr: *mut bindings::pwm_chip,
> +        pwm_ptr: *mut bindings::pwm_device,
> +    ) -> c_int {
> +        // SAFETY: PWM core guarentees `chip_ptr` and `pwm_ptr` are valid pointers.
> +        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
> +
> +        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
> +        let bound_parent = unsafe { chip.bound_parent_device() };
> +        match T::request(chip, pwm, bound_parent) {
> +            Ok(()) => 0,
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Pointers from C must be valid.
> +    unsafe extern "C" fn capture_callback(
> +        chip_ptr: *mut bindings::pwm_chip,
> +        pwm_ptr: *mut bindings::pwm_device,
> +        res: *mut bindings::pwm_capture,
> +        timeout: usize,
> +    ) -> c_int {
> +        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
> +        // pointers.
> +        let (chip, pwm, result) = unsafe {
> +            (
> +                Chip::<T>::from_raw(chip_ptr),
> +                Device::from_raw(pwm_ptr),
> +                &mut *res,
> +            )
> +        };
> +
> +        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
> +        let bound_parent = unsafe { chip.bound_parent_device() };
> +        match T::capture(chip, pwm, result, timeout, bound_parent) {
> +            Ok(()) => 0,
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Pointers from C must be valid.
> +    unsafe extern "C" fn round_waveform_tohw_callback(
> +        chip_ptr: *mut bindings::pwm_chip,
> +        pwm_ptr: *mut bindings::pwm_device,
> +        wf_ptr: *const bindings::pwm_waveform,
> +        wfhw_ptr: *mut c_void,
> +    ) -> c_int {
> +        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
> +        // pointers.
> +        let (chip, pwm, wf) = unsafe {
> +            (
> +                Chip::<T>::from_raw(chip_ptr),
> +                Device::from_raw(pwm_ptr),
> +                Waveform::from(*wf_ptr),
> +            )
> +        };
> +        match T::round_waveform_tohw(chip, pwm, &wf) {
> +            Ok(rounded) => {
> +                // SAFETY: `wfhw_ptr` is valid per this function's safety contract.
> +                if unsafe { Self::serialize_wfhw(&rounded.hardware_waveform, wfhw_ptr) }.is_err() {
> +                    return EINVAL.to_errno();
> +                }
> +                rounded.status
> +            }
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Pointers from C must be valid.
> +    unsafe extern "C" fn round_waveform_fromhw_callback(
> +        chip_ptr: *mut bindings::pwm_chip,
> +        pwm_ptr: *mut bindings::pwm_device,
> +        wfhw_ptr: *const c_void,
> +        wf_ptr: *mut bindings::pwm_waveform,
> +    ) -> c_int {
> +        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
> +        // pointers.
> +        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
> +        // SAFETY: `deserialize_wfhw`'s safety contract is met by this function's contract.
> +        let wfhw = match unsafe { Self::deserialize_wfhw(wfhw_ptr) } {
> +            Ok(v) => v,
> +            Err(e) => return e.to_errno(),
> +        };
> +
> +        let mut rust_wf = Waveform::default();
> +        match T::round_waveform_fromhw(chip, pwm, &wfhw, &mut rust_wf) {
> +            Ok(()) => {
> +                // SAFETY: `wf_ptr` is guaranteed valid by the C caller.
> +                unsafe {
> +                    *wf_ptr = rust_wf.into();
> +                };
> +                0
> +            }
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Pointers from C must be valid.
> +    unsafe extern "C" fn read_waveform_callback(
> +        chip_ptr: *mut bindings::pwm_chip,
> +        pwm_ptr: *mut bindings::pwm_device,
> +        wfhw_ptr: *mut c_void,
> +    ) -> c_int {
> +        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
> +        // pointers.
> +        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
> +
> +        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
> +        let bound_parent = unsafe { chip.bound_parent_device() };
> +        match T::read_waveform(chip, pwm, bound_parent) {
> +            // SAFETY: `wfhw_ptr` is valid per this function's safety contract.
> +            Ok(wfhw) => match unsafe { Self::serialize_wfhw(&wfhw, wfhw_ptr) } {
> +                Ok(()) => 0,
> +                Err(e) => e.to_errno(),
> +            },
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Pointers from C must be valid.
> +    unsafe extern "C" fn write_waveform_callback(
> +        chip_ptr: *mut bindings::pwm_chip,
> +        pwm_ptr: *mut bindings::pwm_device,
> +        wfhw_ptr: *const c_void,
> +    ) -> c_int {
> +        // SAFETY: Relies on the function's contract that `chip_ptr` and `pwm_ptr` are valid
> +        // pointers.
> +        let (chip, pwm) = unsafe { (Chip::<T>::from_raw(chip_ptr), Device::from_raw(pwm_ptr)) };
> +
> +        // SAFETY: The PWM core guarantees the parent device exists and is bound during callbacks.
> +        let bound_parent = unsafe { chip.bound_parent_device() };
> +
> +        // SAFETY: `wfhw_ptr` is valid per this function's safety contract.
> +        let wfhw = match unsafe { Self::deserialize_wfhw(wfhw_ptr) } {
> +            Ok(v) => v,
> +            Err(e) => return e.to_errno(),
> +        };
> +        match T::write_waveform(chip, pwm, &wfhw, bound_parent) {
> +            Ok(()) => 0,
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +}
> +
> +/// VTable structure wrapper for PWM operations.
> +/// Mirrors [`struct pwm_ops`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct PwmOpsVTable(bindings::pwm_ops);
> +
> +// SAFETY: PwmOpsVTable is Send. The vtable contains only function pointers
> +// and a size, which are simple data types that can be safely moved across
> +// threads. The thread-safety of calling these functions is handled by the
> +// kernel's locking mechanisms.
> +unsafe impl Send for PwmOpsVTable {}
> +
> +// SAFETY: PwmOpsVTable is Sync. The vtable is immutable after it is created,
> +// so it can be safely referenced and accessed concurrently by multiple threads
> +// e.g. to read the function pointers.
> +unsafe impl Sync for PwmOpsVTable {}
> +
> +impl PwmOpsVTable {
> +    /// Returns a raw pointer to the underlying `pwm_ops` struct.
> +    pub(crate) fn as_raw(&self) -> *const bindings::pwm_ops {
> +        &self.0
> +    }
> +}
> +
> +/// Creates a PWM operations vtable for a type `T` that implements `PwmOps`.
> +///
> +/// This is used to bridge Rust trait implementations to the C `struct pwm_ops`
> +/// expected by the kernel.
> +pub const fn create_pwm_ops<T: PwmOps>() -> PwmOpsVTable {
> +    // SAFETY: `core::mem::zeroed()` is unsafe. For `pwm_ops`, all fields are
> +    // `Option<extern "C" fn(...)>` or data, so a zeroed pattern (None/0) is valid initially.
> +    let mut ops: bindings::pwm_ops = unsafe { core::mem::zeroed() };
> +
> +    ops.request = Some(Adapter::<T>::request_callback);
> +    ops.capture = Some(Adapter::<T>::capture_callback);
> +
> +    ops.round_waveform_tohw = Some(Adapter::<T>::round_waveform_tohw_callback);
> +    ops.round_waveform_fromhw = Some(Adapter::<T>::round_waveform_fromhw_callback);
> +    ops.read_waveform = Some(Adapter::<T>::read_waveform_callback);
> +    ops.write_waveform = Some(Adapter::<T>::write_waveform_callback);
> +    ops.sizeof_wfhw = core::mem::size_of::<T::WfHw>();
> +
> +    PwmOpsVTable(ops)
> +}
> +
> +/// Wrapper for a PWM chip/controller ([`struct pwm_chip`](srctree/include/linux/pwm.h)).
> +#[repr(transparent)]
> +pub struct Chip<T: PwmOps>(Opaque<bindings::pwm_chip>, PhantomData<T>);
> +
> +impl<T: PwmOps> Chip<T> {
> +    /// Creates a reference to a [`Chip`] from a valid pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
> +    /// returned [`Chip`] reference.
> +    pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::pwm_chip) -> &'a Self {
> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> +        // `Chip` type being transparent makes the cast ok.
> +        unsafe { &*ptr.cast::<Self>() }
> +    }
> +
> +    /// Returns a raw pointer to the underlying `pwm_chip`.
> +    pub(crate) fn as_raw(&self) -> *mut bindings::pwm_chip {
> +        self.0.get()
> +    }
> +
> +    /// Gets the number of PWM channels (hardware PWMs) on this chip.
> +    pub fn num_channels(&self) -> u32 {
> +        // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
> +        unsafe { (*self.as_raw()).npwm }
> +    }
> +
> +    /// Returns `true` if the chip supports atomic operations for configuration.
> +    pub fn is_atomic(&self) -> bool {
> +        // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
> +        unsafe { (*self.as_raw()).atomic }
> +    }
> +
> +    /// Returns a reference to the embedded `struct device` abstraction.
> +    pub fn device(&self) -> &device::Device {
> +        // SAFETY:
> +        // - `self.as_raw()` provides a valid pointer to `bindings::pwm_chip`.
> +        // - The `dev` field is an instance of `bindings::device` embedded
> +        //   within `pwm_chip`.
> +        // - Taking a pointer to this embedded field is valid.
> +        // - `device::Device` is `#[repr(transparent)]`.
> +        // - The lifetime of the returned reference is tied to `self`.
> +        unsafe { device::Device::from_raw(&raw mut (*self.as_raw()).dev) }
> +    }
> +
> +    /// Gets the typed driver specific data associated with this chip's embedded device.
> +    pub fn drvdata(&self) -> &T {
> +        // SAFETY: `pwmchip_get_drvdata` returns the pointer to the private data area,
> +        // which we know holds our `T`. The pointer is valid for the lifetime of `self`.
> +        unsafe { &*bindings::pwmchip_get_drvdata(self.as_raw()).cast::<T>() }
> +    }
> +
> +    /// Returns a reference to the parent device of this PWM chip's device.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must guarantee that the parent device exists and is bound.
> +    /// This is guaranteed by the PWM core during `PwmOps` callbacks.
> +    unsafe fn bound_parent_device(&self) -> &device::Device<Bound> {
> +        // SAFETY: Per the function's safety contract, the parent device exists.
> +        let parent = unsafe { self.device().parent().unwrap_unchecked() };
> +
> +        // SAFETY: Per the function's safety contract, the parent device is bound.
> +        // This is guaranteed by the PWM core during `PwmOps` callbacks.
> +        unsafe { parent.as_bound() }
> +    }
> +
> +    /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
> +    ///
> +    /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
> +    /// on its embedded `struct device`.
> +    pub fn new(
> +        parent_dev: &device::Device,
> +        num_channels: u32,
> +        data: impl pin_init::PinInit<T, Error>,
> +    ) -> Result<ARef<Self>> {
> +        let sizeof_priv = core::mem::size_of::<T>();
> +        // SAFETY: `pwmchip_alloc` allocates memory for the C struct and our private data.
> +        let c_chip_ptr_raw = unsafe {
> +            bindings::pwmchip_alloc(parent_dev.as_raw(), num_channels, sizeof_priv)
> +        };
> +
> +        let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
> +
> +        // SAFETY: The `drvdata` pointer is the start of the private area, which is where
> +        // we will construct our `T` object.
> +        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
> +
> +        // SAFETY: We construct the `T` object in-place in the allocated private memory.
> +        unsafe { data.__pinned_init(drvdata_ptr.cast())? };
> +
> +        // SAFETY: `c_chip_ptr` points to a valid chip.
> +        unsafe { (*c_chip_ptr).dev.release = Some(Adapter::<T>::release_callback); }
> +
> +        // SAFETY: `c_chip_ptr` points to a valid chip.
> +        // The `Adapter`'s `VTABLE` has a 'static lifetime, so the pointer
> +        // returned by `as_raw()` is always valid.
> +        unsafe { (*c_chip_ptr).ops = Adapter::<T>::VTABLE.as_raw(); }
> +
> +        // Cast the `*mut bindings::pwm_chip` to `*mut Chip`. This is valid because
> +        // `Chip` is `repr(transparent)` over `Opaque<bindings::pwm_chip>`, and
> +        // `Opaque<T>` is `repr(transparent)` over `T`.
> +        let chip_ptr_as_self = c_chip_ptr.cast::<Self>();
> +
> +        // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
> +        // `bindings::pwm_chip`) whose embedded device has refcount 1.
> +        // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
> +        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
> +    }
> +}
> +
> +// SAFETY: Implements refcounting for `Chip` using the embedded `struct device`.
> +unsafe impl<T: PwmOps> AlwaysRefCounted for Chip<T> {
> +    #[inline]
> +    fn inc_ref(&self) {
> +        // SAFETY: `self.0.get()` points to a valid `pwm_chip` because `self` exists.
> +        // The embedded `dev` is valid. `get_device` increments its refcount.
> +        unsafe { bindings::get_device(&raw mut (*self.0.get()).dev); }
> +    }
> +
> +    #[inline]
> +    unsafe fn dec_ref(obj: NonNull<Chip<T>>) {
> +        let c_chip_ptr = obj.cast::<bindings::pwm_chip>().as_ptr();
> +
> +        // SAFETY: `obj` is a valid pointer to a `Chip` (and thus `bindings::pwm_chip`)
> +        // with a non-zero refcount. `put_device` handles decrement and final release.
> +        unsafe { bindings::put_device(&raw mut (*c_chip_ptr).dev); }
> +    }
> +}
> +
> +// SAFETY: `Chip` is a wrapper around `*mut bindings::pwm_chip`. The underlying C
> +// structure's state is managed and synchronized by the kernel's device model
> +// and PWM core locking mechanisms. Therefore, it is safe to move the `Chip`
> +// wrapper (and the pointer it contains) across threads.
> +unsafe impl<T: PwmOps + Send> Send for Chip<T> {}
> +
> +// SAFETY: It is safe for multiple threads to have shared access (`&Chip`) because
> +// the `Chip` data is immutable from the Rust side without holding the appropriate
> +// kernel locks, which the C core is responsible for. Any interior mutability is
> +// handled and synchronized by the C kernel code.
> +unsafe impl<T: PwmOps + Sync> Sync for Chip<T> {}
> +
> +/// A resource guard that ensures `pwmchip_remove` is called on drop.
> +///
> +/// This struct is intended to be managed by the `devres` framework by transferring its ownership
> +/// via [`Devres::register`]. This ties the lifetime of the PWM chip registration
> +/// to the lifetime of the underlying device.
> +pub struct Registration<T: PwmOps> {
> +    chip: ARef<Chip<T>>,
> +}
> +
> +impl<T: 'static + PwmOps + Send + Sync> Registration<T> {
> +    /// Registers a PWM chip with the PWM subsystem.
> +    ///
> +    /// Transfers its ownership to the `devres` framework, which ties its lifetime
> +    /// to the parent device.
> +    /// On unbind of the parent device, the `devres` entry will be dropped, automatically
> +    /// calling `pwmchip_remove`. This function should be called from the driver's `probe`.
> +    pub fn register(
> +        dev: &device::Device<Bound>,
> +        chip: ARef<Chip<T>>,
> +    ) -> Result {
> +	let chip_parent = chip.device().parent().ok_or(EINVAL)?;
> +        if dev.as_raw() != chip_parent.as_raw() {
> +            return Err(EINVAL);
> +        }
> +
> +        let c_chip_ptr = chip.as_raw();
> +
> +        // SAFETY: `c_chip_ptr` points to a valid chip with its ops initialized.
> +        // `__pwmchip_add` is the C function to register the chip with the PWM core.
> +        unsafe {
> +            to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
> +        }
> +
> +        let registration = Registration { chip };
> +
> +        devres::register(dev, registration, GFP_KERNEL)
> +    }
> +}
> +
> +impl<T: PwmOps> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        let chip_raw = self.chip.as_raw();
> +
> +        // SAFETY: `chip_raw` points to a chip that was successfully registered.
> +        // `bindings::pwmchip_remove` is the correct C function to unregister it.
> +        // This `drop` implementation is called automatically by `devres` on driver unbind.
> +        unsafe { bindings::pwmchip_remove(chip_raw); }
> +    }
> +}
> 
> -- 
> 2.34.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 3/7] rust: pwm: Add complete abstraction layer

[Michal Wilczynski]

On 8/20/25 23:11, Elle Rhumsaa wrote:

>> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::pwm_device>);
>> +
>> +impl Device {
>> +    /// Creates a reference to a [`Device`] from a valid C pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
>> +    /// returned [`Device`] reference.
>> +    pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::pwm_device) -> &'a Self {
>> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
>> +        // `Device` type being transparent makes the cast ok.
>> +        unsafe { &*ptr.cast::<Self>() }
>> +    }
> 
> This looks like the only constructor for `Device`, and it has restricted
> visibility.
> 
> Would it be helpful to provide a doc-test here to show intended usage?
> 
> After looking at the C code, it looks like the raw `pwm_device` pointer
> is extracted from a `device` struct, or stored in an array of
> `pwm_device` pointers.
> 
> Is the intent that the `pwm_device` will always be allocated from C
> code? For example, is a `pub fn new() -> &'a Self`-style constructor
> not possible/desired?
> 

You're correct the intent is for pwm_device to be allocated and managed
by the C core. The Rust code only ever receives a raw pointer to an
existing device during callbacks.

For this reason, a public new() constructor isn't appropriate, and the
from_raw function is pub(crate) for use only by the FFI adapter. A
doc-test is also impractical since we can't create a valid pwm_device in
a test environment.

> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> 

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski]

Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
utilizing the safe PWM abstractions from the preceding commit.

The driver implements the pwm::PwmOps trait using the modern waveform
API (round_waveform_tohw, write_waveform, etc.) to support configuration
of period, duty cycle, and polarity for the TH1520's PWM channels.

Resource management is handled using idiomatic Rust patterns. The PWM
chip object is allocated via pwm::Chip::new and its registration with
the PWM core is managed by the pwm::Registration RAII guard. This
ensures pwmchip_remove is always called when the driver unbinds,
preventing resource leaks. Device managed resources are used for the
MMIO region, and the clock lifecycle is correctly managed in the
driver's private data Drop implementation.

The driver's core logic is written entirely in safe Rust, with no unsafe
blocks, except for the Send and Sync implementations for the driver
data, which are explained in the comments.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d7c0676c1d00a02b3d7db2de88b039c08c99c6e..d79dc21f22d143ca8cde6a06194545fbc640e695 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21741,6 +21741,7 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
 F:	drivers/pinctrl/pinctrl-th1520.c
 F:	drivers/pmdomain/thead/
 F:	drivers/power/sequencing/pwrseq-thead-gpu.c
+F:	drivers/pwm/pwm_th1520.rs
 F:	drivers/reset/reset-th1520.c
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 F:	include/dt-bindings/power/thead,th1520-power.h
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 2b608f4378138775ee3ba4d53f682952e1914118..dd6db01832ee985e2e588a413a13df869a029d3d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -729,6 +729,17 @@ config PWM_TEGRA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-tegra.
 
+config PWM_TH1520
+	tristate "TH1520 PWM support"
+	depends on RUST
+	select RUST_PWM_ABSTRACTIONS
+	help
+	  This option enables the driver for the PWM controller found on the
+	  T-HEAD TH1520 SoC.
+
+	  To compile this driver as a module, choose M here; the module
+	  will be called pwm-th1520. If you are unsure, say N.
+
 config PWM_TIECAP
 	tristate "ECAP PWM support"
 	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ff4f47e5fb7a0dbac72c12de82c3773e5582db6d..5c15c95c6e49143969389198657eed0ecf4086b2 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
 obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
+obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
new file mode 100644
index 0000000000000000000000000000000000000000..5ef887b1b5dfed92c5d4b87a7d48f673352a257e
--- /dev/null
+++ b/drivers/pwm/pwm_th1520.rs
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! Rust T-HEAD TH1520 PWM driver
+//!
+//! Limitations:
+//! - The period and duty cycle are controlled by 32-bit hardware registers,
+//!   limiting the maximum resolution.
+//! - The driver supports continuous output mode only; one-shot mode is not
+//!   implemented.
+//! - The controller hardware provides up to 6 PWM channels.
+//! - Reconfiguration is glitch free - new period and duty cycle values are
+//!   latched and take effect at the start of the next period.
+//! - Polarity is handled via a simple hardware inversion bit; arbitrary
+//!   duty cycle offsets are not supported.
+//! - Disabling a channel is achieved by configuring its duty cycle to zero to
+//!   produce a static low output. Clearing the `start` does not reliably
+//!   force the static inactive level defined by the `INACTOUT` bit. Hence
+//!   this method is not used in this driver.
+//!
+
+use core::ops::Deref;
+use kernel::{
+    c_str,
+    clk::Clk,
+    device::{Bound, Core, Device},
+    devres,
+    io::mem::IoMem,
+    of, platform,
+    prelude::*,
+    pwm, time,
+};
+
+const TH1520_MAX_PWM_NUM: u32 = 6;
+
+// Register offsets
+const fn th1520_pwm_chn_base(n: u32) -> usize {
+    (n * 0x20) as usize
+}
+
+const fn th1520_pwm_ctrl(n: u32) -> usize {
+    th1520_pwm_chn_base(n)
+}
+
+const fn th1520_pwm_per(n: u32) -> usize {
+    th1520_pwm_chn_base(n) + 0x08
+}
+
+const fn th1520_pwm_fp(n: u32) -> usize {
+    th1520_pwm_chn_base(n) + 0x0c
+}
+
+// Control register bits
+const TH1520_PWM_START: u32 = 1 << 0;
+const TH1520_PWM_CFG_UPDATE: u32 = 1 << 2;
+const TH1520_PWM_CONTINUOUS_MODE: u32 = 1 << 5;
+const TH1520_PWM_FPOUT: u32 = 1 << 8;
+
+const TH1520_PWM_REG_SIZE: usize = 0xB0;
+
+fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
+    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
+
+    (match ns.checked_mul(rate_hz) {
+        Some(product) => product,
+        None => u64::MAX,
+    }) / NSEC_PER_SEC_U64
+}
+
+fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
+    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
+
+    // Round up
+    let Some(numerator) = cycles
+        .checked_mul(NSEC_PER_SEC_U64)
+        .and_then(|p| p.checked_add(rate_hz - 1))
+    else {
+        return u64::MAX;
+    };
+
+    numerator / rate_hz
+}
+
+/// Hardware-specific waveform representation for TH1520.
+#[derive(Copy, Clone, Debug, Default)]
+struct Th1520WfHw {
+    period_cycles: u32,
+    duty_cycles: u32,
+    ctrl_val: u32,
+    enabled: bool,
+}
+
+/// The driver's private data struct. It holds all necessary devres managed resources.
+#[pin_data(PinnedDrop)]
+struct Th1520PwmDriverData {
+    #[pin]
+    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
+    clk: Clk,
+}
+
+// This `unsafe` implementation is a temporary necessity because the underlying `kernel::clk::Clk`
+// type does not yet expose `Send` and `Sync` implementations. This block should be removed
+// as soon as the clock abstraction provides these guarantees directly.
+// TODO: Remove those unsafe impl's when Clk will support them itself.
+
+// SAFETY: The `devres` framework requires the driver's private data to be `Send` and `Sync`.
+// We can guarantee this because the PWM core synchronizes all callbacks, preventing concurrent
+// access to the contained `iomem` and `clk` resources.
+unsafe impl Send for Th1520PwmDriverData {}
+
+// SAFETY: The same reasoning applies as for `Send`. The PWM core's synchronization
+// guarantees that it is safe for multiple threads to have shared access (`&self`)
+// to the driver data during callbacks.
+unsafe impl Sync for Th1520PwmDriverData {}
+
+impl pwm::PwmOps for Th1520PwmDriverData {
+    type WfHw = Th1520WfHw;
+
+    fn round_waveform_tohw(
+        chip: &pwm::Chip<Self>,
+        _pwm: &pwm::Device,
+        wf: &pwm::Waveform,
+    ) -> Result<pwm::RoundedWaveform<Self::WfHw>> {
+        let data = chip.drvdata();
+
+        if wf.period_length_ns == 0 {
+            return Ok(pwm::RoundedWaveform {
+                status: 0,
+                hardware_waveform: Th1520WfHw {
+                    enabled: false,
+                    ..Default::default()
+                },
+            });
+        }
+
+        let rate_hz = data.clk.rate().as_hz() as u64;
+
+        let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u64::from(u32::MAX));
+        let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u64::from(u32::MAX));
+
+        let mut ctrl_val = TH1520_PWM_CONTINUOUS_MODE;
+
+        let is_inversed = wf.duty_length_ns > 0
+            && wf.duty_offset_ns > 0
+            && wf.duty_length_ns + wf.duty_offset_ns >= wf.period_length_ns;
+        if is_inversed {
+            duty_cycles = period_cycles - duty_cycles;
+        } else {
+            ctrl_val |= TH1520_PWM_FPOUT;
+        }
+
+        let wfhw = Th1520WfHw {
+            period_cycles: period_cycles as u32,
+            duty_cycles: duty_cycles as u32,
+            ctrl_val,
+            enabled: true,
+        };
+
+        dev_dbg!(
+            chip.device(),
+            "clk_rate: {}Hz Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
+            rate_hz,
+            wf.period_length_ns,
+            wf.duty_length_ns,
+            wf.duty_offset_ns,
+            wfhw.period_cycles,
+            wfhw.duty_cycles,
+            wfhw.ctrl_val
+        );
+
+        Ok(pwm::RoundedWaveform {
+            status: 0,
+            hardware_waveform: wfhw,
+        })
+    }
+
+    fn round_waveform_fromhw(
+        chip: &pwm::Chip<Self>,
+        _pwm: &pwm::Device,
+        wfhw: &Self::WfHw,
+        wf: &mut pwm::Waveform,
+    ) -> Result {
+        let data = chip.drvdata();
+        let rate_hz = data.clk.rate().as_hz() as u64;
+
+        wf.period_length_ns = cycles_to_ns(u64::from(wfhw.period_cycles), rate_hz);
+
+        let duty_cycles = u64::from(wfhw.duty_cycles);
+
+        if (wfhw.ctrl_val & TH1520_PWM_FPOUT) != 0 {
+            wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
+            wf.duty_offset_ns = 0;
+        } else {
+            let period_cycles = u64::from(wfhw.period_cycles);
+            let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
+
+            // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
+            wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
+            // The offset is the initial low time, which is what the hardware register provides.
+            wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
+        }
+
+        Ok(())
+    }
+
+    fn read_waveform(
+        chip: &pwm::Chip<Self>,
+        pwm: &pwm::Device,
+        parent_dev: &Device<Bound>,
+    ) -> Result<Self::WfHw> {
+        let data = chip.drvdata();
+        let hwpwm = pwm.hwpwm();
+        let iomem_accessor = data.iomem.access(parent_dev)?;
+        let iomap = iomem_accessor.deref();
+
+        let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
+        let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
+        let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
+
+        let wfhw = Th1520WfHw {
+            period_cycles,
+            duty_cycles,
+            ctrl_val: ctrl,
+            enabled: duty_cycles != 0,
+        };
+
+        dev_dbg!(
+            chip.device(),
+            "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
+            hwpwm,
+            wfhw.period_cycles,
+            wfhw.duty_cycles,
+            wfhw.ctrl_val,
+            wfhw.enabled
+        );
+
+        Ok(wfhw)
+    }
+
+    fn write_waveform(
+        chip: &pwm::Chip<Self>,
+        pwm: &pwm::Device,
+        wfhw: &Self::WfHw,
+        parent_dev: &Device<Bound>,
+    ) -> Result {
+        let data = chip.drvdata();
+        let hwpwm = pwm.hwpwm();
+        let iomem_accessor = data.iomem.access(parent_dev)?;
+        let iomap = iomem_accessor.deref();
+        let was_enabled = pwm.state().enabled();
+
+        if !wfhw.enabled {
+            if was_enabled {
+                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
+                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
+                iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
+            }
+            return Ok(());
+        }
+
+        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
+        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
+        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
+        iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
+
+        // The `TH1520_PWM_START` bit must be written in a separate, final transaction, and
+        // only when enabling the channel from a disabled state.
+        if !was_enabled {
+            iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_START, th1520_pwm_ctrl(hwpwm))?;
+        }
+
+        dev_dbg!(
+            chip.device(),
+            "PWM-{}: Wrote (per: {}, duty: {})",
+            hwpwm,
+            wfhw.period_cycles,
+            wfhw.duty_cycles,
+        );
+
+        Ok(())
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for Th1520PwmDriverData {
+    fn drop(self: Pin<&mut Self>) {
+        self.clk.disable_unprepare();
+    }
+}
+
+struct Th1520PwmPlatformDriver;
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
+    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
+);
+
+impl platform::Driver for Th1520PwmPlatformDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(
+        pdev: &platform::Device<Core>,
+        _id_info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
+        let dev = pdev.as_ref();
+        let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
+
+        let clk = Clk::get(dev, None)?;
+
+        clk.prepare_enable()?;
+
+        // TODO: Get exclusive ownership of the clock to prevent rate changes.
+        // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
+        // This should be updated once it is implemented.
+        let rate_hz = clk.rate().as_hz();
+        if rate_hz == 0 {
+            dev_err!(dev, "Clock rate is zero\n");
+            return Err(EINVAL);
+        }
+
+        if rate_hz > time::NSEC_PER_SEC as usize {
+            dev_err!(
+                dev,
+                "Clock rate {} Hz is too high, not supported.\n",
+                rate_hz
+            );
+            return Err(ERANGE);
+        }
+
+        let chip = pwm::Chip::new(
+            dev,
+            TH1520_MAX_PWM_NUM,
+            try_pin_init!(Th1520PwmDriverData {
+                iomem <- request.iomap_sized::<TH1520_PWM_REG_SIZE>(),
+                clk <- clk,
+            }),
+        )?;
+
+        pwm::Registration::register(dev, chip)?;
+
+        Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
+    }
+}
+
+kernel::module_platform_driver! {
+    type: Th1520PwmPlatformDriver,
+    name: "pwm-th1520",
+    authors: ["Michal Wilczynski <m.wilczynski@samsung.com>"],
+    description: "T-HEAD TH1520 PWM driver",
+    license: "GPL v2",
+}

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:39AM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
> utilizing the safe PWM abstractions from the preceding commit.
> 
> The driver implements the pwm::PwmOps trait using the modern waveform
> API (round_waveform_tohw, write_waveform, etc.) to support configuration
> of period, duty cycle, and polarity for the TH1520's PWM channels.
> 
> Resource management is handled using idiomatic Rust patterns. The PWM
> chip object is allocated via pwm::Chip::new and its registration with
> the PWM core is managed by the pwm::Registration RAII guard. This
> ensures pwmchip_remove is always called when the driver unbinds,
> preventing resource leaks. Device managed resources are used for the
> MMIO region, and the clock lifecycle is correctly managed in the
> driver's private data Drop implementation.
> 
> The driver's core logic is written entirely in safe Rust, with no unsafe
> blocks, except for the Send and Sync implementations for the driver
> data, which are explained in the comments.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  11 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm_th1520.rs | 355 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 368 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d7c0676c1d00a02b3d7db2de88b039c08c99c6e..d79dc21f22d143ca8cde6a06194545fbc640e695 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21741,6 +21741,7 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>  F:	drivers/pinctrl/pinctrl-th1520.c
>  F:	drivers/pmdomain/thead/
>  F:	drivers/power/sequencing/pwrseq-thead-gpu.c
> +F:	drivers/pwm/pwm_th1520.rs
>  F:	drivers/reset/reset-th1520.c
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>  F:	include/dt-bindings/power/thead,th1520-power.h
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 2b608f4378138775ee3ba4d53f682952e1914118..dd6db01832ee985e2e588a413a13df869a029d3d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -729,6 +729,17 @@ config PWM_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tegra.
>  
> +config PWM_TH1520
> +	tristate "TH1520 PWM support"
> +	depends on RUST
> +	select RUST_PWM_ABSTRACTIONS
> +	help
> +	  This option enables the driver for the PWM controller found on the
> +	  T-HEAD TH1520 SoC.
> +
> +	  To compile this driver as a module, choose M here; the module
> +	  will be called pwm-th1520. If you are unsure, say N.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ff4f47e5fb7a0dbac72c12de82c3773e5582db6d..5c15c95c6e49143969389198657eed0ecf4086b2 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> +obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..5ef887b1b5dfed92c5d4b87a7d48f673352a257e
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +//!
> +//! Limitations:
> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
> +//!   limiting the maximum resolution.
> +//! - The driver supports continuous output mode only; one-shot mode is not
> +//!   implemented.
> +//! - The controller hardware provides up to 6 PWM channels.
> +//! - Reconfiguration is glitch free - new period and duty cycle values are
> +//!   latched and take effect at the start of the next period.
> +//! - Polarity is handled via a simple hardware inversion bit; arbitrary
> +//!   duty cycle offsets are not supported.
> +//! - Disabling a channel is achieved by configuring its duty cycle to zero to
> +//!   produce a static low output. Clearing the `start` does not reliably
> +//!   force the static inactive level defined by the `INACTOUT` bit. Hence
> +//!   this method is not used in this driver.
> +//!
> +
> +use core::ops::Deref;
> +use kernel::{
> +    c_str,
> +    clk::Clk,
> +    device::{Bound, Core, Device},
> +    devres,
> +    io::mem::IoMem,
> +    of, platform,
> +    prelude::*,
> +    pwm, time,
> +};
> +
> +const TH1520_MAX_PWM_NUM: u32 = 6;
> +
> +// Register offsets
> +const fn th1520_pwm_chn_base(n: u32) -> usize {
> +    (n * 0x20) as usize
> +}
> +
> +const fn th1520_pwm_ctrl(n: u32) -> usize {
> +    th1520_pwm_chn_base(n)
> +}
> +
> +const fn th1520_pwm_per(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x08
> +}
> +
> +const fn th1520_pwm_fp(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +// Control register bits
> +const TH1520_PWM_START: u32 = 1 << 0;
> +const TH1520_PWM_CFG_UPDATE: u32 = 1 << 2;
> +const TH1520_PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const TH1520_PWM_FPOUT: u32 = 1 << 8;
> +
> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> +
> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> +    (match ns.checked_mul(rate_hz) {
> +        Some(product) => product,
> +        None => u64::MAX,
> +    }) / NSEC_PER_SEC_U64
> +}
> +
> +fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> +    // Round up
> +    let Some(numerator) = cycles
> +        .checked_mul(NSEC_PER_SEC_U64)
> +        .and_then(|p| p.checked_add(rate_hz - 1))
> +    else {
> +        return u64::MAX;
> +    };
> +
> +    numerator / rate_hz
> +}
> +
> +/// Hardware-specific waveform representation for TH1520.
> +#[derive(Copy, Clone, Debug, Default)]
> +struct Th1520WfHw {
> +    period_cycles: u32,
> +    duty_cycles: u32,
> +    ctrl_val: u32,
> +    enabled: bool,
> +}
> +
> +/// The driver's private data struct. It holds all necessary devres managed resources.
> +#[pin_data(PinnedDrop)]
> +struct Th1520PwmDriverData {
> +    #[pin]
> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> +    clk: Clk,
> +}
> +
> +// This `unsafe` implementation is a temporary necessity because the underlying `kernel::clk::Clk`
> +// type does not yet expose `Send` and `Sync` implementations. This block should be removed
> +// as soon as the clock abstraction provides these guarantees directly.
> +// TODO: Remove those unsafe impl's when Clk will support them itself.
> +
> +// SAFETY: The `devres` framework requires the driver's private data to be `Send` and `Sync`.
> +// We can guarantee this because the PWM core synchronizes all callbacks, preventing concurrent
> +// access to the contained `iomem` and `clk` resources.
> +unsafe impl Send for Th1520PwmDriverData {}
> +
> +// SAFETY: The same reasoning applies as for `Send`. The PWM core's synchronization
> +// guarantees that it is safe for multiple threads to have shared access (`&self`)
> +// to the driver data during callbacks.
> +unsafe impl Sync for Th1520PwmDriverData {}
> +
> +impl pwm::PwmOps for Th1520PwmDriverData {
> +    type WfHw = Th1520WfHw;
> +
> +    fn round_waveform_tohw(
> +        chip: &pwm::Chip<Self>,
> +        _pwm: &pwm::Device,
> +        wf: &pwm::Waveform,
> +    ) -> Result<pwm::RoundedWaveform<Self::WfHw>> {
> +        let data = chip.drvdata();
> +
> +        if wf.period_length_ns == 0 {
> +            return Ok(pwm::RoundedWaveform {
> +                status: 0,
> +                hardware_waveform: Th1520WfHw {
> +                    enabled: false,
> +                    ..Default::default()
> +                },
> +            });
> +        }
> +
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u64::from(u32::MAX));
> +        let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u64::from(u32::MAX));
> +
> +        let mut ctrl_val = TH1520_PWM_CONTINUOUS_MODE;
> +
> +        let is_inversed = wf.duty_length_ns > 0
> +            && wf.duty_offset_ns > 0
> +            && wf.duty_length_ns + wf.duty_offset_ns >= wf.period_length_ns;
> +        if is_inversed {
> +            duty_cycles = period_cycles - duty_cycles;
> +        } else {
> +            ctrl_val |= TH1520_PWM_FPOUT;
> +        }
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles: period_cycles as u32,
> +            duty_cycles: duty_cycles as u32,
> +            ctrl_val,
> +            enabled: true,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "clk_rate: {}Hz Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
> +            rate_hz,
> +            wf.period_length_ns,
> +            wf.duty_length_ns,
> +            wf.duty_offset_ns,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val
> +        );
> +
> +        Ok(pwm::RoundedWaveform {
> +            status: 0,
> +            hardware_waveform: wfhw,
> +        })
> +    }
> +
> +    fn round_waveform_fromhw(
> +        chip: &pwm::Chip<Self>,
> +        _pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        wf: &mut pwm::Waveform,
> +    ) -> Result {
> +        let data = chip.drvdata();
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        wf.period_length_ns = cycles_to_ns(u64::from(wfhw.period_cycles), rate_hz);
> +
> +        let duty_cycles = u64::from(wfhw.duty_cycles);
> +
> +        if (wfhw.ctrl_val & TH1520_PWM_FPOUT) != 0 {
> +            wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
> +            wf.duty_offset_ns = 0;
> +        } else {
> +            let period_cycles = u64::from(wfhw.period_cycles);
> +            let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
> +
> +            // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
> +            wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> +            // The offset is the initial low time, which is what the hardware register provides.
> +            wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
> +        }
> +
> +        Ok(())
> +    }
> +
> +    fn read_waveform(
> +        chip: &pwm::Chip<Self>,
> +        pwm: &pwm::Device,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result<Self::WfHw> {
> +        let data = chip.drvdata();
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_accessor = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_accessor.deref();
> +
> +        let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
> +        let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
> +        let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles,
> +            duty_cycles,
> +            ctrl_val: ctrl,
> +            enabled: duty_cycles != 0,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val,
> +            wfhw.enabled
> +        );
> +
> +        Ok(wfhw)
> +    }
> +
> +    fn write_waveform(
> +        chip: &pwm::Chip<Self>,
> +        pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result {
> +        let data = chip.drvdata();
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_accessor = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_accessor.deref();
> +        let was_enabled = pwm.state().enabled();
> +
> +        if !wfhw.enabled {
> +            if was_enabled {
> +                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
> +                iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +            }
> +            return Ok(());
> +        }
> +
> +        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
> +        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
> +        iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +
> +        // The `TH1520_PWM_START` bit must be written in a separate, final transaction, and
> +        // only when enabling the channel from a disabled state.
> +        if !was_enabled {
> +            iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_START, th1520_pwm_ctrl(hwpwm))?;
> +        }
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: Wrote (per: {}, duty: {})",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +        );
> +
> +        Ok(())
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for Th1520PwmDriverData {
> +    fn drop(self: Pin<&mut Self>) {
> +        self.clk.disable_unprepare();
> +    }
> +}
> +
> +struct Th1520PwmPlatformDriver;
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
> +    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
> +);
> +
> +impl platform::Driver for Th1520PwmPlatformDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _id_info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let dev = pdev.as_ref();
> +        let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
> +
> +        let clk = Clk::get(dev, None)?;
> +
> +        clk.prepare_enable()?;
> +
> +        // TODO: Get exclusive ownership of the clock to prevent rate changes.
> +        // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
> +        // This should be updated once it is implemented.
> +        let rate_hz = clk.rate().as_hz();
> +        if rate_hz == 0 {
> +            dev_err!(dev, "Clock rate is zero\n");
> +            return Err(EINVAL);
> +        }
> +
> +        if rate_hz > time::NSEC_PER_SEC as usize {
> +            dev_err!(
> +                dev,
> +                "Clock rate {} Hz is too high, not supported.\n",
> +                rate_hz
> +            );
> +            return Err(ERANGE);
> +        }
> +
> +        let chip = pwm::Chip::new(
> +            dev,
> +            TH1520_MAX_PWM_NUM,
> +            try_pin_init!(Th1520PwmDriverData {
> +                iomem <- request.iomap_sized::<TH1520_PWM_REG_SIZE>(),
> +                clk <- clk,
> +            }),
> +        )?;
> +
> +        pwm::Registration::register(dev, chip)?;
> +
> +        Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
> +    }
> +}
> +
> +kernel::module_platform_driver! {
> +    type: Th1520PwmPlatformDriver,
> +    name: "pwm-th1520",
> +    authors: ["Michal Wilczynski <m.wilczynski@samsung.com>"],
> +    description: "T-HEAD TH1520 PWM driver",
> +    license: "GPL v2",
> +}
> 
> -- 
> 2.34.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 16368 bytes --]

Hello,

actually I intended to give my feedback as a patch, but I'm not fluent
enough in Rust yet :-\, so here come my thoughts in text only.

On Wed, Aug 20, 2025 at 10:35:39AM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
> utilizing the safe PWM abstractions from the preceding commit.
> 
> The driver implements the pwm::PwmOps trait using the modern waveform
> API (round_waveform_tohw, write_waveform, etc.) to support configuration
> of period, duty cycle, and polarity for the TH1520's PWM channels.
> 
> Resource management is handled using idiomatic Rust patterns. The PWM
> chip object is allocated via pwm::Chip::new and its registration with
> the PWM core is managed by the pwm::Registration RAII guard. This
> ensures pwmchip_remove is always called when the driver unbinds,
> preventing resource leaks. Device managed resources are used for the
> MMIO region, and the clock lifecycle is correctly managed in the
> driver's private data Drop implementation.
> 
> The driver's core logic is written entirely in safe Rust, with no unsafe
> blocks, except for the Send and Sync implementations for the driver
> data, which are explained in the comments.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  11 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm_th1520.rs | 355 ++++++++++++++++++++++++++++++++++++++++++++++

For consistency I would prefer this file to be named pwm-th1520.rs. I
tried to fix that but then the compiler was unhappy. I guess a minus in
a filename just doesn't work for Rust?

>  4 files changed, 368 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d7c0676c1d00a02b3d7db2de88b039c08c99c6e..d79dc21f22d143ca8cde6a06194545fbc640e695 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21741,6 +21741,7 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>  F:	drivers/pinctrl/pinctrl-th1520.c
>  F:	drivers/pmdomain/thead/
>  F:	drivers/power/sequencing/pwrseq-thead-gpu.c
> +F:	drivers/pwm/pwm_th1520.rs
>  F:	drivers/reset/reset-th1520.c
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>  F:	include/dt-bindings/power/thead,th1520-power.h
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 2b608f4378138775ee3ba4d53f682952e1914118..dd6db01832ee985e2e588a413a13df869a029d3d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -729,6 +729,17 @@ config PWM_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tegra.
>  
> +config PWM_TH1520
> +	tristate "TH1520 PWM support"
> +	depends on RUST
> +	select RUST_PWM_ABSTRACTIONS
> +	help
> +	  This option enables the driver for the PWM controller found on the
> +	  T-HEAD TH1520 SoC.
> +
> +	  To compile this driver as a module, choose M here; the module
> +	  will be called pwm-th1520. If you are unsure, say N.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ff4f47e5fb7a0dbac72c12de82c3773e5582db6d..5c15c95c6e49143969389198657eed0ecf4086b2 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> +obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..5ef887b1b5dfed92c5d4b87a7d48f673352a257e
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Rust T-HEAD TH1520 PWM driver
> +//!
> +//! Limitations:
> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
> +//!   limiting the maximum resolution.
> +//! - The driver supports continuous output mode only; one-shot mode is not
> +//!   implemented.
> +//! - The controller hardware provides up to 6 PWM channels.
> +//! - Reconfiguration is glitch free - new period and duty cycle values are
> +//!   latched and take effect at the start of the next period.
> +//! - Polarity is handled via a simple hardware inversion bit; arbitrary
> +//!   duty cycle offsets are not supported.
> +//! - Disabling a channel is achieved by configuring its duty cycle to zero to
> +//!   produce a static low output. Clearing the `start` does not reliably
> +//!   force the static inactive level defined by the `INACTOUT` bit. Hence
> +//!   this method is not used in this driver.
> +//!
> +
> +use core::ops::Deref;
> +use kernel::{
> +    c_str,
> +    clk::Clk,
> +    device::{Bound, Core, Device},
> +    devres,
> +    io::mem::IoMem,
> +    of, platform,
> +    prelude::*,
> +    pwm, time,
> +};
> +
> +const TH1520_MAX_PWM_NUM: u32 = 6;
> +
> +// Register offsets
> +const fn th1520_pwm_chn_base(n: u32) -> usize {
> +    (n * 0x20) as usize
> +}
> +
> +const fn th1520_pwm_ctrl(n: u32) -> usize {
> +    th1520_pwm_chn_base(n)
> +}
> +
> +const fn th1520_pwm_per(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x08
> +}
> +
> +const fn th1520_pwm_fp(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +// Control register bits
> +const TH1520_PWM_START: u32 = 1 << 0;
> +const TH1520_PWM_CFG_UPDATE: u32 = 1 << 2;
> +const TH1520_PWM_CONTINUOUS_MODE: u32 = 1 << 5;
> +const TH1520_PWM_FPOUT: u32 = 1 << 8;
> +
> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> +
> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> +    (match ns.checked_mul(rate_hz) {
> +        Some(product) => product,
> +        None => u64::MAX,
> +    }) / NSEC_PER_SEC_U64
> +}
> +
> +fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> +
> +    // Round up
> +    let Some(numerator) = cycles
> +        .checked_mul(NSEC_PER_SEC_U64)
> +        .and_then(|p| p.checked_add(rate_hz - 1))
> +    else {
> +        return u64::MAX;
> +    };
> +
> +    numerator / rate_hz

Does cycles_to_ns(18446744074, 66000000) yield u64::MAX or u64::MAX /
66000000? Reading through the docs I found I think it's the former. I
guess we don't reach these spheres in the driver, but conceptually the
latter would be better and I think that's relevant as this is the first
Rust driver and so very likely will be the blueprint for further
drivers.

Further below you're using saturating_sub(), is there a similar variant
for multiplication that could simplify this function to

	let numerator = cyles
	  .saturating_mul(NSEC_PER_SEC_U64)
	  .saturating_add(rate_hz - 1);

	numerator / rate_hz

?

Additionally I'd add a comment that this should be replaced by
mul_u64_u64_div_u64_roundup() once that exists in Rust. (Sigh, I just
notice we still don't have that in C yet.)

> +}
> +
> +/// Hardware-specific waveform representation for TH1520.
> +#[derive(Copy, Clone, Debug, Default)]
> +struct Th1520WfHw {
> +    period_cycles: u32,
> +    duty_cycles: u32,
> +    ctrl_val: u32,
> +    enabled: bool,
> +}
> +
> +/// The driver's private data struct. It holds all necessary devres managed resources.
> +#[pin_data(PinnedDrop)]
> +struct Th1520PwmDriverData {
> +    #[pin]
> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> +    clk: Clk,
> +}
> +
> +// This `unsafe` implementation is a temporary necessity because the underlying `kernel::clk::Clk`
> +// type does not yet expose `Send` and `Sync` implementations. This block should be removed
> +// as soon as the clock abstraction provides these guarantees directly.
> +// TODO: Remove those unsafe impl's when Clk will support them itself.
> +
> +// SAFETY: The `devres` framework requires the driver's private data to be `Send` and `Sync`.
> +// We can guarantee this because the PWM core synchronizes all callbacks, preventing concurrent
> +// access to the contained `iomem` and `clk` resources.
> +unsafe impl Send for Th1520PwmDriverData {}
> +
> +// SAFETY: The same reasoning applies as for `Send`. The PWM core's synchronization
> +// guarantees that it is safe for multiple threads to have shared access (`&self`)
> +// to the driver data during callbacks.
> +unsafe impl Sync for Th1520PwmDriverData {}
> +
> +impl pwm::PwmOps for Th1520PwmDriverData {
> +    type WfHw = Th1520WfHw;
> +
> +    fn round_waveform_tohw(
> +        chip: &pwm::Chip<Self>,
> +        _pwm: &pwm::Device,
> +        wf: &pwm::Waveform,
> +    ) -> Result<pwm::RoundedWaveform<Self::WfHw>> {
> +        let data = chip.drvdata();
> +
> +        if wf.period_length_ns == 0 {

Please add a dev_dbg! here for consistency with the wf.period_length_ns
!= 0 case here.

> +            return Ok(pwm::RoundedWaveform {
> +                status: 0,
> +                hardware_waveform: Th1520WfHw {
> +                    enabled: false,
> +                    ..Default::default()
> +                },
> +            });
> +        }
> +
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u64::from(u32::MAX));

What happens if you get period_cycles = 0 here?

> +        let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u64::from(u32::MAX));
> +
> +        let mut ctrl_val = TH1520_PWM_CONTINUOUS_MODE;
> +
> +        let is_inversed = wf.duty_length_ns > 0
> +            && wf.duty_offset_ns > 0
> +            && wf.duty_length_ns + wf.duty_offset_ns >= wf.period_length_ns;

For
	duty_length_ns =   0x8000000000000000
	duty_offset_ns =   0x8000000000000000
	period_length_ns = 0xffffffffffffffff

this should evaluate to true (right?) but if + overflows in the same way
in Rust as it does in C, you get is_inversed = false here.

The safe condition is

	wf.duty_offset_ns >= wf.period_length_ns - wf.duty_length_ns

here.

> +        if is_inversed {
> +            duty_cycles = period_cycles - duty_cycles;
> +        } else {
> +            ctrl_val |= TH1520_PWM_FPOUT;
> +        }
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles: period_cycles as u32,
> +            duty_cycles: duty_cycles as u32,

This casts period_cycles and duty_cycles from an u64 to an u32, right?
Can you please add a comment that shortly explains why these values fit
into an u32?

> +            ctrl_val,
> +            enabled: true,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "clk_rate: {}Hz Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",

" " before the units please.

I want to establish

	"{}/{} [+{}]", duty_length_ns, period_length_ns, duty_offset_ns

as the typical way to emit a waveform. Please use that consistently.

> +            rate_hz,
> +            wf.period_length_ns,
> +            wf.duty_length_ns,
> +            wf.duty_offset_ns,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val
> +        );
> +
> +        Ok(pwm::RoundedWaveform {
> +            status: 0,
> +            hardware_waveform: wfhw,
> +        })
> +    }
> +
> +    fn round_waveform_fromhw(
> +        chip: &pwm::Chip<Self>,
> +        _pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        wf: &mut pwm::Waveform,
> +    ) -> Result {
> +        let data = chip.drvdata();
> +        let rate_hz = data.clk.rate().as_hz() as u64;
> +
> +        wf.period_length_ns = cycles_to_ns(u64::from(wfhw.period_cycles), rate_hz);

Here I wonder again about wfhw.period_cycles = 0.

> +        let duty_cycles = u64::from(wfhw.duty_cycles);
> +
> +        if (wfhw.ctrl_val & TH1520_PWM_FPOUT) != 0 {
> +            wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
> +            wf.duty_offset_ns = 0;
> +        } else {
> +            let period_cycles = u64::from(wfhw.period_cycles);
> +            let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
> +
> +            // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
> +            wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> +            // The offset is the initial low time, which is what the hardware register provides.
> +            wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
> +        }
> +
> +        Ok(())
> +    }
> +
> +    fn read_waveform(
> +        chip: &pwm::Chip<Self>,
> +        pwm: &pwm::Device,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result<Self::WfHw> {
> +        let data = chip.drvdata();
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_accessor = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_accessor.deref();
> +
> +        let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
> +        let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
> +        let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles,
> +            duty_cycles,
> +            ctrl_val: ctrl,
> +            enabled: duty_cycles != 0,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val,
> +            wfhw.enabled
> +        );
> +
> +        Ok(wfhw)
> +    }
> +
> +    fn write_waveform(
> +        chip: &pwm::Chip<Self>,
> +        pwm: &pwm::Device,
> +        wfhw: &Self::WfHw,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result {
> +        let data = chip.drvdata();
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_accessor = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_accessor.deref();
> +        let was_enabled = pwm.state().enabled();

The driver would be a tad more robust if was_enabled is determined from
the hardware registers instead. (And also it helps not to add another
dependency on pwm.state which I'd like to get rid of in the long run
with waveforms being the canonical representation internally.)

> +
> +        if !wfhw.enabled {

dev_dbg! here for consistence please.

> +            if was_enabled {
> +                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
> +                iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +            }
> +            return Ok(());
> +        }
> +
> +        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
> +        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
> +        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
> +        iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> +
> +        // The `TH1520_PWM_START` bit must be written in a separate, final transaction, and
> +        // only when enabling the channel from a disabled state.
> +        if !was_enabled {
> +            iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_START, th1520_pwm_ctrl(hwpwm))?;
> +        }
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "PWM-{}: Wrote (per: {}, duty: {})",
> +            hwpwm,
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +        );
> +
> +        Ok(())
> +    }
> +}

Best regards
Uwe

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Michal Wilczynski]

On 9/19/25 09:10, Uwe Kleine-König wrote:
> Hello,
> 
> actually I intended to give my feedback as a patch, but I'm not fluent
> enough in Rust yet :-\, so here come my thoughts in text only.

Hi,
Thanks, will do my best to address the comments

> 
> On Wed, Aug 20, 2025 at 10:35:39AM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
>> utilizing the safe PWM abstractions from the preceding commit.
>>
>> The driver implements the pwm::PwmOps trait using the modern waveform
>> API (round_waveform_tohw, write_waveform, etc.) to support configuration
>> of period, duty cycle, and polarity for the TH1520's PWM channels.
>>
>> Resource management is handled using idiomatic Rust patterns. The PWM
>> chip object is allocated via pwm::Chip::new and its registration with
>> the PWM core is managed by the pwm::Registration RAII guard. This
>> ensures pwmchip_remove is always called when the driver unbinds,
>> preventing resource leaks. Device managed resources are used for the
>> MMIO region, and the clock lifecycle is correctly managed in the
>> driver's private data Drop implementation.
>>
>> The driver's core logic is written entirely in safe Rust, with no unsafe
>> blocks, except for the Send and Sync implementations for the driver
>> data, which are explained in the comments.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  MAINTAINERS               |   1 +
>>  drivers/pwm/Kconfig       |  11 ++
>>  drivers/pwm/Makefile      |   1 +
>>  drivers/pwm/pwm_th1520.rs | 355 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> For consistency I would prefer this file to be named pwm-th1520.rs. I
> tried to fix that but then the compiler was unhappy. I guess a minus in
> a filename just doesn't work for Rust?

In Rust by default the Rust module is called the same as the file. And
the module names can't contain hyphens. I think technically this could
be overridden by 'path' attribute, but it's probably just better to use
underscores '_' like other Rust drivers.

> 
>>  4 files changed, 368 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5d7c0676c1d00a02b3d7db2de88b039c08c99c6e..d79dc21f22d143ca8cde6a06194545fbc640e695 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21741,6 +21741,7 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>>  F:	drivers/pinctrl/pinctrl-th1520.c
>>  F:	drivers/pmdomain/thead/
>>  F:	drivers/power/sequencing/pwrseq-thead-gpu.c
>> +F:	drivers/pwm/pwm_th1520.rs
>>  F:	drivers/reset/reset-th1520.c
>>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>>  F:	include/dt-bindings/power/thead,th1520-power.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 2b608f4378138775ee3ba4d53f682952e1914118..dd6db01832ee985e2e588a413a13df869a029d3d 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -729,6 +729,17 @@ config PWM_TEGRA
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-tegra.
>>  
>> +config PWM_TH1520
>> +	tristate "TH1520 PWM support"
>> +	depends on RUST
>> +	select RUST_PWM_ABSTRACTIONS
>> +	help
>> +	  This option enables the driver for the PWM controller found on the
>> +	  T-HEAD TH1520 SoC.
>> +
>> +	  To compile this driver as a module, choose M here; the module
>> +	  will be called pwm-th1520. If you are unsure, say N.
>> +
>>  config PWM_TIECAP
>>  	tristate "ECAP PWM support"
>>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index ff4f47e5fb7a0dbac72c12de82c3773e5582db6d..5c15c95c6e49143969389198657eed0ecf4086b2 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>> +obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
>>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..5ef887b1b5dfed92c5d4b87a7d48f673352a257e
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,355 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Rust T-HEAD TH1520 PWM driver
>> +//!
>> +//! Limitations:
>> +//! - The period and duty cycle are controlled by 32-bit hardware registers,
>> +//!   limiting the maximum resolution.
>> +//! - The driver supports continuous output mode only; one-shot mode is not
>> +//!   implemented.
>> +//! - The controller hardware provides up to 6 PWM channels.
>> +//! - Reconfiguration is glitch free - new period and duty cycle values are
>> +//!   latched and take effect at the start of the next period.
>> +//! - Polarity is handled via a simple hardware inversion bit; arbitrary
>> +//!   duty cycle offsets are not supported.
>> +//! - Disabling a channel is achieved by configuring its duty cycle to zero to
>> +//!   produce a static low output. Clearing the `start` does not reliably
>> +//!   force the static inactive level defined by the `INACTOUT` bit. Hence
>> +//!   this method is not used in this driver.
>> +//!
>> +
>> +use core::ops::Deref;
>> +use kernel::{
>> +    c_str,
>> +    clk::Clk,
>> +    device::{Bound, Core, Device},
>> +    devres,
>> +    io::mem::IoMem,
>> +    of, platform,
>> +    prelude::*,
>> +    pwm, time,
>> +};
>> +
>> +const TH1520_MAX_PWM_NUM: u32 = 6;
>> +
>> +// Register offsets
>> +const fn th1520_pwm_chn_base(n: u32) -> usize {
>> +    (n * 0x20) as usize
>> +}
>> +
>> +const fn th1520_pwm_ctrl(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n)
>> +}
>> +
>> +const fn th1520_pwm_per(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n) + 0x08
>> +}
>> +
>> +const fn th1520_pwm_fp(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +// Control register bits
>> +const TH1520_PWM_START: u32 = 1 << 0;
>> +const TH1520_PWM_CFG_UPDATE: u32 = 1 << 2;
>> +const TH1520_PWM_CONTINUOUS_MODE: u32 = 1 << 5;
>> +const TH1520_PWM_FPOUT: u32 = 1 << 8;
>> +
>> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
>> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
>> +
>> +    (match ns.checked_mul(rate_hz) {
>> +        Some(product) => product,
>> +        None => u64::MAX,
>> +    }) / NSEC_PER_SEC_U64
>> +}
>> +
>> +fn cycles_to_ns(cycles: u64, rate_hz: u64) -> u64 {
>> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
>> +
>> +    // Round up
>> +    let Some(numerator) = cycles
>> +        .checked_mul(NSEC_PER_SEC_U64)
>> +        .and_then(|p| p.checked_add(rate_hz - 1))
>> +    else {
>> +        return u64::MAX;
>> +    };
>> +
>> +    numerator / rate_hz
> 
> Does cycles_to_ns(18446744074, 66000000) yield u64::MAX or u64::MAX /
> 66000000? Reading through the docs I found I think it's the former. I
> guess we don't reach these spheres in the driver, but conceptually the
> latter would be better and I think that's relevant as this is the first
> Rust driver and so very likely will be the blueprint for further
> drivers.
> 
> Further below you're using saturating_sub(), is there a similar variant
> for multiplication that could simplify this function to
> 
> 	let numerator = cyles
> 	  .saturating_mul(NSEC_PER_SEC_U64)
> 	  .saturating_add(rate_hz - 1);
> 
> 	numerator / rate_hz
> 
> ?

Yeah I think this would work

> 
> Additionally I'd add a comment that this should be replaced by
> mul_u64_u64_div_u64_roundup() once that exists in Rust. (Sigh, I just
> notice we still don't have that in C yet.)

Sure

> 
>> +}
>> +
>> +/// Hardware-specific waveform representation for TH1520.
>> +#[derive(Copy, Clone, Debug, Default)]
>> +struct Th1520WfHw {
>> +    period_cycles: u32,
>> +    duty_cycles: u32,
>> +    ctrl_val: u32,
>> +    enabled: bool,
>> +}
>> +
>> +/// The driver's private data struct. It holds all necessary devres managed resources.
>> +#[pin_data(PinnedDrop)]
>> +struct Th1520PwmDriverData {
>> +    #[pin]
>> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
>> +    clk: Clk,
>> +}
>> +
>> +// This `unsafe` implementation is a temporary necessity because the underlying `kernel::clk::Clk`
>> +// type does not yet expose `Send` and `Sync` implementations. This block should be removed
>> +// as soon as the clock abstraction provides these guarantees directly.
>> +// TODO: Remove those unsafe impl's when Clk will support them itself.
>> +
>> +// SAFETY: The `devres` framework requires the driver's private data to be `Send` and `Sync`.
>> +// We can guarantee this because the PWM core synchronizes all callbacks, preventing concurrent
>> +// access to the contained `iomem` and `clk` resources.
>> +unsafe impl Send for Th1520PwmDriverData {}
>> +
>> +// SAFETY: The same reasoning applies as for `Send`. The PWM core's synchronization
>> +// guarantees that it is safe for multiple threads to have shared access (`&self`)
>> +// to the driver data during callbacks.
>> +unsafe impl Sync for Th1520PwmDriverData {}
>> +
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> +    type WfHw = Th1520WfHw;
>> +
>> +    fn round_waveform_tohw(
>> +        chip: &pwm::Chip<Self>,
>> +        _pwm: &pwm::Device,
>> +        wf: &pwm::Waveform,
>> +    ) -> Result<pwm::RoundedWaveform<Self::WfHw>> {
>> +        let data = chip.drvdata();
>> +
>> +        if wf.period_length_ns == 0 {
> 
> Please add a dev_dbg! here for consistency with the wf.period_length_ns
> != 0 case here.
> 
>> +            return Ok(pwm::RoundedWaveform {
>> +                status: 0,
>> +                hardware_waveform: Th1520WfHw {
>> +                    enabled: false,
>> +                    ..Default::default()
>> +                },
>> +            });
>> +        }
>> +
>> +        let rate_hz = data.clk.rate().as_hz() as u64;
>> +
>> +        let period_cycles = ns_to_cycles(wf.period_length_ns, rate_hz).min(u64::from(u32::MAX));
> 
> What happens if you get period_cycles = 0 here?

How about add an if for early return ? Like below:
	if period_cycles == 0 {
            dev_dbg!(
                chip.device(),
                "Requested period {} ns is too small for clock rate {} Hz, disabling PWM.\n",
                wf.period_length_ns,
                rate_hz
            );

            return Ok(pwm::RoundedWaveform {
                status: 0,
                hardware_waveform: Th1520WfHw {
                    enabled: false,
                    ..Default::default()
                },
            });
        }


> 
>> +        let mut duty_cycles = ns_to_cycles(wf.duty_length_ns, rate_hz).min(u64::from(u32::MAX));
>> +
>> +        let mut ctrl_val = TH1520_PWM_CONTINUOUS_MODE;
>> +
>> +        let is_inversed = wf.duty_length_ns > 0
>> +            && wf.duty_offset_ns > 0
>> +            && wf.duty_length_ns + wf.duty_offset_ns >= wf.period_length_ns;
> 
> For
> 	duty_length_ns =   0x8000000000000000
> 	duty_offset_ns =   0x8000000000000000
> 	period_length_ns = 0xffffffffffffffff
> 
> this should evaluate to true (right?) but if + overflows in the same way
> in Rust as it does in C, you get is_inversed = false here.
> 
> The safe condition is
> 
> 	wf.duty_offset_ns >= wf.period_length_ns - wf.duty_length_ns
> 
> here.

OK will change it thanks.

> 
>> +        if is_inversed {
>> +            duty_cycles = period_cycles - duty_cycles;
>> +        } else {
>> +            ctrl_val |= TH1520_PWM_FPOUT;
>> +        }
>> +
>> +        let wfhw = Th1520WfHw {
>> +            period_cycles: period_cycles as u32,
>> +            duty_cycles: duty_cycles as u32,
> 
> This casts period_cycles and duty_cycles from an u64 to an u32, right?
> Can you please add a comment that shortly explains why these values fit
> into an u32?

Will add a comment.

> 
>> +            ctrl_val,
>> +            enabled: true,
>> +        };
>> +
>> +        dev_dbg!(
>> +            chip.device(),
>> +            "clk_rate: {}Hz Requested: period {}ns, duty {}ns, offset {}ns -> HW: period {} cyc, duty {} cyc, ctrl 0x{:x}\n",
> 
> " " before the units please.
> 
> I want to establish
> 
> 	"{}/{} [+{}]", duty_length_ns, period_length_ns, duty_offset_ns
> 
> as the typical way to emit a waveform. Please use that consistently.

Sure, thanks

> 
>> +            rate_hz,
>> +            wf.period_length_ns,
>> +            wf.duty_length_ns,
>> +            wf.duty_offset_ns,
>> +            wfhw.period_cycles,
>> +            wfhw.duty_cycles,
>> +            wfhw.ctrl_val
>> +        );
>> +
>> +        Ok(pwm::RoundedWaveform {
>> +            status: 0,
>> +            hardware_waveform: wfhw,
>> +        })
>> +    }
>> +
>> +    fn round_waveform_fromhw(
>> +        chip: &pwm::Chip<Self>,
>> +        _pwm: &pwm::Device,
>> +        wfhw: &Self::WfHw,
>> +        wf: &mut pwm::Waveform,
>> +    ) -> Result {
>> +        let data = chip.drvdata();
>> +        let rate_hz = data.clk.rate().as_hz() as u64;
>> +
>> +        wf.period_length_ns = cycles_to_ns(u64::from(wfhw.period_cycles), rate_hz);
> 
> Here I wonder again about wfhw.period_cycles = 0.
> 
>> +        let duty_cycles = u64::from(wfhw.duty_cycles);
>> +
>> +        if (wfhw.ctrl_val & TH1520_PWM_FPOUT) != 0 {
>> +            wf.duty_length_ns = cycles_to_ns(duty_cycles, rate_hz);
>> +            wf.duty_offset_ns = 0;
>> +        } else {
>> +            let period_cycles = u64::from(wfhw.period_cycles);
>> +            let original_duty_cycles = period_cycles.saturating_sub(duty_cycles);
>> +
>> +            // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
>> +            wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
>> +            // The offset is the initial low time, which is what the hardware register provides.
>> +            wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>> +    fn read_waveform(
>> +        chip: &pwm::Chip<Self>,
>> +        pwm: &pwm::Device,
>> +        parent_dev: &Device<Bound>,
>> +    ) -> Result<Self::WfHw> {
>> +        let data = chip.drvdata();
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_accessor = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_accessor.deref();
>> +
>> +        let ctrl = iomap.try_read32(th1520_pwm_ctrl(hwpwm))?;
>> +        let period_cycles = iomap.try_read32(th1520_pwm_per(hwpwm))?;
>> +        let duty_cycles = iomap.try_read32(th1520_pwm_fp(hwpwm))?;
>> +
>> +        let wfhw = Th1520WfHw {
>> +            period_cycles,
>> +            duty_cycles,
>> +            ctrl_val: ctrl,
>> +            enabled: duty_cycles != 0,
>> +        };
>> +
>> +        dev_dbg!(
>> +            chip.device(),
>> +            "PWM-{}: read_waveform: Read hw state - period: {}, duty: {}, ctrl: 0x{:x}, enabled: {}",
>> +            hwpwm,
>> +            wfhw.period_cycles,
>> +            wfhw.duty_cycles,
>> +            wfhw.ctrl_val,
>> +            wfhw.enabled
>> +        );
>> +
>> +        Ok(wfhw)
>> +    }
>> +
>> +    fn write_waveform(
>> +        chip: &pwm::Chip<Self>,
>> +        pwm: &pwm::Device,
>> +        wfhw: &Self::WfHw,
>> +        parent_dev: &Device<Bound>,
>> +    ) -> Result {
>> +        let data = chip.drvdata();
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_accessor = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_accessor.deref();
>> +        let was_enabled = pwm.state().enabled();
> 
> The driver would be a tad more robust if was_enabled is determined from
> the hardware registers instead. (And also it helps not to add another
> dependency on pwm.state which I'd like to get rid of in the long run
> with waveforms being the canonical representation internally.)

I checked and it seems to me like I can make it work with HW registers,
will include that in next revision.

> 
>> +
>> +        if !wfhw.enabled {
> 
> dev_dbg! here for consistence please.
> 
>> +            if was_enabled {
>> +                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
>> +                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
>> +                iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
>> +            }
>> +            return Ok(());
>> +        }
>> +
>> +        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
>> +        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
>> +        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
>> +        iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
>> +
>> +        // The `TH1520_PWM_START` bit must be written in a separate, final transaction, and
>> +        // only when enabling the channel from a disabled state.
>> +        if !was_enabled {
>> +            iomap.try_write32(wfhw.ctrl_val | TH1520_PWM_START, th1520_pwm_ctrl(hwpwm))?;
>> +        }
>> +
>> +        dev_dbg!(
>> +            chip.device(),
>> +            "PWM-{}: Wrote (per: {}, duty: {})",
>> +            hwpwm,
>> +            wfhw.period_cycles,
>> +            wfhw.duty_cycles,
>> +        );
>> +
>> +        Ok(())
>> +    }
>> +}
> 
> Best regards
> Uwe

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 5/7] 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.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Tested-by: Drew Fustini <fustini@kernel.org>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../devicetree/bindings/pwm/thead,th1520-pwm.yaml  | 48 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 49 insertions(+)

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

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 5/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:40AM +0200, Michal Wilczynski wrote:
> Add the Device Tree binding documentation for the T-HEAD
> TH1520 SoC PWM controller.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Tested-by: Drew Fustini <fustini@kernel.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../devicetree/bindings/pwm/thead,th1520-pwm.yaml  | 48 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 49 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..855aec59ac53c430adc849271235686e87b10e6c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/thead,th1520-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: T-HEAD TH1520 PWM controller
> +
> +maintainers:
> +  - Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: thead,th1520-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: SoC PWM clock
> +
> +  "#pwm-cells":
> +    const: 3
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/thead,th1520-clk-ap.h>
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      pwm@ffec01c000 {
> +          compatible = "thead,th1520-pwm";
> +          reg = <0xff 0xec01c000 0x0 0x4000>;
> +          clocks = <&clk CLK_PWM>;
> +          #pwm-cells = <3>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d79dc21f22d143ca8cde6a06194545fbc640e695..a64027f441e8e23c579b469b2451b514e5d2802c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21732,6 +21732,7 @@ F:	Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>  F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>  F:	Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
> +F:	Documentation/devicetree/bindings/pwm/thead,th1520-pwm.yaml
>  F:	Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
>  F:	arch/riscv/boot/dts/thead/
>  F:	drivers/clk/thead/clk-th1520-ap.c
> 
> -- 
> 2.34.1

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 6/7] riscv: dts: thead: Add PWM controller node

[Michal Wilczynski]

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

Reviewed-by: Drew Fustini <fustini@kernel.org>
Tested-by: Drew Fustini <fustini@kernel.org>
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 42724bf7e90e08fac326c464d0f080e3bd2cd59b..513dc6977b2633503515ad260913156fbe57d92f 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -493,6 +493,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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 6/7] riscv: dts: thead: Add PWM controller node

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:41AM +0200, Michal Wilczynski wrote:
> Add the Device Tree node for the T-HEAD TH1520 SoC's PWM controller.
> 
> Reviewed-by: Drew Fustini <fustini@kernel.org>
> Tested-by: Drew Fustini <fustini@kernel.org>
> 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 42724bf7e90e08fac326c464d0f080e3bd2cd59b..513dc6977b2633503515ad260913156fbe57d92f 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -493,6 +493,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

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v14 7/7] 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.

Reviewed-by: Drew Fustini <fustini@kernel.org>
Tested-by: Drew Fustini <fustini@kernel.org>
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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 7/7] riscv: dts: thead: Add PWM fan and thermal control

[Elle Rhumsaa]

On Wed, Aug 20, 2025 at 10:35:42AM +0200, Michal Wilczynski wrote:
> 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.
> 
> Reviewed-by: Drew Fustini <fustini@kernel.org>
> Tested-by: Drew Fustini <fustini@kernel.org>
> 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

Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 7/7] riscv: dts: thead: Add PWM fan and thermal control

[Drew Fustini]

On Wed, Aug 20, 2025 at 10:35:42AM +0200, Michal Wilczynski wrote:
> 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.
> 
> Reviewed-by: Drew Fustini <fustini@kernel.org>
> Tested-by: Drew Fustini <fustini@kernel.org>
> 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(+)

Do you think it makes sense to coordinate this with the "Initial thermal
management for Lichee Pi 4A board" series [1] from Icenowy?

Thanks,
Drew

[1] https://lore.kernel.org/linux-riscv/20250816093209.2600355-1-uwu@icenowy.me/

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 7/7] riscv: dts: thead: Add PWM fan and thermal control

[Michal Wilczynski]

On 8/22/25 22:49, Drew Fustini wrote:
> On Wed, Aug 20, 2025 at 10:35:42AM +0200, Michal Wilczynski wrote:
>> 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.
>>
>> Reviewed-by: Drew Fustini <fustini@kernel.org>
>> Tested-by: Drew Fustini <fustini@kernel.org>
>> 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(+)
> 
> Do you think it makes sense to coordinate this with the "Initial thermal
> management for Lichee Pi 4A board" series [1] from Icenowy?

Hi,
It seems to me like using the HW PWM is just a better solution for
driving the fans. However as pointed in the discussion currently Rust
requires LLVM, so people compiling with gcc will not be able to compile
the driver for some time until the gcc support becomes better.

I think there is a way to express in Device Tree the fallback logic -
if Rust not available then use SW PWM. So I guess the pragmatic way to
go is to merge SW PWM temporary fix first, and later when gcc becomes
better at compiling Rust patch it up to use HW PWM instead.

> 
> Thanks,
> Drew
> 
> [1] https://lore.kernel.org/linux-riscv/20250816093209.2600355-1-uwu@icenowy.me/
> 

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 7/7] riscv: dts: thead: Add PWM fan and thermal control

[Michal Wilczynski]

On 8/23/25 12:13, Michal Wilczynski wrote:
> 
> 
> On 8/22/25 22:49, Drew Fustini wrote:
>> On Wed, Aug 20, 2025 at 10:35:42AM +0200, Michal Wilczynski wrote:
>>> 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.
>>>
>>> Reviewed-by: Drew Fustini <fustini@kernel.org>
>>> Tested-by: Drew Fustini <fustini@kernel.org>
>>> 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(+)
>>
>> Do you think it makes sense to coordinate this with the "Initial thermal
>> management for Lichee Pi 4A board" series [1] from Icenowy?
> 
> Hi,
> It seems to me like using the HW PWM is just a better solution for
> driving the fans. However as pointed in the discussion currently Rust
> requires LLVM, so people compiling with gcc will not be able to compile
> the driver for some time until the gcc support becomes better.
> 
> I think there is a way to express in Device Tree the fallback logic -

Sorry I meant 'there is NO way to express this in Device tree'

> if Rust not available then use SW PWM. So I guess the pragmatic way to
> go is to merge SW PWM temporary fix first, and later when gcc becomes
> better at compiling Rust patch it up to use HW PWM instead.
> 
>>
>> Thanks,
>> Drew
>>
>> [1] https://lore.kernel.org/linux-riscv/20250816093209.2600355-1-uwu@icenowy.me/
>>
> 
> Best regards,

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 7/7] riscv: dts: thead: Add PWM fan and thermal control

[Miguel Ojeda]

On Sat, Aug 23, 2025 at 12:14 PM Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> However as pointed in the discussion currently Rust
> requires LLVM, so people compiling with gcc will not be able to compile
> the driver for some time until the gcc support becomes better.

In case it helps/clarifies: the C side can be built with GCC (i.e.
building without `LLVM=1`, with Rust using its LLVM backend), but it
is a best-effort hack and the goal is to eventually use
`rustc_codegen_gcc` and/or GCC Rust. Some users/distributions use it
nevertheless.

Cheers,
Miguel

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v14 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver

[Michal Wilczynski]

On 8/20/25 10:35, Michal Wilczynski wrote:
> This patch series introduces Rust support for the T-HEAD TH1520 PWM
> controller and demonstrates its use for fan control on the Sipeed Lichee
> Pi 4A board.
> 
> The primary goal of this patch series is to introduce a basic set of
> Rust abstractions for the Linux PWM subsystem. As a first user and
> practical demonstration of these abstractions, the series also provides
> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
> of its PWM channels and ultimately enables temperature controlled fan
> support for the Lichee Pi 4A board. This work aims to explore the use of
> Rust for PWM drivers and lay a foundation for potential future Rust
> based PWM drivers.
> 
> The core of this series is a new rust/kernel/pwm.rs module that provides
> abstractions for writing PWM chip provider drivers in Rust. This has
> been significantly reworked from v1 based on extensive feedback. The key
> features of the new abstraction layer include:
> 
>  - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
>    by ARef, correctly tying its lifetime to its embedded struct device
>    reference counter. Chip registration is handled by a pwm::Registration
>    RAII guard, which guarantees that pwmchip_add is always paired with
>    pwmchip_remove, preventing resource leaks.
> 
>  - Modern and Safe API: The PwmOps trait is now based on the modern
>    waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
>    by the subsystem maintainer. It is generic over a driver's
>    hardware specific data structure, moving all unsafe serialization logic
>    into the abstraction layer and allowing drivers to be written in 100%
>    safe Rust.
> 
>  - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
>    types (State, Args, Device, etc.) and uses standard kernel error
>    handling patterns.
> 
> The series is structured as follows:
>  - Expose static function pwmchip_release.
>  - Rust PWM Abstractions: The new safe abstraction layer.
>  - TH1520 PWM Driver: A new Rust driver for the TH1520 SoC, built on
>    top of the new abstractions.
>  - Device Tree Bindings & Nodes: The remaining patches add the necessary
>    DT bindings and nodes for the TH1520 PWM controller, and the PWM fan
>    configuration for the Lichee Pi 4A board.
> 
> Testing:
> Tested on the TH1520 SoC. The fan works correctly. The duty/period
> calculations are correct. Fan starts slow when the chip is not hot and
> gradually increases the speed when PVT reports higher temperatures.
> 
> The patches doesn't contain any dependencies that are not currently in
> the mainline kernel anymore.
> 
> ---
> Changes in v14:
> - Re-base on top of 6.17-rc1.
> - Cosmetic change in label function.
> - Link to v13: https://lore.kernel.org/r/20250806-rust-next-pwm-working-fan-for-sending-v13-0-690b669295b6@samsung.com
> 
> Changes in v13:
> - Re-add the T-HEAD TH1520 PWM driver and its device tree bindings, as
>   Iomem series got merged into mainline kernel.
> - Fix Args struct to be consistent with State - no Opaque needed for
>   copies.
> - Replace tuple retur type in the PwmOps trait with dedicated struct
>   for improved clarity.
> - Use build_assert for WfHw size, as it doesn't have to be runtime
>   check.
> - Various cosmetic changes.
> - Link to v12: https://lore.kernel.org/r/20250717-rust-next-pwm-working-fan-for-sending-v12-0-40f73defae0c@samsung.com
> 
> Changes in v12:
>  - Reworked the PWM abstractions to use the subclassing pattern as
>    suggested by reviewers.
>  - pwm::Chip and its driver data are now allocated in a single, contiguous
>    memory block via pwmchip_alloc() sizeof_priv argument.
>  - Chip::new() now uses the pin init API to construct the driver data
>    in place, removing the need for a separate allocation.
>  - The  PwmOps trait is now implemented directly by the driver data struct
>    itself, removing the DrvData associated type and the ForeignOwnable
>    trait.
>  - The custom release handler has been updated to call drop_in_place on the driver
>    data, ensuring destructors are run correctly before the underlying
>    memory is freed.
>  - Moved the pwmchip_release prototype in the C header to a separate
>    section to clarify it is for FFI use only, as requested.
>  - Added a Prerequisite-patch-id trailer to the cover letter to declare
>    the dependency on the PWM_WFHWSIZE patch.
> 
> - Link to v11: https://lore.kernel.org/r/20250710-rust-next-pwm-working-fan-for-sending-v11-0-93824a16f9ec@samsung.com
> 
> Changes in v11:
> - Dropped driver and DT commits, as they don't compile based on publicly
>   known commit.
> - Re-based on top of pwm/for-next.
> - Reverted back to devres::Devres::new_foreign_owned, as pwm/for-next
>   doesn't contain 'register' re-factor, which is present in linux-next,
>   queued for the next merge window. The conflict is trivial, simply
>   change 'new_foreign_owned' -> 'register'.
> - Added list to MAINTAINERS entry as requested.
> - Link to v10: https://lore.kernel.org/r/20250707-rust-next-pwm-working-fan-for-sending-v10-0-d0c5cf342004@samsung.com
> 
> Changes in v10:
>  - Exported the C pwmchip_release function and called it from the custom
>    Rust release_callback to fix a memory leak of the pwm_chip struct.
>  - Removed the PwmOps::free callback, as it is not needed for idiomatic
>    Rust resource management.
>  - Removed the redundant is_null check for drvdata in the release handler,
>    as the Rust API guarantees a valid pointer is always provided.
> 
> - Link to v9: https://lore.kernel.org/r/20250706-rust-next-pwm-working-fan-for-sending-v9-0-42b5ac2101c7@samsung.com
> 
> Changes in v9:
>  - Encapsulated vtable setup in Chip::new(): The Chip::new() function is
>    now generic over the PwmOps implementation. This allows it to create and
>    assign the vtable internally, which simplifies the public API by
>    removing the ops_vtable parameter from Registration::register().
>  - Fixed memory leak with a release handler: A custom release_callback is
>    now assigned to the embedded struct device's release hook. This
>    guarantees that driver specific data is always freed when the chip is
>    destroyed, even if registration fails.
>  - The PwmOpsVTable is now defined as a const associated item to ensure
>    it has a 'static lifetime.
>  - Combined introductory commits: The Device, Chip, and PwmOps abstractions
>    are now introduced in a single commit. This was necessary to resolve the
>    circular dependencies between them and present a clean, compilable unit
>    for review.
> 
> - Link to v8: https://lore.kernel.org/r/20250704-rust-next-pwm-working-fan-for-sending-v8-0-951e5482c9fd@samsung.com
> 
> Changes in v8:
>  - Dropped already accepted commit, re-based on top of linux-next
>  - Reworked the Chip and PwmOps APIs to address the drvdata() type-safety
>    comment. Chip is now generic, and PwmOps uses an associated type
>    to provide compile-time guarantees.
>  - Added a parent device sanity check to Registration::register().
>  - Updated drvdata() to return the idiomatic T::Borrowed<'_>.
>  - added temporary unsafe blocks in the driver, as the current
>    abstraction for Clk is neiter Safe nor Sync. I think eventually
>    proper abstraction for Clk will be added as in a current state it's
>    not very useful.
> 
> - Link to v7: https://lore.kernel.org/r/20250702-rust-next-pwm-working-fan-for-sending-v7-0-67ef39ff1d29@samsung.com
> 
> Changes in v7:
> - Made parent_device function private and moved casts to Device<Bound>
>   there as well.
> - Link to v6: https://lore.kernel.org/r/20250701-rust-next-pwm-working-fan-for-sending-v6-0-2710932f6f6b@samsung.com
> 
> Changes in v6:
>  - Re-based on top of linux-next, dropped two already accepted commits.
>  - After re-basing the IoMem dependent patchset stopped working,
>    reworked it to use similar API like the PCI subsystem (I think it
>    will end up the same). Re-worked the driver for it as well.
>  - Remove the apply and get_state callbacks, and most of the State as
>    well, as the old way of implementing drivers should not be possible
>    in Rust. Left only enabled(), since it's useful for my driver.
>  - Removed the public set_drvdata() method from pwm::Chip
>  - Moved WFHWSIZE to the public include/linux/pwm.h header and renamed it
>    to PWM_WFHWSIZE, allowing bindgen to create safe FFI bindings.
>  - Corrected the ns_to_cycles integer calculation in the TH1520 driver to
>    handle overflow correctly.
>  - Updated the Kconfig entry for the TH1520 driver to select the Rust
>    abstractions for a better user experience.
> 
> - Link to v5: https://lore.kernel.org/r/20250623-rust-next-pwm-working-fan-for-sending-v5-0-0ca23747c23e@samsung.com
> 
> Changes in v5:
> - Reworked `pwm::Chip` creation to take driver data directly, which
>   allowed making the `chip.drvdata()` accessor infallible
> - added missing `pwm.c` file lost during the commit split (sorry !)
> - Link to v4: https://lore.kernel.org/r/20250618-rust-next-pwm-working-fan-for-sending-v4-0-a6a28f2b6d8a@samsung.com
> 
> Changes in v4:
>  - Reworked the pwm::Registration API to use the devres framework,
>    addressing lifetime issue.
>  - Corrected the PwmOps trait and its callbacks to use immutable references
>    (&Chip, &Device) for improved safety.
>  - Applied various code style and naming cleanups based on feedback
> 
> - Link to v3: https://lore.kernel.org/r/20250617-rust-next-pwm-working-fan-for-sending-v3-0-1cca847c6f9f@samsung.com
> 
> Changes in v3:
>  - Addressed feedback from Uwe by making multiple changes to the TH1520
>    driver and the abstraction layer.
>  - Split the core PWM abstractions into three focused commits to ease
>    review per Benno request.
>  - Confirmed the driver now works correctly with CONFIG_PWM_DEBUG enabled
>    by implementing the full waveform API, which correctly reads the
>    hardware state.
>  - Refactored the Rust code to build cleanly with
>    CONFIG_RUST_BUILD_ASSERT_ALLOW=n, primarily by using the try_* family of
>    functions for IoMem access.
>  - Included several cosmetic changes and cleanups to the abstractions
>    per Miguel review.
> 
> - Link to v2: https://lore.kernel.org/r/20250610-rust-next-pwm-working-fan-for-sending-v2-0-753e2955f110@samsung.com
> 
> Changes in v2:
>  - Reworked the PWM abstraction layer based on extensive feedback.
>  - Replaced initial devm allocation with a proper ARef<Chip> lifetime model
>    using AlwaysRefCounted.
>  - Implemented a Registration RAII guard to ensure safe chip add/remove.
>  - Migrated the PwmOps trait from the legacy .apply callback to the modern
>    waveform API.
>  - Refactored the TH1520 driver to use the new, safer abstractions.
>  - Added a patch to mark essential bus clocks as CLK_IGNORE_UNUSED to fix
>    boot hangs when the PWM and thermal sensors are enabled.
> - Link to v1: https://lore.kernel.org/r/20250524-rust-next-pwm-working-fan-for-sending-v1-0-bdd2d5094ff7@samsung.com
> 
> ---
> Michal Wilczynski (7):
>       pwm: Export `pwmchip_release` for external use
>       rust: pwm: Add Kconfig and basic data structures
>       rust: pwm: Add complete abstraction layer
>       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 PWM fan and thermal control
> 
>  .../devicetree/bindings/pwm/thead,th1520-pwm.yaml  |  48 ++
>  MAINTAINERS                                        |  10 +
>  arch/riscv/boot/dts/thead/th1520-lichee-pi-4a.dts  |  67 ++
>  arch/riscv/boot/dts/thead/th1520.dtsi              |   7 +
>  drivers/pwm/Kconfig                                |  24 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/core.c                                 |   3 +-
>  drivers/pwm/pwm_th1520.rs                          | 355 +++++++++
>  include/linux/pwm.h                                |   6 +
>  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                                 | 790 +++++++++++++++++++++
>  14 files changed, 1334 insertions(+), 1 deletion(-)
> ---
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> change-id: 20250524-rust-next-pwm-working-fan-for-sending-552ad2d1b193
> 
> Best regards,


Hi Uwe
Pinging in case this thread got lost in your mailbox.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv