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

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

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

The driver's core logic is written entirely in safe Rust, with no unsafe
blocks.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21319,6 +21319,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:	drivers/reset/reset-th1520.c
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 F:	include/dt-bindings/power/thead,th1520-power.h
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -719,6 +719,16 @@ 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
+	  This option enables the driver for the PWM controller found on the
+	  T-HEAD TH1520 SoC. This driver is written in Rust.
+
+	  To compile this driver as a module, choose M here; the module
+	  will be called pwm-th1520. If you are unsure, say N.
+
 config PWM_TIECAP
 	tristate "ECAP PWM support"
 	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 96160f4257fcb0e0951581af0090615c0edf5260..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -73,3 +73,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..9e43474f5123b51c49035d71219303a606c20a5a
--- /dev/null
+++ b/drivers/pwm/pwm_th1520.rs
@@ -0,0 +1,287 @@
+// 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 core::ops::Deref;
+use kernel::{
+    c_str,
+    clk::Clk,
+    device::{Bound, Core, Device},
+    devres,
+    error::{code::*, Result},
+    io::mem::IoMem,
+    math::KernelMathExt,
+    of, platform,
+    prelude::*,
+    pwm, time,
+};
+
+const MAX_PWM_NUM: u32 = 6;
+
+// Register offsets
+const fn th1520_pwm_chn_base(n: u32) -> usize {
+    (n * 0x20) as usize
+}
+const fn th1520_pwm_ctrl(n: u32) -> usize {
+    th1520_pwm_chn_base(n)
+}
+const fn th1520_pwm_per(n: u32) -> usize {
+    th1520_pwm_chn_base(n) + 0x08
+}
+const fn th1520_pwm_fp(n: u32) -> usize {
+    th1520_pwm_chn_base(n) + 0x0c
+}
+
+// Control register bits
+const 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 TH1520_PWM_REG_SIZE: usize = 0xB0;
+
+/// Hardware-specific waveform representation for TH1520.
+#[derive(Copy, Clone, Debug, Default)]
+struct Th1520WfHw {
+    period_cycles: u32,
+    duty_cycles: u32,
+    ctrl_val: u32,
+    enabled: bool,
+}
+
+/// The driver's private data struct. It holds all necessary devres-managed resources.
+struct Th1520PwmDriverData {
+    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
+    clk: Clk,
+}
+
+impl pwm::PwmOps for Th1520PwmDriverData {
+    type WfHw = Th1520WfHw;
+
+    fn get_state(
+        chip: &mut pwm::Chip,
+        pwm: &mut pwm::Device,
+        state: &mut pwm::State,
+        parent_dev: &Device<Bound>,
+    ) -> Result {
+        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
+        let hwpwm = pwm.hwpwm();
+        let iomem_guard = data.iomem.access(parent_dev)?;
+        let iomap = iomem_guard.deref();
+        let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
+        let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
+        let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
+
+        state.set_enabled(duty_cycles != 0);
+
+        let rate_hz = data.clk.rate().as_hz();
+        let period_ns = (period_cycles as u64)
+            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
+            .unwrap_or(0);
+        state.set_period(period_ns);
+
+        let duty_ns = (duty_cycles as u64)
+            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
+            .unwrap_or(0);
+        state.set_duty_cycle(duty_ns);
+
+        if (ctrl & PWM_FPOUT) != 0 {
+            state.set_polarity(pwm::Polarity::Normal);
+        } else {
+            state.set_polarity(pwm::Polarity::Inversed);
+        }
+
+        Ok(())
+    }
+
+    fn round_waveform_tohw(
+        chip: &mut pwm::Chip,
+        pwm: &mut pwm::Device,
+        wf: &pwm::Waveform,
+    ) -> Result<(i32, Self::WfHw)> {
+        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
+        let hwpwm = pwm.hwpwm();
+
+        if wf.duty_offset_ns != 0 {
+            dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
+            return Err(ENOTSUPP);
+        }
+
+        if wf.period_length_ns == 0 {
+            return Ok((
+                0,
+                Th1520WfHw {
+                    enabled: false,
+                    ..Default::default()
+                },
+            ));
+        }
+
+        let rate_hz = data.clk.rate().as_hz();
+
+        let period_cycles = wf
+            .period_length_ns
+            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
+            .ok_or(EINVAL)?;
+        if period_cycles > u32::MAX as u64 {
+            dev_err!(
+                chip.device(),
+                "PWM-{}: Calculated period {} cycles is out of range\n",
+                hwpwm,
+                period_cycles
+            );
+            return Err(EINVAL);
+        }
+
+        let duty_cycles = wf
+            .duty_length_ns
+            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
+            .ok_or(EINVAL)?;
+        if duty_cycles > period_cycles {
+            dev_err!(
+                chip.device(),
+                "PWM-{}: Duty {}ns > period {}ns\n",
+                hwpwm,
+                wf.duty_length_ns,
+                wf.period_length_ns
+            );
+            return Err(EINVAL);
+        }
+
+        let mut ctrl_val = PWM_CONTINUOUS_MODE;
+        if pwm.state().polarity() == pwm::Polarity::Normal {
+            ctrl_val |= PWM_FPOUT;
+        }
+
+        let wfhw = Th1520WfHw {
+            period_cycles: period_cycles as u32,
+            duty_cycles: duty_cycles as u32,
+            ctrl_val,
+            enabled: true,
+        };
+
+        dev_dbg!(
+            chip.device(),
+            "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
+            wfhw.period_cycles,
+            wfhw.duty_cycles,
+            wfhw.ctrl_val
+        );
+        Ok((0, wfhw))
+    }
+
+    fn write_waveform(
+        chip: &mut pwm::Chip,
+        pwm: &mut pwm::Device,
+        wfhw: &Self::WfHw,
+        parent_dev: &Device<Bound>,
+    ) -> Result {
+        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
+        let hwpwm = pwm.hwpwm();
+        let iomem_guard = data.iomem.access(parent_dev)?;
+        let iomap = iomem_guard.deref();
+        let was_enabled = pwm.state().enabled();
+
+        if !wfhw.enabled {
+            if was_enabled {
+                let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
+
+                ctrl &= !PWM_CFG_UPDATE;
+
+                iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
+                iomap.write32(0, th1520_pwm_fp(hwpwm));
+                iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
+            }
+            return Ok(());
+        }
+
+        let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
+
+        iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
+        iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
+        iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
+        iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
+
+        if !was_enabled {
+            iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
+        }
+
+        Ok(())
+    }
+}
+
+impl Drop for Th1520PwmDriverData {
+    fn drop(&mut self) {
+        self.clk.disable_unprepare();
+    }
+}
+
+static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
+
+struct Th1520PwmPlatformDriver {
+    _registration: pwm::Registration,
+}
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
+    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
+);
+
+impl platform::Driver for Th1520PwmPlatformDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(
+        pdev: &platform::Device<Core>,
+        _id_info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
+        let dev = pdev.as_ref();
+        let resource = pdev.resource(0).ok_or(ENODEV)?;
+        let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
+        let clk = Clk::get(pdev.as_ref(), None)?;
+
+        clk.prepare_enable()?;
+
+        let rate_hz = clk.rate().as_hz();
+        if rate_hz == 0 {
+            dev_err!(dev, "Clock rate is zero\n");
+            return Err(EINVAL);
+        }
+
+        if rate_hz > time::NSEC_PER_SEC as usize {
+            dev_err!(
+                dev,
+                "Clock rate {} Hz is too high, not supported.\n",
+                rate_hz
+            );
+            return Err(ERANGE);
+        }
+
+        let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
+
+        let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
+        chip.set_drvdata(drvdata);
+
+        let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
+
+        Ok(KBox::new(
+            Th1520PwmPlatformDriver {
+                _registration: registration,
+            },
+            GFP_KERNEL,
+        )?
+        .into())
+    }
+}
+
+kernel::module_platform_driver! {
+    type: Th1520PwmPlatformDriver,
+    name: "pwm-th1520",
+    author: "Michal Wilczynski <m.wilczynski@samsung.com>",
+    description: "T-HEAD TH1520 PWM driver",
+    license: "GPL v2",
+}

-- 
2.34.1

Hello,

On Tue, Jun 10, 2025 at 02:52:50PM +0200, Michal Wilczynski wrote:
> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
> utilizing the safe PWM abstractions from the preceding commit.
> 
> The driver implements the pwm::PwmOps trait using the modern waveform
> API (round_waveform_tohw, write_waveform, etc.) to support configuration
> of period, duty cycle, and polarity for the TH1520's PWM channels.
> 
> Resource management is handled using idiomatic Rust patterns. The PWM
> chip object is allocated via pwm::Chip::new and its registration with
> the PWM core is managed by the pwm::Registration RAII guard. This
> ensures pwmchip_remove is always called when the driver unbinds,
> preventing resource leaks. Device managed resources are used for the
> MMIO region, and the clock lifecycle is correctly managed in the
> driver's private data Drop implementation.
> 
> The driver's core logic is written entirely in safe Rust, with no unsafe
> blocks.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  10 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 299 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21319,6 +21319,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:	drivers/reset/reset-th1520.c
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>  F:	include/dt-bindings/power/thead,th1520-power.h
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -719,6 +719,16 @@ config PWM_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tegra.
>  
> +config PWM_TH1520_RUST

Is "_RUST" relevant here? I'd drop that.

> +	tristate "TH1520 PWM support (Rust)"

Also while having drivers is rust is a great step forward, it's not
relevant to the user selecting support for the TH1520 device.

> +	depends on RUST_PWM_ABSTRACTIONS
> +	help
> +	  This option enables the driver for the PWM controller found on the
> +	  T-HEAD TH1520 SoC. This driver is written in Rust.
> +
> +	  To compile this driver as a module, choose M here; the module
> +	  will be called pwm-th1520. If you are unsure, say N.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 96160f4257fcb0e0951581af0090615c0edf5260..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -73,3 +73,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

Alphabetic ordering please

> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
> --- /dev/null
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -0,0 +1,287 @@
> +// 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

A short paragraph describing the hardware limitations of that driver
here would be nice. While you probably cannot stick to the exact format
used in newer C drivers such that

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c

emits the info for your driver, I'd appreciate you sticking to mostly
this format.

> +use core::ops::Deref;
> +use kernel::{
> +    c_str,
> +    clk::Clk,
> +    device::{Bound, Core, Device},
> +    devres,
> +    error::{code::*, Result},
> +    io::mem::IoMem,
> +    math::KernelMathExt,
> +    of, platform,
> +    prelude::*,
> +    pwm, time,
> +};
> +
> +const MAX_PWM_NUM: u32 = 6;
> +
> +// Register offsets
> +const fn th1520_pwm_chn_base(n: u32) -> usize {
> +    (n * 0x20) as usize
> +}

empty line here between these functions?

> +const fn th1520_pwm_ctrl(n: u32) -> usize {
> +    th1520_pwm_chn_base(n)
> +}
> +const fn th1520_pwm_per(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x08
> +}
> +const fn th1520_pwm_fp(n: u32) -> usize {
> +    th1520_pwm_chn_base(n) + 0x0c
> +}
> +
> +// Control register bits
> +const 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 TH1520_PWM_REG_SIZE: usize = 0xB0;
> +
> +/// Hardware-specific waveform representation for TH1520.

Some comments use 2 and other 3 slashes. Does this have any semantic?

> +#[derive(Copy, Clone, Debug, Default)]
> +struct Th1520WfHw {
> +    period_cycles: u32,
> +    duty_cycles: u32,
> +    ctrl_val: u32,
> +    enabled: bool,
> +}
> +
> +/// The driver's private data struct. It holds all necessary devres-managed resources.
> +struct Th1520PwmDriverData {
> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> +    clk: Clk,
> +}
> +
> +impl pwm::PwmOps for Th1520PwmDriverData {
> +    type WfHw = Th1520WfHw;
> +
> +    fn get_state(
> +        chip: &mut pwm::Chip,
> +        pwm: &mut pwm::Device,
> +        state: &mut pwm::State,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result {

Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
.round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
.write_waveform() or .apply() + .get_state(), but don't mix these.

> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_guard = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_guard.deref();
> +        let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
> +        let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
> +        let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
> +
> +        state.set_enabled(duty_cycles != 0);
> +
> +        let rate_hz = data.clk.rate().as_hz();
> +        let period_ns = (period_cycles as u64)
> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
> +            .unwrap_or(0);

What does .unwrap_or(0) do? You need to round up in this mul_div
operation.

> +        state.set_period(period_ns);
> +
> +        let duty_ns = (duty_cycles as u64)
> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
> +            .unwrap_or(0);
> +        state.set_duty_cycle(duty_ns);
> +
> +        if (ctrl & PWM_FPOUT) != 0 {
> +            state.set_polarity(pwm::Polarity::Normal);
> +        } else {
> +            state.set_polarity(pwm::Polarity::Inversed);
> +        }
> +
> +        Ok(())
> +    }
> +
> +    fn round_waveform_tohw(
> +        chip: &mut pwm::Chip,
> +        pwm: &mut pwm::Device,
> +        wf: &pwm::Waveform,
> +    ) -> Result<(i32, Self::WfHw)> {
> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> +        let hwpwm = pwm.hwpwm();
> +
> +        if wf.duty_offset_ns != 0 {
> +            dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);

That's wrong, pick the biggest offset value that is possible to
implement and not bigger than the requested value.
Your hardware can do inversed polarity, so offset is either 0 or
period-duty.

> +            return Err(ENOTSUPP);
> +        }
> +
> +        if wf.period_length_ns == 0 {
> +            return Ok((
> +                0,
> +                Th1520WfHw {
> +                    enabled: false,
> +                    ..Default::default()
> +                },
> +            ));
> +        }
> +
> +        let rate_hz = data.clk.rate().as_hz();
> +
> +        let period_cycles = wf
> +            .period_length_ns
> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
> +            .ok_or(EINVAL)?;

If period_length_ns is BIG, pick the biggest possible period_cycles
value, not EINVAL.

> +        if period_cycles > u32::MAX as u64 {
> +            dev_err!(
> +                chip.device(),
> +                "PWM-{}: Calculated period {} cycles is out of range\n",
> +                hwpwm,
> +                period_cycles
> +            );
> +            return Err(EINVAL);
> +        }

ditto.

> +        let duty_cycles = wf
> +            .duty_length_ns
> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
> +            .ok_or(EINVAL)?;
> +        if duty_cycles > period_cycles {

You can assume this won't happen.

> +            dev_err!(
> +                chip.device(),
> +                "PWM-{}: Duty {}ns > period {}ns\n",
> +                hwpwm,
> +                wf.duty_length_ns,
> +                wf.period_length_ns
> +            );
> +            return Err(EINVAL);
> +        }
> +
> +        let mut ctrl_val = PWM_CONTINUOUS_MODE;
> +        if pwm.state().polarity() == pwm::Polarity::Normal {
> +            ctrl_val |= PWM_FPOUT;

What is pwm.state()? If that's similar to pwm->state in C this is
irrelevant here. It describes the current state, not the new request.

> +        }
> +
> +        let wfhw = Th1520WfHw {
> +            period_cycles: period_cycles as u32,
> +            duty_cycles: duty_cycles as u32,
> +            ctrl_val,
> +            enabled: true,
> +        };
> +
> +        dev_dbg!(
> +            chip.device(),
> +            "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
> +            wfhw.period_cycles,
> +            wfhw.duty_cycles,
> +            wfhw.ctrl_val
> +        );

This would be much more helpful if it also contained the values from wf.

> +        Ok((0, wfhw))
> +    }
> +
> +    fn write_waveform(
> +        chip: &mut pwm::Chip,
> +        pwm: &mut pwm::Device,
> +        wfhw: &Self::WfHw,
> +        parent_dev: &Device<Bound>,
> +    ) -> Result {
> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
> +        let hwpwm = pwm.hwpwm();
> +        let iomem_guard = data.iomem.access(parent_dev)?;
> +        let iomap = iomem_guard.deref();
> +        let was_enabled = pwm.state().enabled();
> +
> +        if !wfhw.enabled {
> +            if was_enabled {
> +                let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));

Do you need that read? Isn't is clear what the value is?

> +                ctrl &= !PWM_CFG_UPDATE;
> +
> +                iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
> +                iomap.write32(0, th1520_pwm_fp(hwpwm));
> +                iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
> +            }
> +            return Ok(());
> +        }
> +
> +        let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;

wfhw.ctrl_val never has PWM_CFG_UPDATE set.

> +        iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
> +        iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
> +        iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
> +        iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
> +
> +        if !was_enabled {
> +            iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));

Can this be combined with the above write?

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

We don't have clk_rate_get_exclusive() yet, right? Then please add a
comment here that this needs to be added here when it became available.

> +
> +        let rate_hz = clk.rate().as_hz();
> +        if rate_hz == 0 {
> +            dev_err!(dev, "Clock rate is zero\n");
> +            return Err(EINVAL);
> +        }
> +
> +        if rate_hz > time::NSEC_PER_SEC as usize {
> +            dev_err!(
> +                dev,
> +                "Clock rate {} Hz is too high, not supported.\n",
> +                rate_hz
> +            );
> +            return Err(ERANGE);
> +        }
> +
> +        let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
> +
> +        let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
> +        chip.set_drvdata(drvdata);
> +
> +        let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
> +
> +        Ok(KBox::new(
> +            Th1520PwmPlatformDriver {
> +                _registration: registration,
> +            },
> +            GFP_KERNEL,
> +        )?
> +        .into())
> +    }
> +}
> +
> +kernel::module_platform_driver! {
> +    type: Th1520PwmPlatformDriver,
> +    name: "pwm-th1520",
> +    author: "Michal Wilczynski <m.wilczynski@samsung.com>",
> +    description: "T-HEAD TH1520 PWM driver",
> +    license: "GPL v2",
> +}

Best regards
Uwe

On 6/11/25 08:58, Uwe Kleine-König wrote:
> Hello,
> 
> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> .write_waveform() or .apply() + .get_state(), but don't mix these.

In the process of implementing the full "newstyle" waveform API as you
suggested, I discovered a hardware limitation. After writing new values
to the period and duty cycle registers, reading them back does not
return the programmed values, which makes it impossible to reliably
report the current hardware state.

This appears to be a known quirk of the hardware, as the reference C
driver from T-HEAD [1] also omits the .get_state callback, likely for
the same reason.

Given this, would it be acceptable to provide a write-only driver? My
proposed solution would be to omit the .read_waveform() and
.round_waveform_fromhw() implementations from my PwmOps trait. This
would mean the driver can correctly set the PWM state, but attempting to
read it back via sysfs would fail (e.g., with -EOPNOTSUPP), reflecting
the hardware's capability.

Does this sound like a reasonable approach to you?

[1] - https://github.com/revyos/thead-kernel/blob/lpi4a/drivers/pwm/pwm-light.c

> 
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_guard = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_guard.deref();
>> +        let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>> +        let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
>> +        let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
>> +
>> +        state.set_enabled(duty_cycles != 0);
>> +
>> +        let rate_hz = data.clk.rate().as_hz();
>> +        let period_ns = (period_cycles as u64)
>> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> +            .unwrap_or(0);

> Best regards
> Uwe

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

Hello Michael,

On Thu, Jun 12, 2025 at 10:14:13AM +0200, Michal Wilczynski wrote:
> On 6/11/25 08:58, Uwe Kleine-König wrote:
> > Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> > .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> > .write_waveform() or .apply() + .get_state(), but don't mix these.
> 
> In the process of implementing the full "newstyle" waveform API as you
> suggested, I discovered a hardware limitation. After writing new values
> to the period and duty cycle registers, reading them back does not
> return the programmed values, which makes it impossible to reliably
> report the current hardware state.
> 
> This appears to be a known quirk of the hardware, as the reference C
> driver from T-HEAD [1] also omits the .get_state callback, likely for
> the same reason.

Do you read complete non-sense or e.g. the old configuration until
the current period ends?

I guess would be that .get_state wasn't implemented because this is an
oldoldstyle driver and it works also without that function.

> Given this, would it be acceptable to provide a write-only driver? My
> proposed solution would be to omit the .read_waveform() and
> .round_waveform_fromhw() implementations from my PwmOps trait. This

Please don't skip .round_waveform_fromhw(), that one is needed for
pwm_round_waveform_might_sleep().

I don't like it, but given that the hardware doesn't play along there is
no alternative.

> would mean the driver can correctly set the PWM state, but attempting to
> read it back via sysfs would fail (e.g., with -EOPNOTSUPP), reflecting
> the hardware's capability.

I think there might be another patch opportunity then to make PWM_DEBUG
work with that.

Best regards
Uwe

On 6/12/25 22:36, Uwe Kleine-König wrote:
> Hello Michael,
> 
> On Thu, Jun 12, 2025 at 10:14:13AM +0200, Michal Wilczynski wrote:
>> On 6/11/25 08:58, Uwe Kleine-König wrote:
>>> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
>>> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
>>> .write_waveform() or .apply() + .get_state(), but don't mix these.
>>
>> In the process of implementing the full "newstyle" waveform API as you
>> suggested, I discovered a hardware limitation. After writing new values
>> to the period and duty cycle registers, reading them back does not
>> return the programmed values, which makes it impossible to reliably
>> report the current hardware state.
>>
>> This appears to be a known quirk of the hardware, as the reference C
>> driver from T-HEAD [1] also omits the .get_state callback, likely for
>> the same reason.
> 
> Do you read complete non-sense or e.g. the old configuration until
> the current period ends?
> 
> I guess would be that .get_state wasn't implemented because this is an
> oldoldstyle driver and it works also without that function.

Hi Uwe,

My apologies for the confusion. After further testing, it appears I was
mistaken, and the hardware reads are working correctly. I must have made
an error when testing via sysfs.

I'll be submitting a v3 that implements the full round_waveform_tohw(),
round_waveform_fromhw(), read_waveform(), and write_waveform()
combination as you initially suggested.

> 
>> Given this, would it be acceptable to provide a write-only driver? My
>> proposed solution would be to omit the .read_waveform() and
>> .round_waveform_fromhw() implementations from my PwmOps trait. This
> 
> Please don't skip .round_waveform_fromhw(), that one is needed for
> pwm_round_waveform_might_sleep().
> 
> I don't like it, but given that the hardware doesn't play along there is
> no alternative.
> 
>> would mean the driver can correctly set the PWM state, but attempting to
>> read it back via sysfs would fail (e.g., with -EOPNOTSUPP), reflecting
>> the hardware's capability.
> 
> I think there might be another patch opportunity then to make PWM_DEBUG
> work with that.
> 
> Best regards
> Uwe

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

On 6/11/25 08:58, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jun 10, 2025 at 02:52:50PM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
>> utilizing the safe PWM abstractions from the preceding commit.
>>
>> The driver implements the pwm::PwmOps trait using the modern waveform
>> API (round_waveform_tohw, write_waveform, etc.) to support configuration
>> of period, duty cycle, and polarity for the TH1520's PWM channels.
>>
>> Resource management is handled using idiomatic Rust patterns. The PWM
>> chip object is allocated via pwm::Chip::new and its registration with
>> the PWM core is managed by the pwm::Registration RAII guard. This
>> ensures pwmchip_remove is always called when the driver unbinds,
>> preventing resource leaks. Device managed resources are used for the
>> MMIO region, and the clock lifecycle is correctly managed in the
>> driver's private data Drop implementation.
>>
>> The driver's core logic is written entirely in safe Rust, with no unsafe
>> blocks.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  MAINTAINERS               |   1 +
>>  drivers/pwm/Kconfig       |  10 ++
>>  drivers/pwm/Makefile      |   1 +
>>  drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 299 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21319,6 +21319,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:	drivers/reset/reset-th1520.c
>>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>>  F:	include/dt-bindings/power/thead,th1520-power.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -719,6 +719,16 @@ config PWM_TEGRA
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-tegra.
>>  
>> +config PWM_TH1520_RUST
> 
> Is "_RUST" relevant here? I'd drop that.

Yeah I think it's a good idea to drop.

> 
>> +	tristate "TH1520 PWM support (Rust)"
> 
> Also while having drivers is rust is a great step forward, it's not
> relevant to the user selecting support for the TH1520 device.
> 
>> +	depends on RUST_PWM_ABSTRACTIONS
>> +	help
>> +	  This option enables the driver for the PWM controller found on the
>> +	  T-HEAD TH1520 SoC. This driver is written in Rust.
>> +
>> +	  To compile this driver as a module, choose M here; the module
>> +	  will be called pwm-th1520. If you are unsure, say N.
>> +
>>  config PWM_TIECAP
>>  	tristate "ECAP PWM support"
>>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 96160f4257fcb0e0951581af0090615c0edf5260..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -73,3 +73,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
> 
> Alphabetic ordering please
> 
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,287 @@
>> +// 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
> 
> A short paragraph describing the hardware limitations of that driver
> here would be nice. While you probably cannot stick to the exact format
> used in newer C drivers such that
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> 
> emits the info for your driver, I'd appreciate you sticking to mostly
> this format.

Good point. I will add a documentation comment block at the top of the
file outlining the hardware limitations, keeping the format as close as
possible to what is used in the C drivers.

