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 are based on mainline, with some dependencies which are not
merged yet - platform Io support [1].

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

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

---
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                                        |   8 +
 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                                 |   2 +-
 drivers/pwm/pwm_th1520.rs                          | 352 +++++++++
 include/linux/pwm.h                                |   5 +
 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                                 | 800 +++++++++++++++++++++
 14 files changed, 1337 insertions(+), 1 deletion(-)
---
base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
change-id: 20250524-rust-next-pwm-working-fan-for-sending-552ad2d1b193

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

On 7/7/25 11:48, 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 are based on mainline, with some dependencies which are not
> merged yet - platform Io support [1].
> 
> Reference repository with all the patches together can be found on
> github [2].
> 
> [1] - https://lore.kernel.org/rust-for-linux/20250509-topics-tyr-platform_iomem-v8-0-e9f1725a40da@collabora.com/
> [2] - https://github.com/mwilczy/linux/commits/rust-next-pwm-working-fan-for-sending-v15/
> 
> ---
> 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                                        |   8 +
>  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                                 |   2 +-
>  drivers/pwm/pwm_th1520.rs                          | 352 +++++++++
>  include/linux/pwm.h                                |   5 +
>  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                                 | 800 +++++++++++++++++++++
>  14 files changed, 1337 insertions(+), 1 deletion(-)
> ---
> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
> change-id: 20250524-rust-next-pwm-working-fan-for-sending-552ad2d1b193
> 
> Best regards,

Hi Uwe, Danilo,

Many thanks for your reviews and your patience.

I was hoping you could clarify the intended merge path for this series,
as it introduces changes to both the Rust and PWM subsystems.

Is the expectation that the Rust maintainers will take the abstraction
patches into the Rust tree first? Or would Uwe, as the PWM maintainer,
pull the entire series? Any guidance on the coordination would be very
helpful.                                         

I understand that it may be too late in the development cycle to merge
the full series. If that's the case, perhaps patch 2 could be considered
on its own, as it hasn't received comments in the last couple of
revisions. As another possibility, patch 1 and patch 3 are dependent on
each other and could be applied as a pair, depending on your assessment.

The RISC-V driver itself would need to wait for the IoMem series merge [1].

[1] - https://lore.kernel.org/rust-for-linux/20250704-topics-tyr-platform_iomem-v12-0-1d3d4bd8207d@collabora.com/

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

Hello,

On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
> On 7/7/25 11:48, 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.

Is this really necessary? I didn't try to understand the requirements
yet, but I wonder about that. If you get the pwmchip from
__pwmchip_add() the right thing to do to release it is to call
pwmchip_remove(). Feels like a layer violation.

> [...]
> > ---
> > base-commit: 47753b5a1696283930a78aae79b29371f96f5bca

I have problems applying this series and don't have this base commit in
my repo.

Best regards
Uwe

On 7/10/25 15:10, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>> On 7/7/25 11:48, 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.
> 
> Is this really necessary? I didn't try to understand the requirements
> yet, but I wonder about that. If you get the pwmchip from
> __pwmchip_add() the right thing to do to release it is to call
> pwmchip_remove(). Feels like a layer violation.

Hi Uwe,
Thank you for the feedback.

It's required to prevent a memory leak in a specific, critical failure
scenario. The sequence of events is as follows:

    pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
    the Rust drvdata.

    pwm::Registration::register() (which calls pwmchip_add()) fails for
    some reason.

    The ARef<Chip> returned by new() is dropped, its reference count
    goes to zero, and our custom release_callback is called.

In this state:

Calling pwmchip_remove() would be incorrect because the chip was never
successfully added. If our callback only frees the Rust drvdata, the
memory for the C struct pwm_chip is leaked.

Therefore, our custom release_callback  must perform both cleanup tasks:
freeing the Rust drvdata and then calling pwmchip_release to free the C
struct. This "chaining" of the release handlers seems to be the most
robust way to guarantee cleanup in all scenarios.

Note that the bindings still use pwmchip_remove function in 'normal'
failure path.

> 
>> [...]
>>> ---
>>> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
> 
> I have problems applying this series and don't have this base commit in
> my repo.

Sorry for the confusion. Base commit doesn't exist in the mainline
kernel or linux-next, cause I've added some dependecies for compilation,
like IoMem for the driver (uploaded full branch on github [1]). The
bindings however doesn't depend on anything that's not in linux-next.
Anyway series applies cleanly on linux-next/master like so:

