Message-ID: <o5xaoorrqb6a3jwwpdcsowrqbo7owrjzwj4r3laytusms6txdi@ku4bvbynkwh7>
Date: Mon, 23 Jun 2025 10:44:57 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>, 
	William Breathitt Gray <wbg@...nel.org>, Sebastian Reichel <sebastian.reichel@...labora.com>, 
	Kever Yang <kever.yang@...k-chips.com>, Yury Norov <yury.norov@...il.com>, 
	Rasmus Villemoes <linux@...musvillemoes.dk>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Dave Ertman <david.m.ertman@...el.com>, Ira Weiny <ira.weiny@...el.com>, 
	Leon Romanovsky <leon@...nel.org>, Lee Jones <lee@...nel.org>, linux-gpio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	linux-iio@...r.kernel.org, kernel@...labora.com, Jonas Karlman <jonas@...boo.se>, 
	Detlev Casanova <detlev.casanova@...labora.com>
Subject: Re: [PATCH v2 5/7] pwm: Add rockchip PWMv4 driver

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

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists