[PATCH v8 0/5] DesignWare PWM driver updates

[PATCH v8 0/5] DesignWare PWM driver updates

[Ben Dooks]

This series is an update for the DesignWare PWM driver to add support for
OF (and split the PCI bits out if aynone else wants them). This is mostly
the same as the v7 series, but with code moved around and module-namespace
added, plus review comments processed.

Since we no longer have the hardware, the clock code hasn't been changed to
either lock the rate whilst the PWM is running, or to deal with any sort
of change callback. This is left to future work (and I would rather get
something in that does currently work) (second note, the hardware we did
use had a fixed clock tree anyway)

This account is probably going away soon, I have cc'd my main work
email to catch any responses.

Thank you all for the reviews.

The lengthy changelog:

v8:
 - updated reviewed tags
 - fix module name for pci version
 - fix config symbol bug in makefile
 - remove pci compile-test (mostly not used for pci)
 - push the compile-test into the platform/of driver
v7:
 - fixup kconfig from previous pcie changes
 - re-order kconfig to make dwc core be selected by PCI driver
 - move clk variable to patch it is used in
v6:
 - fix removal ordering of DWC_PERIOD_NS
v5:
 - fixed kconfig string error
 - merged pwm-nr into main of code
 - split of code from pci code
 - updated pwm-nr capping
 - fix duplicate error reporting in of-code
 - fix return in of-probe
 - remove unecessary remove function as devm_ functions sort this
 - fixed ordering of properties
 - added missing reg item
 - fixed missing split of the two clock sources.
 - get bus clock in of code
v4:
 - split pci and of into new modules
 - fixup review comments
 - fix typos in dt-bindings
v3:
- change the compatible name
- squash down pwm count patch
- fixup patch naming
v2:
- fix #pwm-cells count to be 3
- fix indetation 
- merge the two clock patches
- add HAS_IOMEM as a config dependency

Ben Dooks (5):
  pwm: dwc: split pci out of core driver
  pwm: dwc: make timer clock configurable
  pwm: dwc: add PWM bit unset in get_state call
  pwm: dwc: use clock rate in hz to avoid rounding issues
  pwm: dwc: add of/platform support

 drivers/pwm/Kconfig        |  24 ++++-
 drivers/pwm/Makefile       |   2 +
 drivers/pwm/pwm-dwc-core.c | 196 ++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-dwc-of.c   |  78 +++++++++++++++
 drivers/pwm/pwm-dwc.c      | 198 +------------------------------------
 drivers/pwm/pwm-dwc.h      |  61 ++++++++++++
 6 files changed, 364 insertions(+), 195 deletions(-)
 create mode 100644 drivers/pwm/pwm-dwc-core.c
 create mode 100644 drivers/pwm/pwm-dwc-of.c
 create mode 100644 drivers/pwm/pwm-dwc.h

-- 
2.39.2

[PATCH v8 1/5] pwm: dwc: split pci out of core driver

[Ben Dooks]

Moving towards adding non-pci support for the driver, move the pci
parts out of the core into their own module. This is partly due to
the module_driver() code only being allowed once in a module and also
to avoid a number of #ifdef if we build a single file in a system
without pci support.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v8:
 - add module namespace
 - remove compile-test for pci case, doesn't make sense
 - fix makefile, missed config symbol changes
v7:
 - re-order kconfig to make dwc core be selected by PCI driver
v6:
 - put DWC_PERIOD_NS back to avoid bisect issues
v4:
 - removed DWC_PERIOD_NS as not needed
---
 drivers/pwm/Kconfig        |  14 ++-
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-dwc-core.c | 176 +++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-dwc.c      | 197 +------------------------------------
 drivers/pwm/pwm-dwc.h      |  60 +++++++++++
 5 files changed, 253 insertions(+), 195 deletions(-)
 create mode 100644 drivers/pwm/pwm-dwc-core.c
 create mode 100644 drivers/pwm/pwm-dwc.h

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8df861b1f4a3..7c54cdcb97a0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -186,9 +186,19 @@ config PWM_CROS_EC
 	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
 	  Controller.
 
+config PWM_DWC_CORE
+	tristate
+	depends on HAS_IOMEM
+	help
+	  PWM driver for Synopsys DWC PWM Controller.
+
+	  To compile this driver as a module, build the dependecies as
+	  modules, this will be called pwm-dwc-core.
+
 config PWM_DWC
-	tristate "DesignWare PWM Controller"
-	depends on PCI
+	tristate "DesignWare PWM Controller (PCI bus)"
+	depends on HAS_IOMEM && PCI
+	select PWM_DWC_CORE
 	help
 	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
 
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 19899b912e00..de3ed77e8d7c 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@ 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
+obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
 obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
