3rd version. No changes to the core except that I added F: drivers/pwm/ to
MAINTAINERS. Arnd, I looked through the comments to the first series again
and this was the only thing I could find I forgot. You referred to three
things. Is there more I forgot?
What changed though is the mxs pwm driver. I now register a pwm core
device which handles the shared enable register as suggested by Arnd.
Sascha
The following changes since commit b0af8dfdd67699e25083478c63eedef2e72ebd85:
Linux 3.0-rc5 (2011-06-27 19:12:22 -0700)
are available in the git repository at:
git://git.pengutronix.de/git/imx/linux-2.6.git pwm
Sascha Hauer (3):
PWM: add pwm framework support
ARM mxs: adjust pwm resources to what the driver expects
pwm: Add a i.MX23/28 pwm driver
Documentation/pwm.txt | 56 +++++
MAINTAINERS | 6 +
arch/arm/mach-mxs/devices/platform-mxs-pwm.c | 32 +++-
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/pwm/Kconfig | 16 ++
drivers/pwm/Makefile | 2 +
drivers/pwm/core.c | 220 ++++++++++++++++++
drivers/pwm/mxs-pwm.c | 312 ++++++++++++++++++++++++++
include/linux/pwm.h | 37 +++
10 files changed, 681 insertions(+), 3 deletions(-)
create mode 100644 Documentation/pwm.txt
create mode 100644 drivers/pwm/Kconfig
create mode 100644 drivers/pwm/Makefile
create mode 100644 drivers/pwm/core.c
create mode 100644 drivers/pwm/mxs-pwm.c
This patch adds framework support for PWM (pulse width modulation) devices.
The is a barebone PWM API already in the kernel under include/linux/pwm.h,
but it does not allow for multiple drivers as each of them implements the
pwm_*() functions.
There are other PWM framework patches around from Bill Gatliff. Unlike
his framework this one does not change the existing API for PWMs so that
this framework can act as a drop in replacement for the existing API.
Why another framework?
Several people argue that there should not be another framework for PWMs
but they should be integrated into one of the existing frameworks like led
or hwmon. Unlike these frameworks the PWM framework is agnostic to the
purpose of the PWM. In fact, a PWM can drive a LED, but this makes the
LED framework a user of a PWM, like already done in leds-pwm.c. The gpio
framework also is not suitable for PWMs. Every gpio could be turned into
a PWM using timer based toggling, but on the other hand not every PWM hardware
device can be turned into a gpio due to the lack of hardware capabilities.
This patch does not try to improve the PWM API yet, this could be done in
subsequent patches.
Signed-off-by: Sascha Hauer <[email protected]>
Acked-by: Kurt Van Dijck <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
Documentation/pwm.txt | 56 +++++++++++++
MAINTAINERS | 6 ++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/pwm/Kconfig | 12 +++
drivers/pwm/Makefile | 1 +
drivers/pwm/core.c | 220 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pwm.h | 37 ++++++++
8 files changed, 335 insertions(+), 0 deletions(-)
create mode 100644 Documentation/pwm.txt
create mode 100644 drivers/pwm/Kconfig
create mode 100644 drivers/pwm/Makefile
create mode 100644 drivers/pwm/core.c
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
new file mode 100644
index 0000000..c7c5cb1
--- /dev/null
+++ b/Documentation/pwm.txt
@@ -0,0 +1,56 @@
+Pulse Width Modulation (PWM) interface
+
+This provides an overview about the Linux PWM interface
+
+PWMs are commonly used for controlling LEDs, fans or vibrators in
+cell phones. PWMs with a fixed purpose have no need implementing
+the Linux PWM API (although they could). However, PWMs are often
+found as discrete devices on SoCs which have no fixed purpose. It's
+up to the board designer to connect them to LEDs or fans. To provide
+this kind of flexibility the generic PWM API exists.
+
+Identifying PWMs
+----------------
+
+PWMs are identified by unique ids throughout the system. A platform
+should call pwmchip_reserve() during init time to reserve the id range
+for internal PWMs so that users have a fixed id to refer to specific
+PWMs.
+
+Using PWMs
+----------
+
+A PWM can be requested using pwm_request() and freed after usage with
+pwm_free(). After being requested a PWM has to be configured using
+
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
+
+To start/stop toggling the PWM output use pwm_enable()/pwm_disable().
+
+Implementing a PWM driver
+-------------------------
+
+Currently there are two ways to implement pwm drivers. Traditionally
+there only has been the barebone API meaning that each driver has
+to implement the pwm_*() functions itself. This means that it's impossible
+to have multiple PWM drivers in the system. For this reason it's mandatory
+for new drivers to use the generic PWM framework.
+A new PWM device can be added using pwmchip_add() and removed again with
+pwmchip_remove(). pwmchip_add() takes a filled in struct pwm_chip as
+argument which provides the ops and the pwm id to the framework.
+
+Locking
+-------
+
+The PWM core list manipulations are protected by a mutex, so pwm_request()
+and pwm_free() may not be called from an atomic context. Currently the
+PWM core does not enforce any locking to pwm_enable(), pwm_disable() and
+pwm_config(), so the calling context is currently driver specific. This
+is an issue derived from the former barebone API and should be fixed soon.
+
+Helpers
+-------
+
+Currently a PWM can only be configured with period_ns and duty_ns. For several
+use cases freq_hz and duty_percent might be better. Instead of calculating
+this in your driver please consider adding appropriate helpers to the framework.
diff --git a/MAINTAINERS b/MAINTAINERS
index f0358cd..79c047b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5142,6 +5142,12 @@ M: Robert Jarzmik <[email protected]>
L: [email protected]
S: Maintained
+PWM core
+M: Sascha Hauer <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/pwm/
+
QLOGIC QLA1280 SCSI DRIVER
M: Michael Reed <[email protected]>
L: [email protected]
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 3bb154d..67f5c27 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -126,4 +126,6 @@ source "drivers/hwspinlock/Kconfig"
source "drivers/clocksource/Kconfig"
+source "drivers/pwm/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 09f3232..c321763 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -6,6 +6,7 @@
#
obj-y += gpio/
+obj-y += pwm/
obj-$(CONFIG_PCI) += pci/
obj-$(CONFIG_PARISC) += parisc/
obj-$(CONFIG_RAPIDIO) += rapidio/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 0000000..93c1052
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,12 @@
+menuconfig PWM
+ bool "PWM Support"
+ help
+ This enables PWM support through the generic PWM framework.
+ You only need to enable this, if you also want to enable
+ one or more of the PWM drivers below.
+
+ If unsure, say N.
+
+if PWM
+
+endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 0000000..3469c3d
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PWM) += core.o
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
new file mode 100644
index 0000000..71de479
--- /dev/null
+++ b/drivers/pwm/core.c
@@ -0,0 +1,220 @@
+/*
+ * Generic pwmlib implementation
+ *
+ * Copyright (C) 2011 Sascha Hauer <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING. If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/module.h>
+#include <linux/pwm.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+
+struct pwm_device {
+ struct pwm_chip *chip;
+ const char *label;
+ unsigned long flags;
+#define FLAG_REQUESTED 0
+#define FLAG_ENABLED 1
+ struct list_head node;
+};
+
+static LIST_HEAD(pwm_list);
+
+static DEFINE_MUTEX(pwm_lock);
+
+static struct pwm_device *_find_pwm(int pwm_id)
+{
+ struct pwm_device *pwm;
+
+ list_for_each_entry(pwm, &pwm_list, node) {
+ if (pwm->chip->pwm_id == pwm_id)
+ return pwm;
+ }
+
+ return NULL;
+}
+
+/**
+ * pwmchip_add() - register a new pwm
+ * @chip: the pwm
+ *
+ * register a new pwm. pwm->pwm_id must be initialized. if pwm_id < 0 then
+ * a dynamically assigned id will be used, otherwise the id specified,
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+ struct pwm_device *pwm;
+ int ret = 0;
+
+ pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ pwm->chip = chip;
+
+ mutex_lock(&pwm_lock);
+
+ if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ list_add_tail(&pwm->node, &pwm_list);
+out:
+ mutex_unlock(&pwm_lock);
+
+ if (ret)
+ kfree(pwm);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_add);
+
+/**
+ * pwmchip_remove() - remove a pwm
+ * @chip: the pwm
+ *
+ * remove a pwm. This function may return busy if the pwm is still requested.
+ */
+int pwmchip_remove(struct pwm_chip *chip)
+{
+ struct pwm_device *pwm;
+ int ret = 0;
+
+ mutex_lock(&pwm_lock);
+
+ pwm = _find_pwm(chip->pwm_id);
+ if (!pwm) {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ list_del(&pwm->node);
+
+ kfree(pwm);
+out:
+ mutex_unlock(&pwm_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_remove);
+
+/*
+ * pwm_request - request a PWM device
+ */
+struct pwm_device *pwm_request(int pwm_id, const char *label)
+{
+ struct pwm_device *pwm;
+ int ret;
+
+ mutex_lock(&pwm_lock);
+
+ pwm = _find_pwm(pwm_id);
+ if (!pwm) {
+ pwm = ERR_PTR(-ENOENT);
+ goto out;
+ }
+
+ if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
+ pwm = ERR_PTR(-EBUSY);
+ goto out;
+ }
+
+ if (!try_module_get(pwm->chip->ops->owner)) {
+ pwm = ERR_PTR(-ENODEV);
+ goto out;
+ }
+
+ if (pwm->chip->ops->request) {
+ ret = pwm->chip->ops->request(pwm->chip);
+ if (ret) {
+ pwm = ERR_PTR(ret);
+ goto out_put;
+ }
+ }
+
+ pwm->label = label;
+ set_bit(FLAG_REQUESTED, &pwm->flags);
+
+ goto out;
+
+out_put:
+ module_put(pwm->chip->ops->owner);
+out:
+ mutex_unlock(&pwm_lock);
+
+ return pwm;
+}
+EXPORT_SYMBOL_GPL(pwm_request);
+
+/*
+ * pwm_free - free a PWM device
+ */
+void pwm_free(struct pwm_device *pwm)
+{
+ mutex_lock(&pwm_lock);
+
+ if (!test_and_clear_bit(FLAG_REQUESTED, &pwm->flags)) {
+ pr_warning("PWM device already freed\n");
+ goto out;
+ }
+
+ pwm->label = NULL;
+
+ module_put(pwm->chip->ops->owner);
+out:
+ mutex_unlock(&pwm_lock);
+}
+EXPORT_SYMBOL_GPL(pwm_free);
+
+/*
+ * pwm_config - change a PWM device configuration
+ */
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+ return pwm->chip->ops->config(pwm->chip, duty_ns, period_ns);
+}
+EXPORT_SYMBOL_GPL(pwm_config);
+
+/*
+ * pwm_enable - start a PWM output toggling
+ */
+int pwm_enable(struct pwm_device *pwm)
+{
+ if (!test_and_set_bit(FLAG_ENABLED, &pwm->flags))
+ return pwm->chip->ops->enable(pwm->chip);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pwm_enable);
+
+/*
+ * pwm_disable - stop a PWM output toggling
+ */
+void pwm_disable(struct pwm_device *pwm)
+{
+ if (test_and_clear_bit(FLAG_ENABLED, &pwm->flags))
+ pwm->chip->ops->disable(pwm->chip);
+}
+EXPORT_SYMBOL_GPL(pwm_disable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 7c77575..df9681b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -28,4 +28,41 @@ int pwm_enable(struct pwm_device *pwm);
*/
void pwm_disable(struct pwm_device *pwm);
+#ifdef CONFIG_PWM
+struct pwm_chip;
+
+/**
+ * struct pwm_ops - PWM operations
+ * @request: optional hook for requesting a PWM
+ * @free: optional hook for freeing a PWM
+ * @config: configure duty cycles and period length for this PWM
+ * @enable: enable PWM output toggling
+ * @disable: disable PWM output toggling
+ */
+struct pwm_ops {
+ int (*request)(struct pwm_chip *chip);
+ void (*free)(struct pwm_chip *chip);
+ int (*config)(struct pwm_chip *chip, int duty_ns,
+ int period_ns);
+ int (*enable)(struct pwm_chip *chip);
+ void (*disable)(struct pwm_chip *chip);
+ struct module *owner;
+};
+
+/**
+ * struct pwm_chip - abstract a PWM
+ * @label: for diagnostics
+ * @owner: helps prevent removal of modules exporting active PWMs
+ * @ops: The callbacks for this PWM
+ */
+struct pwm_chip {
+ int pwm_id;
+ const char *label;
+ struct pwm_ops *ops;
+};
+
+int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_remove(struct pwm_chip *chip);
+#endif
+
#endif /* __LINUX_PWM_H */
--
1.7.5.3
The PWMs on i.MX23/28 have almost seperated register spaces
but share a common enable register. To reflect this register
a parent device to the PWMs which handles the enable register.
Signed-off-by: Sascha Hauer <[email protected]>
---
arch/arm/mach-mxs/devices/platform-mxs-pwm.c | 32 +++++++++++++++++++++++--
1 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-mxs/devices/platform-mxs-pwm.c b/arch/arm/mach-mxs/devices/platform-mxs-pwm.c
index 680f5a9..36580b7 100644
--- a/arch/arm/mach-mxs/devices/platform-mxs-pwm.c
+++ b/arch/arm/mach-mxs/devices/platform-mxs-pwm.c
@@ -9,14 +9,40 @@
#include <asm/sizes.h>
#include <mach/devices-common.h>
+static struct platform_device *__init mxs_add_pwm_core(resource_size_t iobase)
+{
+ struct resource res = {
+ .flags = IORESOURCE_MEM,
+ .start = iobase,
+ .end = iobase + 0xff,
+ };
+
+ return mxs_add_platform_device("mxs-pwm-core", 0, &res, 1, NULL, 0);
+}
+
struct platform_device *__init mxs_add_mxs_pwm(resource_size_t iobase, int id)
{
+ struct resource *r;
+
+ static struct platform_device *pwm_core;
struct resource res = {
.flags = IORESOURCE_MEM,
+ .start = iobase + 0x10 + 0x20 * id,
+ .end = iobase + 0x10 + 0x20 * id + 0x1f,
};
- res.start = iobase + 0x10 + 0x20 * id;
- res.end = res.start + 0x1f;
+ if (!pwm_core) {
+ pwm_core = mxs_add_pwm_core(iobase);
+ if (!pwm_core)
+ return NULL;
+ }
+
+ r = platform_get_resource(pwm_core, IORESOURCE_MEM, 0);
+ if (!r)
+ return NULL;
+
+ res.parent = r;
- return mxs_add_platform_device("mxs-pwm", id, &res, 1, NULL, 0);
+ return platform_device_register_resndata(&pwm_core->dev, "mxs-pwm",
+ id, &res, 1, NULL, 0);
}
--
1.7.5.3
Signed-off-by: Sascha Hauer <[email protected]>
---
drivers/pwm/Kconfig | 6 +-
drivers/pwm/Makefile | 1 +
drivers/pwm/mxs-pwm.c | 312 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 318 insertions(+), 1 deletions(-)
create mode 100644 drivers/pwm/mxs-pwm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 93c1052..5694574 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -2,11 +2,15 @@ menuconfig PWM
bool "PWM Support"
help
This enables PWM support through the generic PWM framework.
- You only need to enable this, if you also want to enable
+ You only need to enable this if you also want to enable
one or more of the PWM drivers below.
If unsure, say N.
if PWM
+config PWM_MXS
+ tristate "MXS pwm support"
+ depends on ARCH_MXS
+
endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 3469c3d..2cadd50 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1 +1,2 @@
obj-$(CONFIG_PWM) += core.o
+obj-$(CONFIG_PWM_MXS) += mxs-pwm.o
diff --git a/drivers/pwm/mxs-pwm.c b/drivers/pwm/mxs-pwm.c
new file mode 100644
index 0000000..150ab32
--- /dev/null
+++ b/drivers/pwm/mxs-pwm.c
@@ -0,0 +1,312 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Sascha Hauer <[email protected]>
+ *
+ * simple driver for PWM (Pulse Width Modulator) controller
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Derived from pxa PWM driver by eric miao <[email protected]>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <mach/hardware.h>
+#include <asm/div64.h>
+
+enum mxs_pwm_devtype {
+ MXS_PWM_CORE,
+ MXS_PWM,
+};
+
+struct mxs_pwm_device {
+ struct device *dev;
+
+ struct clk *clk;
+
+ int enabled;
+ void __iomem *mmio_base;
+ void __iomem *enable_base;
+
+ unsigned int pwm_id;
+
+ u32 val_active;
+ u32 val_period;
+ int period_us;
+ struct pwm_chip chip;
+};
+
+/* common register space */
+#define REG_PWM_CTRL 0x0
+#define REG_PWM_CTRL_SET 0x4
+#define REG_PWM_CTRL_CLEAR 0x8
+#define PWM_SFTRST (1 << 31)
+#define PWM_CLKGATE (1 << 30)
+#define PWM_ENABLE(p) (1 << (p))
+
+/* per pwm register space */
+#define REG_ACTIVE 0x0
+#define REG_PERIOD 0x10
+
+#define PERIOD_PERIOD(p) ((p) & 0xffff)
+#define PERIOD_ACTIVE_HIGH (3 << 16)
+#define PERIOD_INACTIVE_LOW (2 << 18)
+#define PERIOD_CDIV(div) (((div) & 0x7) << 20)
+
+static void pwm_update(struct mxs_pwm_device *pwm)
+{
+ writel(pwm->val_active, pwm->mmio_base + REG_ACTIVE);
+ writel(pwm->val_period, pwm->mmio_base + REG_PERIOD);
+}
+
+#define to_mxs_pwm_device(chip) container_of(chip, struct mxs_pwm_device, chip)
+
+static int mxs_pwm_config(struct pwm_chip *chip, int duty_ns, int period_ns)
+{
+ struct mxs_pwm_device *pwm = to_mxs_pwm_device(chip);
+ int div = 0;
+ unsigned long rate;
+ unsigned long long c;
+ unsigned long period_cycles, duty_cycles;
+
+ rate = clk_get_rate(pwm->clk);
+
+ dev_dbg(pwm->dev, "config: duty_ns: %d, period_ns: %d (clkrate %ld)\n",
+ duty_ns, period_ns, rate);
+
+ while (1) {
+ c = rate / (1 << div);
+ c = c * period_ns;
+ do_div(c, 1000000000);
+ if (c < 0x10000)
+ break;
+ div++;
+
+ if (div > 8)
+ return -EINVAL;
+ }
+
+ period_cycles = c;
+ duty_cycles = period_cycles * duty_ns / period_ns;
+
+ dev_dbg(pwm->dev, "config period_cycles: %ld duty_cycles: %ld\n",
+ period_cycles, duty_cycles);
+
+ pwm->val_active = period_cycles << 16 | duty_cycles;
+ pwm->val_period = PERIOD_PERIOD(period_cycles) | PERIOD_ACTIVE_HIGH |
+ PERIOD_INACTIVE_LOW | PERIOD_CDIV(div);
+ pwm->period_us = period_ns / 1000;
+
+ pwm_update(pwm);
+
+ return 0;
+}
+
+static void __pwm_enable(struct mxs_pwm_device *pwm, int enable)
+{
+ void __iomem *reg;
+
+ if (enable)
+ reg = pwm->enable_base + REG_PWM_CTRL_SET;
+ else
+ reg = pwm->enable_base + REG_PWM_CTRL_CLEAR;
+
+ writel(PWM_ENABLE(pwm->chip.pwm_id), reg);
+}
+
+static int mxs_pwm_enable(struct pwm_chip *chip)
+{
+ struct mxs_pwm_device *pwm = to_mxs_pwm_device(chip);
+ int ret = 0;
+
+ dev_dbg(pwm->dev, "enable\n");
+
+ if (!pwm->enabled) {
+ ret = clk_enable(pwm->clk);
+ if (!ret) {
+ pwm->enabled = 1;
+ __pwm_enable(pwm, 1);
+ pwm_update(pwm);
+ }
+ }
+ return ret;
+}
+
+static void mxs_pwm_disable(struct pwm_chip *chip)
+{
+ struct mxs_pwm_device *pwm = to_mxs_pwm_device(chip);
+
+ dev_dbg(pwm->dev, "disable\n");
+
+ if (pwm->enabled) {
+ /*
+ * We need a little delay here, it takes one period for
+ * the last pwm_config call to take effect. If we disable
+ * the pwm too early it just freezes the current output
+ * state.
+ */
+ usleep_range(pwm->period_us, pwm->period_us * 2);
+ __pwm_enable(pwm, 0);
+ clk_disable(pwm->clk);
+ pwm->enabled = 0;
+ }
+}
+
+static struct pwm_ops mxs_pwm_ops = {
+ .enable = mxs_pwm_enable,
+ .disable = mxs_pwm_disable,
+ .config = mxs_pwm_config,
+ .owner = THIS_MODULE,
+};
+
+static int mxs_pwm_core_probe(struct platform_device *pdev,
+ struct mxs_pwm_device *pwm)
+{
+ struct resource *r;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!r)
+ return -ENODEV;
+
+ r = devm_request_mem_region(&pdev->dev, r->start, 0xf, pdev->name);
+ if (!r)
+ return -EBUSY;
+
+ pwm->mmio_base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+ if (!pwm->mmio_base) {
+ dev_err(&pdev->dev, "failed to ioremap() registers\n");
+ return -ENOMEM;
+ }
+
+ writel(PWM_SFTRST | PWM_CLKGATE, pwm->mmio_base + REG_PWM_CTRL_CLEAR);
+
+ return 0;
+}
+
+static int mxs_pwm_device_probe(struct platform_device *pdev,
+ struct mxs_pwm_device *pwm)
+{
+ struct mxs_pwm_device *parent_pwm;
+ struct resource *r;
+ int ret;
+
+ pwm->chip.ops = &mxs_pwm_ops;
+ pwm->chip.pwm_id = pdev->id;
+
+ parent_pwm = dev_get_drvdata(pdev->dev.parent);
+ if (!parent_pwm) {
+ dev_err(&pdev->dev, "no parent driver data\n");
+ return -EINVAL;
+ }
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!r)
+ return -ENODEV;
+
+ r = devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
+ pdev->name);
+ if (!r)
+ return -EBUSY;
+
+ pwm->enable_base = parent_pwm->mmio_base;
+ pwm->mmio_base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+
+ pwm->clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pwm->clk))
+ return PTR_ERR(pwm->clk);
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret) {
+ clk_put(pwm->clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int __devinit mxs_pwm_probe(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
+ if (!pwm) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ pwm->dev = &pdev->dev;
+
+ platform_set_drvdata(pdev, pwm);
+
+ if (pdev->id_entry->driver_data == MXS_PWM_CORE)
+ return mxs_pwm_core_probe(pdev, pwm);
+ else
+ return mxs_pwm_device_probe(pdev, pwm);
+
+ return 0;
+}
+
+static int __devexit mxs_pwm_remove(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm = platform_get_drvdata(pdev);
+ int ret;
+
+ switch (pdev->id_entry->driver_data) {
+ case MXS_PWM_CORE:
+ writel(PWM_CLKGATE, pwm->mmio_base + REG_PWM_CTRL_SET);
+ return 0;
+ case MXS_PWM:
+ ret = pwmchip_remove(&pwm->chip);
+ if (ret)
+ return ret;
+
+ clk_put(pwm->clk);
+ }
+
+ return 0;
+}
+
+static struct platform_device_id mxs_pwm_devtype[] = {
+ {
+ .name = "mxs-pwm-core",
+ .driver_data = MXS_PWM_CORE,
+ }, {
+ .name = "mxs-pwm",
+ .driver_data = MXS_PWM,
+ }, {
+ /* sentinel */
+ }
+};
+
+static struct platform_driver mxs_pwm_driver = {
+ .driver = {
+ .name = "mxs-pwm",
+ },
+ .id_table = mxs_pwm_devtype,
+ .probe = mxs_pwm_probe,
+ .remove = __devexit_p(mxs_pwm_remove),
+};
+
+static int __init mxs_pwm_init(void)
+{
+ return platform_driver_register(&mxs_pwm_driver);
+}
+arch_initcall(mxs_pwm_init);
+
+static void __exit mxs_pwm_exit(void)
+{
+ platform_driver_unregister(&mxs_pwm_driver);
+}
+module_exit(mxs_pwm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sascha Hauer <[email protected]>");
--
1.7.5.3
Sascha:
I'm completely against this effort.
The objections to my submission weren't ever because I was changing
the user-visible API, so I don't think you can claim any advantage to
mine in that regard.
I will take some blame for not getting my API finished, but I have
been fighting some serious non-work issues that have consumed nearly
all of my available time--- including the professional time I would
otherwise have to invest in getting my code finished.
Is there the possibility that we could cooperate to get my patches
finished, rather than discarding and reinventing them completely?
b.g.
--
Bill Gatliff
[email protected]
On Thursday 30 June 2011, Sascha Hauer wrote:
> The PWMs on i.MX23/28 have almost seperated register spaces
> but share a common enable register. To reflect this register
> a parent device to the PWMs which handles the enable register.
>
> Signed-off-by: Sascha Hauer <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
On Thursday 30 June 2011, Sascha Hauer wrote:
> Signed-off-by: Sascha Hauer <[email protected]>
Hi Sascha,
The probing looks good to me now. I would have added extra code to avoid
ioremapping the same page multiple times, but I see this as a matter of
different preferences, so I'm fine with that.
You are still missing a description of the driver, both in the changelog
above and in the Kconfig patch. A small sentence to spell out PWM
in the changelog and to list the devices that will use this driver
would be helpful to the causal reader.
Provided that you add that:
Reviewed-by: Arnd Bergmann <[email protected]>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 93c1052..5694574 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -2,11 +2,15 @@ menuconfig PWM
> bool "PWM Support"
> help
> This enables PWM support through the generic PWM framework.
> - You only need to enable this, if you also want to enable
> + You only need to enable this if you also want to enable
> one or more of the PWM drivers below.
>
> If unsure, say N.
>
I think this hunk should have been in the other first patch.
Arnd
On Thursday 30 June 2011, Bill Gatliff wrote:
> I'm completely against this effort.
>
> The objections to my submission weren't ever because I was changing
> the user-visible API, so I don't think you can claim any advantage to
> mine in that regard.
>
> I will take some blame for not getting my API finished, but I have
> been fighting some serious non-work issues that have consumed nearly
> all of my available time--- including the professional time I would
> otherwise have to invest in getting my code finished.
>
> Is there the possibility that we could cooperate to get my patches
> finished, rather than discarding and reinventing them completely?
I've looked at your patches again, and it seems that you are doing
two distinct changes, both good:
1. You provide a generic framework for pwm drivers that makes it
possible for multiple drivers to coexist and simplifies the way
that these drivers interact with the core OS.
2. You extend and fix a number of aspects in the global PWM API.
Sascha's patch does only part 1, not part 2, but I don't think that
makes his patches any worse. The introduction of the framework
now is very similar to what you had suggested, and you should
probably be mentioned in the changelog, even though the two
implementations were done independently.
A lot of people want to see a framework get merged, and I think it's
great that Sascha has volunteered to do the work to push that
through this time, especially since you have not been able to
finish your work.
What I think would be the best plan forward is to merge Sascha's
patches as soon as we can, then get all currently existing pwm
drivers converted to that and moved to drivers/pwm, and finally
do the interface changes that you have proposed earlier.
I would also hope that you can give constructive feedback to
the submission and point out potential problems that you see
where the code should be changed now in order to make your
interface changes more easy later.
I realize that it's annoying to spend a lot of time on a specific
implementation and then see competing code get merged. Unfortunately,
this happens all the time, and the code we merge is often not
the one that has had the most effort spent on it, but the one that
looks most promising at the time when it gets merged.
Arnd
On Thu, Jun 30, 2011 at 01:42:35PM +0200, Arnd Bergmann wrote:
> On Thursday 30 June 2011, Sascha Hauer wrote:
> > Signed-off-by: Sascha Hauer <[email protected]>
>
> Hi Sascha,
>
> The probing looks good to me now. I would have added extra code to avoid
> ioremapping the same page multiple times, but I see this as a matter of
> different preferences, so I'm fine with that.
>
> You are still missing a description of the driver, both in the changelog
> above and in the Kconfig patch. A small sentence to spell out PWM
> in the changelog and to list the devices that will use this driver
> would be helpful to the causal reader.
Ok, will add.
>
> Provided that you add that:
>
> Reviewed-by: Arnd Bergmann <[email protected]>
>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 93c1052..5694574 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -2,11 +2,15 @@ menuconfig PWM
> > bool "PWM Support"
> > help
> > This enables PWM support through the generic PWM framework.
> > - You only need to enable this, if you also want to enable
> > + You only need to enable this if you also want to enable
> > one or more of the PWM drivers below.
> >
> > If unsure, say N.
> >
>
> I think this hunk should have been in the other first patch.
Indeed, will fix.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Guys:
On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann <[email protected]> wrote:
> A lot of people want to see a framework get merged, and I think it's
> great that Sascha has volunteered to do the work to push that
> through this time, especially since you have not been able to
> finish your work.
Sascha is wasting his time by reinventing the wheel. He's traveling
over exactly the same path I have already covered. In fact, some of
his reviewer comments are almost word-for-word the same as those I
have received and addressed in the past.
My patches were always kept current in this mailing list and others,
and Sascha clearly has the skills necessary to make improvements and
corrections should he have chosen to do so.
> What I think would be the best plan forward is to merge Sascha's
> patches as soon as we can, then get all currently existing pwm
> drivers converted to that and moved to drivers/pwm, and finally
> do the interface changes that you have proposed earlier.
That's a real duplication of work. That makes no sense.
> I would also hope that you can give constructive feedback to
> the submission and point out potential problems that you see
> where the code should be changed now in order to make your
> interface changes more easy later.
My code has already moved past that point. And if I had the time to
evaluate and improve Sascha's patches, I would have the time to finish
my own. Sascha is more than welcome to apply his efforts to the
preexisting PWM API patches already present in the LKML archives.
> I realize that it's annoying to spend a lot of time on a specific
> implementation and then see competing code get merged.
Annoyed isn't the word you are looking for.
My code has been reviewed, tested and actively used as posted in LKML
by several reviewers (including myself) in actual hardware. We are
consciously choosing to discard a known entity and restart the whole
process with new code. That's a waste of everyone's time and risk
exposure.
I don't consider Sascha's code to be "competing" with mine, as
apparently nobody has bothered to consider one against the other.
> Unfortunately,
> this happens all the time, and the code we merge is often not
> the one that has had the most effort spent on it, but the one that
> looks most promising at the time when it gets merged.
Your definition of "promsing" apparently correlates with "new and shiny".
b.g.
--
Bill Gatliff
[email protected]
Bill,
On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
> Guys:
>
> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann <[email protected]> wrote:
>
> > A lot of people want to see a framework get merged, and I think it's
> > great that Sascha has volunteered to do the work to push that
> > through this time, especially since you have not been able to
> > finish your work.
>
> Sascha is wasting his time by reinventing the wheel. He's traveling
> over exactly the same path I have already covered. In fact, some of
> his reviewer comments are almost word-for-word the same as those I
> have received and addressed in the past.
>
> My patches were always kept current in this mailing list and others,
> and Sascha clearly has the skills necessary to make improvements and
> corrections should he have chosen to do so.
I think that you made the fundamental mistake to completly ignore the
existing pwm API and its users. With a competing api we are basically
stuck. We can't convert the existing hardware drivers to the new API
because leds-pwm.c, pwm_bl.c and others still depend on the old API and
boards using it would break. We can't convert the function drivers
either because again this would break boards for which only an old style
pwm driver exists. So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API
atomically so that nothing breaks. Your patches don't do this, so I
don't think at all that what I did is duplication of work.
Given the current rush to move drivers out of arch/ it probably won't
take long until all pwm drivers are moved to drivers/pwm/ and converted
to use the framework, and then you have a good base to put your work onto.
So please don't complain too much: We are currently only doing the work
you didn't want to do.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Sascha:
On Thu, Jun 30, 2011 at 12:02 PM, Sascha Hauer <[email protected]> wrote:
> So please don't complain too much: We are currently only doing the work
> you didn't want to do.
Per request of some the early reviewers, my plan was to migrate
existing users over to the new API after the API itself was committed
to mainstream.
At any rate, this isn't a question of work I didn't "want" to do. On
the one hand, I was simply doing what was asked of me. On the other
hand, I was trying to make progress while simultaneously dealing with
serious family matters that are still unresolved.
At any rate, it's clear that your patches are going in, and mine
aren't--- I have no choice but to accept that. I'm just frustrated by
the duplication of effort.
b.g.
--
Bill Gatliff
[email protected]
On 01/07/11 03:02, Sascha Hauer wrote:
> Bill,
>
> On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
>> Guys:
>>
>> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann<[email protected]> wrote:
>>
>>> A lot of people want to see a framework get merged, and I think it's
>>> great that Sascha has volunteered to do the work to push that
>>> through this time, especially since you have not been able to
>>> finish your work.
>> Sascha is wasting his time by reinventing the wheel. He's traveling
>> over exactly the same path I have already covered. In fact, some of
>> his reviewer comments are almost word-for-word the same as those I
>> have received and addressed in the past.
>>
>> My patches were always kept current in this mailing list and others,
>> and Sascha clearly has the skills necessary to make improvements and
>> corrections should he have chosen to do so.
> I think that you made the fundamental mistake to completly ignore the
> existing pwm API and its users. With a competing api we are basically
> stuck. We can't convert the existing hardware drivers to the new API
> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
> boards using it would break.
I don't think this is really a blocker to Bill's patches. There are
three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c
All of those drivers are trivial and good easily be updated to work with
Bill's patches. Bill already provided a leds-pwm driver.
There is also the interesting case of the Atmel pwm driver, which does
not fit the current pwm api and has its own backlight and leds drivers.
Bill's patches addressed this, Sasha's patches do not. If we merge
Sasha's patches then we are going to be in the same position as Bill's
patches at some point in that we need to change the pwm api (and all its
users) to meet the needs of the Atmel (and any similar hardware) pwm device.
The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the
current api (though it could), but instead provides a sysfs interface to
user-space. Again, this was addressed by Bill's patches.
> We can't convert the function drivers
> either because again this would break boards for which only an old style
> pwm driver exists. So the logical thing to do is to put a step in
> between: Consolidate the existing drivers and *then* change the API
> atomically so that nothing breaks. Your patches don't do this, so I
> don't think at all that what I did is duplication of work.
You have to modify the drivers anyway match the new pwm core. The Atmel
and ep93xx drivers are going to be difficult to merge into the new api,
and seeing as there are only about seven pwm drivers total in the kernel
I think its a significant portion. Any pwm api patchset could easily
convert all of the existing pwm drivers without becoming overly massive.
If we merge Sasha's api, then we can move most of the existing drivers
and maybe add some new ones, but we will still have the unconsolidated
outliers. When (if?) we try to fix those we will probably need to change
the pwm api and therefore all of the drivers to. So its basically a case
of do the effort now (Bill's patches) or do it later. Doing it later
will probably require more effort.
> Given the current rush to move drivers out of arch/ it probably won't
> take long until all pwm drivers are moved to drivers/pwm/ and converted
> to use the framework, and then you have a good base to put your work onto.
> So please don't complain too much: We are currently only doing the work
> you didn't want to do.
>
You can move all of the drivers out of arch now if you want. You just
need to make sure you are only compiling one of them in. The real job in
consolidating means making sure that the api meets the needs of all of
the drivers. The in kernel Atmel pwm driver at least is not going to
convert easily to this api.
Also, please not my change of email address for future emails.
Thanks,
~Ryan
On Thursday, June 30, 2011 4:24 PM, Ryan Mallon wrote:
> On 01/07/11 03:02, Sascha Hauer wrote:
>> I think that you made the fundamental mistake to completly ignore the
>> existing pwm API and its users. With a competing api we are basically
>> stuck. We can't convert the existing hardware drivers to the new API
>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
>> boards using it would break.
>
> I don't think this is really a blocker to Bill's patches. There are
> three (that I can see) pwm users currently:
> drivers/video/backlight/pwm_bl.c
> drivers/leds/leds-pwm.c
> drivers/input/misc/pwm-beeper.c
>
> All of those drivers are trivial and good easily be updated to work with
> Bill's patches. Bill already provided a leds-pwm driver.
I agree with Ryan. The three drivers listed above appear to be the only
pwm "user" drivers. They are all trivial and could be updated easily.
The "core" drivers using the existing API appear to be:
arch/arm/mach-vt8500/pwm.c
arch/arm/plat-mxc/pwm.c
arch/arm/plat-pxa/pwm.c
arch/arm/plat-samsung/pwm.c
arch/blackfin/kernel/pwm.c
arch/mips/jz4740/pwm.c
arch/unicore32/kernel/pwm.c
drivers/misc/ab8500-pwm.c
drivers/mfd/twl6030-pwm.c
> There is also the interesting case of the Atmel pwm driver, which does
> not fit the current pwm api and has its own backlight and leds drivers.
> Bill's patches addressed this, Sasha's patches do not. If we merge
> Sasha's patches then we are going to be in the same position as Bill's
> patches at some point in that we need to change the pwm api (and all its
> users) to meet the needs of the Atmel (and any similar hardware) pwm device.
>
> The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the
> current api (though it could), but instead provides a sysfs interface to
> user-space. Again, this was addressed by Bill's patches.
I modified the ep93xx pwm driver previously to work with Bill's patches.
The use for that driver required user-space control of the pwm. Bill's
patches handle that.
>> We can't convert the function drivers
>> either because again this would break boards for which only an old style
>> pwm driver exists. So the logical thing to do is to put a step in
>> between: Consolidate the existing drivers and *then* change the API
>> atomically so that nothing breaks. Your patches don't do this, so I
>> don't think at all that what I did is duplication of work.
>
> You have to modify the drivers anyway match the new pwm core. The Atmel
> and ep93xx drivers are going to be difficult to merge into the new api,
> and seeing as there are only about seven pwm drivers total in the kernel
> I think its a significant portion. Any pwm api patchset could easily
> convert all of the existing pwm drivers without becoming overly massive.
>
> If we merge Sasha's api, then we can move most of the existing drivers
> and maybe add some new ones, but we will still have the unconsolidated
> outliers. When (if?) we try to fix those we will probably need to change
> the pwm api and therefore all of the drivers to. So its basically a case
> of do the effort now (Bill's patches) or do it later. Doing it later
> will probably require more effort.
>
>> Given the current rush to move drivers out of arch/ it probably won't
>> take long until all pwm drivers are moved to drivers/pwm/ and converted
>> to use the framework, and then you have a good base to put your work onto.
>> So please don't complain too much: We are currently only doing the work
>> you didn't want to do.
>>
> You can move all of the drivers out of arch now if you want. You just
> need to make sure you are only compiling one of them in. The real job in
> consolidating means making sure that the api meets the needs of all of
> the drivers. The in kernel Atmel pwm driver at least is not going to
> convert easily to this api.
I think the only open issue with Bill's patches was where the pwm subsystem
belongs. Some people thought it should be part of gpiolib and others
thought it should be on it's own. The core library Bill presented does
share some commonality with gpiolib but I feel it still represents it's
own subsystem. On-chip pwms might also be gpio capable but this is not
always the case.
With Bill's patches we could run into resource conflicts between gpio and
pwm but that really exists now anyway. The pinmux subsystem that Linus
Walleij (cc'ed) has been proposing should fix that.
Overall I think Bill's patches will get us to the desired end result quicker
and save a lot of needless churn.
Regards,
Hartley-
On Thu, Jun 30, 2011 at 20:33, H Hartley Sweeten wrote:
> On Thursday, June 30, 2011 4:24 PM, Ryan Mallon wrote:
>> On 01/07/11 03:02, Sascha Hauer wrote:
>>> I think that you made the fundamental mistake to completly ignore the
>>> existing pwm API and its users. With a competing api we are basically
>>> stuck. We can't convert the existing hardware drivers to the new API
>>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
>>> boards using it would break.
>>
>> I don't think this is really a blocker to Bill's patches. There are
>> three (that I can see) pwm users currently:
>> drivers/video/backlight/pwm_bl.c
>> drivers/leds/leds-pwm.c
>> drivers/input/misc/pwm-beeper.c
>>
>> All of those drivers are trivial and good easily be updated to work with
>> Bill's patches. Bill already provided a leds-pwm driver.
>
> I agree with Ryan. The three drivers listed above appear to be the only
> pwm "user" drivers. They are all trivial and could be updated easily.
indeed ... the current pwm API is almost toy like in its simplicity.
> The "core" drivers using the existing API appear to be:
> arch/blackfin/kernel/pwm.c
i only wrote this because Bill's subsystem wasnt yet merged. but i
made sure that it was done in such a way that it's trivial to convert
over to Bill's work once it's done.
-mike