> 
>> +use core::ops::Deref;
>> +use kernel::{
>> +    c_str,
>> +    clk::Clk,
>> +    device::{Bound, Core, Device},
>> +    devres,
>> +    error::{code::*, Result},
>> +    io::mem::IoMem,
>> +    math::KernelMathExt,
>> +    of, platform,
>> +    prelude::*,
>> +    pwm, time,
>> +};
>> +
>> +const MAX_PWM_NUM: u32 = 6;
>> +
>> +// Register offsets
>> +const fn th1520_pwm_chn_base(n: u32) -> usize {
>> +    (n * 0x20) as usize
>> +}
> 
> empty line here between these functions?
> 
>> +const fn th1520_pwm_ctrl(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n)
>> +}
>> +const fn th1520_pwm_per(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n) + 0x08
>> +}
>> +const fn th1520_pwm_fp(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +// Control register bits
>> +const 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 TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +/// Hardware-specific waveform representation for TH1520.
> 
> Some comments use 2 and other 3 slashes. Does this have any semantic?

Yes, they have a semantic difference. /// denotes a documentation
comment that is processed by rustdoc to generate documentation, while //
is a regular implementation comment. The compiler is configured to
require documentation for public items (like structs and functions),
which is why /// is used on the struct definition.

> 
>> +#[derive(Copy, Clone, Debug, Default)]
>> +struct Th1520WfHw {
>> +    period_cycles: u32,
>> +    duty_cycles: u32,
>> +    ctrl_val: u32,
>> +    enabled: bool,
>> +}
>> +
>> +/// The driver's private data struct. It holds all necessary devres-managed resources.
>> +struct Th1520PwmDriverData {
>> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
>> +    clk: Clk,
>> +}
>> +
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> +    type WfHw = Th1520WfHw;
>> +
>> +    fn get_state(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
>> +        state: &mut pwm::State,
>> +        parent_dev: &Device<Bound>,
>> +    ) -> Result {
> 
> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> .write_waveform() or .apply() + .get_state(), but don't mix these.

You are absolutely right. This was a misunderstanding of the new API on
my part. I will remove the .get_state implementation entirely in the
next version and rely solely on the waveform operations.

> 
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_guard = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_guard.deref();
>> +        let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>> +        let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
>> +        let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
>> +
>> +        state.set_enabled(duty_cycles != 0);
>> +
>> +        let rate_hz = data.clk.rate().as_hz();
>> +        let period_ns = (period_cycles as u64)
>> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> +            .unwrap_or(0);
> 
> What does .unwrap_or(0) do? You need to round up in this mul_div
> operation.

The .unwrap_or(0) is to handle the case where the mul_div helper returns
None, which can happen if the divisor (rate_hz) is zero. In that case,
the period  becomes 0. The mul_div helper is introduced in this commit
[1].

[1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/

> 
>> +        state.set_period(period_ns);
>> +
>> +        let duty_ns = (duty_cycles as u64)
>> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> +            .unwrap_or(0);
>> +        state.set_duty_cycle(duty_ns);
>> +
>> +        if (ctrl & PWM_FPOUT) != 0 {
>> +            state.set_polarity(pwm::Polarity::Normal);
>> +        } else {
>> +            state.set_polarity(pwm::Polarity::Inversed);
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>> +    fn round_waveform_tohw(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
>> +        wf: &pwm::Waveform,
>> +    ) -> Result<(i32, Self::WfHw)> {
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +
>> +        if wf.duty_offset_ns != 0 {
>> +            dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
> 
> That's wrong, pick the biggest offset value that is possible to
> implement and not bigger than the requested value.
> Your hardware can do inversed polarity, so offset is either 0 or
> period-duty.

Addressed below with the pwm.state() comment

> 
>> +            return Err(ENOTSUPP);
>> +        }
>> +
>> +        if wf.period_length_ns == 0 {
>> +            return Ok((
>> +                0,
>> +                Th1520WfHw {
>> +                    enabled: false,
>> +                    ..Default::default()
>> +                },
>> +            ));
>> +        }
>> +
>> +        let rate_hz = data.clk.rate().as_hz();
>> +
>> +        let period_cycles = wf
>> +            .period_length_ns
>> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> +            .ok_or(EINVAL)?;
> 
> If period_length_ns is BIG, pick the biggest possible period_cycles
> value, not EINVAL.

In this case EINVAL mean the function would return EINVAL not
'period_cycles' = EINVAL. This won't happen here since
time::NSEC_PER_SEC is a constant, so this won't return None. This is not
checking for overflow.

> 
>> +        if period_cycles > u32::MAX as u64 {

In here I could pick period_cycles = u32::MAX.

>> +            dev_err!(
>> +                chip.device(),
>> +                "PWM-{}: Calculated period {} cycles is out of range\n",
>> +                hwpwm,
>> +                period_cycles
>> +            );
>> +            return Err(EINVAL);
>> +        }
> 
> ditto.
> 
>> +        let duty_cycles = wf
>> +            .duty_length_ns
>> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> +            .ok_or(EINVAL)?;
>> +        if duty_cycles > period_cycles {
> 
> You can assume this won't happen.
> 
>> +            dev_err!(
>> +                chip.device(),
>> +                "PWM-{}: Duty {}ns > period {}ns\n",
>> +                hwpwm,
>> +                wf.duty_length_ns,
>> +                wf.period_length_ns
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let mut ctrl_val = PWM_CONTINUOUS_MODE;
>> +        if pwm.state().polarity() == pwm::Polarity::Normal {
>> +            ctrl_val |= PWM_FPOUT;
> 
> What is pwm.state()? If that's similar to pwm->state in C this is
> irrelevant here. It describes the current state, not the new request.

Yes, you are correct. I should derive the polarity from the requested
waveform arguments, not from the current state. I see now how to handle
both the duty_offset and the polarity based on the arguments in wf. I
will implement the correct logic.

> 
>> +        }
>> +
>> +        let wfhw = Th1520WfHw {
>> +            period_cycles: period_cycles as u32,
>> +            duty_cycles: duty_cycles as u32,
>> +            ctrl_val,
>> +            enabled: true,
>> +        };
>> +
>> +        dev_dbg!(
>> +            chip.device(),
>> +            "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
>> +            wfhw.period_cycles,
>> +            wfhw.duty_cycles,
>> +            wfhw.ctrl_val
>> +        );
> 
> This would be much more helpful if it also contained the values from wf.
> 
>> +        Ok((0, wfhw))
>> +    }
>> +
>> +    fn write_waveform(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
>> +        wfhw: &Self::WfHw,
>> +        parent_dev: &Device<Bound>,
>> +    ) -> Result {
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_guard = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_guard.deref();
>> +        let was_enabled = pwm.state().enabled();
>> +
>> +        if !wfhw.enabled {
>> +            if was_enabled {
>> +                let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
> 
> Do you need that read? Isn't is clear what the value is?
> 
>> +                ctrl &= !PWM_CFG_UPDATE;
>> +
>> +                iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> +                iomap.write32(0, th1520_pwm_fp(hwpwm));
>> +                iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> +            }
>> +            return Ok(());
>> +        }
>> +
>> +        let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
> 
> wfhw.ctrl_val never has PWM_CFG_UPDATE set.

You're right about the redundant read and the logic with PWM_CFG_UPDATE.
These are leftovers from a frustrating debug session related to a clock
gating issue. I will refactor this section to be cleaner and more
direct.

> 
>> +        iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> +        iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
>> +        iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
>> +        iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> +
>> +        if !was_enabled {
>> +            iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
> 
> Can this be combined with the above write?

Per the TH1520 peripheral manual [2] (chapter 6.6.2.1), the PWM_START bit
should only be asserted when enabling the PWM for the first time, not
during a reconfiguration of an alreadyrunning channel. The separate if
statement is there to handle this specific hardware requirement.

[2] - https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/6b56e2d69485c375c5912eaa2791f79f1d089c07/docs/TH1520%20Peripheral%20Interface%20User%2

> 
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl Drop for Th1520PwmDriverData {
>> +    fn drop(&mut self) {
>> +        self.clk.disable_unprepare();
>> +    }
>> +}
>> +
>> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
>> +
>> +struct Th1520PwmPlatformDriver {
>> +    _registration: pwm::Registration,
>> +}
>> +
>> +kernel::of_device_table!(
>> +    OF_TABLE,
>> +    MODULE_OF_TABLE,
>> +    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
>> +    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
>> +);
>> +
>> +impl platform::Driver for Th1520PwmPlatformDriver {
>> +    type IdInfo = ();
>> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
>> +
>> +    fn probe(
>> +        pdev: &platform::Device<Core>,
>> +        _id_info: Option<&Self::IdInfo>,
>> +    ) -> Result<Pin<KBox<Self>>> {
>> +        let dev = pdev.as_ref();
>> +        let resource = pdev.resource(0).ok_or(ENODEV)?;
>> +        let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
>> +        let clk = Clk::get(pdev.as_ref(), None)?;
>> +
>> +        clk.prepare_enable()?;
> 
> We don't have clk_rate_get_exclusive() yet, right? Then please add a
> comment here that this needs to be added here when it became available.

Yeah sadly we don't have that abstraction yet.

> 
>> +
>> +        let rate_hz = clk.rate().as_hz();
>> +        if rate_hz == 0 {
>> +            dev_err!(dev, "Clock rate is zero\n");
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        if rate_hz > time::NSEC_PER_SEC as usize {
>> +            dev_err!(
>> +                dev,
>> +                "Clock rate {} Hz is too high, not supported.\n",
>> +                rate_hz
>> +            );
>> +            return Err(ERANGE);
>> +        }
>> +
>> +        let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
>> +
>> +        let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
>> +        chip.set_drvdata(drvdata);
>> +
>> +        let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
>> +
>> +        Ok(KBox::new(
>> +            Th1520PwmPlatformDriver {
>> +                _registration: registration,
>> +            },
>> +            GFP_KERNEL,
>> +        )?
>> +        .into())
>> +    }
>> +}
>> +
>> +kernel::module_platform_driver! {
>> +    type: Th1520PwmPlatformDriver,
>> +    name: "pwm-th1520",
>> +    author: "Michal Wilczynski <m.wilczynski@samsung.com>",
>> +    description: "T-HEAD TH1520 PWM driver",
>> +    license: "GPL v2",
>> +}
> 
> Best regards
> Uwe

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

Hello Michal,

On Wed, Jun 11, 2025 at 10:04:54PM +0200, Michal Wilczynski wrote:
> On 6/11/25 08:58, Uwe Kleine-König wrote:
> > On Tue, Jun 10, 2025 at 02:52:50PM +0200, Michal Wilczynski wrote:
> > Some comments use 2 and other 3 slashes. Does this have any semantic?
> 
> Yes, they have a semantic difference. /// denotes a documentation
> comment that is processed by rustdoc to generate documentation, while //
> is a regular implementation comment. The compiler is configured to
> require documentation for public items (like structs and functions),
> which is why /// is used on the struct definition.

ok, thanks.

> >> +        let period_ns = (period_cycles as u64)
> >> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
> >> +            .unwrap_or(0);
> > 
> > What does .unwrap_or(0) do? You need to round up in this mul_div
> > operation.
> 
> The .unwrap_or(0) is to handle the case where the mul_div helper returns
> None, which can happen if the divisor (rate_hz) is zero. In that case,

It can *only* happen if rate_hz is zero? If we had
clk_rate_exclusive_get() we could rely on rate_hz being non-zero.

> the period  becomes 0. The mul_div helper is introduced in this commit
> [1].
> 
> [1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/
> 
> > 
> >> +        let period_cycles = wf
> >> +            .period_length_ns
> >> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
> >> +            .ok_or(EINVAL)?;
> > 
> > If period_length_ns is BIG, pick the biggest possible period_cycles
> > value, not EINVAL.
> 
> In this case EINVAL mean the function would return EINVAL not
> 'period_cycles' = EINVAL. This won't happen here since
> time::NSEC_PER_SEC is a constant, so this won't return None. This is not
> checking for overflow.

OK, so this is only there to please the rust compiler, right?

> > 
> >> +        if period_cycles > u32::MAX as u64 {
> 
> In here I could pick period_cycles = u32::MAX.

indeed.

> > 
> >> +        iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
> >> +        iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
> >> +        iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
> >> +        iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
> >> +
> >> +        if !was_enabled {
> >> +            iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
> > 
> > Can this be combined with the above write?
> 
> Per the TH1520 peripheral manual [2] (chapter 6.6.2.1), the PWM_START bit
> should only be asserted when enabling the PWM for the first time, not
> during a reconfiguration of an alreadyrunning channel. The separate if
> statement is there to handle this specific hardware requirement.

Yeah, I wondered if you could simplify that to (well, or make it a bit
faster at least) using:

	ctrl_val = wfhw.ctrl_val | PWM_CFG_UPDATE;
        if !was_enabled {
		ctrl_val |= PWM_START;
	}
		
        iomap.write32(ctrl_val, th1520_pwm_ctrl(hwpwm));

Best regards
Uwe

On 6/11/25 22:04, Michal Wilczynski wrote:
> 
> 
> On 6/11/25 08:58, Uwe Kleine-König wrote:
>> Hello,
>>
>>
>> What does .unwrap_or(0) do? You need to round up in this mul_div
>> operation.

Yeah and I'm thinking that the helper needs to be updated or new one
added like mul_div_round_up, to do the rounding

> 
> The .unwrap_or(0) is to handle the case where the mul_div helper returns
> None, which can happen if the divisor (rate_hz) is zero. In that case,
> the period  becomes 0. The mul_div helper is introduced in this commit
> [1].
> 
> [1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/
> 


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

On 6/11/25 08:58, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jun 10, 2025 at 02:52:50PM +0200, Michal Wilczynski wrote:
>> Introduce a PWM driver for the T-HEAD TH1520 SoC, written in Rust and
>> utilizing the safe PWM abstractions from the preceding commit.
>>
>> The driver implements the pwm::PwmOps trait using the modern waveform
>> API (round_waveform_tohw, write_waveform, etc.) to support configuration
>> of period, duty cycle, and polarity for the TH1520's PWM channels.
>>
>> Resource management is handled using idiomatic Rust patterns. The PWM
>> chip object is allocated via pwm::Chip::new and its registration with
>> the PWM core is managed by the pwm::Registration RAII guard. This
>> ensures pwmchip_remove is always called when the driver unbinds,
>> preventing resource leaks. Device managed resources are used for the
>> MMIO region, and the clock lifecycle is correctly managed in the
>> driver's private data Drop implementation.
>>
>> The driver's core logic is written entirely in safe Rust, with no unsafe
>> blocks.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  MAINTAINERS               |   1 +
>>  drivers/pwm/Kconfig       |  10 ++
>>  drivers/pwm/Makefile      |   1 +
>>  drivers/pwm/pwm_th1520.rs | 287 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 299 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5589c0d2253bcb04e78d7b89ef6ef0ed41121d77..966ce515c8bfefdff1975bb716a267435ec0feae 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21319,6 +21319,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:	drivers/reset/reset-th1520.c
>>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
>>  F:	include/dt-bindings/power/thead,th1520-power.h
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 03c5a100a03e2acdccf8a46b9c70b736b630bd3a..be05658a568cb9156ef623caf54ff1aaba898d01 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -719,6 +719,16 @@ config PWM_TEGRA
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-tegra.
>>  
>> +config PWM_TH1520_RUST
> 
> Is "_RUST" relevant here? I'd drop that.

Yeah I think it's a good idea to drop.

> 
>> +	tristate "TH1520 PWM support (Rust)"
> 
> Also while having drivers is rust is a great step forward, it's not
> relevant to the user selecting support for the TH1520 device.
> 
>> +	depends on RUST_PWM_ABSTRACTIONS
>> +	help
>> +	  This option enables the driver for the PWM controller found on the
>> +	  T-HEAD TH1520 SoC. This driver is written in Rust.
>> +
>> +	  To compile this driver as a module, choose M here; the module
>> +	  will be called pwm-th1520. If you are unsure, say N.
>> +
>>  config PWM_TIECAP
>>  	tristate "ECAP PWM support"
>>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 96160f4257fcb0e0951581af0090615c0edf5260..d41b1940df903ba2036d8e3ed93efcd66834b7ab 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -73,3 +73,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
> 
> Alphabetic ordering please
> 
>> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9e43474f5123b51c49035d71219303a606c20a5a
>> --- /dev/null
>> +++ b/drivers/pwm/pwm_th1520.rs
>> @@ -0,0 +1,287 @@
>> +// 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
> 
> A short paragraph describing the hardware limitations of that driver
> here would be nice. While you probably cannot stick to the exact format
> used in newer C drivers such that
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> 
> emits the info for your driver, I'd appreciate you sticking to mostly
> this format.

Good point. I will add a documentation comment block at the top of the
file outlining the hardware limitations, keeping the format as close as
possible to what is used in the C drivers.

> 
>> +use core::ops::Deref;
>> +use kernel::{
>> +    c_str,
>> +    clk::Clk,
>> +    device::{Bound, Core, Device},
>> +    devres,
>> +    error::{code::*, Result},
>> +    io::mem::IoMem,
>> +    math::KernelMathExt,
>> +    of, platform,
>> +    prelude::*,
>> +    pwm, time,
>> +};
>> +
>> +const MAX_PWM_NUM: u32 = 6;
>> +
>> +// Register offsets
>> +const fn th1520_pwm_chn_base(n: u32) -> usize {
>> +    (n * 0x20) as usize
>> +}
> 
> empty line here between these functions?
> 
>> +const fn th1520_pwm_ctrl(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n)
>> +}
>> +const fn th1520_pwm_per(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n) + 0x08
>> +}
>> +const fn th1520_pwm_fp(n: u32) -> usize {
>> +    th1520_pwm_chn_base(n) + 0x0c
>> +}
>> +
>> +// Control register bits
>> +const 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 TH1520_PWM_REG_SIZE: usize = 0xB0;
>> +
>> +/// Hardware-specific waveform representation for TH1520.
> 
> Some comments use 2 and other 3 slashes. Does this have any semantic?

Yes, they have a semantic difference. /// denotes a documentation
comment that is processed by rustdoc to generate documentation, while //
is a regular implementation comment. The compiler is configured to
require documentation for public items (like structs and functions),
which is why /// is used on the struct definition.

> 
>> +#[derive(Copy, Clone, Debug, Default)]
>> +struct Th1520WfHw {
>> +    period_cycles: u32,
>> +    duty_cycles: u32,
>> +    ctrl_val: u32,
>> +    enabled: bool,
>> +}
>> +
>> +/// The driver's private data struct. It holds all necessary devres-managed resources.
>> +struct Th1520PwmDriverData {
>> +    iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
>> +    clk: Clk,
>> +}
>> +
>> +impl pwm::PwmOps for Th1520PwmDriverData {
>> +    type WfHw = Th1520WfHw;
>> +
>> +    fn get_state(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
>> +        state: &mut pwm::State,
>> +        parent_dev: &Device<Bound>,
>> +    ) -> Result {
> 
> Huh, if you do the newstyle stuff, .get_state() is wrong. It's either
> .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() +
> .write_waveform() or .apply() + .get_state(), but don't mix these.

You are absolutely right. This was a misunderstanding of the new API on
my part. I will remove the .get_state implementation entirely in the
next version and rely solely on the waveform operations.

> 
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_guard = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_guard.deref();
>> +        let ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
>> +        let period_cycles = iomap.read32(th1520_pwm_per(hwpwm));
>> +        let duty_cycles = iomap.read32(th1520_pwm_fp(hwpwm));
>> +
>> +        state.set_enabled(duty_cycles != 0);
>> +
>> +        let rate_hz = data.clk.rate().as_hz();
>> +        let period_ns = (period_cycles as u64)
>> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> +            .unwrap_or(0);
> 
> What does .unwrap_or(0) do? You need to round up in this mul_div
> operation.

The .unwrap_or(0) is to handle the case where the mul_div helper returns
None, which can happen if the divisor (rate_hz) is zero. In that case,
the period  becomes 0. The mul_diver helper is introduced in this commit
[1].

[1] - https://lore.kernel.org/all/20250609-math-rust-v1-v1-1-285fac00031f@samsung.com/

> 
>> +        state.set_period(period_ns);
>> +
>> +        let duty_ns = (duty_cycles as u64)
>> +            .mul_div(time::NSEC_PER_SEC as u64, rate_hz as u64)
>> +            .unwrap_or(0);
>> +        state.set_duty_cycle(duty_ns);
>> +
>> +        if (ctrl & PWM_FPOUT) != 0 {
>> +            state.set_polarity(pwm::Polarity::Normal);
>> +        } else {
>> +            state.set_polarity(pwm::Polarity::Inversed);
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>> +    fn round_waveform_tohw(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
>> +        wf: &pwm::Waveform,
>> +    ) -> Result<(i32, Self::WfHw)> {
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +
>> +        if wf.duty_offset_ns != 0 {
>> +            dev_err!(chip.device(), "PWM-{}: Duty offset not supported\n", hwpwm);
> 
> That's wrong, pick the biggest offset value that is possible to
> implement and not bigger than the requested value.
> Your hardware can do inversed polarity, so offset is either 0 or
> period-duty.

Addressed below with the pwm.state() comment

> 
>> +            return Err(ENOTSUPP);
>> +        }
>> +
>> +        if wf.period_length_ns == 0 {
>> +            return Ok((
>> +                0,
>> +                Th1520WfHw {
>> +                    enabled: false,
>> +                    ..Default::default()
>> +                },
>> +            ));
>> +        }
>> +
>> +        let rate_hz = data.clk.rate().as_hz();
>> +
>> +        let period_cycles = wf
>> +            .period_length_ns
>> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> +            .ok_or(EINVAL)?;
> 
> If period_length_ns is BIG, pick the biggest possible period_cycles
> value, not EINVAL.

In this case EINVAL mean the function would return EINVAL not
'period_cycles' = EINVAL. This won't happen here since
time::NSEC_PER_SEC is a constant, so this won't return None. This is not
checking for overflow.

> 
>> +        if period_cycles > u32::MAX as u64 {

In here I could pick period_cycles = u32::MAX.

>> +            dev_err!(
>> +                chip.device(),
>> +                "PWM-{}: Calculated period {} cycles is out of range\n",
>> +                hwpwm,
>> +                period_cycles
>> +            );
>> +            return Err(EINVAL);
>> +        }
> 
> ditto.
> 
>> +        let duty_cycles = wf
>> +            .duty_length_ns
>> +            .mul_div(rate_hz as u64, time::NSEC_PER_SEC as u64)
>> +            .ok_or(EINVAL)?;
>> +        if duty_cycles > period_cycles {
> 
> You can assume this won't happen.
> 
>> +            dev_err!(
>> +                chip.device(),
>> +                "PWM-{}: Duty {}ns > period {}ns\n",
>> +                hwpwm,
>> +                wf.duty_length_ns,
>> +                wf.period_length_ns
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let mut ctrl_val = PWM_CONTINUOUS_MODE;
>> +        if pwm.state().polarity() == pwm::Polarity::Normal {
>> +            ctrl_val |= PWM_FPOUT;
> 
> What is pwm.state()? If that's similar to pwm->state in C this is
> irrelevant here. It describes the current state, not the new request.

Yes, you are correct. I should derive the polarity from the requested
waveform arguments, not from the current state. I see now how to handle
both the duty_offset and the polarity based on the arguments in wf. I
will implement the correct logic.

> 
>> +        }
>> +
>> +        let wfhw = Th1520WfHw {
>> +            period_cycles: period_cycles as u32,
>> +            duty_cycles: duty_cycles as u32,
>> +            ctrl_val,
>> +            enabled: true,
>> +        };
>> +
>> +        dev_dbg!(
>> +            chip.device(),
>> +            "wfhw -- Period: {}, Duty: {}, Ctrl: 0x{:x}\n",
>> +            wfhw.period_cycles,
>> +            wfhw.duty_cycles,
>> +            wfhw.ctrl_val
>> +        );
> 
> This would be much more helpful if it also contained the values from wf.
> 
>> +        Ok((0, wfhw))
>> +    }
>> +
>> +    fn write_waveform(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
>> +        wfhw: &Self::WfHw,
>> +        parent_dev: &Device<Bound>,
>> +    ) -> Result {
>> +        let data: &Self = chip.drvdata().ok_or(EINVAL)?;
>> +        let hwpwm = pwm.hwpwm();
>> +        let iomem_guard = data.iomem.access(parent_dev)?;
>> +        let iomap = iomem_guard.deref();
>> +        let was_enabled = pwm.state().enabled();
>> +
>> +        if !wfhw.enabled {
>> +            if was_enabled {
>> +                let mut ctrl = iomap.read32(th1520_pwm_ctrl(hwpwm));
> 
> Do you need that read? Isn't is clear what the value is?
> 
>> +                ctrl &= !PWM_CFG_UPDATE;
>> +
>> +                iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> +                iomap.write32(0, th1520_pwm_fp(hwpwm));
>> +                iomap.write32(ctrl | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> +            }
>> +            return Ok(());
>> +        }
>> +
>> +        let ctrl = wfhw.ctrl_val & !PWM_CFG_UPDATE;
> 
> wfhw.ctrl_val never has PWM_CFG_UPDATE set.

You're right about the redundant read and the logic with PWM_CFG_UPDATE.
These are leftovers from a frustrating debug session related to a clock
gating issue. I will refactor this section to be cleaner and more
direct.

> 
>> +        iomap.write32(ctrl, th1520_pwm_ctrl(hwpwm));
>> +        iomap.write32(wfhw.period_cycles, th1520_pwm_per(hwpwm));
>> +        iomap.write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm));
>> +        iomap.write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm));
>> +
>> +        if !was_enabled {
>> +            iomap.write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm));
> 
> Can this be combined with the above write?

Per the TH1520 peripheral manual [2] (chapter 6.6.2.1), the PWM_START bit
should only be asserted when enabling the PWM for the first time, not
during a reconfiguration of an alreadyrunning channel. The separate if
statement is there to handle this specific hardware requirement.

[2] - https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/6b56e2d69485c375c5912eaa2791f79f1d089c07/docs/TH1520%20Peripheral%20Interface%20User%2

> 
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl Drop for Th1520PwmDriverData {
>> +    fn drop(&mut self) {
>> +        self.clk.disable_unprepare();
>> +    }
>> +}
>> +
>> +static TH1520_PWM_OPS: pwm::PwmOpsVTable = pwm::create_pwm_ops::<Th1520PwmDriverData>();
>> +
>> +struct Th1520PwmPlatformDriver {
>> +    _registration: pwm::Registration,
>> +}
>> +
>> +kernel::of_device_table!(
>> +    OF_TABLE,
>> +    MODULE_OF_TABLE,
>> +    <Th1520PwmPlatformDriver as platform::Driver>::IdInfo,
>> +    [(of::DeviceId::new(c_str!("thead,th1520-pwm")), ())]
>> +);
>> +
>> +impl platform::Driver for Th1520PwmPlatformDriver {
>> +    type IdInfo = ();
>> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
>> +
>> +    fn probe(
>> +        pdev: &platform::Device<Core>,
>> +        _id_info: Option<&Self::IdInfo>,
>> +    ) -> Result<Pin<KBox<Self>>> {
>> +        let dev = pdev.as_ref();
>> +        let resource = pdev.resource(0).ok_or(ENODEV)?;
>> +        let iomem = pdev.ioremap_resource_sized::<TH1520_PWM_REG_SIZE>(resource)?;
>> +        let clk = Clk::get(pdev.as_ref(), None)?;
>> +
>> +        clk.prepare_enable()?;
> 
> We don't have clk_rate_get_exclusive() yet, right? Then please add a
> comment here that this needs to be added here when it became available.

Yeah sadly we don't have that abstraction yet.

> 
>> +
>> +        let rate_hz = clk.rate().as_hz();
>> +        if rate_hz == 0 {
>> +            dev_err!(dev, "Clock rate is zero\n");
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        if rate_hz > time::NSEC_PER_SEC as usize {
>> +            dev_err!(
>> +                dev,
>> +                "Clock rate {} Hz is too high, not supported.\n",
>> +                rate_hz
>> +            );
>> +            return Err(ERANGE);
>> +        }
>> +
>> +        let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?;
>> +
>> +        let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?;
>> +        chip.set_drvdata(drvdata);
>> +
>> +        let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?;
>> +
>> +        Ok(KBox::new(
>> +            Th1520PwmPlatformDriver {
>> +                _registration: registration,
>> +            },
>> +            GFP_KERNEL,
>> +        )?
>> +        .into())
>> +    }
>> +}
>> +
>> +kernel::module_platform_driver! {
>> +    type: Th1520PwmPlatformDriver,
>> +    name: "pwm-th1520",
>> +    author: "Michal Wilczynski <m.wilczynski@samsung.com>",
>> +    description: "T-HEAD TH1520 PWM driver",
>> +    license: "GPL v2",
>> +}
> 
> Best regards
> Uwe

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