All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Clock based PWM output driver
@ 2021-12-13 15:03 Nikita Travkin
  2021-12-13 15:03 ` [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Nikita Travkin @ 2021-12-13 15:03 UTC (permalink / raw)
  To: thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, linus.walleij, masneyb,
	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.

[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                         | 143 ++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/clk-pwm.yaml
 create mode 100644 drivers/pwm/pwm-clk.c

-- 
2.30.2


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

* [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller
  2021-12-13 15:03 [PATCH v2 0/2] Clock based PWM output driver Nikita Travkin
@ 2021-12-13 15:03 ` Nikita Travkin
  2021-12-13 16:20   ` Rob Herring
  2021-12-13 15:03 ` [PATCH v2 2/2] pwm: Add clock based PWM output driver Nikita Travkin
  2022-01-17 12:10 ` [PATCH v2 0/2] Clock " Uwe Kleine-König
  2 siblings, 1 reply; 13+ messages in thread
From: Nikita Travkin @ 2021-12-13 15:03 UTC (permalink / raw)
  To: thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, linus.walleij, masneyb,
	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.
---
 .../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..4fb2c1baaad4
--- /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/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:
+  - 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


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

* [PATCH v2 2/2] pwm: Add clock based PWM output driver
  2021-12-13 15:03 [PATCH v2 0/2] Clock based PWM output driver Nikita Travkin
  2021-12-13 15:03 ` [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
@ 2021-12-13 15:03 ` Nikita Travkin
  2021-12-14 17:03     ` kernel test robot
                     ` (2 more replies)
  2022-01-17 12:10 ` [PATCH v2 0/2] Clock " Uwe Kleine-König
  2 siblings, 3 replies; 13+ messages in thread
From: Nikita Travkin @ 2021-12-13 15:03 UTC (permalink / raw)
  To: thierry.reding, lee.jones
  Cc: u.kleine-koenig, robh+dt, sboyd, linus.walleij, masneyb,
	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
---
 drivers/pwm/Kconfig   |  10 +++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-clk.c | 143 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 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..55fd320b9c19
--- /dev/null
+++ b/drivers/pwm/pwm-clk.c
@@ -0,0 +1,143 @@
+// 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:
+ * - There is no way to atomically set both clock rate and
+ *   duty-cycle so glitches are possible when new pwm state
+ *   is applied.
+ * - Period depends on the underlying clock driver and,
+ *   in general, not guaranteed.
+ * - Underlying clock may not be able to give 100%
+ *   duty cycle (constant on) and only set the closest
+ *   possible duty cycle. (e.g. 99.9%)
+ */
+
+#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;
+	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);
+		chip->clk_enabled = true;
+		if (ret)
+			return ret;
+	}
+
+	rate = DIV_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;
+
+	ret = clk_set_duty_cycle(chip->clk, duty_cycle, period);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+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.of_xlate = of_pwm_xlate_with_flags;
+	chip->chip.of_pwm_n_cells = 2;
+	chip->chip.npwm = 1;
+
+	ret = clk_prepare(chip->clk);
+	if (ret < 0)
+		dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0)
+		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.30.2


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

* Re: [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller
  2021-12-13 15:03 ` [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
@ 2021-12-13 16:20   ` Rob Herring
  0 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2021-12-13 16:20 UTC (permalink / raw)
  To: Nikita Travkin
  Cc: linux-kernel, ~postmarketos/upstreaming, linus.walleij,
	thierry.reding, u.kleine-koenig, lee.jones, devicetree, masneyb,
	robh+dt, sboyd, linux-pwm

On Mon, 13 Dec 2021 20:03:34 +0500, 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.
> ---
>  .../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:
/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/1567356

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.


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

* Re: [PATCH v2 2/2] pwm: Add clock based PWM output driver
  2021-12-13 15:03 ` [PATCH v2 2/2] pwm: Add clock based PWM output driver Nikita Travkin
@ 2021-12-14 17:03     ` kernel test robot
  2021-12-14 19:17     ` kernel test robot
  2022-01-17 15:58   ` Uwe Kleine-König
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-12-14 17:03 UTC (permalink / raw)
  To: Nikita Travkin, thierry.reding, lee.jones
  Cc: kbuild-all, u.kleine-koenig, robh+dt, sboyd, linus.walleij,
	masneyb, linux-pwm, devicetree, linux-kernel

Hi Nikita,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on v5.16-rc5 next-20211213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: riscv-randconfig-c024-20211214 (https://download.01.org/0day-ci/archive/20211215/202112150018.zRKkwUhX-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/454624747f4637529777274ae1b5ab7af33fd130
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
        git checkout 454624747f4637529777274ae1b5ab7af33fd130
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   riscv32-linux-ld: drivers/pwm/pwm-clk.o: in function `.L18':
>> pwm-clk.c:(.text+0x86): undefined reference to `__udivdi3'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

* Re: [PATCH v2 2/2] pwm: Add clock based PWM output driver
@ 2021-12-14 17:03     ` kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-12-14 17:03 UTC (permalink / raw)
  To: kbuild-all

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

Hi Nikita,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on v5.16-rc5 next-20211213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: riscv-randconfig-c024-20211214 (https://download.01.org/0day-ci/archive/20211215/202112150018.zRKkwUhX-lkp(a)intel.com/config)
compiler: riscv32-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/454624747f4637529777274ae1b5ab7af33fd130
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
        git checkout 454624747f4637529777274ae1b5ab7af33fd130
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   riscv32-linux-ld: drivers/pwm/pwm-clk.o: in function `.L18':
>> pwm-clk.c:(.text+0x86): undefined reference to `__udivdi3'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

* Re: [PATCH v2 2/2] pwm: Add clock based PWM output driver
  2021-12-13 15:03 ` [PATCH v2 2/2] pwm: Add clock based PWM output driver Nikita Travkin
@ 2021-12-14 19:17     ` kernel test robot
  2021-12-14 19:17     ` kernel test robot
  2022-01-17 15:58   ` Uwe Kleine-König
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-12-14 19:17 UTC (permalink / raw)
  To: Nikita Travkin, thierry.reding, lee.jones
  Cc: kbuild-all, u.kleine-koenig, robh+dt, sboyd, linus.walleij,
	masneyb, linux-pwm, devicetree, linux-kernel

Hi Nikita,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on v5.16-rc5 next-20211213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: microblaze-randconfig-r003-20211214 (https://download.01.org/0day-ci/archive/20211215/202112150357.lrG18diq-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/454624747f4637529777274ae1b5ab7af33fd130
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
        git checkout 454624747f4637529777274ae1b5ab7af33fd130
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__udivdi3" [drivers/pwm/pwm-clk.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

* Re: [PATCH v2 2/2] pwm: Add clock based PWM output driver
@ 2021-12-14 19:17     ` kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-12-14 19:17 UTC (permalink / raw)
  To: kbuild-all

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

Hi Nikita,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on v5.16-rc5 next-20211213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: microblaze-randconfig-r003-20211214 (https://download.01.org/0day-ci/archive/20211215/202112150357.lrG18diq-lkp(a)intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/454624747f4637529777274ae1b5ab7af33fd130
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikita-Travkin/Clock-based-PWM-output-driver/20211213-230628
        git checkout 454624747f4637529777274ae1b5ab7af33fd130
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__udivdi3" [drivers/pwm/pwm-clk.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

* Re: [PATCH v2 0/2] Clock based PWM output driver
  2021-12-13 15:03 [PATCH v2 0/2] Clock based PWM output driver Nikita Travkin
  2021-12-13 15:03 ` [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
  2021-12-13 15:03 ` [PATCH v2 2/2] pwm: Add clock based PWM output driver Nikita Travkin
@ 2022-01-17 12:10 ` Uwe Kleine-König
  2022-01-17 13:09   ` Nikita Travkin
  2 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2022-01-17 12:10 UTC (permalink / raw)
  To: Nikita Travkin
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, linus.walleij,
	masneyb, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

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

Hello Nikita,

On Mon, Dec 13, 2021 at 08:03:33PM +0500, Nikita Travkin wrote:
> 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". 

You got some feedback on your patches and didn't respond to it. Are you
interested to improve your patch set? If yes, I'm willing to review more
deeply. If not, I'm not.

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] 13+ messages in thread

* Re: [PATCH v2 0/2] Clock based PWM output driver
  2022-01-17 12:10 ` [PATCH v2 0/2] Clock " Uwe Kleine-König
@ 2022-01-17 13:09   ` Nikita Travkin
  0 siblings, 0 replies; 13+ messages in thread
From: Nikita Travkin @ 2022-01-17 13:09 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, linus.walleij,
	masneyb, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

Uwe Kleine-König писал(а) 17.01.2022 17:10:
> Hello Nikita,
> 
> On Mon, Dec 13, 2021 at 08:03:33PM +0500, Nikita Travkin wrote:
>> 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".
> 
> You got some feedback on your patches and didn't respond to it. Are you
> interested to improve your patch set? If yes, I'm willing to review more
> deeply. If not, I'm not.
> 

Hi, I do intend on finishing this and getting the patches upstream
however I was very short on time for the last while and couldn't get
to it.

For this v2 I see the dt bindings check failure, I think it's the
regex in the core schema that was defined incorrectly but my attempt
to fix it has failed last time I tried it for some reason.
(Now looking at it one more time, I see that '^pwm(@.*|-[0-9a-f])*$'
only allows [a-f] and I just tried to move the * to the correct place)
I also see that I've used the wrong division with rounding macro.

I am planning to send a v3 a bit later with those fixed.

Sorry for delaying the response
Nikita 


> Best regards
> Uwe

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

* Re: [PATCH v2 2/2] pwm: Add clock based PWM output driver
  2021-12-13 15:03 ` [PATCH v2 2/2] pwm: Add clock based PWM output driver Nikita Travkin
  2021-12-14 17:03     ` kernel test robot
  2021-12-14 19:17     ` kernel test robot
@ 2022-01-17 15:58   ` Uwe Kleine-König
  2022-01-17 18:04     ` Nikita Travkin
  2 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2022-01-17 15:58 UTC (permalink / raw)
  To: Nikita Travkin
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, linus.walleij,
	masneyb, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

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

Hello,

On Mon, Dec 13, 2021 at 08:03:35PM +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
> ---
>  drivers/pwm/Kconfig   |  10 +++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-clk.c | 143 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 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..55fd320b9c19
> --- /dev/null
> +++ b/drivers/pwm/pwm-clk.c
> @@ -0,0 +1,143 @@
> +// 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:
> + * - There is no way to atomically set both clock rate and
> + *   duty-cycle so glitches are possible when new pwm state
> + *   is applied.
> + * - Period depends on the underlying clock driver and,
> + *   in general, not guaranteed.
> + * - Underlying clock may not be able to give 100%
> + *   duty cycle (constant on) and only set the closest
> + *   possible duty cycle. (e.g. 99.9%)

What about 0%?

 - Periods are not completed on changes in general.
 - Behaviour on disable depends on the underlaying clk, don't assume it
   to provide the inactive level.

> + */
> +
> +#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;
> +	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);
> +		chip->clk_enabled = true;
> +		if (ret)
> +			return ret;

if clk_enable() failed better don't set chip->clk_enabled = true;

> +	}
> +
> +	rate = DIV_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;
> +
> +	ret = clk_set_duty_cycle(chip->clk, duty_cycle, period);
> +	if (ret)
> +		return ret;
> +
> +	return ret;

This can be simplified to

	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.of_xlate = of_pwm_xlate_with_flags;
> +	chip->chip.of_pwm_n_cells = 2;

I'd just skip those two assignments. These are the default, anyhow.
(Assuming you have #pwm-cells = <2> in the device tree.)

> +	chip->chip.npwm = 1;
> +
> +	ret = clk_prepare(chip->clk);
> +	if (ret < 0)
> +		dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
> +
> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0)
> +		dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
> +
> +	platform_set_drvdata(pdev, chip);
> +	return 0;
> +}

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] 13+ messages in thread

* Re: [PATCH v2 2/2] pwm: Add clock based PWM output driver
  2022-01-17 15:58   ` Uwe Kleine-König
@ 2022-01-17 18:04     ` Nikita Travkin
  2022-01-17 20:04       ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Nikita Travkin @ 2022-01-17 18:04 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: thierry.reding, lee.jones, robh+dt, sboyd, linus.walleij,
	masneyb, linux-pwm, devicetree, linux-kernel,
	~postmarketos/upstreaming

Hi,

Uwe Kleine-König писал(а) 17.01.2022 20:58:
> Hello,
> 
> On Mon, Dec 13, 2021 at 08:03:35PM +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
>> ---
>>  drivers/pwm/Kconfig   |  10 +++
>>  drivers/pwm/Makefile  |   1 +
>>  drivers/pwm/pwm-clk.c | 143 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 154 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..55fd320b9c19
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-clk.c
>> @@ -0,0 +1,143 @@
>> +// 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:
>> + * - There is no way to atomically set both clock rate and
>> + *   duty-cycle so glitches are possible when new pwm state
>> + *   is applied.
>> + * - Period depends on the underlying clock driver and,
>> + *   in general, not guaranteed.
>> + * - Underlying clock may not be able to give 100%
>> + *   duty cycle (constant on) and only set the closest
>> + *   possible duty cycle. (e.g. 99.9%)
> 
> What about 0%?

You're right, this is also a problematic case that I should've
mentioned here. In fact I *did* have problems with zero written
into the duty cycle register of my clock. I decided that it
should be solved by the hardware driver so I sent a patch
with a zero check there. (As otherwise there might be a clock
that would properly support 0% and 100% cycles so making the
write like this impossible is not a job of this driver I think)

> 
>  - Periods are not completed on changes in general.

I suppose I should reword the limitation, dropping
the reference to impossible atomic operations and
just state that glitches are inevitable.

>  - Behaviour on disable depends on the underlaying clk, don't assume it
>    to provide the inactive level.
> 

Hm, now thinking of it, I'm not sure if the clock line
was set to logic 0 or was left floating (which is what I assume
you mean by the undefined behavior here) on the clock I was
debugging this on with an oscilloscope. (nor am I sure
if I even can make such a conclusion by looking at that...)

Do you think that this should be just documented in the
limitations? Like:

  - Underlying clock may not be able to give 0% or 100%
    duty cycle (constant off or on) and only set the
    closest possible duty cycle. (e.g. 0.1% or 99.9%)
  - When the PWM is disabled, the clock will be disabled
    as well. User should take care of properly pulling 
    the line down in case the disabled clock leaves it
    floating.

(I guess here I'm making an assumption that there is
no clock that will be set to logic 1 upon being disabled
without the way to change that behavior...)

>> + */
>> +
>> +#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;
>> +	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);
>> +		chip->clk_enabled = true;
>> +		if (ret)
>> +			return ret;
> 
> if clk_enable() failed better don't set chip->clk_enabled = true;
> 

Will move it after the check.

>> +	}
>> +
>> +	rate = DIV_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;
>> +
>> +	ret = clk_set_duty_cycle(chip->clk, duty_cycle, period);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
> 
> This can be simplified to
> 
> 	return clk_set_duty_cycle(chip->clk, duty_cycle, period);
> 

Will do.

>> +}
>> +
>> +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.of_xlate = of_pwm_xlate_with_flags;
>> +	chip->chip.of_pwm_n_cells = 2;
> 
> I'd just skip those two assignments. These are the default, anyhow.
> (Assuming you have #pwm-cells = <2> in the device tree.)
> 

Right, now I see that the core sets those and I force the
#pwm-cells = <2> via the dt bindings. Will drop.

Thanks for the comments! I will send a v3 with the changes
slightly later

Nikita

>> +	chip->chip.npwm = 1;
>> +
>> +	ret = clk_prepare(chip->clk);
>> +	if (ret < 0)
>> +		dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
>> +
>> +	ret = pwmchip_add(&chip->chip);
>> +	if (ret < 0)
>> +		dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
>> +
>> +	platform_set_drvdata(pdev, chip);
>> +	return 0;
>> +}
> 
> Best regards
> Uwe

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

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

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

On Mon, Jan 17, 2022 at 11:04:31PM +0500, Nikita Travkin wrote:
> Hi,
> 
> Uwe Kleine-König писал(а) 17.01.2022 20:58:
> > Hello,
> > 
> > On Mon, Dec 13, 2021 at 08:03:35PM +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
> >> ---
> >>  drivers/pwm/Kconfig   |  10 +++
> >>  drivers/pwm/Makefile  |   1 +
> >>  drivers/pwm/pwm-clk.c | 143 ++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 154 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..55fd320b9c19
> >> --- /dev/null
> >> +++ b/drivers/pwm/pwm-clk.c
> >> @@ -0,0 +1,143 @@
> >> +// 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:
> >> + * - There is no way to atomically set both clock rate and
> >> + *   duty-cycle so glitches are possible when new pwm state
> >> + *   is applied.
> >> + * - Period depends on the underlying clock driver and,
> >> + *   in general, not guaranteed.
> >> + * - Underlying clock may not be able to give 100%
> >> + *   duty cycle (constant on) and only set the closest
> >> + *   possible duty cycle. (e.g. 99.9%)
> > 
> > What about 0%?
> 
> You're right, this is also a problematic case that I should've
> mentioned here. In fact I *did* have problems with zero written
> into the duty cycle register of my clock. I decided that it
> should be solved by the hardware driver so I sent a patch
> with a zero check there. (As otherwise there might be a clock
> that would properly support 0% and 100% cycles so making the
> write like this impossible is not a job of this driver I think)
> 
> > 
> >  - Periods are not completed on changes in general.
> 
> I suppose I should reword the limitation, dropping
> the reference to impossible atomic operations and
> just state that glitches are inevitable.
> 
> >  - Behaviour on disable depends on the underlaying clk, don't assume it
> >    to provide the inactive level.
> > 
> 
> Hm, now thinking of it, I'm not sure if the clock line
> was set to logic 0 or was left floating (which is what I assume
> you mean by the undefined behavior here) on the clock I was
> debugging this on with an oscilloscope. (nor am I sure
> if I even can make such a conclusion by looking at that...)
> 
> Do you think that this should be just documented in the
> limitations? Like:
> 
>   - Underlying clock may not be able to give 0% or 100%
>     duty cycle (constant off or on) and only set the
>     closest possible duty cycle. (e.g. 0.1% or 99.9%)

I would not bet on this. Maybe in such a case clk_set_duty_cycle might
also fail. The clk API isn't (TTBOMK) well-defined enough to make
promises like that.

>   - When the PWM is disabled, the clock will be disabled
>     as well. User should take care of properly pulling 
>     the line down in case the disabled clock leaves it
>     floating.

This isn't universally true. I'd expect that just freezing (i.e. driving
either high or low depending on the state when the clk was stopped) is a
very usual behaviour. So a pull isn't always a good idea.

I would just keep that unspecified.

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] 13+ messages in thread

end of thread, other threads:[~2022-01-17 20:05 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13 15:03 [PATCH v2 0/2] Clock based PWM output driver Nikita Travkin
2021-12-13 15:03 ` [PATCH v2 1/2] dt-bindings: pwm: Document clk based PWM controller Nikita Travkin
2021-12-13 16:20   ` Rob Herring
2021-12-13 15:03 ` [PATCH v2 2/2] pwm: Add clock based PWM output driver Nikita Travkin
2021-12-14 17:03   ` kernel test robot
2021-12-14 17:03     ` kernel test robot
2021-12-14 19:17   ` kernel test robot
2021-12-14 19:17     ` kernel test robot
2022-01-17 15:58   ` Uwe Kleine-König
2022-01-17 18:04     ` Nikita Travkin
2022-01-17 20:04       ` Uwe Kleine-König
2022-01-17 12:10 ` [PATCH v2 0/2] Clock " Uwe Kleine-König
2022-01-17 13:09   ` Nikita Travkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.