Re: [PATCH v5 2/3] pwm: rp1: Add RP1 PWM controller driver

[Andrea della Porta <andrea.porta@suse.com>]

Hi Sean,

On 15:29 Fri 12 Jun     , Sean Young wrote:
> On Fri, Jun 12, 2026 at 04:01:27PM +0200, Andrea della Porta wrote:
> > From: Naushir Patuck <naush@raspberrypi.com>
> > 
> > The Raspberry Pi RP1 southbridge features an embedded PWM
> > controller with 4 output channels, alongside an RPM interface
> > to read the fan speed on the Raspberry Pi 5.
> > 
> > Add the supporting driver.
> > 

<...snip...>
> 
> I don't understand the point of the clk_enabled field. If the probe
> function succeeds, then clk_enabled = true. It is set to false during
> suspend, but after suspend the only thing which can follow is resume,
> I think. In resume, we set it in to true again unconditionally. So
> it is always true, no?

It's true unless enabling the clock again in rp1_pwm_resume() will fail.
That will trap the unbalanced condition mentioned by Uwe in:
https://lore.kernel.org/all/adiW1tBC8Imd14LD@monoceros/

> 
> > +};
> > +
> > +struct rp1_pwm_waveform {
> > +	u32 period_ticks;
> > +	u32 duty_ticks;
> > +	bool enabled;
> > +	bool inverted_polarity;
> > +};
> > +

<...snip...>

> > +
> > +	/*
> > +	 * The period is limited to U32_MAX, and it will be decremented by one later
> > +	 * to allow 100% duty cycle.
> > +	 */
> > +	if (period_ticks > U32_MAX) {
> > +		period_ticks = U32_MAX;
> > +	} else if (period_ticks < 2) {
> > +		period_ticks = 2;
> > +		ret = 1;
> > +	}
> 
> period_ticks = clamp(period_ticks, 2, U32_MAX);
> 
> Although that misses out the `ret = 1;` which I am not sure about anyway.

But we need that ret = 1 to announce that values have been rounded.

> 
> > +
> > +	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);
> > +	duty_ticks = min(duty_ticks, period_ticks);
> > +	offset_ticks = mul_u64_u64_div_u64(wf->duty_offset_ns, clk_rate, NSEC_PER_SEC);
> > +	if (offset_ticks >= period_ticks)
> > +		offset_ticks %= period_ticks;
> > +	if (duty_ticks && offset_ticks &&
> > +		ret = dev_err_probe(dev, -EINVAL, "Clock rate > 1 GHz is not supported\n");

<...snip...>

> > +		goto err_disable_clk;
> > +	}
> > +	rp1->clk_rate = clk_rate;
> > +
> > +	chip->ops = &rp1_pwm_ops;
> 
> Can we add the following please:
> 
> 	 chip->atomic = true; 
> 
> This means that the pwm can be controlled from atomic context (not process
> context) using pwm_apply_atomic(). This is very helpful for the pwm-ir-tx
> driver, which produces a much more faithful IR signal in atomic context.
> 
> Using pwm for infrared tx is much nicer than using gpio which bit bangs
> the IR signal and holds the CPU with interrupts disabled for upto one second.
> 
> As far as I can see there is no sleeping code in these code paths, so we
> should be fine.

Sure, added.

Many thanks,
Andrea

> 
> Thanks,
> 
> Sean
>

<...snip...>