* [PWM v3: 0/3] Proposal for a generic PWM API
@ 2011-02-10 15:53 Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 1/3] PWM: Implement a generic PWM framework Bill Gatliff
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Bill Gatliff @ 2011-02-10 15:53 UTC (permalink / raw)
To: linux-embedded; +Cc: Bill Gatliff
This patch series contains the third attempt at implementation of
a generic PWM device interface framework. Think gpiolib, but for
devices and pseudo-devices that generate pulse-wave-modulated outputs.
Functionally-speaking, this series has regressed somewhat from
previous versions because I am currently focusing my attention on the
API itself. I include only implementations for GPIO+hrtimer devices
and the Atmel PWMC peripheral as references in this series; I will
post patches for LED drivers, PXA, Samsung, etc. devices once I know
that the API itself is on its way to mainline. (I believe that the
two reference implementations sufficiently confirm the utility of the
API itself).
The code in this series is significantly clearer and more
straightforward than previous versions. Thanks to everyone who helped
me with this refactoring! I'm pretty convinced that the code you see
here is at last suitable for pulling into mainline.
Finally, the attached code CAN be used to control devices that drive
stepper motors and the like, but doing so is discouraged as I am
anticipating a request to develop an API specifically for such
situations.
Regards,
b.g.
Bill Gatliff (3):
PWM: Implement a generic PWM framework
PWM: GPIO+hrtimer device emulation
PWM: Atmel PWMC driver
Documentation/pwm.txt | 259 +++++++++++++++++++
drivers/Kconfig | 2 +
drivers/Makefile | 2 +
drivers/pwm/Kconfig | 28 ++
drivers/pwm/Makefile | 7 +
drivers/pwm/atmel-pwmc.c | 520 ++++++++++++++++++++++++++++++++++++++
drivers/pwm/gpio-pwm.c | 354 ++++++++++++++++++++++++++
drivers/pwm/pwm.c | 619 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pwm/pwm.h | 163 ++++++++++++
9 files changed, 1954 insertions(+), 0 deletions(-)
create mode 100644 Documentation/pwm.txt
create mode 100644 drivers/pwm/Kconfig
create mode 100644 drivers/pwm/Makefile
create mode 100644 drivers/pwm/atmel-pwmc.c
create mode 100644 drivers/pwm/gpio-pwm.c
create mode 100644 drivers/pwm/pwm.c
create mode 100644 include/linux/pwm/pwm.h
--
1.7.2.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-10 15:53 [PWM v3: 0/3] Proposal for a generic PWM API Bill Gatliff
@ 2011-02-10 15:53 ` Bill Gatliff
2011-02-10 20:39 ` Sascha Hauer
2011-02-10 15:53 ` [PWM v3: 2/3] PWM: GPIO+hrtimer device emulation Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 3/3] PWM: Atmel PWMC driver Bill Gatliff
2 siblings, 1 reply; 11+ messages in thread
From: Bill Gatliff @ 2011-02-10 15:53 UTC (permalink / raw)
To: linux-embedded; +Cc: Bill Gatliff
Updates the existing PWM-related functions to support multiple
and/or hotplugged PWM devices, and adds a sysfs interface.
Moves the code to drivers/pwm.
For now, this new code can exist alongside the current PWM
implementations; the existing implementations will be migrated
to this new framework as time permits. Eventually, the current
PWM implementation will be deprecated and then expunged.
Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
Documentation/pwm.txt | 277 +++++++++++++++++++++
drivers/Kconfig | 2 +
drivers/Makefile | 2 +
drivers/pwm/Kconfig | 11 +
drivers/pwm/Makefile | 4 +
drivers/pwm/pwm.c | 619 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pwm/pwm.h | 163 +++++++++++++
7 files changed, 1078 insertions(+), 0 deletions(-)
create mode 100644 Documentation/pwm.txt
create mode 100644 drivers/pwm/Kconfig
create mode 100644 drivers/pwm/Makefile
create mode 100644 drivers/pwm/pwm.c
create mode 100644 include/linux/pwm/pwm.h
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
new file mode 100644
index 0000000..2b15395
--- /dev/null
+++ b/Documentation/pwm.txt
@@ -0,0 +1,277 @@
+ Generic PWM Device API
+
+ February 7, 2011
+ Bill Gatliff
+ <bgat@billgatliff.com>
+
+
+
+The code in drivers/pwm and include/linux/pwm/ 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 suddenly 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 accommodate 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, sensors and solenoids through a
+custom-designed, optically-isolated H-bridge driver.
+
+
+
+Functional Overview
+
+The Generic PWM Device API framework is implemented in
+include/linux/pwm/pwm.h and drivers/pwm/pwm.c. The functions therein
+use information from pwm_device and pwm__config structures to invoke
+services in PWM peripheral device drivers. Consult
+drivers/pwm/atmel-pwmc.c for an example driver for the Atmel PWMC
+peripheral.
+
+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 Generic PWM API framework also provides a sysfs interface to PWM
+devices, which is adequate for basic application needs and testing.
+
+Driver authors fill out a pwm_device structure, which describes the
+capabilities of the PWM hardware being utilized. 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 as users begin to
+configure and utilize the hardware.
+
+Many PWM-capable peripherals provide two, three, or more channels of
+PWM output. The driver author completes and registers a pwm_device
+structure for each channel they wish to be supported by the Generic
+PWM API.
+
+Note that PWM signals can be produced by a variety of peripherals,
+beyond the true PWM peripherals 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 be accommodated by the Generic PWM Device API framework.
+
+
+
+Using the API to Generate PWM Signals -- Basic Functions for Users
+
+
+pwm_request() -- Returns a pwm_device 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_release() 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_release() -- 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_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 by the config structure.
+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_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, timer
+handlers, and other interrupt contexts, but must confine their
+configuration changes to only those that the driver can implement
+without sleeping. 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. By default, all PWM
+channels are unsynchronized.
+
+
+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 plain-text 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 Atmel PWMC
+platform 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 dev_name(pdev->dev) in your
+probe() method).
+
+
+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) Invoked 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.
+
+
+set_callback -- (optional) If the hardware supports an end-of-period
+interrupt, invoke the function provided in this callback during the
+device's 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-compatible 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.
+
+
+
+FAQs and Additional Notes
+
+The Atmel PWMC pwm_config() function tries to satisfy the user's
+configuration request by first invoking pwm_config_nosleep(). If that
+operation fails, then the PWM peripheral is brought to a synchronized
+stop, the configuration changes are made, and the device is restarted.
+
+The Atmel PWMC's use of pwm_config_nosleep() from pwm_config()
+minimizes redundant code between the two functions, and relieves the
+pwm_config() function of the need to explicitly test whether a
+requested configuration change can be carried out while the PWM device
+is in its current mode.
+
+PWM API driver authors are encouraged to adopt the Atmel PWMC's
+pwm_config()-vs.-pwm_config_nosleep() strategy in implementations for
+other devices as well.
+
+
+
+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. ;)
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 9bfb71f..413e4f9 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -56,6 +56,8 @@ source "drivers/pps/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 b423bb1..4e37abf 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/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 0000000..153d455
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,11 @@
+#
+# PWM infrastructure and devices
+#
+
+menuconfig GENERIC_PWM
+ tristate "PWM Support"
+ default n
+ help
+ Enables PWM device support implemented via a generic
+ framework. If unsure, say N.
+
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 0000000..7baa201
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1,4 @@
+#
+# Makefile for pwm devices
+#
+obj-$(CONFIG_GENERIC_PWM) := pwm.o
diff --git a/drivers/pwm/pwm.c b/drivers/pwm/pwm.c
new file mode 100644
index 0000000..2cad7cd
--- /dev/null
+++ b/drivers/pwm/pwm.c
@@ -0,0 +1,619 @@
+/*
+ * PWM API implementation
+ *
+ * Copyright (C) 2011 Bill Gatliff <bgat@billgatliff.com>
+ * Copyright (C) 2011 Arun Murthy <arun.murthy@stericsson.com>
+ *
+ * This program is free software; you may redistribute and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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/kernel.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/completion.h>
+#include <linux/workqueue.h>
+#include <linux/list.h>
+#include <linux/sched.h>
+#include <linux/platform_device.h>
+#include <linux/pwm/pwm.h>
+
+static const char *REQUEST_SYSFS = "sysfs";
+static LIST_HEAD(pwm_device_list);
+static DEFINE_MUTEX(device_list_mutex);
+static struct class pwm_class;
+static struct workqueue_struct *pwm_handler_workqueue;
+
+static int pwm_match_name(struct device *dev, void *name)
+{
+ return !strcmp(name, dev_name(dev));
+}
+
+static struct pwm_device *__pwm_request(struct pwm_device *p, const char *label)
+{
+ int ret;
+
+ ret = test_and_set_bit(FLAG_REQUESTED, &p->flags);
+ if (ret) {
+ p = ERR_PTR(-EBUSY);
+ goto done;
+ }
+
+ p->label = label;
+ p->pid = current->pid;
+
+ if (p->ops->request) {
+ ret = p->ops->request(p);
+ if (ret) {
+ p = ERR_PTR(ret);
+ clear_bit(FLAG_REQUESTED, &p->flags);
+ goto done;
+ }
+ }
+
+done:
+ return p;
+}
+
+static struct pwm_device *__pwm_request_byname(const char *name,
+ const char *label)
+{
+ struct device *d;
+ struct pwm_device *p;
+
+ d = class_find_device(&pwm_class, NULL, (char*)name, pwm_match_name);
+ if (!d) {
+ p = ERR_PTR(-EINVAL);
+ goto done;
+ }
+ if (IS_ERR(d)) {
+ p = (struct pwm_device *)d;
+ goto done;
+ }
+
+ p = __pwm_request(dev_get_drvdata(d), label);
+
+done:
+ return p;
+}
+
+struct pwm_device *pwm_request_byname(const char *name, const char *label)
+{
+ struct pwm_device *p;
+
+ mutex_lock(&device_list_mutex);
+ p = __pwm_request_byname(name, label);
+ mutex_unlock(&device_list_mutex);
+ return p;
+}
+EXPORT_SYMBOL(pwm_request_byname);
+
+struct pwm_device *pwm_request(const char *bus_id, int id, const char *label)
+{
+ char name[256];
+ int ret;
+
+ if (id == -1)
+ ret = scnprintf(name, sizeof name, "%s", bus_id);
+ else
+ ret = scnprintf(name, sizeof name, "%s:%d", bus_id, id);
+ if (ret <= 0 || ret >= sizeof name)
+ return ERR_PTR(-EINVAL);
+
+ return pwm_request_byname(name, label);
+}
+EXPORT_SYMBOL(pwm_request);
+
+void pwm_release(struct pwm_device *p)
+{
+ mutex_lock(&device_list_mutex);
+
+ if (!test_and_clear_bit(FLAG_REQUESTED, &p->flags)) {
+ BUG();
+ goto done;
+ }
+
+ pwm_stop(p);
+ pwm_unsynchronize(p, NULL);
+ pwm_set_handler(p, NULL, NULL);
+
+ p->label = NULL;
+ p->pid = -1;
+
+ if (p->ops->release)
+ p->ops->release(p);
+done:
+ mutex_unlock(&device_list_mutex);
+}
+EXPORT_SYMBOL(pwm_release);
+
+unsigned long pwm_ns_to_ticks(struct pwm_device *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_device *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_device *p, struct pwm_config *c)
+{
+ if (test_bit(PWM_CONFIG_PERIOD_NS, &c->config_mask)) {
+ c->period_ticks = pwm_ns_to_ticks(p, c->period_ns);
+ clear_bit(PWM_CONFIG_PERIOD_NS, &c->config_mask);
+ set_bit(PWM_CONFIG_PERIOD_TICKS, &c->config_mask);
+ }
+
+ if (test_bit(PWM_CONFIG_DUTY_NS, &c->config_mask)) {
+ c->duty_ticks = pwm_ns_to_ticks(p, c->duty_ns);
+ clear_bit(PWM_CONFIG_DUTY_NS, &c->config_mask);
+ set_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask);
+ }
+}
+
+static void pwm_config_percent_to_ticks(struct pwm_device *p,
+ struct pwm_config *c)
+{
+ if (test_bit(PWM_CONFIG_DUTY_PERCENT, &c->config_mask)) {
+ if (test_bit(PWM_CONFIG_PERIOD_TICKS, &c->config_mask))
+ c->duty_ticks = c->period_ticks;
+ else
+ c->duty_ticks = p->period_ticks;
+
+ c->duty_ticks *= c->duty_percent;
+ c->duty_ticks /= 100;
+ clear_bit(PWM_CONFIG_DUTY_PERCENT, &c->config_mask);
+ set_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask);
+ }
+}
+
+int pwm_config_nosleep(struct pwm_device *p, struct pwm_config *c)
+{
+ if (!p->ops->config_nosleep)
+ return -EINVAL;
+
+ pwm_config_ns_to_ticks(p, c);
+ pwm_config_percent_to_ticks(p, c);
+
+ return p->ops->config_nosleep(p, c);
+}
+EXPORT_SYMBOL(pwm_config_nosleep);
+
+int pwm_config(struct pwm_device *p, struct pwm_config *c)
+{
+ int ret = 0;
+
+ pwm_config_ns_to_ticks(p, c);
+ pwm_config_percent_to_ticks(p, c);
+
+ switch (c->config_mask & (BIT(PWM_CONFIG_PERIOD_TICKS)
+ | BIT(PWM_CONFIG_DUTY_TICKS))) {
+ case BIT(PWM_CONFIG_PERIOD_TICKS):
+ if (p->duty_ticks > c->period_ticks) {
+ ret = -EINVAL;
+ goto err;
+ }
+ break;
+ case BIT(PWM_CONFIG_DUTY_TICKS):
+ if (p->period_ticks < c->duty_ticks) {
+ ret = -EINVAL;
+ goto err;
+ }
+ break;
+ case BIT(PWM_CONFIG_DUTY_TICKS) | BIT(PWM_CONFIG_PERIOD_TICKS):
+ if (c->duty_ticks > c->period_ticks) {
+ ret = -EINVAL;
+ goto err;
+ }
+ break;
+ default:
+ break;
+ }
+
+err:
+ dev_dbg(p->dev, "%s: config_mask %lu 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->ops->config(p, c);
+}
+EXPORT_SYMBOL(pwm_config);
+
+int pwm_set_period_ns(struct pwm_device *p, unsigned long period_ns)
+{
+ struct pwm_config c = {
+ .config_mask = BIT(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_device *p)
+{
+ return pwm_ticks_to_ns(p, p->period_ticks);
+}
+EXPORT_SYMBOL(pwm_get_period_ns);
+
+int pwm_set_duty_ns(struct pwm_device *p, unsigned long duty_ns)
+{
+ struct pwm_config c = {
+ .config_mask = BIT(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_device *p)
+{
+ return pwm_ticks_to_ns(p, p->duty_ticks);
+}
+EXPORT_SYMBOL(pwm_get_duty_ns);
+
+int pwm_set_duty_percent(struct pwm_device *p, int percent)
+{
+ struct pwm_config c = {
+ .config_mask = BIT(PWM_CONFIG_DUTY_PERCENT),
+ .duty_percent = percent,
+ };
+ return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_set_duty_percent);
+
+int pwm_set_polarity(struct pwm_device *p, int active_high)
+{
+ struct pwm_config c = {
+ .config_mask = BIT(PWM_CONFIG_POLARITY),
+ .polarity = active_high,
+ };
+ return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_set_polarity);
+
+int pwm_start(struct pwm_device *p)
+{
+ struct pwm_config c = {
+ .config_mask = BIT(PWM_CONFIG_START),
+ };
+ return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_start);
+
+int pwm_stop(struct pwm_device *p)
+{
+ struct pwm_config c = {
+ .config_mask = BIT(PWM_CONFIG_STOP),
+ };
+ return pwm_config(p, &c);
+}
+EXPORT_SYMBOL(pwm_stop);
+
+int pwm_synchronize(struct pwm_device *p, struct pwm_device *to_p)
+{
+ if (!p->ops->synchronize)
+ return -EINVAL;
+
+ return p->ops->synchronize(p, to_p);
+}
+EXPORT_SYMBOL(pwm_synchronize);
+
+int pwm_unsynchronize(struct pwm_device *p, struct pwm_device *from_p)
+{
+ if (!p->ops->unsynchronize)
+ return -EINVAL;
+
+ return p->ops->unsynchronize(p, from_p);
+}
+EXPORT_SYMBOL(pwm_unsynchronize);
+
+static void pwm_handler(struct work_struct *w)
+{
+ struct pwm_device *p = container_of(w, struct pwm_device,
+ handler_work);
+ if (p->handler && p->handler(p, p->handler_data))
+ pwm_stop(p);
+}
+
+static void __pwm_callback(struct pwm_device *p)
+{
+ queue_work(pwm_handler_workqueue, &p->handler_work);
+}
+
+int pwm_set_handler(struct pwm_device *p, pwm_handler_t handler, void *data)
+{
+ if (p->ops->set_callback) {
+ p->handler_data = data;
+ p->handler = handler;
+ INIT_WORK(&p->handler_work, pwm_handler);
+ return p->ops->set_callback(p, handler ? __pwm_callback : NULL);
+ }
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pwm_set_handler);
+
+static ssize_t pwm_run_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pwm_device *p = dev_get_drvdata(dev);
+ return sprintf(buf, "%d\n", pwm_is_running(p));
+}
+
+static ssize_t pwm_run_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct pwm_device *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, S_IRUGO | S_IWUSR, pwm_run_show, pwm_run_store);
+
+static ssize_t pwm_tick_hz_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pwm_device *p = dev_get_drvdata(dev);
+ return sprintf(buf, "%lu\n", p->tick_hz);
+}
+static DEVICE_ATTR(tick_hz, S_IRUGO, pwm_tick_hz_show, NULL);
+
+static ssize_t pwm_duty_ns_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pwm_device *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_device *p = dev_get_drvdata(dev);
+
+ if (!strict_strtoul(buf, 10, &duty_ns))
+ pwm_set_duty_ns(p, duty_ns);
+ return len;
+}
+static DEVICE_ATTR(duty_ns, S_IRUGO | S_IWUSR, 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_device *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_device *p = dev_get_drvdata(dev);
+
+ if (!strict_strtoul(buf, 10, &period_ns))
+ pwm_set_period_ns(p, period_ns);
+ return len;
+}
+static DEVICE_ATTR(period_ns, S_IRUGO | S_IWUSR, 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_device *p = dev_get_drvdata(dev);
+ return sprintf(buf, "%d\n", p->active_high ? 1 : 0);
+}
+
+static ssize_t pwm_polarity_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ unsigned long polarity;
+ struct pwm_device *p = dev_get_drvdata(dev);
+
+ if (!strict_strtoul(buf, 10, &polarity))
+ pwm_set_polarity(p, polarity);
+ return len;
+}
+static DEVICE_ATTR(polarity, S_IRUGO | S_IWUSR, pwm_polarity_show, pwm_polarity_store);
+
+static ssize_t pwm_request_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pwm_device *p = dev_get_drvdata(dev);
+ struct pwm_device *ret;
+
+ mutex_lock(&device_list_mutex);
+ ret = __pwm_request(p, REQUEST_SYSFS);
+ mutex_unlock(&device_list_mutex);
+
+ if (IS_ERR_OR_NULL(ret))
+ return sprintf(buf, "fail (owner: %s pid: %d)\n",
+ p->label, p->pid);
+ else
+ return sprintf(buf, "%s (pid %d)\n", ret->label, ret->pid);
+}
+
+static ssize_t pwm_request_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct pwm_device *p = dev_get_drvdata(dev);
+
+ pwm_release(p);
+ return len;
+}
+static DEVICE_ATTR(request, S_IRUGO | S_IWUSR, pwm_request_show, pwm_request_store);
+
+static const struct attribute *pwm_attrs[] =
+{
+ &dev_attr_tick_hz.attr,
+ &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 struct class_attribute pwm_class_attrs[] = {
+ __ATTR_NULL,
+};
+
+static struct class pwm_class = {
+ .name = "pwm",
+ .owner = THIS_MODULE,
+
+ .class_attrs = pwm_class_attrs,
+};
+
+int pwm_register_byname(struct pwm_device *p, struct device *parent,
+ const char *name)
+{
+ struct device *d;
+ int ret;
+
+ if (!p->ops || !p->ops->config)
+ return -EINVAL;
+
+ mutex_lock(&device_list_mutex);
+
+ d = class_find_device(&pwm_class, NULL, (char*)name, pwm_match_name);
+ if (d) {
+ ret = -EEXIST;
+ goto err_found_device;
+ }
+
+ p->dev = device_create(&pwm_class, parent, MKDEV(0, 0), NULL, name);
+ if (IS_ERR(p->dev)) {
+ ret = PTR_ERR(p->dev);
+ goto err_device_create;
+ }
+
+ ret = sysfs_create_group(&p->dev->kobj, &pwm_device_attr_group);
+ if (ret)
+ goto err_create_group;
+
+ dev_set_drvdata(p->dev, p);
+ p->flags = BIT(FLAG_REGISTERED);
+
+ goto done;
+
+err_create_group:
+ device_unregister(p->dev);
+ p->flags = 0;
+
+err_device_create:
+err_found_device:
+done:
+ mutex_unlock(&device_list_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL(pwm_register_byname);
+
+int pwm_register(struct pwm_device *p, struct device *parent, int id)
+{
+ int ret;
+ char name[256];
+
+ if (IS_ERR_OR_NULL(parent))
+ return -EINVAL;
+
+ if (id == -1)
+ ret = scnprintf(name, sizeof name, "%s", dev_name(parent));
+ else
+ ret = scnprintf(name, sizeof name, "%s:%d", dev_name(parent), id);
+ if (ret <= 0 || ret >= sizeof name)
+ return -EINVAL;
+
+ return pwm_register_byname(p, parent, name);
+}
+EXPORT_SYMBOL(pwm_register);
+
+int pwm_unregister(struct pwm_device *p)
+{
+ int ret = 0;
+
+ mutex_lock(&device_list_mutex);
+
+ if (pwm_is_running(p) || pwm_is_requested(p)) {
+ ret = -EBUSY;
+ goto done;
+ }
+
+ sysfs_remove_group(&p->dev->kobj, &pwm_device_attr_group);
+ device_unregister(p->dev);
+ p->flags = 0;
+
+done:
+ mutex_unlock(&device_list_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL(pwm_unregister);
+
+static int __init pwm_init(void)
+{
+ return class_register(&pwm_class);
+}
+
+static void __exit pwm_exit(void)
+{
+ class_unregister(&pwm_class);
+}
+
+#ifdef MODULE
+module_init(pwm_init);
+module_exit(pwm_exit);
+MODULE_LICENSE("GPL");
+#else
+postcore_initcall(pwm_init);
+#endif
diff --git a/include/linux/pwm/pwm.h b/include/linux/pwm/pwm.h
new file mode 100644
index 0000000..871e38c
--- /dev/null
+++ b/include/linux/pwm/pwm.h
@@ -0,0 +1,163 @@
+/*
+ * Copyright (C) 2011 Bill Gatliff < bgat@billgatliff.com>
+ * Copyright (C) 2011 Arun Murthy <arun.murth@stericsson.com>
+ *
+ * This program is free software; you may redistribute and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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
+ */
+#ifndef __LINUX_PWM_H
+#define __LINUX_PWM_H
+
+enum {
+ FLAG_REGISTERED = 0,
+ FLAG_REQUESTED = 1,
+ FLAG_STOP = 2,
+ FLAG_RUNNING = 3,
+};
+
+enum {
+ PWM_CONFIG_DUTY_TICKS = 0,
+ PWM_CONFIG_PERIOD_TICKS = 1,
+ PWM_CONFIG_POLARITY = 2,
+ PWM_CONFIG_START = 3,
+ PWM_CONFIG_STOP = 4,
+
+ PWM_CONFIG_HANDLER = 5,
+
+ PWM_CONFIG_DUTY_NS = 6,
+ PWM_CONFIG_DUTY_PERCENT = 7,
+ PWM_CONFIG_PERIOD_NS = 8,
+};
+
+struct pwm_config;
+struct pwm_device;
+
+typedef int (*pwm_handler_t)(struct pwm_device *p, void *data);
+typedef void (*pwm_callback_t)(struct pwm_device *p);
+
+struct pwm_device_ops {
+ int (*request) (struct pwm_device *p);
+ void (*release) (struct pwm_device *p);
+ int (*config) (struct pwm_device *p,
+ struct pwm_config *c);
+ int (*config_nosleep) (struct pwm_device *p,
+ struct pwm_config *c);
+ int (*synchronize) (struct pwm_device *p,
+ struct pwm_device *to_p);
+ int (*unsynchronize) (struct pwm_device *p,
+ struct pwm_device *from_p);
+ int (*set_callback) (struct pwm_device *p,
+ pwm_callback_t callback);
+};
+
+struct pwm_config {
+ unsigned long 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;
+
+ struct device *dev;
+ struct pwm_device_ops *ops;
+
+ void *data;
+
+ const char *label;
+ pid_t pid;
+
+ volatile unsigned long flags;
+
+ unsigned long tick_hz;
+
+ pwm_callback_t callback;
+
+ struct work_struct handler_work;
+ pwm_handler_t handler;
+ void *handler_data;
+
+ int active_high;
+ unsigned long period_ticks;
+ unsigned long duty_ticks;
+};
+
+struct pwm_device *pwm_request_byname(const char *name, const char *label);
+struct pwm_device *pwm_request(const char *bus_id, int id, const char *label);
+void pwm_release(struct pwm_device *p);
+
+static inline int pwm_is_registered(struct pwm_device *p)
+{
+ return test_bit(FLAG_REGISTERED, &p->flags);
+}
+
+static inline int pwm_is_requested(struct pwm_device *p)
+{
+ return test_bit(FLAG_REQUESTED, &p->flags);
+}
+
+static inline int pwm_is_running(struct pwm_device *p)
+{
+ return test_bit(FLAG_RUNNING, &p->flags);
+}
+
+static inline void pwm_set_drvdata(struct pwm_device *p, void *data)
+{
+ p->data = data;
+}
+
+static inline void *pwm_get_drvdata(const struct pwm_device *p)
+{
+ return p->data;
+}
+
+unsigned long pwm_ns_to_ticks(struct pwm_device *p, unsigned long nsecs);
+unsigned long pwm_ticks_to_ns(struct pwm_device *p, unsigned long ticks);
+
+int pwm_config_nosleep(struct pwm_device *p, struct pwm_config *c);
+int pwm_config(struct pwm_device *p, struct pwm_config *c);
+
+int pwm_set_period_ns(struct pwm_device *p, unsigned long period_ns);
+unsigned long pwm_get_period_ns(struct pwm_device *p);
+int pwm_set_duty_ns(struct pwm_device *p, unsigned long duty_ns);
+unsigned long pwm_get_duty_ns(struct pwm_device *p);
+int pwm_set_duty_percent(struct pwm_device *p, int percent);
+int pwm_set_polarity(struct pwm_device *p, int active_high);
+
+int pwm_start(struct pwm_device *p);
+int pwm_stop(struct pwm_device *p);
+
+int pwm_synchronize(struct pwm_device *p, struct pwm_device *to_p);
+int pwm_unsynchronize(struct pwm_device *p, struct pwm_device *from_p);
+int pwm_set_handler(struct pwm_device *p, pwm_handler_t handler, void *data);
+
+int pwm_register(struct pwm_device *p, struct device *parent, int id);
+int pwm_register_byname(struct pwm_device *p, struct device *parent,
+ const char *name);
+int pwm_unregister(struct pwm_device *p);
+
+#ifdef CONFIG_GPIO_PWM
+struct pwm_device *gpio_pwm_create(int gpio);
+int gpio_pwm_destroy(struct pwm_device *p);
+#endif
+
+
+#endif
--
1.7.2.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PWM v3: 2/3] PWM: GPIO+hrtimer device emulation
2011-02-10 15:53 [PWM v3: 0/3] Proposal for a generic PWM API Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 1/3] PWM: Implement a generic PWM framework Bill Gatliff
@ 2011-02-10 15:53 ` Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 3/3] PWM: Atmel PWMC driver Bill Gatliff
2 siblings, 0 replies; 11+ messages in thread
From: Bill Gatliff @ 2011-02-10 15:53 UTC (permalink / raw)
To: linux-embedded; +Cc: Bill Gatliff
Emulates a PWM channel using a GPIO pin and an hrtimer. Subject
to CPU, scheduler and hardware limitations, can support many
PWM outputs, e.g. as many as you have GPIO pins available for.
On a 200 MHz ARM9 processor, a PWM frequency of 100 Hz can be
attained with this code so long as the duty cycle remains between
about 20-80%. At higher or lower levels, the transition events
arrive too close for the scheduler and CPU to reliably service.
This driver supports creation of new GPIO+hrtimer PWM devices via
configfs:
# mount config /config -t configfs
# mkdir /config/gpio_pwm/<gpio number>
The new PWM device will appear as /sys/class/pwm/gpio_pwm.<gpio number>.
Caveats:
* The GPIO pin number must be valid, not already in use
* The output state of the GPIO pin is configured when the PWM starts
running i.e. not immediately upon request, because the polarity of
the inactive state of the pin isn't known until the pwm_channel's
'polarity' attribute is configured
* After creating and binding the pwm_channel, you must then request
it by reading from /sys/class/pwm/gpio_pwm.<gpio number>/request
Unbind and destroy the pwm_channel by first stopping and unrequesting
the pwm_channel under sysfs as usual; then do:
# rm -rf /config/gpio_pwm/<gpio number>
Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
drivers/pwm/Kconfig | 9 ++
drivers/pwm/Makefile | 2 +
drivers/pwm/gpio-pwm.c | 354 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 365 insertions(+), 0 deletions(-)
create mode 100644 drivers/pwm/gpio-pwm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 153d455..8eb2718 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -9,3 +9,12 @@ menuconfig GENERIC_PWM
Enables PWM device support implemented via a generic
framework. If unsure, say N.
+config GPIO_PWM
+ tristate "GPIO+hrtimer PWM device emulation"
+ depends on GENERIC_PWM
+ help
+ This option enables code that emulates a single-channel
+ PWM device using a high-resolution timer and a GPIO
+ pin. The PWM framework allows you to create as many
+ of these devices as desired, subject to CPU overhead
+ and GPIO pin availability. If unsure, say N.
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7baa201..ecec3e4 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,3 +2,5 @@
# Makefile for pwm devices
#
obj-$(CONFIG_GENERIC_PWM) := pwm.o
+
+obj-$(CONFIG_GPIO_PWM) += gpio-pwm.o
diff --git a/drivers/pwm/gpio-pwm.c b/drivers/pwm/gpio-pwm.c
new file mode 100644
index 0000000..d701c3b
--- /dev/null
+++ b/drivers/pwm/gpio-pwm.c
@@ -0,0 +1,354 @@
+/*
+ * Emulates a PWM device using an hrtimer and GPIO pin
+ *
+ * Copyright (C) 2011 Bill Gatliff <bgat@billgatliff.com>
+ *
+ * This program is free software; you may redistribute and/or modify
+ * it under the terms of the GNU General Public License Version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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/hrtimer.h>
+#include <linux/err.h>
+#include <linux/workqueue.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/pwm/pwm.h>
+
+#define DRIVER_NAME "gpio-pwm"
+
+struct gpio_pwm {
+ struct pwm_device pwm;
+ struct hrtimer t;
+ struct work_struct work;
+ pwm_callback_t callback;
+ spinlock_t lock;
+ struct completion complete;
+ int gpio;
+ unsigned long polarity : 1;
+ unsigned long active : 1;
+ char name[16];
+};
+
+static inline struct gpio_pwm *to_gpio_pwm(const struct pwm_device *p)
+{
+ return container_of(p, struct gpio_pwm, pwm);
+}
+
+static void gpio_pwm_work (struct work_struct *work)
+{
+ struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
+
+ gpio_direction_output(gp->gpio, !(!!gp->polarity ^ !!gp->active));
+}
+
+static enum hrtimer_restart gpio_pwm_timeout(struct hrtimer *t)
+{
+ struct gpio_pwm *gp = container_of(t, struct gpio_pwm, t);
+ struct pwm_device *p = &gp->pwm;
+
+ if (unlikely(p->duty_ticks == 0))
+ gp->active = 0;
+ else if (unlikely(p->duty_ticks == p->period_ticks))
+ gp->active = 1;
+ else
+ gp->active ^= 1;
+
+ if (gpio_cansleep(gp->gpio))
+ schedule_work(&gp->work);
+ else
+ gpio_pwm_work(&gp->work);
+
+ if (!gp->active && p->callback)
+ p->callback(p);
+
+ if (unlikely(!gp->active && test_bit(FLAG_STOP, &p->flags))) {
+ clear_bit(FLAG_STOP, &p->flags);
+ complete_all(&gp->complete);
+ goto done;
+ }
+
+ /* TODO: shouldn't I really be using e.g. hrtimer_forward here? */
+ if (gp->active)
+ hrtimer_start(&gp->t, ktime_set(0, p->duty_ticks),
+ HRTIMER_MODE_REL);
+ else
+ hrtimer_start(&gp->t,
+ ktime_set(0, p->period_ticks - p->duty_ticks),
+ HRTIMER_MODE_REL);
+done:
+ return HRTIMER_NORESTART;
+}
+
+static void gpio_pwm_start(struct pwm_device *p)
+{
+ struct gpio_pwm *gp = to_gpio_pwm(p);
+
+ gp->active = 0;
+ gpio_pwm_timeout(&gp->t);
+ set_bit(FLAG_RUNNING, &p->flags);
+}
+
+static int gpio_pwm_config_nosleep(struct pwm_device *p, struct pwm_config *c)
+{
+ struct gpio_pwm *gp = to_gpio_pwm(p);
+ int ret = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&gp->lock, flags);
+
+ switch (c->config_mask) {
+
+ case BIT(PWM_CONFIG_DUTY_TICKS):
+ p->duty_ticks = c->duty_ticks;
+ break;
+
+ case BIT(PWM_CONFIG_START):
+ if (!hrtimer_active(&gp->t)) {
+ gpio_pwm_start(p);
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ spin_unlock_irqrestore(&gp->lock, flags);
+ return ret;
+}
+
+static int gpio_pwm_stop_sync(struct pwm_device *p)
+{
+ struct gpio_pwm *gp = to_gpio_pwm(p);
+ int ret;
+ int was_on = hrtimer_active(&gp->t);
+
+ if (was_on) {
+ do {
+ init_completion(&gp->complete);
+ set_bit(FLAG_STOP, &p->flags);
+ ret = wait_for_completion_interruptible(&gp->complete);
+ if (ret)
+ return ret;
+ } while (test_bit(FLAG_STOP, &p->flags));
+ }
+
+ clear_bit(FLAG_RUNNING, &p->flags);
+
+ return was_on;
+}
+
+static int gpio_pwm_config(struct pwm_device *p, struct pwm_config *c)
+{
+ struct gpio_pwm *gp = to_gpio_pwm(p);
+ int was_on = 0;
+
+ if (p->ops->config_nosleep) {
+ if (!p->ops->config_nosleep(p, c))
+ return 0;
+ }
+
+ might_sleep();
+
+ was_on = gpio_pwm_stop_sync(p);
+ if (was_on < 0)
+ return was_on;
+
+ if (test_bit(PWM_CONFIG_PERIOD_TICKS, &c->config_mask))
+ p->period_ticks = c->period_ticks;
+
+ if (test_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask))
+ p->duty_ticks = c->duty_ticks;
+
+ if (test_bit(PWM_CONFIG_POLARITY, &c->config_mask))
+ gp->polarity = !!c->polarity;
+
+ if (test_bit(PWM_CONFIG_START, &c->config_mask)
+ || (was_on && !test_bit(PWM_CONFIG_STOP, &c->config_mask)))
+ gpio_pwm_start(p);
+
+ return 0;
+}
+
+static int gpio_pwm_set_callback(struct pwm_device *p, pwm_callback_t callback)
+{
+ struct gpio_pwm *gp = to_gpio_pwm(p);
+ gp->callback = callback;
+ return 0;
+}
+
+static int gpio_pwm_request(struct pwm_device *p)
+{
+ p->tick_hz = 1000000000UL;
+ return 0;
+}
+
+static struct pwm_device_ops gpio_pwm_device_ops = {
+ .config = gpio_pwm_config,
+ .config_nosleep = gpio_pwm_config_nosleep,
+ .request = gpio_pwm_request,
+ .set_callback = gpio_pwm_set_callback,
+};
+
+struct pwm_device *gpio_pwm_create(int gpio)
+{
+ struct gpio_pwm *gp;
+ int ret = 0;
+
+ if (!gpio_is_valid(gpio))
+ return ERR_PTR(-EINVAL);
+
+ if (gpio_request(gpio, DRIVER_NAME))
+ return ERR_PTR(-EBUSY);
+
+ gp = kzalloc(sizeof(*gp), GFP_KERNEL);
+ if (IS_ERR_OR_NULL(gp))
+ goto err_alloc;
+
+ pwm_set_drvdata(&gp->pwm, gp);
+ gp->pwm.ops = &gpio_pwm_device_ops;
+ gp->gpio = gpio;
+
+ INIT_WORK(&gp->work, gpio_pwm_work);
+ init_completion(&gp->complete);
+ hrtimer_init(&gp->t, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ gp->t.function = gpio_pwm_timeout;
+
+ scnprintf(gp->name, sizeof(gp->name), "%s:%d", DRIVER_NAME, gpio);
+ ret = pwm_register_byname(&gp->pwm, NULL, gp->name);
+ if (ret)
+ goto err_pwm_register;
+
+ return &gp->pwm;
+
+err_pwm_register:
+ kfree(gp);
+err_alloc:
+ gpio_free(gpio);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(gpio_pwm_create);
+
+int gpio_pwm_destroy(struct pwm_device *p)
+{
+ struct gpio_pwm *gp = pwm_get_drvdata(p);
+
+ if (pwm_is_requested(&gp->pwm)) {
+ if (pwm_is_running(&gp->pwm))
+ pwm_stop(&gp->pwm);
+ pwm_release(&gp->pwm);
+ }
+ hrtimer_cancel(&gp->t);
+ cancel_work_sync(&gp->work);
+
+ pwm_unregister(&gp->pwm);
+ gpio_free(gp->gpio);
+ kfree(gp);
+
+ return 0;
+}
+EXPORT_SYMBOL(gpio_pwm_destroy);
+
+#ifdef CONFIG_CONFIGFS_FS
+struct gpio_pwm_target {
+ struct config_item item;
+ struct pwm_device *p;
+};
+
+static struct config_item_type gpio_pwm_item_type = {
+ .ct_owner = THIS_MODULE,
+};
+
+static struct config_item *make_gpio_pwm_target(struct config_group *group,
+ const char *name)
+{
+ struct gpio_pwm_target *t;
+ unsigned long gpio;
+ int ret;
+
+ t = kzalloc(sizeof(*t), GFP_KERNEL);
+ if (IS_ERR_OR_NULL(t))
+ return ERR_PTR(-ENOMEM);
+
+ ret = strict_strtoul(name, 10, &gpio);
+ if (ret || !gpio_is_valid(gpio)) {
+ ret = -EINVAL;
+ goto err_invalid_gpio;
+ }
+
+ config_item_init_type_name(&t->item, name, &gpio_pwm_item_type);
+
+ t->p = gpio_pwm_create(gpio);
+ if (IS_ERR_OR_NULL(t->p))
+ goto err_gpio_pwm_create;
+
+ return &t->item;
+
+err_gpio_pwm_create:
+err_invalid_gpio:
+ kfree(t);
+ return ERR_PTR(ret);
+}
+
+static void drop_gpio_pwm_target(struct config_group *group,
+ struct config_item *item)
+{
+ struct gpio_pwm_target *t =
+ container_of(item, struct gpio_pwm_target, item);
+
+ gpio_pwm_destroy(t->p);
+ config_item_put(&t->item);
+ kfree(t);
+}
+
+static struct configfs_group_operations gpio_pwm_subsys_group_ops = {
+ .make_item = make_gpio_pwm_target,
+ .drop_item = drop_gpio_pwm_target,
+};
+
+static struct config_item_type gpio_pwm_subsys_type = {
+ .ct_group_ops = &gpio_pwm_subsys_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem gpio_pwm_subsys = {
+ .su_group = {
+ .cg_item = {
+ .ci_name = DRIVER_NAME,
+ .ci_type = &gpio_pwm_subsys_type,
+ },
+ },
+};
+
+static int __init gpio_pwm_init(void)
+{
+ config_group_init(&gpio_pwm_subsys.su_group);
+ mutex_init(&gpio_pwm_subsys.su_mutex);
+ return configfs_register_subsystem(&gpio_pwm_subsys);
+}
+module_init(gpio_pwm_init);
+
+static void __exit gpio_pwm_exit(void)
+{
+ configfs_unregister_subsystem(&gpio_pwm_subsys);
+}
+module_exit(gpio_pwm_exit);
+#endif
+
+MODULE_AUTHOR("Bill Gatliff <bgat@billgatliff.com>");
+MODULE_DESCRIPTION("PWM channel emulator using GPIO and a high-resolution timer");
+MODULE_LICENSE("GPL");
--
1.7.2.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PWM v3: 3/3] PWM: Atmel PWMC driver
2011-02-10 15:53 [PWM v3: 0/3] Proposal for a generic PWM API Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 1/3] PWM: Implement a generic PWM framework Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 2/3] PWM: GPIO+hrtimer device emulation Bill Gatliff
@ 2011-02-10 15:53 ` Bill Gatliff
2 siblings, 0 replies; 11+ messages in thread
From: Bill Gatliff @ 2011-02-10 15:53 UTC (permalink / raw)
To: linux-embedded; +Cc: Bill Gatliff
Driver to allow the Atmel PWMC peripheral found on various
AT91 SoCs to be controlled using the Generic PWM framework.
Tested on the AT91SAM9263.
Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
drivers/pwm/Kconfig | 8 +
drivers/pwm/Makefile | 1 +
drivers/pwm/atmel-pwmc.c | 520 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 529 insertions(+), 0 deletions(-)
create mode 100644 drivers/pwm/atmel-pwmc.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8eb2718..5adc3a9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -18,3 +18,11 @@ config GPIO_PWM
pin. The PWM framework allows you to create as many
of these devices as desired, subject to CPU overhead
and GPIO pin availability. If unsure, say N.
+
+config ATMEL_PWMC
+ tristate "Atmel AT32/AT91 PWMC support"
+ depends on GENERIC_PWM && (AVR32 || ARCH_AT91SAM9263 || ARCH_AT91SAM9RL || ARCH_AT91CAP9)
+ help
+ This option enables support under the generic PWM
+ framework for PWMC peripheral channels found on
+ certain Atmel microcontrollers. If unsure, say N.
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ecec3e4..d274fa0 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_GENERIC_PWM) := pwm.o
obj-$(CONFIG_GPIO_PWM) += gpio-pwm.o
+obj-$(CONFIG_ATMEL_PWMC) += atmel-pwmc.o
diff --git a/drivers/pwm/atmel-pwmc.c b/drivers/pwm/atmel-pwmc.c
new file mode 100644
index 0000000..1be1891
--- /dev/null
+++ b/drivers/pwm/atmel-pwmc.c
@@ -0,0 +1,520 @@
+/*
+ * Atmel PWMC peripheral driver
+ *
+ * Copyright (C) 2011 Bill Gatliff <bgat@billgatliff.com>
+ * Copyright (C) 2007 David Brownell
+ *
+ * This program is free software; you may redistribute and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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
+ */
+#define DEBUG
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/completion.h>
+#include <linux/pwm/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,
+};
+
+/* TODO: NCHAN==4 only for certain AT91-ish parts! */
+#define NCHAN 4
+struct atmel_pwmc {
+ struct pwm_device p[NCHAN];
+ struct pwm_device_ops ops;
+ spinlock_t lock;
+ struct completion complete;
+ void __iomem *iobase;
+ struct clk *clk;
+ int irq;
+ u32 ccnt_mask;
+};
+
+/* TODO: debugfs attributes for peripheral register values */
+
+static inline void pwmc_writel(const struct atmel_pwmc *p, unsigned offset, u32 val)
+{
+ __raw_writel(val, p->iobase + offset);
+}
+
+static inline u32 pwmc_readl(const struct atmel_pwmc *p, unsigned offset)
+{
+ return __raw_readl(p->iobase + offset);
+}
+
+static inline void pwmc_chan_writel(const struct pwm_device *p,
+ u32 offset, u32 val)
+{
+ const struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int chan = p - &ap->p[0];
+
+ 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
+ + (chan * PWMC_CHAN_STRIDE), val);
+}
+
+static inline u32 pwmc_chan_readl(const struct pwm_device *p, u32 offset)
+{
+ const struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int chan = p - &ap->p[0];
+
+ return pwmc_readl(ap, offset + PWMC_CHAN_BASE
+ + (chan * PWMC_CHAN_STRIDE));
+}
+
+static inline int __atmel_pwmc_is_on(struct pwm_device *p)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int chan = p - &ap->p[0];
+
+ return (pwmc_readl(ap, PWMC_SR) & BIT(chan)) ? 1 : 0;
+}
+
+static inline void __atmel_pwmc_stop(struct pwm_device *p)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int chan = p - &ap->p[0];
+
+ pwmc_writel(ap, PWMC_DIS, BIT(chan));
+}
+
+static inline void __atmel_pwmc_start(struct pwm_device *p)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int chan = p - &ap->p[0];
+
+ pwmc_writel(ap, PWMC_ENA, BIT(chan));
+}
+
+static inline int __atmel_pwmc_config_polarity(struct pwm_device *p,
+ struct pwm_config *c)
+{
+ unsigned long cmr = pwmc_chan_readl(p, PWMC_CMR);
+
+ if (c->polarity)
+ clear_bit(PWMC_CMR_CPOL, &cmr);
+ else
+ set_bit(PWMC_CMR_CPOL, &cmr);
+ pwmc_chan_writel(p, PWMC_CMR, cmr);
+ p->active_high = c->polarity ? 1 : 0;
+
+ dev_dbg(p->dev, "polarity %d\n", c->polarity);
+ return 0;
+}
+
+static inline int __atmel_pwmc_config_duty_ticks(struct pwm_device *p,
+ struct pwm_config *c)
+{
+ unsigned long cmr, cprd, cpre, cdty;
+
+ cmr = pwmc_chan_readl(p, PWMC_CMR);
+ cprd = pwmc_chan_readl(p, PWMC_CPRD);
+
+ cpre = cmr & PWMC_CMR_CPRE_MASK;
+ clear_bit(PWMC_CMR_CPD, &cmr);
+
+ cdty = cprd - (c->duty_ticks >> cpre);
+
+ p->duty_ticks = c->duty_ticks;
+
+ if (__atmel_pwmc_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);
+
+ dev_dbg(p->dev, "duty_ticks = %lu cprd = %lx"
+ " cdty = %lx cpre = %lx\n", p->duty_ticks,
+ cprd, cdty, cpre);
+
+ return 0;
+}
+
+static inline int __atmel_pwmc_config_period_ticks(struct pwm_device *p,
+ struct pwm_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;
+
+ dev_dbg(p->dev, "period_ticks = %lu cprd = %x cpre = %x\n",
+ p->period_ticks, cprd, cpre);
+
+ return 0;
+}
+
+static int atmel_pwmc_config_nosleep(struct pwm_device *p, struct pwm_config *c)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int ret = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ap->lock, flags);
+
+ switch (c->config_mask) {
+
+ case BIT(PWM_CONFIG_DUTY_TICKS):
+ __atmel_pwmc_config_duty_ticks(p, c);
+ break;
+
+ case BIT(PWM_CONFIG_STOP):
+ __atmel_pwmc_stop(p);
+ break;
+
+ case BIT(PWM_CONFIG_START):
+ __atmel_pwmc_start(p);
+ break;
+
+ case BIT(PWM_CONFIG_POLARITY):
+ __atmel_pwmc_config_polarity(p, c);
+ break;
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ spin_unlock_irqrestore(&ap->lock, flags);
+ return ret;
+}
+
+static int atmel_pwmc_stop_sync(struct pwm_device *p)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int was_on = __atmel_pwmc_is_on(p);
+ int chan = p - &ap->p[0];
+ int ret;
+
+ if (was_on) {
+ do {
+ init_completion(&ap->complete);
+ set_bit(FLAG_STOP, &p->flags);
+ pwmc_writel(ap, PWMC_IER, BIT(chan));
+
+ dev_dbg(p->dev, "waiting on stop_sync completion...\n");
+
+ ret = wait_for_completion_interruptible(&ap->complete);
+
+ dev_dbg(p->dev, "stop_sync complete (%d)\n", ret);
+
+ if (ret)
+ return ret;
+ } while (test_bit(FLAG_STOP, &p->flags));
+ }
+
+ return was_on;
+}
+
+static int atmel_pwmc_config(struct pwm_device *p, struct pwm_config *c)
+{
+ int was_on = 0;
+
+ if (p->ops->config_nosleep) {
+ if (!p->ops->config_nosleep(p, c))
+ return 0;
+ }
+
+ might_sleep();
+
+ dev_dbg(p->dev, "config_mask %lx\n", c->config_mask);
+
+ was_on = atmel_pwmc_stop_sync(p);
+ if (was_on < 0)
+ return was_on;
+
+ if (test_bit(PWM_CONFIG_PERIOD_TICKS, &c->config_mask)) {
+ __atmel_pwmc_config_period_ticks(p, c);
+ if (!test_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask)) {
+ struct pwm_config d = {
+ .config_mask = PWM_CONFIG_DUTY_TICKS,
+ .duty_ticks = p->duty_ticks,
+ };
+ __atmel_pwmc_config_duty_ticks(p, &d);
+ }
+ }
+
+ if (test_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask))
+ __atmel_pwmc_config_duty_ticks(p, c);
+
+ if (test_bit(PWM_CONFIG_POLARITY, &c->config_mask))
+ __atmel_pwmc_config_polarity(p, c);
+
+ if (test_bit(PWM_CONFIG_START, &c->config_mask)
+ || (was_on && !test_bit(PWM_CONFIG_STOP, &c->config_mask)))
+ __atmel_pwmc_start(p);
+
+ return 0;
+}
+
+static void __atmel_pwmc_set_callback(struct pwm_device *p,
+ pwm_callback_t callback)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ int chan = p - &ap->p[0];
+
+ p->callback = callback;
+ pwmc_writel(ap, p->callback ? PWMC_IER : PWMC_IDR, BIT(chan));
+}
+
+static int atmel_pwmc_set_callback(struct pwm_device *p,
+ pwm_callback_t callback)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ap->lock, flags);
+ __atmel_pwmc_set_callback(p, callback);
+ spin_unlock_irqrestore(&ap->lock, flags);
+
+ return 0;
+}
+
+static int atmel_pwmc_request(struct pwm_device *p)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ap->lock, flags);
+ clk_enable(ap->clk);
+ p->tick_hz = clk_get_rate(ap->clk);
+ __atmel_pwmc_stop(p);
+ spin_unlock_irqrestore(&ap->lock, flags);
+
+ return 0;
+}
+
+static void atmel_pwmc_release(struct pwm_device *p)
+{
+ struct atmel_pwmc *ap = pwm_get_drvdata(p);
+ clk_disable(ap->clk);
+}
+
+static irqreturn_t atmel_pwmc_irq(int irq, void *data)
+{
+ struct atmel_pwmc *ap = data;
+ struct pwm_device *p;
+ u32 isr;
+ int chan;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ap->lock, flags);
+
+ isr = pwmc_readl(ap, PWMC_ISR);
+ for (chan = 0; isr; chan++, isr >>= 1) {
+ p = &ap->p[chan];
+ if (isr & 1) {
+ if (p->callback)
+ p->callback(p);
+ if (test_bit(FLAG_STOP, &p->flags)) {
+ __atmel_pwmc_stop(p);
+ clear_bit(FLAG_STOP, &p->flags);
+ }
+ complete_all(&ap->complete);
+ }
+ }
+
+ spin_unlock_irqrestore(&ap->lock, flags);
+
+ return IRQ_HANDLED;
+}
+
+static int __devinit atmel_pwmc_probe(struct platform_device *pdev)
+{
+ struct atmel_pwmc *ap;
+ struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ int chan;
+ int ret = 0;
+
+ dev_err(&pdev->dev, "%s\n", __func__);
+
+ ap = kzalloc(sizeof *ap, GFP_KERNEL);
+ if (!ap) {
+ ret = -ENOMEM;
+ goto err_atmel_pwmc_alloc;
+ }
+
+ spin_lock_init(&ap->lock);
+ init_completion(&ap->complete);
+ platform_set_drvdata(pdev, ap);
+
+ /* TODO: the datasheets are unclear as to how large CCNT
+ * actually is across all adopters of the PWMC; sixteen bits
+ * seems a safe assumption for now */
+ ap->ccnt_mask = 0xffffUL;
+
+ ap->ops.request = atmel_pwmc_request;
+ ap->ops.release = atmel_pwmc_release;
+ ap->ops.config_nosleep = atmel_pwmc_config_nosleep;
+ ap->ops.config = atmel_pwmc_config;
+ ap->ops.set_callback = atmel_pwmc_set_callback;
+
+ ap->clk = clk_get(&pdev->dev, "pwm_clk");
+ if (IS_ERR(ap->clk)) {
+ ret = -ENODEV;
+ goto err_clk_get;
+ }
+
+ ap->iobase = ioremap_nocache(r->start, r->end - r->start + 1);
+ if (IS_ERR_OR_NULL(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);
+
+ for (chan = 0; chan < NCHAN; chan++) {
+ ap->p[chan].ops = &ap->ops;
+ pwm_set_drvdata(&ap->p[chan], ap);
+ ret = pwm_register(&ap->p[chan], &pdev->dev, chan);
+ if (ret)
+ goto err_pwm_register;
+ }
+
+ ap->irq = platform_get_irq(pdev, 0);
+ if (ap->irq != -ENXIO) {
+ ret = request_irq(ap->irq, atmel_pwmc_irq, 0,
+ dev_name(&pdev->dev), ap);
+ if (ret)
+ goto err_request_irq;
+ }
+
+ return 0;
+
+err_request_irq:
+err_pwm_register:
+ for (chan = 0; chan < NCHAN; chan++) {
+ if (pwm_is_registered(&ap->p[chan]))
+ pwm_unregister(&ap->p[chan]);
+ }
+
+ iounmap(ap->iobase);
+err_ioremap:
+ clk_put(ap->clk);
+err_clk_get:
+ platform_set_drvdata(pdev, NULL);
+ kfree(ap);
+err_atmel_pwmc_alloc:
+ dev_dbg(&pdev->dev, "%s: error, returning %d\n", __func__, ret);
+ return ret;
+}
+
+static int __devexit atmel_pwmc_remove(struct platform_device *pdev)
+{
+ struct atmel_pwmc *ap = platform_get_drvdata(pdev);
+ int chan;
+
+ for (chan = 0; chan < NCHAN; chan++)
+ if (pwm_is_registered(&ap->p[chan]))
+ pwm_unregister(&ap->p[chan]);
+
+ 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_pwmc_driver = {
+ .driver = {
+ /* note: this name has to match the one in at91*_devices.c */
+ .name = "atmel_pwmc",
+ .owner = THIS_MODULE,
+ },
+ .probe = atmel_pwmc_probe,
+ .remove = __devexit_p(atmel_pwmc_remove),
+};
+
+static int __init atmel_pwmc_init(void)
+{
+ return platform_driver_register(&atmel_pwmc_driver);
+}
+module_init(atmel_pwmc_init);
+
+static void __exit atmel_pwmc_exit(void)
+{
+ platform_driver_unregister(&atmel_pwmc_driver);
+}
+module_exit(atmel_pwmc_exit);
+
+MODULE_AUTHOR("Bill Gatliff <bgat@billgatliff.com>");
+MODULE_DESCRIPTION("Driver for Atmel PWMC peripheral");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:atmel_pwmc");
--
1.7.2.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-10 15:53 ` [PWM v3: 1/3] PWM: Implement a generic PWM framework Bill Gatliff
@ 2011-02-10 20:39 ` Sascha Hauer
2011-02-11 18:22 ` Bill Gatliff
0 siblings, 1 reply; 11+ messages in thread
From: Sascha Hauer @ 2011-02-10 20:39 UTC (permalink / raw)
To: Bill Gatliff; +Cc: linux-embedded
Hi Bill,
Thanks for still working on this.
On Thu, Feb 10, 2011 at 09:53:49AM -0600, Bill Gatliff wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> new file mode 100644
> index 0000000..153d455
> --- /dev/null
> +++ b/drivers/pwm/Kconfig
> @@ -0,0 +1,11 @@
> +#
> +# PWM infrastructure and devices
> +#
> +
> +menuconfig GENERIC_PWM
> + tristate "PWM Support"
> + default n
default n is the default and can be removed.
> + help
> + Enables PWM device support implemented via a generic
> + framework. If unsure, say N.
> +
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> new file mode 100644
> index 0000000..7baa201
> --- /dev/null
> +++ b/drivers/pwm/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for pwm devices
> +#
> +obj-$(CONFIG_GENERIC_PWM) := pwm.o
> diff --git a/drivers/pwm/pwm.c b/drivers/pwm/pwm.c
> new file mode 100644
> index 0000000..2cad7cd
> --- /dev/null
> +++ b/drivers/pwm/pwm.c
> @@ -0,0 +1,619 @@
> +/*
> + * PWM API implementation
> + *
> + * Copyright (C) 2011 Bill Gatliff <bgat@billgatliff.com>
> + * Copyright (C) 2011 Arun Murthy <arun.murthy@stericsson.com>
> + *
> + * This program is free software; you may redistribute and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/kernel.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/completion.h>
> +#include <linux/workqueue.h>
> +#include <linux/list.h>
> +#include <linux/sched.h>
> +#include <linux/platform_device.h>
I can't see anything from platform_device.h being used here.
> +#include <linux/pwm/pwm.h>
> +
> +static const char *REQUEST_SYSFS = "sysfs";
> +static LIST_HEAD(pwm_device_list);
This is unused.
> +static DEFINE_MUTEX(device_list_mutex);
> +static struct class pwm_class;
> +static struct workqueue_struct *pwm_handler_workqueue;
This is used but never initialized.
> +
> +static int pwm_match_name(struct device *dev, void *name)
> +{
> + return !strcmp(name, dev_name(dev));
> +}
> +
> +static struct pwm_device *__pwm_request(struct pwm_device *p, const char *label)
> +{
I think this function should return int.
> + int ret;
> +
> + ret = test_and_set_bit(FLAG_REQUESTED, &p->flags);
> + if (ret) {
> + p = ERR_PTR(-EBUSY);
> + goto done;
> + }
> +
> + p->label = label;
> + p->pid = current->pid;
> +
> + if (p->ops->request) {
> + ret = p->ops->request(p);
> + if (ret) {
> + p = ERR_PTR(ret);
> + clear_bit(FLAG_REQUESTED, &p->flags);
> + goto done;
> + }
> + }
> +
> +done:
> + return p;
> +}
> +
> +static struct pwm_device *__pwm_request_byname(const char *name,
> + const char *label)
> +{
> + struct device *d;
> + struct pwm_device *p;
> +
> + d = class_find_device(&pwm_class, NULL, (char*)name, pwm_match_name);
> + if (!d) {
> + p = ERR_PTR(-EINVAL);
> + goto done;
> + }
> + if (IS_ERR(d)) {
> + p = (struct pwm_device *)d;
> + goto done;
> + }
class_find_device does not return error pointers. Luckily we can get rid
of this horrible cast here.
> +
> + p = __pwm_request(dev_get_drvdata(d), label);
> +
> +done:
> + return p;
> +}
> +
> +struct pwm_device *pwm_request_byname(const char *name, const char *label)
> +{
> + struct pwm_device *p;
> +
> + mutex_lock(&device_list_mutex);
> + p = __pwm_request_byname(name, label);
> + mutex_unlock(&device_list_mutex);
> + return p;
> +}
> +EXPORT_SYMBOL(pwm_request_byname);
> +
> +struct pwm_device *pwm_request(const char *bus_id, int id, const char *label)
> +{
> + char name[256];
> + int ret;
> +
> + if (id == -1)
> + ret = scnprintf(name, sizeof name, "%s", bus_id);
> + else
> + ret = scnprintf(name, sizeof name, "%s:%d", bus_id, id);
> + if (ret <= 0 || ret >= sizeof name)
> + return ERR_PTR(-EINVAL);
> +
> + return pwm_request_byname(name, label);
> +}
> +EXPORT_SYMBOL(pwm_request);
> +
> +void pwm_release(struct pwm_device *p)
> +{
> + mutex_lock(&device_list_mutex);
> +
> + if (!test_and_clear_bit(FLAG_REQUESTED, &p->flags)) {
> + BUG();
> + goto done;
> + }
> +
> + pwm_stop(p);
> + pwm_unsynchronize(p, NULL);
> + pwm_set_handler(p, NULL, NULL);
> +
> + p->label = NULL;
> + p->pid = -1;
> +
> + if (p->ops->release)
> + p->ops->release(p);
> +done:
> + mutex_unlock(&device_list_mutex);
> +}
> +EXPORT_SYMBOL(pwm_release);
> +
> +unsigned long pwm_ns_to_ticks(struct pwm_device *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_device *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_device *p, struct pwm_config *c)
> +{
> + if (test_bit(PWM_CONFIG_PERIOD_NS, &c->config_mask)) {
> + c->period_ticks = pwm_ns_to_ticks(p, c->period_ns);
> + clear_bit(PWM_CONFIG_PERIOD_NS, &c->config_mask);
> + set_bit(PWM_CONFIG_PERIOD_TICKS, &c->config_mask);
> + }
> +
> + if (test_bit(PWM_CONFIG_DUTY_NS, &c->config_mask)) {
> + c->duty_ticks = pwm_ns_to_ticks(p, c->duty_ns);
> + clear_bit(PWM_CONFIG_DUTY_NS, &c->config_mask);
> + set_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask);
> + }
> +}
> +
> +static void pwm_config_percent_to_ticks(struct pwm_device *p,
> + struct pwm_config *c)
> +{
> + if (test_bit(PWM_CONFIG_DUTY_PERCENT, &c->config_mask)) {
> + if (test_bit(PWM_CONFIG_PERIOD_TICKS, &c->config_mask))
> + c->duty_ticks = c->period_ticks;
> + else
> + c->duty_ticks = p->period_ticks;
> +
> + c->duty_ticks *= c->duty_percent;
> + c->duty_ticks /= 100;
> + clear_bit(PWM_CONFIG_DUTY_PERCENT, &c->config_mask);
> + set_bit(PWM_CONFIG_DUTY_TICKS, &c->config_mask);
> + }
> +}
> +
> +int pwm_config_nosleep(struct pwm_device *p, struct pwm_config *c)
> +{
> + if (!p->ops->config_nosleep)
> + return -EINVAL;
> +
> + pwm_config_ns_to_ticks(p, c);
> + pwm_config_percent_to_ticks(p, c);
> +
> + return p->ops->config_nosleep(p, c);
> +}
> +EXPORT_SYMBOL(pwm_config_nosleep);
> +
> +int pwm_config(struct pwm_device *p, struct pwm_config *c)
> +{
> + int ret = 0;
> +
> + pwm_config_ns_to_ticks(p, c);
> + pwm_config_percent_to_ticks(p, c);
> +
> + switch (c->config_mask & (BIT(PWM_CONFIG_PERIOD_TICKS)
> + | BIT(PWM_CONFIG_DUTY_TICKS))) {
> + case BIT(PWM_CONFIG_PERIOD_TICKS):
> + if (p->duty_ticks > c->period_ticks) {
> + ret = -EINVAL;
> + goto err;
> + }
> + break;
> + case BIT(PWM_CONFIG_DUTY_TICKS):
> + if (p->period_ticks < c->duty_ticks) {
> + ret = -EINVAL;
> + goto err;
> + }
> + break;
> + case BIT(PWM_CONFIG_DUTY_TICKS) | BIT(PWM_CONFIG_PERIOD_TICKS):
> + if (c->duty_ticks > c->period_ticks) {
> + ret = -EINVAL;
> + goto err;
> + }
> + break;
> + default:
> + break;
> + }
> +
> +err:
> + dev_dbg(p->dev, "%s: config_mask %lu 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->ops->config(p, c);
> +}
> +EXPORT_SYMBOL(pwm_config);
> +
> +int pwm_set_period_ns(struct pwm_device *p, unsigned long period_ns)
> +{
> + struct pwm_config c = {
> + .config_mask = BIT(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_device *p)
> +{
> + return pwm_ticks_to_ns(p, p->period_ticks);
> +}
> +EXPORT_SYMBOL(pwm_get_period_ns);
> +
> +int pwm_set_duty_ns(struct pwm_device *p, unsigned long duty_ns)
> +{
> + struct pwm_config c = {
> + .config_mask = BIT(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_device *p)
> +{
> + return pwm_ticks_to_ns(p, p->duty_ticks);
> +}
> +EXPORT_SYMBOL(pwm_get_duty_ns);
> +
> +int pwm_set_duty_percent(struct pwm_device *p, int percent)
> +{
> + struct pwm_config c = {
> + .config_mask = BIT(PWM_CONFIG_DUTY_PERCENT),
> + .duty_percent = percent,
> + };
> + return pwm_config(p, &c);
> +}
> +EXPORT_SYMBOL(pwm_set_duty_percent);
> +
> +int pwm_set_polarity(struct pwm_device *p, int active_high)
> +{
> + struct pwm_config c = {
> + .config_mask = BIT(PWM_CONFIG_POLARITY),
> + .polarity = active_high,
> + };
> + return pwm_config(p, &c);
> +}
> +EXPORT_SYMBOL(pwm_set_polarity);
> +
> +int pwm_start(struct pwm_device *p)
> +{
> + struct pwm_config c = {
> + .config_mask = BIT(PWM_CONFIG_START),
> + };
> + return pwm_config(p, &c);
> +}
> +EXPORT_SYMBOL(pwm_start);
> +
> +int pwm_stop(struct pwm_device *p)
> +{
> + struct pwm_config c = {
> + .config_mask = BIT(PWM_CONFIG_STOP),
> + };
> + return pwm_config(p, &c);
> +}
> +EXPORT_SYMBOL(pwm_stop);
> +
> +int pwm_synchronize(struct pwm_device *p, struct pwm_device *to_p)
> +{
> + if (!p->ops->synchronize)
> + return -EINVAL;
> +
> + return p->ops->synchronize(p, to_p);
> +}
> +EXPORT_SYMBOL(pwm_synchronize);
> +
> +int pwm_unsynchronize(struct pwm_device *p, struct pwm_device *from_p)
> +{
> + if (!p->ops->unsynchronize)
> + return -EINVAL;
> +
> + return p->ops->unsynchronize(p, from_p);
> +}
> +EXPORT_SYMBOL(pwm_unsynchronize);
> +
> +static void pwm_handler(struct work_struct *w)
> +{
> + struct pwm_device *p = container_of(w, struct pwm_device,
> + handler_work);
> + if (p->handler && p->handler(p, p->handler_data))
> + pwm_stop(p);
> +}
A pwm should not stop when the handler returns non null. Instead the
handler should call pwm_stop if it wants to. This is much clearer.
> +
> +static void __pwm_callback(struct pwm_device *p)
> +{
> + queue_work(pwm_handler_workqueue, &p->handler_work);
> +}
> +
> +int pwm_set_handler(struct pwm_device *p, pwm_handler_t handler, void *data)
> +{
> + if (p->ops->set_callback) {
> + p->handler_data = data;
> + p->handler = handler;
> + INIT_WORK(&p->handler_work, pwm_handler);
> + return p->ops->set_callback(p, handler ? __pwm_callback : NULL);
Why must a pwm driver be bothered with the __pwm_callback argument? It
could call a function exported from the pwm API instead.
> + }
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(pwm_set_handler);
> +
> +static ssize_t pwm_run_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pwm_device *p = dev_get_drvdata(dev);
> + return sprintf(buf, "%d\n", pwm_is_running(p));
> +}
> +
> +static ssize_t pwm_run_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct pwm_device *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, S_IRUGO | S_IWUSR, pwm_run_show, pwm_run_store);
> +
> +static ssize_t pwm_tick_hz_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pwm_device *p = dev_get_drvdata(dev);
> + return sprintf(buf, "%lu\n", p->tick_hz);
> +}
> +static DEVICE_ATTR(tick_hz, S_IRUGO, pwm_tick_hz_show, NULL);
> +
> +static ssize_t pwm_duty_ns_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pwm_device *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_device *p = dev_get_drvdata(dev);
> +
> + if (!strict_strtoul(buf, 10, &duty_ns))
> + pwm_set_duty_ns(p, duty_ns);
> + return len;
> +}
> +static DEVICE_ATTR(duty_ns, S_IRUGO | S_IWUSR, 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_device *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_device *p = dev_get_drvdata(dev);
> +
> + if (!strict_strtoul(buf, 10, &period_ns))
> + pwm_set_period_ns(p, period_ns);
> + return len;
> +}
> +static DEVICE_ATTR(period_ns, S_IRUGO | S_IWUSR, 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_device *p = dev_get_drvdata(dev);
> + return sprintf(buf, "%d\n", p->active_high ? 1 : 0);
> +}
> +
> +static ssize_t pwm_polarity_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + unsigned long polarity;
> + struct pwm_device *p = dev_get_drvdata(dev);
> +
> + if (!strict_strtoul(buf, 10, &polarity))
> + pwm_set_polarity(p, polarity);
> + return len;
> +}
> +static DEVICE_ATTR(polarity, S_IRUGO | S_IWUSR, pwm_polarity_show, pwm_polarity_store);
> +
> +static ssize_t pwm_request_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pwm_device *p = dev_get_drvdata(dev);
> + struct pwm_device *ret;
> +
> + mutex_lock(&device_list_mutex);
> + ret = __pwm_request(p, REQUEST_SYSFS);
> + mutex_unlock(&device_list_mutex);
> +
> + if (IS_ERR_OR_NULL(ret))
> + return sprintf(buf, "fail (owner: %s pid: %d)\n",
> + p->label, p->pid);
> + else
> + return sprintf(buf, "%s (pid %d)\n", ret->label, ret->pid);
> +}
I think it has been mentioned in an earlier review that it's not good
to request a pwm by reading a sysfs entry. People are not expecting to
change anything while reading a file.
I think the whole concept of requesting a pwm via sysfs in userspace is
broken as there is no way to free the pwm again when the process which
requested a pwm dies.
> +
> +static ssize_t pwm_request_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct pwm_device *p = dev_get_drvdata(dev);
> +
> + pwm_release(p);
> + return len;
> +}
> +static DEVICE_ATTR(request, S_IRUGO | S_IWUSR, pwm_request_show, pwm_request_store);
> +
> +static const struct attribute *pwm_attrs[] =
> +{
> + &dev_attr_tick_hz.attr,
> + &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 struct class_attribute pwm_class_attrs[] = {
> + __ATTR_NULL,
> +};
> +
> +static struct class pwm_class = {
> + .name = "pwm",
> + .owner = THIS_MODULE,
> +
> + .class_attrs = pwm_class_attrs,
> +};
> +
> +int pwm_register_byname(struct pwm_device *p, struct device *parent,
> + const char *name)
> +{
> + struct device *d;
> + int ret;
> +
> + if (!p->ops || !p->ops->config)
> + return -EINVAL;
> +
> + mutex_lock(&device_list_mutex);
> +
> + d = class_find_device(&pwm_class, NULL, (char*)name, pwm_match_name);
> + if (d) {
> + ret = -EEXIST;
> + goto err_found_device;
> + }
> +
> + p->dev = device_create(&pwm_class, parent, MKDEV(0, 0), NULL, name);
> + if (IS_ERR(p->dev)) {
> + ret = PTR_ERR(p->dev);
> + goto err_device_create;
> + }
> +
> + ret = sysfs_create_group(&p->dev->kobj, &pwm_device_attr_group);
> + if (ret)
> + goto err_create_group;
> +
> + dev_set_drvdata(p->dev, p);
> + p->flags = BIT(FLAG_REGISTERED);
> +
> + goto done;
> +
> +err_create_group:
> + device_unregister(p->dev);
> + p->flags = 0;
> +
> +err_device_create:
> +err_found_device:
> +done:
> + mutex_unlock(&device_list_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(pwm_register_byname);
> +
> +int pwm_register(struct pwm_device *p, struct device *parent, int id)
> +{
> + int ret;
> + char name[256];
> +
> + if (IS_ERR_OR_NULL(parent))
> + return -EINVAL;
> +
> + if (id == -1)
> + ret = scnprintf(name, sizeof name, "%s", dev_name(parent));
> + else
> + ret = scnprintf(name, sizeof name, "%s:%d", dev_name(parent), id);
> + if (ret <= 0 || ret >= sizeof name)
> + return -EINVAL;
> +
> + return pwm_register_byname(p, parent, name);
> +}
> +EXPORT_SYMBOL(pwm_register);
> +
> +int pwm_unregister(struct pwm_device *p)
> +{
> + int ret = 0;
> +
> + mutex_lock(&device_list_mutex);
> +
> + if (pwm_is_running(p) || pwm_is_requested(p)) {
> + ret = -EBUSY;
> + goto done;
> + }
> +
> + sysfs_remove_group(&p->dev->kobj, &pwm_device_attr_group);
> + device_unregister(p->dev);
> + p->flags = 0;
> +
> +done:
> + mutex_unlock(&device_list_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(pwm_unregister);
> +
> +static int __init pwm_init(void)
> +{
> + return class_register(&pwm_class);
> +}
> +
> +static void __exit pwm_exit(void)
> +{
> + class_unregister(&pwm_class);
> +}
> +
> +#ifdef MODULE
> +module_init(pwm_init);
> +module_exit(pwm_exit);
> +MODULE_LICENSE("GPL");
> +#else
> +postcore_initcall(pwm_init);
> +#endif
> diff --git a/include/linux/pwm/pwm.h b/include/linux/pwm/pwm.h
> new file mode 100644
> index 0000000..871e38c
> --- /dev/null
> +++ b/include/linux/pwm/pwm.h
> @@ -0,0 +1,163 @@
> +/*
> + * Copyright (C) 2011 Bill Gatliff < bgat@billgatliff.com>
> + * Copyright (C) 2011 Arun Murthy <arun.murth@stericsson.com>
> + *
> + * This program is free software; you may redistribute and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + *
> + * 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
> + */
> +#ifndef __LINUX_PWM_H
> +#define __LINUX_PWM_H
> +
> +enum {
> + FLAG_REGISTERED = 0,
> + FLAG_REQUESTED = 1,
> + FLAG_STOP = 2,
> + FLAG_RUNNING = 3,
> +};
> +
> +enum {
> + PWM_CONFIG_DUTY_TICKS = 0,
> + PWM_CONFIG_PERIOD_TICKS = 1,
> + PWM_CONFIG_POLARITY = 2,
> + PWM_CONFIG_START = 3,
> + PWM_CONFIG_STOP = 4,
> +
> + PWM_CONFIG_HANDLER = 5,
> +
> + PWM_CONFIG_DUTY_NS = 6,
> + PWM_CONFIG_DUTY_PERCENT = 7,
> + PWM_CONFIG_PERIOD_NS = 8,
> +};
There is no need for introducing ioctl like interfaces in the kernel.
You should create the appropriate callbacks instead which is both easier
to read and easier to implement.
> +
> +struct pwm_config;
> +struct pwm_device;
> +
> +typedef int (*pwm_handler_t)(struct pwm_device *p, void *data);
> +typedef void (*pwm_callback_t)(struct pwm_device *p);
> +
> +struct pwm_device_ops {
> + int (*request) (struct pwm_device *p);
> + void (*release) (struct pwm_device *p);
> + int (*config) (struct pwm_device *p,
> + struct pwm_config *c);
> + int (*config_nosleep) (struct pwm_device *p,
> + struct pwm_config *c);
> + int (*synchronize) (struct pwm_device *p,
> + struct pwm_device *to_p);
> + int (*unsynchronize) (struct pwm_device *p,
> + struct pwm_device *from_p);
> + int (*set_callback) (struct pwm_device *p,
> + pwm_callback_t callback);
> +};
> +
> +struct pwm_config {
> + unsigned long 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;
> +};
This struct shows the major problem with this API. It allows to pass in
inconsistent and contradicting values. We should have a single internal
representation of the pwm values, either in duty_ns/period_ns or
duty_ticks/period_ticks. Everything else should be provided with helper
functions.
This would also make the implementation and sanity checking above much
simpler.
> +
> +struct pwm_device {
> + struct list_head list;
This is unused.
> +
> + struct device *dev;
> + struct pwm_device_ops *ops;
> +
> + void *data;
> +
> + const char *label;
> + pid_t pid;
> +
> + volatile unsigned long flags;
This should not be volatile.
> +
> + unsigned long tick_hz;
> +
> + pwm_callback_t callback;
This field is unused and seems to be a duplicate of handler.
> +
> + struct work_struct handler_work;
> + pwm_handler_t handler;
> + void *handler_data;
> +
> + int active_high;
> + unsigned long period_ticks;
> + unsigned long duty_ticks;
> +};
> +
> +struct pwm_device *pwm_request_byname(const char *name, const char *label);
> +struct pwm_device *pwm_request(const char *bus_id, int id, const char *label);
> +void pwm_release(struct pwm_device *p);
> +
> +static inline int pwm_is_registered(struct pwm_device *p)
> +{
> + return test_bit(FLAG_REGISTERED, &p->flags);
> +}
I think theres no value in having the FLAG_REGISTERED flag and this
function.
> +
> +static inline int pwm_is_requested(struct pwm_device *p)
> +{
> + return test_bit(FLAG_REQUESTED, &p->flags);
> +}
> +
> +static inline int pwm_is_running(struct pwm_device *p)
> +{
> + return test_bit(FLAG_RUNNING, &p->flags);
> +}
> +
> +static inline void pwm_set_drvdata(struct pwm_device *p, void *data)
> +{
> + p->data = data;
> +}
> +
> +static inline void *pwm_get_drvdata(const struct pwm_device *p)
> +{
> + return p->data;
> +}
> +
> +unsigned long pwm_ns_to_ticks(struct pwm_device *p, unsigned long nsecs);
> +unsigned long pwm_ticks_to_ns(struct pwm_device *p, unsigned long ticks);
> +
> +int pwm_config_nosleep(struct pwm_device *p, struct pwm_config *c);
> +int pwm_config(struct pwm_device *p, struct pwm_config *c);
> +
> +int pwm_set_period_ns(struct pwm_device *p, unsigned long period_ns);
> +unsigned long pwm_get_period_ns(struct pwm_device *p);
> +int pwm_set_duty_ns(struct pwm_device *p, unsigned long duty_ns);
> +unsigned long pwm_get_duty_ns(struct pwm_device *p);
> +int pwm_set_duty_percent(struct pwm_device *p, int percent);
> +int pwm_set_polarity(struct pwm_device *p, int active_high);
> +
> +int pwm_start(struct pwm_device *p);
> +int pwm_stop(struct pwm_device *p);
> +
> +int pwm_synchronize(struct pwm_device *p, struct pwm_device *to_p);
> +int pwm_unsynchronize(struct pwm_device *p, struct pwm_device *from_p);
> +int pwm_set_handler(struct pwm_device *p, pwm_handler_t handler, void *data);
> +
> +int pwm_register(struct pwm_device *p, struct device *parent, int id);
> +int pwm_register_byname(struct pwm_device *p, struct device *parent,
> + const char *name);
> +int pwm_unregister(struct pwm_device *p);
> +
> +#ifdef CONFIG_GPIO_PWM
> +struct pwm_device *gpio_pwm_create(int gpio);
> +int gpio_pwm_destroy(struct pwm_device *p);
> +#endif
Function declarations should not be ifdefed.
> +
> +
> +#endif
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-10 20:39 ` Sascha Hauer
@ 2011-02-11 18:22 ` Bill Gatliff
2011-02-12 5:48 ` Bill Gatliff
2011-02-14 8:03 ` Sascha Hauer
0 siblings, 2 replies; 11+ messages in thread
From: Bill Gatliff @ 2011-02-11 18:22 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-embedded
Sascha:
Thanks so much for the great feedback! My responses are inlined below.
b.g.
On Thu, Feb 10, 2011 at 2:39 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> default n is the default and can be removed.
Fixed.
> I can't see anything from platform_device.h being used here.
Correct. I forgot to remove that header file. Fixed.
>
>> +#include <linux/pwm/pwm.h>
>> +
>> +static const char *REQUEST_SYSFS = "sysfs";
>> +static LIST_HEAD(pwm_device_list);
>
> This is unused.
>
>> +static DEFINE_MUTEX(device_list_mutex);
>> +static struct class pwm_class;
>> +static struct workqueue_struct *pwm_handler_workqueue;
>
> This is used but never initialized.
Fixed.
>
>> +
>> +static int pwm_match_name(struct device *dev, void *name)
>> +{
>> + return !strcmp(name, dev_name(dev));
>> +}
>> +
>> +static struct pwm_device *__pwm_request(struct pwm_device *p, const char *label)
>> +{
>
> I think this function should return int.
I think that might probably work. :) Fixed.
> class_find_device does not return error pointers. Luckily we can get rid
> of this horrible cast here.
Agreed. Fixed. Though, I still use IS_ERR_OR_NULL on the return
value from class_find_device in case that someday it decides to return
error pointers.
> A pwm should not stop when the handler returns non null. Instead the
> handler should call pwm_stop if it wants to. This is much clearer.
Good point. Fixed.
> Why must a pwm driver be bothered with the __pwm_callback argument? It
> could call a function exported from the pwm API instead.
Because a driver needs to know whether or not a callback is requested.
If there is no callback, then the driver has the opportunity to mask
the interrupt(s) that trigger the callback mechanism.
I suppose that an alternative would be for the framework to provide
drivers enable_callback() and disable_callback() hooks, plus a
pwm_callback() that they invoke when the callback occurs. That would
hide the details of the callback implementation fully inside of the
PWM API implementation.
I suppose I can live with that. Will fix.
> I think it has been mentioned in an earlier review that it's not good
> to request a pwm by reading a sysfs entry. People are not expecting to
> change anything while reading a file.
> I think the whole concept of requesting a pwm via sysfs in userspace is
> broken as there is no way to free the pwm again when the process which
> requested a pwm dies.
I don't enforce that the requester's PID and the unrequester's PID be
the same. It is strictly an advisory interface a'la gpiolib. I
really don't think this concept is broken at all.
I generally use echo(1) to request and unrequest PWM channels from
userspace, as well as to set the period, duty, etc.. This lets me
test the API without having to implement an ioctl program, and also
allows for PWM control from shell scripts, etc.
The objective of the sysfs request attribute is to prevent
simultaneous uses of a channel in both kernel and user spaces. Same
as with GPIO pins.
> There is no need for introducing ioctl like interfaces in the kernel.
> You should create the appropriate callbacks instead which is both easier
> to read and easier to implement.
I disagree.
The problem with implementing callbacks to do the job of these
configuration flags is trying to come up with an approach that lets
users set multiple parameters at the same time. To achieve what I am
doing with these flags using callbacks, I would need a
pwm_set_period_and_duty_ns(), pwm_set_period_and_duty_and_polarity(),
pwm_set_duty_and_polarity(), and so on.
I'm thinking mostly about DC motor control applications, where you
vary both the period and the duty cycle based on the speed of the
motor. The two values have to change at the same time.
You might argue that all I need to add, therefore, is a
pwm_set_duty_and_period_ns(). You would be right--- and I can
certainly do that at the API level. But I still want one
configuration entry point into the driver code itself, which means I
still need flags somewhere.
Why do I want one entry point into drivers? Because at the same time
users want to change single or multiple parameters in one go, the
hardware may prevent doing so simply because it doesn't work that way.
The pwm_config structure conveys to the hardware what the desired
final state of the hardware is for that invocation of the
configuration event. The flags tell which fields in the pwm_config
structure are valid. So long as PWM hardware and my API work as they
do, I can't see a way to avoid having both.
I guess the above is my polite way of saying "nak". :)
> This struct shows the major problem with this API. It allows to pass in
> inconsistent and contradicting values. We should have a single internal
> representation of the pwm values, either in duty_ns/period_ns or
> duty_ticks/period_ticks. Everything else should be provided with helper
> functions.
I agree with you, and in fact things started out that way. The
redundancy was added by a user's request, but has been somewhat
problematic. I'll see if I can take them back out.
>> +
>> +struct pwm_device {
>> + struct list_head list;
>
> This is unused.
Fixed.
>> + volatile unsigned long flags;
>
> This should not be volatile.
I added that in because I was getting compiler warnings with bitops.
But now that I take it back out, I can't seem to reproduce those
warnings. Fixed.
>> + pwm_callback_t callback;
>
> This field is unused and seems to be a duplicate of handler.
It's definitely unused and unnecessary. I think I put it there so
that drivers wouldn't have to allocate their own storage for the
callback pointer. At any rate, it'll go away when I tweak the
callback API itself. Fixed.
> I think theres no value in having the FLAG_REGISTERED flag and this
> function.
I think it sneaked in before I was using the mutex to prevent the list
of devices from changing during a lookup. Fixed.
> Function declarations should not be ifdefed.
Fixed.
b.g.
--
Bill Gatliff
bgat@billgatliff.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-11 18:22 ` Bill Gatliff
@ 2011-02-12 5:48 ` Bill Gatliff
2011-02-14 8:03 ` Sascha Hauer
1 sibling, 0 replies; 11+ messages in thread
From: Bill Gatliff @ 2011-02-12 5:48 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-embedded
Sascha:
>> I think theres no value in having the FLAG_REGISTERED flag and this
>> function.
>
> I think it sneaked in before I was using the mutex to prevent the list
> of devices from changing during a lookup. Fixed.
I take that back. What having that flag lets one do is error recovery
that looks like this:
for (wchan = 0; wchan < NCHAN; wchan++) {
ret = pwm_register(&x->p[wchan], ... );
if (ret) goto error;
}
...
error:
for (wchan = 0; wchan < NCHAN; wchan++) {
if (pwm_is_registered(&x->p[wchan]))
pwm_unregister(&x->p[wchan]);
}
Without pwm_is_registered() the driver must itself keep track of which
devices were successfully registered and which ones weren't---
something the internals of the API already knows.
True, in the above example it's simply a matter of always registering
in increasing channel order AND preserving the wchan value if an error
occurs. And then having an "errchan" index that counts from zero up
to the last successfully-registered channel in the error loop.
The pwm_is_registered() function does work of tracking successful
device registrations, and therefore leads to simpler code in device
drivers. In particular, the error paths are simpler (and less
redundant across drivers) than without pwm_is_registered().
So I think pwm_is_registered() is a keeper.
b.g.
--
Bill Gatliff
bgat@billgatliff.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-11 18:22 ` Bill Gatliff
2011-02-12 5:48 ` Bill Gatliff
@ 2011-02-14 8:03 ` Sascha Hauer
2011-02-14 16:56 ` Bill Gatliff
1 sibling, 1 reply; 11+ messages in thread
From: Sascha Hauer @ 2011-02-14 8:03 UTC (permalink / raw)
To: Bill Gatliff; +Cc: linux-embedded
On Fri, Feb 11, 2011 at 12:22:27PM -0600, Bill Gatliff wrote:
> Sascha:
>
>
>
> The objective of the sysfs request attribute is to prevent
> simultaneous uses of a channel in both kernel and user spaces. Same
> as with GPIO pins.
The gpio framework does this with 'export', and it does exactly what you
need: Make sure that a gpio is not used by both the kernel and
userspace. Unlike the pwm api this does not give the impression that a
gpio is owned by a specific process.
>
> > There is no need for introducing ioctl like interfaces in the kernel.
> > You should create the appropriate callbacks instead which is both easier
> > to read and easier to implement.
>
> I disagree.
>
> The problem with implementing callbacks to do the job of these
> configuration flags is trying to come up with an approach that lets
> users set multiple parameters at the same time. To achieve what I am
> doing with these flags using callbacks, I would need a
> pwm_set_period_and_duty_ns(), pwm_set_period_and_duty_and_polarity(),
> pwm_set_duty_and_polarity(), and so on.
A single pwm_set(int period_ns, int duty_ns, int polarity) would do.
Arguably this function is by definition non atomic because it will
always take effect during the next period and not in the current one.
If it takes effect during the current period it means that the current
period will be interrupted which renders your pwm useless for motors.
>
> I'm thinking mostly about DC motor control applications, where you
> vary both the period and the duty cycle based on the speed of the
> motor. The two values have to change at the same time.
>
> You might argue that all I need to add, therefore, is a
> pwm_set_duty_and_period_ns(). You would be right--- and I can
> certainly do that at the API level. But I still want one
> configuration entry point into the driver code itself, which means I
> still need flags somewhere.
>
> Why do I want one entry point into drivers? Because at the same time
> users want to change single or multiple parameters in one go, the
> hardware may prevent doing so simply because it doesn't work that way.
So you are prososing an API which does not abstract from the hardware
capabilities and does not provide a way to query the hardware
capabilities. This means your motor control application will work with
one pwm in the background while throwing stack traces with another
pwm.
Honestly, putting a 'this call might or might not be possible' into API
level does not seem to be a good idea to me.
How about making all calls to the pwm non atomic?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-14 8:03 ` Sascha Hauer
@ 2011-02-14 16:56 ` Bill Gatliff
2011-02-16 8:18 ` Sascha Hauer
0 siblings, 1 reply; 11+ messages in thread
From: Bill Gatliff @ 2011-02-14 16:56 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-embedded
Sascha:
Keep the great comments coming! My replies inlined below.
On Mon, Feb 14, 2011 at 2:03 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The gpio framework does this with 'export', and it does exactly what you
> need: Make sure that a gpio is not used by both the kernel and
> userspace. Unlike the pwm api this does not give the impression that a
> gpio is owned by a specific process.
The gpio framework doesn't provide a way for applications to know that
a pin is already exported and in use by another process, either.
Unless you stipulate that the only way to know that a pin ISN'T
already in use by another process is that it isn't already exported.
But the check-then-export procedure is a test-then-act, which is
non-atomic. So two applications can attempt to export the pin for
themselves, and both risk actually thinking that they got it.
[Actually, I take the above back; prior to clicking send I checked
export_store in gpiolib.c, and it looks like the export will fail for
one application if two attempt to export simultaneously. That leaves
just the namespace question.]
In my implementation, a process merely reads from the request
attribute; if the pid it gets back is its own, then it knows that it
now has the (albeit advisory) lock on the device. That's atomic (or
at least it's supposed to be, there may be bugs left in my code :) ).
Maybe the whole concept I have implemented is flawed, and I'm willing
to accept a failed experiment if that's what it really is. Sounds
like you might think that it actually is.
The namespace for PWMs is more complex than with GPIO pins, though,
and I'm not sure I would ever be comfortable with integer enumerations
of PWM devices; hardware usually dictates that specific functions are
tied to specific PWM devices, regardless of the order in which said
devices are registered in the system across kernel builds and/or
releases.
The complexity of the namespace for PWM devices makes it convenient
for users to have a reference to the names of available devices. The
entries in /sys/class/pwm provide this list.
What I'm getting to is this. I think we need to have a list of PWM
devices present in a system available under /sys/class/pwm at all
times, not just when those devices have been exported to users. If
you accept that, and you also accept the need for a mechanism for
applications to provide notification that they are now exerting
control over a PWM device, how would you prefer the interface for that
mechanism look?
My answer to that question is the current code. I'd love to hear other answers!
> A single pwm_set(int period_ns, int duty_ns, int polarity) would do.
... except when you want to change only one of those three parameters.
Which is all you would be able to do if you provide period_ns,
duty_ns, and polarity attributes to userspace.
But then look at it from the backend. Are you going to force driver
authors to implement a multitude of functions for the different
permutations of arguments? That would be redundant work, which I
decided to put into the PWM API itself so that it wouldn't appear over
and over again in driver code.
> Arguably this function is by definition non atomic because it will
> always take effect during the next period and not in the current one.
> If it takes effect during the current period it means that the current
> period will be interrupted which renders your pwm useless for motors.
Agreed. But some hardware, like the Atmel PWMC, have double-buffered
registers. So they do the handoff at the end of the period
themselves. Other "hardware", like GPIO pins, force you do to the
handoff manually. But the PWM API looks the same to users in both
cases.
> So you are proposing an API which does not abstract from the hardware
> capabilities and does not provide a way to query the hardware
> capabilities. This means your motor control application will work with
> one pwm in the background while throwing stack traces with another
> pwm.
The API provides a consistent set of capabilities to users (be they
kernel or otherwise), regardless of what the hardware looks like. All
hardware implementations are required to support the ability to
control period_ticks and duty_ticks, but they may choose to implement
that support in whatever way is dictated by the hardware.
If a user (kernel or otherwise) of pwm_config() causes a stack trace,
then it's because they didn't check the return value of that function.
That's hardly my problem. :)
I'm trying to hide as few details as possible from users of
pwm_config(), actually. If they call pwm_config_nosleep() and get an
error, then the user can then decide for themselves whether to
implement a kthread or whatever other way is easiest for them. I
don't want to make those decisions myself, because then I'll get
complaints from people calling pwm_config_nosleep() and discovering
that the hardware reconfiguration is much later than they expected
because of the kthread *I* have to create. I would rather just tell
them the problem up front.
Gpiolib does the same thing, but in a different way, with
gpio_cansleep(). I don't make them ask up front, I just notify them
after the fact when I couldn't carry out the request as they intended.
> Honestly, putting a 'this call might or might not be possible' into API
> level does not seem to be a good idea to me.
You'd rather me do away with pwm_config_nosleep() entirely? That
would be the alternative. Then nobody could use it, even on hardware
(like Atmel PWMC) that can support it!
> How about making all calls to the pwm non atomic?
If you want that, just call pwm_config() all the time, and pretend
that pwm_config_nosleep() doesn't exist. But then you can't do PWM
updates from hrtimers or interrupt handlers. I'm not arguing as to
whether someone should be doing that or not, I'm just giving them the
capability to do so in situations where the hardware will allow it---
and informing them when they are in situations where the hardware
won't allow it.
b.g.
--
Bill Gatliff
bgat@billgatliff.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-14 16:56 ` Bill Gatliff
@ 2011-02-16 8:18 ` Sascha Hauer
2011-02-16 17:00 ` Bill Gatliff
0 siblings, 1 reply; 11+ messages in thread
From: Sascha Hauer @ 2011-02-16 8:18 UTC (permalink / raw)
To: Bill Gatliff; +Cc: linux-embedded
On Mon, Feb 14, 2011 at 10:56:57AM -0600, Bill Gatliff wrote:
> Sascha:
>
>
> Keep the great comments coming! My replies inlined below.
>
> On Mon, Feb 14, 2011 at 2:03 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > The gpio framework does this with 'export', and it does exactly what you
> > need: Make sure that a gpio is not used by both the kernel and
> > userspace. Unlike the pwm api this does not give the impression that a
> > gpio is owned by a specific process.
>
> The gpio framework doesn't provide a way for applications to know that
> a pin is already exported and in use by another process, either.
> Unless you stipulate that the only way to know that a pin ISN'T
> already in use by another process is that it isn't already exported.
> But the check-then-export procedure is a test-then-act, which is
> non-atomic. So two applications can attempt to export the pin for
> themselves, and both risk actually thinking that they got it.
The discussion started with me saying that the requesting process may
die and the kernel has no way to clean up the requested resources (pwm).
You responded that you want to make sure that there is no resource
conflict between the kernel and userspace. Now we are at two userspace
processes again. We are drawing circles.
What should a userspace process do if it finds the pwm requested by
another process (which may be already dead)? Free it and request it
again when -f is supplied? I think that if you want resource management
then you have to implement a /dev interface. I don't think that's
necessary though.
>
> [Actually, I take the above back; prior to clicking send I checked
> export_store in gpiolib.c, and it looks like the export will fail for
> one application if two attempt to export simultaneously. That leaves
> just the namespace question.]
>
> In my implementation, a process merely reads from the request
> attribute; if the pid it gets back is its own, then it knows that it
> now has the (albeit advisory) lock on the device. That's atomic (or
> at least it's supposed to be, there may be bugs left in my code :) ).
> Maybe the whole concept I have implemented is flawed, and I'm willing
> to accept a failed experiment if that's what it really is. Sounds
> like you might think that it actually is.
There are two things I really don't like about this. First is reading
a sysfs entry should not alter the system state. Second is the kernel
should only grant resources it can claim back when the process dies.
>
> The namespace for PWMs is more complex than with GPIO pins, though,
> and I'm not sure I would ever be comfortable with integer enumerations
> of PWM devices; hardware usually dictates that specific functions are
> tied to specific PWM devices, regardless of the order in which said
> devices are registered in the system across kernel builds and/or
> releases.
>
> The complexity of the namespace for PWM devices makes it convenient
> for users to have a reference to the names of available devices. The
> entries in /sys/class/pwm provide this list.
>
> What I'm getting to is this. I think we need to have a list of PWM
> devices present in a system available under /sys/class/pwm at all
> times, not just when those devices have been exported to users. If
> you accept that, and you also accept the need for a mechanism for
> applications to provide notification that they are now exerting
> control over a PWM device, how would you prefer the interface for that
> mechanism look?
>
> My answer to that question is the current code. I'd love to hear other answers!
>
> > A single pwm_set(int period_ns, int duty_ns, int polarity) would do.
>
> ... except when you want to change only one of those three parameters.
> Which is all you would be able to do if you provide period_ns,
> duty_ns, and polarity attributes to userspace.
A pwm user can store the actual values for period_ns/duty_ns in his
private data and can leave the values constant it does not want to
change. I consider a sysfs driver a pwm being a pwm user also.
>
> But then look at it from the backend. Are you going to force driver
> authors to implement a multitude of functions for the different
> permutations of arguments?
What multitude of arguments are you talking about? With my proposed
version of pwm_set() a driver only has to parse three arguments.
> That would be redundant work, which I
> decided to put into the PWM API itself so that it wouldn't appear over
> and over again in driver code.
>
> > Arguably this function is by definition non atomic because it will
> > always take effect during the next period and not in the current one.
> > If it takes effect during the current period it means that the current
> > period will be interrupted which renders your pwm useless for motors.
>
> Agreed. But some hardware, like the Atmel PWMC, have double-buffered
> registers. So they do the handoff at the end of the period
> themselves. Other "hardware", like GPIO pins, force you do to the
> handoff manually. But the PWM API looks the same to users in both
> cases.
>
> > So you are proposing an API which does not abstract from the hardware
> > capabilities and does not provide a way to query the hardware
> > capabilities. This means your motor control application will work with
> > one pwm in the background while throwing stack traces with another
> > pwm.
>
> The API provides a consistent set of capabilities to users (be they
> kernel or otherwise), regardless of what the hardware looks like. All
> hardware implementations are required to support the ability to
> control period_ticks and duty_ticks, but they may choose to implement
> that support in whatever way is dictated by the hardware.
>
> If a user (kernel or otherwise) of pwm_config() causes a stack trace,
> then it's because they didn't check the return value of that function.
> That's hardly my problem. :)
My bad. I stumbled upon the might_sleep() in the atmel pwm driver.
Indeed this does not cause a stack trace if used correctly.
>
> I'm trying to hide as few details as possible from users of
> pwm_config(), actually. If they call pwm_config_nosleep() and get an
> error, then the user can then decide for themselves whether to
> implement a kthread or whatever other way is easiest for them. I
> don't want to make those decisions myself, because then I'll get
> complaints from people calling pwm_config_nosleep() and discovering
> that the hardware reconfiguration is much later than they expected
> because of the kthread *I* have to create. I would rather just tell
> them the problem up front.
>
> Gpiolib does the same thing, but in a different way, with
> gpio_cansleep(). I don't make them ask up front, I just notify them
> after the fact when I couldn't carry out the request as they intended.
>
> > Honestly, putting a 'this call might or might not be possible' into API
> > level does not seem to be a good idea to me.
>
> You'd rather me do away with pwm_config_nosleep() entirely? That
> would be the alternative. Then nobody could use it, even on hardware
> (like Atmel PWMC) that can support it!
>
> > How about making all calls to the pwm non atomic?
>
> If you want that, just call pwm_config() all the time, and pretend
> that pwm_config_nosleep() doesn't exist. But then you can't do PWM
> updates from hrtimers or interrupt handlers.
We have threaded interrupts for this. I haven't thought about hrtimers
though.
> I'm not arguing as to
> whether someone should be doing that or not, I'm just giving them the
> capability to do so in situations where the hardware will allow it---
> and informing them when they are in situations where the hardware
> won't allow it.
>
>
> b.g.
> --
> Bill Gatliff
> bgat@billgatliff.com
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PWM v3: 1/3] PWM: Implement a generic PWM framework
2011-02-16 8:18 ` Sascha Hauer
@ 2011-02-16 17:00 ` Bill Gatliff
0 siblings, 0 replies; 11+ messages in thread
From: Bill Gatliff @ 2011-02-16 17:00 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-embedded
Sascha:
On Wed, Feb 16, 2011 at 12:18 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The discussion started with me saying that the requesting process may
> die and the kernel has no way to clean up the requested resources (pwm).
> You responded that you want to make sure that there is no resource
> conflict between the kernel and userspace. Now we are at two userspace
> processes again. We are drawing circles.
Yes, and I think part of the reason we are drawing circles is that the
whole "request" concept as I have implemented it is ill-defined. It
may be a communication/documentation issue, but it might also be an
implementation issue. It could even be that my my design decisions
which deviate from gpiolib's are magnifying flaws in the whole
concept.
I'm trying to emulate gpiolib's export/unexport mechanism, while at
the same time keeping the set of all present PWM devices visible in
/sys/class/pwm. A user process reads from ...<pwm device>/request to
"claim" a PWM device for userspace; subsequent kernel pwm_request()
calls will fail until a user process then writes to ...<pwm
device>/request again. That's all.
Multiple user processes are free to manipulate the PWM's period, duty,
and polarity states. (And due to some missing code on my part, user
processes can do that even when kernelspace is using the device. Will
fix). But kernel-side pwm_request() calls on the device will fail
until after any user process writes back to the request attribute to
signal that it is relinquishing the userspace advisory lock on the
device.
I think things would be helped immensely in what I have implemented if
I were to get rid of all mention of pids in the request interface.
Oh, and fix that thing about simultaneous kernel-user access being
permitted. :)
I'm also thinking that I might go back over to gpiolib's way of
handling userspace, which is to not have any attributes visible under
/sys/class/pwm for devices that aren't actually exported to userspace.
Do you think that would settle this issue?
> What should a userspace process do if it finds the pwm requested by
> another process (which may be already dead)? Free it and request it
> again when -f is supplied?
Ignore it, just like what happens with gpiolib. As with gpiolib, if a
pwm device is already exported to userspace then it's a free-for-all
for any user process to access.
> There are two things I really don't like about this. First is reading
> a sysfs entry should not alter the system state. Second is the kernel
> should only grant resources it can claim back when the process dies.
Except that what I'm doing isn't resource granting per se. It's
advisory locking.
> A pwm user can store the actual values for period_ns/duty_ns in his
> private data and can leave the values constant it does not want to
> change. I consider a sysfs driver a pwm being a pwm user also.
So each time a pwm device is exported to userspace, the kernel API has
to maintain cached values for period, duty, and polarity attributes
since sysfs allows for changing only one of those values at a time. I
hate cached values. :)
I would also lose the ability to judge whether a user is changing only
one of those three values, unless I consult those cached copies.
Without such information, I can't take advantage of hardware features
like PWMC's double-buffering that let me change one of those values
(but not both) without stopping the hardware.
> What multitude of arguments are you talking about? With my proposed
> version of pwm_set() a driver only has to parse three arguments.
... if you want to force driver authors to always implement the
worst-case scenario, where all three values are assumed to be
changing. If the hardware looks like Atmel's PWMC, however, then
there are opportunities to change the period or duty attributes
without stopping the hardware--- when you can tell that's all that the
user is trying to do. This fact is what motivated my design of the
pwm_config() function.
> We have threaded interrupts for this. I haven't thought about hrtimers
> though.
Yes, I'm aware of threaded interrupts. But I don't want to mandate
them in situations where the hardware doesn't require them.
b.g.
--
Bill Gatliff
bgat@billgatliff.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-02-16 17:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-10 15:53 [PWM v3: 0/3] Proposal for a generic PWM API Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 1/3] PWM: Implement a generic PWM framework Bill Gatliff
2011-02-10 20:39 ` Sascha Hauer
2011-02-11 18:22 ` Bill Gatliff
2011-02-12 5:48 ` Bill Gatliff
2011-02-14 8:03 ` Sascha Hauer
2011-02-14 16:56 ` Bill Gatliff
2011-02-16 8:18 ` Sascha Hauer
2011-02-16 17:00 ` Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 2/3] PWM: GPIO+hrtimer device emulation Bill Gatliff
2011-02-10 15:53 ` [PWM v3: 3/3] PWM: Atmel PWMC driver Bill Gatliff
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.