linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] Clock based PWM output driver
@ 2022-02-12 16:23 Nikita Travkin
  2022-02-12 16:23 ` [PATCH v5 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
  2022-02-12 16:23 ` [PATCH v5 2/2] pwm: Add clock based PWM output driver Nikita Travkin
  0 siblings, 2 replies; 10+ messages in thread
From: Nikita Travkin @ 2022-02-12 16:23 UTC (permalink / raw)
  To: thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, krzk, linus.walleij, masneyb,
	sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming, 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".

Changes in v2:
 - Fix filename in the DT schema.
 - Address Uwe's review comments.
Changes in v3:
 - Fix node pattern in the core pwm schema.
 - Address Uwe's review comments.
Changes in v4:
 - Drop the (incorrect) pwm schema change.
 - Use generic node name in the dt bindings example.
Changes in v5:
 - Correct required properties.
 - add missed returns.

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

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

-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v5 1/2] dt-bindings: pwm: Document clk based PWM controller
  2022-02-12 16:23 [PATCH v5 0/2] Clock based PWM output driver Nikita Travkin
@ 2022-02-12 16:23 ` Nikita Travkin
  2022-02-14  7:43   ` Krzysztof Kozlowski
  2022-02-12 16:23 ` [PATCH v5 2/2] pwm: Add clock based PWM output driver Nikita Travkin
  1 sibling, 1 reply; 10+ messages in thread
From: Nikita Travkin @ 2022-02-12 16:23 UTC (permalink / raw)
  To: thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, krzk, linus.walleij, masneyb,
	sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming, Nikita Travkin

Add YAML devicetree binding for clk based PWM controller

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
--
Changes in v2:
 - fix the file name.
Changes in v4:
 - Use generic node name in the dt bindings example.
Changes in v5:
 - make compatible required
---
 .../devicetree/bindings/pwm/clk-pwm.yaml      | 46 +++++++++++++++++++
 1 file changed, 46 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..ec1768291503
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/clk-pwm.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:
+  - compatible
+  - clocks
+
+examples:
+  - |
+    pwm {
+      compatible = "clk-pwm";
+      #pwm-cells = <2>;
+      clocks = <&gcc 0>;
+      pinctrl-names = "default";
+      pinctrl-0 = <&pwm_clk_flash_default>;
+    };
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-12 16:23 [PATCH v5 0/2] Clock based PWM output driver Nikita Travkin
  2022-02-12 16:23 ` [PATCH v5 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
@ 2022-02-12 16:23 ` Nikita Travkin
  2022-02-14 11:19   ` Krzysztof Kozlowski
  2022-02-14 18:43   ` Uwe Kleine-König
  1 sibling, 2 replies; 10+ messages in thread
From: Nikita Travkin @ 2022-02-12 16:23 UTC (permalink / raw)
  To: thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, krzk, linus.walleij, masneyb,
	sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming, 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>
--

Changes in v2:
 - Address Uwe's review comments:
   - Round set clk rate up
   - Add a description with limitations of the driver
   - Disable and unprepare clock before removing pwmchip
Changes in v3:
 - Use 64bit version of div round up
 - Address Uwe's review comments:
   - Reword the limitations to avoid incorrect claims
   - Move the clk_enabled flag assignment
   - Drop unnecessary statements
Changes in v5:
 - add missed returns
---
 drivers/pwm/Kconfig   |  10 +++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-clk.c | 139 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 150 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..e503337ad055
--- /dev/null
+++ b/drivers/pwm/pwm-clk.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock based PWM controller
+ *
+ * Copyright (c) 2021 Nikita Travkin <nikita@trvn.ru>
+ *
+ * This is an "adapter" driver that allows PWM consumers to use
+ * system clocks with duty cycle control as PWM outputs.
+ *
+ * Limitations:
+ * - Glitches are possible when new pwm state is applied.
+ * - Due to the fact that exact behavior depends on the underlying
+ *   clock driver, various limitations are possible.
+ * - Period depends on the clock and, in general, not guaranteed.
+ * - Underlying clock may not be able to give 0% or 100% duty cycle
+ *   (constant off or on), exact behavior will depend on the clock.
+ * - When the PWM is disabled, the clock will be disabled as well,
+ *   line state will depend on the clock.
+ */
+
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/pwm.h>
+
+struct pwm_clk_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	bool clk_enabled;
+};
+
+#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;
+	u64 period = state->period;
+	u64 duty_cycle = state->duty_cycle;
+
+	if (!state->enabled) {
+		if (pwm->state.enabled) {
+			clk_disable(chip->clk);
+			chip->clk_enabled = false;
+		}
+		return 0;
+	} else if (!pwm->state.enabled) {
+		ret = clk_enable(chip->clk);
+		if (ret)
+			return ret;
+		chip->clk_enabled = true;
+	}
+
+	rate = DIV64_U64_ROUND_UP(NSEC_PER_SEC, period);
+	ret = clk_set_rate(chip->clk, rate);
+	if (ret)
+		return ret;
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		duty_cycle = period - duty_cycle;
+
+	return clk_set_duty_cycle(chip->clk, duty_cycle, 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))
+		return dev_err_probe(&pdev->dev, PTR_ERR(chip->clk),
+				     "Failed to get clock\n");
+
+	chip->chip.dev = &pdev->dev;
+	chip->chip.ops = &pwm_clk_ops;
+	chip->chip.npwm = 1;
+
+	ret = clk_prepare(chip->clk);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
+
+	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);
+
+	pwmchip_remove(&chip->chip);
+
+	if (chip->clk_enabled)
+		clk_disable(chip->clk);
+
+	clk_unprepare(chip->clk);
+
+	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 = "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:pwm-clk");
+MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>");
+MODULE_DESCRIPTION("Clock based PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: pwm: Document clk based PWM controller
  2022-02-12 16:23 ` [PATCH v5 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
@ 2022-02-14  7:43   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-14  7:43 UTC (permalink / raw)
  To: Nikita Travkin, thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, linus.walleij, masneyb,
	sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

On 12/02/2022 17:23, Nikita Travkin wrote:
> Add YAML devicetree binding for clk based PWM controller
> 
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> --
> Changes in v2:
>  - fix the file name.
> Changes in v4:
>  - Use generic node name in the dt bindings example.
> Changes in v5:
>  - make compatible required
> ---
>  .../devicetree/bindings/pwm/clk-pwm.yaml      | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/clk-pwm.yaml
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-12 16:23 ` [PATCH v5 2/2] pwm: Add clock based PWM output driver Nikita Travkin
@ 2022-02-14 11:19   ` Krzysztof Kozlowski
  2022-02-19  6:48     ` Nikita Travkin
  2022-02-14 18:43   ` Uwe Kleine-König
  1 sibling, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-14 11:19 UTC (permalink / raw)
  To: Nikita Travkin, thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, linus.walleij, masneyb,
	sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

On 12/02/2022 17:23, Nikita Travkin wrote:
> 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>
> --
> 

(...)

> +
> +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))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(chip->clk),
> +				     "Failed to get clock\n");
> +
> +	chip->chip.dev = &pdev->dev;
> +	chip->chip.ops = &pwm_clk_ops;
> +	chip->chip.npwm = 1;
> +
> +	ret = clk_prepare(chip->clk);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
> +
> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");

