Hello Nicolas,

On Mon, Jun 02, 2025 at 06:19:16PM +0200, Nicolas Frattaroli wrote:

+/**
+ * rockchip_pwm_v4_round_params - convert PWM parameters to hardware
+ * @rate: PWM clock rate to do the calculations at
+ * @duty: PWM duty cycle in nanoseconds
+ * @period: PWM period in nanoseconds
+ * @offset: PWM offset in nanoseconds
+ * @out_duty: pointer to where the rounded duty value should be stored
+ * @out_period: pointer to where the rounded period value should be stored
+ * @out_offset: pointer to where the rounded offset value should be stored
+ *
+ * Convert nanosecond-based duty/period/offset parameters to the PWM hardware's
+ * native rounded representation in number of cycles at clock rate @rate. Should
+ * any of the input parameters be out of range for the hardware, the
+ * corresponding output parameter is the maximum permissible value for said
+ * parameter with considerations to the others.
+ */
+static void rockchip_pwm_v4_round_params(unsigned long rate, u64 duty,
+					u64 period, u64 offset, u32 *out_duty,
+					u32 *out_period, u32 *out_offset)
+{
+	int ret;
+
+	ret = rockchip_pwm_v4_round_single(rate, period, out_period);
+	if (ret)
+		*out_period = U32_MAX;

It's strange to let rockchip_pwm_v4_round_single return failure just to
reset it to U32_MAX here then. I'd make rockchip_pwm_v4_round_single do:

	tmp = mul_u64_u64_div_u64(rate, in_val, NSEC_PER_SEC);
	if (tmp > U32_MAX)
		return U32_MAX
	return tmp;

and then just do

	*out_period = rockchip_pwm_v4_round_single(rate, period);
	*out_duty = rockchip_pwm_v4_round_single(rate, duty)
	...

+
+	ret = rockchip_pwm_v4_round_single(rate, duty, out_duty);
+	if (ret || *out_duty > *out_period)
+		*out_duty = *out_period;

You can assume that .round_wf_tohw() is called only with duty <= period.

+	ret = rockchip_pwm_v4_round_single(rate, offset, out_offset);
+	if (ret || *out_offset > (*out_period - *out_duty))
+		*out_offset = *out_period - *out_duty;

Is this a hardware limitation? In general

	.period_length_ns = 1000
	.duty_length_ns = 600
	.duty_offset_ns = 600

is a valid waveform.

+}
+
+static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip,
+					 struct pwm_device *pwm,
+					 const struct pwm_waveform *wf,
+					 void *_wfhw)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	unsigned long rate;
+	int ret;
+
+	/* We do not want chosen_clk to change out from under us here */
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = clk_get_rate(pc->pwmf->core);
+
+	rockchip_pwm_v4_round_params(rate, wf->duty_length_ns,
+				     wf->period_length_ns, wf->duty_offset_ns,
+				     &wfhw->duty, &wfhw->period, &wfhw->offset);
+
+	if (wf->period_length_ns > 0)
+		wfhw->enable = PWMV4_EN_BOTH_MASK;
+	else
+		wfhw->enable = 0;
+
+	dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n",
+		wfhw->duty, wfhw->period, wfhw->offset, rate);

This is more helpful if the input parameters (i.e. wf) is also emitted.

+	mfpwm_release(pc->pwmf);
+	return 0;
+}
+
+static int rockchip_pwm_v4_round_wf_fromhw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const void *_wfhw,
+					   struct pwm_waveform *wf)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	const struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	unsigned long rate;
+	int ret = 0;
+
+	/* We do not want chosen_clk to change out from under us here */
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;

Hmm, there is little gain here. Correct me if I'm wrong, but you prevent
a rate change only until mfpwm_release() is called, so the assertion
ends before the caller can use the calculated parameters anyhow. So
maybe drop the acquire/release pair?

+	rate = clk_get_rate(pc->pwmf->core);
+
+	if (rockchip_pwm_v4_is_enabled(wfhw->enable)) {
+		if (!rate) {
+			ret = -EINVAL;
+			goto out_mfpwm_release;
+		}
+		wf->period_length_ns = mul_u64_u64_div_u64(wfhw->period, NSEC_PER_SEC, rate);

(u64)wfhw->period * NSEC_PER_SEC cannot overflow, so a plain
multiplication and then a division is cheaper here.

+		wf->duty_length_ns = mul_u64_u64_div_u64(wfhw->duty, NSEC_PER_SEC, rate);
+		wf->duty_offset_ns = mul_u64_u64_div_u64(wfhw->offset, NSEC_PER_SEC, rate);
+	} else {
+		wf->period_length_ns = 0;
+		wf->duty_length_ns = 0;
+		wf->duty_offset_ns = 0;
+	}
+
+	dev_dbg(&chip->dev, "fromhw: duty = %llu, period = %llu, offset = %llu, rate = %lu\n",
+		wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, rate);

As above, please include wfhw in the output.

+out_mfpwm_release:
+	mfpwm_release(pc->pwmf);
+	return ret;
+}
+
[...]
+static int rockchip_pwm_v4_probe(struct platform_device *pdev)
+{
+	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
+	struct rockchip_pwm_v4 *pc;
+	struct pwm_chip *chip;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, 1, sizeof(*pc));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	pc = to_rockchip_pwm_v4(chip);
+	pc->pwmf = pwmf;
+
+	ret = mfpwm_acquire(pwmf);
+	if (ret == -EBUSY)
+		dev_warn(dev, "pwm hardware already in use, can't check initial state\n");
+	else if (ret < 0)
+		return dev_err_probe(dev, ret, "couldn't acquire mfpwm in probe\n");
+
+	if (!rockchip_pwm_v4_on_and_continuous(pc))
+		mfpwm_release(pwmf);
+	else {
+		dev_dbg(dev, "pwm was already on at probe time\n");
+		ret = clk_enable(pwmf->core);
+		if (ret)
+			return dev_err_probe(dev, ret, "enabling pwm clock failed\n");
+		ret = clk_rate_exclusive_get(pc->pwmf->core);
+		if (ret) {
+			clk_disable(pwmf->core);
+			return dev_err_probe(dev, ret, "protecting pwm clock failed\n");
+		}
+	}
+
+	platform_set_drvdata(pdev, chip);
+
+	chip->ops = &rockchip_pwm_v4_ops;
+
+	ret = pwmchip_add(chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add PWM chip\n");

I like error messages starting with a capital letter.

+
+	return 0;
+}
+
+static void rockchip_pwm_v4_remove(struct platform_device *pdev)
+{
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+
+	mfpwm_remove_func(pc->pwmf);

What does this function do? It is not used in .probe()'s error path?!

+	pwmchip_remove(chip);

Wrong order (I think). If mfpwm_remove_func() affects operation,
pwmchip_remove() must be called first.

+}
+
+static const struct platform_device_id rockchip_pwm_v4_ids[] = {
+	{ .name = "pwm-rockchip-v4", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, rockchip_pwm_v4_ids);
+
+static struct platform_driver rockchip_pwm_v4_driver = {
+	.probe = rockchip_pwm_v4_probe,
+	.remove = rockchip_pwm_v4_remove,
+	.driver = {
+		.name = "pwm-rockchip-v4",
+	},
+	.id_table = rockchip_pwm_v4_ids,
+};
+module_platform_driver(rockchip_pwm_v4_driver);
+
+MODULE_AUTHOR("Nicolas Frattaroli [off-list ref]");
+MODULE_DESCRIPTION("Rockchip PWMv4 Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("ROCKCHIP_MFPWM");

Best regards
Uwe