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

[Andrea della Porta]

Hi Uwe,

On 19:31 Fri 10 Apr     , Uwe Kleine-König wrote:
> Hello Andrea,
> 
> nice work for a v2!

Thanks!

> 
> On Fri, Apr 10, 2026 at 04:09:58PM +0200, Andrea della Porta wrote:

<...snip...>

> > +#define RP1_PWM_GLOBAL_CTRL	0x000
> > +#define RP1_PWM_CHANNEL_CTRL(x)	(0x014 + ((x) * 0x10))
> > +#define RP1_PWM_RANGE(x)	(0x018 + ((x) * 0x10))
> > +#define RP1_PWM_PHASE(x)	(0x01C + ((x) * 0x10))
> > +#define RP1_PWM_DUTY(x)		(0x020 + ((x) * 0x10))
> > +
> > +/* 8:FIFO_POP_MASK + 0:Trailing edge M/S modulation */
> > +#define RP1_PWM_CHANNEL_DEFAULT		(BIT(8) + BIT(0))
> 
> Please add a #define for BIT(8) and then use that and
> FIELD_PREP(RP1_PWM_MODE, RP1_PWM_MODE_SOMENICENAME) to define the
> constant. Also I would define it below the register defines.

Ack.

> 
> > +#define RP1_PWM_CHANNEL_ENABLE(x)	BIT(x)
> > +#define RP1_PWM_POLARITY		BIT(3)
> > +#define RP1_PWM_SET_UPDATE		BIT(31)
> > +#define RP1_PWM_MODE_MASK		GENMASK(1, 0)
> 
> s/_MASK// please
> 
> It would be great if the bitfield's names started with the register
> name.

Ack.

> 
> > +
> > +#define RP1_PWM_NUM_PWMS	4
> > +
> > +struct rp1_pwm {
> > +	struct regmap	*regmap;
> > +	struct clk	*clk;
> > +	unsigned long	clk_rate;
> > +	bool		clk_enabled;
> > +};
> > +
> > +struct rp1_pwm_waveform {
> > +	u32	period_ticks;
> > +	u32	duty_ticks;
> > +	bool	enabled;
> > +	bool	inverted_polarity;
> > +};
> > +
> > +static const struct regmap_config rp1_pwm_regmap_config = {
> > +	.reg_bits    = 32,
> > +	.val_bits    = 32,
> > +	.reg_stride  = 4,
> > +	.max_register = 0x60,
> 
> I'm not a fan of aligning the = in a struct, still more if it fails like
> here. Please consistently align all =s, or even better, use a single
> space before each =. (Same for the struct definitions above, but I won't
> insist.)

Let's use the single space.

> 
> > +};
> > +
> > +static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > +	u32 value;
> > +
> > +	/* update the changed registers on the next strobe to avoid glitches */
> > +	regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value);
> > +	value |= RP1_PWM_SET_UPDATE;
> > +	regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value);
> 
> I assume there is a glitch if I update two channels and the old
> configuration of the first channel ends while I'm in the middle of
> configuring the second?

The configuration registers are per-channel but the update flag is global.
I don't have details of the hw insights, my best guess is that anything that
you set in the registers before updating the flag will take effect, so there
should be no glitches.

> 
> > +}
> > +
> > +static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > +
> > +	/* init channel to reset defaults */
> > +	regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), RP1_PWM_CHANNEL_DEFAULT);
> > +	return 0;
> > +}
> > +
> > +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);
> > +	struct rp1_pwm_waveform *wfhw = _wfhw;
> > +	u64 clk_rate = rp1->clk_rate;
> > +	u64 ticks;
> 
> 	if (!wf->period_length_ns)
> 		wfhw->enabled = false
> 		return 0;
> 
> > +	ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
> 
> To ensure this doesn't overflow please fail to probe the driver if
> clk_rate > 1 GHz with an explaining comment. (Or alternatively calculate
> the length of period_ticks = U32_MAX and skip the calculation if
> wf->period_length_ns is bigger.)

Ack.

> 
> > +	if (ticks > U32_MAX)
> > +		ticks = U32_MAX;
> > +	wfhw->period_ticks = ticks;
> 
> What happens if wf->period_length_ns > 0 but ticks == 0?

I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1
in this case.

> 
> > +	if (wf->duty_offset_ns + wf->duty_length_ns >= wf->period_length_ns) {
> 
> The maybe surprising effect here is that in the two cases
> 
> 	wf->duty_offset_ns == wf->period_length_ns and wf->duty_length_ns == 0
> 
> and
> 	
> 	wf->duty_length_ns == wf->period_length_ns and wf->duty_offset_ns == 0
> 
> you're configuring inverted polarity. I doesn't matter technically
> because the result is the same, but for consumers still using pwm_state
> this is irritating. That's why pwm-stm32 uses inverted polarity only if
> also wf->duty_length_ns and wf->duty_offset_ns are non-zero.

Ack.

> 
> > +		ticks = mul_u64_u64_div_u64(wf->period_length_ns - wf->duty_length_ns,
> > +					    clk_rate, NSEC_PER_SEC);
> 
> The rounding is wrong here. You should pick the biggest duty_length not
> bigger than wf->duty_length_ns, so you have to use
> 
> 	ticks = wfhw->period_ticks - mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC):
> 
> . I see this is a hole in the pwmtestperf coverage.

Ack.

> 
> > +		wfhw->inverted_polarity = true;
> > +	} else {
> > +		ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);
> > +		wfhw->inverted_polarity = false;
> > +	}
> > +
> > +	if (ticks > wfhw->period_ticks)
> > +		ticks = wfhw->period_ticks;
> 
> You can and should assume that wf->duty_length_ns <=
> wf->period_length_ns. Then the if condition can never become true.

Ack.

> 
> > +	wfhw->duty_ticks = ticks;
> > +
> > +	wfhw->enabled = !!wfhw->duty_ticks;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> > +					 struct pwm_device *pwm,
> > +					 const void *_wfhw,
> > +					 struct pwm_waveform *wf)
> > +{
> > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> > +	u64 clk_rate = rp1->clk_rate;
> > +	u32 ticks;
> > +
> > +	memset(wf, 0, sizeof(*wf));
> 
> 	wf = (struct pwm_waveform){ };
> 
> is usually more efficient.

Ack.

> 
> > +	if (!wfhw->enabled)
> > +		return 0;
> > +
> > +	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->period_ticks * NSEC_PER_SEC, clk_rate);
> > +
> > +	if (wfhw->inverted_polarity) {
> > +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> > +						      clk_rate);
> > +	} else {
> > +		wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> > +						      clk_rate);
> > +		ticks = wfhw->period_ticks - wfhw->duty_ticks;
> > +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate);
> > +	}
> 
> This needs adaption after the rounding issue in tohw is fixed.

Ack.

> 
> > +	return 0;
> > +}
> > +
> > +static int rp1_pwm_write_waveform(struct pwm_chip *chip,
> > +				  struct pwm_device *pwm,
> > +				  const void *_wfhw)
> > +{
> > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> > +	u32 value;
> > +
> > +	/* set period and duty cycle */
> > +	regmap_write(rp1->regmap,
> > +		     RP1_PWM_RANGE(pwm->hwpwm), wfhw->period_ticks);
> > +	regmap_write(rp1->regmap,
> > +		     RP1_PWM_DUTY(pwm->hwpwm), wfhw->duty_ticks);
> > +
> > +	/* set polarity */
> > +	regmap_read(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), &value);
> > +	if (!wfhw->inverted_polarity)
> > +		value &= ~RP1_PWM_POLARITY;
> > +	else
> > +		value |= RP1_PWM_POLARITY;
> > +	regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), value);
> > +
> > +	/* enable/disable */
> > +	regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value);
> > +	if (wfhw->enabled)
> > +		value |= RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm);
> > +	else
> > +		value &= ~RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm);
> > +	regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value);
> 
> You can exit early if wfhw->enabled is false after clearing the channel
> enable bit.

Ack.

> 
> > +	rp1_pwm_apply_config(chip, pwm);
> > +
> > +	return 0;
> > +}
> > +

<,...snip...>

> > +	}
> > +
> > +	return 0;
> > +
> > +err_disable_clk:
> > +	clk_disable_unprepare(rp1->clk);
> > +
> > +	return ret;
> > +}
> 
> On remove you miss to balance the call to clk_prepare_enable() (if no
> failed call to clk_prepare_enable() in rp1_pwm_resume() happend).

Since this driver now exports a syscon, it's only builtin (=Y) so
it cannot be unloaded.
I've also avoided the .remove callback via .suppress_bind_attrs.

> 
> > +
> > +static int rp1_pwm_suspend(struct device *dev)
> > +{
> > +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> > +
> > +	if (rp1->clk_enabled) {
> > +		clk_disable_unprepare(rp1->clk);
> > +		rp1->clk_enabled = false;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rp1_pwm_resume(struct device *dev)
> > +{
> > +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(rp1->clk);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clock on resume: %d\n", ret);
> 
> Please use %pe for error codes.

Ack.

Best regards,
Andrea

> 
> > +		return ret;
> > +	}
> > +
> > +	rp1->clk_enabled = true;
> > +
> > +	return 0;
> > +}
> 
> Best regards
> Uwe