$ git fetch linux-next
$ git checkout linux-next/master
$ b4 shazam https://lore.kernel.org/all/20250707-rust-next-pwm-working-fan-for-sending-v10-0-d0c5cf342004@samsung.com/
Grabbing thread from lore.kernel.org/all/20250707-rust-next-pwm-working-fan-for-sending-v10-0-d0c5cf342004@samsung.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 14 messages in the thread
Looking for additional code-review trailers on lore.kernel.org
Analyzing 165 code-review messages
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v10 1/7] pwm: Export `pwmchip_release` for external use
  ✓ [PATCH v10 2/7] rust: pwm: Add Kconfig and basic data structures
  ✓ [PATCH v10 3/7] rust: pwm: Add complete abstraction layer
  ✓ [PATCH v10 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
  ✓ [PATCH v10 5/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
  ✓ [PATCH v10 6/7] riscv: dts: thead: Add PWM controller node
  ✓ [PATCH v10 7/7] riscv: dts: thead: Add PWM fan and thermal control
  ---
  ✓ Signed: DKIM/samsung.com
---
Total patches: 7
---
 Base: using specified base-commit 47753b5a1696283930a78aae79b29371f96f5bca
Applying: pwm: Export `pwmchip_release` for external use
Applying: rust: pwm: Add Kconfig and basic data structures
Applying: rust: pwm: Add complete abstraction layer
Applying: pwm: Add Rust driver for T-HEAD TH1520 SoC
Applying: dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
Applying: riscv: dts: thead: Add PWM controller node
Applying: riscv: dts: thead: Add PWM fan and thermal control


> 
> Best regards
> Uwe

[1] - https://github.com/mwilczy/linux/commits/rust-next-pwm-working-fan-for-sending-v15/

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

Hello Michal,

On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
> On 7/10/25 15:10, Uwe Kleine-König wrote:
> > On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
> >> On 7/7/25 11:48, Michal Wilczynski wrote:
> >>> The series is structured as follows:
> >>>  - Expose static function pwmchip_release.
> > 
> > Is this really necessary? I didn't try to understand the requirements
> > yet, but I wonder about that. If you get the pwmchip from
> > __pwmchip_add() the right thing to do to release it is to call
> > pwmchip_remove(). Feels like a layer violation.
> 
> It's required to prevent a memory leak in a specific, critical failure
> scenario. The sequence of events is as follows:
> 
>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>     the Rust drvdata.
> 
>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>     some reason.

If you called pwmchip_alloc() but not yet pwmchip_add(), the right
function to call for cleanup is pwmchip_put().

>     The ARef<Chip> returned by new() is dropped, its reference count
>     goes to zero, and our custom release_callback is called.
> 
> [...]
> >>> ---
> >>> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
> > 
> > I have problems applying this series and don't have this base commit in
> > my repo.
> 
> Sorry for the confusion. Base commit doesn't exist in the mainline
> kernel or linux-next, cause I've added some dependecies for compilation,
> like IoMem for the driver (uploaded full branch on github [1]). The
> bindings however doesn't depend on anything that's not in linux-next.

The series didn't apply to my pwm/for-next branch.

Note that the base-commit should always be a publically known commit.
See the chapter about "Base Tree Information" in git-format-patch(1).

Best regards
Uwe

On 7/10/25 17:25, Uwe Kleine-König wrote:
> Hello Michal,
> 
> On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>> On 7/10/25 15:10, Uwe Kleine-König wrote:
>>> On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>>>> On 7/7/25 11:48, Michal Wilczynski wrote:
>>>>> The series is structured as follows:
>>>>>  - Expose static function pwmchip_release.
>>>
>>> Is this really necessary? I didn't try to understand the requirements
>>> yet, but I wonder about that. If you get the pwmchip from
>>> __pwmchip_add() the right thing to do to release it is to call
>>> pwmchip_remove(). Feels like a layer violation.
>>
>> It's required to prevent a memory leak in a specific, critical failure
>> scenario. The sequence of events is as follows:
>>
>>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>>     the Rust drvdata.
>>
>>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>>     some reason.
> 
> If you called pwmchip_alloc() but not yet pwmchip_add(), the right
> function to call for cleanup is pwmchip_put().
> 
>>     The ARef<Chip> returned by new() is dropped, its reference count
>>     goes to zero, and our custom release_callback is called.
>>
>> [...]
>>>>> ---
>>>>> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
>>>
>>> I have problems applying this series and don't have this base commit in
>>> my repo.
>>
>> Sorry for the confusion. Base commit doesn't exist in the mainline
>> kernel or linux-next, cause I've added some dependecies for compilation,
>> like IoMem for the driver (uploaded full branch on github [1]). The
>> bindings however doesn't depend on anything that's not in linux-next.
> 
> The series didn't apply to my pwm/for-next branch.
> 
> Note that the base-commit should always be a publically known commit.
> See the chapter about "Base Tree Information" in git-format-patch(1).

Hello Uwe,

Okay, thank you for the clarification. I understand the requirement for
a public base commit.

My intention was to include the TH1520 driver primarily as a practical
demonstration of the new abstractions. However the driver can't be
merged as is, since it depends on the unmerged IoMem series and won't
compile against a public commit.

I will rebase the series on pwm/for-next and drop the driver and its
associated device tree patches for now. I'll send a new version
containing just the core PWM abstraction patches, which apply cleanly.

I will resubmit the driver patches once their dependencies are available
in a public tree.

> 
> Best regards
> Uwe

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

Hello,

On Thu, Jul 10, 2025 at 06:58:41PM +0200, Michal Wilczynski wrote:
> On 7/10/25 17:25, Uwe Kleine-König wrote:
> > On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
> >> On 7/10/25 15:10, Uwe Kleine-König wrote:
> >>> On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
> >>>> On 7/7/25 11:48, Michal Wilczynski wrote:
> >>>>> The series is structured as follows:
> >>>>>  - Expose static function pwmchip_release.
> >>>
> >>> Is this really necessary? I didn't try to understand the requirements
> >>> yet, but I wonder about that. If you get the pwmchip from
> >>> __pwmchip_add() the right thing to do to release it is to call
> >>> pwmchip_remove(). Feels like a layer violation.
> >>
> >> It's required to prevent a memory leak in a specific, critical failure
> >> scenario. The sequence of events is as follows:
> >>
> >>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
> >>     the Rust drvdata.
> >>
> >>     pwm::Registration::register() (which calls pwmchip_add()) fails for
> >>     some reason.
> > 
> > If you called pwmchip_alloc() but not yet pwmchip_add(), the right
> > function to call for cleanup is pwmchip_put().
> > 
> >>     The ARef<Chip> returned by new() is dropped, its reference count
> >>     goes to zero, and our custom release_callback is called.
> >>
> >> [...]
> >>>>> ---
> >>>>> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
> >>>
> >>> I have problems applying this series and don't have this base commit in
> >>> my repo.
> >>
> >> Sorry for the confusion. Base commit doesn't exist in the mainline
> >> kernel or linux-next, cause I've added some dependecies for compilation,
> >> like IoMem for the driver (uploaded full branch on github [1]). The
> >> bindings however doesn't depend on anything that's not in linux-next.
> > 
> > The series didn't apply to my pwm/for-next branch.
> > 
> > Note that the base-commit should always be a publically known commit.
> > See the chapter about "Base Tree Information" in git-format-patch(1).
> 
> Hello Uwe,
> 
> Okay, thank you for the clarification. I understand the requirement for
> a public base commit.
> 
> My intention was to include the TH1520 driver primarily as a practical
> demonstration of the new abstractions. However the driver can't be
> merged as is, since it depends on the unmerged IoMem series and won't
> compile against a public commit.
> 
> I will rebase the series on pwm/for-next and drop the driver and its
> associated device tree patches for now. I'll send a new version
> containing just the core PWM abstraction patches, which apply cleanly.
> 
> I will resubmit the driver patches once their dependencies are available
> in a public tree.

If you base your tree on (say) v6.16-rc1, then add some Rust
dependencies up to 47753b5a1696283930a78aae79b29371f96f5bca and then add
your series, you just do:

	git format-patch --base v6.16-rc1 47753b5a1696283930a78aae79b29371f96f5bca..

. This results in a base-commit line that I (and maybe also build bots)
can use and a bunch of further lines listing the commits between
v6.16-rc1 and 47753b5a1696283930a78aae79b29371f96f5bca that might be
findable on lore.k.o.

Best regards
Uwe

On 7/10/25 22:39, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jul 10, 2025 at 06:58:41PM +0200, Michal Wilczynski wrote:
>> On 7/10/25 17:25, Uwe Kleine-König wrote:
>>> On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>>>> On 7/10/25 15:10, Uwe Kleine-König wrote:
>>>>> On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>>>>>> On 7/7/25 11:48, Michal Wilczynski wrote:
>>>>>>> The series is structured as follows:
>>>>>>>  - Expose static function pwmchip_release.
>>>>>
>>>>> Is this really necessary? I didn't try to understand the requirements
>>>>> yet, but I wonder about that. If you get the pwmchip from
>>>>> __pwmchip_add() the right thing to do to release it is to call
>>>>> pwmchip_remove(). Feels like a layer violation.
>>>>
>>>> It's required to prevent a memory leak in a specific, critical failure
>>>> scenario. The sequence of events is as follows:
>>>>
>>>>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>>>>     the Rust drvdata.
>>>>
>>>>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>>>>     some reason.
>>>
>>> If you called pwmchip_alloc() but not yet pwmchip_add(), the right
>>> function to call for cleanup is pwmchip_put().
>>>
>>>>     The ARef<Chip> returned by new() is dropped, its reference count
>>>>     goes to zero, and our custom release_callback is called.
>>>>
>>>> [...]
>>>>>>> ---
>>>>>>> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
>>>>>
>>>>> I have problems applying this series and don't have this base commit in
>>>>> my repo.
>>>>
>>>> Sorry for the confusion. Base commit doesn't exist in the mainline
>>>> kernel or linux-next, cause I've added some dependecies for compilation,
>>>> like IoMem for the driver (uploaded full branch on github [1]). The
>>>> bindings however doesn't depend on anything that's not in linux-next.
>>>
>>> The series didn't apply to my pwm/for-next branch.
>>>
>>> Note that the base-commit should always be a publically known commit.
>>> See the chapter about "Base Tree Information" in git-format-patch(1).
>>
>> Hello Uwe,
>>
>> Okay, thank you for the clarification. I understand the requirement for
>> a public base commit.
>>
>> My intention was to include the TH1520 driver primarily as a practical
>> demonstration of the new abstractions. However the driver can't be
>> merged as is, since it depends on the unmerged IoMem series and won't
>> compile against a public commit.
>>
>> I will rebase the series on pwm/for-next and drop the driver and its
>> associated device tree patches for now. I'll send a new version
>> containing just the core PWM abstraction patches, which apply cleanly.
>>
>> I will resubmit the driver patches once their dependencies are available
>> in a public tree.
> 
> If you base your tree on (say) v6.16-rc1, then add some Rust
> dependencies up to 47753b5a1696283930a78aae79b29371f96f5bca and then add
> your series, you just do:
> 
> 	git format-patch --base v6.16-rc1 47753b5a1696283930a78aae79b29371f96f5bca..
> 
> . This results in a base-commit line that I (and maybe also build bots)
> can use and a bunch of further lines listing the commits between
> v6.16-rc1 and 47753b5a1696283930a78aae79b29371f96f5bca that might be
> findable on lore.k.o.

Hi Uwe,

Thank you very much for the detailed advice on using git format-patch
--base. I appreciate you taking the time to explain the workflow.

I investigated this approach, and the difficulty is that the IoMem
series [1], which my driver depends on, is itself based on an
integration tree rather than a clean public tag.

This means that to create a series based on v6.16-rc1, I would have to
include a very large number of intermediate commits from linux-next,
which, would not be helpful for review.

Therefore, I believe that dropping the driver and its device tree
patches for now is the best path forward. This will result in a much
smaller, self contained series for the core PWM abstractions that
applies cleanly to your pwm/for-next branch.


[1] - https://lore.kernel.org/rust-for-linux/20250704-topics-tyr-platform_iomem-v12-0-1d3d4bd8207d@collabora.com/
> 
> Best regards
> Uwe

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

On Thu Jul 10, 2025 at 5:25 PM CEST, Uwe Kleine-König wrote:
> Hello Michal,
>
> On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>> On 7/10/25 15:10, Uwe Kleine-König wrote:
>> > On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>> >> On 7/7/25 11:48, Michal Wilczynski wrote:
>> >>> The series is structured as follows:
>> >>>  - Expose static function pwmchip_release.
>> > 
>> > Is this really necessary? I didn't try to understand the requirements
>> > yet, but I wonder about that. If you get the pwmchip from
>> > __pwmchip_add() the right thing to do to release it is to call
>> > pwmchip_remove(). Feels like a layer violation.
>> 
>> It's required to prevent a memory leak in a specific, critical failure
>> scenario. The sequence of events is as follows:
>> 
>>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>>     the Rust drvdata.
>> 
>>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>>     some reason.
>

(Just trying to help clear up the confusion.)

> If you called pwmchip_alloc() but not yet pwmchip_add(), the right
> function to call for cleanup is pwmchip_put().

That is exactly what is happening when ARef<Chip> is dropped. If the reference
count drops to zero, pwmchip_release() is called, which frees the chip. However,
this would leave the driver's private data allocation behind, which is owned by
the Chip instance.

So, in Rust we not only have to free the chip itself on release, but also the
driver's private data. The solution Michal went for is overwriting the PWM
chip's dev->release() with a callback that drops the driver's private data and
subsequently calls the "original" pwmchip_release().

This is a common pattern in Rust that we use in DRM as well. One thing that is
different in DRM is, that a struct drm_device (equivalent of struct pwm_chip in
this case), has it's own release callback for drivers that we can attach to.

PWM does not have such a callback AFAICS, hence the Rust abstraction uses the
underlying device's release callback and then forwards to pwmchip_release().

Hope this helps. :)

On Thu, Jul 10, 2025 at 06:06:26PM +0200, Danilo Krummrich wrote:
> On Thu Jul 10, 2025 at 5:25 PM CEST, Uwe Kleine-König wrote:
> > Hello Michal,
> >
> > On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
> >> On 7/10/25 15:10, Uwe Kleine-König wrote:
> >> > On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
> >> >> On 7/7/25 11:48, Michal Wilczynski wrote:
> >> >>> The series is structured as follows:
> >> >>>  - Expose static function pwmchip_release.
> >> > 
> >> > Is this really necessary? I didn't try to understand the requirements
> >> > yet, but I wonder about that. If you get the pwmchip from
> >> > __pwmchip_add() the right thing to do to release it is to call
> >> > pwmchip_remove(). Feels like a layer violation.
> >> 
> >> It's required to prevent a memory leak in a specific, critical failure
> >> scenario. The sequence of events is as follows:
> >> 
> >>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
> >>     the Rust drvdata.
> >> 
> >>     pwm::Registration::register() (which calls pwmchip_add()) fails for
> >>     some reason.
> >
> 
> (Just trying to help clear up the confusion.)

Very appreciated!

> > If you called pwmchip_alloc() but not yet pwmchip_add(), the right
> > function to call for cleanup is pwmchip_put().
> 
> That is exactly what is happening when ARef<Chip> is dropped. If the reference
> count drops to zero, pwmchip_release() is called, which frees the chip. However,
> this would leave the driver's private data allocation behind, which is owned by
> the Chip instance.

I don't understand that. The chip and the driver private data both are
located in the same allocation. How is this a problem of the driver
private data only then? The kfree() in pwmchip_release() is good enough
for both?!

> So, in Rust we not only have to free the chip itself on release, but also the
> driver's private data. The solution Michal went for is overwriting the PWM
> chip's dev->release() with a callback that drops the driver's private data and
> subsequently calls the "original" pwmchip_release().
> 
> This is a common pattern in Rust that we use in DRM as well. One thing that is
> different in DRM is, that a struct drm_device (equivalent of struct pwm_chip in
> this case), has it's own release callback for drivers that we can attach to.
> 
> PWM does not have such a callback AFAICS, hence the Rust abstraction uses the
> underlying device's release callback and then forwards to pwmchip_release().
> 
> Hope this helps. :)

Not yet ... :-)

Best regards
Uwe

On Thu Jul 10, 2025 at 10:57 PM CEST, Uwe Kleine-König wrote:
> On Thu, Jul 10, 2025 at 06:06:26PM +0200, Danilo Krummrich wrote:
>> On Thu Jul 10, 2025 at 5:25 PM CEST, Uwe Kleine-König wrote:
>> > Hello Michal,
>> >
>> > On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>> >> On 7/10/25 15:10, Uwe Kleine-König wrote:
>> >> > On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>> >> >> On 7/7/25 11:48, Michal Wilczynski wrote:
>> >> >>> The series is structured as follows:
>> >> >>>  - Expose static function pwmchip_release.
>> >> > 
>> >> > Is this really necessary? I didn't try to understand the requirements
>> >> > yet, but I wonder about that. If you get the pwmchip from
>> >> > __pwmchip_add() the right thing to do to release it is to call
>> >> > pwmchip_remove(). Feels like a layer violation.
>> >> 
>> >> It's required to prevent a memory leak in a specific, critical failure
>> >> scenario. The sequence of events is as follows:
>> >> 
>> >>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>> >>     the Rust drvdata.
>> >> 
>> >>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>> >>     some reason.
>> >
>> 
>> (Just trying to help clear up the confusion.)
>
> Very appreciated!
>
>> > If you called pwmchip_alloc() but not yet pwmchip_add(), the right
>> > function to call for cleanup is pwmchip_put().
>> 
>> That is exactly what is happening when ARef<Chip> is dropped. If the reference
>> count drops to zero, pwmchip_release() is called, which frees the chip. However,
>> this would leave the driver's private data allocation behind, which is owned by
>> the Chip instance.
>
> I don't understand that. The chip and the driver private data both are
> located in the same allocation. How is this a problem of the driver
> private data only then? The kfree() in pwmchip_release() is good enough
> for both?!

Not in the current abstractions, there are two allocations, one for the Chip and
one for the driver's private data, or in other words the abstraction uses
pwmchip_set_drvdata() and pwmchip_get_drvdata().

Having a brief look at pwmchip_alloc(), it seems to me that PWM supports the
subclassing pattern with pwmchip_priv().

We should probably take advantage of that. Assuming we do that, the Rust
abstraction still needs a release() callback because we still need to call
drop_in_place() in order to get the destructor of the driver's private data
type called. We actually missed this in DRM and I fixed it up recently [1].

@Michal: With the subclassing pattern the Chip structure would look like this:

	#[repr(C)]
	#[pin_data]
	pub struct Chip<T> {
	   inner: Opaque<bindings::pwm_chip>,
	   #[pin]
	   data: T,
	}

And in the release() callback would look like this:

    extern "C" fn release(ptr: *mut bindings::pwm_chip) {
        // CAST: Casting `ptr` to `Chip<T>` is valid, since [...].
        let this = ptr.cast<Chip<T>>();

        // SAFETY:
        // - When `release` runs it is guaranteed that there is no further access to `this`.
        // - `this` is valid for dropping.
        unsafe { core::ptr::drop_in_place(this) };
    }

This is exactly what we're doing in DRM as well, I would have recommended this
to begin with, but I didn't recognize that PWM supports subclassing. :)

I recommend having a look at [2].

[1] https://lore.kernel.org/all/20250629153747.72536-1-dakr@kernel.org/
[2] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-fixes/rust/kernel/drm/device.rs

On 7/10/25 23:19, Danilo Krummrich wrote:
> On Thu Jul 10, 2025 at 10:57 PM CEST, Uwe Kleine-König wrote:
>> On Thu, Jul 10, 2025 at 06:06:26PM +0200, Danilo Krummrich wrote:
>>> On Thu Jul 10, 2025 at 5:25 PM CEST, Uwe Kleine-König wrote:
>>>> Hello Michal,
>>>>
>>>> On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>>>>> On 7/10/25 15:10, Uwe Kleine-König wrote:
>>>>>> On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>>>>>>> On 7/7/25 11:48, Michal Wilczynski wrote:
>>>>>>>> The series is structured as follows:
>>>>>>>>  - Expose static function pwmchip_release.
>>>>>>
>>>>>> Is this really necessary? I didn't try to understand the requirements
>>>>>> yet, but I wonder about that. If you get the pwmchip from
>>>>>> __pwmchip_add() the right thing to do to release it is to call
>>>>>> pwmchip_remove(). Feels like a layer violation.
>>>>>
>>>>> It's required to prevent a memory leak in a specific, critical failure
>>>>> scenario. The sequence of events is as follows:
>>>>>
>>>>>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>>>>>     the Rust drvdata.
>>>>>
>>>>>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>>>>>     some reason.
>>>>
>>>
>>> (Just trying to help clear up the confusion.)
>>
>> Very appreciated!
>>
>>>> If you called pwmchip_alloc() but not yet pwmchip_add(), the right
>>>> function to call for cleanup is pwmchip_put().
>>>
>>> That is exactly what is happening when ARef<Chip> is dropped. If the reference
>>> count drops to zero, pwmchip_release() is called, which frees the chip. However,
>>> this would leave the driver's private data allocation behind, which is owned by
>>> the Chip instance.
>>
>> I don't understand that. The chip and the driver private data both are
>> located in the same allocation. How is this a problem of the driver
>> private data only then? The kfree() in pwmchip_release() is good enough
>> for both?!
> 
> Not in the current abstractions, there are two allocations, one for the Chip and
> one for the driver's private data, or in other words the abstraction uses
> pwmchip_set_drvdata() and pwmchip_get_drvdata().
> 
> Having a brief look at pwmchip_alloc(), it seems to me that PWM supports the
> subclassing pattern with pwmchip_priv().
> 
> We should probably take advantage of that. Assuming we do that, the Rust
> abstraction still needs a release() callback because we still need to call
> drop_in_place() in order to get the destructor of the driver's private data
> type called. We actually missed this in DRM and I fixed it up recently [1].
> 
> @Michal: With the subclassing pattern the Chip structure would look like this:
> 
> 	#[repr(C)]
> 	#[pin_data]
> 	pub struct Chip<T> {
> 	   inner: Opaque<bindings::pwm_chip>,
> 	   #[pin]
> 	   data: T,
> 	}
}

Hello

Thank you both for the detailed feedback and suggestions.

Danilo, you are right, we should absolutely use the subclassing pattern
to have a single allocation for the chip and driver data. This is a much
cleaner design.

As I looked into this, the main difference is that the C struct pwm_chip
doesn't have a fixed size because of the pwms[] array at the end. This
prevents us from using the exact struct layout you suggested.

pub pwms: __IncompleteArrayField<pwm_device>,

Therefore, to correctly implement the subclassing pattern, it would be
sufficient to leave the current struct as is and use pwmchip_get_drvdata to
acquire pointers to the allocated drvdata.

pub struct Chip<T: PwmOps>(Opaque<bindings::pwm_chip>, PhantomData<T>);

This will still achieve the goal of a single allocation via
pwmchip_alloc's sizeof_priv argument, while working around the DST
limitation.

> 
> And in the release() callback would look like this:
> 
>     extern "C" fn release(ptr: *mut bindings::pwm_chip) {
>         // CAST: Casting `ptr` to `Chip<T>` is valid, since [...].
>         let this = ptr.cast<Chip<T>>();

I think this would use pwmchip_get_drvdata instead.

> 
>         // SAFETY:
>         // - When `release` runs it is guaranteed that there is no further access to `this`.
>         // - `this` is valid for dropping.
>         unsafe { core::ptr::drop_in_place(this) };
>     }
> 
> This is exactly what we're doing in DRM as well, I would have recommended this
> to begin with, but I didn't recognize that PWM supports subclassing. :)
> 
> I recommend having a look at [2].
> 
> [1] https://lore.kernel.org/all/20250629153747.72536-1-dakr@kernel.org/
> [2] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-fixes/rust/kernel/drm/device.rs
> 

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

