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

[Uwe Kleine-König]

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@xxxxxxxxxxx/
> 
> > 
> >> +        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

Attachment: signature.asc
Description: PGP signature