Re: [PATCH v2 5/7] pwm: Add rockchip PWMv4 driver

Uwe Kleine-König
2025-06-23T08:44:59Z

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

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 <nicolas.frattaroli@collabora.com>");
> +MODULE_DESCRIPTION("Rockchip PWMv4 Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("ROCKCHIP_MFPWM");

Best regards
Uwe

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