[PATCH 0/6] Generic PWM API implementation

[PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

This series implements a Generic PWM Device API, including reference
implementations for the Atmel PWMC device, an LED device, and an LED
trigger.  It is based on linux-2.6.27.

This API is motivated by the author's need to support pluggable
devices; a secondary objective is to consolidate the existing PWM
implementations behind an agreeable, consistent, redundancy-reducing
interface.

The code included in this patch series draws heavily from the existing
PWM infrastructure and driver for the AT91SAM9263 PWMC.  The author is
grateful to Russell King, Eric Miao, David Brownell and many others
for providing such tall "shoulders" to stand upon.  Modifications to
their code as described in this changeset should not be interpreted as
attempts to address shortcomings, but rather to extend functionality
in ways that were not originally required.

The implementation of the Generic PWM Device API is structurally
similar to the generic GPIO API, except that the PWM code uses
platform bus_id strings instead of integers to identify target
deviices.  A configuration structure is also provided, both to
facilitate atomic hardware state changes and so that the API can be
extended in a source-code-compatible way to accomodate devices with
features not anticipated by the current code.

Pulse width modulated signals are used in an astounding number and
range of applications, and there is no "one true way" of either
realizing them or employing them to accomplish real work.  The review
process revealed many use cases to the author, but countless others
undoubtedly remain.  The current API attempts to provide a useful
feature set for the most basic users, packaged in such a way as to
allow the API to be extended in a backwards-compatible way as new
needs are identified.

The proposed code has been run-tested on a Cogent CSB737
(AT91SAM9263), mated to a custom circuit that drives multiple DC
motors and sensors.


Feedback is welcome!



b.g.
--
Bill Gatliff
<bgat@billgatliff.com>

---

Bill Gatliff (6):
  [PWM] Generic PWM API implementation
  [PWM] Changes to existing include/linux/pwm.h to adapt to generic PWM
    API
  [PWM] Documentation
  [PWM] Driver for Atmel PWMC peripheral
  [PWM] Install new Atmel PWMC driver in Kconfig, expunge old one
  [PWM] New LED driver and trigger that use PWM API

 Documentation/pwm.txt      |  258 +++++++++++++++++
 arch/arm/Kconfig           |    2 +
 drivers/Makefile           |    2 +
 drivers/leds/Kconfig       |   24 ++-
 drivers/leds/Makefile      |    2 +
 drivers/leds/leds-pwm.c    |  167 +++++++++++
 drivers/leds/ledtrig-dim.c |   95 ++++++
 drivers/misc/Kconfig       |    9 -
 drivers/misc/Makefile      |    1 -
 drivers/misc/atmel_pwm.c   |  409 --------------------------
 drivers/pwm/Kconfig        |   24 ++
 drivers/pwm/Makefile       |    6 +
 drivers/pwm/atmel-pwm.c    |  633 ++++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm.c          |  681 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm-led.h    |   34 +++
 include/linux/pwm.h        |  172 ++++++++++--
 16 files changed, 2074 insertions(+), 445 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/leds/leds-pwm.c
 create mode 100644 drivers/leds/ledtrig-dim.c
 delete mode 100644 drivers/misc/atmel_pwm.c
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/atmel-pwm.c
 create mode 100644 drivers/pwm/pwm.c
 create mode 100644 include/linux/pwm-led.h

[PATCH 1/6] [PWM] Generic PWM API implementation

[Bill Gatliff]

An API that consolidates PWM-capable devices behind a common interface.

This code defines a pwm_device structure, which identifies a PWM-capable
platform device having one or more output channels of type pwm_channel.  The
pwm_register() function maintains a list of pwm_devices; the pwm_request()
and pwm_free() functions allocate and un-allocate PWM channels on user
request.

The pwm_config_nosleep() and pwm_config() functions are the main entry
points into the PWM device driver itself.  The former may be called
from contexts that are not allowed to sleep, i.e. interrupt handlers, and
will return an error if the requested configuration change cannot be
performed by the hardware without sleeping; the latter may be called
everywhere else.

PWM drivers might sleep because some hardware makes it difficult to update
the period, duty cycle or polarity of an output while the peripheral is
running.  The Atmel PWMC peripheral is one example: the duty cycle and
period control registers share a single buffer register, but some
updates require changes to both registers and/or registers that aren't
buffered.  When that is required, you must stop the peripheral at
the end of a PWM period in order to make the state change.  The Atmel PWMC
driver implements the stop by sleeping until the end of the period.

An alternative implementation is to simply brute-force stop the PWM,
update the peripheral control registers, and then restart the hardware.
That approach is fine if you don't care about glitches in the PWM output.
The Atmel PWMC driver tries to be more elegant, at the expense of some
complexity and occasional conflict with sleep-prohibited contexts.

If you don't need to sleep or you don't care about glitch-free
operation, then put everything into pwm_config_nosleep()
and redirect pwm_config() there.

There are various helper functions that build the pwm_channel_config
structure for simple updates, like just modifying the duty cycle
percentage.  These should probably be inlined, but aren't.

PWM channels can be "synchronized".  As defined by this API, synchronized
PWM channels all start, stop and restart together when any channel in the
group is started, stopped, or restarted.  The Atmel PWMC driver emulates
this behavior; other hardware might support it more explicitly.  There is
currently no implementation to synchronize channels across PWM devices,
but the API will allow for that if someone wants to write the code.
Hint: there will be a measurable delay between starting and stopping the
various channels in a cross-device synchronization scenario, because
you'll have to start and stop each channel individually.

Finally, the pwm_set_handler() function registers a callback function that
the API will invoke in a worker thread at the end of the PWM period.

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 drivers/pwm/pwm.c |  681 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 681 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm.c

diff --git a/drivers/pwm/pwm.c b/drivers/pwm/pwm.c
new file mode 100644
index 0000000..5009de9
--- /dev/null
+++ b/drivers/pwm/pwm.c
@@ -0,0 +1,681 @@
+/*
+ * drivers/pwm/pwm.c
+ *
+ * Copyright (C) 2008 Bill Gatliff
+ *
+ * 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 of the License, 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; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/spinlock.h>
+#include <linux/fs.h>
+#include <linux/completion.h>
+#include <linux/workqueue.h>
+#include <linux/pwm.h>
+
+
+static int __pwm_create_sysfs(struct pwm_device *pwm);
+
+static LIST_HEAD(pwm_device_list);
+static DEFINE_MUTEX(device_list_mutex);
+static struct class pwm_class;
+static struct workqueue_struct *pwm_handler_workqueue;
+
+
+int pwm_register(struct pwm_device *pwm)
+{
+	struct pwm_channel *p;
+	int wchan;
+	int ret = 0;
+
+	spin_lock_init(&pwm->list_lock);
+
+	p = kcalloc(pwm->nchan, sizeof(struct pwm_channel), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	for (wchan = 0; wchan < pwm->nchan; wchan++) {
+		spin_lock_init(&p[wchan].lock);
+		init_completion(&p[wchan].complete);
+		p[wchan].chan = wchan;
+		p[wchan].pwm = pwm;
+	}
+
+	pwm->channels = p;
+
+	mutex_lock(&device_list_mutex);
+
+	list_add_tail(&pwm->list, &pwm_device_list);
+	ret = __pwm_create_sysfs(pwm);
+	if (ret) {
+		mutex_unlock(&device_list_mutex);
+		goto err_create_sysfs;
+	}
+
+	mutex_unlock(&device_list_mutex);
+
+	pr_info("%s: %d channels\n", pwm->bus_id, pwm->nchan);
+	return 0;
+
+err_create_sysfs:
+	kfree(p);
+
+	return ret;
+}
+EXPORT_SYMBOL(pwm_register);
+
+
+static int __match_device(struct device *dev, void *data)
+{
+	return dev_get_drvdata(dev) == data;
+}
+
+
+int pwm_unregister(struct pwm_device *pwm)
+{
+	int wchan;
+	struct device *dev;
+
+	mutex_lock(&device_list_mutex);
+
+	for (wchan = 0; wchan < pwm->nchan; wchan++) {
+		if (pwm->channels[wchan].flags & FLAG_REQUESTED) {
+			mutex_unlock(&device_list_mutex);
+			return -EBUSY;
+		}
+	}
+
+	for (wchan = 0; wchan < pwm->nchan; wchan++) {
+		dev = class_find_device(&pwm_class, NULL,
+					&pwm->channels[wchan],
+					__match_device);
+		if (dev) {
+			put_device(dev);
+			device_unregister(dev);
+		}
+	}
+
+	kfree(pwm->channels);
+	list_del(&pwm->list);
+	mutex_unlock(&device_list_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL(pwm_unregister);
+
+
+static struct pwm_device *
+__pwm_find_device(const char *bus_id)
+{
+	struct pwm_device *p;
+
+	list_for_each_entry(p, &pwm_device_list, list)
+	{
+		if (!strcmp(bus_id, p->bus_id))
+			return p;
+	}
+	return NULL;
+}
+
+
+static int
+__pwm_request_channel(struct pwm_channel *p,
+		      const char *requester)
+{
+	int ret;
+
+	if (test_and_set_bit(FLAG_REQUESTED, &p->flags))
+		return -EBUSY;
+
+	if (p->pwm->request) {
+		ret = p->pwm->request(p);
+		if (ret) {
+			clear_bit(FLAG_REQUESTED, &p->flags);
+			return ret;
+		}
+	}
+
+	p->requester = requester;
+	return 0;
+}
+
+
+struct pwm_channel *
+pwm_request(const char *bus_id,
+	    int chan,
+	    const char *requester)
+{
+	struct pwm_device *p;
+	int ret;
+
+	mutex_lock(&device_list_mutex);
+
+	p = __pwm_find_device(bus_id);
+	if (!p || chan >= p->nchan)
+		goto err_no_device;
+
+	if (!try_module_get(p->owner))
+		goto err_module_get_failed;
+
+	ret = __pwm_request_channel(&p->channels[chan], requester);
+	if (ret)
+		goto err_request_failed;
+
+	mutex_unlock(&device_list_mutex);
+
+	pr_debug("%s: %s:%d returns %p\n", __func__,
+		 bus_id, chan, &p->channels[chan]);
+
+	return &p->channels[chan];
+
+err_request_failed:
+	module_put(p->owner);
+err_module_get_failed:
+err_no_device:
+
+	mutex_unlock(&device_list_mutex);
+
+	pr_debug("%s: %s:%d returns NULL\n",
+		 __func__, bus_id, chan);
+
+	return NULL;
+}
+EXPORT_SYMBOL(pwm_request);
+
+
+void pwm_free(struct pwm_channel *p)
+{
+	mutex_lock(&device_list_mutex);
+
+	if (!test_and_clear_bit(FLAG_REQUESTED, &p->flags))
+		goto done;
+
+	pwm_stop(p);
+	pwm_unsynchronize(p, NULL);
+	pwm_set_handler(p, NULL, NULL);
+
+	if (p->pwm->free)
+		p->pwm->free(p);
+	module_put(p->pwm->owner);
+
+	pr_debug("%s: %s:%d free\n",
+		 __func__, p->pwm->bus_id, p->chan);
+
+done:
+	mutex_unlock(&device_list_mutex);
+}
+EXPORT_SYMBOL(pwm_free);
+
+
+unsigned long pwm_ns_to_ticks(struct pwm_channel *p,
+			      unsigned long nsecs)
+{
+	unsigned long long ticks;
+
+	ticks = nsecs;
+	ticks *= p->tick_hz;
+	do_div(ticks, 1000000000);
+	return ticks;
+}
+EXPORT_SYMBOL(pwm_ns_to_ticks);
+
+
+unsigned long pwm_ticks_to_ns(struct pwm_channel *p,
+			      unsigned long ticks)
+{
+	unsigned long long ns;
+
+	if (!p->tick_hz)
+		return 0;
+
+	ns = ticks;
+	ns *= 1000000000UL;
+	do_div(ns, p->tick_hz);
+	return ns;
+}
+EXPORT_SYMBOL(pwm_ticks_to_ns);
+
+
+static void
+pwm_config_ns_to_ticks(struct pwm_channel *p,
+		       struct pwm_channel_config *c)
+{
+	if (c->config_mask & PWM_CONFIG_PERIOD_NS) {
+		c->period_ticks = pwm_ns_to_ticks(p, c->period_ns);
+		c->config_mask &= ~PWM_CONFIG_PERIOD_NS;
+		c->config_mask |= PWM_CONFIG_PERIOD_TICKS;
+	}
+
+	if (c->config_mask & PWM_CONFIG_DUTY_NS) {
+		c->duty_ticks = pwm_ns_to_ticks(p, c->duty_ns);
+		c->config_mask &= ~PWM_CONFIG_DUTY_NS;
+		c->config_mask |= PWM_CONFIG_DUTY_TICKS;
+	}
+}
+
+
+static void
+pwm_config_percent_to_ticks(struct pwm_channel *p,
+			    struct pwm_channel_config *c)
+{
+	if (c->config_mask & PWM_CONFIG_DUTY_PERCENT) {
+		if (c->config_mask & PWM_CONFIG_PERIOD_TICKS)
+			c->duty_ticks = c->period_ticks;
+		else
+			c->duty_ticks = p->period_ticks;
+
+		c->duty_ticks *= c->duty_percent;
+		c->duty_ticks /= 100;
+		c->config_mask &= ~PWM_CONFIG_DUTY_PERCENT;
+		c->config_mask |= PWM_CONFIG_DUTY_TICKS;
+	}
+}
+
+
+int pwm_config_nosleep(struct pwm_channel *p,
+		       struct pwm_channel_config *c)
+{
+	if (!p->pwm->config_nosleep)
+		return -EINVAL;
+
+	pwm_config_ns_to_ticks(p, c);
+	pwm_config_percent_to_ticks(p, c);
+
+	return p->pwm->config_nosleep(p, c);
+}
+EXPORT_SYMBOL(pwm_config_nosleep);
+
+
+int pwm_config(struct pwm_channel *p,
+	       struct pwm_channel_config *c)
+{
+	int ret = 0;
+
+	if (unlikely(!p->pwm->config)) {
+		pr_debug("%s: %s:%d has no config handler (-EINVAL)\n",
+			 __func__, p->pwm->bus_id, p->chan);
+		return -EINVAL;
+	}
+
+	pwm_config_ns_to_ticks(p, c);
+	pwm_config_percent_to_ticks(p, c);
+
+	switch (c->config_mask & (PWM_CONFIG_PERIOD_TICKS
+				  | PWM_CONFIG_DUTY_TICKS)) {
+	case PWM_CONFIG_PERIOD_TICKS:
+		if (p->duty_ticks > c->period_ticks) {
+			ret = -EINVAL;
+			goto err;
+		}
+		break;
+	case PWM_CONFIG_DUTY_TICKS:
+		if (p->period_ticks < c->duty_ticks) {
+			ret = -EINVAL;
+			goto err;
+		}
+		break;
+	case PWM_CONFIG_DUTY_TICKS | PWM_CONFIG_PERIOD_TICKS:
+		if (c->duty_ticks > c->period_ticks) {
+			ret = -EINVAL;
+			goto err;
+		}
+		break;
+	default:
+		break;
+	}
+
+err:
+	pr_debug("%s: config_mask %d period_ticks %lu duty_ticks %lu"
+		 " polarity %d duty_ns %lu period_ns %lu duty_percent %d\n",
+		 __func__, c->config_mask, c->period_ticks, c->duty_ticks,
+		 c->polarity, c->duty_ns, c->period_ns, c->duty_percent);
+
+	if (ret)
+		return ret;
+	return p->pwm->config(p, c);
+}
+EXPORT_SYMBOL(pwm_config);
+
+
+int pwm_set_period_ns(struct pwm_channel *p,
+		      unsigned long period_ns)
+{
+	struct pwm_channel_config c = {
+		.config_mask = PWM_CONFIG_PERIOD_TICKS,
+		.period_ticks = pwm_ns_to_ticks(p, period_ns),
+	};
+
+	return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_set_period_ns);
+
+
+unsigned long pwm_get_period_ns(struct pwm_channel *p)
+{
+	return pwm_ticks_to_ns(p, p->period_ticks);
+}
+EXPORT_SYMBOL(pwm_get_period_ns);
+
+
+int pwm_set_duty_ns(struct pwm_channel *p,
+		    unsigned long duty_ns)
+{
+	struct pwm_channel_config c = {
+		.config_mask = PWM_CONFIG_DUTY_TICKS,
+		.duty_ticks = pwm_ns_to_ticks(p, duty_ns),
+	};
+	return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_set_duty_ns);
+
+
+unsigned long pwm_get_duty_ns(struct pwm_channel *p)
+{
+	return pwm_ticks_to_ns(p, p->duty_ticks);
+}
+EXPORT_SYMBOL(pwm_get_duty_ns);
+
+
+int pwm_set_duty_percent(struct pwm_channel *p,
+			 int percent)
+{
+	struct pwm_channel_config c = {
+		.config_mask = PWM_CONFIG_DUTY_PERCENT,
+		.duty_percent = percent,
+	};
+	return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_set_duty_percent);
+
+
+int pwm_set_polarity(struct pwm_channel *p,
+		     int active_high)
+{
+	struct pwm_channel_config c = {
+		.config_mask = PWM_CONFIG_POLARITY,
+		.polarity = active_high,
+	};
+	return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_set_polarity);
+
+
+int pwm_start(struct pwm_channel *p)
+{
+	struct pwm_channel_config c = {
+		.config_mask = PWM_CONFIG_START,
+	};
+	return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_start);
+
+
+int pwm_stop(struct pwm_channel *p)
+{
+	struct pwm_channel_config c = {
+		.config_mask = PWM_CONFIG_STOP,
+	};
+	return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_stop);
+
+
+int pwm_synchronize(struct pwm_channel *p,
+		    struct pwm_channel *to_p)
+{
+	if (p->pwm != to_p->pwm) {
+		/* TODO: support cross-device synchronization */
+		return -EINVAL;
+	}
+
+	if (!p->pwm->synchronize)
+		return -EINVAL;
+
+	return p->pwm->synchronize(p, to_p);
+}
+EXPORT_SYMBOL(pwm_synchronize);
+
+
+int pwm_unsynchronize(struct pwm_channel *p,
+		      struct pwm_channel *from_p)
+{
+	if (from_p && (p->pwm != from_p->pwm)) {
+		/* TODO: support cross-device synchronization */
+		return -EINVAL;
+	}
+
+	if (!p->pwm->unsynchronize)
+		return -EINVAL;
+
+	return p->pwm->unsynchronize(p, from_p);
+}
+EXPORT_SYMBOL(pwm_unsynchronize);
+
+
+static void pwm_handler(struct work_struct *w)
+{
+	struct pwm_channel *p = container_of(w, struct pwm_channel,
+					     handler_work);
+	if (p->handler && p->handler(p, p->handler_data))
+		pwm_stop(p);
+}
+
+
+static void __pwm_callback(struct pwm_channel *p)
+{
+	queue_work(pwm_handler_workqueue, &p->handler_work);
+	pr_debug("%s:%d handler %p scheduled with data %p\n",
+		 p->pwm->bus_id, p->chan, p->handler, p->handler_data);
+}
+
+
+int pwm_set_handler(struct pwm_channel *p,
+		    pwm_handler_t handler,
+		    void *data)
+{
+	if (p->pwm->set_callback) {
+		p->handler_data = data;
+		p->handler = handler;
+		INIT_WORK(&p->handler_work, pwm_handler);
+		return p->pwm->set_callback(p, handler ? __pwm_callback : NULL);
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pwm_set_handler);
+
+
+static ssize_t pwm_run_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf,
+			     size_t len)
+{
+	struct pwm_channel *p = dev_get_drvdata(dev);
+	if (sysfs_streq(buf, "1"))
+		pwm_start(p);
+	else if (sysfs_streq(buf, "0"))
+		pwm_stop(p);
+	return len;
+}
+static DEVICE_ATTR(run, 0200, NULL, pwm_run_store);
+
+
+static ssize_t pwm_duty_ns_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct pwm_channel *p = dev_get_drvdata(dev);
+	return sprintf(buf, "%lu\n", pwm_get_duty_ns(p));
+}
+
+static ssize_t pwm_duty_ns_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t len)
+{
+	unsigned long duty_ns;
+	struct pwm_channel *p = dev_get_drvdata(dev);
+
+	if (1 == sscanf(buf, "%lu", &duty_ns))
+		pwm_set_duty_ns(p, duty_ns);
+	return len;
+}
+static DEVICE_ATTR(duty_ns, 0644, pwm_duty_ns_show, pwm_duty_ns_store);
+
+
+static ssize_t pwm_period_ns_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct pwm_channel *p = dev_get_drvdata(dev);
+	return sprintf(buf, "%lu\n", pwm_get_period_ns(p));
+}
+
+
+static ssize_t pwm_period_ns_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf,
+				   size_t len)
+{
+	unsigned long period_ns;
+	struct pwm_channel *p = dev_get_drvdata(dev);
+
+	if (1 == sscanf(buf, "%lu", &period_ns))
+		pwm_set_period_ns(p, period_ns);
+	return len;
+}
+static DEVICE_ATTR(period_ns, 0644, pwm_period_ns_show, pwm_period_ns_store);
+
+
+static ssize_t pwm_polarity_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pwm_channel *p = dev_get_drvdata(dev);
+	return sprintf(buf, "%d\n", p->active_low ? 0 : 1);
+}
+
+
+static ssize_t pwm_polarity_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t len)
+{
+	int polarity;
+	struct pwm_channel *p = dev_get_drvdata(dev);
+
+	if (1 == sscanf(buf, "%d", &polarity))
+		pwm_set_polarity(p, polarity);
+	return len;
+}
+static DEVICE_ATTR(polarity, 0644, pwm_polarity_show, pwm_polarity_store);
+
+
+static ssize_t pwm_request_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct pwm_channel *p = dev_get_drvdata(dev);
+	mutex_lock(&device_list_mutex);
+	__pwm_request_channel(p, "sysfs");
+	mutex_unlock(&device_list_mutex);
+
+	return sprintf(buf, "%s\n", p->requester);
+}
+
+
+static ssize_t pwm_request_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t len)
+{
+	struct pwm_channel *p = dev_get_drvdata(dev);
+	pwm_free(p);
+	return len;
+}
+static DEVICE_ATTR(request, 0644, pwm_request_show, pwm_request_store);
+
+
+static const struct attribute *pwm_attrs[] =
+{
+	&dev_attr_run.attr,
+	&dev_attr_polarity.attr,
+	&dev_attr_duty_ns.attr,
+	&dev_attr_period_ns.attr,
+	&dev_attr_request.attr,
+	NULL,
+};
+
+
+static const struct attribute_group pwm_device_attr_group = {
+	.attrs = (struct attribute **)pwm_attrs,
+};
+
+
+static int __pwm_create_sysfs(struct pwm_device *pwm)
+{
+	int ret = 0;
+	struct device *dev;
+	int wchan;
+
+	for (wchan = 0; wchan < pwm->nchan; wchan++) {
+		dev = device_create(&pwm_class, pwm->dev, MKDEV(0, 0),
+				    pwm->channels + wchan,
+				    "%s:%d", pwm->bus_id, wchan);
+		if (!dev)
+			goto err_dev_create;
+		ret = sysfs_create_group(&dev->kobj, &pwm_device_attr_group);
+		if (ret)
+			goto err_dev_group_create;
+	}
+
+	return ret;
+
+err_dev_group_create:
+err_dev_create:
+	/* TODO: undo all the successful device_create calls */
+	return -ENODEV;
+}
+
+
+static struct class_attribute pwm_class_attrs[] = {
+	__ATTR_NULL,
+};
+
+static struct class pwm_class = {
+	.name = "pwm",
+	.owner = THIS_MODULE,
+
+	.class_attrs = pwm_class_attrs,
+};
+
+
+static int __init pwm_init(void)
+{
+	int ret;
+
+	/* TODO: how to deal with devices that register very early? */
+
+	ret = class_register(&pwm_class);
+	if (ret < 0)
+		return ret;
+
+	pwm_handler_workqueue = create_workqueue("pwmd");
+
+	return 0;
+}
+postcore_initcall(pwm_init);
-- 
1.5.6.5

[PATCH 2/6] [PWM] Changes to existing include/linux/pwm.h to adapt to generic PWM API

[Bill Gatliff]

Modifications to the existing include/linux/pwm.h file for the generic PWM
API.

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 include/linux/pwm.h |  172 ++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 151 insertions(+), 21 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 3945f80..1a4308e 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -1,31 +1,161 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
-struct pwm_device;
-
 /*
- * pwm_request - request a PWM device
+ * include/linux/pwm.h
+ *
+ * Copyright (C) 2008 Bill Gatliff
+ *
+ * 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 of the License, 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; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
-struct pwm_device *pwm_request(int pwm_id, const char *label);
 
-/*
- * pwm_free - free a PWM device
- */
-void pwm_free(struct pwm_device *pwm);
+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 - change a PWM device configuration
- */
-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
+	PWM_CONFIG_HANDLER = BIT(5),
 
-/*
- * pwm_enable - start a PWM output toggling
- */
-int pwm_enable(struct pwm_device *pwm);
+	PWM_CONFIG_DUTY_NS = BIT(6),
+	PWM_CONFIG_DUTY_PERCENT = BIT(7),
+	PWM_CONFIG_PERIOD_NS = BIT(8),
+};
+
+struct pwm_channel;
+struct work_struct;
+
+typedef int (*pwm_handler_t)(struct pwm_channel *p, void *data);
+typedef void (*pwm_callback_t)(struct pwm_channel *p);
+
+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;
+};
+
+struct pwm_device {
+	struct list_head list;
+	spinlock_t list_lock;
+	struct device *dev;
+	struct module *owner;
+	struct pwm_channel *channels;
+
+	const char *bus_id;
+	int nchan;
+
+	int	(*request)	(struct pwm_channel *p);
+	void	(*free)		(struct pwm_channel *p);
+	int	(*config)	(struct pwm_channel *p,
+				 struct pwm_channel_config *c);
+	int	(*config_nosleep)(struct pwm_channel *p,
+				  struct pwm_channel_config *c);
+	int	(*synchronize)	(struct pwm_channel *p,
+				 struct pwm_channel *to_p);
+	int	(*unsynchronize)(struct pwm_channel *p,
+				 struct pwm_channel *from_p);
+	int	(*set_callback)	(struct pwm_channel *p,
+				 pwm_callback_t callback);
+};
+
+int pwm_register(struct pwm_device *pwm);
+int pwm_unregister(struct pwm_device *pwm);
+
+enum {
+	FLAG_REQUESTED = 0,
+	FLAG_STOP = 1,
+};
+
+struct pwm_channel {
+	struct list_head list;
+	struct pwm_device *pwm;
+	const char *requester;
+	int chan;
+	unsigned long flags;
+	unsigned long tick_hz;
+
+	spinlock_t lock;
+	struct completion complete;
+
+	pwm_callback_t callback;
+
+	struct work_struct handler_work;
+	pwm_handler_t handler;
+	void *handler_data;
+
+	int active_low;
+	unsigned long period_ticks;
+	unsigned long duty_ticks;
+};
+
+struct pwm_channel *
+pwm_request(const char *bus_id, int chan,
+	    const char *requester);
+
+void pwm_free(struct pwm_channel *pwm);
+
+int pwm_config_nosleep(struct pwm_channel *pwm,
+		       struct pwm_channel_config *c);
+
+int pwm_config(struct pwm_channel *pwm,
+	       struct pwm_channel_config *c);
+
+unsigned long pwm_ns_to_ticks(struct pwm_channel *pwm,
+			      unsigned long nsecs);
+
+unsigned long pwm_ticks_to_ns(struct pwm_channel *pwm,
+			      unsigned long ticks);
+
+int pwm_set_period_ns(struct pwm_channel *pwm,
+		      unsigned long period_ns);
+
+unsigned long int pwm_get_period_ns(struct pwm_channel *pwm);
+
+int pwm_set_duty_ns(struct pwm_channel *pwm,
+		    unsigned long duty_ns);
+
+int pwm_set_duty_percent(struct pwm_channel *pwm,
+			 int percent);
+
+unsigned long pwm_get_duty_ns(struct pwm_channel *pwm);
+
+int pwm_set_polarity(struct pwm_channel *pwm,
+		     int active_high);
+
+int pwm_start(struct pwm_channel *pwm);
+
+int pwm_stop(struct pwm_channel *pwm);
+
+int pwm_set_handler(struct pwm_channel *pwm,
+		    pwm_handler_t handler,
+		    void *data);
+
+int pwm_synchronize(struct pwm_channel *p,
+		    struct pwm_channel *to_p);
+
+
+int pwm_unsynchronize(struct pwm_channel *p,
+		      struct pwm_channel *from_p);
 
-/*
- * pwm_disable - stop a PWM output toggling
- */
-void pwm_disable(struct pwm_device *pwm);
 
-#endif /* __ASM_ARCH_PWM_H */
+#endif /* __LINUX_PWM_H */
-- 
1.5.6.5

[PATCH 3/6] [PWM] Documentation

[Bill Gatliff]

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 Documentation/pwm.txt |  258 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 258 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/pwm.txt

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
new file mode 100644
index 0000000..b8932dd
--- /dev/null
+++ b/Documentation/pwm.txt
@@ -0,0 +1,258 @@
+                       Generic PWM Device API
+
+                          October 8, 2008
+                           Bill Gatliff
+                       <bgat@billgatliff.com>
+
+
+
+The code in drivers/pwm and include/linux/pwm.h implements an API for
+applications involving pulse-width-modulation signals.  This document
+describes how the API implementation facilitates both PWM-generating
+devices, and users of those devices.
+
+
+
+Motivation
+
+The primary goals for implementing the "generic PWM API" are to
+consolidate the various PWM implementations within a consistent and
+redundancy-reducing framework, and to facilitate the use of
+hotpluggable PWM devices.
+
+Previous PWM-related implementations within the Linux kernel achieved
+their consistency via cut-and-paste, but did not need to (and didn't)
+facilitate more than one PWM-generating device within the system---
+hotplug or otherwise.  The Generic PWM Device API might be most
+appropriately viewed as an update to those implementations, rather
+than a complete rewrite.
+
+
+
+Challenges
+
+One of the difficulties in implementing a generic PWM framework is the
+fact that pulse-width-modulation applications involve real-world
+signals, which often must be carefully managed to prevent destruction
+of hardware that is linked to those signals.  A DC motor that
+experiences a brief interruption in the PWM signal controlling it
+might destructively overheat; it could change speed, losing
+synchronization with a sensor; it could even suddenly change direction
+or torque, breaking the mechanical device connected to it.
+
+(A generic PWM device framework is not directly responsible for
+preventing the above scenarios: that responsibility lies with the
+hardware designer and the application and driver authors.  But it must
+to the greatest extent possible make it easy to avoid such problems).
+
+A generic PWM device framework must accomodate the substantial
+differences between available PWM-generating hardware devices, without
+becoming sub-optimal for any of them.
+
+Finally, a generic PWM device framework must be relatively
+lightweight, computationally speaking.  Some PWM users demand
+high-speed outputs, plus the ability to regulate those outputs
+quickly.  A device framework must be able to "keep up" with such
+hardware, while still leaving time to do real work.
+
+The Generic PWM Device API is an attempt to meet all of the above
+requirements.  At its initial publication, the API was already in use
+managing small DC motors through a custom-designed, optically-isolated
+H-bridge driver.
+
+
+
+Functional Overview
+
+The Generic PWM Device API framework is implemented in
+include/linux/pwm.h and drivers/pwm/pwm.c.  The functions therein use
+information from pwm_device, pwm_channel and pwm_channel_config
+structures to invoke services in PWM peripheral device drivers.
+Consult drivers/pwm/atmel-pwm.c for an example driver.
+
+There are two classes of adopters of the PWM framework:
+
+  "Users" -- those wishing to employ the API merely to produce PWM
+  signals; once they have identified the appropriate physical output
+  on the platform in question, they don't care about the details of
+  the underlying hardware
+
+  "Driver authors" -- those wishing to bind devices that can generate
+  PWM signals to the Generic PWM Device API, so that the services of
+  those devices become available to users; assuming the hardware can
+  support the needs of a user, driver authors don't care about the
+  details of the user's application
+
+Generally speaking, users will first invoke pwm_request() to obtain a
+handle to a PWM device.  They will then pass that handle to functions
+like pwm_duty_ns() and pwm_period_ns() to set the duty cycle and
+period of the PWM signal, respectively.  They will also invoke
+pwm_start() and pwm_stop() to turn the signal on and off.
+
+The framework also provides a sysfs interface to PWM devices, which is
+adequate for basic needs and testing.
+
+Driver authors fill out a pwm_device structure, which describes the
+capabilities of the PWM hardware being driven--- including the number
+of distinct output "channels" the peripheral offers.  They then invoke
+pwm_register() (usually from within their device's probe() handler) to
+make the PWM API aware of their device.  The framework will call back
+to the methods described in the pwm_device structure to configure and
+use the hardware.
+
+Note that PWM signals can be produced by a variety of peripherals,
+beyond the true "PWM hardware" offered by many system-on-chip devices.
+Other possibilities include timer/counters with compare-match
+capabilities, carefully-programmed synchronous serial ports
+(e.g. SPI), and GPIO pins driven by kernel interval timers.  With a
+proper pwm_device structure, these devices and pseudo-devices can all
+be accomodated by the Generic PWM Device API framework.
+
+
+
+Using the API to Generate PWM Signals -- Basic Functions for Users
+
+
+pwm_request() -- Returns a pwm_channel pointer, which is subsequently
+passed to the other user-related PWM functions.  Once requested, a PWM
+channel is marked as in-use and subsequent requests prior to
+pwm_free() will fail.
+
+The names used to refer to PWM devices are defined by driver authors.
+Typically they are platform device bus identifiers, and this
+convention is encouraged for consistency.
+
+
+pwm_free() -- Marks a PWM channel as no longer in use.  The PWM device
+is stopped before it is released by the API.
+
+
+pwm_period_ns() -- Specifies the PWM signal's period, in nanoseconds.
+
+
+pwm_duty_ns() -- Specifies the PWM signal's active duration, in nanoseconds.
+
+
+pwm_duty_percent() -- Specifies the PWM signal's active duration, as a
+percentage of the current period of the signal.  NOTE: this value is
+not recalculated if the period of the signal is subsequently changed.
+
+
+pwm_start(), pwm_stop() -- Turns the PWM signal on and off.  Except
+where stated otherwise by a driver author, signals are stopped at the
+end of the current period, at which time the output is set to its
+inactive state.
+
+
+pwm_polarity() -- Defines whether the PWM signal output's active
+region is "1" or "0".  A 10% duty-cycle, polarity=1 signal will
+conventionally be at 5V (or 3.3V, or 1000V, or whatever the platform
+hardware does) for 10% of the period.  The same configuration of a
+polarity=0 signal will be at 5V (or 3.3V, or ...) for 90% of the
+period.
+
+
+
+Using the API to Generate PWM Signals -- Advanced Functions
+
+
+pwm_config() -- Passes a pwm_channel_config structure to the
+associated device driver.  This function is invoked by pwm_start(),
+pwm_duty_ns(), etc. and is one of two main entry points to the PWM
+driver for the hardware being used.  The configuration change is
+guaranteed atomic if multiple configuration changes are specified.
+This function might sleep, depending on what the device driver has to
+do to satisfy the request.  All PWM device drivers must support this
+entry point.
+
+
+pwm_config_nosleep() -- Passes a pwm_channel_config structure to the
+associated device driver.  If the driver must sleep in order to
+implement the requested configuration change, -EWOULDBLOCK is
+returned.  Users may call this function from interrupt handlers, for
+example.  This is the other main entry point into the PWM hardware
+driver, but not all device drivers support this entry point.
+
+
+pwm_synchronize(), pwm_unsynchronize() -- "Synchronizes" two or more
+PWM channels, if the underlying hardware permits.  (If it doesn't, the
+framework facilitates emulating this capability but it is not yet
+implemented).  Synchronized channels will start and stop
+simultaneously when any single channel in the group is started or
+stopped.  Use pwm_unsynchronize(..., NULL) to completely detach a
+channel from any other synchronized channels.
+
+
+pwm_set_handler() -- Defines an end-of-period callback.  The indicated
+function will be invoked in a worker thread at the end of each PWM
+period, and can subsequently invoke pwm_config(), etc.  Must be used
+with extreme care for high-speed PWM outputs.  Set the handler
+function to NULL to un-set the handler.
+
+
+
+Implementing a PWM Device API Driver -- Functions for Driver Authors
+
+
+Fill out the appropriate fields in a pwm_device structure, and submit
+to pwm_register():
+
+
+bus_id -- the plaintext name of the device.  Users will bind to a
+channel on the device using this name plus the channel number.  For
+example, the Atmel PWMC's bus_id is "atmel_pwmc", the same as used by
+the platform device driver (recommended).  The first device registered
+thereby receives bus_id "atmel_pwmc.0", which is what you put in
+pwm_device.bus_id.  Channels are then named "atmel_pwmc.0:[0-3]".
+(Hint: just use pdev->dev.bus_id in your probe() method).
+
+
+nchan -- the number of distinct output channels provided by the device.
+
+
+request -- (optional) Invoked each time a user requests a channel.
+Use to turn on clocks, clean up register states, etc.  The framework
+takes care of device locking/unlocking; you will see only successful
+requests.
+
+
+free -- (optional) Callback for each time a user relinquishes a
+channel.  The framework will have already stopped, unsynchronized and
+un-handled the channel.  Use to turn off clocks, etc. as necessary.
+
+
+synchronize, unsynchronize -- (optional) Callbacks to
+synchronize/unsynchronize channels.  Some devices provide this
+capability in hardware; for others, it can be emulated (see
+atmel_pwmc.c's sync_mask for an example).
+
+
+set_callback -- (optional) Invoked when a user requests a handler.  If
+the hardware supports an end-of-period interrupt, invoke the function
+indicated during your interrupt handler.  The callback function itself
+is always internal to the API, and does not map directly to the user's
+callback function.
+
+
+config -- Invoked to change the device configuration, always from a
+sleep-capable context.  All the changes indicated must be performed
+atomically, ideally synchronized to an end-of-period event (so that
+you avoid short or long output pulses).  You may sleep, etc. as
+necessary within this function.
+
+
+config_nosleep -- (optional) Invoked to change device configuration
+from within a context that is not allowed to sleep.  If you cannot
+perform the requested configuration changes without sleeping, return
+-EWOULDBLOCK.
+
+
+
+Acknowledgements
+
+
+The author expresses his gratitude to the countless developers who
+have reviewed and submitted feedback on the various versions of the
+Generic PWM Device API code, and those who have submitted drivers and
+applications that use the framework.  You know who you are.  ;)
+
-- 
1.5.6.5

[PATCH 4/6] [PWM] Driver for Atmel PWMC peripheral

[Bill Gatliff]

This driver binds the Atmel PWMC platform device to the Generic
PWM Device API.

The config_nosleep() function changes only the peripheral states that do
not require a sleep in order to avoid a spurious output; other state
changes are in config(), which calls stop_sync() to wait for the end of
the current PWM period.  Note that the PWMC hardware has limited
double-buffering: within some constraints, you can change either the
period OR the duty cycle with the hardware active, and the hardware will
commit the register update at the end of the current PWM period.

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 drivers/pwm/atmel-pwm.c |  633 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 633 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/atmel-pwm.c

diff --git a/drivers/pwm/atmel-pwm.c b/drivers/pwm/atmel-pwm.c
new file mode 100644
index 0000000..c7ec676
--- /dev/null
+++ b/drivers/pwm/atmel-pwm.c
@@ -0,0 +1,633 @@
+/*
+ * drivers/pwm/atmel-pwm.c
+ *
+ * Copyright (C) 2008 Bill Gatliff
+ * Copyright (C) 2007 David Brownell
+ *
+ * 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 of the License, 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; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+
+enum {
+	/* registers common to the PWMC peripheral */
+	PWMC_MR = 0,
+	PWMC_ENA = 4,
+	PWMC_DIS = 8,
+	PWMC_SR = 0xc,
+	PWMC_IER = 0x10,
+	PWMC_IDR = 0x14,
+	PWMC_IMR = 0x18,
+	PWMC_ISR = 0x1c,
+
+	/* registers per each PWMC channel */
+	PWMC_CMR = 0,
+	PWMC_CDTY = 4,
+	PWMC_CPRD = 8,
+	PWMC_CCNT = 0xc,
+	PWMC_CUPD = 0x10,
+
+	/* how to find each channel */
+	PWMC_CHAN_BASE = 0x200,
+	PWMC_CHAN_STRIDE = 0x20,
+
+	/* CMR bits of interest */
+	PWMC_CMR_CPD = 10,
+	PWMC_CMR_CPOL = 9,
+	PWMC_CMR_CALG = 8,
+	PWMC_CMR_CPRE_MASK = 0xf,
+};
+
+struct atmel_pwm {
+	struct pwm_device pwm;
+	spinlock_t lock;
+	void __iomem *iobase;
+	struct clk *clk;
+	u32 *sync_mask;
+	int irq;
+	u32 ccnt_mask;
+};
+
+
+static inline void
+pwmc_writel(const struct atmel_pwm *p,
+	    unsigned offset, u32 val)
+{
+	__raw_writel(val, p->iobase + offset);
+}
+
+
+static inline u32
+pwmc_readl(const struct atmel_pwm *p,
+	   unsigned offset)
+{
+	return __raw_readl(p->iobase + offset);
+}
+
+
+static inline void
+pwmc_chan_writel(const struct pwm_channel *p,
+		 u32 offset, u32 val)
+{
+	const struct atmel_pwm *ap
+		= container_of(p->pwm, struct atmel_pwm, pwm);
+
+	if (PWMC_CMR == offset)
+		val &= ((1 << PWMC_CMR_CPD)
+			| (1 << PWMC_CMR_CPOL)
+			| (1 << PWMC_CMR_CALG)
+			| (PWMC_CMR_CPRE_MASK));
+	else
+		val &= ap->ccnt_mask;
+
+	pwmc_writel(ap, offset + PWMC_CHAN_BASE
+		    + (p->chan * PWMC_CHAN_STRIDE), val);
+}
+
+
+static inline u32
+pwmc_chan_readl(const struct pwm_channel *p,
+		u32 offset)
+{
+	const struct atmel_pwm *ap
+		= container_of(p->pwm, struct atmel_pwm, pwm);
+
+	return pwmc_readl(ap, offset + PWMC_CHAN_BASE
+			  + (p->chan * PWMC_CHAN_STRIDE));
+}
+
+
+static inline int
+__atmel_pwm_is_on(struct pwm_channel *p)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	return (pwmc_readl(ap, PWMC_SR) & (1 << p->chan)) ? 1 : 0;
+}
+
+
+static inline void
+__atmel_pwm_unsynchronize(struct pwm_channel *p,
+			  struct pwm_channel *to_p)
+{
+	const struct atmel_pwm *ap
+		= container_of(p->pwm, struct atmel_pwm, pwm);
+	int wchan;
+
+	if (to_p) {
+		ap->sync_mask[p->chan] &= ~(1 << to_p->chan);
+		ap->sync_mask[to_p->chan] &= ~(1 << p->chan);
+		goto done;
+	}
+
+	ap->sync_mask[p->chan] = 0;
+	for (wchan = 0; wchan < ap->pwm.nchan; wchan++)
+		ap->sync_mask[wchan] &= ~(1 << p->chan);
+done:
+	pr_debug("%s:%d sync_mask %x\n",
+		 p->pwm->bus_id, p->chan, ap->sync_mask[p->chan]);
+}
+
+
+static inline void
+__atmel_pwm_synchronize(struct pwm_channel *p,
+			struct pwm_channel *to_p)
+{
+	const struct atmel_pwm *ap
+		= container_of(p->pwm, struct atmel_pwm, pwm);
+
+	if (!to_p)
+		return;
+
+	ap->sync_mask[p->chan] |= (1 << to_p->chan);
+	ap->sync_mask[to_p->chan] |= (1 << p->chan);
+
+	pr_debug("%s:%d sync_mask %x\n",
+		 p->pwm->bus_id, p->chan, ap->sync_mask[p->chan]);
+}
+
+
+static inline void
+__atmel_pwm_stop(struct pwm_channel *p)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	u32 chid = 1 << p->chan;
+
+	pwmc_writel(ap, PWMC_DIS, ap->sync_mask[p->chan] | chid);
+}
+
+
+static inline void
+__atmel_pwm_start(struct pwm_channel *p)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	u32 chid = 1 << p->chan;
+
+	pwmc_writel(ap, PWMC_ENA, ap->sync_mask[p->chan] | chid);
+}
+
+
+static int
+atmel_pwm_synchronize(struct pwm_channel *p,
+		      struct pwm_channel *to_p)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&p->lock, flags);
+	__atmel_pwm_synchronize(p, to_p);
+	spin_unlock_irqrestore(&p->lock, flags);
+	return 0;
+}
+
+
+static int
+atmel_pwm_unsynchronize(struct pwm_channel *p,
+			struct pwm_channel *from_p)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&p->lock, flags);
+	__atmel_pwm_unsynchronize(p, from_p);
+	spin_unlock_irqrestore(&p->lock, flags);
+	return 0;
+}
+
+
+static inline int
+__atmel_pwm_config_polarity(struct pwm_channel *p,
+			    struct pwm_channel_config *c)
+{
+	u32 cmr = pwmc_chan_readl(p, PWMC_CMR);
+
+	if (c->polarity)
+		cmr &= ~BIT(PWMC_CMR_CPOL);
+	else
+		cmr |= BIT(PWMC_CMR_CPOL);
+	pwmc_chan_writel(p, PWMC_CMR, cmr);
+
+	pr_debug("%s:%d polarity %d\n", p->pwm->bus_id,
+		 p->chan, c->polarity);
+	return 0;
+}
+
+
+static inline int
+__atmel_pwm_config_duty_ticks(struct pwm_channel *p,
+			      struct pwm_channel_config *c)
+{
+	u32 cmr, cprd, cpre, cdty;
+
+	cmr = pwmc_chan_readl(p, PWMC_CMR);
+	cprd = pwmc_chan_readl(p, PWMC_CPRD);
+
+	cpre = cmr & PWMC_CMR_CPRE_MASK;
+	cmr &= ~BIT(PWMC_CMR_CPD);
+
+	cdty = cprd - (c->duty_ticks >> cpre);
+
+	p->duty_ticks = c->duty_ticks;
+
+	if (__atmel_pwm_is_on(p)) {
+		pwmc_chan_writel(p, PWMC_CMR, cmr);
+		pwmc_chan_writel(p, PWMC_CUPD, cdty);
+	} else
+		pwmc_chan_writel(p, PWMC_CDTY, cdty);
+
+	pr_debug("%s:%d duty_ticks = %lu cprd = %x"
+		 " cdty = %x cpre = %x\n",
+		 p->pwm->bus_id, p->chan, p->duty_ticks,
+		 cprd, cdty, cpre);
+
+	return 0;
+}
+
+
+static inline int
+__atmel_pwm_config_period_ticks(struct pwm_channel *p,
+				struct pwm_channel_config *c)
+{
+	u32 cmr, cprd, cpre;
+
+	cpre = fls(c->period_ticks);
+	if (cpre < 16)
+		cpre = 0;
+	else {
+		cpre -= 15;
+		if (cpre > 10)
+			return -EINVAL;
+	}
+
+	cmr = pwmc_chan_readl(p, PWMC_CMR);
+	cmr &= ~PWMC_CMR_CPRE_MASK;
+	cmr |= cpre;
+
+	cprd = c->period_ticks >> cpre;
+
+	pwmc_chan_writel(p, PWMC_CMR, cmr);
+	pwmc_chan_writel(p, PWMC_CPRD, cprd);
+	p->period_ticks = c->period_ticks;
+
+	pr_debug("%s:%d period_ticks = %lu cprd = %x cpre = %x\n",
+		 p->pwm->bus_id, p->chan, p->period_ticks, cprd, cpre);
+
+	return 0;
+}
+
+
+
+static int
+atmel_pwm_config_nosleep(struct pwm_channel *p,
+			 struct pwm_channel_config *c)
+{
+	int ret = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&p->lock, flags);
+
+	switch (c->config_mask) {
+
+	case PWM_CONFIG_DUTY_TICKS:
+		__atmel_pwm_config_duty_ticks(p, c);
+		break;
+
+	case PWM_CONFIG_STOP:
+		__atmel_pwm_stop(p);
+		pr_debug("%s:%d stop\n", p->pwm->bus_id, p->chan);
+		break;
+
+	case PWM_CONFIG_START:
+		__atmel_pwm_start(p);
+		pr_debug("%s:%d start\n", p->pwm->bus_id, p->chan);
+		break;
+
+	case PWM_CONFIG_POLARITY:
+		__atmel_pwm_config_polarity(p, c);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	spin_unlock_irqrestore(&p->lock, flags);
+	return ret;
+}
+
+
+static int
+atmel_pwm_stop_sync(struct pwm_channel *p)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	int ret;
+	int was_on = __atmel_pwm_is_on(p);
+
+	if (was_on) {
+		do {
+			init_completion(&p->complete);
+			set_bit(FLAG_STOP, &p->flags);
+			pwmc_writel(ap, PWMC_IER, 1 << p->chan);
+
+			pr_debug("%s:%d waiting on stop_sync completion...\n",
+				 p->pwm->bus_id, p->chan);
+
+			ret = wait_for_completion_interruptible(&p->complete);
+
+			pr_debug("%s:%d stop_sync complete (%d)\n",
+				 p->pwm->bus_id, p->chan, ret);
+
+			if (ret)
+				return ret;
+		} while (p->flags & BIT(FLAG_STOP));
+	}
+
+	pr_debug("%s:%d stop_sync returning %d\n",
+		 p->pwm->bus_id, p->chan, was_on);
+
+	return was_on;
+}
+
+
+static int
+atmel_pwm_config(struct pwm_channel *p,
+		 struct pwm_channel_config *c)
+{
+	int was_on = 0;
+
+	if (p->pwm->config_nosleep) {
+		if (!p->pwm->config_nosleep(p, c))
+			return 0;
+	}
+
+	might_sleep();
+
+	pr_debug("%s:%d config_mask %x\n",
+		 p->pwm->bus_id, p->chan, c->config_mask);
+
+	was_on = atmel_pwm_stop_sync(p);
+	if (was_on < 0)
+		return was_on;
+
+	if (c->config_mask & PWM_CONFIG_PERIOD_TICKS) {
+		__atmel_pwm_config_period_ticks(p, c);
+		if (!(c->config_mask & PWM_CONFIG_DUTY_TICKS)) {
+			struct pwm_channel_config d = {
+				.config_mask = PWM_CONFIG_DUTY_TICKS,
+				.duty_ticks = p->duty_ticks,
+			};
+			__atmel_pwm_config_duty_ticks(p, &d);
+		}
+	}
+
+	if (c->config_mask & PWM_CONFIG_DUTY_TICKS)
+		__atmel_pwm_config_duty_ticks(p, c);
+
+	if (c->config_mask & PWM_CONFIG_POLARITY)
+		__atmel_pwm_config_polarity(p, c);
+
+	if ((c->config_mask & PWM_CONFIG_START)
+	    || (was_on && !(c->config_mask & PWM_CONFIG_STOP)))
+		__atmel_pwm_start(p);
+
+	return 0;
+}
+
+
+static void
+__atmel_pwm_set_callback(struct pwm_channel *p,
+			 pwm_callback_t callback)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+
+	p->callback = callback;
+	pwmc_writel(ap, p->callback ? PWMC_IER : PWMC_IDR, 1 << p->chan);
+	pr_debug("%s:%d set_callback %p\n", p->pwm->bus_id, p->chan, callback);
+}
+
+
+static int
+atmel_pwm_set_callback(struct pwm_channel *p,
+		       pwm_callback_t callback)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ap->lock, flags);
+	__atmel_pwm_set_callback(p, callback);
+	spin_unlock_irqrestore(&ap->lock, flags);
+
+	return 0;
+}
+
+
+static int
+atmel_pwm_request(struct pwm_channel *p)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	unsigned long flags;
+
+	spin_lock_irqsave(&p->lock, flags);
+	clk_enable(ap->clk);
+	p->tick_hz = clk_get_rate(ap->clk);
+	__atmel_pwm_unsynchronize(p, NULL);
+	__atmel_pwm_stop(p);
+	spin_unlock_irqrestore(&p->lock, flags);
+
+	return 0;
+}
+
+
+static void
+atmel_pwm_free(struct pwm_channel *p)
+{
+	struct atmel_pwm *ap = container_of(p->pwm, struct atmel_pwm, pwm);
+	clk_disable(ap->clk);
+}
+
+
+static irqreturn_t
+atmel_pwmc_irq(int irq, void *data)
+{
+	struct atmel_pwm *ap = data;
+	struct pwm_channel *p;
+	u32 isr;
+	int chid;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ap->lock, flags);
+
+	isr = pwmc_readl(ap, PWMC_ISR);
+	for (chid = 0; isr; chid++, isr >>= 1) {
+		p = &ap->pwm.channels[chid];
+		if (isr & 1) {
+			if (p->callback)
+				p->callback(p);
+			if (p->flags & BIT(FLAG_STOP)) {
+				__atmel_pwm_stop(p);
+				clear_bit(FLAG_STOP, &p->flags);
+			}
+			complete_all(&p->complete);
+		}
+	}
+
+	spin_unlock_irqrestore(&ap->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+
+static int __init
+atmel_pwmc_probe(struct platform_device *pdev)
+{
+	struct atmel_pwm *ap;
+	struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	int ret = 0;
+
+	ap = kzalloc(sizeof(*ap), GFP_KERNEL);
+	if (!ap) {
+		ret = -ENOMEM;
+		goto err_atmel_pwm_alloc;
+	}
+
+	spin_lock_init(&ap->lock);
+	platform_set_drvdata(pdev, ap);
+
+	ap->pwm.bus_id = pdev->dev.bus_id;
+
+	ap->pwm.nchan = 4; /* TODO: true only for SAM9263 and AP7000 */
+	ap->ccnt_mask = 0xffffUL; /* TODO: true only for SAM9263 */
+
+	ap->sync_mask = kzalloc(ap->pwm.nchan * sizeof(u32), GFP_KERNEL);
+	if (!ap->sync_mask) {
+		ret = -ENOMEM;
+		goto err_alloc_sync_masks;
+	}
+
+	ap->pwm.owner = THIS_MODULE;
+	ap->pwm.request = atmel_pwm_request;
+	ap->pwm.free = atmel_pwm_free;
+	ap->pwm.config_nosleep = atmel_pwm_config_nosleep;
+	ap->pwm.config = atmel_pwm_config;
+	ap->pwm.synchronize = atmel_pwm_synchronize;
+	ap->pwm.unsynchronize = atmel_pwm_unsynchronize;
+	ap->pwm.set_callback = atmel_pwm_set_callback;
+
+	ap->clk = clk_get(&pdev->dev, "pwm_clk");
+	if (IS_ERR(ap->clk)) {
+		pr_info("%s: clk_get error %ld\n",
+			ap->pwm.bus_id, PTR_ERR(ap->clk));
+		ret = -ENODEV;
+		goto err_clk_get;
+	}
+
+	ap->iobase = ioremap_nocache(r->start, r->end - r->start + 1);
+	if (!ap->iobase) {
+		ret = -ENODEV;
+		goto err_ioremap;
+	}
+
+	clk_enable(ap->clk);
+	pwmc_writel(ap, PWMC_DIS, -1);
+	pwmc_writel(ap, PWMC_IDR, -1);
+	clk_disable(ap->clk);
+
+	ap->irq = platform_get_irq(pdev, 0);
+	if (ap->irq != -ENXIO) {
+		ret = request_irq(ap->irq, atmel_pwmc_irq, 0,
+				  ap->pwm.bus_id, ap);
+		if (ret)
+			goto err_request_irq;
+	}
+
+	ret = pwm_register(&ap->pwm);
+	if (ret)
+		goto err_pwm_register;
+
+	return 0;
+
+err_pwm_register:
+	if (ap->irq != -ENXIO)
+		free_irq(ap->irq, ap);
+err_request_irq:
+	iounmap(ap->iobase);
+err_ioremap:
+	clk_put(ap->clk);
+err_clk_get:
+	platform_set_drvdata(pdev, NULL);
+err_alloc_sync_masks:
+	kfree(ap);
+err_atmel_pwm_alloc:
+	return ret;
+}
+
+
+static int __devexit
+atmel_pwmc_remove(struct platform_device *pdev)
+{
+	struct atmel_pwm *ap = platform_get_drvdata(pdev);
+	int ret;
+
+	/* TODO: what can we do if this fails? */
+	ret = pwm_unregister(&ap->pwm);
+
+	clk_enable(ap->clk);
+	pwmc_writel(ap, PWMC_IDR, -1);
+	pwmc_writel(ap, PWMC_DIS, -1);
+	clk_disable(ap->clk);
+
+	if (ap->irq != -ENXIO)
+		free_irq(ap->irq, ap);
+
+	clk_put(ap->clk);
+	iounmap(ap->iobase);
+
+	kfree(ap);
+
+	return 0;
+}
+
+
+static struct platform_driver atmel_pwm_driver = {
+	.driver = {
+		.name = "atmel_pwmc",
+		.owner = THIS_MODULE,
+	},
+	.probe = atmel_pwmc_probe,
+	.remove = __devexit_p(atmel_pwmc_remove),
+};
+
+
+static int __init atmel_pwm_init(void)
+{
+	return platform_driver_register(&atmel_pwm_driver);
+}
+module_init(atmel_pwm_init);
+
+
+static void atmel_pwm_exit(void)
+{
+	platform_driver_unregister(&atmel_pwm_driver);
+}
+module_exit(atmel_pwm_exit);
+
+
+MODULE_AUTHOR("Bill Gatliff <bgat@billgatliff.com>");
+MODULE_DESCRIPTION("Driver for Atmel PWMC peripheral");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:atmel_pwmc");
-- 
1.5.6.5

[PATCH 5/6] [PWM] Install new Atmel PWMC driver in Kconfig, expunge old one

[Bill Gatliff]

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 arch/arm/Kconfig         |    2 +
 drivers/Makefile         |    2 +
 drivers/misc/Kconfig     |    9 -
 drivers/misc/Makefile    |    1 -
 drivers/misc/atmel_pwm.c |  409 ----------------------------------------------
 drivers/pwm/Kconfig      |   24 +++
 drivers/pwm/Makefile     |    6 +
 7 files changed, 34 insertions(+), 419 deletions(-)
 delete mode 100644 drivers/misc/atmel_pwm.c
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4853f9d..80c09fa 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1228,6 +1228,8 @@ source "drivers/spi/Kconfig"
 
 source "drivers/gpio/Kconfig"
 
+source "drivers/pwm/Kconfig"
+
 source "drivers/w1/Kconfig"
 
 source "drivers/power/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 2735bde..f242fc6 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -6,6 +6,8 @@
 #
 
 obj-y				+= gpio/
+obj-$(CONFIG_GENERIC_PWM)	+= pwm/
+
 obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a726f3b..cdea0bb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -13,15 +13,6 @@ menuconfig MISC_DEVICES
 
 if MISC_DEVICES
 
-config ATMEL_PWM
-	tristate "Atmel AT32/AT91 PWM support"
-	depends on AVR32 || ARCH_AT91
-	help
-	  This option enables device driver support for the PWM channels
-	  on certain Atmel prcoessors.  Pulse Width Modulation is used for
-	  purposes including software controlled power-efficent backlights
-	  on LCD displays, motor control, and waveform generation.
-
 config ATMEL_TCLIB
 	bool "Atmel AT32/AT91 Timer/Counter Library"
 	depends on (AVR32 || ARCH_AT91)
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c6c13f6..9e67012 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -10,7 +10,6 @@ obj-$(CONFIG_EEEPC_LAPTOP)	+= eeepc-laptop.o
 obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
 obj-$(CONFIG_ACER_WMI)		+= acer-wmi.o
-obj-$(CONFIG_ATMEL_PWM)		+= atmel_pwm.o
 obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
 obj-$(CONFIG_ATMEL_TCLIB)	+= atmel_tclib.o
 obj-$(CONFIG_HP_WMI)		+= hp-wmi.o
diff --git a/drivers/misc/atmel_pwm.c b/drivers/misc/atmel_pwm.c
deleted file mode 100644
index 6aa5294..0000000
--- a/drivers/misc/atmel_pwm.c
+++ /dev/null
@@ -1,409 +0,0 @@
-#include <linux/module.h>
-#include <linux/clk.h>
-#include <linux/err.h>
-#include <linux/io.h>
-#include <linux/interrupt.h>
-#include <linux/platform_device.h>
-#include <linux/atmel_pwm.h>
-
-
-/*
- * This is a simple driver for the PWM controller found in various newer
- * Atmel SOCs, including the AVR32 series and the AT91sam9263.
- *
- * Chips with current Linux ports have only 4 PWM channels, out of max 32.
- * AT32UC3A and AT32UC3B chips have 7 channels (but currently no Linux).
- * Docs are inconsistent about the width of the channel counter registers;
- * it's at least 16 bits, but several places say 20 bits.
- */
-#define	PWM_NCHAN	4		/* max 32 */
-
-struct pwm {
-	spinlock_t		lock;
-	struct platform_device	*pdev;
-	u32			mask;
-	int			irq;
-	void __iomem		*base;
-	struct clk		*clk;
-	struct pwm_channel	*channel[PWM_NCHAN];
-	void			(*handler[PWM_NCHAN])(struct pwm_channel *);
-};
-
-
-/* global PWM controller registers */
-#define PWM_MR		0x00
-#define PWM_ENA		0x04
-#define PWM_DIS		0x08
-#define PWM_SR		0x0c
-#define PWM_IER		0x10
-#define PWM_IDR		0x14
-#define PWM_IMR		0x18
-#define PWM_ISR		0x1c
-
-static inline void pwm_writel(const struct pwm *p, unsigned offset, u32 val)
-{
-	__raw_writel(val, p->base + offset);
-}
-
-static inline u32 pwm_readl(const struct pwm *p, unsigned offset)
-{
-	return __raw_readl(p->base + offset);
-}
-
-static inline void __iomem *pwmc_regs(const struct pwm *p, int index)
-{
-	return p->base + 0x200 + index * 0x20;
-}
-
-static struct pwm *pwm;
-
-static void pwm_dumpregs(struct pwm_channel *ch, char *tag)
-{
-	struct device	*dev = &pwm->pdev->dev;
-
-	dev_dbg(dev, "%s: mr %08x, sr %08x, imr %08x\n",
-		tag,
-		pwm_readl(pwm, PWM_MR),
-		pwm_readl(pwm, PWM_SR),
-		pwm_readl(pwm, PWM_IMR));
-	dev_dbg(dev,
-		"pwm ch%d - mr %08x, dty %u, prd %u, cnt %u\n",
-		ch->index,
-		pwm_channel_readl(ch, PWM_CMR),
-		pwm_channel_readl(ch, PWM_CDTY),
-		pwm_channel_readl(ch, PWM_CPRD),
-		pwm_channel_readl(ch, PWM_CCNT));
-}
-
-
-/**
- * pwm_channel_alloc - allocate an unused PWM channel
- * @index: identifies the channel
- * @ch: structure to be initialized
- *
- * Drivers allocate PWM channels according to the board's wiring, and
- * matching board-specific setup code.  Returns zero or negative errno.
- */
-int pwm_channel_alloc(int index, struct pwm_channel *ch)
-{
-	unsigned long	flags;
-	int		status = 0;
-
-	/* insist on PWM init, with this signal pinned out */
-	if (!pwm || !(pwm->mask & 1 << index))
-		return -ENODEV;
-
-	if (index < 0 || index >= PWM_NCHAN || !ch)
-		return -EINVAL;
-	memset(ch, 0, sizeof *ch);
-
-	spin_lock_irqsave(&pwm->lock, flags);
-	if (pwm->channel[index])
-		status = -EBUSY;
-	else {
-		clk_enable(pwm->clk);
-
-		ch->regs = pwmc_regs(pwm, index);
-		ch->index = index;
-
-		/* REVISIT: ap7000 seems to go 2x as fast as we expect!! */
-		ch->mck = clk_get_rate(pwm->clk);
-
-		pwm->channel[index] = ch;
-		pwm->handler[index] = NULL;
-
-		/* channel and irq are always disabled when we return */
-		pwm_writel(pwm, PWM_DIS, 1 << index);
-		pwm_writel(pwm, PWM_IDR, 1 << index);
-	}
-	spin_unlock_irqrestore(&pwm->lock, flags);
-	return status;
-}
-EXPORT_SYMBOL(pwm_channel_alloc);
-
-static int pwmcheck(struct pwm_channel *ch)
-{
-	int		index;
-
-	if (!pwm)
-		return -ENODEV;
-	if (!ch)
-		return -EINVAL;
-	index = ch->index;
-	if (index < 0 || index >= PWM_NCHAN || pwm->channel[index] != ch)
-		return -EINVAL;
-
-	return index;
-}
-
-/**
- * pwm_channel_free - release a previously allocated channel
- * @ch: the channel being released
- *
- * The channel is completely shut down (counter and IRQ disabled),
- * and made available for re-use.  Returns zero, or negative errno.
- */
-int pwm_channel_free(struct pwm_channel *ch)
-{
-	unsigned long	flags;
-	int		t;
-
-	spin_lock_irqsave(&pwm->lock, flags);
-	t = pwmcheck(ch);
-	if (t >= 0) {
-		pwm->channel[t] = NULL;
-		pwm->handler[t] = NULL;
-
-		/* channel and irq are always disabled when we return */
-		pwm_writel(pwm, PWM_DIS, 1 << t);
-		pwm_writel(pwm, PWM_IDR, 1 << t);
-
-		clk_disable(pwm->clk);
-		t = 0;
-	}
-	spin_unlock_irqrestore(&pwm->lock, flags);
-	return t;
-}
-EXPORT_SYMBOL(pwm_channel_free);
-
-int __pwm_channel_onoff(struct pwm_channel *ch, int enabled)
-{
-	unsigned long	flags;
-	int		t;
-
-	/* OMITTED FUNCTIONALITY:  starting several channels in synch */
-
-	spin_lock_irqsave(&pwm->lock, flags);
-	t = pwmcheck(ch);
-	if (t >= 0) {
-		pwm_writel(pwm, enabled ? PWM_ENA : PWM_DIS, 1 << t);
-		t = 0;
-		pwm_dumpregs(ch, enabled ? "enable" : "disable");
-	}
-	spin_unlock_irqrestore(&pwm->lock, flags);
-
-	return t;
-}
-EXPORT_SYMBOL(__pwm_channel_onoff);
-
-/**
- * pwm_clk_alloc - allocate and configure CLKA or CLKB
- * @prescale: from 0..10, the power of two used to divide MCK
- * @div: from 1..255, the linear divisor to use
- *
- * Returns PWM_CPR_CLKA, PWM_CPR_CLKB, or negative errno.  The allocated
- * clock will run with a period of (2^prescale * div) / MCK, or twice as
- * long if center aligned PWM output is used.  The clock must later be
- * deconfigured using pwm_clk_free().
- */
-int pwm_clk_alloc(unsigned prescale, unsigned div)
-{
-	unsigned long	flags;
-	u32		mr;
-	u32		val = (prescale << 8) | div;
-	int		ret = -EBUSY;
-
-	if (prescale >= 10 || div == 0 || div > 255)
-		return -EINVAL;
-
-	spin_lock_irqsave(&pwm->lock, flags);
-	mr = pwm_readl(pwm, PWM_MR);
-	if ((mr & 0xffff) == 0) {
-		mr |= val;
-		ret = PWM_CPR_CLKA;
-	} else if ((mr & (0xffff << 16)) == 0) {
-		mr |= val << 16;
-		ret = PWM_CPR_CLKB;
-	}
-	if (ret > 0)
-		pwm_writel(pwm, PWM_MR, mr);
-	spin_unlock_irqrestore(&pwm->lock, flags);
-	return ret;
-}
-EXPORT_SYMBOL(pwm_clk_alloc);
-
-/**
- * pwm_clk_free - deconfigure and release CLKA or CLKB
- *
- * Reverses the effect of pwm_clk_alloc().
- */
-void pwm_clk_free(unsigned clk)
-{
-	unsigned long	flags;
-	u32		mr;
-
-	spin_lock_irqsave(&pwm->lock, flags);
-	mr = pwm_readl(pwm, PWM_MR);
-	if (clk == PWM_CPR_CLKA)
-		pwm_writel(pwm, PWM_MR, mr & ~(0xffff << 0));
-	if (clk == PWM_CPR_CLKB)
-		pwm_writel(pwm, PWM_MR, mr & ~(0xffff << 16));
-	spin_unlock_irqrestore(&pwm->lock, flags);
-}
-EXPORT_SYMBOL(pwm_clk_free);
-
-/**
- * pwm_channel_handler - manage channel's IRQ handler
- * @ch: the channel
- * @handler: the handler to use, possibly NULL
- *
- * If the handler is non-null, the handler will be called after every
- * period of this PWM channel.  If the handler is null, this channel
- * won't generate an IRQ.
- */
-int pwm_channel_handler(struct pwm_channel *ch,
-		void (*handler)(struct pwm_channel *ch))
-{
-	unsigned long	flags;
-	int		t;
-
-	spin_lock_irqsave(&pwm->lock, flags);
-	t = pwmcheck(ch);
-	if (t >= 0) {
-		pwm->handler[t] = handler;
-		pwm_writel(pwm, handler ? PWM_IER : PWM_IDR, 1 << t);
-		t = 0;
-	}
-	spin_unlock_irqrestore(&pwm->lock, flags);
-
-	return t;
-}
-EXPORT_SYMBOL(pwm_channel_handler);
-
-static irqreturn_t pwm_irq(int id, void *_pwm)
-{
-	struct pwm	*p = _pwm;
-	irqreturn_t	handled = IRQ_NONE;
-	u32		irqstat;
-	int		index;
-
-	spin_lock(&p->lock);
-
-	/* ack irqs, then handle them */
-	irqstat = pwm_readl(pwm, PWM_ISR);
-
-	while (irqstat) {
-		struct pwm_channel *ch;
-		void (*handler)(struct pwm_channel *ch);
-
-		index = ffs(irqstat) - 1;
-		irqstat &= ~(1 << index);
-		ch = pwm->channel[index];
-		handler = pwm->handler[index];
-		if (handler && ch) {
-			spin_unlock(&p->lock);
-			handler(ch);
-			spin_lock(&p->lock);
-			handled = IRQ_HANDLED;
-		}
-	}
-
-	spin_unlock(&p->lock);
-	return handled;
-}
-
-static int __init pwm_probe(struct platform_device *pdev)
-{
-	struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	int irq = platform_get_irq(pdev, 0);
-	u32 *mp = pdev->dev.platform_data;
-	struct pwm *p;
-	int status = -EIO;
-
-	if (pwm)
-		return -EBUSY;
-	if (!r || irq < 0 || !mp || !*mp)
-		return -ENODEV;
-	if (*mp & ~((1<<PWM_NCHAN)-1)) {
-		dev_warn(&pdev->dev, "mask 0x%x ... more than %d channels\n",
-			*mp, PWM_NCHAN);
-		return -EINVAL;
-	}
-
-	p = kzalloc(sizeof(*p), GFP_KERNEL);
-	if (!p)
-		return -ENOMEM;
-
-	spin_lock_init(&p->lock);
-	p->pdev = pdev;
-	p->mask = *mp;
-	p->irq = irq;
-	p->base = ioremap(r->start, r->end - r->start + 1);
-	if (!p->base)
-		goto fail;
-	p->clk = clk_get(&pdev->dev, "pwm_clk");
-	if (IS_ERR(p->clk)) {
-		status = PTR_ERR(p->clk);
-		p->clk = NULL;
-		goto fail;
-	}
-
-	status = request_irq(irq, pwm_irq, 0, pdev->name, p);
-	if (status < 0)
-		goto fail;
-
-	pwm = p;
-	platform_set_drvdata(pdev, p);
-
-	return 0;
-
-fail:
-	if (p->clk)
-		clk_put(p->clk);
-	if (p->base)
-		iounmap(p->base);
-
-	kfree(p);
-	return status;
-}
-
-static int __exit pwm_remove(struct platform_device *pdev)
-{
-	struct pwm *p = platform_get_drvdata(pdev);
-
-	if (p != pwm)
-		return -EINVAL;
-
-	clk_enable(pwm->clk);
-	pwm_writel(pwm, PWM_DIS, (1 << PWM_NCHAN) - 1);
-	pwm_writel(pwm, PWM_IDR, (1 << PWM_NCHAN) - 1);
-	clk_disable(pwm->clk);
-
-	pwm = NULL;
-
-	free_irq(p->irq, p);
-	clk_put(p->clk);
-	iounmap(p->base);
-	kfree(p);
-
-	return 0;
-}
-
-static struct platform_driver atmel_pwm_driver = {
-	.driver = {
-		.name = "atmel_pwm",
-		.owner = THIS_MODULE,
-	},
-	.remove = __exit_p(pwm_remove),
-
-	/* NOTE: PWM can keep running in AVR32 "idle" and "frozen" states;
-	 * and all AT91sam9263 states, albeit at reduced clock rate if
-	 * MCK becomes the slow clock (i.e. what Linux labels STR).
-	 */
-};
-
-static int __init pwm_init(void)
-{
-	return platform_driver_probe(&atmel_pwm_driver, pwm_probe);
-}
-module_init(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&atmel_pwm_driver);
-}
-module_exit(pwm_exit);
-
-MODULE_DESCRIPTION("Driver for AT32/AT91 PWM module");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:atmel_pwm");
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 0000000..933bb2c
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,24 @@
+#
+# PWM infrastructure and devices
+#
+
+menuconfig GENERIC_PWM
+	tristate "PWM Support"
+	help
+	  This enables PWM support through the generic PWM library.
+	  If unsure, say N.
+
+if GENERIC_PWM
+
+config ATMEL_PWM
+	tristate "Atmel AT32/AT91 PWM support"
+	depends on AVR32 || ARCH_AT91
+	help
+	  This option enables device driver support for the PWMC
+	  peripheral channels found on certain Atmel processors.
+	  Pulse Width Modulation is used many for purposes, including
+	  software controlled power-efficent backlights on LCD
+	  displays, motor control, and waveform generation.  If
+	  unsure, say N.
+
+endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 0000000..21634f6
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for pwm devices
+#
+obj-y := pwm.o
+
+obj-$(CONFIG_ATMEL_PWM)		+= atmel-pwm.o
-- 
1.5.6.5

[PATCH 6/6] [PWM] New LED driver and trigger that use PWM API

[Bill Gatliff]

The leds-pwm driver maps LED API calls into PWM API calls.  Both
brighness_get/set and blink_set are supported.

Some LED API entry points are not sleep-safe, which will cause problems
with PWM implementations that require sleeping during hardware state
updates.  The Atmel PWMC driver avoids these cases; other driver
implementations must be aware.

The ledtrig-dim trigger maps current system load to an LED brighness of
0-100% for system loads in the range 0-1 as reported by avenrun[].  If the
LED associated with the trigger is a suitable PWM output, then the user
will see the brightness of the LED change as system load changes.

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 drivers/leds/Kconfig       |   24 +++++--
 drivers/leds/Makefile      |    2 +
 drivers/leds/leds-pwm.c    |  167 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/leds/ledtrig-dim.c |   95 +++++++++++++++++++++++++
 include/linux/pwm-led.h    |   34 +++++++++
 5 files changed, 317 insertions(+), 5 deletions(-)
 create mode 100644 drivers/leds/leds-pwm.c
 create mode 100644 drivers/leds/ledtrig-dim.c
 create mode 100644 include/linux/pwm-led.h

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index e3e4042..3339a4c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -17,12 +17,12 @@ config LEDS_CLASS
 
 comment "LED drivers"
 
-config LEDS_ATMEL_PWM
-	tristate "LED Support using Atmel PWM outputs"
-	depends on LEDS_CLASS && ATMEL_PWM
+config LEDS_CORGI
+	tristate "LED Support for the Sharp SL-C7x0 series"
+	depends on LEDS_CLASS && PXA_SHARP_C7xx
 	help
-	  This option enables support for LEDs driven using outputs
-	  of the dedicated PWM controller found on newer Atmel SOCs.
+	  This option enables support for the LEDs on Sharp Zaurus
+	  SL-C7x0 series (C700, C750, C760, C860).
 
 config LEDS_LOCOMO
 	tristate "LED Support for Locomo device"
@@ -113,6 +113,12 @@ config LEDS_GPIO
 	  outputs. To be useful the particular board must have LEDs
 	  and they must be connected to the GPIO lines.
 
+config LEDS_PWM
+       tristate "LED Support for PWM connected LEDs"
+       depends on LEDS_CLASS && GENERIC_PWM
+       help
+         Enables support for LEDs connected to PWM outputs.
+
 config LEDS_CM_X270
 	tristate "LED Support for the CM-X270 LEDs"
 	depends on LEDS_CLASS && MACH_ARMCORE
@@ -184,6 +190,14 @@ config LEDS_TRIGGER_IDE_DISK
 	  This allows LEDs to be controlled by IDE disk activity.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_DIM
+	tristate "LED Dimmer Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  Regulates the brightness of an LED based on the 1-minute CPU
+	  load average.  Ideal for PWM-driven LEDs.
+	  If unsure, say Y.
+
 config LEDS_TRIGGER_HEARTBEAT
 	tristate "LED Heartbeat Trigger"
 	depends on LEDS_TRIGGERS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index eb186c3..3a16e37 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
 obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
 obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
 obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
+obj-$(CONFIG_LEDS_PWM)					+= leds-pwm.o
 obj-$(CONFIG_LEDS_CM_X270)              += leds-cm-x270.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
@@ -26,5 +27,6 @@ obj-$(CONFIG_LEDS_PCA955X)		+= leds-pca955x.o
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
+obj-$(CONFIG_LEDS_TRIGGER_DIM)		+= ledtrig-dim.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
new file mode 100644
index 0000000..3103dc3
--- /dev/null
+++ b/drivers/leds/leds-pwm.c
@@ -0,0 +1,167 @@
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <linux/pwm-led.h>
+
+
+struct led_pwm {
+	struct led_classdev	led;
+	struct pwm_channel	*pwm;
+	int percent;
+};
+
+
+static void
+led_pwm_brightness_set(struct led_classdev *c,
+		       enum led_brightness b)
+{
+	struct led_pwm *led;
+	int percent;
+
+	percent = (b * 100) / (LED_FULL - LED_OFF);
+	led = container_of(c, struct led_pwm, led);
+	led->percent = percent;
+	pwm_set_duty_percent(led->pwm, percent);
+}
+
+
+static enum led_brightness
+led_pwm_brightness_get(struct led_classdev *c)
+{
+	struct led_pwm *led;
+	led = container_of(c, struct led_pwm, led);
+	return led->percent;
+}
+
+
+static int
+led_pwm_blink_set(struct led_classdev *c,
+		  unsigned long *on_ms,
+		  unsigned long *off_ms)
+{
+	struct led_pwm *led;
+	struct pwm_channel_config cfg;
+
+	led = container_of(c, struct led_pwm, led);
+
+	if (*on_ms == 0 && *off_ms == 0) {
+		*on_ms = 1000UL;
+		*off_ms = 1000UL;
+	}
+
+	cfg.config_mask = PWM_CONFIG_DUTY_NS
+		| PWM_CONFIG_PERIOD_NS;
+
+	cfg.duty_ns = *on_ms * 1000000UL;
+	cfg.period_ns = (*on_ms + *off_ms) * 1000000UL;
+
+	return pwm_config(led->pwm, &cfg);
+}
+
+
+static int __init
+led_pwm_probe(struct platform_device *pdev)
+{
+	struct pwm_led_platform_data *pdata = pdev->dev.platform_data;
+	struct led_pwm *led;
+	struct device *d = &pdev->dev;
+	int ret;
+
+	if (!pdata || !pdata->led_info)
+		return -EINVAL;
+
+	if (!try_module_get(d->driver->owner))
+		return -ENODEV;
+
+	led = kzalloc(sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->pwm = pwm_request(pdata->bus_id, pdata->chan,
+			       pdata->led_info->name);
+	if (!led->pwm) {
+		ret = -EINVAL;
+		goto err_pwm_request;
+	}
+
+	platform_set_drvdata(pdev, led);
+
+	led->led.name = pdata->led_info->name;
+	led->led.default_trigger = pdata->led_info->default_trigger;
+	led->led.brightness_set = led_pwm_brightness_set;
+	led->led.brightness_get = led_pwm_brightness_get;
+	led->led.blink_set = led_pwm_blink_set;
+	led->led.brightness = LED_OFF;
+
+	ret = pwm_config(led->pwm, pdata->config);
+	if (ret)
+		goto err_pwm_config;
+	pwm_start(led->pwm);
+
+	ret = led_classdev_register(&pdev->dev, &led->led);
+	if (ret < 0)
+		goto err_classdev_register;
+
+	return 0;
+
+err_classdev_register:
+	pwm_stop(led->pwm);
+err_pwm_config:
+	pwm_free(led->pwm);
+err_pwm_request:
+	kfree(led);
+
+	return ret;
+}
+
+
+static int
+led_pwm_remove(struct platform_device *pdev)
+{
+	struct led_pwm *led = platform_get_drvdata(pdev);
+	struct device *d = &pdev->dev;
+
+	led_classdev_unregister(&led->led);
+
+	if (led->pwm) {
+		pwm_stop(led->pwm);
+		pwm_free(led->pwm);
+	}
+
+	kfree(led);
+	module_put(d->driver->owner);
+
+	return 0;
+}
+
+
+static struct platform_driver led_pwm_driver = {
+	.driver = {
+		.name =		"leds-pwm",
+		.owner =	THIS_MODULE,
+	},
+	.probe = led_pwm_probe,
+	.remove = led_pwm_remove,
+};
+
+
+static int __init led_pwm_modinit(void)
+{
+	return platform_driver_register(&led_pwm_driver);
+}
+module_init(led_pwm_modinit);
+
+
+static void __exit led_pwm_modexit(void)
+{
+	platform_driver_unregister(&led_pwm_driver);
+}
+module_exit(led_pwm_modexit);
+
+
+MODULE_AUTHOR("Bill Gatliff <bgat@billgatliff.com>");
+MODULE_DESCRIPTION("Driver for LEDs with PWM-controlled brightness");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-pwm");
diff --git a/drivers/leds/ledtrig-dim.c b/drivers/leds/ledtrig-dim.c
new file mode 100644
index 0000000..299865b
--- /dev/null
+++ b/drivers/leds/ledtrig-dim.c
@@ -0,0 +1,95 @@
+/*
+ * LED Dim Trigger
+ *
+ * Copyright (C) 2008 Bill Gatliff <bgat@billgatliff.com>
+ *
+ * "Dims" an LED based on system load.  Derived from Atsushi Nemoto's
+ * ledtrig-heartbeat.c.
+ *
+ * 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.
+ *
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/timer.h>
+#include <linux/sched.h>
+#include <linux/leds.h>
+
+#include "leds.h"
+
+struct dim_trig_data {
+	struct timer_list timer;
+};
+
+
+static void
+led_dim_function(unsigned long data)
+{
+	struct led_classdev *led_cdev = (struct led_classdev *)data;
+	struct dim_trig_data *dim_data = led_cdev->trigger_data;
+	unsigned int brightness;
+
+	brightness = ((LED_FULL - LED_OFF) * avenrun[0]) / EXP_1;
+	if (brightness > LED_FULL)
+		brightness = LED_FULL;
+
+	led_set_brightness(led_cdev, brightness);
+	mod_timer(&dim_data->timer, jiffies + msecs_to_jiffies(500));
+}
+
+
+static void
+dim_trig_activate(struct led_classdev *led_cdev)
+{
+	struct dim_trig_data *dim_data;
+
+	dim_data = kzalloc(sizeof(*dim_data), GFP_KERNEL);
+	if (!dim_data)
+		return;
+
+	led_cdev->trigger_data = dim_data;
+	setup_timer(&dim_data->timer,
+		    led_dim_function, (unsigned long)led_cdev);
+	led_dim_function(dim_data->timer.data);
+}
+
+
+static void
+dim_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct dim_trig_data *dim_data = led_cdev->trigger_data;
+
+	if (dim_data) {
+		del_timer_sync(&dim_data->timer);
+		kfree(dim_data);
+	}
+}
+
+
+static struct led_trigger dim_led_trigger = {
+	.name     = "dim",
+	.activate = dim_trig_activate,
+	.deactivate = dim_trig_deactivate,
+};
+
+
+static int __init dim_trig_init(void)
+{
+	return led_trigger_register(&dim_led_trigger);
+}
+module_init(dim_trig_init);
+
+
+static void __exit dim_trig_exit(void)
+{
+	led_trigger_unregister(&dim_led_trigger);
+}
+module_exit(dim_trig_exit);
+
+
+MODULE_AUTHOR("Bill Gatliff <bgat@billgatliff.com>");
+MODULE_DESCRIPTION("Dim LED trigger");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/pwm-led.h b/include/linux/pwm-led.h
new file mode 100644
index 0000000..92363c8
--- /dev/null
+++ b/include/linux/pwm-led.h
@@ -0,0 +1,34 @@
+#ifndef __LINUX_PWM_LED_H
+#define __LINUX_PWM_LED_H
+
+/*
+ * include/linux/pwm-led.h
+ *
+ * Copyright (C) 2008 Bill Gatliff
+ *
+ * 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 of the License, 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; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+struct led_info;
+struct pwm_channel_config;
+
+struct pwm_led_platform_data {
+	const char *bus_id;
+	int chan;
+	struct pwm_channel_config *config;
+	struct led_info *led_info;
+};
+
+#endif /* __LINUX_PWM_LED_H */
-- 
1.5.6.5

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[Mike Frysinger]

On Wed, Oct 15, 2008 at 14:14, Bill Gatliff wrote:
> +int pwm_register(struct pwm_device *pwm)
> +{
> +       struct pwm_channel *p;
> +       int wchan;
> +       int ret = 0;

the initialization to 0 here isnt needed
-mike

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[Bill Gatliff]

Mike Frysinger wrote:
> On Wed, Oct 15, 2008 at 14:14, Bill Gatliff wrote:
>> +int pwm_register(struct pwm_device *pwm)
>> +{
>> +       struct pwm_channel *p;
>> +       int wchan;
>> +       int ret = 0;
> 
> the initialization to 0 here isnt needed

This is literally the only feedback I have received.  I take that to mean that
the code basically works as advertised, and nobody else has any problems with it
(it continues to work fine for me here).

So, what's the next step?  How do I advocate for getting this API into mainline?


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[Mike Frysinger]

On Tue, Nov 4, 2008 at 15:16, Bill Gatliff wrote:
> Mike Frysinger wrote:
>> On Wed, Oct 15, 2008 at 14:14, Bill Gatliff wrote:
>>> +int pwm_register(struct pwm_device *pwm)
>>> +{
>>> +       struct pwm_channel *p;
>>> +       int wchan;
>>> +       int ret = 0;
>>
>> the initialization to 0 here isnt needed
>
> This is literally the only feedback I have received.  I take that to mean that
> the code basically works as advertised, and nobody else has any problems with it
> (it continues to work fine for me here).
>
> So, what's the next step?  How do I advocate for getting this API into mainline?

you looking to be maintainer ?  if so, i imagine you start a git tree
(on kernel.org?) for people to pull from, post patches to mainline now
and indicate you'd ask for a 2.6.29 merge ...
-mike

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[David Brownell]

On Tuesday 04 November 2008, Bill Gatliff wrote:
> This is literally the only feedback I have received. 

I was waiting for followup too.  I finally get onto this
list (for some reason the list management software dropped
several previous "please add me" requests) and ... nothing!!


Patches #1 and #2 seem to be in the wrong order ... and
#2 shouldn't give the header's full pathname.  I'd have
merged the two, myself.

Patch #6 seems to remove LEDS_ATMEL_PWM from Kconfig but
leave the driver around ... and restore the now-deleted
LEDS_CORGI support (leds-gpio suffices).  If you're going
to remove LEDS_ATMEL_PWM you should update the code in
mach-at91/leds.c which sets up for that driver...


> So, what's the next step?  How do I advocate for getting
> this API into mainline? 

The number of backing implementations seemed a bit low ...
just one.  And that's switching Atmel's dedicated PWM, but
not the other PWM driver in the tree (for PXA); so it's
not terrifically accessible.	

And even for Atmel's SOC's it seemed a bit light:  the TC
blocks will do PWM quite happily, and are found on many
more chips than the dedicated PWM hardware.

My rule of thumb is that it's not worth considering a
framework as being general enough until it's got three
fairly different backing implementations.  Two is enough
to consider merging, especially with complicated drivers.

Plus the number of framework users is an issue.  The
PWM led driver is a good demo for two basic features,
and it's useful:  brightness (duty cycle beyond visual
perception) and blink rate (duty cycle easily seen with
the eye).  But it's still only really one API customer,
even given that new LED trigger...

So I'd suggest fleshing it out a bit more, so it works
on some non-Atmel hardware as well as more Atmel SOCs.
And fix the goofs I noted above.  :)

- Dave

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[Mike Frysinger]

On Tue, Nov 4, 2008 at 18:55, David Brownell wrote:
> On Tuesday 04 November 2008, Bill Gatliff wrote:
>> So, what's the next step?  How do I advocate for getting
>> this API into mainline?
>
> The number of backing implementations seemed a bit low ...
> just one.  And that's switching Atmel's dedicated PWM, but
> not the other PWM driver in the tree (for PXA); so it's
> not terrifically accessible.
>
> My rule of thumb is that it's not worth considering a
> framework as being general enough until it's got three
> fairly different backing implementations.  Two is enough
> to consider merging, especially with complicated drivers.

every Blackfin processor so far has had dedicated PWM hardware in it,
so a backend driver for that arch would show up ...

> Plus the number of framework users is an issue.  The
> PWM led driver is a good demo for two basic features,
> and it's useful:  brightness (duty cycle beyond visual
> perception) and blink rate (duty cycle easily seen with
> the eye).  But it's still only really one API customer,
> even given that new LED trigger...

there is an lcd driver or two in the tree (probably Blackfin specific)
that uses the PWM hardware as an output to drive the hsync/vsync
signals.

and while LIRC is still out-of-tree for mainline, i wrote a driver
using the PWM hardware as an input ... but there wasnt anything else
Blackfin specific in said driver other than the PWM stuff ...

i'm guessing you'd prefer to see those implemented before moving to
mainline though ?
-mike

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[Bill Gatliff]

David Brownell wrote:
 >
> Patches #1 and #2 seem to be in the wrong order ... and
> #2 shouldn't give the header's full pathname.  I'd have
> merged the two, myself.

Ok, that's good feedback.

> Patch #6 seems to remove LEDS_ATMEL_PWM from Kconfig but
> leave the driver around ... and restore the now-deleted
> LEDS_CORGI support (leds-gpio suffices).  If you're going
> to remove LEDS_ATMEL_PWM you should update the code in
> mach-at91/leds.c which sets up for that driver...

I'll look into this before my next update.

> The number of backing implementations seemed a bit low ...
> just one.  And that's switching Atmel's dedicated PWM, but
> not the other PWM driver in the tree (for PXA); so it's
> not terrifically accessible.	
> 
> And even for Atmel's SOC's it seemed a bit light:  the TC
> blocks will do PWM quite happily, and are found on many
> more chips than the dedicated PWM hardware.

Alright.  The SAM9263 has both PWMC and TC, so it's pretty easy to add TC's to
the list of backing implementations because that hardware is already plugged in.
 :)  And I have PXA hardware here somewhere too, so I'll bring that one over as
