linux-kernel - Re: [PATCH v2 2/7] pwm: Add Rust driver for T-HEAD TH1520 SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fya3ewoofrazpwh725ms4yaw6faq6txjyhxkr7hpl5i4az2iq2@fvfpisv6hjml>
Date: Wed, 11 Jun 2025 23:40:22 +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 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

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