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