You need to cleanup - unprepare the clock.

> +
> +	platform_set_drvdata(pdev, chip);
> +	return 0;
> +}
> +


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-12 16:23 ` [PATCH v5 2/2] pwm: Add clock based PWM output driver Nikita Travkin
  2022-02-14 11:19   ` Krzysztof Kozlowski
@ 2022-02-14 18:43   ` Uwe Kleine-König
  2022-02-19  6:46     ` Nikita Travkin
  1 sibling, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 18:43 UTC (permalink / raw)
  To: Nikita Travkin
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, krzk, linus.walleij,
	masneyb, sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

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

On Sat, Feb 12, 2022 at 09:23:42PM +0500, Nikita Travkin wrote:
> 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>
> --
> 
> Changes in v2:
>  - Address Uwe's review comments:
>    - Round set clk rate up
>    - Add a description with limitations of the driver
>    - Disable and unprepare clock before removing pwmchip
> Changes in v3:
>  - Use 64bit version of div round up
>  - Address Uwe's review comments:
>    - Reword the limitations to avoid incorrect claims
>    - Move the clk_enabled flag assignment
>    - Drop unnecessary statements
> Changes in v5:
>  - add missed returns
> ---
>  drivers/pwm/Kconfig   |  10 +++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-clk.c | 139 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 150 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..e503337ad055
> --- /dev/null
> +++ b/drivers/pwm/pwm-clk.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Clock based PWM controller
> + *
> + * Copyright (c) 2021 Nikita Travkin <nikita@trvn.ru>
> + *
> + * This is an "adapter" driver that allows PWM consumers to use
> + * system clocks with duty cycle control as PWM outputs.
> + *
> + * Limitations:
> + * - Glitches are possible when new pwm state is applied.
> + * - Due to the fact that exact behavior depends on the underlying
> + *   clock driver, various limitations are possible.
> + * - Period depends on the clock and, in general, not guaranteed.

This sentence is broken.

> + * - Underlying clock may not be able to give 0% or 100% duty cycle
> + *   (constant off or on), exact behavior will depend on the clock.
> + * - When the PWM is disabled, the clock will be disabled as well,
> + *   line state will depend on the clock.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/pwm.h>
> +
> +struct pwm_clk_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	bool clk_enabled;
> +};
> +
> +#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;
> +	u64 period = state->period;
> +	u64 duty_cycle = state->duty_cycle;
> +
> +	if (!state->enabled) {
> +		if (pwm->state.enabled) {
> +			clk_disable(chip->clk);
> +			chip->clk_enabled = false;
> +		}
> +		return 0;
> +	} else if (!pwm->state.enabled) {
> +		ret = clk_enable(chip->clk);
> +		if (ret)
> +			return ret;
> +		chip->clk_enabled = true;
> +	}
> +
> +	rate = DIV64_U64_ROUND_UP(NSEC_PER_SEC, period);
> +	ret = clk_set_rate(chip->clk, rate);
> +	if (ret)
> +		return ret;
> +
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		duty_cycle = period - duty_cycle;
> +
> +	return clk_set_duty_cycle(chip->clk, duty_cycle, 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))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(chip->clk),
> +				     "Failed to get clock\n");
> +
> +	chip->chip.dev = &pdev->dev;
> +	chip->chip.ops = &pwm_clk_ops;
> +	chip->chip.npwm = 1;
> +
> +	ret = clk_prepare(chip->clk);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
> +
> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");

As was already pointed out, here is some error cleanup necessary.

> +	platform_set_drvdata(pdev, chip);
> +	return 0;
> +}

