* [PATCH 1/2] pwmlib: add pwm support
2011-01-28 12:21 [RFC] add pwmlib support Sascha Hauer
@ 2011-01-28 12:21 ` Sascha Hauer
2011-01-30 20:52 ` Ryan Mallon
2011-01-28 12:21 ` [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver Sascha Hauer
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Sascha Hauer @ 2011-01-28 12:21 UTC (permalink / raw)
To: linux-arm-kernel
The barebone pwm API is present in the kernel for longer.
This patch adds pwmlib support to support dynamically registered
pwms. Porions of this code are inspired by the gpiolib support.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/pwm/Kconfig | 11 +++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwmlib.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pwm.h | 38 ++++++++
6 files changed, 299 insertions(+), 0 deletions(-)
create mode 100644 drivers/pwm/Kconfig
create mode 100644 drivers/pwm/Makefile
create mode 100644 drivers/pwm/pwmlib.c
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 9bfb71f..bb332a6 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -117,4 +117,6 @@ source "drivers/staging/Kconfig"
source "drivers/platform/Kconfig"
source "drivers/clk/Kconfig"
+
+source "drivers/pwm/Kconfig"
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 7eb35f4..8cd30a5 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..04ddbfd
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,11 @@
+menuconfig PWMLIB
+ bool "PWM Support"
+ help
+ This enables PWM support through the generic PWM library.
+ 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 PWMLIB
+endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 0000000..389c049
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PWMLIB) += pwmlib.o
diff --git a/drivers/pwm/pwmlib.c b/drivers/pwm/pwmlib.c
new file mode 100644
index 0000000..0d6c4cc
--- /dev/null
+++ b/drivers/pwm/pwmlib.c
@@ -0,0 +1,246 @@
+/*
+ * Generic pwmlib implementation
+ *
+ * Copyright (C) 2011 Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * 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;
+ int enabled;
+ 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;
+}
+
+/*
+ * The next pwm id to assign. We do not bother to fill gaps which
+ * occur during dynamic registering/deregistering of pwms, but
+ * instead assign a uniq id to each new pwm.
+ */
+static int next_pwm_id;
+
+/**
+ * pwmchip_reserve() - reserve range of pwms to use with platform code only
+ * @npwms: number of pwms to reserve
+ * Context: platform init
+ *
+ * Maybe called only once. It reserves the first pwm_ids for platform use so
+ * that they can refer to pwm_ids during compile time.
+ */
+int __init pwmchip_reserve(int npwms)
+{
+ if (next_pwm_id)
+ return -EBUSY;
+
+ next_pwm_id = npwms;
+
+ return 0;
+}
+
+/**
+ * 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;
+
+ mutex_lock(&pwm_lock);
+
+ if (chip->pwm_id >= 0 && find_pwm(chip->pwm_id)) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
+ if (!pwm) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ pwm->chip = chip;
+
+ if (chip->pwm_id < 0)
+ chip->pwm_id = next_pwm_id++;
+
+ list_add_tail(&pwm->node, &pwm_list);
+out:
+ mutex_unlock(&pwm_lock);
+
+ 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);
+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->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->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->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..e3e2139 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -28,4 +28,42 @@ int pwm_enable(struct pwm_device *pwm);
*/
void pwm_disable(struct pwm_device *pwm);
+#ifdef CONFIG_PWMLIB
+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 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 module *owner;
+ struct pwm_ops *ops;
+};
+
+int __must_check pwmchip_reserve(int npwms);
+int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_remove(struct pwm_chip *chip);
+#endif
+
#endif /* __LINUX_PWM_H */
--
1.7.2.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 1/2] pwmlib: add pwm support
2011-01-28 12:21 ` [PATCH 1/2] pwmlib: add pwm support Sascha Hauer
@ 2011-01-30 20:52 ` Ryan Mallon
2011-01-31 7:49 ` Sascha Hauer
0 siblings, 1 reply; 22+ messages in thread
From: Ryan Mallon @ 2011-01-30 20:52 UTC (permalink / raw)
To: linux-arm-kernel
On 01/29/2011 01:21 AM, Sascha Hauer wrote:
> The barebone pwm API is present in the kernel for longer.
> This patch adds pwmlib support to support dynamically registered
> pwms. Porions of this code are inspired by the gpiolib support.
Hi Sascha,
A couple of comments below. I have added Bill and Arun to the Cc list.
As somebody else pointed out there have been a couple of attempts to
create a generic pwm framework so far. It would be good to try and
consolidate the efforts.
~Ryan
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/pwm/Kconfig | 11 +++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwmlib.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pwm.h | 38 ++++++++
> 6 files changed, 299 insertions(+), 0 deletions(-)
> create mode 100644 drivers/pwm/Kconfig
> create mode 100644 drivers/pwm/Makefile
> create mode 100644 drivers/pwm/pwmlib.c
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 9bfb71f..bb332a6 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -117,4 +117,6 @@ source "drivers/staging/Kconfig"
> source "drivers/platform/Kconfig"
>
> source "drivers/clk/Kconfig"
> +
> +source "drivers/pwm/Kconfig"
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 7eb35f4..8cd30a5 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..04ddbfd
> --- /dev/null
> +++ b/drivers/pwm/Kconfig
> @@ -0,0 +1,11 @@
> +menuconfig PWMLIB
> + bool "PWM Support"
> + help
> + This enables PWM support through the generic PWM library.
> + 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 PWMLIB
> +endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> new file mode 100644
> index 0000000..389c049
> --- /dev/null
> +++ b/drivers/pwm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PWMLIB) += pwmlib.o
> diff --git a/drivers/pwm/pwmlib.c b/drivers/pwm/pwmlib.c
> new file mode 100644
> index 0000000..0d6c4cc
> --- /dev/null
> +++ b/drivers/pwm/pwmlib.c
> @@ -0,0 +1,246 @@
> +/*
> + * Generic pwmlib implementation
> + *
> + * Copyright (C) 2011 Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * 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;
> + int enabled;
> + 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;
> +}
> +
> +/*
> + * The next pwm id to assign. We do not bother to fill gaps which
> + * occur during dynamic registering/deregistering of pwms, but
> + * instead assign a uniq id to each new pwm.
> + */
> +static int next_pwm_id;
> +
> +/**
> + * pwmchip_reserve() - reserve range of pwms to use with platform code only
> + * @npwms: number of pwms to reserve
> + * Context: platform init
> + *
> + * Maybe called only once. It reserves the first pwm_ids for platform use so
> + * that they can refer to pwm_ids during compile time.
> + */
> +int __init pwmchip_reserve(int npwms)
> +{
This concept is a bit ugly.
> + if (next_pwm_id)
> + return -EBUSY;
> +
> + next_pwm_id = npwms;
> +
> + return 0;
> +}
> +
> +/**
> + * 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;
> +
> + mutex_lock(&pwm_lock);
> +
> + if (chip->pwm_id >= 0 && find_pwm(chip->pwm_id)) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
> + if (!pwm) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + pwm->chip = chip;
> +
> + if (chip->pwm_id < 0)
> + chip->pwm_id = next_pwm_id++;
> +
> + list_add_tail(&pwm->node, &pwm_list);
> +out:
> + mutex_unlock(&pwm_lock);
The locking here is a little heavier than it needs to be. Only the list
lookup, incrementing next_pwm_id and the list add need to be protected.
The pwm allocation and assignment of chip do not need to be protected by
the mutex.
The performance difference is negligible, but it does make it more clear
what the lock actually protects.
> + 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);
> +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)
> +{
What purpose does the label serve? It gets assigned but never used.
Possibly it could be useful later in sysfs/debugfs output?
I think requesting pwm's by id is not particularly useful in practice.
Drivers which need to request a pwm would need to know what the id of
the correct pwm was, which may vary between platforms. A better approach
would be to either use device associations (similar to the clock api) or
to just use a string label for lookup.
> + 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->owner)) {
> + pwm = ERR_PTR(-ENODEV);
> + goto out;
> + }
> +
> + if (pwm->chip->ops->request) {
The pwm driver you have provide does not have a request callback. Based
on just the id/label why might a particular pwm driver refuse a request
if the pwm core would grant it?
> + 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->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->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)
duty_ns/period_ns should probably be an unsigned type. Is 32 bits enough
for all pwms?
> +{
> + 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..e3e2139 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -28,4 +28,42 @@ int pwm_enable(struct pwm_device *pwm);
> */
> void pwm_disable(struct pwm_device *pwm);
>
> +#ifdef CONFIG_PWMLIB
> +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 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 module *owner;
> + struct pwm_ops *ops;
> +};
> +
> +int __must_check pwmchip_reserve(int npwms);
> +int pwmchip_add(struct pwm_chip *chip);
> +int pwmchip_remove(struct pwm_chip *chip);
> +#endif
> +
> #endif /* __LINUX_PWM_H */
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/2] pwmlib: add pwm support
2011-01-30 20:52 ` Ryan Mallon
@ 2011-01-31 7:49 ` Sascha Hauer
0 siblings, 0 replies; 22+ messages in thread
From: Sascha Hauer @ 2011-01-31 7:49 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 31, 2011 at 09:52:38AM +1300, Ryan Mallon wrote:
> On 01/29/2011 01:21 AM, Sascha Hauer wrote:
> > The barebone pwm API is present in the kernel for longer.
> > This patch adds pwmlib support to support dynamically registered
> > pwms. Porions of this code are inspired by the gpiolib support.
>
> Hi Sascha,
>
> A couple of comments below. I have added Bill and Arun to the Cc list.
> As somebody else pointed out there have been a couple of attempts to
> create a generic pwm framework so far. It would be good to try and
> consolidate the efforts.
Sorry, I was not aware of these attempts, otherwise I wouldn't have
implemented this myself. So yes, we should consolidate the efforts.
I will happily drop my patches and work on Bills version instead as
his version is more advanced than mine.
> > +
> > +/**
> > + * pwmchip_reserve() - reserve range of pwms to use with platform code only
> > + * @npwms: number of pwms to reserve
> > + * Context: platform init
> > + *
> > + * Maybe called only once. It reserves the first pwm_ids for platform use so
> > + * that they can refer to pwm_ids during compile time.
> > + */
> > +int __init pwmchip_reserve(int npwms)
> > +{
>
> This concept is a bit ugly.
>
> > + if (next_pwm_id)
> > + return -EBUSY;
> > +
> > + next_pwm_id = npwms;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * 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;
> > +
> > + mutex_lock(&pwm_lock);
> > +
> > + if (chip->pwm_id >= 0 && find_pwm(chip->pwm_id)) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
> > + if (!pwm) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + pwm->chip = chip;
> > +
> > + if (chip->pwm_id < 0)
> > + chip->pwm_id = next_pwm_id++;
> > +
> > + list_add_tail(&pwm->node, &pwm_list);
> > +out:
> > + mutex_unlock(&pwm_lock);
>
> The locking here is a little heavier than it needs to be. Only the list
> lookup, incrementing next_pwm_id and the list add need to be protected.
> The pwm allocation and assignment of chip do not need to be protected by
> the mutex.
>
> The performance difference is negligible, but it does make it more clear
> what the lock actually protects.
>
> > + 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);
> > +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)
> > +{
>
> What purpose does the label serve? It gets assigned but never used.
> Possibly it could be useful later in sysfs/debugfs output?
Yes, it was intended for debugfs. Note that this part of the existing
barebone pwm kernel API. I haven't changed it.
>
> I think requesting pwm's by id is not particularly useful in practice.
> Drivers which need to request a pwm would need to know what the id of
> the correct pwm was, which may vary between platforms. A better approach
> would be to either use device associations (similar to the clock api) or
> to just use a string label for lookup.
>
> > + 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->owner)) {
> > + pwm = ERR_PTR(-ENODEV);
> > + goto out;
> > + }
> > +
> > + if (pwm->chip->ops->request) {
>
> The pwm driver you have provide does not have a request callback. Based
> on just the id/label why might a particular pwm driver refuse a request
> if the pwm core would grant it?
We can drop this and add later when we need it.
>
> > + 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->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->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)
>
> duty_ns/period_ns should probably be an unsigned type. Is 32 bits enough
> for all pwms?
Again, this is from the existing API. 32 bits is enough for 4 seconds
which is enough for the use cases I have in mind (backlight for LCDs),
but it is not enough to reflect the capabilities of some hardware which
allows for very high dividers.
--
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 |
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver
2011-01-28 12:21 [RFC] add pwmlib support Sascha Hauer
2011-01-28 12:21 ` [PATCH 1/2] pwmlib: add pwm support Sascha Hauer
@ 2011-01-28 12:21 ` Sascha Hauer
2011-01-28 14:12 ` [RFC] add pwmlib support root
` (2 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: Sascha Hauer @ 2011-01-28 12:21 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/pwm/Kconfig | 5 +
drivers/pwm/Makefile | 2 +
drivers/pwm/mxs-pwm.c | 275 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 282 insertions(+), 0 deletions(-)
create mode 100644 drivers/pwm/mxs-pwm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 04ddbfd..f9a728b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -8,4 +8,9 @@ menuconfig PWMLIB
If unsure, say N.
if PWMLIB
+
+config PWM_MXS
+ tristate "MXS pwm support"
+ depends on ARCH_MXS
+
endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 389c049..1c6441c 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1 +1,3 @@
obj-$(CONFIG_PWMLIB) += pwmlib.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..052cb0c
--- /dev/null
+++ b/drivers/pwm/mxs-pwm.c
@@ -0,0 +1,275 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * 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 <eric.miao@marvell.com>
+ */
+
+#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 <mach/mxs.h>
+#include <mach/mx23.h>
+#include <mach/mx28.h>
+#include <asm/div64.h>
+
+struct mxs_pwm_device {
+ struct device *dev;
+
+ struct clk *clk;
+
+ int enabled;
+ void __iomem *mmio_base;
+
+ unsigned int pwm_id;
+
+ u32 val_active;
+ u32 val_period;
+ int period_us;
+ struct pwm_chip chip;
+};
+
+/* common register space */
+static void __iomem *pwm_base_common;
+#define REG_PWM_CTRL 0x0
+#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)
+{
+ if (enable)
+ __mxs_setl(PWM_ENABLE(pwm->chip.pwm_id), pwm_base_common + REG_PWM_CTRL);
+ else
+ __mxs_clrl(PWM_ENABLE(pwm->chip.pwm_id), pwm_base_common + REG_PWM_CTRL);
+}
+
+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,
+};
+
+static int __devinit mxs_pwm_probe(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+ struct resource *r;
+ int ret = 0;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
+ if (pwm == NULL) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ pwm->clk = clk_get(&pdev->dev, NULL);
+
+ if (IS_ERR(pwm->clk)) {
+ ret = PTR_ERR(pwm->clk);
+ goto err_free;
+ }
+
+ pwm->chip.ops = &mxs_pwm_ops;
+
+ pwm->enabled = 0;
+
+ pwm->chip.pwm_id = pdev->id;
+ pwm->chip.owner = THIS_MODULE;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (r == NULL) {
+ dev_err(&pdev->dev, "no memory resource defined\n");
+ ret = -ENODEV;
+ goto err_free_clk;
+ }
+
+ r = request_mem_region(r->start, resource_size(r), pdev->name);
+ if (r == NULL) {
+ dev_err(&pdev->dev, "failed to request memory resource\n");
+ ret = -EBUSY;
+ goto err_free_clk;
+ }
+
+ pwm->mmio_base = ioremap(r->start, resource_size(r));
+ if (pwm->mmio_base == NULL) {
+ dev_err(&pdev->dev, "failed to ioremap() registers\n");
+ ret = -ENODEV;
+ goto err_free_mem;
+ }
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret)
+ goto err_free_mem;
+
+ platform_set_drvdata(pdev, pwm);
+ return 0;
+
+err_free_mem:
+ release_mem_region(r->start, resource_size(r));
+err_free_clk:
+ clk_put(pwm->clk);
+err_free:
+ kfree(pwm);
+ return ret;
+}
+
+static int __devexit mxs_pwm_remove(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+ struct resource *r;
+ int ret;
+
+ pwm = platform_get_drvdata(pdev);
+
+ ret = pwmchip_remove(&pwm->chip);
+ if (ret)
+ return ret;
+
+ iounmap(pwm->mmio_base);
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(r->start, resource_size(r));
+
+ clk_put(pwm->clk);
+
+ return 0;
+}
+
+static struct platform_driver mxs_pwm_driver = {
+ .driver = {
+ .name = "mxs-pwm",
+ },
+ .probe = mxs_pwm_probe,
+ .remove = __devexit_p(mxs_pwm_remove),
+};
+
+static int __init mxs_pwm_init(void)
+{
+ if (cpu_is_mx28())
+ pwm_base_common = MX28_IO_ADDRESS(MX28_PWM_BASE_ADDR);
+ else
+ pwm_base_common = MX23_IO_ADDRESS(MX23_PWM_BASE_ADDR);
+
+ __mxs_clrl(PWM_SFTRST | PWM_CLKGATE, pwm_base_common + REG_PWM_CTRL);
+
+ 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 <s.hauer@pengutronix.de>");
--
1.7.2.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFC] add pwmlib support
2011-01-28 12:21 [RFC] add pwmlib support Sascha Hauer
2011-01-28 12:21 ` [PATCH 1/2] pwmlib: add pwm support Sascha Hauer
2011-01-28 12:21 ` [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver Sascha Hauer
@ 2011-01-28 14:12 ` root
2011-01-29 0:21 ` Matt Sealey
2011-01-29 20:38 ` Jean Delvare
2011-01-29 20:53 ` Lars-Peter Clausen
4 siblings, 1 reply; 22+ messages in thread
From: root @ 2011-01-28 14:12 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jan 28, 2011 at 01:21:21PM +0100, Sascha Hauer wrote:
> Hi all,
>
> While implementing just another pwm driver I thought it's time to implement
> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
> code is inspired by gpiolib support and tested using the backlight pwm
> driver by Eric Miao.
You know, I really wish driver cores like this where not called lib... it
is hardly as if they can be taken out of the kernel and usefully re-used
in another project.
--
Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-28 14:12 ` [RFC] add pwmlib support root
@ 2011-01-29 0:21 ` Matt Sealey
0 siblings, 0 replies; 22+ messages in thread
From: Matt Sealey @ 2011-01-29 0:21 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jan 28, 2011 at 8:12 AM, root <ben@fluff.org> wrote:
>
> On Fri, Jan 28, 2011 at 01:21:21PM +0100, Sascha Hauer wrote:
>> Hi all,
>>
>> While implementing just another pwm driver I thought it's time to implement
>> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
>> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
>> code is inspired by gpiolib support and tested using the backlight pwm
>> driver by Eric Miao.
>
> You know, I really wish driver cores like this where not called lib... it
> is hardly as if they can be taken out of the kernel and usefully re-used
> in another project.
Well, I've never known a collection of code shared across multiple
client applications with the specific intent of consolidating
functonality under a single framework to be called many other things
than a "library". It doesn't matter what address space it's in, it's a
library of reusable code, so I think the name stands up.
Sascha, this is awesome, BTW. I'm going to see if I can get PWM for
MX5x and MC13892 under this..
--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-28 12:21 [RFC] add pwmlib support Sascha Hauer
` (2 preceding siblings ...)
2011-01-28 14:12 ` [RFC] add pwmlib support root
@ 2011-01-29 20:38 ` Jean Delvare
2011-01-29 21:51 ` Wolfram Sang
2011-01-29 20:53 ` Lars-Peter Clausen
4 siblings, 1 reply; 22+ messages in thread
From: Jean Delvare @ 2011-01-29 20:38 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha,
On Fri, 28 Jan 2011 13:21:21 +0100, Sascha Hauer wrote:
> While implementing just another pwm driver I thought it's time to implement
> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
> code is inspired by gpiolib support and tested using the backlight pwm
> driver by Eric Miao.
>
> Currently the pwm_request, pwm_add, pwm_remove and pwm_free operations are
> protected with a single mutex wherea the pwm_enable, pwm_disable and
> pwm_config operations are unlocked. It is assumed that the owners of the
> pwm handle the serialization of the pwm accesses. This may not be enough, so
> i'd like to discuss the locking (and type of locking) here.
>
> I Cced the people working with PWMs in the kernel in the hope that they can
> give input on what's missing / wrong in this implementation
I do not have the time to review your patches. But I think your
introduction above lacks a fundamental point, which is: what are you
trying to achieve? What would you be able to do with pwmlib, which you
currently aren't?
As far as hwmon driver are concerned, please understand that "PWM" is a
misnomer for "fan speed control". Some hwmon drivers have pwm
attributes which actually correspond to analog voltage outputs. In both
cases (PWM and DC) these are outputs which are dedicated to fan
control, and I've never seen any other use of these, so I doubt it
would make much sense to register them as generic pwm outputs.
So I believe that hwmon drivers are out of scope for pwmlib. Which
makes me wonder what you intend to do with pwmlib (back to my first
question...) as the only use I know of for PWM signals in computers is
for fan control.
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFC] add pwmlib support
2011-01-29 20:38 ` Jean Delvare
@ 2011-01-29 21:51 ` Wolfram Sang
2011-01-30 3:49 ` arden jay
0 siblings, 1 reply; 22+ messages in thread
From: Wolfram Sang @ 2011-01-29 21:51 UTC (permalink / raw)
To: linux-arm-kernel
> > code is inspired by gpiolib support and tested using the backlight pwm
^^^^^^^^^
> So I believe that hwmon drivers are out of scope for pwmlib. Which
> makes me wonder what you intend to do with pwmlib (back to my first
> question...) as the only use I know of for PWM signals in computers is
> for fan control.
It was meant for backlight control (was mentioned very indirectly).
I also pointed him to Bill's implementation, too.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110129/1f089d13/attachment.sig>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-29 21:51 ` Wolfram Sang
@ 2011-01-30 3:49 ` arden jay
2011-01-31 7:58 ` Sascha Hauer
0 siblings, 1 reply; 22+ messages in thread
From: arden jay @ 2011-01-30 3:49 UTC (permalink / raw)
To: linux-arm-kernel
Sorry, I'm confused about this.
If it is for backlight control, why it don't go through led_class?
Do I miss anything?
2011/1/30 Wolfram Sang <w.sang@pengutronix.de>:
>
>> > code is inspired by gpiolib support and tested using the backlight pwm
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ^^^^^^^^^
>> So I believe that hwmon drivers are out of scope for pwmlib. Which
>> makes me wonder what you intend to do with pwmlib (back to my first
>> question...) as the only use I know of for PWM signals in computers is
>> for fan control.
>
> It was meant for backlight control (was mentioned very indirectly).
> I also pointed him to Bill's implementation, too.
>
> Regards,
>
> ? Wolfram
>
> --
> Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? | Wolfram Sang ? ? ? ? ? ? ? ?|
> Industrial Linux Solutions ? ? ? ? ? ? ? ? | http://www.pengutronix.de/ ?|
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAk1Ei+0ACgkQD27XaX1/VRvjkACeOhJonBEZQX8U+ihKtiMjQT/U
> IfAAoMdZiOrAOvuH1Iv6ZKNYoT+VhhTM
> =ROYN
> -----END PGP SIGNATURE-----
>
>
--
cheers,
jay
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-30 3:49 ` arden jay
@ 2011-01-31 7:58 ` Sascha Hauer
2011-02-06 13:22 ` Linus Walleij
0 siblings, 1 reply; 22+ messages in thread
From: Sascha Hauer @ 2011-01-31 7:58 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jan 30, 2011 at 11:49:32AM +0800, arden jay wrote:
> Sorry, I'm confused about this.
> If it is for backlight control, why it don't go through led_class?
PWMs are not limited to backlight. As Jean pointed out they can be used
for fans (or more generally motors) aswell. Have a look at
drivers/leds/leds-pwm.c, it is a user of this pwm API.
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 |
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-31 7:58 ` Sascha Hauer
@ 2011-02-06 13:22 ` Linus Walleij
0 siblings, 0 replies; 22+ messages in thread
From: Linus Walleij @ 2011-02-06 13:22 UTC (permalink / raw)
To: linux-arm-kernel
2011/1/31 Sascha Hauer <s.hauer@pengutronix.de>:
> On Sun, Jan 30, 2011 at 11:49:32AM +0800, arden jay wrote:
>> Sorry, I'm confused about this.
>> If it is for backlight control, why it don't go through led_class?
>
> PWMs are not limited to backlight. As Jean pointed out they can be used
> for fans (or more generally motors) aswell. Have a look at
> drivers/leds/leds-pwm.c, it is a user of this pwm API.
Low-frequency PWM is used for vibrators of the type found in
silent mode cellphones, to mention another use case.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-28 12:21 [RFC] add pwmlib support Sascha Hauer
` (3 preceding siblings ...)
2011-01-29 20:38 ` Jean Delvare
@ 2011-01-29 20:53 ` Lars-Peter Clausen
2011-01-31 3:35 ` Arun MURTHY
4 siblings, 1 reply; 22+ messages in thread
From: Lars-Peter Clausen @ 2011-01-29 20:53 UTC (permalink / raw)
To: linux-arm-kernel
On 01/28/2011 01:21 PM, Sascha Hauer wrote:
> Hi all,
>
> While implementing just another pwm driver I thought it's time to implement
> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
> code is inspired by gpiolib support and tested using the backlight pwm
> driver by Eric Miao.
>
> Currently the pwm_request, pwm_add, pwm_remove and pwm_free operations are
> protected with a single mutex wherea the pwm_enable, pwm_disable and
> pwm_config operations are unlocked. It is assumed that the owners of the
> pwm handle the serialization of the pwm accesses. This may not be enough, so
> i'd like to discuss the locking (and type of locking) here.
>
> I Cced the people working with PWMs in the kernel in the hope that they can
> give input on what's missing / wrong in this implementation
>
> Sascha
>
Hi
There have been two other proposals for a generic PWM api during the last year.
You might want to take a look at them.
https://lkml.org/lkml/2010/2/9/275
https://lkml.org/lkml/2010/9/28/107
I've added Bill Gatliff and Arun Murthy to Cc.
- Lars
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFC] add pwmlib support
2011-01-29 20:53 ` Lars-Peter Clausen
@ 2011-01-31 3:35 ` Arun MURTHY
2011-01-31 7:54 ` Sascha Hauer
0 siblings, 1 reply; 22+ messages in thread
From: Arun MURTHY @ 2011-01-31 3:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha,
> > I Cced the people working with PWMs in the kernel in the hope that
> they can
> > give input on what's missing / wrong in this implementation
> >
> > Sascha
> >
>
> Hi
>
> There have been two other proposals for a generic PWM api during the
> last year.
> You might want to take a look at them.
>
> https://lkml.org/lkml/2010/2/9/275
> https://lkml.org/lkml/2010/9/28/107
>
> I've added Bill Gatliff and Arun Murthy to Cc.
>
As said by Lars, we already have developed the pwm core driver and
progressing towards aligning the existing pwm drivers to the pwm core
driver.
These set of patches are expected to be out in LKML by this week.
Thanks and Regards,
Arun R Murthy
------------
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-31 3:35 ` Arun MURTHY
@ 2011-01-31 7:54 ` Sascha Hauer
2011-01-31 12:48 ` Lars-Peter Clausen
0 siblings, 1 reply; 22+ messages in thread
From: Sascha Hauer @ 2011-01-31 7:54 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 31, 2011 at 04:35:33AM +0100, Arun MURTHY wrote:
> Hi Sascha,
>
> > > I Cced the people working with PWMs in the kernel in the hope that
> > they can
> > > give input on what's missing / wrong in this implementation
> > >
> > > Sascha
> > >
> >
> > Hi
> >
> > There have been two other proposals for a generic PWM api during the
> > last year.
> > You might want to take a look at them.
> >
> > https://lkml.org/lkml/2010/2/9/275
> > https://lkml.org/lkml/2010/9/28/107
> >
> > I've added Bill Gatliff and Arun Murthy to Cc.
> >
>
> As said by Lars, we already have developed the pwm core driver and
> progressing towards aligning the existing pwm drivers to the pwm core
> driver.
> These set of patches are expected to be out in LKML by this week.
Nice, problem solved without me having to work on it ;). Please consider
Ccing linux-arm-kernel as many of your potential users are following this
list.
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 |
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-31 7:54 ` Sascha Hauer
@ 2011-01-31 12:48 ` Lars-Peter Clausen
2011-01-31 13:00 ` Sascha Hauer
0 siblings, 1 reply; 22+ messages in thread
From: Lars-Peter Clausen @ 2011-01-31 12:48 UTC (permalink / raw)
To: linux-arm-kernel
On 01/31/2011 08:54 AM, Sascha Hauer wrote:
> On Mon, Jan 31, 2011 at 04:35:33AM +0100, Arun MURTHY wrote:
>> Hi Sascha,
>>
>>>> I Cced the people working with PWMs in the kernel in the hope that
>>> they can
>>>> give input on what's missing / wrong in this implementation
>>>>
>>>> Sascha
>>>>
>>>
>>> Hi
>>>
>>> There have been two other proposals for a generic PWM api during the
>>> last year.
>>> You might want to take a look at them.
>>>
>>> https://lkml.org/lkml/2010/2/9/275
>>> https://lkml.org/lkml/2010/9/28/107
>>>
>>> I've added Bill Gatliff and Arun Murthy to Cc.
>>>
>>
>> As said by Lars, we already have developed the pwm core driver and
>> progressing towards aligning the existing pwm drivers to the pwm core
>> driver.
>> These set of patches are expected to be out in LKML by this week.
>
> Nice, problem solved without me having to work on it ;).
I wouldn't call it problem solved yet.
I liked your approach better so far, but lets see how the next iteration of Aruns
patches turn out.
- Lars
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC] add pwmlib support
2011-01-31 12:48 ` Lars-Peter Clausen
@ 2011-01-31 13:00 ` Sascha Hauer
0 siblings, 0 replies; 22+ messages in thread
From: Sascha Hauer @ 2011-01-31 13:00 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 31, 2011 at 01:48:43PM +0100, Lars-Peter Clausen wrote:
> On 01/31/2011 08:54 AM, Sascha Hauer wrote:
> > On Mon, Jan 31, 2011 at 04:35:33AM +0100, Arun MURTHY wrote:
> >> Hi Sascha,
> >>
> >>>> I Cced the people working with PWMs in the kernel in the hope that
> >>> they can
> >>>> give input on what's missing / wrong in this implementation
> >>>>
> >>>> Sascha
> >>>>
> >>>
> >>> Hi
> >>>
> >>> There have been two other proposals for a generic PWM api during the
> >>> last year.
> >>> You might want to take a look at them.
> >>>
> >>> https://lkml.org/lkml/2010/2/9/275
> >>> https://lkml.org/lkml/2010/9/28/107
> >>>
> >>> I've added Bill Gatliff and Arun Murthy to Cc.
> >>>
> >>
> >> As said by Lars, we already have developed the pwm core driver and
> >> progressing towards aligning the existing pwm drivers to the pwm core
> >> driver.
> >> These set of patches are expected to be out in LKML by this week.
> >
> > Nice, problem solved without me having to work on it ;).
>
> I wouldn't call it problem solved yet.
> I liked your approach better so far, but lets see how the next iteration of Aruns
> patches turn out.
What I don't like about Bills patches is the way PWMs are configured.
Bill, since you are on Cc, maybe you can comment on this:
> +enum {
> + PWM_CONFIG_DUTY_TICKS = BIT(0),
> + PWM_CONFIG_PERIOD_TICKS = BIT(1),
> + PWM_CONFIG_POLARITY = BIT(2),
> + PWM_CONFIG_START = BIT(3),
> + PWM_CONFIG_STOP = BIT(4),
> +
> + PWM_CONFIG_HANDLER = BIT(5),
> +
> + PWM_CONFIG_DUTY_NS = BIT(6),
> + PWM_CONFIG_DUTY_PERCENT = BIT(7),
> + PWM_CONFIG_PERIOD_NS = BIT(8),
> +};
> +
>
> ...
>
> +
> +struct pwm_channel_config {
> + int config_mask;
> + unsigned long duty_ticks;
> + unsigned long period_ticks;
> + int polarity;
> +
> + pwm_handler_t handler;
> +
> + unsigned long duty_ns;
> + unsigned long period_ns;
> + int duty_percent;
> +};
>
> ...
>
> +int pwm_config(struct pwm_channel *pwm,
> + struct pwm_channel_config *c);
I think we should have a single internal interpretation of how a pwm is
configured, either ticks or ns (or whatever else), but not ticks, ns and
percent. Instead we could provide helpers to convert between them.
Also, I don't like ioctl like function calls. Instead of dispatching
PWM_CONFIG_* we should use discrete functions for each functionality.
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 |
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver
2011-06-28 10:02 [RFC] implement a generic PWM framework - once again Sascha Hauer
@ 2011-06-28 10:02 ` Sascha Hauer
2011-06-28 10:28 ` Lothar Waßmann
2011-06-28 15:22 ` Arnd Bergmann
0 siblings, 2 replies; 22+ messages in thread
From: Sascha Hauer @ 2011-06-28 10:02 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/pwm/mxs-pwm.c | 275 +++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 275 insertions(+), 0 deletions(-)
create mode 100644 drivers/pwm/mxs-pwm.c
diff --git a/drivers/pwm/mxs-pwm.c b/drivers/pwm/mxs-pwm.c
new file mode 100644
index 0000000..052cb0c
--- /dev/null
+++ b/drivers/pwm/mxs-pwm.c
@@ -0,0 +1,275 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * 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 <eric.miao@marvell.com>
+ */
+
+#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 <mach/mxs.h>
+#include <mach/mx23.h>
+#include <mach/mx28.h>
+#include <asm/div64.h>
+
+struct mxs_pwm_device {
+ struct device *dev;
+
+ struct clk *clk;
+
+ int enabled;
+ void __iomem *mmio_base;
+
+ unsigned int pwm_id;
+
+ u32 val_active;
+ u32 val_period;
+ int period_us;
+ struct pwm_chip chip;
+};
+
+/* common register space */
+static void __iomem *pwm_base_common;
+#define REG_PWM_CTRL 0x0
+#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)
+{
+ if (enable)
+ __mxs_setl(PWM_ENABLE(pwm->chip.pwm_id), pwm_base_common + REG_PWM_CTRL);
+ else
+ __mxs_clrl(PWM_ENABLE(pwm->chip.pwm_id), pwm_base_common + REG_PWM_CTRL);
+}
+
+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,
+};
+
+static int __devinit mxs_pwm_probe(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+ struct resource *r;
+ int ret = 0;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
+ if (pwm == NULL) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ pwm->clk = clk_get(&pdev->dev, NULL);
+
+ if (IS_ERR(pwm->clk)) {
+ ret = PTR_ERR(pwm->clk);
+ goto err_free;
+ }
+
+ pwm->chip.ops = &mxs_pwm_ops;
+
+ pwm->enabled = 0;
+
+ pwm->chip.pwm_id = pdev->id;
+ pwm->chip.owner = THIS_MODULE;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (r == NULL) {
+ dev_err(&pdev->dev, "no memory resource defined\n");
+ ret = -ENODEV;
+ goto err_free_clk;
+ }
+
+ r = request_mem_region(r->start, resource_size(r), pdev->name);
+ if (r == NULL) {
+ dev_err(&pdev->dev, "failed to request memory resource\n");
+ ret = -EBUSY;
+ goto err_free_clk;
+ }
+
+ pwm->mmio_base = ioremap(r->start, resource_size(r));
+ if (pwm->mmio_base == NULL) {
+ dev_err(&pdev->dev, "failed to ioremap() registers\n");
+ ret = -ENODEV;
+ goto err_free_mem;
+ }
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret)
+ goto err_free_mem;
+
+ platform_set_drvdata(pdev, pwm);
+ return 0;
+
+err_free_mem:
+ release_mem_region(r->start, resource_size(r));
+err_free_clk:
+ clk_put(pwm->clk);
+err_free:
+ kfree(pwm);
+ return ret;
+}
+
+static int __devexit mxs_pwm_remove(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+ struct resource *r;
+ int ret;
+
+ pwm = platform_get_drvdata(pdev);
+
+ ret = pwmchip_remove(&pwm->chip);
+ if (ret)
+ return ret;
+
+ iounmap(pwm->mmio_base);
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(r->start, resource_size(r));
+
+ clk_put(pwm->clk);
+
+ return 0;
+}
+
+static struct platform_driver mxs_pwm_driver = {
+ .driver = {
+ .name = "mxs-pwm",
+ },
+ .probe = mxs_pwm_probe,
+ .remove = __devexit_p(mxs_pwm_remove),
+};
+
+static int __init mxs_pwm_init(void)
+{
+ if (cpu_is_mx28())
+ pwm_base_common = MX28_IO_ADDRESS(MX28_PWM_BASE_ADDR);
+ else
+ pwm_base_common = MX23_IO_ADDRESS(MX23_PWM_BASE_ADDR);
+
+ __mxs_clrl(PWM_SFTRST | PWM_CLKGATE, pwm_base_common + REG_PWM_CTRL);
+
+ 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 <s.hauer@pengutronix.de>");
--
1.7.5.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver
2011-06-28 10:02 ` [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver Sascha Hauer
@ 2011-06-28 10:28 ` Lothar Waßmann
2011-06-28 15:22 ` Arnd Bergmann
1 sibling, 0 replies; 22+ messages in thread
From: Lothar Waßmann @ 2011-06-28 10:28 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
just some nitpicks...
Sascha Hauer writes:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/pwm/mxs-pwm.c | 275 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 275 insertions(+), 0 deletions(-)
> create mode 100644 drivers/pwm/mxs-pwm.c
>
> diff --git a/drivers/pwm/mxs-pwm.c b/drivers/pwm/mxs-pwm.c
> new file mode 100644
> index 0000000..052cb0c
> --- /dev/null
> +++ b/drivers/pwm/mxs-pwm.c
> @@ -0,0 +1,275 @@
> +/*
> + * Copyright (C) 2011 Pengutronix
> + * Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * 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 <eric.miao@marvell.com>
> + */
> +
> +#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 <mach/mxs.h>
> +#include <mach/mx23.h>
> +#include <mach/mx28.h>
> +#include <asm/div64.h>
> +
> +struct mxs_pwm_device {
> + struct device *dev;
> +
> + struct clk *clk;
> +
> + int enabled;
> + void __iomem *mmio_base;
> +
> + unsigned int pwm_id;
> +
> + u32 val_active;
> + u32 val_period;
> + int period_us;
> + struct pwm_chip chip;
> +};
> +
> +/* common register space */
> +static void __iomem *pwm_base_common;
> +#define REG_PWM_CTRL 0x0
> +#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 int __devinit mxs_pwm_probe(struct platform_device *pdev)
> +{
> + struct mxs_pwm_device *pwm;
> + struct resource *r;
> + int ret = 0;
> +
> + pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
> + if (pwm == NULL) {
> + dev_err(&pdev->dev, "failed to allocate memory\n");
> + return -ENOMEM;
> + }
> +
> + pwm->clk = clk_get(&pdev->dev, NULL);
> +
> + if (IS_ERR(pwm->clk)) {
Why an empty line here, but not in other similar places?
> + ret = PTR_ERR(pwm->clk);
> + goto err_free;
> + }
> +
> + pwm->chip.ops = &mxs_pwm_ops;
> +
> + pwm->enabled = 0;
is already 0 due to kzalloc().
> +
> + pwm->chip.pwm_id = pdev->id;
> + pwm->chip.owner = THIS_MODULE;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (r == NULL) {
> + dev_err(&pdev->dev, "no memory resource defined\n");
> + ret = -ENODEV;
> + goto err_free_clk;
> + }
> +
> + r = request_mem_region(r->start, resource_size(r), pdev->name);
> + if (r == NULL) {
> + dev_err(&pdev->dev, "failed to request memory resource\n");
> + ret = -EBUSY;
> + goto err_free_clk;
> + }
> +
> + pwm->mmio_base = ioremap(r->start, resource_size(r));
> + if (pwm->mmio_base == NULL) {
> + dev_err(&pdev->dev, "failed to ioremap() registers\n");
> + ret = -ENODEV;
-ENOMEM?.
> + if (ret)
> + goto err_free_mem;
> +
> + platform_set_drvdata(pdev, pwm);
> + return 0;
> +
> +err_free_mem:
> + release_mem_region(r->start, resource_size(r));
> +err_free_clk:
> + clk_put(pwm->clk);
> +err_free:
> + kfree(pwm);
> + return ret;
> +}
> +
> +static int __devexit mxs_pwm_remove(struct platform_device *pdev)
> +{
> + struct mxs_pwm_device *pwm;
> + struct resource *r;
> + int ret;
> +
> + pwm = platform_get_drvdata(pdev);
> +
> + ret = pwmchip_remove(&pwm->chip);
> + if (ret)
> + return ret;
> +
> + iounmap(pwm->mmio_base);
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(r->start, resource_size(r));
> +
> + clk_put(pwm->clk);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mxs_pwm_driver = {
> + .driver = {
> + .name = "mxs-pwm",
> + },
> + .probe = mxs_pwm_probe,
> + .remove = __devexit_p(mxs_pwm_remove),
> +};
> +
> +static int __init mxs_pwm_init(void)
> +{
> + if (cpu_is_mx28())
> + pwm_base_common = MX28_IO_ADDRESS(MX28_PWM_BASE_ADDR);
> + else
> + pwm_base_common = MX23_IO_ADDRESS(MX23_PWM_BASE_ADDR);
> +
> + __mxs_clrl(PWM_SFTRST | PWM_CLKGATE, pwm_base_common + REG_PWM_CTRL);
> +
Shouldn't CLKGATE be set again in the remove() function?
Lothar Wa?mann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver
2011-06-28 10:02 ` [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver Sascha Hauer
2011-06-28 10:28 ` Lothar Waßmann
@ 2011-06-28 15:22 ` Arnd Bergmann
1 sibling, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2011-06-28 15:22 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha,
On Tuesday 28 June 2011, Sascha Hauer wrote:
> +
> +/* common register space */
> +static void __iomem *pwm_base_common;
> +#define REG_PWM_CTRL 0x0
> +#define PWM_SFTRST (1 << 31)
> +#define PWM_CLKGATE (1 << 30)
> +#define PWM_ENABLE(p) (1 << (p))
> +
The driver looks pretty good overall, but the global pwm_base_common register
is rather ugly. I think this should really be passed down through resources
from the platform device in one way or another.
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver
2011-06-29 9:03 [PATCH v2] implement a generic PWM framework Sascha Hauer
@ 2011-06-29 9:03 ` Sascha Hauer
2011-06-29 11:37 ` Arnd Bergmann
0 siblings, 1 reply; 22+ messages in thread
From: Sascha Hauer @ 2011-06-29 9:03 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/pwm/Kconfig | 6 +-
drivers/pwm/Makefile | 1 +
drivers/pwm/mxs-pwm.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 327 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..6788f48
--- /dev/null
+++ b/drivers/pwm/mxs-pwm.c
@@ -0,0 +1,321 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * 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 <eric.miao@marvell.com>
+ */
+
+#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>
+
+struct mxs_pwm_device {
+ struct device *dev;
+
+ struct clk *clk;
+
+ int enabled;
+ void __iomem *mmio_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 __iomem *pwm_base_common;
+static int num_instances;
+static DEFINE_MUTEX(pwm_common_mutex);
+
+/*
+ * each pwm has a separate register space but all share a common
+ * enable register.
+ */
+static int mxs_pwm_common_get(struct platform_device *pdev)
+{
+ struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ resource_size_t start = r->start & ~0xfff;
+ int ret = 0;
+
+ mutex_lock(&pwm_common_mutex);
+
+ if (!num_instances) {
+ r = request_mem_region(start, 0x10, "mxs-pwm-common");
+ if (!r)
+ goto err_request;
+
+ pwm_base_common = ioremap(start, 0x10);
+ if (!pwm_base_common) {
+ ret = -ENOMEM;
+ goto err_ioremap;
+ }
+ }
+
+ writel(PWM_SFTRST | PWM_CLKGATE, pwm_base_common + REG_PWM_CTRL_CLEAR);
+
+ num_instances++;
+
+ mutex_unlock(&pwm_common_mutex);
+
+ return 0;
+
+err_ioremap:
+ release_mem_region(start, 0x10);
+err_request:
+ mutex_unlock(&pwm_common_mutex);
+ return ret;
+}
+
+static void mxs_pwm_common_put(struct platform_device *pdev)
+{
+ struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ resource_size_t start = r->start & ~0xfff;
+
+ mutex_lock(&pwm_common_mutex);
+
+ num_instances--;
+
+ if (!num_instances) {
+ iounmap(pwm_base_common);
+ release_mem_region(start, 0x10);
+
+ writel(PWM_CLKGATE, pwm_base_common + REG_PWM_CTRL_SET);
+ }
+
+ mutex_unlock(&pwm_common_mutex);
+}
+
+static void __pwm_enable(struct mxs_pwm_device *pwm, int enable)
+{
+ void __iomem *reg;
+
+ if (enable)
+ reg = pwm_base_common + REG_PWM_CTRL_SET;
+ else
+ reg = pwm_base_common + 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 __devinit mxs_pwm_probe(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+ struct resource *r;
+ int ret = 0;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
+ if (pwm == NULL) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ pwm->chip.ops = &mxs_pwm_ops;
+ pwm->chip.pwm_id = pdev->id;
+
+ 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->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;
+ }
+
+ pwm->clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pwm->clk)) {
+ ret = PTR_ERR(pwm->clk);
+ goto err_out;
+ }
+
+ ret = mxs_pwm_common_get(pdev);
+ if (ret)
+ goto err_free_clk;
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret)
+ goto err_remove_common;
+
+ platform_set_drvdata(pdev, pwm);
+ return 0;
+
+err_remove_common:
+ mxs_pwm_common_put(pdev);
+err_free_clk:
+ clk_put(pwm->clk);
+err_out:
+ return ret;
+}
+
+static int __devexit mxs_pwm_remove(struct platform_device *pdev)
+{
+ struct mxs_pwm_device *pwm;
+ int ret;
+
+ pwm = platform_get_drvdata(pdev);
+
+ ret = pwmchip_remove(&pwm->chip);
+ if (ret)
+ return ret;
+
+ clk_put(pwm->clk);
+
+ mxs_pwm_common_put(pdev);
+
+ return 0;
+}
+
+static struct platform_driver mxs_pwm_driver = {
+ .driver = {
+ .name = "mxs-pwm",
+ },
+ .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 <s.hauer@pengutronix.de>");
--
1.7.5.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver
2011-06-29 9:03 ` [PATCH 2/2] pwm: Add a i.MX23/28 pwm driver Sascha Hauer
@ 2011-06-29 11:37 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2011-06-29 11:37 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 29 June 2011, Sascha Hauer wrote:
> +/*
> + * each pwm has a separate register space but all share a common
> + * enable register.
> + */
> +static int mxs_pwm_common_get(struct platform_device *pdev)
> +{
> + struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + resource_size_t start = r->start & ~0xfff;
> + int ret = 0;
> +
> + if (!num_instances) {
> + r = request_mem_region(start, 0x10, "mxs-pwm-common");
> + if (!r)
> + goto err_request;
Yes, this looks better than the original approach, but it still feels
a bit awkward:
You are requesting a region outside of the platform device resource.
This will cause problems with the device tree conversion, where the
idea is to list all registers that are used for each device.
It also becomes a problem if a system has multiple PWM regions
that are a page long each -- you only map one of them currently,
so the first one would win.
When you model the pwm device in the device tree, the most logical
representation IMHO would be to have a nested device, like:
/amba/pwm_core at 0fa0000/pwm at 0
/pwm at 1
/pwm at 2
The pwm_core then has the MMIO registers and provides an interface
for the individual pwms to access the registers, like an MFD
device. The resources for the slave devices are not MMIO ranges
but simply offsets. The pwm_enable function will then do something
like
static void __pwm_enable(struct mxs_pwm_device *pwm, int enable)
{
struct device *parent = &pwm->chip.dev.parent->parent;
void __iomem *pwm_base_common = dev_get_drvdata(parent);
if (enable)
reg = pwm_base_common + REG_PWM_CTRL_SET;
else
reg = pwm_base_common + REG_PWM_CTRL_CLEAR;
writel(PWM_ENABLE(pwm->chip.pwm_id), reg);
}
The pwm driver obviously has to register for both device types,
the parent and the child, and do different things in the two
cases, e.g.
static int __devinit mxs_pwm_probe(struct platform_device *pdev)
{
switch (pdev->id_entry->driver_data) {
case MXS_PWM_MASTER:
return mxs_pwm_map_master_resources(pdev);
case MXS_PWM_SLAVE:
return mxs_pwm_register_pwmchip(pdev, to_platform_device(pdev->dev.parent));
}
return -ENODEV;
}
I'm normally not that picky, but I think we should have the best
possible solution for this in the mx23 driver, because it will
likely be used as an example for other drivers that have the
same problem.
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread