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

[Markus Probst <markus.probst@posteo.de>]

[-- 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 --]