Otherwise looks good.

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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-14 18:43   ` Uwe Kleine-König
@ 2022-02-19  6:46     ` Nikita Travkin
  2022-02-19 10:10       ` Uwe Kleine-König
  0 siblings, 1 reply; 10+ messages in thread
From: Nikita Travkin @ 2022-02-19  6:46 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, krzk, linus.walleij,
	masneyb, sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

Hi,

Uwe Kleine-König писал(а) 14.02.2022 23:43:
> On Sat, Feb 12, 2022 at 09:23:42PM +0500, Nikita Travkin wrote:
>> 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>
>> --
>>
>> Changes in v2:
>>  - Address Uwe's review comments:
>>    - Round set clk rate up
>>    - Add a description with limitations of the driver
>>    - Disable and unprepare clock before removing pwmchip
>> Changes in v3:
>>  - Use 64bit version of div round up
>>  - Address Uwe's review comments:
>>    - Reword the limitations to avoid incorrect claims
>>    - Move the clk_enabled flag assignment
>>    - Drop unnecessary statements
>> Changes in v5:
>>  - add missed returns
>> ---
>>  drivers/pwm/Kconfig   |  10 +++
>>  drivers/pwm/Makefile  |   1 +
>>  drivers/pwm/pwm-clk.c | 139 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 150 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..e503337ad055
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-clk.c
>> @@ -0,0 +1,139 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Clock based PWM controller
>> + *
>> + * Copyright (c) 2021 Nikita Travkin <nikita@trvn.ru>
>> + *
>> + * This is an "adapter" driver that allows PWM consumers to use
>> + * system clocks with duty cycle control as PWM outputs.
>> + *
>> + * Limitations:
>> + * - Glitches are possible when new pwm state is applied.
>> + * - Due to the fact that exact behavior depends on the underlying
>> + *   clock driver, various limitations are possible.
>> + * - Period depends on the clock and, in general, not guaranteed.
> 
> This sentence is broken.
> 

Here what I mean is that the clock driver might e.g. have a lookup table
for some rates and will only set one close to the requested ones.
(Extreme scenario is that only one rate is allowed in the lookup table,
which is a real possibility for some platforms that I think this driver
will be used with, the lookup may need to be changed for those clocks)

I will reword this like:

  Some clock drivers may only pick the closest available rate
  and not the exact requested one. Because of this, exact period
  is not guaranteed.

Thanks,
Nikita

>> + * - Underlying clock may not be able to give 0% or 100% duty cycle
>> + *   (constant off or on), exact behavior will depend on the clock.
>> + * - When the PWM is disabled, the clock will be disabled as well,
>> + *   line state will depend on the clock.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/math64.h>
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/pwm.h>
>> +
>> +struct pwm_clk_chip {
>> +	struct pwm_chip chip;
>> +	struct clk *clk;
>> +	bool clk_enabled;
>> +};
>> +
>> +#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;
>> +	u64 period = state->period;
>> +	u64 duty_cycle = state->duty_cycle;
>> +
>> +	if (!state->enabled) {
>> +		if (pwm->state.enabled) {
>> +			clk_disable(chip->clk);
>> +			chip->clk_enabled = false;
>> +		}
>> +		return 0;
>> +	} else if (!pwm->state.enabled) {
>> +		ret = clk_enable(chip->clk);
>> +		if (ret)
>> +			return ret;
>> +		chip->clk_enabled = true;
>> +	}
>> +
>> +	rate = DIV64_U64_ROUND_UP(NSEC_PER_SEC, period);
>> +	ret = clk_set_rate(chip->clk, rate);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (state->polarity == PWM_POLARITY_INVERSED)
>> +		duty_cycle = period - duty_cycle;
>> +
>> +	return clk_set_duty_cycle(chip->clk, duty_cycle, 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))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(chip->clk),
>> +				     "Failed to get clock\n");
>> +
>> +	chip->chip.dev = &pdev->dev;
>> +	chip->chip.ops = &pwm_clk_ops;
>> +	chip->chip.npwm = 1;
>> +
>> +	ret = clk_prepare(chip->clk);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
>> +
>> +	ret = pwmchip_add(&chip->chip);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
> 
> As was already pointed out, here is some error cleanup necessary.
> 
>> +	platform_set_drvdata(pdev, chip);
>> +	return 0;
>> +}
> 
> Otherwise looks good.
> 
> Best regards
> Uwe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-14 11:19   ` Krzysztof Kozlowski
@ 2022-02-19  6:48     ` Nikita Travkin
  0 siblings, 0 replies; 10+ messages in thread
From: Nikita Travkin @ 2022-02-19  6:48 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: thierry.reding, lee.jones, u.kleine-koenig, robh+dt, sboyd,
	linus.walleij, masneyb, sean.anderson, linux-pwm, devicetree,
	linux-kernel, ~postmarketos/upstreaming

Hi,

Krzysztof Kozlowski писал(а) 14.02.2022 16:19:
> On 12/02/2022 17:23, Nikita Travkin wrote:
>> 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>
>> --
>>
> 
> (...)
> 
>> +
>> +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))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(chip->clk),
>> +				     "Failed to get clock\n");
>> +
>> +	chip->chip.dev = &pdev->dev;
>> +	chip->chip.ops = &pwm_clk_ops;
>> +	chip->chip.npwm = 1;
>> +
>> +	ret = clk_prepare(chip->clk);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
>> +
>> +	ret = pwmchip_add(&chip->chip);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
> 
> You need to cleanup - unprepare the clock.
> 

Oops, I will add unprepare in v6, Thanks!

Nikita

>> +
>> +	platform_set_drvdata(pdev, chip);
>> +	return 0;
>> +}
>> +
> 
> 
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-19  6:46     ` Nikita Travkin
@ 2022-02-19 10:10       ` Uwe Kleine-König
  2022-02-19 13:54         ` Nikita Travkin
  0 siblings, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2022-02-19 10:10 UTC (permalink / raw)
  To: Nikita Travkin
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, krzk, linus.walleij,
	masneyb, sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

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

