[PATCH v5 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC

[Uwe Kleine-König]

Hello Michal,

On Sat, Jun 28, 2025 at 08:14:59PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:28, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:52PM +0200, Michal Wilczynski wrote:
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index cfddeae0eab3523f04f361fb41ccd1345c0c937b..a675b3bd68392d1b05a47a2a1390c5606647ca15 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
> >> +	tristate "TH1520 PWM support"
> >> +	depends on RUST_PWM_ABSTRACTIONS
> > 
> > RUST_PWM_ABSTRACTIONS is user selectable. Is that sensible. From a
> > user's POV it shouldn't matter if the driver is written in Rust or not.
> 
> You make an excellent point about user experience. My initial thought
> was to follow the depends on pattern that I saw in other Rust drivers.
> 
> I can see how using select would be cleaner for the end user. My only
> hesitation was that it differs from the current convention for Rust
> drivers, and I wanted to be careful about changing an established
> pattern.
> 
> If you are comfortable with setting this direction for the PWM
> subsystem, I am happy to make the change to use select
> RUST_PWM_ABSTRACTIONS (gated by depends on RUST). Please let me know.

Sounds good.

> >> +const TH1520_PWM_REG_SIZE: usize = 0xB0;
> >> +
> >> +fn ns_to_cycles(ns: u64, rate_hz: u64) -> u64 {
> >> +    const NSEC_PER_SEC_U64: u64 = time::NSEC_PER_SEC as u64;
> >> +
> >> +    match ns.checked_mul(rate_hz) {
> >> +        Some(product) => product / NSEC_PER_SEC_U64,
> >> +        None => u64::MAX,
> >> +    }
> > 
> > The semantic here is: If ns * rate_hz overflows, return U64_MAX, else ns
> > * rate_hz / NSEC_PER_SEC, right?
> > 
> > If you cannot easily reproduce what mul_u64_u64_div_u64() does, I think
> > it would be more prudent do make this:
> > 
> > 	match ns.checked_mul(rate_hz) {
> > 	    Some(product) => product,
> > 	    None => u64::MAX,
> > 	} / NSEC_PER_SEC_U64
> 
> Thank you for the feedback on the calculation. I analyzed the two
> approaches and found that on overflow, my version saturates to u64::MAX,
> while the suggested version would result in u64::MAX / NSEC_PER_SEC. I
> believe my original implementation's saturation behavior is more
> predictable. With this in mind, would you be comfortable with me
> retaining the original implementation?

I'm convinced that my alternative is better. Consider the implemented
mapping: Assuming rate_hz = 160000000 you have:

	      ns     |       cycles
	-------------+---------------------
	 ...         |
	115292150452 |          18446744072
	115292150453 |          18446744072
	115292150454 |          18446744072
	115292150455 |          18446744072
	115292150456 |          18446744072
	115292150457 |          18446744073
	115292150458 |          18446744073
	115292150459 |          18446744073
	115292150460 |          18446744073
	115292150461 | 18446744073709551615
	115292150462 | 18446744073709551615
	 ...

that's strange, isn't it?

> >> +            wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> >> +            // We can't recover the original non-zero offset, so we just set it
> >> +            // to a representative non-zero value.
> >> +            wf.duty_offset_ns = 1;
> > 
> > For an inversed polarity signal the duty_offset is polarity - duty_cycle.
> 
> I believe there was a typo in your suggestion and you meant period
> instead of polarity.

ack.

> Based on that, my understanding is that for an
> inverted signal, the generic pwm_waveform struct expects duty_offset_ns
> to represent the duration of the initial low time, while duty_length_ns
> represents the high time.

right.

> so the code would look like this:
> // For an inverted signal, `duty_length_ns` is the high time (period - low_time).
> wf.duty_length_ns = cycles_to_ns(original_duty_cycles, rate_hz);
> // The offset is the initial low time, which is what the hardware register provides.
> wf.duty_offset_ns = cycles_to_ns(duty_cycles, rate_hz);

Looks correct

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20250629/039b1443/attachment.sig>