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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3351 bytes --]

Hello Andrea,

On Fri, Jul 17, 2026 at 11:25:35AM +0200, Andrea della Porta wrote:
> On 11:18 Thu 16 Jul     , Uwe Kleine-König wrote:
> > On Fri, Jul 03, 2026 at 07:05:25PM +0200, Andrea della Porta wrote:
> > > +static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip,
> > > +				       struct pwm_device *pwm,
> > > +				       const struct pwm_waveform *wf,
> > > +				       void *_wfhw)
> > > +{
> > > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > > +	u64 period_ticks, duty_ticks, offset_ticks;
> > > +	struct rp1_pwm_waveform *wfhw = _wfhw;
> > > +	u64 clk_rate = rp1->clk_rate;
> > > +	int ret = 0;
> > > +
> > > +	if (!wf->period_length_ns) {
> > > +		wfhw->enabled = false;
> > > +		wfhw->inverted_polarity = (pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED);
> > 
> > pwm_get_polarity(pwm) looks wrong here for several reasons. 1st the
> > polarity is defined in *wf and should not depend on the current state,
> > 2nd for a disabled hardware the polarity doesn't matter anyhow, and 3rd
> > you should not call pwm API functions from the lowlevel driver (which
> > might interfere with subsystem locking).
> 
> Ok, but in this case what the output of the disabled channel should be?
> If it was inverted before the disable, would it make sense to let the output
> be high? Or are we allowed to hard code a polarity value on disable?

Given that not all hardwares can drive the output of a disabled PWM to a
fixed level, a consumer that relies on a fixed inactive level output
must not disable the PWM. So it doesn't matter what you do on
wf->period_length_ns == 0, the only objective is to save power.

> > > +	if (!wfhw->inverted_polarity) {
> > > +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> > > +						      (u32)clk_rate);
> > > +	} else {
> > > +		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
> > > +			/* 100% duty cycle case */
> > > +			ticks = 0;
> > > +		} else {
> > > +			ticks = (u64)wfhw->period_ticks + 1 - wfhw->duty_ticks;
> > > +		}
> > > +		wf->duty_length_ns = DIV_ROUND_UP_ULL(ticks * NSEC_PER_SEC, clk_rate);
> > > +		wf->duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
> > 
> > The duty_offset_ns calculation is wrong.
> > 
> > Consider clk_rate = 3000000, period_ticks = 8, inverted_polarity = true and
> > duty_ticks = 4.
> > 
> > Then you have:
> > 
> > 	.period_length_ns = 2666.6666666666666 ns ~> 2667
> > 	.duty_length_ns = 1333.3333333333333 ns -> 1334
> > 	.duty_offset_ns = 1333.3333333333333 ns -> 1334
> > 
> > but .period_length_ns - .duty_length_ns is 1333.
> > 
> > To get this right, you have to calculate
> > 
> > 	wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - ticks) * NSEC_PER_SEC, clk_rate);
> 
> So it will end up as 'duty_length_ns + duty_offset_ns >
> period_length_ns', which is timing violation. Is it allowed (or even
> required) in the PWM subsystem?

Not all hardwares support that, but logically it makes sense. The active
phase of the output just crosses the period border. With duty_length_ns
+ duty_offset_ns < period_length_ns (and duty_offset > 0) it's the
inactive phase that crosses the period border, and there is no reason
why the active phase should be more special than the inactive one.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]