Hello,

On Sat, Feb 19, 2022 at 11:46:31AM +0500, Nikita Travkin wrote:
> Uwe Kleine-König писал(а) 14.02.2022 23:43:
> > On Sat, Feb 12, 2022 at 09:23:42PM +0500, Nikita Travkin wrote:
> >> + * Limitations:
> >> + * - Glitches are possible when new pwm state is applied.
> >> + * - Due to the fact that exact behavior depends on the underlying
> >> + *   clock driver, various limitations are possible.
> >> + * - Period depends on the clock and, in general, not guaranteed.
> > 
> > This sentence is broken.
> > 
> 
> Here what I mean is that the clock driver might e.g. have a lookup table
> for some rates and will only set one close to the requested ones.
> (Extreme scenario is that only one rate is allowed in the lookup table,
> which is a real possibility for some platforms that I think this driver
> will be used with, the lookup may need to be changed for those clocks)
> 
> I will reword this like:
> 
>   Some clock drivers may only pick the closest available rate
>   and not the exact requested one. Because of this, exact period
>   is not guaranteed.

That there is no exact match is quite normal also for dedicated PWM
HW blocks. So I think the second item in your list is good enough to
cover the non-existing guaranteed for period and glitches.

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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] pwm: Add clock based PWM output driver
  2022-02-19 10:10       ` Uwe Kleine-König
@ 2022-02-19 13:54         ` Nikita Travkin
  0 siblings, 0 replies; 10+ messages in thread
From: Nikita Travkin @ 2022-02-19 13:54 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, krzk, linus.walleij,
	masneyb, sean.anderson, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

Uwe Kleine-König писал(а) 19.02.2022 15:10:
> Hello,
> 
> On Sat, Feb 19, 2022 at 11:46:31AM +0500, Nikita Travkin wrote:
>> Uwe Kleine-König писал(а) 14.02.2022 23:43:
>> > On Sat, Feb 12, 2022 at 09:23:42PM +0500, Nikita Travkin wrote:
>> >> + * Limitations:
>> >> + * - Glitches are possible when new pwm state is applied.
>> >> + * - Due to the fact that exact behavior depends on the underlying
>> >> + *   clock driver, various limitations are possible.
>> >> + * - Period depends on the clock and, in general, not guaranteed.
>> >
>> > This sentence is broken.
>> >
>>
>> Here what I mean is that the clock driver might e.g. have a lookup table
>> for some rates and will only set one close to the requested ones.
>> (Extreme scenario is that only one rate is allowed in the lookup table,
>> which is a real possibility for some platforms that I think this driver
>> will be used with, the lookup may need to be changed for those clocks)
>>
>> I will reword this like:
>>
>>   Some clock drivers may only pick the closest available rate
>>   and not the exact requested one. Because of this, exact period
>>   is not guaranteed.
> 
> That there is no exact match is quite normal also for dedicated PWM
> HW blocks. So I think the second item in your list is good enough to
> cover the non-existing guaranteed for period and glitches.
> 

Oh, I will just drop the 1 (glitches) and 3 (period) then.
Thanks for explaining!

Nikita

> Best regards
> Uwe

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-02-19 13:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-12 16:23 [PATCH v5 0/2] Clock based PWM output driver Nikita Travkin
2022-02-12 16:23 ` [PATCH v5 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
2022-02-14  7:43   ` Krzysztof Kozlowski
2022-02-12 16:23 ` [PATCH v5 2/2] pwm: Add clock based PWM output driver Nikita Travkin
2022-02-14 11:19   ` Krzysztof Kozlowski
2022-02-19  6:48     ` Nikita Travkin
2022-02-14 18:43   ` Uwe Kleine-König
2022-02-19  6:46     ` Nikita Travkin
2022-02-19 10:10       ` Uwe Kleine-König
2022-02-19 13:54         ` Nikita Travkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).