Add RP1 PWM controller support

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Missing pwmchip_remove() on the of_syscon_register_regmap() error path leaves a dangling PWM chip.
- [High] When disabling a PWM channel, the driver fails to clear the polarity bit, causing the output to erroneously remain HIGH if it was previously inverted.
- [High] Registering a devm-managed regmap with the permanent syscon registry creates a teardown asymmetry.
- [High] Hardware channel control registers (CHAN_CTRL) are not restored across system suspend/resume.
- [Medium] Non-monotonic period limit checking in rp1_pwm_round_waveform_tohw() causes larger requested periods to result in smaller hardware periods.
- [Medium] Calculating period_length_ns from hardware state suffers from a 32-bit integer overflow before casting to u64, resulting in a zero period.
- [Medium] Calculating inverted polarity duty length underflows if the hardware is configured for a 100% duty cycle.
- [Medium] Returning a positive integer (1) from round_waveform_tohw incorrectly propagates as an error.
--

commit 90d2019e94496c6949f2d04c2605c6a197b16d6a
Author: Naushir Patuck <[email protected]>

pwm: rp1: Add RP1 PWM controller driver

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.

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +0 -0

new file mode 100644
index 0000000000000..052c3ae770569

[ ... ]

Quoted 14 lines

drivers/pwm/pwm-rp1.c new file +14 -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);
+	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;
+		return 0;

drivers/pwm/pwm-rp1.c line 147

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	}
+
+	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
+
+	/*
+	 * The period is limited to U32_MAX - 1
+	 * to allow 100% duty cycle.
+	 */
+	if (period_ticks > U32_MAX) {
+		period_ticks = U32_MAX - 1;

[Severity: Medium]
Does this limit check break monotonicity for large periods?

If the requested period translates to U32_MAX, this condition is false and the period becomes U32_MAX - 1.

If it translates to U32_MAX + 1, it becomes U32_MAX - 1, which is then subtracted by 1 later, yielding U32_MAX - 2. This makes a larger requested period result in a smaller actual hardware period.

drivers/pwm/pwm-rp1.c line 151

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +4 -0

+	} else if (!period_ticks) {
+		period_ticks = 2;
+		ret = 1;
+	}

[Severity: Medium]
Is returning a positive integer here intentional?

The PWM core subsystem expects round_waveform_tohw callbacks to return 0 on success or a negative error code on failure. Returning 1 could propagate as an error up to consumer drivers, breaking operation for very small periods.

Quoted 3 lines

drivers/pwm/pwm-rp1.c new file +2 -0

+
+	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);

[ ... ]

Quoted 7 lines

drivers/pwm/pwm-rp1.c new file +7 -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;

drivers/pwm/pwm-rp1.c line 190

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	u64 clk_rate = rp1->clk_rate;
+	u32 ticks;
+
+	*wf = (struct pwm_waveform){ };
+
+	if (!wfhw->enabled)
+		return 0;
+
+	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1) * NSEC_PER_SEC,
+						clk_rate);

[Severity: Medium]
Could this encounter a 32-bit integer overflow?

Since wfhw->period_ticks is a 32-bit integer, the addition of 1 happens in 32-bit space before the cast to u64.

If the hardware register was configured to 0xffffffff by the bootloader for a 100% duty cycle, 0xffffffff + 1 wraps around to 0, producing a period of 0.

Should the cast be placed before the addition?

drivers/pwm/pwm-rp1.c line 197

Quoted 8 lines

drivers/pwm/pwm-rp1.c new file +7 -0

+
+	if (!wfhw->inverted_polarity) {
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
+						      clk_rate);
+	} else {
+		ticks = wfhw->period_ticks + 1 - wfhw->duty_ticks;
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate);

[Severity: Medium]
Will this subtraction underflow if the hardware is configured for a 100% duty cycle?

The driver allows duty_ticks to exceed period_ticks + 1 to achieve a 100% duty cycle.

