[PATCH 0/2] Clock based PWM output driver

[PATCH 0/2] Clock based PWM output driver

[Nikita Travkin]

This series introduces an "adapter" driver that allows PWM consumers
to control clock outputs with duty-cycle control.

Some platforms (e.g. some Qualcomm chipsets) have "General Purpose"
clocks that can be muxed to GPIO outputs and used as PWM outputs. 
Those outputs may be connected to various peripherals such as
leds in display backlight or haptic feedback motor driver. 

To avoid re-implementing every single PWM consumer driver with clk
support (like in [1]) and don't put the burden of providing the PWM
sources on the clock drivers (as was proposed in [2]), clk based
pwm controller driver is introduced.

There is an existing driver that provides the opposite function
in drivers/clk/clk-pwm.c with a compatible "pwm-clock" so the new
driver uses the opposite naming scheme: drivers/pwm/pwm-clk.c
and compatible "clk-pwm". 

[1] - https://lore.kernel.org/lkml/20191205002503.13088-1-masneyb@onstation.org/
[2] - https://lore.kernel.org/lkml/CACRpkdZxu1LfK11OHEx5L_4kyjMZ7qERpvDzFj5u3Pk2kD1qRA@mail.gmail.com/

Nikita Travkin (2):
  dt-bindings: pwm: Document clk based PWM controller
  pwm: Add clock based PWM output driver

 .../devicetree/bindings/pwm/clk-pwm.yaml      |  45 +++++++
 drivers/pwm/Kconfig                           |  10 ++
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-clk.c                         | 119 ++++++++++++++++++
 4 files changed, 175 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/clk-pwm.yaml
 create mode 100644 drivers/pwm/pwm-clk.c

-- 
2.30.2

[PATCH 1/2] dt-bindings: pwm: Document clk based PWM controller

[Nikita Travkin]

Add YAML devicetree binding for clk based PWM controller

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
 .../devicetree/bindings/pwm/clk-pwm.yaml      | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/clk-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/clk-pwm.yaml b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
new file mode 100644
index 000000000000..2b2801d0278f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Clock based PWM controller
+
+maintainers:
+  - Nikita Travkin <nikita@trvn.ru>
+
+description: |
+  Some systems have clocks that can be exposed to external devices.
+  (e.g. by muxing them to GPIO pins)
+  It's often possible to control duty-cycle of such clocks which makes them
+  suitable for generating PWM signal.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: clk-pwm
+
+  clocks:
+    description: Clock used to generate the signal.
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+unevaluatedProperties: false
+
+required:
+  - clocks
+
+examples:
+  - |
+    pwm-flash {
+      compatible = "clk-pwm";
+      #pwm-cells = <2>;
+      clocks = <&gcc 0>;
+      pinctrl-names = "default";
+      pinctrl-0 = <&pwm_clk_flash_default>;
+    };
-- 
2.30.2

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

[Nikita Travkin]

Some systems have clocks exposed to external devices. If the clock
controller supports duty-cycle configuration, such clocks can be used as
pwm outputs. In fact PWM and CLK subsystems are interfaced with in a
similar way and an "opposite" driver already exists (clk-pwm). Add a
driver that would enable pwm devices to be used via clk subsystem.

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
 drivers/pwm/Kconfig   |  10 ++++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-clk.c | 119 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+)
 create mode 100644 drivers/pwm/pwm-clk.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 21e3b05a5153..daa2491a4054 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -140,6 +140,16 @@ config PWM_BRCMSTB
 	  To compile this driver as a module, choose M Here: the module
 	  will be called pwm-brcmstb.c.
 
+config PWM_CLK
+	tristate "Clock based PWM support"
+	depends on HAVE_CLK || COMPILE_TEST
+	help
+	  Generic PWM framework driver for outputs that can be
+	  muxed to clocks.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-clk.
+
 config PWM_CLPS711X
 	tristate "CLPS711X PWM support"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 708840b7fba8..4a860103c470 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
 obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
 obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
+obj-$(CONFIG_PWM_CLK)		+= pwm-clk.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c
new file mode 100644
index 000000000000..11c156529761
--- /dev/null
+++ b/drivers/pwm/pwm-clk.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+struct pwm_clk_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+};
+
+#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;
+	}
+
+	rate = div64_u64(NSEC_PER_SEC, state->period);
+	ret = clk_set_rate(chip->clk, rate);
+	if (ret)
+		return ret;
+
+	return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period);
+}
+
+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);
+	}
+
+	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;
+	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);
+	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",
+		.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@trvn.ru>");
+MODULE_DESCRIPTION("Clock based PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH 1/2] dt-bindings: pwm: Document clk based PWM controller

[Rob Herring]

On Thu, 09 Dec 2021 21:20:19 +0500, Nikita Travkin wrote:
> Add YAML devicetree binding for clk based PWM controller
> 
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> ---
>  .../devicetree/bindings/pwm/clk-pwm.yaml      | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/clk-pwm.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/pwm/clk-pwm.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/pwm/clk-pwm.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/clk-pwm.example.dt.yaml: pwm-flash: $nodename:0: 'pwm-flash' does not match '^pwm(@.*|-[0-9a-f])*$'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/clk-pwm.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1565817

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Uwe Kleine-König]

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

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@trvn.ru>");
> +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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Nikita Travkin]

Hi,

Thanks for the review!

Uwe Kleine-König писал(а) 10.12.2021 03:05:
> 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.
> 

Having three discrete checks for three possible outcomes is a bit
easier for me to understand, but I have no preference and can change
it to your version. 

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

Will add the latter.

> 
>> +	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.

Will do. I'm not sure if the underlying clock drivers guarantee the
chosen rate to be rounded in line with that however, e.g. qcom SoC
clocks that I target use lookup tables to find the closest rate
with known M/N config values and set that. (So unless one makes sure
the table has all the required rates, period is not guaranteed.)

This is not an issue for my use cases though: Don't think any
of the led or haptic motor controllers I've seen in my devices
need a perfect rate.

I think this is another line into the "Limitations" description
that was suggested later.

> 
>> +	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.
> 

Will move clk_enable() as the last action. After that it makes more
sense to "squash" two leftover checks for disabled state so will do
that as well.

... Except moving it caused a nice big WARNING from my clock controller...

[   76.353557] gcc_camss_gp1_clk status stuck at 'off'
[   76.353593] WARNING: CPU: 2 PID: 97 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x144/0x160
(...)
[   76.571531] Call trace:
[   76.578644]  clk_branch_wait+0x144/0x160
[   76.580903]  clk_branch2_enable+0x34/0x44
[   76.585069]  clk_core_enable+0x6c/0xc0
[   76.588974]  clk_enable+0x30/0x50
[   76.592620]  pwm_clk_apply+0xb0/0xe4
[   76.596007]  pwm_apply_state+0x6c/0x1ec
[   76.599651]  sgm3140_brightness_set+0xb4/0x190 [leds_sgm3140]

(Which doesn't stop it from working afaict, but very much undesirable
for me.)
Unsure if this is something common or just a quirk of this specific
driver but I'd rather take a little glitch on the output than
make clock driver unhappy knowing how picky this hardware sometimes is...

> 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.)
> 

Will add the description with limitations.

>> +}
>> +
>> +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.
> 

Will do.

>> +	}
>> +
>> +	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)

Will drop.

> 
>> +	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?
> 

Will change the order. Also adding an clk_disable() there for cases
when it's still enabled.

>> +	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_ ...

Oops, existence of "pwm-clock" (clk-pwm.c) and my attempts to not
collide with it's naming causes quite a bit of weird mix-up's like this.
.name should be "pwm-clk" in line with the driver filename and
all the method prefixes there but compatible should still be "clk-pwm"
so it's not as similar with an opposite "pwm-clock" compatible.

> 
>> +		.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@trvn.ru>");
>> +MODULE_DESCRIPTION("Clock based PWM driver");
>> +MODULE_LICENSE("GPL v2");
> 
> MODULE_LICENSE("GPL");
> 
> is the more usual today (and has the same meaning).

Will use the more common one then.

I will send a v2 with those changes (as well as with a filename fix
for the DT bindings) a bit later.

Thanks,
Nikita

> 
> Best regards
> Uwe

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

[Uwe Kleine-König]

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

Hello,

On Fri, Dec 10, 2021 at 06:13:34PM +0500, Nikita Travkin wrote:
> Uwe Kleine-König писал(а) 10.12.2021 03:05:
> > 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.
> 
> Having three discrete checks for three possible outcomes is a bit
> easier for me to understand, but I have no preference and can change
> it to your version. 
> 
> > Missing handling for polarity. Either refuse inverted polarity, or set
> > the duty_cycle to state->period - state->duty_cycle in the inverted
> > case.
> 
> Will add the latter.
> 
> > 
> >> +	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.
> 
> Will do. I'm not sure if the underlying clock drivers guarantee the
> chosen rate to be rounded in line with that however, e.g. qcom SoC

This is a problem that most drivers have. Very few use clk_set_rate, but
also clk_get_rate is subject to (much smaller) rounding issues.
There doesn't seem to be an agreement that this is important enough to
address.

> clocks that I target use lookup tables to find the closest rate
> with known M/N config values and set that. (So unless one makes sure
> the table has all the required rates, period is not guaranteed.)
> 
> This is not an issue for my use cases though: Don't think any
> of the led or haptic motor controllers I've seen in my devices
> need a perfect rate.
> 
> I think this is another line into the "Limitations" description
> that was suggested later.
> 
> > 
> >> +	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.
> > 
> 
> Will move clk_enable() as the last action. After that it makes more
> sense to "squash" two leftover checks for disabled state so will do
> that as well.
> 
> ... Except moving it caused a nice big WARNING from my clock controller...
> 
> [   76.353557] gcc_camss_gp1_clk status stuck at 'off'
> [   76.353593] WARNING: CPU: 2 PID: 97 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x144/0x160
> (...)
> [   76.571531] Call trace:
> [   76.578644]  clk_branch_wait+0x144/0x160
> [   76.580903]  clk_branch2_enable+0x34/0x44
> [   76.585069]  clk_core_enable+0x6c/0xc0
> [   76.588974]  clk_enable+0x30/0x50
> [   76.592620]  pwm_clk_apply+0xb0/0xe4
> [   76.596007]  pwm_apply_state+0x6c/0x1ec
> [   76.599651]  sgm3140_brightness_set+0xb4/0x190 [leds_sgm3140]
> 
> (Which doesn't stop it from working afaict, but very much undesirable
> for me.)
> Unsure if this is something common or just a quirk of this specific
> driver but I'd rather take a little glitch on the output than
> make clock driver unhappy knowing how picky this hardware sometimes is...

I think clk_set_rate for a disabled clk isn't well defined.

Best regards
Uwe

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

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