Re: [PATCH 2/2] pwm: Add clock based PWM output driver

[Uwe Kleine-König]

Hello,

On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote:
> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
> +
> +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip);
> +	int ret;
> +	u32 rate;
> +
> +	if (!state->enabled && !pwm->state.enabled)
> +		return 0;
> +
> +	if (state->enabled && !pwm->state.enabled) {
> +		ret = clk_enable(chip->clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!state->enabled && pwm->state.enabled) {
> +		clk_disable(chip->clk);
> +		return 0;
> +	}

This can be written a bit more compact as:

	if (!state->enabled) {
		if (pwm->state.enabled)
			clk_disable(chip->clk);
		return 0;
	} else if (!pwm->state.enabled) {
		ret = clk_enable(chip->clk);
		if (ret)
			return ret;
	}

personally I find my version also easier to read, but that might be
subjective.

Missing handling for polarity. Either refuse inverted polarity, or set
the duty_cycle to state->period - state->duty_cycle in the inverted
case.

> +	rate = div64_u64(NSEC_PER_SEC, state->period);

Please round up here, as .apply() should never implement a period bigger
than requested. This also automatically improves the behaviour if
state->period > NSEC_PER_SEC.

> +	ret = clk_set_rate(chip->clk, rate);
> +	if (ret)
> +		return ret;
> +
> +	return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period);

Is it possible to enable only after the duty cycle is set? This way we
could prevent in some cases that a wrong setting makes it to the output.

As there is not a single function to set rate (i.e. period) and
duty_cycle it's not possible to prevent all glitches.

Can you please note that in a paragraph at the beginning of the driver
as does e.g. drivers/pwm/pwm-sl28cpld.c. (Please stick to the format,
i.e.  "Limitations:" and then all items without an empty line, to make
this greppable.)

> +}
> +
> +static const struct pwm_ops pwm_clk_ops = {
> +	.apply = pwm_clk_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int pwm_clk_probe(struct platform_device *pdev)
> +{
> +	struct pwm_clk_chip *chip;
> +	int ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(chip->clk)) {
> +		dev_err(&pdev->dev, "Failed to get clock: %ld\n", PTR_ERR(chip->clk));
> +		return PTR_ERR(chip->clk);

Please use dev_err_probe() here and in the other error paths below.

> +	}
> +
> +	chip->chip.dev = &pdev->dev;
> +	chip->chip.ops = &pwm_clk_ops;
> +	chip->chip.of_xlate = of_pwm_xlate_with_flags;
> +	chip->chip.of_pwm_n_cells = 2;
> +	chip->chip.base = 0;

Please drop this line (see commit f9a8ee8c8bcd)

> +	chip->chip.npwm = 1;
> +
> +	ret = clk_prepare(chip->clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to prepare clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to add pwm chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, chip);
> +	return 0;
> +}
> +
> +static int pwm_clk_remove(struct platform_device *pdev)
> +{
> +	struct pwm_clk_chip *chip = platform_get_drvdata(pdev);
> +
> +	clk_unprepare(chip->clk);
> +
> +	pwmchip_remove(&chip->chip);

This is bad. clk_unprepare() stops the output which must not happen
before pwmchip_remove() returns. What happens if the PWM (and so the
clk) is still on and you call clk_unprepare? Is this allowed at all if
the enable count might be > 0?

> +	return 0;
> +}
> +
> +static const struct of_device_id pwm_clk_dt_ids[] = {
> +	{ .compatible = "clk-pwm", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pwm_clk_dt_ids);
> +
> +static struct platform_driver pwm_clk_driver = {
> +	.driver = {
> +		.name = "clk-pwm",

Hmm, is this name sane? I would have expected that a driver called
"clk-pwm" exposes a clk using a PWM. OTOH there is a "pwm-clock" driver
that does exactly that. To complete the confusion the function prefix of
said driver is clk_pwm_ and this one used pwm_clk_ ...

> +		.of_match_table = pwm_clk_dt_ids,
> +	},
> +	.probe = pwm_clk_probe,
> +	.remove = pwm_clk_remove,
> +};
> +module_platform_driver(pwm_clk_driver);
> +
> +MODULE_ALIAS("platform:clk-pwm");
> +MODULE_AUTHOR("Nikita Travkin <nikita@xxxxxxx>");
> +MODULE_DESCRIPTION("Clock based PWM driver");
> +MODULE_LICENSE("GPL v2");

MODULE_LICENSE("GPL");

is the more usual today (and has the same meaning).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature