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 [off-list ref]");
+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/ |