On Thu Jul 10, 2025 at 10:42 AM CEST, Michal Wilczynski wrote:
> I was hoping you could clarify the intended merge path for this series,
> as it introduces changes to both the Rust and PWM subsystems.
>
> Is the expectation that the Rust maintainers will take the abstraction
> patches into the Rust tree first? Or would Uwe, as the PWM maintainer,
> pull the entire series? Any guidance on the coordination would be very
> helpful.

Except for the helpers I only see PWM code, so this is fully on Uwe's purview I
think.

I see that there is a new MAINTAINERS entry:

	PWM SUBSYSTEM BINDINGS [RUST]
	M:	Michal Wilczynski <m.wilczynski@samsung.com>
	S:	Maintained
	F:	rust/helpers/pwm.c
	F:	rust/kernel/pwm.rs

I assume this is agreed with Uwe?

In case there's no agreement yet, the typical options are:

  1) Maintain the Rust abstractions as part of the existing MAINTAINERS entry.
     Optionally, the author can be added as another maintainer or reviewer.

  2) Add a separate MAINTAINERS entry; patches / PRs still go through the same
     subsystem tree.

  3) Add a separate MAINTAINERS entry; patches don't go through the subsystem
     tree (e.g. because the subsystem maintainers don't want to deal with it).

I don't recommend (3), since it's really just a fallback.

The above looks like (2). In this case I recommend to also add the C maintainers
as reviewers, such that they can easily follow along and specifiy the tree (T:).

But, of course, that's up to you and Uwe.

> I understand that it may be too late in the development cycle to merge
> the full series. If that's the case, perhaps patch 2 could be considered
> on its own, as it hasn't received comments in the last couple of
> revisions. As another possibility, patch 1 and patch 3 are dependent on
> each other and could be applied as a pair, depending on your assessment.
>
> The RISC-V driver itself would need to wait for the IoMem series merge [1].
>
> [1] - https://lore.kernel.org/rust-for-linux/20250704-topics-tyr-platform_iomem-v12-0-1d3d4bd8207d@collabora.com/
>
> Best regards,

On 7/10/25 12:17, Danilo Krummrich wrote:
> On Thu Jul 10, 2025 at 10:42 AM CEST, Michal Wilczynski wrote:
>> I was hoping you could clarify the intended merge path for this series,
>> as it introduces changes to both the Rust and PWM subsystems.
>>
>> Is the expectation that the Rust maintainers will take the abstraction
>> patches into the Rust tree first? Or would Uwe, as the PWM maintainer,
>> pull the entire series? Any guidance on the coordination would be very
>> helpful.
> 
> Except for the helpers I only see PWM code, so this is fully on Uwe's purview I
> think.
> 
> I see that there is a new MAINTAINERS entry:
> 
> 	PWM SUBSYSTEM BINDINGS [RUST]
> 	M:	Michal Wilczynski <m.wilczynski@samsung.com>
> 	S:	Maintained
> 	F:	rust/helpers/pwm.c
> 	F:	rust/kernel/pwm.rs
> 
> I assume this is agreed with Uwe?
> 
> In case there's no agreement yet, the typical options are:
> 
>   1) Maintain the Rust abstractions as part of the existing MAINTAINERS entry.
>      Optionally, the author can be added as another maintainer or reviewer.
> 
>   2) Add a separate MAINTAINERS entry; patches / PRs still go through the same
>      subsystem tree.
> 
>   3) Add a separate MAINTAINERS entry; patches don't go through the subsystem
>      tree (e.g. because the subsystem maintainers don't want to deal with it).
> 
> I don't recommend (3), since it's really just a fallback.
> 
> The above looks like (2). In this case I recommend to also add the C maintainers
> as reviewers, such that they can easily follow along and specifiy the tree (T:).
> 
> But, of course, that's up to you and Uwe.

Thanks, it is not agreed yet, I've included a MAINTAINERS entry, since I
would like to help with the maintenance of the code, so I would also
vote for the 2) option, but ultimately it's an Uwe decision so I would
be happy to follow on anything he decides.