If inverted polarity is active and duty_ticks is strictly larger than period_ticks + 1, this unsigned 32-bit subtraction will wrap around to a massive integer, corrupting the duty_length_ns.

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+		wf->duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
+	}
+
+	return 0;
+}
+
+static int rp1_pwm_write_waveform(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const void *_wfhw)
+{

drivers/pwm/pwm-rp1.c line 217

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	const struct rp1_pwm_waveform *wfhw = _wfhw;
+	u32 value, ctrl;
+
+	/* early exit if disabled */
+	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
+	if (!wfhw->enabled) {
+		ctrl &= ~RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
+		goto exit_disable;
+	}

[Severity: High]
Does skipping the polarity update here cause the output to erroneously remain HIGH while disabled?

The early exit skips the read-modify-write block that updates RP1_PWM_CHAN_CTRL_POLARITY.

The driver notes state that when disabled, the output is driven to 1 if polarity is inversed. If a channel previously configured with inverted polarity is disabled, it seems it will continually output a HIGH signal, violating the PWM framework expectation.

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +3 -0

+
+	/* set period and duty cycle */
+	regmap_write(rp1->regmap,

[ ... ]

Quoted 2 lines

drivers/pwm/pwm-rp1.c new file +2 -0

+static int rp1_pwm_probe(struct platform_device *pdev)
+{

[ ... ]

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +5 -0

+	chip->ops = &rp1_pwm_ops;
+
+	platform_set_drvdata(pdev, chip);
+
+	ret = pwmchip_add(chip);

drivers/pwm/pwm-rp1.c line 346

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to register PWM chip\n");
+		goto err_disable_clk;
+	}
+
+	ret = of_syscon_register_regmap(np, rp1->regmap);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to register syscon\n");
+		goto err_disable_clk;
+	}

[Severity: High]
Is there a missing call to pwmchip_remove() on this error path?

If of_syscon_register_regmap() fails, the function jumps to err_disable_clk and returns an error.

Since the chip was allocated with devm_pwmchip_alloc(), devres will free the memory, but the PWM subsystem will retain pointers to the freed chip in its IDR mapping, potentially leading to a use-after-free.

[Severity: High]
Also, does registering a devm-managed regmap with the permanent syscon registry create a teardown asymmetry?

The regmap is allocated with devm_regmap_init_mmio(). If the underlying device is hotplug removed, devres will automatically free the regmap memory.

However, the syscon framework does not provide an unregister function, leaving it with a dangling pointer in its static list that could cause a use-after-free upon subsequent lookups.

Quoted 9 lines

drivers/pwm/pwm-rp1.c new file +8 -0

+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(rp1->clk);
+
+	return ret;
+}

[ ... ]

Quoted 6 lines

drivers/pwm/pwm-rp1.c new file +6 -0

+static int rp1_pwm_resume(struct device *dev)
+{
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	int ret;
+

drivers/pwm/pwm-rp1.c line 397

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	ret = clk_prepare_enable(rp1->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock on resume: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	rp1->clk_enabled = true;
+
+	return 0;
+}

[Severity: High]
Should the hardware channel control registers (CHAN_CTRL) be restored across resume?

When the system suspends, the southbridge may lose power and reset its registers. During probe, these registers are initialized with required configuration bits like MODE_TE_MS and FIFO_POP_MASK.

This resume function merely re-enables the clock. Because the regmap is not configured to cache and restore registers, a subsequent write might update only the polarity bit, leaving the channel mode at 0 and breaking PWM output.

Signature · 2 lines

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Quoted 54 lines · On 15:47 Wed 03 Jun , [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Missing pwmchip_remove() on the of_syscon_register_regmap() error path leaves a dangling PWM chip.
- [High] When disabling a PWM channel, the driver fails to clear the polarity bit, causing the output to erroneously remain HIGH if it was previously inverted.
- [High] Registering a devm-managed regmap with the permanent syscon registry creates a teardown asymmetry.
- [High] Hardware channel control registers (CHAN_CTRL) are not restored across system suspend/resume.
- [Medium] Non-monotonic period limit checking in rp1_pwm_round_waveform_tohw() causes larger requested periods to result in smaller hardware periods.
- [Medium] Calculating period_length_ns from hardware state suffers from a 32-bit integer overflow before casting to u64, resulting in a zero period.
- [Medium] Calculating inverted polarity duty length underflows if the hardware is configured for a 100% duty cycle.
- [Medium] Returning a positive integer (1) from round_waveform_tohw incorrectly propagates as an error.
--

commit 90d2019e94496c6949f2d04c2605c6a197b16d6a
Author: Naushir Patuck <[email protected]>

pwm: rp1: Add RP1 PWM controller driver

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.

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +0 -0

new file mode 100644
index 0000000000000..052c3ae770569

[ ... ]

Quoted 14 lines

drivers/pwm/pwm-rp1.c new file +14 -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);
+	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;
+		return 0;

drivers/pwm/pwm-rp1.c line 147

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	}
+
+	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
+
+	/*
+	 * The period is limited to U32_MAX - 1
+	 * to allow 100% duty cycle.
+	 */
+	if (period_ticks > U32_MAX) {
+		period_ticks = U32_MAX - 1;

[Severity: Medium]
Does this limit check break monotonicity for large periods?

Quoted 6 lines

If the requested period translates to U32_MAX, this condition is false and the period becomes U32_MAX - 1.

If it translates to U32_MAX + 1, it becomes U32_MAX - 1, which is then subtracted by 1 later, yielding U32_MAX - 2. This makes a larger requested period result in a smaller actual hardware period.

Ack. I will set period_ticks = U32_MAX in that if.

Quoted 9 lines

drivers/pwm/pwm-rp1.c line 151

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +4 -0

+	} else if (!period_ticks) {
+		period_ticks = 2;
+		ret = 1;
+	}

[Severity: Medium]
Is returning a positive integer here intentional?

Quoted 3 lines

The PWM core subsystem expects round_waveform_tohw callbacks to return 0 on success or a negative error code on failure. Returning 1 could propagate as an error up to consumer drivers, breaking operation for very small periods.

It should return 1 in case the if any value is rounded, which in this case is the period.

Quoted 23 lines

Quoted 1 line

drivers/pwm/pwm-rp1.c new file +1 -0

+	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);

[ ... ]

Quoted 7 lines

drivers/pwm/pwm-rp1.c new file +7 -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;

drivers/pwm/pwm-rp1.c line 190

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	u64 clk_rate = rp1->clk_rate;
+	u32 ticks;
+
+	*wf = (struct pwm_waveform){ };
+
+	if (!wfhw->enabled)
+		return 0;
+
+	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1) * NSEC_PER_SEC,
+						clk_rate);

[Severity: Medium]
Could this encounter a 32-bit integer overflow?

Quoted 7 lines

Since wfhw->period_ticks is a 32-bit integer, the addition of 1 happens in 32-bit space before the cast to u64.

If the hardware register was configured to 0xffffffff by the bootloader for a 100% duty cycle, 0xffffffff + 1 wraps around to 0, producing a period of 0.

Should the cast be placed before the addition?

Ack. 0xffffffff should be deemed as a non valid value though.

Quoted 13 lines

drivers/pwm/pwm-rp1.c line 197

Quoted 8 lines

drivers/pwm/pwm-rp1.c new file +7 -0

+
+	if (!wfhw->inverted_polarity) {
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
+						      clk_rate);
+	} else {
+		ticks = wfhw->period_ticks + 1 - wfhw->duty_ticks;
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate);

[Severity: Medium]
Will this subtraction underflow if the hardware is configured for a 100% duty cycle?

Quoted 2 lines

The driver allows duty_ticks to exceed period_ticks + 1 to achieve a 100% duty cycle.

The driver is not allowing this since tohw() has this:

duty_ticks = min(duty_ticks, period_ticks);

Anyway, if fromhw() is called first and the bootloader has configured 0xffffffff in the period register, should it returns an error? The period should always be at most U32_MAX-1.

