[PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[Markus Probst]

The `pwm::Registration::register` function provides no guarantee that the
function isn't called twice with the same pwm chip, which is considered
unsafe.

Add `pwm::UnregisteredChip` as wrapper around `pwm::Chip`.
Implement `pwm::UnregisteredChip::register` for the registration. This
function takes ownership of `pwm::UnregisteredChip` and therefore
guarantees that the registration can't be called twice on the same pwm
chip.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
This patch provides the additional guarantee that the pwm chip doesn't
get registered twice.

The following changes were made:
- change the visibility of `pwm::Registration` to private
- remove the `pwm::Registration::register` function
- add `pwm::UnregisteredChip` - a wrapper around `pwm::Chip`
- return `pwm::UnregisteredChip` in `pwm::Chip::new`
- add `pwm::UnregisteredChip::register` for registering the pwm chip
  once
- add Send + Sync bounds to `PwmOps`

Note that I wasn't able to test this patch, due to the lack of hardware.

Also I was not able to successfully compile drivers/pwm/pwm_th1520.rs,
as `clk::Clk` seems to be missing. I haven't found the missing patch
in linux-next nor in

https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/log/?h=pwm/for-next

(the tree in which the th1520 pwm driver was merged).
So please verify yourself that the driver compiles and throws no errors.
---
Changes in v2:
- use the `pwm::UnregisteredChip` wrapper instead of moving the
  registration into `pwm::Chip::new` to allow access to the chip prior
  to the registration
- Link to v1: https://lore.kernel.org/r/20251127-pwm_safe_register-v1-1-d22d0ed068ac@posteo.de
---
 drivers/pwm/pwm_th1520.rs |  2 +-
 rust/kernel/pwm.rs        | 68 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
index 955c359b07fb..65a52b6620ab 100644
--- a/drivers/pwm/pwm_th1520.rs
+++ b/drivers/pwm/pwm_th1520.rs
@@ -372,7 +372,7 @@ fn probe(
             }),
         )?;
 
-        pwm::Registration::register(dev, chip)?;
+        chip.register()?;
 
         Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
     }
diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
index cb00f8a8765c..bf7515d04d8a 100644
--- a/rust/kernel/pwm.rs
+++ b/rust/kernel/pwm.rs
@@ -15,7 +15,11 @@
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque}, //
 };
-use core::{marker::PhantomData, ptr::NonNull};
+use core::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::NonNull, //
+};
 
 /// Represents a PWM waveform configuration.
 /// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
@@ -173,7 +177,7 @@ pub struct RoundedWaveform<WfHw> {
 }
 
 /// Trait defining the operations for a PWM driver.
-pub trait PwmOps: 'static + Sized {
+pub trait PwmOps: 'static + Send + Sync + Sized {
     /// The driver-specific hardware representation of a waveform.
     ///
     /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
@@ -584,11 +588,12 @@ unsafe fn bound_parent_device(&self) -> &device::Device<Bound> {
     ///
     /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
     /// on its embedded `struct device`.
-    pub fn new(
-        parent_dev: &device::Device,
+    #[allow(clippy::new_ret_no_self)]
+    pub fn new<'a>(
+        parent_dev: &'a device::Device<Bound>,
         num_channels: u32,
         data: impl pin_init::PinInit<T, Error>,
-    ) -> Result<ARef<Self>> {
+    ) -> Result<UnregisteredChip<'a, T>> {
         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 =
@@ -623,7 +628,9 @@ pub fn new(
         // 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)) })
+        let chip = unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) };
+
+        Ok(UnregisteredChip { chip, parent_dev })
     }
 }
 
@@ -654,37 +661,29 @@ unsafe fn dec_ref(obj: NonNull<Chip<T>>) {
 // 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> {}
+unsafe impl<T: PwmOps> 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> {}
+unsafe impl<T: PwmOps> 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> {
+/// A wrapper arround `ARef<Chip<T>>` that ensures that `register` can only be called once.
+pub struct UnregisteredChip<'a, T: PwmOps> {
     chip: ARef<Chip<T>>,
+    parent_dev: &'a device::Device<Bound>,
 }
 
-impl<T: 'static + PwmOps + Send + Sync> Registration<T> {
+impl<T: PwmOps> UnregisteredChip<'_, 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();
+    pub fn register(self) -> Result<ARef<Chip<T>>> {
+        let c_chip_ptr = self.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.
@@ -692,12 +691,33 @@ pub fn register(dev: &device::Device<Bound>, chip: ARef<Chip<T>>) -> Result {
             to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
         }
 
-        let registration = Registration { chip };
+        let registration = Registration {
+            chip: ARef::clone(&self.chip),
+        };
+
+        devres::register(self.parent_dev, registration, GFP_KERNEL)?;
 
-        devres::register(dev, registration, GFP_KERNEL)
+        Ok(self.chip)
     }
 }
 
