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

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

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

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

-- 
2.34.1

Hello,

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

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

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

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

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

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

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

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

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

I don't spot the get_state implementation?!

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

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

> +    ) -> Result {
> +        let data: &Th1520PwmChipData = pwm_chip_ref.get_drvdata().ok_or(EINVAL)?;
> +        let hwpwm = pwm_dev.hwpwm();
> +
> +        if !target_state.enabled() {
> +            if pwm_dev.state().enabled() {
> +                data._disable(hwpwm)?;
> +            }
> +
> +            return Ok(());
> +        }
> +
> +        // Configure period, duty, and polarity.
> +        // This function also latches period/duty with CFG_UPDATE.
> +        // It returns the control value that was written with CFG_UPDATE set.
> +        let ctrl_val_after_config = data._config(
> +            hwpwm,
> +            target_state.duty_cycle(),
> +            target_state.period(),
> +            target_state.polarity(),
> +        )?;
> +
> +        // Enable by setting START bit if it wasn't enabled before this apply call
> +        if !pwm_dev.state().enabled() {
> +            data._enable(hwpwm, ctrl_val_after_config)?;
> +        }
> +
> +        Ok(())
> +    }
> +}
> +
> +impl Drop for Th1520PwmChipData {
> +    fn drop(&mut self) {
> +        self.clk.disable_unprepare();
> +    }
> +}
> +
> +fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
> +    if c == 0 {
> +        return 0;
> +    }
> +    a.wrapping_mul(b) / c
> +}

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

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

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

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

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

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

Best regards
Uwe

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

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

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

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

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

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

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

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

> 
>> +    ) -> Result {
>> +        let data: &Th1520PwmChipData = pwm_chip_ref.get_drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm_dev.hwpwm();
>> +
>> +        if !target_state.enabled() {
>> +            if pwm_dev.state().enabled() {
>> +                data._disable(hwpwm)?;
>> +            }
>> +
>> +            return Ok(());
>> +        }
>> +
>> +        // Configure period, duty, and polarity.
>> +        // This function also latches period/duty with CFG_UPDATE.
>> +        // It returns the control value that was written with CFG_UPDATE set.
>> +        let ctrl_val_after_config = data._config(
>> +            hwpwm,
>> +            target_state.duty_cycle(),
>> +            target_state.period(),
>> +            target_state.polarity(),
>> +        )?;
>> +
>> +        // Enable by setting START bit if it wasn't enabled before this apply call
>> +        if !pwm_dev.state().enabled() {
>> +            data._enable(hwpwm, ctrl_val_after_config)?;
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl Drop for Th1520PwmChipData {
>> +    fn drop(&mut self) {
>> +        self.clk.disable_unprepare();
>> +    }
>> +}
>> +
>> +fn mul_div_u64(a: u64, b: u64, c: u64) -> u64 {
>> +    if c == 0 {
>> +        return 0;
>> +    }
>> +    a.wrapping_mul(b) / c
>> +}
> 
> Is this save if a * b > U64_MAX? I would have expected such a function
> to already exist in generic code.

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

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

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

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

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

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

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

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

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

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

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

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

---
Cheers,
Benno

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

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

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

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

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

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

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

I hope that helps & thanks!

Cheers,
Miguel

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

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

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

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

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

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

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

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

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

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

> +impl pwm::PwmOps for Th1520PwmChipData {
> +    // This driver implements get_state
> +    fn apply(
> +        pwm_chip_ref: &mut pwm::Chip,
> +        pwm_dev: &mut pwm::Device,
> +        target_state: &pwm::State,
> +    ) -> Result {

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

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

This would allow you to access iomem more efficiently.

Instead of

	data.iomem.try_access()

you could do

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

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

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

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

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

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

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

Thanks ! Appreciate your review !
Michał

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

Hello,

On Tue, May 27, 2025 at 02:44:57PM +0200, Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics wrote:
> W dniu 25.05.2025 o 14:03, Danilo Krummrich pisze:
> > On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote:
> >> +impl pwm::PwmOps for Th1520PwmChipData {
> >> +    // This driver implements get_state
> >> +    fn apply(
> >> +        pwm_chip_ref: &mut pwm::Chip,
> >> +        pwm_dev: &mut pwm::Device,
> >> +        target_state: &pwm::State,
> >> +    ) -> Result {
> > I assume those callbacks can't race with pwmchip_remove() called from driver
> > remove()? I.e. the callbacks are guaranteed to complete before pwmchip_remove()
> > completes?
> 
> Yeah this is my understanding as well - this is something that the PWM 
> core should
> guarantee. Fairly recently there was a commit adding even more locking
> 1cc2e1faafb3 ("pwm: Add more locking")

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

Best regards
Uwe

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

There are two sizes:

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

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

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

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

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

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

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

I hope this helps.

- Danilo