well.  Finally, I suddenly have the need to do bit-bang PWM over GPIO, so if
nobody beats me to it (please, someone beat me to it!) then I'll implement that
one as well.

> Plus the number of framework users is an issue.  The
> PWM led driver is a good demo for two basic features,
> and it's useful:  brightness (duty cycle beyond visual
> perception) and blink rate (duty cycle easily seen with
> the eye).  But it's still only really one API customer,
> even given that new LED trigger...

That's a bit of a catch-22.  The whole point of the API is to provide a
framework for users to adopt, where previously there was none.  Adopters are
more likely if the framework is mainlined.

Granted, having a git tree on kernel.org is pretty darned close.  I'll look into
that per Mike Frysinger.

> So I'd suggest fleshing it out a bit more, so it works
> on some non-Atmel hardware as well as more Atmel SOCs.
> And fix the goofs I noted above.  :)

Roger that.  And then I expect an Acked-by from you.  :)


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[Bill Gatliff]

Mike Frysinger wrote:
> every Blackfin processor so far has had dedicated PWM hardware in it,
> so a backend driver for that arch would show up ...

From who?  I don't have that hardware.  I'm happy to hear that someone is
working on it, though, and I would be happy to help review.

> i'm guessing you'd prefer to see those implemented before moving to
> mainline though ?

Sounds like if we can get a git tree on kernel.org, then the first stop would be
my pulling the code into the tree, and then asking Linus to do a pull.

To which is response will probably be: "Bill who?"  :)


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 1/6] [PWM] Generic PWM API implementation

[David Brownell]

On Tuesday 04 November 2008, Mike Frysinger wrote:
> On Tue, Nov 4, 2008 at 18:55, David Brownell wrote:
> > On Tuesday 04 November 2008, Bill Gatliff wrote:
> >> So, what's the next step?  How do I advocate for getting
> >> this API into mainline?
> >
> > The number of backing implementations seemed a bit low ...
> > just one.  And that's switching Atmel's dedicated PWM, but
> > not the other PWM driver in the tree (for PXA); so it's
> > not terrifically accessible.
> >
> > My rule of thumb is that it's not worth considering a
> > framework as being general enough until it's got three
> > fairly different backing implementations.  Two is enough
> > to consider merging, especially with complicated drivers.
> 
> every Blackfin processor so far has had dedicated PWM hardware in it,
> so a backend driver for that arch would show up ...

I'm not questioning the notion that more generic PWM support
would be useful, note!  Just pointing out my rule of thumb,
which has proven to be a good one.

A variety of implementations shakes out portability issues,
and highlights (what *WE* already know!) that this stuff is
actually generally useful.  And when there's no test harness,
having most of a 3-providers vs 3-users matrix known to work
is a good stand-in.


> > Plus the number of framework users is an issue. ...
> 
> there is an lcd driver or two in the tree (probably Blackfin specific)
> that uses the PWM hardware as an output to drive the hsync/vsync
> signals.

... Right, but not through *this* framework.  The proof that
the framework is sufficient for that application too would be
the code.  And hsync/vsync seems like a usefully "different" app
context to me.  Certainly not one that came quickly to my mind!

But as we know, PWM gets used in all kinds of ways.  :)

 
> and while LIRC is still out-of-tree for mainline, i wrote a driver
> using the PWM hardware as an input ... but there wasnt anything else
> Blackfin specific in said driver other than the PWM stuff ...
> 
> i'm guessing you'd prefer to see those implemented before moving to
> mainline though ?

Well, more than just Bill's single implementation and client, yes.
Maybe the Blackfin PWM provider, and one more API client ... though
more of each would be fine too.

I'd imagine Bill would be glad to have more active hands on the
code too ... a few issues always seem to turn up with the first
users, and it's better to sort them out with "early adopters"
before the floodgates open.

Plus:  more users provides some evidence to folk like Andrew and
Linus that the code is useful enough to merge.  Embedded Linux
isn't as visible to them as x86.

- Dave

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

Hi Bill

On Wed, Oct 15, 2008 at 11:14 AM, Bill Gatliff <bgat@billgatliff.com> wrote:
> This series implements a Generic PWM Device API, including reference
> implementations for the Atmel PWMC device, an LED device, and an LED
> trigger.  It is based on linux-2.6.27.
[...]
> The implementation of the Generic PWM Device API is structurally
> similar to the generic GPIO API, except that the PWM code uses
> platform bus_id strings instead of integers to identify target
> deviices.  A configuration structure is also provided, both to
> facilitate atomic hardware state changes and so that the API can be
> extended in a source-code-compatible way to accomodate devices with
> features not anticipated by the current code.

Hey Bill,

I'm concerned about the approach taken here.  As I understand it, the
PWM signals are very similar to GPIOs in that each PWM device controls
an external signal line, just like GPIO lines.  The difference being
that PWMs cannot do input, and has additional capabilities (can be
programmed with a signal; not just on/off/tristate).  Actually, many
GPIOs have these properties too.  I've got a part with output-only
gpios, and gpio devices that also have a PWM.

What is the reason for bringing in an entirely new framework instead
of extending the GPIO API or gpiolib?  I'm not too excited about
having two entirely different frameworks for what basically boils down
to "numbered signal pins".

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Mike Frysinger]

On Fri, Nov 13, 2009 at 14:08, Grant Likely wrote:
> On Wed, Oct 15, 2008 at 11:14 AM, Bill Gatliff wrote:
>> This series implements a Generic PWM Device API, including reference
>> implementations for the Atmel PWMC device, an LED device, and an LED
>> trigger.  It is based on linux-2.6.27.
> [...]
>> The implementation of the Generic PWM Device API is structurally
>> similar to the generic GPIO API, except that the PWM code uses
>> platform bus_id strings instead of integers to identify target
>> deviices.  A configuration structure is also provided, both to
>> facilitate atomic hardware state changes and so that the API can be
>> extended in a source-code-compatible way to accomodate devices with
>> features not anticipated by the current code.
>
> I'm concerned about the approach taken here.  As I understand it, the
> PWM signals are very similar to GPIOs in that each PWM device controls
> an external signal line, just like GPIO lines.  The difference being
> that PWMs cannot do input, and has additional capabilities (can be
> programmed with a signal; not just on/off/tristate).  Actually, many
> GPIOs have these properties too.  I've got a part with output-only
> gpios, and gpio devices that also have a PWM.
>
> What is the reason for bringing in an entirely new framework instead
> of extending the GPIO API or gpiolib?  I'm not too excited about
> having two entirely different frameworks for what basically boils down
> to "numbered signal pins".

unifying resource management obviously makes sense so as to avoid
conflicts, but i dont think the fact that one pin can be multi purpose
means it should be entirely forced into the GPIO framework, nor do i
see any real gain for doing so.
-mike

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Fri, Nov 13, 2009 at 9:22 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Fri, Nov 13, 2009 at 14:08, Grant Likely wrote:
>> On Wed, Oct 15, 2008 at 11:14 AM, Bill Gatliff wrote:
>>> This series implements a Generic PWM Device API, including reference
>>> implementations for the Atmel PWMC device, an LED device, and an LED
>>> trigger.  It is based on linux-2.6.27.
>> [...]
>>> The implementation of the Generic PWM Device API is structurally
>>> similar to the generic GPIO API, except that the PWM code uses
>>> platform bus_id strings instead of integers to identify target
>>> deviices.  A configuration structure is also provided, both to
>>> facilitate atomic hardware state changes and so that the API can be
>>> extended in a source-code-compatible way to accomodate devices with
>>> features not anticipated by the current code.
>>
>> I'm concerned about the approach taken here.  As I understand it, the
>> PWM signals are very similar to GPIOs in that each PWM device controls
>> an external signal line, just like GPIO lines.  The difference being
>> that PWMs cannot do input, and has additional capabilities (can be
>> programmed with a signal; not just on/off/tristate).  Actually, many
>> GPIOs have these properties too.  I've got a part with output-only
>> gpios, and gpio devices that also have a PWM.
>>
>> What is the reason for bringing in an entirely new framework instead
>> of extending the GPIO API or gpiolib?  I'm not too excited about
>> having two entirely different frameworks for what basically boils down
>> to "numbered signal pins".
>
> unifying resource management obviously makes sense so as to avoid
> conflicts, but i dont think the fact that one pin can be multi purpose
> means it should be entirely forced into the GPIO framework, nor do i
> see any real gain for doing so.

Common code is a big gain in and of itself.  It means less to develop,
less to maintain, and fewer APIs in the kernel.  Right now, I don't
see a fundamental difference is between GPIO and PWM pin management.
It is essentially the same problem, and in many cases PWM pins can
also be used as GPIOs.  I think the question should be flipped around;
rather than asking for a compelling reason for them to be merged; I
want to know the compelling reason to keep them separate.  What is the
fundamental difference that keeps them apart?

However, since it was mentioned, I do see some real gains for using
the same infrastructure:
- Devices using GPIO pins can easily be extended to take advantage of
PWM modes.  i could see GPIO LEDs taking advantage of this for example
- (as I already mentioned) PWM pins that can also behave as GPIOs
don't need to register 2 interfaces.
- All the existing support code for hooking up GPIO pins to other
devices can be reused as is.
- Individual platforms have the option of implementing the GPIO+PWM
API directly (fast, but static), or they can hook in via GPIOLIB
(dynamic; slower but pluggable)
- All the OF device tree bindings for GPIOs also work with PWMs.

What I would like to see is the PWM functions added to the GPIO API.
GPIO drivers can then either implement them or not.  If a GPIO driver
supports the PWM function, great.  If not, then it returns -EINVAL.
Heck, I'll even got a driver right now that I'd use it with.  I'm more
than happy to help code it up even.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Friday 13 November 2009, Grant Likely wrote:
> 		  Right now, I don't
> see a fundamental difference is between GPIO and PWM pin management.
> It is essentially the same problem, and in many cases PWM pins can
> also be used as GPIOs. 

Pin management for a given SoC is going to be relevant to setting
every signal, no matter what peripheral it's associated with.  The
same argument applies to an MDIO bus, I2C, 1-wire, and more.

And I don't buy it in those cases either.


> I think the question should be flipped around; 
> rather than asking for a compelling reason for them to be merged; I
> want to know the compelling reason to keep them separate.  What is the
> fundamental difference that keeps them apart?

PWM is about periodic signal generation without CPU intervention.

GPIO is about explicit CPU management/interrogation of single
signals.


> What I would like to see is the PWM functions added to the GPIO API.

No.  If you want a pin mux interface, come up with one of them.

It shouldn't be a PWM interface, a GPIO interface, an I2C interface,
a SPI interface, an MDIO interface, a 1-wire interface ... or any of
dozens of other things.  It'd be purely for pinmux.

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Friday 13 November 2009, Grant Likely wrote:
> I'm concerned about the approach taken here.  As I understand it, the
> PWM signals are very similar to GPIOs in that each PWM device controls
> an external signal line, just like GPIO lines.

PWM is not GPIO, and doesn't fit into a GPIO framework.

Since *everything* boils down to one or more signal lines,
your argument leads directly to Linux having no native
hardware interface except GPIOs.  Not ... practical. ;)



> The difference being 
> that PWMs cannot do input, and has additional capabilities (can be
> programmed with a signal; not just on/off/tristate)

If you want to combine PWM with something else ... timers would
be a better target.  They're both fundamentally about periodic
phenomena.  And quite a lot of timers support PWM output modes...

(A generic interface to hardware timers is lacking, too.)


> What is the reason for bringing in an entirely new framework instead
> of extending the GPIO API or gpiolib?  I'm not too excited about
> having two entirely different frameworks for what basically boils down
> to "numbered signal pins".

You seem to mis-understand what PWM is all about, then.
The whole point of a PWM is to set up a periodic activity
that will run without CPU intervention.

GPIOs, on the other hand, are packaged for manual bit
twiddling.  While it's possible to create low-speed
implementations of serial protocols using GPIOs (like
2-wire/I2C, one-wire, and various SPI variants), those
are explicitly the high-overhead (and low performance)
substitutes, to be used only when native hardware isn't
available (or is broken etc).

For example you won't often get 40 Mbit/sec SPI if you
are bitbanging over GPIOs ... and if you do, it won't
leave much CPU horsepower for much of anything else.

- Dave

Re: [PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

Grant Likely wrote:
> Hi Bill
>
> On Wed, Oct 15, 2008 at 11:14 AM, Bill Gatliff <bgat@billgatliff.com> wrote:
>   
>> This series implements a Generic PWM Device API, including reference
>> implementations for the Atmel PWMC device, an LED device, and an LED
>> trigger.  It is based on linux-2.6.27.
>>     
> [...]
>   
>> The implementation of the Generic PWM Device API is structurally
>> similar to the generic GPIO API, except that the PWM code uses
>> platform bus_id strings instead of integers to identify target
>> deviices.  A configuration structure is also provided, both to
>> facilitate atomic hardware state changes and so that the API can be
>> extended in a source-code-compatible way to accomodate devices with
>> features not anticipated by the current code.
>>     
>
> Hey Bill,
>
> I'm concerned about the approach taken here.  As I understand it, the
> PWM signals are very similar to GPIOs in that each PWM device controls
> an external signal line, just like GPIO lines.  The difference being
> that PWMs cannot do input, and has additional capabilities (can be
> programmed with a signal; not just on/off/tristate).  Actually, many
> GPIOs have these properties too.  I've got a part with output-only
> gpios, and gpio devices that also have a PWM.
>   


It's true that PWM signals can be emitted via GPIO pins, and in fact the
API code I submitted uses the GPIO API combined with an hrtimer to
generate a low-speed PWM output.

It's also true that things like LEDs might want to be driven by PWM
signals, and indeed LEDs can also be driven by GPIO pins.

Finally, it's accurate to say that a lot of microcontrollers multiplex
GPIO functions onto pins that also provide PWM functionality.

But in my opinion, none of the above means that the PWM API and GPIO API
should be coupled at the interface abstraction.  All it means is that in
some cases, one implementation might be able to "stand in" as an
embodiment of the other's interface.  And we all agree on that for PWM
and GPIO.

The reason I think that PWM merits its own API is because I want the
users of the interface to be able to treat it as a PWM-only metaphor. 
Behind the scenes, we may in fact have a connection with the GPIO
interface--- and in fact we already do with my code.  But PWM generation
can also come from timer/counter peripherals having a compare/match
capability, from true PWM generation hardware (which are in most cases
just three tightly-coupled timer/counters with compare/match hardware),
as well as from standalone chips that are controlled over an I2C or SPI
interface.  The latter example is in fact the initial motivation for the
code, even though at the moment I don't have any patches available to
submit because the devices in question are 8-bit microcontrollers
running custom firmware.

Given a PWM-capable chip that you control over an I2C or SPI interface,
I don't think anyone would argue that such chips should be viewed as an
extension of the SPI or I2C APIs just because the chips use those
communications protocols.  I think it's a similar argument for PWM and GPIO.

Overall, I think your question is a very good one and it's one that I
asked myself early on.  And it's a question that we should ask for EVERY
new interface abstraction that's developed for kernel-facing or
application-facing users.


> What is the reason for bringing in an entirely new framework instead
> of extending the GPIO API or gpiolib?  I'm not too excited about
> having two entirely different frameworks for what basically boils down
> to "numbered signal pins".
>   

I hope my reasoning is clear.  I might not be right, but at least you
know what my thoughts are.  Questions, comments and flames welcome!


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

Grant Likely wrote:
>
> Common code is a big gain in and of itself.

I completely agree!  Which is why I used the GPIO API in my PWM
pseudo-device, along with an hrtimer.

> What I would like to see is the PWM functions added to the GPIO API.
> GPIO drivers can then either implement them or not.  If a GPIO driver
> supports the PWM function, great.  If not, then it returns -EINVAL.
> Heck, I'll even got a driver right now that I'd use it with.  I'm more
> than happy to help code it up even.
>   

I think the appropriate thing to do in such cases is to reimplement the
GPIO driver at its lowest level to provide both GPIO API and PWM API
interfaces.  In my way of thinking, such hardware is a kind of
multi-function device and it should be treated that way.  No need to
expose that multi-functionality to the user unless necessary.

The odd case to me is the concept of "timed GPIO", as in the Android
kernels.  I see the timing capability as a convenience feature that
could be realized by adding timers to the GPIO interface (as they did),
or by having a one-shot PWM channel.  I'm not really comfortable with
either as an interface concept, however, because it just doesn't... feel
right.  :)


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

David Brownell wrote:
>
> It'd be purely for pinmux.
>   

Ugh.  That would be a tough interface to design.  It makes me think of
an old-time telephone switchboard, with an undefined number of wires and
an equally-undefined number of plugs to insert them into.

"Good morning, Mabel!  Give me SCL1 on AA25 please!  No, I don't know
who SCL1 is nor where AA25 is.  I'll be waiting."


:)


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

David Brownell wrote:
> On Friday 13 November 2009, Grant Likely wrote:
>   
>> I'm concerned about the approach taken here.  As I understand it, the
>> PWM signals are very similar to GPIOs in that each PWM device controls
>> an external signal line, just like GPIO lines.
>>     
>
> PWM is not GPIO, and doesn't fit into a GPIO framework.
>
> Since *everything* boils down to one or more signal lines,
> your argument leads directly to Linux having no native
> hardware interface except GPIOs.  Not ... practical. ;)
>   

Wait.  Isn't that what Ubicom's chips do?  :)


> If you want to combine PWM with something else ... timers would
> be a better target.  They're both fundamentally about periodic
> phenomena.  And quite a lot of timers support PWM output modes...
>
> (A generic interface to hardware timers is lacking, too.)
>   

True, and I have code (not yet published) to support a couple of
timer/counter peripherals under the PWM interface.  So for that
functionality at least, I don't think a more generic or standalone API
is necessary.

I don't know how to define a generic interface for the counter a.k.a.
"input capture" behavior of such devices, though.  Still an unsolved
problem, but I don't think it will be a part of the PWM API.  It's a
different metaphor.


> GPIOs, on the other hand, are packaged for manual bit
> twiddling.  While it's possible to create low-speed
> implementations of serial protocols using GPIOs (like
> 2-wire/I2C, one-wire, and various SPI variants), those
> are explicitly the high-overhead (and low performance)
> substitutes, to be used only when native hardware isn't
> available (or is broken etc).
>   

And when using GPIO to generate I2C or SPI signals, you don't do it
through the GPIO interface--- you do it through the I2C or SPI
interface, and a driver behind that API talks to the GPIO API.... or to
a different driver if you change your mind.  Same idea for the PWM API.


-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Tuesday 17 November 2009, Bill Gatliff wrote:
> David Brownell wrote:
> >
> > It'd be purely for pinmux.
> >   
> 
> Ugh.  That would be a tough interface to design.

True.  That's part of why I object to wanting to combine
it with GPIOs ... or, combine everything else with GPIOs
too (like PWMs).

But Grant talks about wanting such things, and if he can
deliver it, more power to him.
 

> It makes me think of 
> an old-time telephone switchboard, with an undefined number of wires and
> an equally-undefined number of plugs to insert them into.
> 
> "Good morning, Mabel!  Give me SCL1 on AA25 please!  No, I don't know
> who SCL1 is nor where AA25 is.  I'll be waiting."

"What?  You're saying that I've got to take three other connections
at the same time?  And cut these other two calls?  No, I only wanted
to make that one call ..."


> 
> 
> :)
> 
> 
> 

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Tue, Nov 17, 2009 at 12:47 AM, David Brownell <david-b@pacbell.net> wrote:
> On Friday 13 November 2009, Grant Likely wrote:
>>                 Right now, I don't
>> see a fundamental difference is between GPIO and PWM pin management.
>> It is essentially the same problem, and in many cases PWM pins can
>> also be used as GPIOs.
>
> Pin management for a given SoC is going to be relevant to setting
> every signal, no matter what peripheral it's associated with.  The
> same argument applies to an MDIO bus, I2C, 1-wire, and more.
>
> And I don't buy it in those cases either.
>
>
>> I think the question should be flipped around;
>> rather than asking for a compelling reason for them to be merged; I
>> want to know the compelling reason to keep them separate.  What is the
>> fundamental difference that keeps them apart?
>
> PWM is about periodic signal generation without CPU intervention.
>
> GPIO is about explicit CPU management/interrogation of single
> signals.

Can also be viewed from the perspective: It is about putting a pin
into a particular state until I explicitly tell you to change it.
Whether that state be a GPIO input, a GPIO high, a GPIO low, or a PWM
periodic.

>> What I would like to see is the PWM functions added to the GPIO API.
>
> No.  If you want a pin mux interface, come up with one of them.
>
> It shouldn't be a PWM interface, a GPIO interface, an I2C interface,
> a SPI interface, an MDIO interface, a 1-wire interface ... or any of
> dozens of other things.  It'd be purely for pinmux.

I'm not talking about a pin mux interface.  I'm talking about discrete
controllable entities.  I agree that pin muxing is an entirely
different scope.  I'm also not talking about layers on top of the
GPIO.  I'm talking about the management code to obtain a reference to
the pin your interested in.  There is a non-trivial amount of code
associated with getting a reference to a pin and the behaviour
required is largely identical between GPIO and PWM.  I don't want to
see a new subsystem that largely does the exact same job, but is
different in subtle ways.  I think it should either be a unified
PWM/GPIO pin management subsystem, or a common library used by each.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Tue, Nov 17, 2009 at 1:27 AM, David Brownell <david-b@pacbell.net> wrote:
> On Friday 13 November 2009, Grant Likely wrote:
>> I'm concerned about the approach taken here.  As I understand it, the
>> PWM signals are very similar to GPIOs in that each PWM device controls
>> an external signal line, just like GPIO lines.
>
> PWM is not GPIO, and doesn't fit into a GPIO framework.
>
> Since *everything* boils down to one or more signal lines,
> your argument leads directly to Linux having no native
> hardware interface except GPIOs.  Not ... practical. ;)

I think you've missed my point and taken it to an illogical extreme to
counter it.  I agree that PWMs are not GPIOs and visa versa.  However,
*some* devices are both GPIOs and PWMs.  Also what is needed to manage
GPIO and PWM pins is pretty much identical.

>> The difference being
>> that PWMs cannot do input, and has additional capabilities (can be
>> programmed with a signal; not just on/off/tristate)
>
> If you want to combine PWM with something else ... timers would
> be a better target.  They're both fundamentally about periodic
> phenomena.  And quite a lot of timers support PWM output modes...
>
> (A generic interface to hardware timers is lacking, too.)
>
>
>> What is the reason for bringing in an entirely new framework instead
>> of extending the GPIO API or gpiolib?  I'm not too excited about
>> having two entirely different frameworks for what basically boils down
>> to "numbered signal pins".
>
> You seem to mis-understand what PWM is all about, then.
> The whole point of a PWM is to set up a periodic activity
> that will run without CPU intervention.

I understand that.

> GPIOs, on the other hand, are packaged for manual bit
> twiddling.  While it's possible to create low-speed
> implementations of serial protocols using GPIOs (like
> 2-wire/I2C, one-wire, and various SPI variants), those
> are explicitly the high-overhead (and low performance)
> substitutes, to be used only when native hardware isn't
> available (or is broken etc).

But that *isn't* the primary purpose of the GPIO subsystem.  All that
stuff is layered on top of the GPIO pin management code and doesn't
really play into this debate.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Tue, Nov 17, 2009 at 8:39 AM, Bill Gatliff <bgat@billgatliff.com> wrote:
> Grant Likely wrote:
>> Hi Bill
>>
>> On Wed, Oct 15, 2008 at 11:14 AM, Bill Gatliff <bgat@billgatliff.com> wrote:
>>
>>> This series implements a Generic PWM Device API, including reference
>>> implementations for the Atmel PWMC device, an LED device, and an LED
>>> trigger.  It is based on linux-2.6.27.
>>>
>> [...]
>>
>>> The implementation of the Generic PWM Device API is structurally
>>> similar to the generic GPIO API, except that the PWM code uses
>>> platform bus_id strings instead of integers to identify target
>>> deviices.  A configuration structure is also provided, both to
>>> facilitate atomic hardware state changes and so that the API can be
>>> extended in a source-code-compatible way to accomodate devices with
>>> features not anticipated by the current code.
>>>
>>
>> Hey Bill,
>>
>> I'm concerned about the approach taken here.  As I understand it, the
>> PWM signals are very similar to GPIOs in that each PWM device controls
>> an external signal line, just like GPIO lines.  The difference being
>> that PWMs cannot do input, and has additional capabilities (can be
>> programmed with a signal; not just on/off/tristate).  Actually, many
>> GPIOs have these properties too.  I've got a part with output-only
>> gpios, and gpio devices that also have a PWM.
>
> It's true that PWM signals can be emitted via GPIO pins, and in fact the
> API code I submitted uses the GPIO API combined with an hrtimer to
> generate a low-speed PWM output.
>
> It's also true that things like LEDs might want to be driven by PWM
> signals, and indeed LEDs can also be driven by GPIO pins.
>
> Finally, it's accurate to say that a lot of microcontrollers multiplex
> GPIO functions onto pins that also provide PWM functionality.
>
> But in my opinion, none of the above means that the PWM API and GPIO API
> should be coupled at the interface abstraction.  All it means is that in
> some cases, one implementation might be able to "stand in" as an
> embodiment of the other's interface.  And we all agree on that for PWM
> and GPIO.

I have no problem with the PWM API.  I think it describes most of the
PWM *usage* requirements well, and I agree that GPIO devices can never
implement the PWM API.

> Given a PWM-capable chip that you control over an I2C or SPI interface,
> I don't think anyone would argue that such chips should be viewed as an
> extension of the SPI or I2C APIs just because the chips use those
> communications protocols.

Nor would I.

> I think it's a similar argument for PWM and GPIO.

I don't think that's true, but I'm approaching it from the viewpoint
of managing pins; not the usage API.  ie. the difference between
gpiolib (management) and the gpio api (usage).

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Tue, Nov 17, 2009 at 9:53 AM, David Brownell <david-b@pacbell.net> wrote:
> On Tuesday 17 November 2009, Bill Gatliff wrote:
>> David Brownell wrote:
>> >
>> > It'd be purely for pinmux.
>> >
>>
>> Ugh.  That would be a tough interface to design.
>
> True.  That's part of why I object to wanting to combine
> it with GPIOs ... or, combine everything else with GPIOs
> too (like PWMs).
>
> But Grant talks about wanting such things, and if he can
> deliver it, more power to him.

Just to be clear, I'm *not* talking about pin mux.  I fully agree that
is a different problem.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

Grant Likely wrote:
> I'm talking about discrete controllable entities.

At the extreme, I see discrete, single-pin GPIO as being a degenerate
case of PWM: only 0% or 100% duty cycle e.g. one-bit granularity, a
single output, and no dependencies with any other channels or pins.  
But the perspectives of the two groups of users are completely
different, so I don't see any advantage to having a combined user-facing
API.


> I'm talking about the management code to obtain a reference to
> the pin your interested in.  There is a non-trivial amount of code
> associated with getting a reference to a pin and the behaviour
> required is largely identical between GPIO and PWM.

True, but anything involving pins has to be dealt with at the platform
level.  And the general approach for that so far is that the board's
startup code sets the pins how it wants them, and the peripheral drivers
more or less assume that things are as they should be when it comes time
to activate the peripherals themselves.  Anything more sophisticated
than that and you prevent the kind of reuse that's taking place now
between AVR32 and AT91, for example.

Maybe that's an opportunity for improvement: an API for pin reservation,
that GPIO, PWM and other platform-oriented drivers could use to request
the pins that they want.  A kind of common version of the OMAP mux
management code, if you will.

But I don't think such an API would be all that useful for GPIO- and
PWM-related scenarios.  Out of necessity, PWM and GPIO are very specific
to the board hardware, and on-the-fly pin configuration changes aren't
possible: either the hardware needs that pin to be an output, or it
doesn't.  You can't make that decision at runtime because you'd probably
have to swap out or add resistors, drivers, etc.  The software to do
*that* would be.... tricky.  :)


> I don't want to see a new subsystem that largely does the exact same job, but is
> different in subtle ways.  I think it should either be a unified
> PWM/GPIO pin management subsystem, or a common library used by each.
>   

Where is there overlap?  My PWM code is totally pin-agnostic, at least
for the drivers I've worked with so far.  And I'm not aware of any GPIO
chip drivers that deal with pin multiplexing, either.  Not saying there
aren't any, only that I haven't seen them.

It's a nice idea in the abstract, but I sure don't see how to make it
work well enough in practice to be worth the effort.  I'm not saying it
can't be done, just that it isn't my focus.  Yours?  :)


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 0/6] Generic PWM API implementation

[Bill Gatliff]

Grant Likely wrote:
>
> But that *isn't* the primary purpose of the GPIO subsystem.  All that
> stuff is layered on top of the GPIO pin management code and doesn't
> really play into this debate.
>   

I don't understand you.  You are saying that the majority of gpiochip
implementations also deal with pin multiplexing?  They must be terribly
broken on the platforms I'm working on, then, because they don't seem to
work at all!  :)


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 0/6] Generic PWM API implementation

[Mark Brown]

On Fri, Nov 20, 2009 at 03:21:31PM -0700, Grant Likely wrote:
> On Tue, Nov 17, 2009 at 1:27 AM, David Brownell <david-b@pacbell.net> wrote:

> > Since *everything* boils down to one or more signal lines,
> > your argument leads directly to Linux having no native
> > hardware interface except GPIOs. ?Not ... practical. ;)

> I think you've missed my point and taken it to an illogical extreme to
> counter it.  I agree that PWMs are not GPIOs and visa versa.  However,
> *some* devices are both GPIOs and PWMs.  Also what is needed to manage
> GPIO and PWM pins is pretty much identical.

On most of the ARM SoCs PWM and GPIO aren't particularly special here -
most of the on-SoC functionality is multiplexed onto pins through the
same hardware interface.  A very large proportion of the pins of the SoC
will have muxes to bring out the signals from the internal IP blocks,
and pretty much all of those will have GPIO as one of those functions.

For many SoCs there will be multiple pin options for bringing out each
of the internal signals which complicates matters further.

> But that *isn't* the primary purpose of the GPIO subsystem.  All that
> stuff is layered on top of the GPIO pin management code and doesn't
> really play into this debate.

The GPIO subsystem isn't doing pin management in that way for most
systems, it's just controlling the GPIO functionality and relies on
separate configuration to ensure that the relevant pins are in GPIO
mode.

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Mon, Nov 23, 2009 at 7:12 AM, Bill Gatliff <bgat@billgatliff.com> wrote:
> Grant Likely wrote:
>> I'm talking about discrete controllable entities.
>
> At the extreme, I see discrete, single-pin GPIO as being a degenerate
> case of PWM: only 0% or 100% duty cycle e.g. one-bit granularity, a
> single output, and no dependencies with any other channels or pins.

yes

> But the perspectives of the two groups of users are completely
> different, so I don't see any advantage to having a combined user-facing
> API.

... for the control API; yes.  ie, the functions that set the GPIO pin
state or the PWM duty cycle.  There is no overlap here (except for the
degenerate case of using a PWM as an output only GPIO... which is
useful.  Board designers do crazy things).

>> I'm talking about the management code to obtain a reference to
>> the pin your interested in.  There is a non-trivial amount of code
>> associated with getting a reference to a pin and the behaviour
>> required is largely identical between GPIO and PWM.
>
> True, but anything involving pins has to be dealt with at the platform
> level.

Not exactly true.  GPIO pins do not have to be dealt with at the
platform level when GPIO lib is used.  I've got several platforms
where the routing of GPIOs is specified in the device tree data, and
platform code never touches them.  When the driver probes a device
implemented via GPIOs, it reads the data and requests the GPIO numbers
that it needs.

*however* I do agree that it is the responsibility of platform code to
set up chip-internal pin muxing and routing.  Actually, further than
that, I think it is actually firmware's responsibility to set up chip
internal pin muxing because it leads to more common platform code in
the kernel (less board specific fixups), but the kernel can fix it up
in a pinch.  But I'm not arguing about the pin (hardware) setup code.

>  And the general approach for that so far is that the board's
> startup code sets the pins how it wants them, and the peripheral drivers
> more or less assume that things are as they should be when it comes time
> to activate the peripherals themselves.  Anything more sophisticated
> than that and you prevent the kind of reuse that's taking place now
> between AVR32 and AT91, for example.
>
> Maybe that's an opportunity for improvement: an API for pin reservation,
> that GPIO, PWM and other platform-oriented drivers could use to request
> the pins that they want.  A kind of common version of the OMAP mux
> management code, if you will.

Perhaps.  But that's not what I'm taking issue with.

> But I don't think such an API would be all that useful for GPIO- and
> PWM-related scenarios.  Out of necessity, PWM and GPIO are very specific
> to the board hardware, and on-the-fly pin configuration changes aren't
> possible: either the hardware needs that pin to be an output, or it
> doesn't.  You can't make that decision at runtime because you'd probably
> have to swap out or add resistors, drivers, etc.  The software to do
> *that* would be.... tricky.  :)
>
>
>> I don't want to see a new subsystem that largely does the exact same job, but is
>> different in subtle ways.  I think it should either be a unified
>> PWM/GPIO pin management subsystem, or a common library used by each.
>
> Where is there overlap?  My PWM code is totally pin-agnostic, at least
> for the drivers I've worked with so far.  And I'm not aware of any GPIO
> chip drivers that deal with pin multiplexing, either.  Not saying there
> aren't any, only that I haven't seen them.

The overlap is in the management of registered pins.  I do not take
issue with the API.  In fact, I don't take issue with the code that
you've written either.  It appears to be well written.  I take issue
with all the common code behind the API to make it work and to allow
GPIOs or PWMs to be registered at runtime.  The overlap is the code
and behaviour used to register pins and to obtain a reference to a
pin.

On the PWM side; it's the code backing pwm_register().
pwm_unregister(), pwm_request(), pwm_free(), and the sysfs hooks.

For GPIO; it's gpiochip_add(), gpiochip_remove(), gpio_request(),
gpio_free(), the sysfs hooks, and the device tree bindings.

They perform exactly the same task.  The difference is that PWM
devices implement the PWM API and GPIO devices implement the GPIO API;
but the behaviour needed to manage them is identical.  I don't like
the fact that it will be implemented twice with subtle differences
between them.

The argument has been made that the split is appropriate because the
use case between GPIO and PWM is considerably different, and to a
point I agree.  The PWM use case definitely requires a different API.
However, the argument also makes the assumption that what is a GPIO
and what is a PWM can easily be pinned down.  For example, I've got
output only GPIOs, input only GPIOs, and a device that implements both
PWM and GPIO behaviour (not pin muxing).  And, as you say, the
degenerate of PWM behaviour is analogous to GPIO output.  The range of
behaviour supported by a PWM or GPIO device is entirely dependent on
what the hardware designer chooses to implement, and there isn't a
canonical definition of either GPIO or PWM.  If a hard distinction is
made now to manage GPIO and PWM pins separately, what then do we do
with other similar-yet-different pin controllers?  What about a
multi-voltage output GPIO pin?  Or a Frequency generator pin?  Is it
appropriate to create a new subsystem for managing each one?  And then
have devices that implement more than one common-case api to register
against each subsystem?

What an API can do is capture the common use cases, but it can never
capture the full range of behaviour implemented by a particular
device.  What I see as important to driver writers (PWM/GPIO users) is
the ability to obtain and use a reference to a controllable pin.  Then
it can either use a common-case API, or if it needs to, call device
specific extra hooks to do something funky, like hardware triggering.

For pin controller driver writers, the need is to export as much
behaviour as possible via a common-case API so that the device can
easily be reused by things the driver author hasn't even conceived of.
 Some behaviors won't fit, and some devices don't support behaviour
exposed by the API.  It is perfectly valid for a pin controller driver
to only implement the ops supported by the device and use -EINVAL if
an unsupported op is attempted.

I say that it is better to have a single pin-controller subsystem (and
again, not talking about pin-mux, that is something different) for
registering pins to, and leave it up to the drivers to choose which
ops to implement.  It can implement just the PWM ops, or just the GPIO
ops, or both.  A pin user will use the same mechanism to obtain a
reference to the pin regardless of the pin type, and can use that
reference to call the common case apis (gpio_get/set_value(),
pwm_config(), etc), or can use the reference when calling into a
driver-specific hook.  When new capabilities are identified, the list
of implementable ops can be extended without impact on existing
drivers.  Drivers can take advantage of the new behaviour by
implementing the new ops.

> It's a nice idea in the abstract, but I sure don't see how to make it
> work well enough in practice to be worth the effort.  I'm not saying it
> can't be done, just that it isn't my focus.  Yours?  :)

The problem that I have is that it shifts the maintenance burden from
the time of development (ie. now) to code maintenance time.  By
duplicating the infrastructure it doubles the code to be maintained.
It means there are differences between how a GPIO/PWM controllers
register their pins, and differences in how other drivers obtain and
use them.  It also establishes a pattern of writing new subsystems to
handle similar-yet-different pin controllers.  To me that is a
non-trivial cost.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Mon, Nov 23, 2009 at 7:13 AM, Bill Gatliff <bgat@billgatliff.com> wrote:
> Grant Likely wrote:
>>
>> But that *isn't* the primary purpose of the GPIO subsystem.  All that
>> stuff is layered on top of the GPIO pin management code and doesn't
>> really play into this debate.
>>
>
> I don't understand you.  You are saying that the majority of gpiochip
> implementations also deal with pin multiplexing?  They must be terribly
> broken on the platforms I'm working on, then, because they don't seem to
> work at all!  :)

Nope.  pin mux is a different problem.  I'm talking about the code to
register pin controller drivers and for users to obtain a reference to
a pin.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Grant Likely]

On Mon, Nov 23, 2009 at 8:29 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Nov 20, 2009 at 03:21:31PM -0700, Grant Likely wrote:
>> On Tue, Nov 17, 2009 at 1:27 AM, David Brownell <david-b@pacbell.net> wrote:
>
>> > Since *everything* boils down to one or more signal lines,
>> > your argument leads directly to Linux having no native
>> > hardware interface except GPIOs. ?Not ... practical. ;)
>
>> I think you've missed my point and taken it to an illogical extreme to
>> counter it.  I agree that PWMs are not GPIOs and visa versa.  However,
>> *some* devices are both GPIOs and PWMs.  Also what is needed to manage
>> GPIO and PWM pins is pretty much identical.
>
> On most of the ARM SoCs PWM and GPIO aren't particularly special here -
> most of the on-SoC functionality is multiplexed onto pins through the
> same hardware interface.  A very large proportion of the pins of the SoC
> will have muxes to bring out the signals from the internal IP blocks,
> and pretty much all of those will have GPIO as one of those functions.

Right, pin-mux is a different problem.  But there are also devices
that implement both PWM and GPIO functionality in the same IP block.
I think pin muxing, and pin controller drivers are different problem
domains and should be handled separately.

>> But that *isn't* the primary purpose of the GPIO subsystem.  All that
>> stuff is layered on top of the GPIO pin management code and doesn't
>> really play into this debate.
>
> The GPIO subsystem isn't doing pin management in that way for most
> systems, it's just controlling the GPIO functionality and relies on
> separate configuration to ensure that the relevant pins are in GPIO
> mode.

Sorry. when I said pin management I meant how Linux keeps track of pin
controllers.  Not pin mux.  I should use different terminology perhaps
to reduce confusion.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/6] Generic PWM API implementation

[Mark Brown]

On Mon, Nov 23, 2009 at 10:44:25AM -0700, Grant Likely wrote:

> Right, pin-mux is a different problem.  But there are also devices
> that implement both PWM and GPIO functionality in the same IP block.

That's not the general case, though - most of the SoCs seem to have PWM
as a separate IP block.  In the general case PWM and GPIO have nothing
to do with each other.  

> I think pin muxing, and pin controller drivers are different problem
> domains and should be handled separately.

...

> Sorry. when I said pin management I meant how Linux keeps track of pin
> controllers.  Not pin mux.  I should use different terminology perhaps
> to reduce confusion.

I have to confess I'm a bit lost as to what you mean by a "pin
controller" as opposed to "pin mux" interface.  For a substantial
proportion of ARMs they're going to be one and the same.

Judging from some of the other messages in the thread I suspect you're
thinking of a much closer mapping between PWM and GPIO pins - many SoCs
do have distinct PWM controllers that aren't terribly tied to a GPIO
pin.  For them the whole concept of requesting a "pin" or having the PWM
controller be tied to a particular pin is going to be at best confusing,
you really do want to request the PWM controller itself and let the pin
mux setup figure out where that emerges from the SoC.

Re: [PATCH 0/6] Generic PWM API implementation

[Albrecht Dreß]

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

Am 23.11.09 18:39 schrieb(en) Grant Likely:
> There is no overlap here (except for the degenerate case of using a PWM as an output only GPIO... which is useful.  Board designers do crazy things).

But there may be cases where a device pin is used as GPIO *and* as pwm output.  Think of a relay output driver which may be used a state GPIO to indicate that the device is up, or as a pulsed heartbeat output (some safety schemes require that) for the  
same purpose, selected by a run-time (not device tree!) config option.

I actually do exactly this with one of the Freescale 5200's pins, and would therefore also appreciate a "unified" approach.

Cheers, Albrecht.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Friday 20 November 2009, Grant Likely wrote:
> I don't think that's true, but I'm approaching it from the viewpoint
> of managing pins; not the usage API.  ie. the difference between
> gpiolib (management) and the gpio api (usage).

Don't follow.  The gpiolib code is unrelated to pin managment.
It's purely an aid for implementations of the API.

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Monday 23 November 2009, Grant Likely wrote: 
> *however* I do agree that it is the responsibility of platform code to
> set up chip-internal pin muxing and routing.

Fo over 95% of systems, I'd agree -- given that "platform" code
includes the arch/.../mach-X/board-Y.c files.  It's not realistic
to expect boot loaders to always handle that stuff.  If for no
other reason than the way they're produced:  get something that
will boot <OS> and call it done.  There will be nuances that need
to be corrected later.


> Actually, further than 
> that, I think it is actually firmware's responsibility to set up chip
> internal pin muxing because it leads to more common platform code in
> the kernel (less board specific fixups), but the kernel can fix it up
> in a pinch.

That something-less-than-5% remaining includes a lot of developer
boards, where there are multiple viable configurations.  The OS
needs to know which config it's going into at boot time.

And there's even a crazed subset of that 5% which wants to do
runtime reconfiguration.  Those folk do not accept static board
configs, whether done by a bootloader or anything else.

Some of that subset isn't entirely crazed.  I was reading a
chip errata document not long ago, which pointed out an issue
I've seen before:  suspend/resume cycles needed to reconfigure
things dynamically, to prevent leakage.  That's board-specific
and non-static.


>  But I'm not arguing about the pin (hardware) setup code.

That's good, since I don't think there's a Grand Scheme that
can be agreed to in that space, either for boot time setup
or runtime reconfiguration.  The hardware varies too much.

- Dave

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Monday 23 November 2009, Mark Brown wrote:
> Judging from some of the other messages in the thread I suspect you're
> thinking of a much closer mapping between PWM and GPIO pins - many SoCs
> do have distinct PWM controllers that aren't terribly tied to a GPIO
> pin. 

Sometimes they can be coupled to one of several pins ... possibly
even outputting to several at the same time.


> For them the whole concept of requesting a "pin" or having the PWM 
> controller be tied to a particular pin is going to be at best confusing,
> you really do want to request the PWM controller itself and let the pin
> mux setup figure out where that emerges from the SoC.

Nicely put.  At some level one ends with something like "pwm.2 channel 3"
getting allocated, and does not care about pins except as more low-level
artifacts to be ignored outside of board setup code.

- Dave

Re: [PATCH 0/6] Generic PWM API implementation

[David Brownell]

On Monday 23 November 2009, Grant Likely wrote:
> 			I take issue
> with all the common code behind the API to make it work and to allow
> GPIOs or PWMs to be registered at runtime.  The overlap is the code
> and behaviour used to register pins and to obtain a reference to a
> pin.
> 
> On the PWM side; it's the code backing pwm_register().
> pwm_unregister(), pwm_request(), pwm_free(), and the sysfs hooks.
> 
> For GPIO; it's gpiochip_add(), gpiochip_remove(), gpio_request(),
> gpio_free(), the sysfs hooks, and the device tree bindings.
> 
> They perform exactly the same task.  The difference is that PWM
> devices implement the PWM API and GPIO devices implement the GPIO API;
> but the behaviour needed to manage them is identical.  I don't like
> the fact that it will be implemented twice with subtle differences
> between them.

Most of that stuff sits inside sysfs.  What's actually sharable?


> The argument has been made that the split is appropriate because the
> use case between GPIO and PWM is considerably different, and to a
> point I agree.  The PWM use case definitely requires a different API.
> However, the argument also makes the assumption that what is a GPIO
> and what is a PWM can easily be pinned down.  For example, I've got
> output only GPIOs, input only GPIOs, and a device that implements both
> PWM and GPIO behaviour (not pin muxing).

In object oriented programming there's this thing called an "interface",
and its application here is:  you write a driver implementing both
the "gpio" and "pwm" interfaces.  Someone wanting a given behavior
goes through that interface.


> If a hard distinction is 
> made now to manage GPIO and PWM pins separately, what then do we do
> with other similar-yet-different pin controllers?  What about a
> multi-voltage output GPIO pin?  Or a Frequency generator pin?  Is it
> appropriate to create a new subsystem for managing each one?

None of those are particularly common.  The Linux approach is to come
up with interfaces as needed ... then generalize once there's enough
data, if indeed there is enough.  Just because you *can* come up with
hardware doesn't mean it's widespread enough to need a shared API.


> What an API can do is capture the common use cases, but it can never
> capture the full range of behaviour implemented by a particular
> device.

Right, so you need to become accustomed to the notion that there
can be platform-specific capabilities.  Kitchen-sink APIs are bad.