Message-ID: <98d6b8e5-4694-44df-9ba0-33e6c00d8183@samsung.com>
Date: Tue, 17 Jun 2025 21:43:05 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Uwe Kleine-König <ukleinek@...nel.org>, 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>, 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 v3 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC



On 6/17/25 16:28, Danilo Krummrich wrote:
> On Tue, Jun 17, 2025 at 04:07:27PM +0200, Michal Wilczynski wrote:
>> +    fn write_waveform(
>> +        chip: &mut pwm::Chip,
>> +        pwm: &mut pwm::Device,
> 
> I think you can't hand out mutable references here. This would allow things like
> mem::swap(), which I think are not valid on those structures.
> 
>> +        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)?;
> 
> Technically, this isn't a guard, hence would't call it that way.
> 
>> +        let iomap = iomem_guard.deref();
>> +        let was_enabled = pwm.state().enabled();
>> +
>> +        if !wfhw.enabled {
>> +            if was_enabled {
>> +                iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
>> +                iomap.try_write32(0, th1520_pwm_fp(hwpwm))?;
>> +                iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
>> +            }
>> +            return Ok(());
>> +        }
>> +
>> +        iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?;
>> +        iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?;
>> +        iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?;
>> +        iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?;
> 
> None of the offsets are known at compile time? :(

Sadly they are computed based on runtime parameter hwpwm, so Rust can't
guarantee correctness during compilation :-(.

Thank you for your other feedback, appreciate it !

> 
>> +
>> +        // The `PWM_START` bit must be written in a separate, final transaction, and
>> +        // only when enabling the channel from a disabled state.
>> +        if !was_enabled {
>> +            iomap.try_write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm))?;
>> +        }
>> +
>> +        dev_dbg!(
>> +            chip.device(),
>> +            "PWM-{}: Wrote (per: {}, duty: {})",
>> +            hwpwm,
>> +            wfhw.period_cycles,
>> +            wfhw.duty_cycles,
>> +        );
>> +
>> +        Ok(())
>> +    }
>> +}
> 
> <snip>
> 
>> +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()?;
>> +
>> +        // TODO: Get exclusive ownership of the clock to prevent rate changes.
>> +        // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
>> +        // This should be updated once it is implemented.
>> +        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())
> 
> Here you are setting up the registration for the correct lifetime, however
> drivers could extend the lifetime of the registration arbitrarily, which would
> break the guarantee of the &Device<Bound> we rely on in the callbacks above
> (e.g. write_waveform()).
> 
> Hence, pwm::Registration's lifetime has to be controlled by Devres. I'll also
> add a corresponding comment in your registration patch.
> 
>> +    }
>> +}
> 

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

Powered by blists - more mailing lists