new file mode 100644
index 000000000000..b693cb7fa812
--- /dev/null
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver core
+ *
+ * Copyright (C) 2018-2020 Intel Corporation
+ *
+ * Author: Felipe Balbi (Intel)
+ * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
+ * Author: Raymond Tan <raymond.tan@intel.com>
+ */
+
+#define DEFAULT_SYMBOL_NAMESPACE dwc_pwm
+
+#include <linux/bitops.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+
+#include "pwm-dwc.h"
+
+static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
+{
+	u32 reg;
+
+	reg = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm));
+
+	if (enabled)
+		reg |= DWC_TIM_CTRL_EN;
+	else
+		reg &= ~DWC_TIM_CTRL_EN;
+
+	dwc_pwm_writel(dwc, reg, DWC_TIM_CTRL(pwm));
+}
+
+static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
+				     struct pwm_device *pwm,
+				     const struct pwm_state *state)
+{
+	u64 tmp;
+	u32 ctrl;
+	u32 high;
+	u32 low;
+
+	/*
+	 * Calculate width of low and high period in terms of input clock
+	 * periods and check are the result within HW limits between 1 and
+	 * 2^32 periods.
+	 */
+	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
+	if (tmp < 1 || tmp > (1ULL << 32))
+		return -ERANGE;
+	low = tmp - 1;
+
+	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
+				    DWC_CLK_PERIOD_NS);
+	if (tmp < 1 || tmp > (1ULL << 32))
+		return -ERANGE;
+	high = tmp - 1;
+
+	/*
+	 * Specification says timer usage flow is to disable timer, then
+	 * program it followed by enable. It also says Load Count is loaded
+	 * into timer after it is enabled - either after a disable or
+	 * a reset. Based on measurements it happens also without disable
+	 * whenever Load Count is updated. But follow the specification.
+	 */
+	__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
+
+	/*
+	 * Write Load Count and Load Count 2 registers. Former defines the
+	 * width of low period and latter the width of high period in terms
+	 * multiple of input clock periods:
+	 * Width = ((Count + 1) * input clock period).
+	 */
+	dwc_pwm_writel(dwc, low, DWC_TIM_LD_CNT(pwm->hwpwm));
+	dwc_pwm_writel(dwc, high, DWC_TIM_LD_CNT2(pwm->hwpwm));
+
+	/*
+	 * Set user-defined mode, timer reloads from Load Count registers
+	 * when it counts down to 0.
+	 * Set PWM mode, it makes output to toggle and width of low and high
+	 * periods are set by Load Count registers.
+	 */
+	ctrl = DWC_TIM_CTRL_MODE_USER | DWC_TIM_CTRL_PWM;
+	dwc_pwm_writel(dwc, ctrl, DWC_TIM_CTRL(pwm->hwpwm));
+
+	/*
+	 * Enable timer. Output starts from low period.
+	 */
+	__dwc_pwm_set_enable(dwc, pwm->hwpwm, state->enabled);
+
+	return 0;
+}
+
+static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct dwc_pwm *dwc = to_dwc_pwm(chip);
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	if (state->enabled) {
+		if (!pwm->state.enabled)
+			pm_runtime_get_sync(chip->dev);
+		return __dwc_pwm_configure_timer(dwc, pwm, state);
+	} else {
+		if (pwm->state.enabled) {
+			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
+			pm_runtime_put_sync(chip->dev);
+		}
+	}
+
+	return 0;
+}
+
+static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct dwc_pwm *dwc = to_dwc_pwm(chip);
+	u64 duty, period;
+
+	pm_runtime_get_sync(chip->dev);
+
+	state->enabled = !!(dwc_pwm_readl(dwc,
+				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
+
+	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
+	duty += 1;
+	duty *= DWC_CLK_PERIOD_NS;
+	state->duty_cycle = duty;
+
+	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
+	period += 1;
+	period *= DWC_CLK_PERIOD_NS;
+	period += duty;
+	state->period = period;
+
+	state->polarity = PWM_POLARITY_INVERSED;
+
+	pm_runtime_put_sync(chip->dev);
+
+	return 0;
+}
+
+static const struct pwm_ops dwc_pwm_ops = {
+	.apply = dwc_pwm_apply,
+	.get_state = dwc_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
+{
+	struct dwc_pwm *dwc;
+
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	if (!dwc)
+		return NULL;
+
+	dwc->chip.dev = dev;
+	dwc->chip.ops = &dwc_pwm_ops;
+	dwc->chip.npwm = DWC_TIMERS_TOTAL;
+
+	dev_set_drvdata(dev, dwc);
+	return dwc;
+}
+EXPORT_SYMBOL_GPL(dwc_pwm_alloc);
+
+MODULE_AUTHOR("Felipe Balbi (Intel)");
+MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@linux.intel.com>");
+MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
+MODULE_DESCRIPTION("DesignWare PWM Controller");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 3bbb26c862c3..bd9cadb497d7 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * DesignWare PWM Controller driver
+ * DesignWare PWM Controller driver (PCI part)
  *
  * Copyright (C) 2018-2020 Intel Corporation
  *
@@ -13,6 +13,8 @@
  *   periods are one or more input clock periods long.
  */
 
+#define DEFAULT_MOUDLE_NAMESPACE dwc_pwm
+
 #include <linux/bitops.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
@@ -21,198 +23,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
-#define DWC_TIM_LD_CNT(n)	((n) * 0x14)
-#define DWC_TIM_LD_CNT2(n)	(((n) * 4) + 0xb0)
-#define DWC_TIM_CUR_VAL(n)	(((n) * 0x14) + 0x04)
-#define DWC_TIM_CTRL(n)		(((n) * 0x14) + 0x08)
-#define DWC_TIM_EOI(n)		(((n) * 0x14) + 0x0c)
-#define DWC_TIM_INT_STS(n)	(((n) * 0x14) + 0x10)
-
-#define DWC_TIMERS_INT_STS	0xa0
-#define DWC_TIMERS_EOI		0xa4
-#define DWC_TIMERS_RAW_INT_STS	0xa8
-#define DWC_TIMERS_COMP_VERSION	0xac
-
-#define DWC_TIMERS_TOTAL	8
-#define DWC_CLK_PERIOD_NS	10
-
-/* Timer Control Register */
-#define DWC_TIM_CTRL_EN		BIT(0)
-#define DWC_TIM_CTRL_MODE	BIT(1)
-#define DWC_TIM_CTRL_MODE_FREE	(0 << 1)
-#define DWC_TIM_CTRL_MODE_USER	(1 << 1)
-#define DWC_TIM_CTRL_INT_MASK	BIT(2)
-#define DWC_TIM_CTRL_PWM	BIT(3)
-
-struct dwc_pwm_ctx {
-	u32 cnt;
-	u32 cnt2;
-	u32 ctrl;
-};
-
-struct dwc_pwm {
-	struct pwm_chip chip;
-	void __iomem *base;
-	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
-};
-#define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
-
-static inline u32 dwc_pwm_readl(struct dwc_pwm *dwc, u32 offset)
-{
-	return readl(dwc->base + offset);
-}
-
-static inline void dwc_pwm_writel(struct dwc_pwm *dwc, u32 value, u32 offset)
-{
-	writel(value, dwc->base + offset);
-}
-
-static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
-{
-	u32 reg;
-
-	reg = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm));
-
-	if (enabled)
-		reg |= DWC_TIM_CTRL_EN;
-	else
-		reg &= ~DWC_TIM_CTRL_EN;
-
-	dwc_pwm_writel(dwc, reg, DWC_TIM_CTRL(pwm));
-}
-
-static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
-				     struct pwm_device *pwm,
-				     const struct pwm_state *state)
-{
-	u64 tmp;
-	u32 ctrl;
-	u32 high;
-	u32 low;
-
-	/*
-	 * Calculate width of low and high period in terms of input clock
-	 * periods and check are the result within HW limits between 1 and
-	 * 2^32 periods.
-	 */
-	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
-	if (tmp < 1 || tmp > (1ULL << 32))
-		return -ERANGE;
-	low = tmp - 1;
-
-	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
-				    DWC_CLK_PERIOD_NS);
-	if (tmp < 1 || tmp > (1ULL << 32))
-		return -ERANGE;
-	high = tmp - 1;
-
-	/*
-	 * Specification says timer usage flow is to disable timer, then
-	 * program it followed by enable. It also says Load Count is loaded
-	 * into timer after it is enabled - either after a disable or
-	 * a reset. Based on measurements it happens also without disable
-	 * whenever Load Count is updated. But follow the specification.
-	 */
-	__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
-
-	/*
-	 * Write Load Count and Load Count 2 registers. Former defines the
-	 * width of low period and latter the width of high period in terms
-	 * multiple of input clock periods:
-	 * Width = ((Count + 1) * input clock period).
-	 */
-	dwc_pwm_writel(dwc, low, DWC_TIM_LD_CNT(pwm->hwpwm));
-	dwc_pwm_writel(dwc, high, DWC_TIM_LD_CNT2(pwm->hwpwm));
-
-	/*
-	 * Set user-defined mode, timer reloads from Load Count registers
-	 * when it counts down to 0.
-	 * Set PWM mode, it makes output to toggle and width of low and high
-	 * periods are set by Load Count registers.
-	 */
-	ctrl = DWC_TIM_CTRL_MODE_USER | DWC_TIM_CTRL_PWM;
-	dwc_pwm_writel(dwc, ctrl, DWC_TIM_CTRL(pwm->hwpwm));
-
-	/*
-	 * Enable timer. Output starts from low period.
-	 */
-	__dwc_pwm_set_enable(dwc, pwm->hwpwm, state->enabled);
-
-	return 0;
-}
-
-static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			 const struct pwm_state *state)
-{
-	struct dwc_pwm *dwc = to_dwc_pwm(chip);
-
-	if (state->polarity != PWM_POLARITY_INVERSED)
-		return -EINVAL;
-
-	if (state->enabled) {
-		if (!pwm->state.enabled)
-			pm_runtime_get_sync(chip->dev);
-		return __dwc_pwm_configure_timer(dwc, pwm, state);
-	} else {
-		if (pwm->state.enabled) {
-			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
-			pm_runtime_put_sync(chip->dev);
-		}
-	}
-
-	return 0;
-}
-
-static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
-			     struct pwm_state *state)
-{
-	struct dwc_pwm *dwc = to_dwc_pwm(chip);
-	u64 duty, period;
-
-	pm_runtime_get_sync(chip->dev);
-
-	state->enabled = !!(dwc_pwm_readl(dwc,
-				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
-
-	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
-	duty += 1;
-	duty *= DWC_CLK_PERIOD_NS;
-	state->duty_cycle = duty;
-
-	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
-	period += 1;
-	period *= DWC_CLK_PERIOD_NS;
-	period += duty;
-	state->period = period;
-
-	state->polarity = PWM_POLARITY_INVERSED;
-
-	pm_runtime_put_sync(chip->dev);
-
-	return 0;
-}
-
-static const struct pwm_ops dwc_pwm_ops = {
-	.apply = dwc_pwm_apply,
-	.get_state = dwc_pwm_get_state,
-	.owner = THIS_MODULE,
-};
-
-static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
-{
-	struct dwc_pwm *dwc;
-
-	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
-	if (!dwc)
-		return NULL;
-
-	dwc->chip.dev = dev;
-	dwc->chip.ops = &dwc_pwm_ops;
-	dwc->chip.npwm = DWC_TIMERS_TOTAL;
-
-	dev_set_drvdata(dev, dwc);
-	return dwc;
-}
+#include "pwm-dwc.h"
 
 static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
new file mode 100644
index 000000000000..56deab4e28ec
--- /dev/null
+++ b/drivers/pwm/pwm-dwc.h
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver
+ *
+ * Copyright (C) 2018-2020 Intel Corporation
+ *
+ * Author: Felipe Balbi (Intel)
+ * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
+ * Author: Raymond Tan <raymond.tan@intel.com>
+ */
+
+MODULE_IMPORT_NS(dwc_pwm);
+
+#define DWC_TIM_LD_CNT(n)	((n) * 0x14)
+#define DWC_TIM_LD_CNT2(n)	(((n) * 4) + 0xb0)
+#define DWC_TIM_CUR_VAL(n)	(((n) * 0x14) + 0x04)
+#define DWC_TIM_CTRL(n)		(((n) * 0x14) + 0x08)
+#define DWC_TIM_EOI(n)		(((n) * 0x14) + 0x0c)
+#define DWC_TIM_INT_STS(n)	(((n) * 0x14) + 0x10)
+
+#define DWC_TIMERS_INT_STS	0xa0
+#define DWC_TIMERS_EOI		0xa4
+#define DWC_TIMERS_RAW_INT_STS	0xa8
+#define DWC_TIMERS_COMP_VERSION	0xac
+
+#define DWC_TIMERS_TOTAL	8
+#define DWC_CLK_PERIOD_NS	10
+
+/* Timer Control Register */
+#define DWC_TIM_CTRL_EN		BIT(0)
+#define DWC_TIM_CTRL_MODE	BIT(1)
+#define DWC_TIM_CTRL_MODE_FREE	(0 << 1)
+#define DWC_TIM_CTRL_MODE_USER	(1 << 1)
+#define DWC_TIM_CTRL_INT_MASK	BIT(2)
+#define DWC_TIM_CTRL_PWM	BIT(3)
+
+struct dwc_pwm_ctx {
+	u32 cnt;
+	u32 cnt2;
+	u32 ctrl;
+};
+
+struct dwc_pwm {
+	struct pwm_chip chip;
+	void __iomem *base;
+	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
+};
+#define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
+
+static inline u32 dwc_pwm_readl(struct dwc_pwm *dwc, u32 offset)
+{
+	return readl(dwc->base + offset);
+}
+
+static inline void dwc_pwm_writel(struct dwc_pwm *dwc, u32 value, u32 offset)
+{
+	writel(value, dwc->base + offset);
+}
+
+extern struct dwc_pwm *dwc_pwm_alloc(struct device *dev);
-- 
2.39.2

[PATCH v8 2/5] pwm: dwc: make timer clock configurable

[Ben Dooks]

Add a configurable clock base rate for the pwm as when being built
for non-PCI the block may be sourced from an internal clock.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
v8:
 - add reviewed by, fixed issue with previous renames.
v7:
 - remove the "struct clk *" clk field from dwc_pwm_ctx, not used here,
v6:
 - removed DWC_CLK_PERIOD_NS as it is now not needed
v4:
 - moved earlier before the of changes to make the of changes one patch
v2:
  - removed the ifdef and merged the other clock patch in here
---
 drivers/pwm/pwm-dwc-core.c | 9 +++++----
 drivers/pwm/pwm-dwc.h      | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
index b693cb7fa812..4b4b7b9e1d82 100644
--- a/drivers/pwm/pwm-dwc-core.c
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -49,13 +49,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	 * periods and check are the result within HW limits between 1 and
 	 * 2^32 periods.
 	 */
-	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
+	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	low = tmp - 1;
 
 	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
-				    DWC_CLK_PERIOD_NS);
+				    dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	high = tmp - 1;
@@ -130,12 +130,12 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
 	duty += 1;
-	duty *= DWC_CLK_PERIOD_NS;
+	duty *= dwc->clk_ns;
 	state->duty_cycle = duty;
 
 	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 	period += 1;
-	period *= DWC_CLK_PERIOD_NS;
+	period *= dwc->clk_ns;
 	period += duty;
 	state->period = period;
 
@@ -160,6 +160,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	if (!dwc)
 		return NULL;
 
+	dwc->clk_ns = 10;
 	dwc->chip.dev = dev;
 	dwc->chip.ops = &dwc_pwm_ops;
 	dwc->chip.npwm = DWC_TIMERS_TOTAL;
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index 56deab4e28ec..64795247c54c 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -24,7 +24,6 @@ MODULE_IMPORT_NS(dwc_pwm);
 #define DWC_TIMERS_COMP_VERSION	0xac
 
 #define DWC_TIMERS_TOTAL	8
-#define DWC_CLK_PERIOD_NS	10
 
 /* Timer Control Register */
 #define DWC_TIM_CTRL_EN		BIT(0)
@@ -43,6 +42,7 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
+	unsigned int clk_ns;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
 #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
-- 
2.39.2

[PATCH v8 3/5] pwm: dwc: add PWM bit unset in get_state call

[Ben Dooks]

If we are not in PWM mode, then the output is technically a 50%
output based on a single timer instead of the high-low based on
the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.

This may only be an issue on initialisation, as the rest of the
code currently assumes we're always going to have the extended
PWM mode using two counters.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v8:
 - fixed rename issues
v4:
 - fixed review comment on mulit-line calculations
---
 drivers/pwm/pwm-dwc-core.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
index 4b4b7b9e1d82..38cd2163fe01 100644
--- a/drivers/pwm/pwm-dwc-core.c
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -122,24 +122,31 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct dwc_pwm *dwc = to_dwc_pwm(chip);
 	u64 duty, period;
+	u32 ctrl, ld, ld2;
 
 	pm_runtime_get_sync(chip->dev);
 
-	state->enabled = !!(dwc_pwm_readl(dwc,
-				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
+	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
+	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
+	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 
-	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
-	duty += 1;
-	duty *= dwc->clk_ns;
-	state->duty_cycle = duty;
+	state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
 
-	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
-	period += 1;
-	period *= dwc->clk_ns;
-	period += duty;
-	state->period = period;
+	/* If we're not in PWM, technically the output is a 50-50
+	 * based on the timer load-count only.
+	 */
+	if (ctrl & DWC_TIM_CTRL_PWM) {
+		duty = (ld + 1) * dwc->clk_ns;
+		period = (ld2 + 1)  * dwc->clk_ns;
+		period += duty;
+	} else {
+		duty = (ld + 1) * dwc->clk_ns;
+		period = duty * 2;
+	}
 
 	state->polarity = PWM_POLARITY_INVERSED;
+	state->period = period;
+	state->duty_cycle = duty;
 
 	pm_runtime_put_sync(chip->dev);
 
-- 
2.39.2

[PATCH v8 4/5] pwm: dwc: use clock rate in hz to avoid rounding issues

[Ben Dooks]

As noted, the clock-rate when not a nice multiple of ns is probably
going to end up with inacurate caculations, as well as on a non pci
system the rate may change (although we've not put a clock rate
change notifier in this code yet) so we also add some quick checks
of the rate when we do any calculations with it.

Signed-off-by; Ben Dooks <ben.dooks@sifive.com>
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
v8:
 - fixup post rename
 - move to earlier in series
---
 drivers/pwm/pwm-dwc-core.c | 24 +++++++++++++++---------
 drivers/pwm/pwm-dwc.h      |  2 +-
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
index 38cd2163fe01..0f07e26e6c30 100644
--- a/drivers/pwm/pwm-dwc-core.c
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -49,13 +49,14 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	 * periods and check are the result within HW limits between 1 and
 	 * 2^32 periods.
 	 */
-	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
+	tmp = state->duty_cycle * dwc->clk_rate;
+	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	low = tmp - 1;
 
-	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
-				    dwc->clk_ns);
+	tmp = (state->period - state->duty_cycle) * dwc->clk_rate;
+	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	high = tmp - 1;
@@ -121,11 +122,14 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 			     struct pwm_state *state)
 {
 	struct dwc_pwm *dwc = to_dwc_pwm(chip);
+	unsigned long clk_rate;
 	u64 duty, period;
 	u32 ctrl, ld, ld2;
 
 	pm_runtime_get_sync(chip->dev);
 
+	clk_rate = dwc->clk_rate;
+
 	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
 	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
 	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
@@ -136,17 +140,19 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * based on the timer load-count only.
 	 */
 	if (ctrl & DWC_TIM_CTRL_PWM) {
-		duty = (ld + 1) * dwc->clk_ns;
-		period = (ld2 + 1)  * dwc->clk_ns;
+		duty = ld + 1;
+		period = ld2 + 1;
 		period += duty;
 	} else {
-		duty = (ld + 1) * dwc->clk_ns;
+		duty = ld + 1;
 		period = duty * 2;
 	}
 
+	duty *= NSEC_PER_SEC;
+	period *= NSEC_PER_SEC;
+	state->period = DIV_ROUND_CLOSEST_ULL(period, clk_rate);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(duty, clk_rate);
 	state->polarity = PWM_POLARITY_INVERSED;
-	state->period = period;
-	state->duty_cycle = duty;
 
 	pm_runtime_put_sync(chip->dev);
 
@@ -167,7 +173,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	if (!dwc)
 		return NULL;
 
-	dwc->clk_ns = 10;
+	dwc->clk_rate = NSEC_PER_SEC / 10;
 	dwc->chip.dev = dev;
 	dwc->chip.ops = &dwc_pwm_ops;
 	dwc->chip.npwm = DWC_TIMERS_TOTAL;
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index 64795247c54c..e0a940fd6e87 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -42,7 +42,7 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
-	unsigned int clk_ns;
+	unsigned long clk_rate;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
 #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
-- 
2.39.2

[PATCH v8 5/5] pwm: dwc: add of/platform support

[Ben Dooks]

The dwc pwm controller can be used in non-PCI systems, so allow
either platform or OF based probing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v8:
 - add compile test for of-case
 - add module namespace
 - move later in the series
v7:
 - fixup kconfig from previous pcie changes
v5:
 - fix missing " in kconfig
 - remove .remove method, devm already sorts this.
 - merge pwm-number code
 - split the of code out of the core
 - get bus clock
v4:
 - moved the compile test code earlier
 - fixed review comments
 - used NS_PER_SEC
 - use devm_clk_get_enabled
 - ensure we get the bus clock
v3:
 - changed compatible name
---
 drivers/pwm/Kconfig        | 10 +++++
 drivers/pwm/Makefile       |  1 +
 drivers/pwm/pwm-dwc-core.c |  6 +++
 drivers/pwm/pwm-dwc-of.c   | 78 ++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-dwc.c      |  1 +
 drivers/pwm/pwm-dwc.h      |  1 +
 6 files changed, 97 insertions(+)
 create mode 100644 drivers/pwm/pwm-dwc-of.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7c54cdcb97a0..61f5d3f30fd7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -205,6 +205,16 @@ config PWM_DWC
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-dwc.
 
+config PWM_DWC_OF
+	tristate "DesignWare PWM Controller (OF bus)"
+	depends on HAS_IOMEM && (OF || COMPILE_TEST)
+	select PWM_DWC_CORE
+	help
+	  PWM driver for Synopsys DWC PWM Controller on an OF bus.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-dwc-of.
+
 config PWM_EP93XX
 	tristate "Cirrus Logic EP93xx PWM support"
 	depends on ARCH_EP93XX || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index de3ed77e8d7c..d27dfbb850b7 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
 obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
 obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
+obj-$(CONFIG_PWM_DWC_OF)	+= pwm-dwc-of.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
 obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
index 0f07e26e6c30..ed102fc4b30a 100644
--- a/drivers/pwm/pwm-dwc-core.c
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/clk.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
@@ -44,6 +45,9 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	u32 high;
 	u32 low;
 
+	if (dwc->clk)
+		dwc->clk_rate = clk_get_rate(dwc->clk);
+
 	/*
 	 * Calculate width of low and high period in terms of input clock
 	 * periods and check are the result within HW limits between 1 and
@@ -128,6 +132,8 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	pm_runtime_get_sync(chip->dev);
 
+	if (dwc->clk)
+		dwc->clk_rate = clk_get_rate(dwc->clk);
 	clk_rate = dwc->clk_rate;
 
 	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
new file mode 100644
index 000000000000..13a0b534b383
--- /dev/null
+++ b/drivers/pwm/pwm-dwc-of.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver OF
+ *
+ * Copyright (C) 2022 SiFive, Inc.
+ */
+
+#define DEFAULT_MODULE_NAMESACE dwc_pwm
+
+#include <linux/bitops.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/io.h>
+
+#include "pwm-dwc.h"
+
+static int dwc_pwm_plat_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dwc_pwm *dwc;
+	struct clk *bus;
+	u32 nr_pwm;
+
+	dwc = dwc_pwm_alloc(dev);
+	if (!dwc)
+		return -ENOMEM;
+
+	if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
+		if (nr_pwm > DWC_TIMERS_TOTAL)
+			dev_err(dev, "too many PWMs (%d) specified, capping at %d\n",
+				nr_pwm, dwc->chip.npwm);
+		else
+			dwc->chip.npwm = nr_pwm;
+	}
+
+	dwc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dwc->base))
+		return PTR_ERR(dwc->base);
+
+	bus = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(bus))
+		return dev_err_probe(dev, PTR_ERR(bus),
+				     "failed to get clock\n");
+
+	dwc->clk = devm_clk_get_enabled(dev, "timer");
+	if (IS_ERR(dwc->clk))
+		return dev_err_probe(dev, PTR_ERR(dwc->clk),
+				     "failed to get timer clock\n");
+
+	dwc->clk_rate = clk_get_rate(dwc->clk);
+	return devm_pwmchip_add(dev, &dwc->chip);
+}
+
+static const struct of_device_id dwc_pwm_dt_ids[] = {
+	{ .compatible = "snps,dw-apb-timers-pwm2" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
+
+static struct platform_driver dwc_pwm_plat_driver = {
+	.driver = {
+		.name		= "dwc-pwm",
+		.of_match_table  = dwc_pwm_dt_ids,
+	},
+	.probe	= dwc_pwm_plat_probe,
+};
+
+module_platform_driver(dwc_pwm_plat_driver);
+
+MODULE_ALIAS("platform:dwc-pwm-of");
+MODULE_AUTHOR("Ben Dooks <ben.dooks@sifive.com>");
+MODULE_DESCRIPTION("DesignWare PWM Controller");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index bd9cadb497d7..7c32bd06ed33 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/clk.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index e0a940fd6e87..18e98c2c07d7 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -42,6 +42,7 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
 	unsigned long clk_rate;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
-- 
2.39.2

Re: [PATCH v8 0/5] DesignWare PWM driver updates

[Jarkko Nikula]

On 6/14/23 20:14, Ben Dooks wrote:
> This series is an update for the DesignWare PWM driver to add support for
> OF (and split the PCI bits out if aynone else wants them). This is mostly
> the same as the v7 series, but with code moved around and module-namespace
> added, plus review comments processed.
> 
> Since we no longer have the hardware, the clock code hasn't been changed to
> either lock the rate whilst the PWM is running, or to deal with any sort
> of change callback. This is left to future work (and I would rather get
> something in that does currently work) (second note, the hardware we did
> use had a fixed clock tree anyway)
> 
> This account is probably going away soon, I have cc'd my main work
> email to catch any responses.
> 
> Thank you all for the reviews.
> 
I tested the patchset on Intel Elkhart Lake and didn't see issues.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v8 0/5] DesignWare PWM driver updates

[Ben Dooks]

On 2023-06-20 13:59, Jarkko Nikula wrote:
> On 6/14/23 20:14, Ben Dooks wrote:
>> This series is an update for the DesignWare PWM driver to add support 
>> for
>> OF (and split the PCI bits out if aynone else wants them). This is 
>> mostly
>> the same as the v7 series, but with code moved around and 
>> module-namespace
>> added, plus review comments processed.
>> 
>> Since we no longer have the hardware, the clock code hasn't been 
>> changed to
>> either lock the rate whilst the PWM is running, or to deal with any 
>> sort
>> of change callback. This is left to future work (and I would rather 
>> get
>> something in that does currently work) (second note, the hardware we 
>> did
>> use had a fixed clock tree anyway)
>> 
>> This account is probably going away soon, I have cc'd my main work
>> email to catch any responses.
>> 
>> Thank you all for the reviews.
>> 
> I tested the patchset on Intel Elkhart Lake and didn't see issues.
> 
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Great, thank you.

Is this series likely to get into the next kernel release?

-- 
ben

Re: [PATCH v8 1/5] pwm: dwc: split pci out of core driver

[Uwe Kleine-König]

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

On Wed, Jun 14, 2023 at 06:14:53PM +0100, Ben Dooks wrote:
> Moving towards adding non-pci support for the driver, move the pci
> parts out of the core into their own module. This is partly due to
> the module_driver() code only being allowed once in a module and also
> to avoid a number of #ifdef if we build a single file in a system
> without pci support.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v8:
>  - add module namespace
>  - remove compile-test for pci case, doesn't make sense
>  - fix makefile, missed config symbol changes
> v7:
>  - re-order kconfig to make dwc core be selected by PCI driver
> v6:
>  - put DWC_PERIOD_NS back to avoid bisect issues
> v4:
>  - removed DWC_PERIOD_NS as not needed
> ---
>  drivers/pwm/Kconfig        |  14 ++-
>  drivers/pwm/Makefile       |   1 +
>  drivers/pwm/pwm-dwc-core.c | 176 +++++++++++++++++++++++++++++++++
>  drivers/pwm/pwm-dwc.c      | 197 +------------------------------------
>  drivers/pwm/pwm-dwc.h      |  60 +++++++++++
>  5 files changed, 253 insertions(+), 195 deletions(-)
>  create mode 100644 drivers/pwm/pwm-dwc-core.c
>  create mode 100644 drivers/pwm/pwm-dwc.h
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8df861b1f4a3..7c54cdcb97a0 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -186,9 +186,19 @@ config PWM_CROS_EC
>  	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
>  	  Controller.
>  
> +config PWM_DWC_CORE
> +	tristate
> +	depends on HAS_IOMEM
> +	help
> +	  PWM driver for Synopsys DWC PWM Controller.
> +
> +	  To compile this driver as a module, build the dependecies as
> +	  modules, this will be called pwm-dwc-core.
> +
>  config PWM_DWC
> -	tristate "DesignWare PWM Controller"
> -	depends on PCI
> +	tristate "DesignWare PWM Controller (PCI bus)"
> +	depends on HAS_IOMEM && PCI
> +	select PWM_DWC_CORE
>  	help
>  	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
>  
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 19899b912e00..de3ed77e8d7c 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ 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
> +obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
>  obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o

Would it make sense to call this pwm-dwc-pci.o? And the symbol
CONFIG_PWM_DWC_PCI? (The latter would break make oldconfig. Hmm, I'm
unsure myself.)

I didn't check all the details, but assuming that this is a split
without further changes it looks ok to me.

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 v8 3/5] pwm: dwc: add PWM bit unset in get_state call

[Uwe Kleine-König]

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

On Wed, Jun 14, 2023 at 06:14:55PM +0100, Ben Dooks wrote:
> If we are not in PWM mode, then the output is technically a 50%
> output based on a single timer instead of the high-low based on
> the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
> and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.
> 
> This may only be an issue on initialisation, as the rest of the
> code currently assumes we're always going to have the extended
> PWM mode using two counters.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v8:
>  - fixed rename issues
> v4:
>  - fixed review comment on mulit-line calculations
> ---
>  drivers/pwm/pwm-dwc-core.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 4b4b7b9e1d82..38cd2163fe01 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -122,24 +122,31 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>  	struct dwc_pwm *dwc = to_dwc_pwm(chip);
>  	u64 duty, period;
> +	u32 ctrl, ld, ld2;
>  
>  	pm_runtime_get_sync(chip->dev);
>  
> -	state->enabled = !!(dwc_pwm_readl(dwc,
> -				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
> +	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
> +	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> +	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
>  
> -	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> -	duty += 1;
> -	duty *= dwc->clk_ns;
> -	state->duty_cycle = duty;
> +	state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
>  
> -	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
> -	period += 1;
> -	period *= dwc->clk_ns;
> -	period += duty;
> -	state->period = period;
> +	/* If we're not in PWM, technically the output is a 50-50

Multiline comments should have /* on a line for itself.

> +	 * based on the timer load-count only.
> +	 */
> +	if (ctrl & DWC_TIM_CTRL_PWM) {
> +		duty = (ld + 1) * dwc->clk_ns;
> +		period = (ld2 + 1)  * dwc->clk_ns;

s/  / /

> +		period += duty;
> +	} else {
> +		duty = (ld + 1) * dwc->clk_ns;
> +		period = duty * 2;
> +	}
>  
>  	state->polarity = PWM_POLARITY_INVERSED;
> +	state->period = period;
> +	state->duty_cycle = duty;
>  
>  	pm_runtime_put_sync(chip->dev);

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 v8 4/5] pwm: dwc: use clock rate in hz to avoid rounding issues

[Uwe Kleine-König]

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

On Wed, Jun 14, 2023 at 06:14:56PM +0100, Ben Dooks wrote:
> As noted, the clock-rate when not a nice multiple of ns is probably
> going to end up with inacurate caculations, as well as on a non pci

s/caculation/calculations/

> system the rate may change (although we've not put a clock rate
> change notifier in this code yet) so we also add some quick checks
> of the rate when we do any calculations with it.

An externally triggered clock rate change is bad. If you drive a motor
you probably want to prevent an uncontrolled change here. I already
considered to add a call to clk_rate_exclusive_get() in various pwm
drivers for that reason, but didn't come around yet.

> Signed-off-by; Ben Dooks <ben.dooks@sifive.com>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> v8:
>  - fixup post rename
>  - move to earlier in series
> ---
>  drivers/pwm/pwm-dwc-core.c | 24 +++++++++++++++---------
>  drivers/pwm/pwm-dwc.h      |  2 +-
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 38cd2163fe01..0f07e26e6c30 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -49,13 +49,14 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
>  	 * periods and check are the result within HW limits between 1 and
>  	 * 2^32 periods.
>  	 */
> -	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
> +	tmp = state->duty_cycle * dwc->clk_rate;
> +	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);

New drivers should implement round-down behaviour (i.e. pick the biggest
period (and duty_cycle) that is not bigger than the requested value.
With clk_ns = 10 (which it is the hardcoded value up to now) it doesn't
matter much how you round the division. I suggest to use the opportunity
to align to how new drivers should round. (That would be a separate
patch.)

>  	if (tmp < 1 || tmp > (1ULL << 32))
>  		return -ERANGE;
>  	low = tmp - 1;
>  
> -	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
> -				    dwc->clk_ns);
> +	tmp = (state->period - state->duty_cycle) * dwc->clk_rate;
> +	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
>  	if (tmp < 1 || tmp > (1ULL << 32))
>  		return -ERANGE;
>  	high = tmp - 1;
> @@ -121,11 +122,14 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     struct pwm_state *state)
>  {
>  	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> +	unsigned long clk_rate;
>  	u64 duty, period;
>  	u32 ctrl, ld, ld2;
>  
>  	pm_runtime_get_sync(chip->dev);
>  
> +	clk_rate = dwc->clk_rate;
> +
>  	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
>  	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
>  	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
> @@ -136,17 +140,19 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * based on the timer load-count only.
>  	 */
>  	if (ctrl & DWC_TIM_CTRL_PWM) {
> -		duty = (ld + 1) * dwc->clk_ns;
> -		period = (ld2 + 1)  * dwc->clk_ns;
> +		duty = ld + 1;
> +		period = ld2 + 1;
>  		period += duty;
>  	} else {
> -		duty = (ld + 1) * dwc->clk_ns;
> +		duty = ld + 1;
>  		period = duty * 2;
>  	}
>  
> +	duty *= NSEC_PER_SEC;
> +	period *= NSEC_PER_SEC;
> +	state->period = DIV_ROUND_CLOSEST_ULL(period, clk_rate);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(duty, clk_rate);
>  	state->polarity = PWM_POLARITY_INVERSED;
> -	state->period = period;
> -	state->duty_cycle = duty;
>  
>  	pm_runtime_put_sync(chip->dev);
>  
> @@ -167,7 +173,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
>  	if (!dwc)
>  		return NULL;
>  
> -	dwc->clk_ns = 10;
> +	dwc->clk_rate = NSEC_PER_SEC / 10;
>  	dwc->chip.dev = dev;
>  	dwc->chip.ops = &dwc_pwm_ops;
>  	dwc->chip.npwm = DWC_TIMERS_TOTAL;
> diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
> index 64795247c54c..e0a940fd6e87 100644
> --- a/drivers/pwm/pwm-dwc.h
> +++ b/drivers/pwm/pwm-dwc.h
> @@ -42,7 +42,7 @@ struct dwc_pwm_ctx {
>  struct dwc_pwm {
>  	struct pwm_chip chip;
>  	void __iomem *base;
> -	unsigned int clk_ns;
> +	unsigned long clk_rate;
>  	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
>  };
>  #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))

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 v8 5/5] pwm: dwc: add of/platform support

[Uwe Kleine-König]

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

On Wed, Jun 14, 2023 at 06:14:57PM +0100, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.

A document describing the binding is needed here.

> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v8:
>  - add compile test for of-case
>  - add module namespace
>  - move later in the series
> v7:
>  - fixup kconfig from previous pcie changes
> v5:
>  - fix missing " in kconfig
>  - remove .remove method, devm already sorts this.
>  - merge pwm-number code
>  - split the of code out of the core
>  - get bus clock
> v4:
>  - moved the compile test code earlier
>  - fixed review comments
>  - used NS_PER_SEC
>  - use devm_clk_get_enabled
>  - ensure we get the bus clock
> v3:
>  - changed compatible name
> ---
>  drivers/pwm/Kconfig        | 10 +++++
>  drivers/pwm/Makefile       |  1 +
>  drivers/pwm/pwm-dwc-core.c |  6 +++
>  drivers/pwm/pwm-dwc-of.c   | 78 ++++++++++++++++++++++++++++++++++++++
>  drivers/pwm/pwm-dwc.c      |  1 +
>  drivers/pwm/pwm-dwc.h      |  1 +
>  6 files changed, 97 insertions(+)
>  create mode 100644 drivers/pwm/pwm-dwc-of.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7c54cdcb97a0..61f5d3f30fd7 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -205,6 +205,16 @@ config PWM_DWC
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-dwc.
>  
> +config PWM_DWC_OF
> +	tristate "DesignWare PWM Controller (OF bus)"
> +	depends on HAS_IOMEM && (OF || COMPILE_TEST)
> +	select PWM_DWC_CORE
> +	help
> +	  PWM driver for Synopsys DWC PWM Controller on an OF bus.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-dwc-of.
> +
>  config PWM_EP93XX
>  	tristate "Cirrus Logic EP93xx PWM support"
>  	depends on ARCH_EP93XX || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index de3ed77e8d7c..d27dfbb850b7 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
>  obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
>  obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
> +obj-$(CONFIG_PWM_DWC_OF)	+= pwm-dwc-of.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
>  obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 0f07e26e6c30..ed102fc4b30a 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -16,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/clk.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
>  
> @@ -44,6 +45,9 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
>  	u32 high;
>  	u32 low;
>  
> +	if (dwc->clk)
> +		dwc->clk_rate = clk_get_rate(dwc->clk);
> +
>  	/*
>  	 * Calculate width of low and high period in terms of input clock
>  	 * periods and check are the result within HW limits between 1 and
> @@ -128,6 +132,8 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	pm_runtime_get_sync(chip->dev);
>  
> +	if (dwc->clk)
> +		dwc->clk_rate = clk_get_rate(dwc->clk);
>  	clk_rate = dwc->clk_rate;
>  
>  	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> new file mode 100644
> index 000000000000..13a0b534b383
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-of.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DesignWare PWM Controller driver OF
> + *
> + * Copyright (C) 2022 SiFive, Inc.
> + */
> +
> +#define DEFAULT_MODULE_NAMESACE dwc_pwm

missing P? I'd have put this into drivers/pwm/pwm-dwc.h.

> +
> +#include <linux/bitops.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/io.h>
> +
> +#include "pwm-dwc.h"
> +
> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dwc_pwm *dwc;
> +	struct clk *bus;
> +	u32 nr_pwm;
> +
> +	dwc = dwc_pwm_alloc(dev);
> +	if (!dwc)
> +		return -ENOMEM;
> +
> +	if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
> +		if (nr_pwm > DWC_TIMERS_TOTAL)
> +			dev_err(dev, "too many PWMs (%d) specified, capping at %d\n",
> +				nr_pwm, dwc->chip.npwm);
> +		else
> +			dwc->chip.npwm = nr_pwm;
> +	}
> +
> +	dwc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(dwc->base))
> +		return PTR_ERR(dwc->base);
> +
> +	bus = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(bus))
> +		return dev_err_probe(dev, PTR_ERR(bus),
> +				     "failed to get clock\n");
> +
> +	dwc->clk = devm_clk_get_enabled(dev, "timer");
> +	if (IS_ERR(dwc->clk))
> +		return dev_err_probe(dev, PTR_ERR(dwc->clk),
> +				     "failed to get timer clock\n");
> +
> +	dwc->clk_rate = clk_get_rate(dwc->clk);

Do you need this here? Isn't clk_rate assigned each time it's used when
clk != NULL?

> +	return devm_pwmchip_add(dev, &dwc->chip);
> +}
> +
> +static const struct of_device_id dwc_pwm_dt_ids[] = {
> +	{ .compatible = "snps,dw-apb-timers-pwm2" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
> +
> +static struct platform_driver dwc_pwm_plat_driver = {
> +	.driver = {
> +		.name		= "dwc-pwm",
> +		.of_match_table  = dwc_pwm_dt_ids,
> +	},
> +	.probe	= dwc_pwm_plat_probe,
> +};
> +
> +module_platform_driver(dwc_pwm_plat_driver);
> +
> +MODULE_ALIAS("platform:dwc-pwm-of");
> +MODULE_AUTHOR("Ben Dooks <ben.dooks@sifive.com>");

Given that this email address is (or soon will be) unavailable, maybe
better put your codethink address here?

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 v8 1/5] pwm: dwc: split pci out of core driver

[Ben Dooks]

On 15/07/2023 20:28, Uwe Kleine-König wrote:
> On Wed, Jun 14, 2023 at 06:14:53PM +0100, Ben Dooks wrote:
>> Moving towards adding non-pci support for the driver, move the pci
>> parts out of the core into their own module. This is partly due to
>> the module_driver() code only being allowed once in a module and also
>> to avoid a number of #ifdef if we build a single file in a system
>> without pci support.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>> v8:
>>   - add module namespace
>>   - remove compile-test for pci case, doesn't make sense
>>   - fix makefile, missed config symbol changes
>> v7:
>>   - re-order kconfig to make dwc core be selected by PCI driver
>> v6:
>>   - put DWC_PERIOD_NS back to avoid bisect issues
>> v4:
>>   - removed DWC_PERIOD_NS as not needed
>> ---
>>   drivers/pwm/Kconfig        |  14 ++-
>>   drivers/pwm/Makefile       |   1 +
>>   drivers/pwm/pwm-dwc-core.c | 176 +++++++++++++++++++++++++++++++++
>>   drivers/pwm/pwm-dwc.c      | 197 +------------------------------------
>>   drivers/pwm/pwm-dwc.h      |  60 +++++++++++
>>   5 files changed, 253 insertions(+), 195 deletions(-)
>>   create mode 100644 drivers/pwm/pwm-dwc-core.c
>>   create mode 100644 drivers/pwm/pwm-dwc.h
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 8df861b1f4a3..7c54cdcb97a0 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -186,9 +186,19 @@ config PWM_CROS_EC
>>   	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
>>   	  Controller.
>>   
>> +config PWM_DWC_CORE
>> +	tristate
>> +	depends on HAS_IOMEM
>> +	help
>> +	  PWM driver for Synopsys DWC PWM Controller.
>> +
>> +	  To compile this driver as a module, build the dependecies as
>> +	  modules, this will be called pwm-dwc-core.
>> +
>>   config PWM_DWC
>> -	tristate "DesignWare PWM Controller"
>> -	depends on PCI
>> +	tristate "DesignWare PWM Controller (PCI bus)"
>> +	depends on HAS_IOMEM && PCI
>> +	select PWM_DWC_CORE
>>   	help
>>   	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
>>   
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 19899b912e00..de3ed77e8d7c 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -15,6 +15,7 @@ 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
>> +obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
>>   obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
> 
> Would it make sense to call this pwm-dwc-pci.o? And the symbol
> CONFIG_PWM_DWC_PCI? (The latter would break make oldconfig. Hmm, I'm
> unsure myself.)

i left the pci as the pwm-dwc so that anyone moving up and using
this as a module won't have to change config or their module loading
if they're not autoloading modules.

> I didn't check all the details, but assuming that this is a split
> without further changes it looks ok to me.
> 
> Best regards
> Uwe
> 

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v8 1/5] pwm: dwc: split pci out of core driver