> 
>> I understand that it may be too late in the development cycle to merge
>> the full series. If that's the case, perhaps patch 2 could be considered
>> on its own, as it hasn't received comments in the last couple of
>> revisions. As another possibility, patch 1 and patch 3 are dependent on
>> each other and could be applied as a pair, depending on your assessment.
>>
>> The RISC-V driver itself would need to wait for the IoMem series merge [1].
>>
>> [1] - https://lore.kernel.org/rust-for-linux/20250704-topics-tyr-platform_iomem-v12-0-1d3d4bd8207d@collabora.com/
>>
>> Best regards,
> 
> 

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

Hello,

On Thu, Jul 10, 2025 at 12:29:59PM +0200, Michal Wilczynski wrote:
> On 7/10/25 12:17, Danilo Krummrich wrote:
> > On Thu Jul 10, 2025 at 10:42 AM CEST, Michal Wilczynski wrote:
> >> I was hoping you could clarify the intended merge path for this series,
> >> as it introduces changes to both the Rust and PWM subsystems.
> >>
> >> Is the expectation that the Rust maintainers will take the abstraction
> >> patches into the Rust tree first? Or would Uwe, as the PWM maintainer,
> >> pull the entire series? Any guidance on the coordination would be very
> >> helpful.
> > 
> > Except for the helpers I only see PWM code, so this is fully on Uwe's purview I
> > think.
> > 
> > I see that there is a new MAINTAINERS entry:
> > 
> > 	PWM SUBSYSTEM BINDINGS [RUST]
> > 	M:	Michal Wilczynski <m.wilczynski@samsung.com>
> > 	S:	Maintained
> > 	F:	rust/helpers/pwm.c
> > 	F:	rust/kernel/pwm.rs
> > 
> > I assume this is agreed with Uwe?

I suggest to add

	L:	linux-pwm@vger.kernel.org

then I'm happy with it. I'm definitively not capable to review the Rust
details of Rust code. But I think Rust is readable enough that I can
judge the algorithmic parts.

Best regards
Uwe

On Thu Jul 10, 2025 at 3:36 PM CEST, Uwe Kleine-König wrote:
> On Thu, Jul 10, 2025 at 12:29:59PM +0200, Michal Wilczynski wrote:
>> On 7/10/25 12:17, Danilo Krummrich wrote:
>> > I see that there is a new MAINTAINERS entry:
>> > 
>> > 	PWM SUBSYSTEM BINDINGS [RUST]
>> > 	M:	Michal Wilczynski <m.wilczynski@samsung.com>
>> > 	S:	Maintained
>> > 	F:	rust/helpers/pwm.c
>> > 	F:	rust/kernel/pwm.rs
>> > 
>> > I assume this is agreed with Uwe?
>
> I suggest to add
>
> 	L:	linux-pwm@vger.kernel.org
>
> then I'm happy with it. I'm definitively not capable to review the Rust
> details of Rust code. But I think Rust is readable enough that I can
> judge the algorithmic parts.

What about the merge strategy? Do you want to pick up patches yourself (through
your tree), receive PRs from Michal or none of those?