Quoted 28 lines

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+		wf->duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
+	}
+
+	return 0;
+}
+
+static int rp1_pwm_write_waveform(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const void *_wfhw)
+{

drivers/pwm/pwm-rp1.c line 217

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	const struct rp1_pwm_waveform *wfhw = _wfhw;
+	u32 value, ctrl;
+
+	/* early exit if disabled */
+	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
+	if (!wfhw->enabled) {
+		ctrl &= ~RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
+		goto exit_disable;
+	}

[Severity: High]
Does skipping the polarity update here cause the output to erroneously remain HIGH while disabled?

The early exit skips the read-modify-write block that updates RP1_PWM_CHAN_CTRL_POLARITY.

Quoted 4 lines

The driver notes state that when disabled, the output is driven to 1 if polarity is inversed. If a channel previously configured with inverted polarity is disabled, it seems it will continually output a HIGH signal, violating the PWM framework expectation.

I'm not sure whether an inverted polarity pin shoudl stay low when disabled. After all, the inactive state for a reversed pin is high.

Quoted 7 lines

[Severity: High]
Also, does registering a devm-managed regmap with the permanent syscon registry create a teardown asymmetry?

The regmap is allocated with devm_regmap_init_mmio(). If the underlying device is hotplug removed, devres will automatically free the regmap memory.

Quoted 3 lines

However, the syscon framework does not provide an unregister function, leaving it with a dangling pointer in its static list that could cause a use-after-free upon subsequent lookups.

The device is not hotpluggable and the driver cannot be unloaded/unbounded, so there should be no dangling pointer around.

Hello Andrea,

Quoted 3 lines · Andrea della Porta

Quoted 2 lines · On 15:47 Wed 03 Jun , [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Missing pwmchip_remove() on the of_syscon_register_regmap() error path leaves a dangling PWM chip.

Quoted 50 lines

Quoted 28 lines

Quoted 14 lines

drivers/pwm/pwm-rp1.c new file +14 -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);
+	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;
+		return 0;

drivers/pwm/pwm-rp1.c line 147

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	}
+
+	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
+
+	/*
+	 * The period is limited to U32_MAX - 1
+	 * to allow 100% duty cycle.
+	 */
+	if (period_ticks > U32_MAX) {
+		period_ticks = U32_MAX - 1;

[Severity: Medium]
Does this limit check break monotonicity for large periods?

Quoted 6 lines

If the requested period translates to U32_MAX, this condition is false and the period becomes U32_MAX - 1.

If it translates to U32_MAX + 1, it becomes U32_MAX - 1, which is then subtracted by 1 later, yielding U32_MAX - 2. This makes a larger requested period result in a smaller actual hardware period.

Ack. I will set period_ticks = U32_MAX in that if.

Quoted 12 lines

drivers/pwm/pwm-rp1.c line 151

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +4 -0

+	} else if (!period_ticks) {
+		period_ticks = 2;
+		ret = 1;
+	}

[Severity: Medium]
Is returning a positive integer here intentional?

The PWM core subsystem expects round_waveform_tohw callbacks to return 0 on success or a negative error code on failure. Returning 1 could propagate as an error up to consumer drivers, breaking operation for very small periods.

Quoted 2 lines

It should return 1 in case the if any value is rounded, which in this case is the period.

Ack. However it's a bit strange that you fixup period_ticks = 0 to 2 while 1 is kept as is. Either using 1 is invalid, then it should be updated to two, too. Or it gives a shorter period than 2, then 0 should be fixed up to 1 only.

Quoted 54 lines

Quoted 24 lines

Quoted 2 lines

drivers/pwm/pwm-rp1.c new file +1 -0

+	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);

[ ... ]

Quoted 7 lines

drivers/pwm/pwm-rp1.c new file +7 -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;

drivers/pwm/pwm-rp1.c line 190

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	u64 clk_rate = rp1->clk_rate;
+	u32 ticks;
+
+	*wf = (struct pwm_waveform){ };
+
+	if (!wfhw->enabled)
+		return 0;
+
+	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1) * NSEC_PER_SEC,
+						clk_rate);

[Severity: Medium]
Could this encounter a 32-bit integer overflow?

Quoted 7 lines

Since wfhw->period_ticks is a 32-bit integer, the addition of 1 happens in 32-bit space before the cast to u64.

If the hardware register was configured to 0xffffffff by the bootloader for a 100% duty cycle, 0xffffffff + 1 wraps around to 0, producing a period of 0.

Should the cast be placed before the addition?

Ack. 0xffffffff should be deemed as a non valid value though.

Quoted 13 lines

drivers/pwm/pwm-rp1.c line 197

Quoted 8 lines

drivers/pwm/pwm-rp1.c new file +7 -0

+
+	if (!wfhw->inverted_polarity) {
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
+						      clk_rate);
+	} else {
+		ticks = wfhw->period_ticks + 1 - wfhw->duty_ticks;
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate);

[Severity: Medium]
Will this subtraction underflow if the hardware is configured for a 100% duty cycle?

Quoted 2 lines

The driver allows duty_ticks to exceed period_ticks + 1 to achieve a 100% duty cycle.

