Message-ID: <jbm3qvowi5vskhnjyqlp3xek36gzzqjt35m66eayxi6lmi525t@iefevopxjl53>
Date: Wed, 11 Jun 2025 08:58:57 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>, 
	Fu Wei <wefu@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Benno Lossin <lossin@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-pwm@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

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@...sung.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@...sung.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@...sung.com>",
> +    description: "T-HEAD TH1520 PWM driver",
> +    license: "GPL v2",
> +}

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists