lkml.org 
[lkml]   [2011]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PWM v6 1/3] PWM: Implement a generic PWM framework
    On 03/06/2011 05:17 AM, Bill Gatliff wrote:
    > 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 | 10 +
    > drivers/pwm/Makefile | 4 +
    > drivers/pwm/pwm.c | 610 +++++++++++++++++++++++++++++++++++++++++++++++
    > include/linux/pwm/pwm.h | 155 ++++++++++++
    > 7 files changed, 1060 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. ;)

    Two minor remarks on the documentation: You are not mentioning the functions
    arguments and there is no documentation on the sysfs interface at all.

    > 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..bc550f7
    > --- /dev/null
    > +++ b/drivers/pwm/Kconfig
    > @@ -0,0 +1,10 @@
    > +#
    > +# PWM infrastructure and devices
    > +#
    > +
    > +menuconfig GENERIC_PWM
    > + tristate "PWM Support"
    > + 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..f958e4b
    > --- /dev/null
    > +++ b/drivers/pwm/pwm.c
    > @@ -0,0 +1,610 @@
    > +/*
    > + * 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/pwm/pwm.h>
    > +
    > +static const char *REQUEST_SYSFS = "sysfs";
    > +static LIST_HEAD(pwm_device_list);
    The list is not used.

    > +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 int __pwm_request(struct pwm_device *p, const char *label)
    > +{
    > + int ret;
    > +
    > + ret = test_and_set_bit(FLAG_REQUESTED, &p->flags);
    > + if (ret) {
    > + ret = -EBUSY;
    > + goto done;
    > + }
    > +
    > + p->label = label;
    > +
    > + if (p->ops->request) {
    > + ret = p->ops->request(p);
    > + if (ret) {
    > + clear_bit(FLAG_REQUESTED, &p->flags);
    > + goto done;
    > + }
    > + }
    > +
    > +done:
    > + return ret;
    > +}
    > +
    > +static struct pwm_device *__pwm_request_byname(const char *name,
    > + const char *label)
    > +{
    > + struct device *d;
    > + struct pwm_device *p;
    > + int ret;
    > +
    > + d = class_find_device(&pwm_class, NULL, (char*)name, pwm_match_name);
    > + if (IS_ERR_OR_NULL(d))
    > + return ERR_PTR(-EINVAL);
    > +
    > + p = dev_get_drvdata(d);
    > + ret = __pwm_request(p, label);
    > +
    > + if (ret)
    > + return ERR_PTR(ret);
    > + 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);

    sizeof(name)

    > + 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)) {
    > + WARN(1, "%s: releasing unrequested PWM device %s\n",
    > + __func__, dev_name(p->dev));
    > + goto done;
    > + }
    > +
    > + pwm_stop(p);
    > + pwm_unsynchronize(p, NULL);
    > + pwm_set_handler(p, NULL, NULL);
    > +
    > + p->label = NULL;
    > +
    > + if (p->ops->release)
    > + p->ops->release(p);
    > +done:
    > + mutex_unlock(&device_list_mutex);
    > +}
    > +EXPORT_SYMBOL(pwm_release);
    > +
    > +static 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;
    > +}
    > +
    > +static 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;
    > +}
    > +
    > +int pwm_config_nosleep(struct pwm_device *p, struct pwm_config *c)
    > +{
    > + if (!p->ops->config_nosleep)
    > + return -EINVAL;

    Would ENOSYS be more appropriate?

    > +
    > + 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;
    > +
    > + 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\n",
    > + __func__, c->config_mask, c->period_ticks,
    > + c->duty_ticks, c->polarity);
    > +
    > + if (ret)
    > + return ret;

    This looks a bit confusing. Maybe it would be better to move the dev_dbg at the
    top of the function and the 'err' label at the end of the function.


    > + return p->ops->config(p, c);
    > +}
    > +EXPORT_SYMBOL(pwm_config);
    > +
    > +int pwm_set(struct pwm_device *p, unsigned long period_ns,
    > + unsigned long duty_ns, int active_high)
    > +{
    > + struct pwm_config c = {
    > + .config_mask = (BIT(PWM_CONFIG_PERIOD_TICKS)
    > + | BIT(PWM_CONFIG_DUTY_TICKS)
    > + | BIT(PWM_CONFIG_POLARITY)),
    > + .period_ticks = pwm_ns_to_ticks(p, period_ns),
    > + .duty_ticks = pwm_ns_to_ticks(p, duty_ns),
    > + .polarity = active_high
    > + };
    > +
    > + return pwm_config(p, &c);
    > +}
    > +EXPORT_SYMBOL(pwm_set);
    > +
    > +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_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;
    ENOSYS here too
    > +
    > + 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;
    and here.
    > +
    > + 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);
    > +}
    > +
    > +void pwm_callback(struct pwm_device *p)
    > +{
    > + queue_work(pwm_handler_workqueue, &p->handler_work);
    > +}
    > +EXPORT_SYMBOL(pwm_callback);
    > +
    > +int pwm_set_handler(struct pwm_device *p, pwm_handler_t handler, void *data)
    > +{
    > + struct pwm_config c;
    > + int ret;
    > +
    > + if (handler)
    > + c.config_mask = BIT(PWM_CONFIG_ENABLE_CALLBACK);
    > + else
    > + c.config_mask = BIT(PWM_CONFIG_DISABLE_CALLBACK);
    > +
    > + ret = pwm_config(p, &c);

    There is a chance for a race here. The device could fire, for example due to an
    irq, before the handler has been initalized.

    > +
    > + if (!ret && handler) {
    > + p->handler_data = data;
    > + p->handler = handler;
    > + INIT_WORK(&p->handler_work, pwm_handler);

    The INIT_WORK should probably into pwm_device_register.

    > + }
    > +
    > + return ret;
    > +}
    > +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 (!pwm_is_exported(p))
    > + return -EINVAL;

    I'm not sure, but maybe -EPERM is better here.

    > +
    > + if (sysfs_streq(buf, "1"))
    > + pwm_start(p);
    > + else if (sysfs_streq(buf, "0"))
    > + pwm_stop(p);
    Maybe return -EINVAL if it is neither 0 or 1.

    > +
    > + 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 (!pwm_is_exported(p))
    > + return -EINVAL;
    > +
    > + if (!strict_strtoul(buf, 10, &duty_ns))
    > + pwm_set_duty_ns(p, duty_ns);

    How about returning the error value of strict_strtou if it fails?

    > + 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 (!pwm_is_exported(p))
    > + return -EINVAL;
    > +
    > + 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 (!pwm_is_exported(p))
    > + return -EINVAL;
    > +
    > + 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);
    > + int ret;
    > +
    > + mutex_lock(&device_list_mutex);
    > + ret = __pwm_request(p, REQUEST_SYSFS);
    > + mutex_unlock(&device_list_mutex);
    > +
    > + if (!ret)
    > + set_bit(FLAG_EXPORTED, &p->flags);
    > +
    > + return sprintf(buf, "%s\n", p->label);
    > +}
    > +
    > +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);
    > + clear_bit(FLAG_EXPORTED, &p->flags);
    > + return len;
    > +}
    > +static DEVICE_ATTR(request, S_IRUGO | S_IWUSR, pwm_request_show, pwm_request_store);

    So `cat request` requests the device and `echo > request` frees the device?
    Thats a bit odd in my opinion.

    > +
    > +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,

    Just leaving it NULL should work, if you don't want any class attributes.

    > +};
    > +
    > +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;
    > + }
    device_create should fail if a device with the same name already exits, so I'm
    not sure if it makes sense to do the check here.

    > +
    > + p->dev = device_create(&pwm_class, parent, MKDEV(0, 0), NULL, name);
    ... NULL, "%s", name);

    Or you could use device_create_vargs and get rid of that scnprintf in pwm_register.

    > + if (IS_ERR(p->dev)) {
    > + ret = PTR_ERR(p->dev);
    > + goto err_device_create;
    > + }
    I think it would be better to embedd the device struct directly into the
    pwm_device struct. You could also remove the data field of the pwm_device
    struct and use dev_{get,set}_drvdata for pwm_{get,set}_drvdata.


    > +
    > + ret = sysfs_create_group(&p->dev->kobj, &pwm_device_attr_group);
    > + if (ret)
    > + goto err_create_group;

    It should be possible to use the classes dev_attrs here instead.


    > +
    > + 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)) {

    Is it possible that the pwm is running but not requested?


    > + 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)
    > +{
    > + pwm_handler_workqueue = create_singlethread_workqueue("pwm");
    > + if (IS_ERR_OR_NULL(pwm_handler_workqueue)) {
    > + pr_err("%s: failed to create PWM workqueue; aborting\n",
    > + __func__);
    > + return -ENOMEM;
    > + }
    > +
    > + return class_register(&pwm_class);
    > +}
    > +
    > +static void __exit pwm_exit(void)
    > +{
    > + destroy_workqueue(pwm_handler_workqueue);
    > + class_unregister(&pwm_class);
    > +}
    > +
    > +postcore_initcall(pwm_init);
    > +module_exit(pwm_exit);
    > +MODULE_LICENSE("GPL");
    > +MODULE_AUTHOR("Bill Gatliff <bgat@billgatliff.com>");
    > +MODULE_DESCRIPTION("Generic PWM device API implementation");
    > diff --git a/include/linux/pwm/pwm.h b/include/linux/pwm/pwm.h
    > new file mode 100644
    > index 0000000..1447333
    > --- /dev/null
    > +++ b/include/linux/pwm/pwm.h
    > @@ -0,0 +1,155 @@
    > +/*
    > + * 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,
    Does the REGISTERED flag makes any sense, I mean are there any usecases for
    pwm_is_registered? Non registered pwm_devices should not be visible to the
    outside world, so if you got an pointer to an pwm_device outside of an
    pwm_device driver it should also be registered. And inside a pwm_device driver
    the driver should know whether it already registered the pwm_device or not.
    > + FLAG_REQUESTED = 1,
    > + FLAG_STOP = 2,
    > + FLAG_RUNNING = 3,
    > + FLAG_EXPORTED = 4,
    > +};
    > +
    > +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_ENABLE_CALLBACK = 9,
    > + PWM_CONFIG_DISABLE_CALLBACK = 10,
    > +};
    > +
    > +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);
    > +};
    > +
    > +struct pwm_config {
    > + unsigned long config_mask;
    > +
    > + unsigned long duty_ticks;
    > + unsigned long period_ticks;
    > + int polarity;
    > +};
    > +
    > +struct pwm_device {
    > + struct device *dev;
    > + const struct pwm_device_ops *ops;
    > +
    > + const void *data;
    > +
    > + const char *label;
    > +
    > + unsigned long flags;
    > +
    > + unsigned long tick_hz;
    > +
    > + struct work_struct handler_work;
    > + pwm_handler_t handler;
    > + void *handler_data;
    > +
    > + int active_high;
    In the config you call it polarity, here you call it active_high. A consistent
    naming would be better in my opinion.

    > + 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(const struct pwm_device *p)
    > +{
    > + return test_bit(FLAG_REGISTERED, &p->flags);
    > +}
    > +
    > +static inline int pwm_is_requested(const struct pwm_device *p)
    > +{
    > + return test_bit(FLAG_REQUESTED, &p->flags);
    > +}
    > +
    > +static inline int pwm_is_running(const struct pwm_device *p)
    > +{
    > + return test_bit(FLAG_RUNNING, &p->flags);
    > +}
    > +
    > +static inline int pwm_is_exported(const struct pwm_device *p)
    > +{
    > + return test_bit(FLAG_EXPORTED, &p->flags);
    > +}
    > +
    > +static inline void pwm_set_drvdata(struct pwm_device *p, const void *data)
    > +{
    > + p->data = data;
    > +}
    > +
    > +static inline void *pwm_get_drvdata(const struct pwm_device *p)
    > +{
    > + return (void*)p->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);
    > +
    > +int pwm_set(struct pwm_device *p, unsigned long period_ns,
    > + unsigned long duty_ns, int active_high);
    > +
    > +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_polarity(struct pwm_device *p, int active_high);
    > +
    > +int pwm_start(struct pwm_device *p);
    > +int pwm_stop(struct pwm_device *p);
    > +
    > +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_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);
    > +
    > +void pwm_callback(struct pwm_device *p);
    > +
    > +struct pwm_device *gpio_pwm_create(int gpio);
    > +int gpio_pwm_destroy(struct pwm_device *p);

    These two definitions probably belong to the second patch.

    > +
    > +#endif



    \
     
     \ /
      Last update: 2011-03-06 12:07    [W:0.137 / U:0.372 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site