[Uwe Kleine-König]

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

On Mon, Jul 17, 2023 at 08:07:10AM +0100, Ben Dooks wrote:
> On 15/07/2023 20:28, Uwe Kleine-König wrote:
> > On Wed, Jun 14, 2023 at 06:14:53PM +0100, Ben Dooks wrote:
> > > Moving towards adding non-pci support for the driver, move the pci
> > > parts out of the core into their own module. This is partly due to
> > > the module_driver() code only being allowed once in a module and also
> > > to avoid a number of #ifdef if we build a single file in a system
> > > without pci support.
> > > 
> > > Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> > > ---
> > > v8:
> > >   - add module namespace
> > >   - remove compile-test for pci case, doesn't make sense
> > >   - fix makefile, missed config symbol changes
> > > v7:
> > >   - re-order kconfig to make dwc core be selected by PCI driver
> > > v6:
> > >   - put DWC_PERIOD_NS back to avoid bisect issues
> > > v4:
> > >   - removed DWC_PERIOD_NS as not needed
> > > ---
> > >   drivers/pwm/Kconfig        |  14 ++-
> > >   drivers/pwm/Makefile       |   1 +
> > >   drivers/pwm/pwm-dwc-core.c | 176 +++++++++++++++++++++++++++++++++
> > >   drivers/pwm/pwm-dwc.c      | 197 +------------------------------------
> > >   drivers/pwm/pwm-dwc.h      |  60 +++++++++++
> > >   5 files changed, 253 insertions(+), 195 deletions(-)
> > >   create mode 100644 drivers/pwm/pwm-dwc-core.c
> > >   create mode 100644 drivers/pwm/pwm-dwc.h
> > > 
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 8df861b1f4a3..7c54cdcb97a0 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -186,9 +186,19 @@ config PWM_CROS_EC
> > >   	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
> > >   	  Controller.
> > > +config PWM_DWC_CORE
> > > +	tristate
> > > +	depends on HAS_IOMEM
> > > +	help
> > > +	  PWM driver for Synopsys DWC PWM Controller.
> > > +
> > > +	  To compile this driver as a module, build the dependecies as
> > > +	  modules, this will be called pwm-dwc-core.
> > > +
> > >   config PWM_DWC
> > > -	tristate "DesignWare PWM Controller"
> > > -	depends on PCI
> > > +	tristate "DesignWare PWM Controller (PCI bus)"
> > > +	depends on HAS_IOMEM && PCI
> > > +	select PWM_DWC_CORE
> > >   	help
> > >   	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 19899b912e00..de3ed77e8d7c 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -15,6 +15,7 @@ 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
> > > +obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
> > >   obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
> > 
> > Would it make sense to call this pwm-dwc-pci.o? And the symbol
> > CONFIG_PWM_DWC_PCI? (The latter would break make oldconfig. Hmm, I'm
> > unsure myself.)
> 
> i left the pci as the pwm-dwc so that anyone moving up and using
> this as a module won't have to change config or their module loading
> if they're not autoloading modules.

Yeah, I thought so. I don't care much either way, but maybe that's worth
mentioning in the commit log or even a comment in drivers/pwm/Makefile.

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