The driver is not allowing this since tohw() has this:

duty_ticks = min(duty_ticks, period_ticks);

Quoted 3 lines

Anyway, if fromhw() is called first and the bootloader has configured 0xffffffff in the period register, should it returns an error? The period should always be at most U32_MAX-1.

I'm not sure if this triggers a PWM_DEBUG check, but I'd say the right thing is to let fromhw() report what the actual setting even if tohw() will never result in that setting.

Thinking again, the tohw() callback could be a bit more clever and also use period_ticks = 0xffffffff, as this fine if duty_ticks is less than this value and if duty_ticks = period_ticks = 0xffffffff you can still configure the hardware using period_ticks = 0xfffffffe to achieve the 100% relative dutycycle. (But keeping the current behaviour is fine for me, too.)

Quoted 29 lines

Quoted 29 lines

Quoted 11 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+		wf->duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
+	}
+
+	return 0;
+}
+
+static int rp1_pwm_write_waveform(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const void *_wfhw)
+{

drivers/pwm/pwm-rp1.c line 217

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	const struct rp1_pwm_waveform *wfhw = _wfhw;
+	u32 value, ctrl;
+
+	/* early exit if disabled */
+	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
+	if (!wfhw->enabled) {
+		ctrl &= ~RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
+		goto exit_disable;
+	}

[Severity: High]
Does skipping the polarity update here cause the output to erroneously remain HIGH while disabled?

The early exit skips the read-modify-write block that updates RP1_PWM_CHAN_CTRL_POLARITY.

Quoted 7 lines

Quoted 4 lines

The driver notes state that when disabled, the output is driven to 1 if polarity is inversed. If a channel previously configured with inverted polarity is disabled, it seems it will continually output a HIGH signal, violating the PWM framework expectation.

I'm not sure whether an inverted polarity pin shoudl stay low when disabled. After all, the inactive state for a reversed pin is high.

Sashiko's concern is correctly stated, if you go from

polarity = inversed, enabled

to

polarity = normal, disabled

the output stays high, which is active for polarity = normal.

However the behaviour of a disabled PWM isn't specified, so any behaviour is fine, the only objective is to save power. And if the consumer relies on a constant inactive output, it's supposed to not disable it.

For me both behaviours are fine. Making the hardware emit the inactive level might prevent a surprise if the consumer isn't aware of the missing guarantee, but being lazy and so surprise the consumer is also fine as this might uncover that wrong assumption and allow the consumer to be fixed.

(And not all PWM implementations allow to configure the output level, so a guarantee cannot be given. Some go to 0 irrespective of the configured polarity, some go to High-Z.)

Quoted 8 lines

Quoted 8 lines

[Severity: High]
Also, does registering a devm-managed regmap with the permanent syscon registry create a teardown asymmetry?

The regmap is allocated with devm_regmap_init_mmio(). If the underlying device is hotplug removed, devres will automatically free the regmap memory.

Quoted 6 lines

Quoted 3 lines

However, the syscon framework does not provide an unregister function, leaving it with a dangling pointer in its static list that could cause a use-after-free upon subsequent lookups.

The device is not hotpluggable and the driver cannot be unloaded/unbounded, so there should be no dangling pointer around.

ack.

Best regards
Uwe

Quoted 6 lines · Uwe Kleine-König

Thinking again, the tohw() callback could be a bit more clever and also use period_ticks = 0xffffffff, as this fine if duty_ticks is less than this value and if duty_ticks = period_ticks = 0xffffffff you can still configure the hardware using period_ticks = 0xfffffffe to achieve the 100% relative dutycycle. (But keeping the current behaviour is fine for me, too.)

I think this is what it's currently doing in this patch iteration. I stand corrected here when I said that period_ticks should be at max U32_MAX-1: logically it can be U32_MAX but it's getting 'translated' into U32_MAX-1 as the value which is fed to the register. So the period is still U32_MAX, accounting for the extra tick at the end. So I guess we're on the same page.

Quoted 7 lines

Quoted 2 lines

I'm not sure whether an inverted polarity pin shoudl stay low when disabled. After all, the inactive state for a reversed pin is high.

Sashiko's concern is correctly stated, if you go from

polarity = inversed, enabled

Quoted 5 lines

to

polarity = normal, disabled

the output stays high, which is active for polarity = normal.

Ack. I'll set the polarity first so there will be no uncovered corner case.

Quoted 11 lines

However the behaviour of a disabled PWM isn't specified, so any behaviour is fine, the only objective is to save power. And if the consumer relies on a constant inactive output, it's supposed to not disable it.

For me both behaviours are fine. Making the hardware emit the inactive level might prevent a surprise if the consumer isn't aware of the missing guarantee, but being lazy and so surprise the consumer is also fine as this might uncover that wrong assumption and allow the consumer to be fixed.

Quoted 3 lines

(And not all PWM implementations allow to configure the output level, so a guarantee cannot be given. Some go to 0 irrespective of the configured polarity, some go to High-Z.)

I was just curious about sashiko saying it's violating the pwm framework expectations, while according to your words it seems there's no constraints.

BTW, I've tried to install sashiko and use it on my patches but it's obvious that some custom settings are in order, since all I can get is some error aborting the review after 3 attempts. Any chance you can share your Settings.toml or any customization so I can test it in advance before submitting the new patchset or do you recommend just throwing the new V5 at your script?

Hello Andrea,

Quoted 8 lines · Andrea della Porta

Quoted 6 lines · Uwe Kleine-König

Thinking again, the tohw() callback could be a bit more clever and also use period_ticks = 0xffffffff, as this fine if duty_ticks is less than this value and if duty_ticks = period_ticks = 0xffffffff you can still configure the hardware using period_ticks = 0xfffffffe to achieve the 100% relative dutycycle. (But keeping the current behaviour is fine for me, too.)

Quoted 5 lines

I think this is what it's currently doing in this patch iteration. I stand corrected here when I said that period_ticks should be at max U32_MAX-1: logically it can be U32_MAX but it's getting 'translated' into U32_MAX-1 as the value which is fed to the register. So the period is still U32_MAX, accounting for the extra tick at the end. So I guess we're on the same page.

Not sure you got what I wrote. You can use the maximal value possible in hardware, because for all but 100% relative duty cycle it works as expected. You only need to map 100% relative duty cycle with that maximal period to a different 100% relative duty cycle setting. I think PWM_DEBUG even doesn't yell at you for that. (And if it does, that needs fixing.)

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

I'm not sure whether an inverted polarity pin shoudl stay low when disabled. After all, the inactive state for a reversed pin is high.

Sashiko's concern is correctly stated, if you go from

polarity = inversed, enabled

to

Quoted 6 lines

Quoted 3 lines

polarity = normal, disabled

the output stays high, which is active for polarity = normal.

Ack. I'll set the polarity first so there will be no uncovered corner case.

You can, but as I wrote below, being lazy is also fine.

Quoted 12 lines

Quoted 12 lines

However the behaviour of a disabled PWM isn't specified, so any behaviour is fine, the only objective is to save power. And if the consumer relies on a constant inactive output, it's supposed to not disable it.

For me both behaviours are fine. Making the hardware emit the inactive level might prevent a surprise if the consumer isn't aware of the missing guarantee, but being lazy and so surprise the consumer is also fine as this might uncover that wrong assumption and allow the consumer to be fixed.

Quoted 6 lines

Quoted 3 lines

(And not all PWM implementations allow to configure the output level, so a guarantee cannot be given. Some go to 0 irrespective of the configured polarity, some go to High-Z.)

I was just curious about sashiko saying it's violating the pwm framework expectations, while according to your words it seems there's no constraints.

In doubt trust me :-)

Quoted 6 lines

BTW, I've tried to install sashiko and use it on my patches but it's obvious that some custom settings are in order, since all I can get is some error aborting the review after 3 attempts. Any chance you can share your Settings.toml or any customization so I can test it in advance before submitting the new patchset or do you recommend just throwing the new V5 at your script?

Note that sashiko is not "my script", it was setup by Google engineers and I have nothing to do with it (apart from benefitting from its review feedback). Kevin (added to Cc) tried to setup a local instance with his Claude plan, but (IIUC) it quickly ate his day's amount of tokens before completing review of a series of only 4 patches.

So without knowing the size of Kevin's or your AI plan, probably it's easier to just rely on the public instance. (And IMHO that's nothing to be afraid of, just handle it like a human reviewer. For these you also don't know what they will reply for your next revision.)

Best regards
Uwe