+impl<T: PwmOps> Deref for UnregisteredChip<'_, T> {
+    type Target = Chip<T>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.chip
+    }
+}
+
+/// 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.
+struct Registration<T: PwmOps> {
+    chip: ARef<Chip<T>>,
+}
+
 impl<T: PwmOps> Drop for Registration<T> {
     fn drop(&mut self) {
         let chip_raw = self.chip.as_raw();

---
base-commit: fae00ea9f00367771003ace78f29549dead58fc7
change-id: 20251127-pwm_safe_register-e49b0a261101

Re: [PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[Michal Wilczynski]

Hi Markus,
On 12/2/25 19:17, Markus Probst wrote:
> The `pwm::Registration::register` function provides no guarantee that the
> function isn't called twice with the same pwm chip, which is considered
> unsafe.
> 
> Add `pwm::UnregisteredChip` as wrapper around `pwm::Chip`.
> Implement `pwm::UnregisteredChip::register` for the registration. This
> function takes ownership of `pwm::UnregisteredChip` and therefore
> guarantees that the registration can't be called twice on the same pwm
> chip.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> This patch provides the additional guarantee that the pwm chip doesn't
> get registered twice.
> 
> The following changes were made:
> - change the visibility of `pwm::Registration` to private
> - remove the `pwm::Registration::register` function
> - add `pwm::UnregisteredChip` - a wrapper around `pwm::Chip`
> - return `pwm::UnregisteredChip` in `pwm::Chip::new`
> - add `pwm::UnregisteredChip::register` for registering the pwm chip
>   once
> - add Send + Sync bounds to `PwmOps`
> 
> Note that I wasn't able to test this patch, due to the lack of hardware.
> 
> Also I was not able to successfully compile drivers/pwm/pwm_th1520.rs,
> as `clk::Clk` seems to be missing. I haven't found the missing patch
> in linux-next nor in

I suspect your kernel configuration is missing CONFIG_COMMON_CLK.

> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/log/?h=pwm/for-next
> 
> (the tree in which the th1520 pwm driver was merged).
> So please verify yourself that the driver compiles and throws no errors.
> ---
> Changes in v2:
> - use the `pwm::UnregisteredChip` wrapper instead of moving the
>   registration into `pwm::Chip::new` to allow access to the chip prior
>   to the registration

Agree with this. By returning UnregisteredChip, you solved the safety
issue (preventing double registration) while correctly preserving the
"configuration gap" needed for complex drivers. Good example of Type
State pattern :-)

> - Link to v1: https://lore.kernel.org/r/20251127-pwm_safe_register-v1-1-d22d0ed068ac@posteo.de
> ---
>  drivers/pwm/pwm_th1520.rs |  2 +-
>  rust/kernel/pwm.rs        | 68 ++++++++++++++++++++++++++++++-----------------
>  2 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> index 955c359b07fb..65a52b6620ab 100644
> --- a/drivers/pwm/pwm_th1520.rs
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -372,7 +372,7 @@ fn probe(
>              }),
>          )?;
>  
> -        pwm::Registration::register(dev, chip)?;
> +        chip.register()?;
>  
>          Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
>      }
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> index cb00f8a8765c..bf7515d04d8a 100644
> --- a/rust/kernel/pwm.rs
> +++ b/rust/kernel/pwm.rs
> @@ -15,7 +15,11 @@
>      prelude::*,
>      types::{ARef, AlwaysRefCounted, Opaque}, //
>  };
> -use core::{marker::PhantomData, ptr::NonNull};
> +use core::{
> +    marker::PhantomData,
> +    ops::Deref,
> +    ptr::NonNull, //
> +};
>  
>  /// Represents a PWM waveform configuration.
>  /// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
> @@ -173,7 +177,7 @@ pub struct RoundedWaveform<WfHw> {
>  }
>  
>  /// Trait defining the operations for a PWM driver.
> -pub trait PwmOps: 'static + Sized {
> +pub trait PwmOps: 'static + Send + Sync + Sized {
>      /// The driver-specific hardware representation of a waveform.
>      ///
>      /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
> @@ -584,11 +588,12 @@ unsafe fn bound_parent_device(&self) -> &device::Device<Bound> {
>      ///
>      /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
>      /// on its embedded `struct device`.
> -    pub fn new(
> -        parent_dev: &device::Device,
> +    #[allow(clippy::new_ret_no_self)]
> +    pub fn new<'a>(
> +        parent_dev: &'a device::Device<Bound>,
>          num_channels: u32,
>          data: impl pin_init::PinInit<T, Error>,
> -    ) -> Result<ARef<Self>> {
> +    ) -> Result<UnregisteredChip<'a, T>> {
>          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 =
> @@ -623,7 +628,9 @@ pub fn new(
>          // 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)) })
> +        let chip = unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) };
> +
> +        Ok(UnregisteredChip { chip, parent_dev })
>      }
>  }
>  
> @@ -654,37 +661,29 @@ unsafe fn dec_ref(obj: NonNull<Chip<T>>) {
>  // 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> {}
> +unsafe impl<T: PwmOps> 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> {}
> +unsafe impl<T: PwmOps> 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> {
> +/// A wrapper arround `ARef<Chip<T>>` that ensures that `register` can only be called once.

nit:
s/arround/around/

> +pub struct UnregisteredChip<'a, T: PwmOps> {
>      chip: ARef<Chip<T>>,
> +    parent_dev: &'a device::Device<Bound>,
>  }
>  
> -impl<T: 'static + PwmOps + Send + Sync> Registration<T> {
> +impl<T: PwmOps> UnregisteredChip<'_, 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();
> +    pub fn register(self) -> Result<ARef<Chip<T>>> {
> +        let c_chip_ptr = self.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.
> @@ -692,12 +691,33 @@ pub fn register(dev: &device::Device<Bound>, chip: ARef<Chip<T>>) -> Result {
>              to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
>          }
>  
> -        let registration = Registration { chip };
> +        let registration = Registration {
> +            chip: ARef::clone(&self.chip),
> +        };
> +
> +        devres::register(self.parent_dev, registration, GFP_KERNEL)?;
>  
> -        devres::register(dev, registration, GFP_KERNEL)
> +        Ok(self.chip)
>      }
>  }
>  
> +impl<T: PwmOps> Deref for UnregisteredChip<'_, T> {
> +    type Target = Chip<T>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.chip
> +    }
> +}
> +
> +/// 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.
> +struct Registration<T: PwmOps> {
> +    chip: ARef<Chip<T>>,
> +}
> +
>  impl<T: PwmOps> Drop for Registration<T> {
>      fn drop(&mut self) {
>          let chip_raw = self.chip.as_raw();
> 
> ---
> base-commit: fae00ea9f00367771003ace78f29549dead58fc7
> change-id: 20251127-pwm_safe_register-e49b0a261101
> 
> 

Thank you for your work. I've tested this patch successfully.
Other than the typo this looks perfect.

Tested-by: Michal Wilczynski <m.wilczynski@samsung.com>
Acked-by: Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[Uwe Kleine-König]

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

On Sun, Dec 07, 2025 at 11:16:59PM +0100, Michal Wilczynski wrote:
> On 12/2/25 19:17, Markus Probst wrote:
> > The `pwm::Registration::register` function provides no guarantee that the
> > function isn't called twice with the same pwm chip, which is considered
> > unsafe.
> > 
> > Add `pwm::UnregisteredChip` as wrapper around `pwm::Chip`.
> > Implement `pwm::UnregisteredChip::register` for the registration. This
> > function takes ownership of `pwm::UnregisteredChip` and therefore
> > guarantees that the registration can't be called twice on the same pwm
> > chip.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> > This patch provides the additional guarantee that the pwm chip doesn't
> > get registered twice.
> > 
> > The following changes were made:
> > - change the visibility of `pwm::Registration` to private
> > - remove the `pwm::Registration::register` function
> > - add `pwm::UnregisteredChip` - a wrapper around `pwm::Chip`
> > - return `pwm::UnregisteredChip` in `pwm::Chip::new`
> > - add `pwm::UnregisteredChip::register` for registering the pwm chip
> >   once
> > - add Send + Sync bounds to `PwmOps`
> > 
> > Note that I wasn't able to test this patch, due to the lack of hardware.
> > 
> > Also I was not able to successfully compile drivers/pwm/pwm_th1520.rs,
> > as `clk::Clk` seems to be missing. I haven't found the missing patch
> > in linux-next nor in
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/log/?h=pwm/for-next
> > 
> > (the tree in which the th1520 pwm driver was merged).
> > So please verify yourself that the driver compiles and throws no errors.

What does "not able to successfully compile drivers/pwm/pwm_th1520.rs"
mean? You were unable to find a .config that included CONFIG_PWM_TH1520,
or you got a compiler error? If it's the latter, ...

> I suspect your kernel configuration is missing CONFIG_COMMON_CLK.

... are we missing a dependency for the driver?

> Thank you for your work. I've tested this patch successfully.
> Other than the typo this looks perfect.
> 
> Tested-by: Michal Wilczynski <m.wilczynski@samsung.com>
> Acked-by: Michal Wilczynski <m.wilczynski@samsung.com>

I applied the the patch to

	https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-nexxt

as 6.20 material with Michal's tags and the typo fixed.

Thanks
Uwe

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

Re: [PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[Markus Probst]

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

On Tue, 2025-12-09 at 09:08 +0100, Uwe Kleine-König wrote:
> On Sun, Dec 07, 2025 at 11:16:59PM +0100, Michal Wilczynski wrote:
> > On 12/2/25 19:17, Markus Probst wrote:
> > > The `pwm::Registration::register` function provides no guarantee that the
> > > function isn't called twice with the same pwm chip, which is considered
> > > unsafe.
> > > 
> > > Add `pwm::UnregisteredChip` as wrapper around `pwm::Chip`.
> > > Implement `pwm::UnregisteredChip::register` for the registration. This
> > > function takes ownership of `pwm::UnregisteredChip` and therefore
> > > guarantees that the registration can't be called twice on the same pwm
> > > chip.
> > > 
> > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > > ---
> > > This patch provides the additional guarantee that the pwm chip doesn't
> > > get registered twice.
> > > 
> > > The following changes were made:
> > > - change the visibility of `pwm::Registration` to private
> > > - remove the `pwm::Registration::register` function
> > > - add `pwm::UnregisteredChip` - a wrapper around `pwm::Chip`
> > > - return `pwm::UnregisteredChip` in `pwm::Chip::new`
> > > - add `pwm::UnregisteredChip::register` for registering the pwm chip
> > >   once
> > > - add Send + Sync bounds to `PwmOps`
> > > 
> > > Note that I wasn't able to test this patch, due to the lack of hardware.
> > > 
> > > Also I was not able to successfully compile drivers/pwm/pwm_th1520.rs,
> > > as `clk::Clk` seems to be missing. I haven't found the missing patch
> > > in linux-next nor in
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/log/?h=pwm/for-next
> > > 
> > > (the tree in which the th1520 pwm driver was merged).
> > > So please verify yourself that the driver compiles and throws no errors.
> 
> What does "not able to successfully compile drivers/pwm/pwm_th1520.rs"
> mean? You were unable to find a .config that included CONFIG_PWM_TH1520,
> or you got a compiler error? If it's the latter, ...
> 
> > I suspect your kernel configuration is missing CONFIG_COMMON_CLK.
> 
> ... are we missing a dependency for the driver?
I got a compiler error. Enabling CONFIG_COMMON_CLK indeed fixes it.

Without it:
  CLIPPY     drivers/pwm/pwm_th1520.o
error[E0432]: unresolved import `kernel::clk::Clk`
  --> drivers/pwm/pwm_th1520.rs:26:5
   |
26 |     clk::Clk,
   |     ^^^^^^^^ no `Clk` in `clk`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0432`.
make[4]: *** [scripts/Makefile.build:354: drivers/pwm/pwm_th1520.o]
Error 1
make[3]: *** [scripts/Makefile.build:556: drivers/pwm] Error 2
make[2]: *** [scripts/Makefile.build:556: drivers] Error 2
make[1]: *** [/home/markustieger/build/synology/linux-
upstream/Makefile:2010: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2

A simple fix would be to add "depends on COMMON_CLK" in
drivers/pwm/Kconfig for PWM_TH1520.

Thanks
- Markus Probst

> 
> > Thank you for your work. I've tested this patch successfully.
> > Other than the typo this looks perfect.
> > 
> > Tested-by: Michal Wilczynski <m.wilczynski@samsung.com>
> > Acked-by: Michal Wilczynski <m.wilczynski@samsung.com>
> 
> I applied the the patch to
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-nexxt
> 
> as 6.20 material with Michal's tags and the typo fixed.
> 
> Thanks
> Uwe

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[Michal Wilczynski]

On 12/9/25 13:26, Markus Probst wrote:
> On Tue, 2025-12-09 at 09:08 +0100, Uwe Kleine-König wrote:
>> On Sun, Dec 07, 2025 at 11:16:59PM +0100, Michal Wilczynski wrote:
>>> On 12/2/25 19:17, Markus Probst wrote:
>> What does "not able to successfully compile drivers/pwm/pwm_th1520.rs"
>> mean? You were unable to find a .config that included CONFIG_PWM_TH1520,
>> or you got a compiler error? If it's the latter, ...
>>
>>> I suspect your kernel configuration is missing CONFIG_COMMON_CLK.
>>
>> ... are we missing a dependency for the driver?
> I got a compiler error. Enabling CONFIG_COMMON_CLK indeed fixes it.
> 
> Without it:
>   CLIPPY     drivers/pwm/pwm_th1520.o
> error[E0432]: unresolved import `kernel::clk::Clk`
>   --> drivers/pwm/pwm_th1520.rs:26:5
>    |
> 26 |     clk::Clk,
>    |     ^^^^^^^^ no `Clk` in `clk`
> 
> error: aborting due to 1 previous error
> 
> For more information about this error, try `rustc --explain E0432`.
> make[4]: *** [scripts/Makefile.build:354: drivers/pwm/pwm_th1520.o]
> Error 1
> make[3]: *** [scripts/Makefile.build:556: drivers/pwm] Error 2
> make[2]: *** [scripts/Makefile.build:556: drivers] Error 2
> make[1]: *** [/home/markustieger/build/synology/linux-
> upstream/Makefile:2010: .] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
> 
> A simple fix would be to add "depends on COMMON_CLK" in
> drivers/pwm/Kconfig for PWM_TH1520.
	
While at it I think we should also add dependency on ARCH_THEAD |
COMPILE_TEST as well as HAS_IOMEM in my opinion. I think some of those
dependencies were lost when we were iterating on the patchset for some
reason, can't recall when exactly.

If you plan to send a patch include those as well. Otherwise let me know
and I send a patch.

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

Re: [PATCH v2] rust: pwm: Add UnregisteredChip wrapper around Chip

[Markus Probst]

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

On Tue, 2025-12-09 at 14:41 +0100, Michal Wilczynski wrote:
> 
> On 12/9/25 13:26, Markus Probst wrote:
> > On Tue, 2025-12-09 at 09:08 +0100, Uwe Kleine-König wrote:
> > > On Sun, Dec 07, 2025 at 11:16:59PM +0100, Michal Wilczynski wrote:
> > > > On 12/2/25 19:17, Markus Probst wrote:
> > > What does "not able to successfully compile drivers/pwm/pwm_th1520.rs"
> > > mean? You were unable to find a .config that included CONFIG_PWM_TH1520,
> > > or you got a compiler error? If it's the latter, ...
> > > 
> > > > I suspect your kernel configuration is missing CONFIG_COMMON_CLK.
> > > 
> > > ... are we missing a dependency for the driver?
> > I got a compiler error. Enabling CONFIG_COMMON_CLK indeed fixes it.
> > 
> > Without it:
> >   CLIPPY     drivers/pwm/pwm_th1520.o
> > error[E0432]: unresolved import `kernel::clk::Clk`
> >   --> drivers/pwm/pwm_th1520.rs:26:5
> >    |
> > 26 |     clk::Clk,
> >    |     ^^^^^^^^ no `Clk` in `clk`
> > 
> > error: aborting due to 1 previous error
> > 
> > For more information about this error, try `rustc --explain E0432`.
> > make[4]: *** [scripts/Makefile.build:354: drivers/pwm/pwm_th1520.o]
> > Error 1
> > make[3]: *** [scripts/Makefile.build:556: drivers/pwm] Error 2
> > make[2]: *** [scripts/Makefile.build:556: drivers] Error 2
> > make[1]: *** [/home/markustieger/build/synology/linux-
> > upstream/Makefile:2010: .] Error 2
> > make: *** [Makefile:248: __sub-make] Error 2
> > 
> > A simple fix would be to add "depends on COMMON_CLK" in
> > drivers/pwm/Kconfig for PWM_TH1520.
> 	
> While at it I think we should also add dependency on ARCH_THEAD |
> COMPILE_TEST as well as HAS_IOMEM in my opinion. I think some of those
> dependencies were lost when we were iterating on the patchset for some
> reason, can't recall when exactly.
> 
> If you plan to send a patch include those as well. Otherwise let me know
> and I send a patch.
No, I am currently busy writing the serial device bus rust abstractions
(include/linux/serdev.h) for a driver.

Thanks
- Markus Probst

> 
> > 
> > Thanks
> > - Markus Probst
> Best regards,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]