[PATCH v3] implement a generic PWM framework

[PATCH v3] implement a generic PWM framework

[Sascha Hauer]

3rd version. No changes to the core except that I added F: drivers/pwm/ to
MAINTAINERS. Arnd, I looked through the comments to the first series again
and this was the only thing I could find I forgot. You referred to three
things. Is there more I forgot?

What changed though is the mxs pwm driver. I now register a pwm core
device which handles the shared enable register as suggested by Arnd.

Sascha

The following changes since commit b0af8dfdd67699e25083478c63eedef2e72ebd85:

  Linux 3.0-rc5 (2011-06-27 19:12:22 -0700)

are available in the git repository at:
  git://git.pengutronix.de/git/imx/linux-2.6.git pwm

Sascha Hauer (3):
      PWM: add pwm framework support
      ARM mxs: adjust pwm resources to what the driver expects
      pwm: Add a i.MX23/28 pwm driver

 Documentation/pwm.txt                        |   56 +++++
 MAINTAINERS                                  |    6 +
 arch/arm/mach-mxs/devices/platform-mxs-pwm.c |   32 +++-
 drivers/Kconfig                              |    2 +
 drivers/Makefile                             |    1 +
 drivers/pwm/Kconfig                          |   16 ++
 drivers/pwm/Makefile                         |    2 +
 drivers/pwm/core.c                           |  220 ++++++++++++++++++
 drivers/pwm/mxs-pwm.c                        |  312 ++++++++++++++++++++++++++
 include/linux/pwm.h                          |   37 +++
 10 files changed, 681 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/core.c
 create mode 100644 drivers/pwm/mxs-pwm.c

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

This patch adds framework support for PWM (pulse width modulation) devices.

The is a barebone PWM API already in the kernel under include/linux/pwm.h,
but it does not allow for multiple drivers as each of them implements the
pwm_*() functions.

There are other PWM framework patches around from Bill Gatliff. Unlike
his framework this one does not change the existing API for PWMs so that
this framework can act as a drop in replacement for the existing API.

Why another framework?

Several people argue that there should not be another framework for PWMs
but they should be integrated into one of the existing frameworks like led
or hwmon. Unlike these frameworks the PWM framework is agnostic to the
purpose of the PWM. In fact, a PWM can drive a LED, but this makes the
LED framework a user of a PWM, like already done in leds-pwm.c. The gpio
framework also is not suitable for PWMs. Every gpio could be turned into
a PWM using timer based toggling, but on the other hand not every PWM hardware
device can be turned into a gpio due to the lack of hardware capabilities.

This patch does not try to improve the PWM API yet, this could be done in
subsequent patches.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Matthias Kaehlcke <matthias@kaehlcke.net>
---
 Documentation/pwm.txt |   56 +++++++++++++
 MAINTAINERS           |    6 ++
 drivers/Kconfig       |    2 +
 drivers/Makefile      |    1 +
 drivers/pwm/Kconfig   |   12 +++
 drivers/pwm/Makefile  |    1 +
 drivers/pwm/core.c    |  220 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h   |   37 ++++++++
 8 files changed, 335 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/core.c

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
new file mode 100644
index 0000000..c7c5cb1
--- /dev/null
+++ b/Documentation/pwm.txt
@@ -0,0 +1,56 @@
+Pulse Width Modulation (PWM) interface
+
+This provides an overview about the Linux PWM interface
+
+PWMs are commonly used for controlling LEDs, fans or vibrators in
+cell phones. PWMs with a fixed purpose have no need implementing
+the Linux PWM API (although they could). However, PWMs are often
+found as discrete devices on SoCs which have no fixed purpose. It's
+up to the board designer to connect them to LEDs or fans. To provide
+this kind of flexibility the generic PWM API exists.
+
+Identifying PWMs
+----------------
+
+PWMs are identified by unique ids throughout the system. A platform
+should call pwmchip_reserve() during init time to reserve the id range
+for internal PWMs so that users have a fixed id to refer to specific
+PWMs.
+
+Using PWMs
+----------
+
+A PWM can be requested using pwm_request() and freed after usage with
+pwm_free(). After being requested a PWM has to be configured using
+
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
+
+To start/stop toggling the PWM output use pwm_enable()/pwm_disable().
+
+Implementing a PWM driver
+-------------------------
+
+Currently there are two ways to implement pwm drivers. Traditionally
+there only has been the barebone API meaning that each driver has
+to implement the pwm_*() functions itself. This means that it's impossible
+to have multiple PWM drivers in the system. For this reason it's mandatory
+for new drivers to use the generic PWM framework.
+A new PWM device can be added using pwmchip_add() and removed again with
+pwmchip_remove(). pwmchip_add() takes a filled in struct pwm_chip as
+argument which provides the ops and the pwm id to the framework.
+
+Locking
+-------
+
+The PWM core list manipulations are protected by a mutex, so pwm_request()
+and pwm_free() may not be called from an atomic context. Currently the
+PWM core does not enforce any locking to pwm_enable(), pwm_disable() and
+pwm_config(), so the calling context is currently driver specific. This
+is an issue derived from the former barebone API and should be fixed soon.
+
+Helpers
+-------
+
+Currently a PWM can only be configured with period_ns and duty_ns. For several
+use cases freq_hz and duty_percent might be better. Instead of calculating 
+this in your driver please consider adding appropriate helpers to the framework.
diff --git a/MAINTAINERS b/MAINTAINERS
index f0358cd..79c047b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5142,6 +5142,12 @@ M:	Robert Jarzmik <robert.jarzmik@free.fr>
 L:	rtc-linux at googlegroups.com
 S:	Maintained
 
+PWM core
+M:	Sascha Hauer <s.hauer@pengutronix.de>
+L:	linux-kernel at vger.kernel.org
+S:	Maintained
+F:	drivers/pwm/
+
 QLOGIC QLA1280 SCSI DRIVER
 M:	Michael Reed <mdr@sgi.com>
 L:	linux-scsi at vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 3bb154d..67f5c27 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -126,4 +126,6 @@ source "drivers/hwspinlock/Kconfig"
 
 source "drivers/clocksource/Kconfig"
 
+source "drivers/pwm/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 09f3232..c321763 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -6,6 +6,7 @@
 #
 
 obj-y				+= gpio/
+obj-y				+= pwm/
 obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 0000000..93c1052
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,12 @@
+menuconfig PWM
+	bool "PWM Support"
+	help
+	  This enables PWM support through the generic PWM framework.
+	  You only need to enable this, if you also want to enable
+	  one or more of the PWM drivers below.
+
+	  If unsure, say N.
+
+if PWM
+
+endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 0000000..3469c3d
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PWM)		+= core.o
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
new file mode 100644
index 0000000..71de479
--- /dev/null
+++ b/drivers/pwm/core.c
@@ -0,0 +1,220 @@
+/*
+ * Generic pwmlib implementation
+ *
+ * Copyright (C) 2011 Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2, or (at your option)
+ *  any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; see the file COPYING.  If not, write to
+ *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/module.h>
+#include <linux/pwm.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+
+struct pwm_device {
+	struct			pwm_chip *chip;
+	const char		*label;
+	unsigned long		flags;
+#define FLAG_REQUESTED	0
+#define FLAG_ENABLED	1
+	struct list_head	node;
+};
+
+static LIST_HEAD(pwm_list);
+
+static DEFINE_MUTEX(pwm_lock);
+
+static struct pwm_device *_find_pwm(int pwm_id)
+{
+	struct pwm_device *pwm;
+
+	list_for_each_entry(pwm, &pwm_list, node) {
+		if (pwm->chip->pwm_id == pwm_id)
+			return pwm;
+	}
+
+	return NULL;
+}
+
+/**
+ * pwmchip_add() - register a new pwm
+ * @chip: the pwm
+ *
+ * register a new pwm. pwm->pwm_id must be initialized. if pwm_id < 0 then
+ * a dynamically assigned id will be used, otherwise the id specified,
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	struct pwm_device *pwm;
+	int ret = 0;
+
+	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->chip = chip;
+
+	mutex_lock(&pwm_lock);
+
+	if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	list_add_tail(&pwm->node, &pwm_list);
+out:
+	mutex_unlock(&pwm_lock);
+
+	if (ret)
+		kfree(pwm);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_add);
+
+/**
+ * pwmchip_remove() - remove a pwm
+ * @chip: the pwm
+ *
+ * remove a pwm. This function may return busy if the pwm is still requested.
+ */
+int pwmchip_remove(struct pwm_chip *chip)
+{
+	struct pwm_device *pwm;
+	int ret = 0;
+
+	mutex_lock(&pwm_lock);
+
+	pwm = _find_pwm(chip->pwm_id);
+	if (!pwm) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	list_del(&pwm->node);
+
+	kfree(pwm);
+out:
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_remove);
+
+/*
+ * pwm_request - request a PWM device
+ */
+struct pwm_device *pwm_request(int pwm_id, const char *label)
+{
+	struct pwm_device *pwm;
+	int ret;
+
+	mutex_lock(&pwm_lock);
+
+	pwm = _find_pwm(pwm_id);
+	if (!pwm) {
+		pwm = ERR_PTR(-ENOENT);
+		goto out;
+	}
+
+	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
+		pwm = ERR_PTR(-EBUSY);
+		goto out;
+	}
+
+	if (!try_module_get(pwm->chip->ops->owner)) {
+		pwm = ERR_PTR(-ENODEV);
+		goto out;
+	}
+
+	if (pwm->chip->ops->request) {
+		ret = pwm->chip->ops->request(pwm->chip);
+		if (ret) {
+			pwm = ERR_PTR(ret);
+			goto out_put;
+		}
+	}
+
+	pwm->label = label;
+	set_bit(FLAG_REQUESTED, &pwm->flags);
+
+	goto out;
+
+out_put:
+	module_put(pwm->chip->ops->owner);
+out:
+	mutex_unlock(&pwm_lock);
+
+	return pwm;
+}
+EXPORT_SYMBOL_GPL(pwm_request);
+
+/*
+ * pwm_free - free a PWM device
+ */
+void pwm_free(struct pwm_device *pwm)
+{
+	mutex_lock(&pwm_lock);
+
+	if (!test_and_clear_bit(FLAG_REQUESTED, &pwm->flags)) {
+		pr_warning("PWM device already freed\n");
+		goto out;
+	}
+
+	pwm->label = NULL;
+
+	module_put(pwm->chip->ops->owner);
+out:
+	mutex_unlock(&pwm_lock);
+}
+EXPORT_SYMBOL_GPL(pwm_free);
+
+/*
+ * pwm_config - change a PWM device configuration
+ */
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+	return pwm->chip->ops->config(pwm->chip, duty_ns, period_ns);
+}
+EXPORT_SYMBOL_GPL(pwm_config);
+
+/*
+ * pwm_enable - start a PWM output toggling
+ */
+int pwm_enable(struct pwm_device *pwm)
+{
+	if (!test_and_set_bit(FLAG_ENABLED, &pwm->flags))
+		return pwm->chip->ops->enable(pwm->chip);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwm_enable);
+
+/*
+ * pwm_disable - stop a PWM output toggling
+ */
+void pwm_disable(struct pwm_device *pwm)
+{
+	if (test_and_clear_bit(FLAG_ENABLED, &pwm->flags))
+		pwm->chip->ops->disable(pwm->chip);
+}
+EXPORT_SYMBOL_GPL(pwm_disable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 7c77575..df9681b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -28,4 +28,41 @@ int pwm_enable(struct pwm_device *pwm);
  */
 void pwm_disable(struct pwm_device *pwm);
 
+#ifdef CONFIG_PWM
+struct pwm_chip;
+
+/**
+ * struct pwm_ops - PWM operations
+ * @request: optional hook for requesting a PWM
+ * @free: optional hook for freeing a PWM
+ * @config: configure duty cycles and period length for this PWM
+ * @enable: enable PWM output toggling
+ * @disable: disable PWM output toggling
+ */
+struct pwm_ops {
+	int			(*request)(struct pwm_chip *chip);
+	void			(*free)(struct pwm_chip *chip);
+	int			(*config)(struct pwm_chip *chip, int duty_ns,
+						int period_ns);
+	int			(*enable)(struct pwm_chip *chip);
+	void			(*disable)(struct pwm_chip *chip);
+	struct module		*owner;
+};
+
+/**
+ * struct pwm_chip - abstract a PWM
+ * @label: for diagnostics
+ * @owner: helps prevent removal of modules exporting active PWMs
+ * @ops: The callbacks for this PWM
+ */
+struct pwm_chip {
+	int			pwm_id;
+	const char		*label;
+	struct pwm_ops		*ops;
+};
+
+int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_remove(struct pwm_chip *chip);
+#endif
+
 #endif /* __LINUX_PWM_H */
-- 
1.7.5.3

[PATCH 1/3] PWM: add pwm framework support

[Bill Gatliff]

Sascha:


I'm completely against this effort.

The objections to my submission weren't ever because I was changing
the user-visible API, so I don't think you can claim any advantage to
mine in that regard.

I will take some blame for not getting my API finished, but I have
been fighting some serious non-work issues that have consumed nearly
all of my available time--- including the professional time I would
otherwise have to invest in getting my code finished.

Is there the possibility that we could cooperate to get my patches
finished, rather than discarding and reinventing them completely?



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

[PATCH 1/3] PWM: add pwm framework support

[Arnd Bergmann]

On Thursday 30 June 2011, Bill Gatliff wrote:
> I'm completely against this effort.
> 
> The objections to my submission weren't ever because I was changing
> the user-visible API, so I don't think you can claim any advantage to
> mine in that regard.
> 
> I will take some blame for not getting my API finished, but I have
> been fighting some serious non-work issues that have consumed nearly
> all of my available time--- including the professional time I would
> otherwise have to invest in getting my code finished.
> 
> Is there the possibility that we could cooperate to get my patches
> finished, rather than discarding and reinventing them completely?

I've looked at your patches again, and it seems that you are doing
two distinct changes, both good:

1. You provide a generic framework for pwm drivers that makes it
possible for multiple drivers to coexist and simplifies the way
that these drivers interact with the core OS.

2. You extend and fix a number of aspects in the global PWM API.

Sascha's patch does only part 1, not part 2, but I don't think that
makes his patches any worse. The introduction of the framework
now is very similar to what you had suggested, and you should
probably be mentioned in the changelog, even though the two
implementations were done independently.

A lot of people want to see a framework get merged, and I think it's
great that Sascha has volunteered to do the work to push that
through this time, especially since you have not been able to
finish your work.

What I think would be the best plan forward is to merge Sascha's
patches as soon as we can, then get all currently existing pwm
drivers converted to that and moved to drivers/pwm, and finally
do the interface changes that you have proposed earlier.

I would also hope that you can give constructive feedback to
the submission and point out potential problems that you see
where the code should be changed now in order to make your
interface changes more easy later.

I realize that it's annoying to spend a lot of time on a specific
implementation and then see competing code get merged. Unfortunately,
this happens all the time, and the code we merge is often not
the one that has had the most effort spent on it, but the one that
looks most promising at the time when it gets merged.

	Arnd

[PATCH 1/3] PWM: add pwm framework support

[Bill Gatliff]

Guys:

On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> A lot of people want to see a framework get merged, and I think it's
> great that Sascha has volunteered to do the work to push that
> through this time, especially since you have not been able to
> finish your work.

Sascha is wasting his time by reinventing the wheel.  He's traveling
over exactly the same path I have already covered.  In fact, some of
his reviewer comments are almost word-for-word the same as those I
have received and addressed in the past.

My patches were always kept current in this mailing list and others,
and Sascha clearly has the skills necessary to make improvements and
corrections should he have chosen to do so.

> What I think would be the best plan forward is to merge Sascha's
> patches as soon as we can, then get all currently existing pwm
> drivers converted to that and moved to drivers/pwm, and finally
> do the interface changes that you have proposed earlier.

That's a real duplication of work.  That makes no sense.

> I would also hope that you can give constructive feedback to
> the submission and point out potential problems that you see
> where the code should be changed now in order to make your
> interface changes more easy later.

My code has already moved past that point.  And if I had the time to
evaluate and improve Sascha's patches, I would have the time to finish
my own.  Sascha is more than welcome to apply his efforts to the
preexisting PWM API patches already present in the LKML archives.

> I realize that it's annoying to spend a lot of time on a specific
> implementation and then see competing code get merged.

Annoyed isn't the word you are looking for.

My code has been reviewed, tested and actively used as posted in LKML
by several reviewers (including myself) in actual hardware.  We are
consciously choosing to discard a known entity and restart the whole
process with new code.  That's a waste of everyone's time and risk
exposure.

I don't consider Sascha's code to be "competing" with mine, as
apparently nobody has bothered to consider one against the other.


> Unfortunately,
> this happens all the time, and the code we merge is often not
> the one that has had the most effort spent on it, but the one that
> looks most promising at the time when it gets merged.

Your definition of "promsing" apparently correlates with "new and shiny".


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

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

Bill,

On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
> Guys:
> 
> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > A lot of people want to see a framework get merged, and I think it's
> > great that Sascha has volunteered to do the work to push that
> > through this time, especially since you have not been able to
> > finish your work.
> 
> Sascha is wasting his time by reinventing the wheel.  He's traveling
> over exactly the same path I have already covered.  In fact, some of
> his reviewer comments are almost word-for-word the same as those I
> have received and addressed in the past.
> 
> My patches were always kept current in this mailing list and others,
> and Sascha clearly has the skills necessary to make improvements and
> corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the
existing pwm API and its users. With a competing api we are basically
stuck. We can't convert the existing hardware drivers to the new API
because leds-pwm.c, pwm_bl.c and others still depend on the old API and
boards using it would break. We can't convert the function drivers
either because again this would break boards for which only an old style
pwm driver exists.  So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API
atomically so that nothing breaks. Your patches don't do this, so I
don't think at all that what I did is duplication of work.

Given the current rush to move drivers out of arch/ it probably won't
take long until all pwm drivers are moved to drivers/pwm/ and converted
to use the framework, and then you have a good base to put your work onto.
So please don't complain too much: We are currently only doing the work
you didn't want to do.


Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/3] PWM: add pwm framework support

[Bill Gatliff]

Sascha:

On Thu, Jun 30, 2011 at 12:02 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> So please don't complain too much: We are currently only doing the work
> you didn't want to do.

Per request of some the early reviewers, my plan was to migrate
existing users over to the new API after the API itself was committed
to mainstream.

At any rate, this isn't a question of work I didn't "want" to do.  On
the one hand, I was simply doing what was asked of me.  On the other
hand, I was trying to make progress while simultaneously dealing with
serious family matters that are still unresolved.

At any rate, it's clear that your patches are going in, and mine
aren't--- I have no choice but to accept that.  I'm just frustrated by
the duplication of effort.


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

[PATCH 1/3] PWM: add pwm framework support

[Ryan Mallon]

On 01/07/11 03:02, Sascha Hauer wrote:
> Bill,
>
> On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
>> Guys:
>>
>> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann<arnd@arndb.de>  wrote:
>>
>>> A lot of people want to see a framework get merged, and I think it's
>>> great that Sascha has volunteered to do the work to push that
>>> through this time, especially since you have not been able to
>>> finish your work.
>> Sascha is wasting his time by reinventing the wheel.  He's traveling
>> over exactly the same path I have already covered.  In fact, some of
>> his reviewer comments are almost word-for-word the same as those I
>> have received and addressed in the past.
>>
>> My patches were always kept current in this mailing list and others,
>> and Sascha clearly has the skills necessary to make improvements and
>> corrections should he have chosen to do so.
> I think that you made the fundamental mistake to completly ignore the
> existing pwm API and its users. With a competing api we are basically
> stuck. We can't convert the existing hardware drivers to the new API
> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
> boards using it would break.

I don't think this is really a blocker to Bill's patches. There are 
three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with 
Bill's patches. Bill already provided a leds-pwm driver.

There is also the interesting case of the Atmel pwm driver, which does 
not fit the current pwm api and has its own backlight and leds drivers. 
Bill's patches addressed this, Sasha's patches do not. If we merge 
Sasha's patches then we are going to be in the same position as Bill's 
patches at some point in that we need to change the pwm api (and all its 
users) to meet the needs of the Atmel (and any similar hardware) pwm device.

The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the 
current api (though it could), but instead provides a sysfs interface to 
user-space. Again, this was addressed by Bill's patches.

> We can't convert the function drivers
> either because again this would break boards for which only an old style
> pwm driver exists.  So the logical thing to do is to put a step in
> between: Consolidate the existing drivers and *then* change the API
> atomically so that nothing breaks. Your patches don't do this, so I
> don't think at all that what I did is duplication of work.

You have to modify the drivers anyway match the new pwm core. The Atmel 
and ep93xx drivers are going to be difficult to merge into the new api, 
and seeing as there are only about seven pwm drivers total in the kernel 
I think its a significant portion. Any pwm api patchset could easily 
convert all of the existing pwm drivers without becoming overly massive.

If we merge Sasha's api, then we can move most of the existing drivers 
and maybe add some new ones, but we will still have the unconsolidated 
outliers. When (if?) we try to fix those we will probably need to change 
the pwm api and therefore all of the drivers to. So its basically a case 
of do the effort now (Bill's patches) or do it later. Doing it later 
will probably require more effort.

> Given the current rush to move drivers out of arch/ it probably won't
> take long until all pwm drivers are moved to drivers/pwm/ and converted
> to use the framework, and then you have a good base to put your work onto.
> So please don't complain too much: We are currently only doing the work
> you didn't want to do.
>

You can move all of the drivers out of arch now if you want. You just 
need to make sure you are only compiling one of them in. The real job in 
consolidating means making sure that the api meets the needs of all of 
the drivers. The in kernel Atmel pwm driver at least is not going to 
convert easily to this api.

Also, please not my change of email address for future emails.

Thanks,
~Ryan

[PATCH 1/3] PWM: add pwm framework support

[H Hartley Sweeten]

On Thursday, June 30, 2011 4:24 PM, Ryan Mallon wrote:
> On 01/07/11 03:02, Sascha Hauer wrote:
>> I think that you made the fundamental mistake to completly ignore the
>> existing pwm API and its users. With a competing api we are basically
>> stuck. We can't convert the existing hardware drivers to the new API
>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
>> boards using it would break.
>
> I don't think this is really a blocker to Bill's patches. There are 
> three (that I can see) pwm users currently:
> drivers/video/backlight/pwm_bl.c
> drivers/leds/leds-pwm.c
> drivers/input/misc/pwm-beeper.c
>
> All of those drivers are trivial and good easily be updated to work with 
> Bill's patches. Bill already provided a leds-pwm driver.

I agree with Ryan.  The three drivers listed above appear to be the only
pwm "user" drivers.  They are all trivial and could be updated easily.

The "core" drivers using the existing API appear to be:
arch/arm/mach-vt8500/pwm.c
arch/arm/plat-mxc/pwm.c
arch/arm/plat-pxa/pwm.c
arch/arm/plat-samsung/pwm.c
arch/blackfin/kernel/pwm.c
arch/mips/jz4740/pwm.c
arch/unicore32/kernel/pwm.c
drivers/misc/ab8500-pwm.c
drivers/mfd/twl6030-pwm.c

> There is also the interesting case of the Atmel pwm driver, which does 
> not fit the current pwm api and has its own backlight and leds drivers. 
> Bill's patches addressed this, Sasha's patches do not. If we merge 
> Sasha's patches then we are going to be in the same position as Bill's 
> patches at some point in that we need to change the pwm api (and all its 
> users) to meet the needs of the Atmel (and any similar hardware) pwm device.
> 
> The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the 
> current api (though it could), but instead provides a sysfs interface to 
> user-space. Again, this was addressed by Bill's patches.

I modified the ep93xx pwm driver previously to work with Bill's patches.
The use for that driver required user-space control of the pwm.  Bill's
patches handle that.

>> We can't convert the function drivers
>> either because again this would break boards for which only an old style
>> pwm driver exists.  So the logical thing to do is to put a step in
>> between: Consolidate the existing drivers and *then* change the API
>> atomically so that nothing breaks. Your patches don't do this, so I
>> don't think at all that what I did is duplication of work.
>
> You have to modify the drivers anyway match the new pwm core. The Atmel 
> and ep93xx drivers are going to be difficult to merge into the new api, 
> and seeing as there are only about seven pwm drivers total in the kernel 
> I think its a significant portion. Any pwm api patchset could easily 
> convert all of the existing pwm drivers without becoming overly massive.
>
> If we merge Sasha's api, then we can move most of the existing drivers 
> and maybe add some new ones, but we will still have the unconsolidated 
> outliers. When (if?) we try to fix those we will probably need to change 
> the pwm api and therefore all of the drivers to. So its basically a case 
> of do the effort now (Bill's patches) or do it later. Doing it later 
> will probably require more effort.
>
>> Given the current rush to move drivers out of arch/ it probably won't
>> take long until all pwm drivers are moved to drivers/pwm/ and converted
>> to use the framework, and then you have a good base to put your work onto.
>> So please don't complain too much: We are currently only doing the work
>> you didn't want to do.
>>
> You can move all of the drivers out of arch now if you want. You just 
> need to make sure you are only compiling one of them in. The real job in 
> consolidating means making sure that the api meets the needs of all of 
> the drivers. The in kernel Atmel pwm driver at least is not going to 
> convert easily to this api.

I think the only open issue with Bill's patches was where the pwm subsystem
belongs.  Some people thought it should be part of gpiolib and others
thought it should be on it's own.  The core library Bill presented does
share some commonality with gpiolib but I feel it still represents it's
own subsystem.  On-chip pwms might also be gpio capable but this is not
always the case.

With Bill's patches we could run into resource conflicts between gpio and
pwm but that really exists now anyway.  The pinmux subsystem that Linus
Walleij (cc'ed) has been proposing should fix that.

Overall I think Bill's patches will get us to the desired end result quicker
and save a lot of needless churn.

Regards,
Hartley

[PATCH 1/3] PWM: add pwm framework support

[Mike Frysinger]

On Thu, Jun 30, 2011 at 20:33, H Hartley Sweeten wrote:
> On Thursday, June 30, 2011 4:24 PM, Ryan Mallon wrote:
>> On 01/07/11 03:02, Sascha Hauer wrote:
>>> I think that you made the fundamental mistake to completly ignore the
>>> existing pwm API and its users. With a competing api we are basically
>>> stuck. We can't convert the existing hardware drivers to the new API
>>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
>>> boards using it would break.
>>
>> I don't think this is really a blocker to Bill's patches. There are
>> three (that I can see) pwm users currently:
>> drivers/video/backlight/pwm_bl.c
>> drivers/leds/leds-pwm.c
>> drivers/input/misc/pwm-beeper.c
>>
>> All of those drivers are trivial and good easily be updated to work with
>> Bill's patches. Bill already provided a leds-pwm driver.
>
> I agree with Ryan. ?The three drivers listed above appear to be the only
> pwm "user" drivers. ?They are all trivial and could be updated easily.

indeed ... the current pwm API is almost toy like in its simplicity.

> The "core" drivers using the existing API appear to be:
> arch/blackfin/kernel/pwm.c

i only wrote this because Bill's subsystem wasnt yet merged.  but i
made sure that it was done in such a way that it's trivial to convert
over to Bill's work once it's done.
-mike

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

On Fri, Jul 01, 2011 at 09:24:05AM +1000, Ryan Mallon wrote:
> On 01/07/11 03:02, Sascha Hauer wrote:
> >Bill,
> >
> >On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
> >>Guys:
> >>
> >>On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann<arnd@arndb.de>  wrote:
> >>
> >>>A lot of people want to see a framework get merged, and I think it's
> >>>great that Sascha has volunteered to do the work to push that
> >>>through this time, especially since you have not been able to
> >>>finish your work.
> >>Sascha is wasting his time by reinventing the wheel.  He's traveling
> >>over exactly the same path I have already covered.  In fact, some of
> >>his reviewer comments are almost word-for-word the same as those I
> >>have received and addressed in the past.
> >>
> >>My patches were always kept current in this mailing list and others,
> >>and Sascha clearly has the skills necessary to make improvements and
> >>corrections should he have chosen to do so.
> >I think that you made the fundamental mistake to completly ignore the
> >existing pwm API and its users. With a competing api we are basically
> >stuck. We can't convert the existing hardware drivers to the new API
> >because leds-pwm.c, pwm_bl.c and others still depend on the old API and
> >boards using it would break.
> 
> I don't think this is really a blocker to Bill's patches. There are
> three (that I can see) pwm users currently:
> drivers/video/backlight/pwm_bl.c
> drivers/leds/leds-pwm.c
> drivers/input/misc/pwm-beeper.c
> 
> All of those drivers are trivial and good easily be updated to work
> with Bill's patches. Bill already provided a leds-pwm driver.

Yes, it is easy but that's not my point. The point is that you can't
convert the drivers without converting *all* hardware drivers in a
*single* step. If you choose to have two competing APIs in the tree
for one purpose you can't convert the drivers but instead you have
to copy them, either with cp or ifdefs. I have just looked at the
leds-pwm driver Bill provided. Applying it immediately breaks all
users.

> 
> There is also the interesting case of the Atmel pwm driver, which
> does not fit the current pwm api and has its own backlight and leds
> drivers. Bill's patches addressed this, Sasha's patches do not. If
> we merge Sasha's patches then we are going to be in the same
> position as Bill's patches at some point in that we need to change
> the pwm api (and all its users) to meet the needs of the Atmel (and
> any similar hardware) pwm device.

My patches are compatible to the current (user-) API on purpose. It
offers the possibility to convert each hardware driver independently
of the others. Once we have a framework we can change the driver API
independent of the user API.

> 
> The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit
> the current api (though it could), but instead provides a sysfs
> interface to user-space. Again, this was addressed by Bill's
> patches.
> 
> >We can't convert the function drivers
> >either because again this would break boards for which only an old style
> >pwm driver exists.  So the logical thing to do is to put a step in
> >between: Consolidate the existing drivers and *then* change the API
> >atomically so that nothing breaks. Your patches don't do this, so I
> >don't think at all that what I did is duplication of work.
> 
> You have to modify the drivers anyway match the new pwm core.

Yes. But changing the user API *and* the driver API in a single patch
really is a bad idea.

I don't say at all that the end result Bill is aiming at is bad because
it isn't. We are not talking about the end result, but only the way to
get there. And getting from a to b in bisectable small steps is a well
established development model in the Kernel, or have I missed something?

> The
> Atmel and ep93xx drivers are going to be difficult to merge into the
> new api, and seeing as there are only about seven pwm drivers total
> in the kernel I think its a significant portion. Any pwm api
> patchset could easily convert all of the existing pwm drivers
> without becoming overly massive.
> 
> If we merge Sasha's api, then we can move most of the existing
> drivers and maybe add some new ones, but we will still have the
> unconsolidated outliers. When (if?) we try to fix those we will
> probably need to change the pwm api and therefore all of the drivers
> to. So its basically a case of do the effort now (Bill's patches) or
> do it later. Doing it later will probably require more effort.
> 
> >Given the current rush to move drivers out of arch/ it probably won't
> >take long until all pwm drivers are moved to drivers/pwm/ and converted
> >to use the framework, and then you have a good base to put your work onto.
> >So please don't complain too much: We are currently only doing the work
> >you didn't want to do.
> >
> 
> You can move all of the drivers out of arch now if you want. You
> just need to make sure you are only compiling one of them in.

Yes, we can. But this does not solve the migration problem I try
to describe in this and the previous mail.

Let's face it: Bill is working on his PWM patches for three years now,
still the merge of these patches is not in sight. Let's just split
them into smaller parts which are easier to swallow.

> The
> real job in consolidating means making sure that the api meets the
> needs of all of the drivers. The in kernel Atmel pwm driver at least
> is not going to convert easily to this api.

Then let's change the API. I have nothing against this.

> 
> Also, please not my change of email address for future emails.

ack

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 |

[PATCH 1/3] PWM: add pwm framework support

[Ryan Mallon]

On 01/07/11 17:37, Sascha Hauer wrote:
> On Fri, Jul 01, 2011 at 09:24:05AM +1000, Ryan Mallon wrote:
>> On 01/07/11 03:02, Sascha Hauer wrote:
>>> Bill,
>>>
>>> On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
>>>> Guys:
>>>>
>>>> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann<arnd@arndb.de>  wrote:
>>>>
>>>>> A lot of people want to see a framework get merged, and I think it's
>>>>> great that Sascha has volunteered to do the work to push that
>>>>> through this time, especially since you have not been able to
>>>>> finish your work.
>>>> Sascha is wasting his time by reinventing the wheel.  He's traveling
>>>> over exactly the same path I have already covered.  In fact, some of
>>>> his reviewer comments are almost word-for-word the same as those I
>>>> have received and addressed in the past.
>>>>
>>>> My patches were always kept current in this mailing list and others,
>>>> and Sascha clearly has the skills necessary to make improvements and
>>>> corrections should he have chosen to do so.
>>> I think that you made the fundamental mistake to completly ignore the
>>> existing pwm API and its users. With a competing api we are basically
>>> stuck. We can't convert the existing hardware drivers to the new API
>>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
>>> boards using it would break.
>> I don't think this is really a blocker to Bill's patches. There are
>> three (that I can see) pwm users currently:
>> drivers/video/backlight/pwm_bl.c
>> drivers/leds/leds-pwm.c
>> drivers/input/misc/pwm-beeper.c
>>
>> All of those drivers are trivial and good easily be updated to work
>> with Bill's patches. Bill already provided a leds-pwm driver.
> Yes, it is easy but that's not my point. The point is that you can't
> convert the drivers without converting *all* hardware drivers in a
> *single* step. If you choose to have two competing APIs in the tree
> for one purpose you can't convert the drivers but instead you have
> to copy them, either with cp or ifdefs. I have just looked at the
> leds-pwm driver Bill provided. Applying it immediately breaks all
> users.
>

At _any_ point that you change the pwm api you will need to change all
of the drivers. How do you plan to adapt the api in the future to
support the oddball pwm drivers without having to change all of the
drivers in one go?

The number of pwm drivers and users is so small that it would be nice to
see a patch series that introduces the new framework/api and converts
all of the drivers over. That way we don't get left in an intermediate
state with some drivers using the new framework and some using the old one.

>> There is also the interesting case of the Atmel pwm driver, which
>> does not fit the current pwm api and has its own backlight and leds
>> drivers. Bill's patches addressed this, Sasha's patches do not. If
>> we merge Sasha's patches then we are going to be in the same
>> position as Bill's patches at some point in that we need to change
>> the pwm api (and all its users) to meet the needs of the Atmel (and
>> any similar hardware) pwm device.
> My patches are compatible to the current (user-) API on purpose. It
> offers the possibility to convert each hardware driver independently
> of the others. Once we have a framework we can change the driver API
> independent of the user API.

We should just convert all of the drivers now rather than waiting for
the various maintainers to do it. Because your proposed api is backwards
compatible and most SoC platforms don't have additional external pwms
there is little motivation for them to do this work.

>> The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit
>> the current api (though it could), but instead provides a sysfs
>> interface to user-space. Again, this was addressed by Bill's
>> patches.
>>
>>> We can't convert the function drivers
>>> either because again this would break boards for which only an old style
>>> pwm driver exists.  So the logical thing to do is to put a step in
>>> between: Consolidate the existing drivers and *then* change the API
>>> atomically so that nothing breaks. Your patches don't do this, so I
>>> don't think at all that what I did is duplication of work.
>> You have to modify the drivers anyway match the new pwm core.
> Yes. But changing the user API *and* the driver API in a single patch
> really is a bad idea.

It doesn't have to be in a single patch, but it should be in a single
patch series.

> I don't say at all that the end result Bill is aiming at is bad because
> it isn't. We are not talking about the end result, but only the way to
> get there. And getting from a to b in bisectable small steps is a well
> established development model in the Kernel, or have I missed something?

The bi-sectable steps is fine. But what we have is a patch series which
introduces a new framework with a vague promise that the existing
drivers will get converted and the api will be improved later. Again,
the number of in-kernel users is so small that we may as well do it all
in one series.

>> The
>> Atmel and ep93xx drivers are going to be difficult to merge into the
>> new api, and seeing as there are only about seven pwm drivers total
>> in the kernel I think its a significant portion. Any pwm api
>> patchset could easily convert all of the existing pwm drivers
>> without becoming overly massive.
>>
>> If we merge Sasha's api, then we can move most of the existing
>> drivers and maybe add some new ones, but we will still have the
>> unconsolidated outliers. When (if?) we try to fix those we will
>> probably need to change the pwm api and therefore all of the drivers
>> to. So its basically a case of do the effort now (Bill's patches) or
>> do it later. Doing it later will probably require more effort.
>>
>>> Given the current rush to move drivers out of arch/ it probably won't
>>> take long until all pwm drivers are moved to drivers/pwm/ and converted
>>> to use the framework, and then you have a good base to put your work onto.
>>> So please don't complain too much: We are currently only doing the work
>>> you didn't want to do.
>>>
>> You can move all of the drivers out of arch now if you want. You
>> just need to make sure you are only compiling one of them in.
> Yes, we can. But this does not solve the migration problem I try
> to describe in this and the previous mail.
>
> Let's face it: Bill is working on his PWM patches for three years now,
> still the merge of these patches is not in sight. Let's just split
> them into smaller parts which are easier to swallow.
>

Sure, but your series still leaves a lot of work to be done. How many
years will the ep93xx and atmel pwm drivers exist in isolation from the
rest of the pwm drivers?

~Ryan

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

On Fri, Jul 01, 2011 at 06:28:48PM +1000, Ryan Mallon wrote:
> On 01/07/11 17:37, Sascha Hauer wrote:
> > On Fri, Jul 01, 2011 at 09:24:05AM +1000, Ryan Mallon wrote:
> >> On 01/07/11 03:02, Sascha Hauer wrote:
> >>> Bill,
> >>>
> >>> On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
> >>>> Guys:
> >>>>
> >>>> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann<arnd@arndb.de>  wrote:
> >>>>
> >>>>> A lot of people want to see a framework get merged, and I think it's
> >>>>> great that Sascha has volunteered to do the work to push that
> >>>>> through this time, especially since you have not been able to
> >>>>> finish your work.
> >>>> Sascha is wasting his time by reinventing the wheel.  He's traveling
> >>>> over exactly the same path I have already covered.  In fact, some of
> >>>> his reviewer comments are almost word-for-word the same as those I
> >>>> have received and addressed in the past.
> >>>>
> >>>> My patches were always kept current in this mailing list and others,
> >>>> and Sascha clearly has the skills necessary to make improvements and
> >>>> corrections should he have chosen to do so.
> >>> I think that you made the fundamental mistake to completly ignore the
> >>> existing pwm API and its users. With a competing api we are basically
> >>> stuck. We can't convert the existing hardware drivers to the new API
> >>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
> >>> boards using it would break.
> >> I don't think this is really a blocker to Bill's patches. There are
> >> three (that I can see) pwm users currently:
> >> drivers/video/backlight/pwm_bl.c
> >> drivers/leds/leds-pwm.c
> >> drivers/input/misc/pwm-beeper.c
> >>
> >> All of those drivers are trivial and good easily be updated to work
> >> with Bill's patches. Bill already provided a leds-pwm driver.
> > Yes, it is easy but that's not my point. The point is that you can't
> > convert the drivers without converting *all* hardware drivers in a
> > *single* step. If you choose to have two competing APIs in the tree
> > for one purpose you can't convert the drivers but instead you have
> > to copy them, either with cp or ifdefs. I have just looked at the
> > leds-pwm driver Bill provided. Applying it immediately breaks all
> > users.
> >
> 
> At _any_ point that you change the pwm api you will need to change all
> of the drivers. How do you plan to adapt the api in the future to
> support the oddball pwm drivers without having to change all of the
> drivers in one go?

It is a framework between hardware drivers and user drivers and as such
it is able to abstract between the API to the hardware drivers and the
API to the user drivers. You can change one of those without touching
the other.

> 
> The number of pwm drivers and users is so small that it would be nice to
> see a patch series that introduces the new framework/api and converts
> all of the drivers over. That way we don't get left in an intermediate
> state with some drivers using the new framework and some using the old one.

Go ahead, send patches.

> 
> >> There is also the interesting case of the Atmel pwm driver, which
> >> does not fit the current pwm api and has its own backlight and leds
> >> drivers. Bill's patches addressed this, Sasha's patches do not. If
> >> we merge Sasha's patches then we are going to be in the same
> >> position as Bill's patches at some point in that we need to change
> >> the pwm api (and all its users) to meet the needs of the Atmel (and
> >> any similar hardware) pwm device.
> > My patches are compatible to the current (user-) API on purpose. It
> > offers the possibility to convert each hardware driver independently
> > of the others. Once we have a framework we can change the driver API
> > independent of the user API.
> 
> We should just convert all of the drivers now rather than waiting for
> the various maintainers to do it. Because your proposed api is backwards
> compatible and most SoC platforms don't have additional external pwms
> there is little motivation for them to do this work.

And with a compatible API this is an easy job. Just remove the list
handling from the drivers and put the pwm_* functions into callbacks.
The motivation to do this comes from the people who want to change
the pwm API. I volunteer to convert at least the PXA and i.MX driver.

> 
> >> The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit
> >> the current api (though it could), but instead provides a sysfs
> >> interface to user-space. Again, this was addressed by Bill's
> >> patches.
> >>
> >>> We can't convert the function drivers
> >>> either because again this would break boards for which only an old style
> >>> pwm driver exists.  So the logical thing to do is to put a step in
> >>> between: Consolidate the existing drivers and *then* change the API
> >>> atomically so that nothing breaks. Your patches don't do this, so I
> >>> don't think at all that what I did is duplication of work.
> >> You have to modify the drivers anyway match the new pwm core.
> > Yes. But changing the user API *and* the driver API in a single patch
> > really is a bad idea.
> 
> It doesn't have to be in a single patch, but it should be in a single
> patch series.
> 
> > I don't say at all that the end result Bill is aiming at is bad because
> > it isn't. We are not talking about the end result, but only the way to
> > get there. And getting from a to b in bisectable small steps is a well
> > established development model in the Kernel, or have I missed something?
> 
> The bi-sectable steps is fine. But what we have is a patch series which
> introduces a new framework with a vague promise that the existing
> drivers will get converted and the api will be improved later. Again,
> the number of in-kernel users is so small that we may as well do it all
> in one series.

I don't understand you. On one hand you argue that it's trivial enough to
port all drivers over the a new API in a single series and on the other
hand you argue that we have to wait for several subsystem maintainers
to port the drivers over to an existing-but-now-in-a-framework API.

> 
> >> The
> >> Atmel and ep93xx drivers are going to be difficult to merge into the
> >> new api, and seeing as there are only about seven pwm drivers total
> >> in the kernel I think its a significant portion. Any pwm api
> >> patchset could easily convert all of the existing pwm drivers
> >> without becoming overly massive.
> >>
> >> If we merge Sasha's api, then we can move most of the existing
> >> drivers and maybe add some new ones, but we will still have the
> >> unconsolidated outliers. When (if?) we try to fix those we will
> >> probably need to change the pwm api and therefore all of the drivers
> >> to. So its basically a case of do the effort now (Bill's patches) or
> >> do it later. Doing it later will probably require more effort.
> >>
> >>> Given the current rush to move drivers out of arch/ it probably won't
> >>> take long until all pwm drivers are moved to drivers/pwm/ and converted
> >>> to use the framework, and then you have a good base to put your work onto.
> >>> So please don't complain too much: We are currently only doing the work
> >>> you didn't want to do.
> >>>
> >> You can move all of the drivers out of arch now if you want. You
> >> just need to make sure you are only compiling one of them in.
> > Yes, we can. But this does not solve the migration problem I try
> > to describe in this and the previous mail.
> >
> > Let's face it: Bill is working on his PWM patches for three years now,
> > still the merge of these patches is not in sight. Let's just split
> > them into smaller parts which are easier to swallow.
> >
> 
> Sure, but your series still leaves a lot of work to be done. How many
> years will the ep93xx and atmel pwm drivers exist in isolation from the
> rest of the pwm drivers?

If we continue to argue probably many years...

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 |

[PATCH 1/3] PWM: add pwm framework support

[Ryan Mallon]

On 01/07/11 18:54, Sascha Hauer wrote:
> On Fri, Jul 01, 2011 at 06:28:48PM +1000, Ryan Mallon wrote:
>> On 01/07/11 17:37, Sascha Hauer wrote:
>>> On Fri, Jul 01, 2011 at 09:24:05AM +1000, Ryan Mallon wrote:
>>>> On 01/07/11 03:02, Sascha Hauer wrote:
>>>>> Bill,
>>>>>
>>>>> On Thu, Jun 30, 2011 at 11:17:54AM -0500, Bill Gatliff wrote:
>>>>>> Guys:
>>>>>>
>>>>>> On Thu, Jun 30, 2011 at 7:41 AM, Arnd Bergmann<arnd@arndb.de>  wrote:
>>>>>>
>>>>>>> A lot of people want to see a framework get merged, and I think it's
>>>>>>> great that Sascha has volunteered to do the work to push that
>>>>>>> through this time, especially since you have not been able to
>>>>>>> finish your work.
>>>>>> Sascha is wasting his time by reinventing the wheel.  He's traveling
>>>>>> over exactly the same path I have already covered.  In fact, some of
>>>>>> his reviewer comments are almost word-for-word the same as those I
>>>>>> have received and addressed in the past.
>>>>>>
>>>>>> My patches were always kept current in this mailing list and others,
>>>>>> and Sascha clearly has the skills necessary to make improvements and
>>>>>> corrections should he have chosen to do so.
>>>>> I think that you made the fundamental mistake to completly ignore the
>>>>> existing pwm API and its users. With a competing api we are basically
>>>>> stuck. We can't convert the existing hardware drivers to the new API
>>>>> because leds-pwm.c, pwm_bl.c and others still depend on the old API and
>>>>> boards using it would break.
>>>> I don't think this is really a blocker to Bill's patches. There are
>>>> three (that I can see) pwm users currently:
>>>> drivers/video/backlight/pwm_bl.c
>>>> drivers/leds/leds-pwm.c
>>>> drivers/input/misc/pwm-beeper.c
>>>>
>>>> All of those drivers are trivial and good easily be updated to work
>>>> with Bill's patches. Bill already provided a leds-pwm driver.
>>> Yes, it is easy but that's not my point. The point is that you can't
>>> convert the drivers without converting *all* hardware drivers in a
>>> *single* step. If you choose to have two competing APIs in the tree
>>> for one purpose you can't convert the drivers but instead you have
>>> to copy them, either with cp or ifdefs. I have just looked at the
>>> leds-pwm driver Bill provided. Applying it immediately breaks all
>>> users.
>>>
>> At _any_ point that you change the pwm api you will need to change all
>> of the drivers. How do you plan to adapt the api in the future to
>> support the oddball pwm drivers without having to change all of the
>> drivers in one go?
> It is a framework between hardware drivers and user drivers and as such
> it is able to abstract between the API to the hardware drivers and the
> API to the user drivers. You can change one of those without touching
> the other.

You are missing the point. For example, the current in-kernel api to
configure a pwm is this:

int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);

Bill's proposed change to the api, which is more flexible, looks like this:

int pwm_config(struct pwm_channel *p,
        struct pwm_channel_config *c)

If you want to change the api in this way at any time, you will need to
change all of the pwm drivers and all of the users (pwm-bl, pwm-leds,
etc) at the same time, or introduce some sort of wrapper layer to ease
the conversion. Either way, if you do it now or do it after your patch
set it is still the same amount of work.

We do want a better api for pwm drivers, if only to support oddball
hardware like the atmel. The process I see is this if we go with Bill's
patches:
 - Introduce new api with common framework
 - Move drivers to new api

With your patches we get this:
 - Introduce common framework
 - Move drivers to common framework
 - Modify api
 - Rewrite drivers for new api

The problem we have seen with these processes in the past is they get
left half way with some drivers moving to the new framework and some
trailing behind and some having their own hand-rolled approach because
the common api doesn't meet their needs.

>>> I don't say at all that the end result Bill is aiming at is bad because
>>> it isn't. We are not talking about the end result, but only the way to
>>> get there. And getting from a to b in bisectable small steps is a well
>>> established development model in the Kernel, or have I missed something?
>> The bi-sectable steps is fine. But what we have is a patch series which
>> introduces a new framework with a vague promise that the existing
>> drivers will get converted and the api will be improved later. Again,
>> the number of in-kernel users is so small that we may as well do it all
>> in one series.
> I don't understand you. On one hand you argue that it's trivial enough to
> port all drivers over the a new API in a single series and on the other
> hand you argue that we have to wait for several subsystem maintainers
> to port the drivers over to an existing-but-now-in-a-framework API.
>

No, I am saying that if we don't port all the drivers and leave it up to
the maintainers then it won't get done and we get left with some drivers
using the old approach and some using the new approach. For example, we
still have platforms which aren't using gpiolib yet and that was
introduced 3 years ago.

~Ryan

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

On Sat, Jul 02, 2011 at 10:40:54AM +1000, Ryan Mallon wrote:
> On 01/07/11 18:54, Sascha Hauer wrote:
> >> At _any_ point that you change the pwm api you will need to change all
> >> of the drivers. How do you plan to adapt the api in the future to
> >> support the oddball pwm drivers without having to change all of the
> >> drivers in one go?
> > It is a framework between hardware drivers and user drivers and as such
> > it is able to abstract between the API to the hardware drivers and the
> > API to the user drivers. You can change one of those without touching
> > the other.
> 
> You are missing the point. For example, the current in-kernel api to
> configure a pwm is this:
> 
> int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
> 
> Bill's proposed change to the api, which is more flexible, looks like this:
> 
> int pwm_config(struct pwm_channel *p,
>         struct pwm_channel_config *c)
> 
> If you want to change the api in this way at any time, you will need to
> change all of the pwm drivers and all of the users (pwm-bl, pwm-leds,
> etc) at the same time, or introduce some sort of wrapper layer to ease
> the conversion. Either way, if you do it now or do it after your patch
> set it is still the same amount of work.
> 
> We do want a better api for pwm drivers, if only to support oddball
> hardware like the atmel. The process I see is this if we go with Bill's
> patches:
>  - Introduce new api with common framework

I'm repeating myself. At this point you can't convert any of the old
drivers because it breaks the support we already have.

>  - Move drivers to new api

And that's one single step. To be really bisectible have would have to
convert *all* PWM drivers in a single patch, not even a patch series
as it would break bisectibility in between.

> 
> With your patches we get this:
>  - Introduce common framework

And still everything works

>  - Move drivers to common framework

One after another, still everything works

>  - Modify api
>  - Rewrite drivers for new api

There are two of them. One for the hardware drivers and one for the
users. You can change them independently, introduce wrappers where
necessary, you have all the time in the world to do this.

Your aim is to change the API? Then you have to clean up first. People
like Thomas have cleaned up in all areas of the kernel to reach their
goal of merging the RT stuff. They wouldn't have got anywhere with
a here's-a-patch-that-changes-everything approach.

> 
> The problem we have seen with these processes in the past is they get
> left half way with some drivers moving to the new framework and some
> trailing behind and some having their own hand-rolled approach because
> the common api doesn't meet their needs.

With two competing APIs it's even worse because you can't go anywhere
without converting everything in tree.

Just imagine Bill or someone else comes up with a series

PATCH: introduce new pwm framework with completely changed API, sysfs
interface and interrupt support, also convert leds-pwm, pwm_bl, pwm-beeper
to new API and convert all hardware drivers to new API aswell.

What do you think would happen? I can think of two things: Either no
reaction at all from others or a huge thread with no result at all
because we can't agree on details. Then someone comes up with the
recommendation to start small and do it in little steps.


I am tired of discussing this. It seems we can't agree and unless
someone else jumps in here we will probably have to wait for another
year until something moves in the PWM area.

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 |

[PATCH 1/3] PWM: add pwm framework support

[Dmitry Eremin-Solenikov]

Sascha Hauer wrote:

> I am tired of discussing this. It seems we can't agree and unless
> someone else jumps in here we will probably have to wait for another
> year until something moves in the PWM area.

Please continue your work on PWM area! Patch reviews show a really
positive attitute towards merging your patchset. Please don't stop here!

-- 
With best wishes
Dmitry

[PATCH 1/3] PWM: add pwm framework support

[Ryan Mallon]

On 04/07/11 17:55, Sascha Hauer wrote:
> I am tired of discussing this. It seems we can't agree and unless
> someone else jumps in here we will probably have to wait for another
> year until something moves in the PWM area.

If we are going to introduce a new framework for pwms then we should
create one which meets the needs of at least all of the in kernel
drivers. This patch series provides no solution for either the atmel or
ep93xx drivers, so it is not a complete solution. At some point the
api/framework _must_ be changed. If we can introduce transition layers
then we should do that now so they we can provide a common framework
along with some forward thinking about how the other drivers are going
to be migrated to the new framework. This patch series doesn't even
migrate _any_ of the existing drivers.

It doesn't have to be an all or nothing approach. Possibly Bill's series
is perhaps too involved by changing the api, introducing sysfs support
and reworking the pwm users. But your series is at the opposite end of
the spectrum. It does too little. It will take a few release cycles to
get all of the existing drivers migrated and since we can't change the
api until that happens the atmel and ep93xx drivers will take longer
still. At the very least your series should migrate some of the drivers.

The timeline argument is a bit poor. Yes, there has been discussion for
a lengthy time about how the pwm api should be developed, but I think
that is because it is non-trivial to come up with a framework which is
good enough to support all of the pwm hardware (some of which is already
in the mainline). Getting something merged now just because it can be
done quickly is not a good idea if it all has to get reworked in the
future so that it can support all the hardware.

The pwm framework needs to incorporate at least the following:
 - sysfs access (ep93xx driver)
 - Multiple channels per device (atmel driver)

The mxs driver you introduce looks like it could be implemented as a
single device (continuous mmio space) with multiple channels rather than
the pwm core/driver approach you have. I also can't see anything in this
patch set which hooks up the mxs pwms to an actual board (i.e. nothing
calls mxs_add_mxs_pwm)?

The other nice things to have for the pwm framework are:
 - More fine grained control of pwms: pwm_period_ns, period_duty_ns, etc
 - Polarity control
 - Synchronisation support for multi-channel devices
 - Interrupt handler support
 - Sleeping vs non-sleeping configuration api

The fine-grained control api could be added now. pwm_config could be
left as is for the time being (the new api could be a wrapper around it
to start with). Polarity control and interrupt handling apis could also
be defined without affecting the drivers which don't need to implement
them. Multiple channels and the sleeping/non-sleeping api are the more
difficult ones, but at least having some sort of indication about how
these plan to be solved would be useful.

~Ryan

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

On Mon, Jul 04, 2011 at 08:43:23PM +1000, Ryan Mallon wrote:
> On 04/07/11 17:55, Sascha Hauer wrote:
> > I am tired of discussing this. It seems we can't agree and unless
> > someone else jumps in here we will probably have to wait for another
> > year until something moves in the PWM area.
> 
> If we are going to introduce a new framework for pwms then we should
> create one which meets the needs of at least all of the in kernel
> drivers. This patch series provides no solution for either the atmel or
> ep93xx drivers, so it is not a complete solution. At some point the
> api/framework _must_ be changed. If we can introduce transition layers
> then we should do that now so they we can provide a common framework
> along with some forward thinking about how the other drivers are going
> to be migrated to the new framework. This patch series doesn't even
> migrate _any_ of the existing drivers.
> 
> It doesn't have to be an all or nothing approach. Possibly Bill's series
> is perhaps too involved by changing the api, introducing sysfs support
> and reworking the pwm users. But your series is at the opposite end of
> the spectrum. It does too little. It will take a few release cycles to
> get all of the existing drivers migrated and since we can't change the
> api until that happens the atmel and ep93xx drivers will take longer
> still. At the very least your series should migrate some of the drivers.
> 
> The timeline argument is a bit poor. Yes, there has been discussion for
> a lengthy time about how the pwm api should be developed, but I think
> that is because it is non-trivial to come up with a framework which is
> good enough to support all of the pwm hardware (some of which is already
> in the mainline). Getting something merged now just because it can be
> done quickly is not a good idea if it all has to get reworked in the
> future so that it can support all the hardware.
> 
> The pwm framework needs to incorporate at least the following:
>  - sysfs access (ep93xx driver)

The sysfs interface will likely raise smoe more discussion, that's why I
intentionally have no support for it. It can be added later, but right
now I see no reason why we should add artificial barriers to merge these
patches.

>  - Multiple channels per device (atmel driver)

Again, this can be added later.

> 
> The mxs driver you introduce looks like it could be implemented as a
> single device (continuous mmio space) with multiple channels rather than
> the pwm core/driver approach you have. I also can't see anything in this
> patch set which hooks up the mxs pwms to an actual board (i.e. nothing
> calls mxs_add_mxs_pwm)?

Why should anyone register a device for which no driver is in the tree?

> 
> The other nice things to have for the pwm framework are:
>  - More fine grained control of pwms: pwm_period_ns, period_duty_ns, etc
>  - Polarity control
>  - Synchronisation support for multi-channel devices
>  - Interrupt handler support
>  - Sleeping vs non-sleeping configuration api
> 
> The fine-grained control api could be added now. pwm_config could be
> left as is for the time being (the new api could be a wrapper around it
> to start with). Polarity control and interrupt handling apis could also
> be defined without affecting the drivers which don't need to implement
> them. Multiple channels and the sleeping/non-sleeping api are the more
> difficult ones, but at least having some sort of indication about how
> these plan to be solved would be useful.

Again, why should we add these *now*? It only raises the chance that
there's more discussion.

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 |

[PATCH 1/3] PWM: add pwm framework support

[Arnd Bergmann]

On Monday 04 July 2011, Sascha Hauer wrote:
> On Mon, Jul 04, 2011 at 08:43:23PM +1000, Ryan Mallon wrote:
>
> > The fine-grained control api could be added now. pwm_config could be
> > left as is for the time being (the new api could be a wrapper around it
> > to start with). Polarity control and interrupt handling apis could also
> > be defined without affecting the drivers which don't need to implement
> > them. Multiple channels and the sleeping/non-sleeping api are the more
> > difficult ones, but at least having some sort of indication about how
> > these plan to be solved would be useful.
> 
> Again, why should we add these *now*? It only raises the chance that
> there's more discussion.

My impression is that there are a lot of things that could easily be
done to improve the state of PWM drivers, but I don't care about the
order in which they are done. My main issue is the lack of a subsystem
core driver, which both you and Bill have patches for. It's clear that
other people have other issues and want to see their problems addressed
first.

I also think that the pwm code is simple enough that we don't have
to worry too much about the order that things are done in. Any patch
that is making the code better should just get in and not have to
wait for something else to be completed first.

What we do need now is a maintainer that can coordinate the patches
and merge the ones that have been agreed on. Or multiple maintainers.

	Arnd

[PATCH 1/3] PWM: add pwm framework support

[Thierry Reding]

* Arnd Bergmann wrote:
> On Monday 04 July 2011, Sascha Hauer wrote:
> > On Mon, Jul 04, 2011 at 08:43:23PM +1000, Ryan Mallon wrote:
> >
> > > The fine-grained control api could be added now. pwm_config could be
> > > left as is for the time being (the new api could be a wrapper around it
> > > to start with). Polarity control and interrupt handling apis could also
> > > be defined without affecting the drivers which don't need to implement
> > > them. Multiple channels and the sleeping/non-sleeping api are the more
> > > difficult ones, but at least having some sort of indication about how
> > > these plan to be solved would be useful.
> > 
> > Again, why should we add these *now*? It only raises the chance that
> > there's more discussion.
> 
> My impression is that there are a lot of things that could easily be
> done to improve the state of PWM drivers, but I don't care about the
> order in which they are done. My main issue is the lack of a subsystem
> core driver, which both you and Bill have patches for. It's clear that
> other people have other issues and want to see their problems addressed
> first.
> 
> I also think that the pwm code is simple enough that we don't have
> to worry too much about the order that things are done in. Any patch
> that is making the code better should just get in and not have to
> wait for something else to be completed first.
> 
> What we do need now is a maintainer that can coordinate the patches
> and merge the ones that have been agreed on. Or multiple maintainers.

Hi,

I'm looking at adding DT support for pwm-backlight, and I think a central PWM
API will be required first. Looking through the archives this seems to be the
last activity in that direction. Perhaps we can get the efforts restarted?

I pretty much agree with Arnd and Sascha here in that we should try to get a
basic framework added. Everything else can be added on top later.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111207/9b74b01f/attachment.sig>

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

On Wed, Dec 07, 2011 at 09:53:39AM +0100, Thierry Reding wrote:
> * Arnd Bergmann wrote:
> > On Monday 04 July 2011, Sascha Hauer wrote:
> > > On Mon, Jul 04, 2011 at 08:43:23PM +1000, Ryan Mallon wrote:
> > >
> > > > The fine-grained control api could be added now. pwm_config could be
> > > > left as is for the time being (the new api could be a wrapper around it
> > > > to start with). Polarity control and interrupt handling apis could also
> > > > be defined without affecting the drivers which don't need to implement
> > > > them. Multiple channels and the sleeping/non-sleeping api are the more
> > > > difficult ones, but at least having some sort of indication about how
> > > > these plan to be solved would be useful.
> > > 
> > > Again, why should we add these *now*? It only raises the chance that
> > > there's more discussion.
> > 
> > My impression is that there are a lot of things that could easily be
> > done to improve the state of PWM drivers, but I don't care about the
> > order in which they are done. My main issue is the lack of a subsystem
> > core driver, which both you and Bill have patches for. It's clear that
> > other people have other issues and want to see their problems addressed
> > first.
> > 
> > I also think that the pwm code is simple enough that we don't have
> > to worry too much about the order that things are done in. Any patch
> > that is making the code better should just get in and not have to
> > wait for something else to be completed first.
> > 
> > What we do need now is a maintainer that can coordinate the patches
> > and merge the ones that have been agreed on. Or multiple maintainers.
> 
> Hi,
> 
> I'm looking at adding DT support for pwm-backlight, and I think a central PWM
> API will be required first. Looking through the archives this seems to be the
> last activity in that direction. Perhaps we can get the efforts restarted?
> 
> I pretty much agree with Arnd and Sascha here in that we should try to get a
> basic framework added. Everything else can be added on top later.

I have a branch which adds a basic PWM framework. It doesn't change the
pwm API at all, but only adds support for registering multiple PWM
drivers. All in Kernel drivers are converted to this new framework, but
except the i.MX driver all of them are untested. I'm quite confident
that the drivers itself work, but there will probably be some Kconfig
related issues which I wasn't able to sort out. The corresponding
Maintainers should probably have a look over it. Feel free to post
the patches if you are prepared to work on the comments you receive.
Otherwise I'll see if I find some time maybe next week.

Sascha


You can find my latest work on this here:


The following changes since commit c3b92c8787367a8bb53d57d9789b558f1295cc96:

  Linux 3.1 (2011-10-24 09:10:05 +0200)

are available in the git repository at:
  git://git.pengutronix.de/git/imx/linux-2.6.git pwmlib

Sascha Hauer (7):
      PWM: add pwm framework support
      ARM mxs: adjust pwm resources to what the driver expects
      pwm: Add a i.MX23/28 pwm driver
      ARM i.MX: Move i.MX pwm driver to pwm framework
      ARM pxa: Move pxa pwm driver to pwm framework
      ARM Samsung: Move s3c pwm driver to pwm framework
      ARM vt8500: Move vt8500 pwm driver to pwm framework

 Documentation/pwm.txt                              |   56 ++++
 MAINTAINERS                                        |    6 +
 arch/arm/mach-mxs/devices/platform-mxs-pwm.c       |   32 ++-
 arch/arm/mach-pxa/Kconfig                          |   14 -
 arch/arm/plat-mxc/Kconfig                          |    6 -
 arch/arm/plat-mxc/Makefile                         |    1 -
 arch/arm/plat-pxa/Makefile                         |    1 -
 arch/arm/plat-samsung/Makefile                     |    4 -
 drivers/Kconfig                                    |    2 +
 drivers/Makefile                                   |    1 +
 drivers/pwm/Kconfig                                |   32 ++
 drivers/pwm/Makefile                               |    6 +
 drivers/pwm/core.c                                 |  220 ++++++++++++++
 arch/arm/plat-mxc/pwm.c => drivers/pwm/imx-pwm.c   |  102 ++-----
 drivers/pwm/mxs-pwm.c                              |  312 ++++++++++++++++++++
 arch/arm/plat-pxa/pwm.c => drivers/pwm/pxa-pwm.c   |  118 +++-----
 .../pwm.c => drivers/pwm/samsung-pwm.c             |  108 ++-----
 .../mach-vt8500/pwm.c => drivers/pwm/vt8500-pwm.c  |  103 ++-----
 include/linux/pwm.h                                |   37 +++
 19 files changed, 832 insertions(+), 329 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/core.c
 rename arch/arm/plat-mxc/pwm.c => drivers/pwm/imx-pwm.c (78%)
 create mode 100644 drivers/pwm/mxs-pwm.c
 rename arch/arm/plat-pxa/pwm.c => drivers/pwm/pxa-pwm.c (72%)
 rename arch/arm/plat-samsung/pwm.c => drivers/pwm/samsung-pwm.c (80%)
 rename arch/arm/mach-vt8500/pwm.c => drivers/pwm/vt8500-pwm.c (75%)

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

[PATCH 1/3] PWM: add pwm framework support

[Thierry Reding]

* Sascha Hauer wrote:
> I have a branch which adds a basic PWM framework. It doesn't change the
> pwm API at all, but only adds support for registering multiple PWM
> drivers. All in Kernel drivers are converted to this new framework, but
> except the i.MX driver all of them are untested. I'm quite confident
> that the drivers itself work, but there will probably be some Kconfig
> related issues which I wasn't able to sort out. The corresponding
> Maintainers should probably have a look over it. Feel free to post
> the patches if you are prepared to work on the comments you receive.
> Otherwise I'll see if I find some time maybe next week.
[...]

I've looked at this in more detail and one thing that irritates me is that
the current driver API requires a driver to register a pwm_chip structure for
each PWM it can control. Wouldn't it be easier in such cases to just specify
how many PWMs a chip wants to register, much like gpiolib does it?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111214/dcaef88c/attachment.sig>

[PATCH 1/3] PWM: add pwm framework support

[Sascha Hauer]

On Wed, Dec 14, 2011 at 11:03:38AM +0100, Thierry Reding wrote:
> * Sascha Hauer wrote:
> > I have a branch which adds a basic PWM framework. It doesn't change the
> > pwm API at all, but only adds support for registering multiple PWM
> > drivers. All in Kernel drivers are converted to this new framework, but
> > except the i.MX driver all of them are untested. I'm quite confident
> > that the drivers itself work, but there will probably be some Kconfig
> > related issues which I wasn't able to sort out. The corresponding
> > Maintainers should probably have a look over it. Feel free to post
> > the patches if you are prepared to work on the comments you receive.
> > Otherwise I'll see if I find some time maybe next week.
> [...]
> 
> I've looked at this in more detail and one thing that irritates me is that
> the current driver API requires a driver to register a pwm_chip structure for
> each PWM it can control. Wouldn't it be easier in such cases to just specify
> how many PWMs a chip wants to register, much like gpiolib does it?

It may be more suitable for some devices like for example the mxs driver
or the at91 driver which Bill has in his tree. However, the idea of this
series was to move the drivers over with the least possible breakage.
Once the drivers are consolidated in a single directory with a single
framework it will be easier to modify.

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 |

[PATCH 1/3] PWM: add pwm framework support

[Arnd Bergmann]

On Monday 04 July 2011, Kurt Van Dijck wrote:
> > 
> > The pwm framework needs to incorporate at least the following:
> >  - sysfs access (ep93xx driver)
> >  - Multiple channels per device (atmel driver)
> 
> These are 2 very hardware dependant additions. Is this really the job
> for a framework to incorporate this?
> IMHO, the job of a framework is to allow such things. Creating a framework
> that does all special things of all vendors makes such thing
> complicated.

I think you shouldn't make it too easy for drivers to add extra
sysfs files. If at all possible, the default should be to add
them to the core, and define them in a way that makes sense for
future similar drivers.

We need to be careful to avoid a situation where multiple driver
writers introduce the same feature with a sysfs attribute, but do
so in an incompatible way.

> With socketCAN, we encountered a similar problem. Every chip maker
> tries to create added value by means of special options. You can't
> support them all in the framework. Therefore, sysfs can be added
> to configure special things.

I would expect that pwm is much simpler than CAN, so the amount
of creativity there is also limited.

	Arnd

[PATCH 2/3] ARM mxs: adjust pwm resources to what the driver expects

[Sascha Hauer]

The PWMs on i.MX23/28 have almost seperated register spaces
but share a common enable register. To reflect this register
a parent device to the PWMs which handles the enable register.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-mxs/devices/platform-mxs-pwm.c |   32 +++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mxs/devices/platform-mxs-pwm.c b/arch/arm/mach-mxs/devices/platform-mxs-pwm.c
index 680f5a9..36580b7 100644
--- a/arch/arm/mach-mxs/devices/platform-mxs-pwm.c
+++ b/arch/arm/mach-mxs/devices/platform-mxs-pwm.c
@@ -9,14 +9,40 @@
 #include <asm/sizes.h>
 #include <mach/devices-common.h>
 
+static struct platform_device *__init mxs_add_pwm_core(resource_size_t iobase)
+{
+	struct resource res = {
+		.flags = IORESOURCE_MEM,
+		.start = iobase,
+		.end = iobase + 0xff,
+	};
+
+	return mxs_add_platform_device("mxs-pwm-core", 0, &res, 1, NULL, 0);
+}
+
 struct platform_device *__init mxs_add_mxs_pwm(resource_size_t iobase, int id)
 {
+	struct resource *r;
+
+	static struct platform_device *pwm_core;
 	struct resource res = {
 		.flags = IORESOURCE_MEM,
+		.start = iobase + 0x10 + 0x20 * id,
+		.end = iobase + 0x10 + 0x20 * id + 0x1f,
 	};
 
-	res.start = iobase + 0x10 + 0x20 * id;
-	res.end = res.start + 0x1f;
+	if (!pwm_core) {
+		pwm_core = mxs_add_pwm_core(iobase);
+		if (!pwm_core)
+			return NULL;
+	}
+
+	r = platform_get_resource(pwm_core, IORESOURCE_MEM, 0);
+	if (!r)
+		return NULL;
+
+	res.parent = r;
 
-	return mxs_add_platform_device("mxs-pwm", id, &res, 1, NULL, 0);
+	return platform_device_register_resndata(&pwm_core->dev, "mxs-pwm",
+			id, &res, 1, NULL, 0);
 }
-- 
1.7.5.3

[PATCH 2/3] ARM mxs: adjust pwm resources to what the driver expects

[Arnd Bergmann]

On Thursday 30 June 2011, Sascha Hauer wrote:
> The PWMs on i.MX23/28 have almost seperated register spaces
> but share a common enable register. To reflect this register
> a parent device to the PWMs which handles the enable register.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 3/3] pwm: Add a i.MX23/28 pwm driver

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pwm/Kconfig   |    6 +-
 drivers/pwm/Makefile  |    1 +
 drivers/pwm/mxs-pwm.c |  312 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+), 1 deletions(-)
 create mode 100644 drivers/pwm/mxs-pwm.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 93c1052..5694574 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -2,11 +2,15 @@ menuconfig PWM
 	bool "PWM Support"
 	help
 	  This enables PWM support through the generic PWM framework.
-	  You only need to enable this, if you also want to enable
+	  You only need to enable this if you also want to enable
 	  one or more of the PWM drivers below.
 
 	  If unsure, say N.
 
 if PWM
 
+config PWM_MXS
+	tristate "MXS pwm support"
+	depends on ARCH_MXS
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 3469c3d..2cadd50 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_MXS)		+= mxs-pwm.o
diff --git a/drivers/pwm/mxs-pwm.c b/drivers/pwm/mxs-pwm.c
new file mode 100644
index 0000000..150ab32
--- /dev/null
+++ b/drivers/pwm/mxs-pwm.c
@@ -0,0 +1,312 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * simple driver for PWM (Pulse Width Modulator) controller
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Derived from pxa PWM driver by eric miao <eric.miao@marvell.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <mach/hardware.h>
+#include <asm/div64.h>
+
+enum mxs_pwm_devtype {
+	MXS_PWM_CORE,
+	MXS_PWM,
+};
+
+struct mxs_pwm_device {
+	struct device	*dev;
+
+	struct clk	*clk;
+
+	int		enabled;
+	void __iomem	*mmio_base;
+	void __iomem	*enable_base;
+
+	unsigned int	pwm_id;
+
+	u32		val_active;
+	u32		val_period;
+	int		period_us;
+	struct pwm_chip chip;
+};
+
+/* common register space */
+#define REG_PWM_CTRL		0x0
+#define REG_PWM_CTRL_SET	0x4
+#define REG_PWM_CTRL_CLEAR	0x8
+#define PWM_SFTRST	(1 << 31)
+#define PWM_CLKGATE	(1 << 30)
+#define PWM_ENABLE(p)	(1 << (p))
+
+/* per pwm register space */
+#define REG_ACTIVE	0x0
+#define REG_PERIOD	0x10
+
+#define PERIOD_PERIOD(p)	((p) & 0xffff)
+#define PERIOD_ACTIVE_HIGH	(3 << 16)
+#define PERIOD_INACTIVE_LOW	(2 << 18)
+#define PERIOD_CDIV(div)	(((div) & 0x7) << 20)
+
+static void pwm_update(struct mxs_pwm_device *pwm)
+{
+	writel(pwm->val_active, pwm->mmio_base + REG_ACTIVE);
+	writel(pwm->val_period, pwm->mmio_base + REG_PERIOD);
+}
+
+#define to_mxs_pwm_device(chip)	container_of(chip, struct mxs_pwm_device, chip)
+
+static int mxs_pwm_config(struct pwm_chip *chip, int duty_ns, int period_ns)
+{
+	struct mxs_pwm_device *pwm = to_mxs_pwm_device(chip);
+	int div = 0;
+	unsigned long rate;
+	unsigned long long c;
+	unsigned long period_cycles, duty_cycles;
+
+	rate = clk_get_rate(pwm->clk);
+
+	dev_dbg(pwm->dev, "config: duty_ns: %d, period_ns: %d (clkrate %ld)\n",
+			duty_ns, period_ns, rate);
+
+	while (1) {
+		c = rate / (1 << div);
+		c = c * period_ns;
+		do_div(c, 1000000000);
+		if (c < 0x10000)
+			break;
+		div++;
+
+		if (div > 8)
+			return -EINVAL;
+	}
+
+	period_cycles = c;
+	duty_cycles = period_cycles * duty_ns / period_ns;
+
+	dev_dbg(pwm->dev, "config period_cycles: %ld duty_cycles: %ld\n",
+			period_cycles, duty_cycles);
+
+	pwm->val_active = period_cycles << 16 | duty_cycles;
+	pwm->val_period = PERIOD_PERIOD(period_cycles) | PERIOD_ACTIVE_HIGH |
+			PERIOD_INACTIVE_LOW | PERIOD_CDIV(div);
+	pwm->period_us = period_ns / 1000;
+
+	pwm_update(pwm);
+
+	return 0;
+}
+
+static void __pwm_enable(struct mxs_pwm_device *pwm, int enable)
+{
+	void __iomem *reg;
+
+	if (enable)
+		reg = pwm->enable_base + REG_PWM_CTRL_SET;
+	else
+		reg = pwm->enable_base + REG_PWM_CTRL_CLEAR;
+
+	writel(PWM_ENABLE(pwm->chip.pwm_id), reg);
+}
+
+static int mxs_pwm_enable(struct pwm_chip *chip)
+{
+	struct mxs_pwm_device *pwm = to_mxs_pwm_device(chip);
+	int ret = 0;
+
+	dev_dbg(pwm->dev, "enable\n");
+
+	if (!pwm->enabled) {
+		ret = clk_enable(pwm->clk);
+		if (!ret) {
+			pwm->enabled = 1;
+			__pwm_enable(pwm, 1);
+			pwm_update(pwm);
+		}
+	}
+	return ret;
+}
+
+static void mxs_pwm_disable(struct pwm_chip *chip)
+{
+	struct mxs_pwm_device *pwm = to_mxs_pwm_device(chip);
+
+	dev_dbg(pwm->dev, "disable\n");
+
+	if (pwm->enabled) {
+		/*
+		 * We need a little delay here, it takes one period for
+		 * the last pwm_config call to take effect. If we disable
+		 * the pwm too early it just freezes the current output
+		 * state.
+		 */
+		usleep_range(pwm->period_us, pwm->period_us * 2);
+		__pwm_enable(pwm, 0);
+		clk_disable(pwm->clk);
+		pwm->enabled = 0;
+	}
+}
+
+static struct pwm_ops mxs_pwm_ops = {
+	.enable = mxs_pwm_enable,
+	.disable = mxs_pwm_disable,
+	.config = mxs_pwm_config,
+	.owner = THIS_MODULE,
+};
+
+static int mxs_pwm_core_probe(struct platform_device *pdev,
+		struct mxs_pwm_device *pwm)
+{
+	struct resource *r;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r)
+		return -ENODEV;
+
+	r = devm_request_mem_region(&pdev->dev, r->start, 0xf, pdev->name);
+	if (!r)
+		return -EBUSY;
+
+	pwm->mmio_base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+	if (!pwm->mmio_base) {
+		dev_err(&pdev->dev, "failed to ioremap() registers\n");
+		return -ENOMEM;
+	}
+
+	writel(PWM_SFTRST | PWM_CLKGATE, pwm->mmio_base + REG_PWM_CTRL_CLEAR);
+
+	return 0;
+}
+
+static int mxs_pwm_device_probe(struct platform_device *pdev,
+		struct mxs_pwm_device *pwm)
+{
+	struct mxs_pwm_device *parent_pwm;
+	struct resource *r;
+	int ret;
+
+	pwm->chip.ops = &mxs_pwm_ops;
+	pwm->chip.pwm_id = pdev->id;
+
+	parent_pwm = dev_get_drvdata(pdev->dev.parent);
+	if (!parent_pwm) {
+		dev_err(&pdev->dev, "no parent driver data\n");
+		return -EINVAL;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r)
+		return -ENODEV;
+
+	r = devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
+			pdev->name);
+	if (!r)
+		return -EBUSY;
+
+	pwm->enable_base = parent_pwm->mmio_base;
+	pwm->mmio_base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+
+	pwm->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return PTR_ERR(pwm->clk);
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret) {
+		clk_put(pwm->clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devinit mxs_pwm_probe(struct platform_device *pdev)
+{
+	struct mxs_pwm_device *pwm;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
+	if (!pwm) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	pwm->dev = &pdev->dev;
+
+	platform_set_drvdata(pdev, pwm);
+
+	if (pdev->id_entry->driver_data == MXS_PWM_CORE)
+		return mxs_pwm_core_probe(pdev, pwm);
+	else
+		return mxs_pwm_device_probe(pdev, pwm);
+
+	return 0;
+}
+
+static int __devexit mxs_pwm_remove(struct platform_device *pdev)
+{
+	struct mxs_pwm_device *pwm = platform_get_drvdata(pdev);
+	int ret;
+
+	switch (pdev->id_entry->driver_data) {
+	case MXS_PWM_CORE:
+		writel(PWM_CLKGATE, pwm->mmio_base + REG_PWM_CTRL_SET);
+		return 0;
+	case MXS_PWM:
+		ret = pwmchip_remove(&pwm->chip);
+		if (ret)
+			return ret;
+
+		clk_put(pwm->clk);
+	}
+
+	return 0;
+}
+
+static struct platform_device_id mxs_pwm_devtype[] = {
+	{
+		.name = "mxs-pwm-core",
+		.driver_data = MXS_PWM_CORE,
+	}, {
+		.name = "mxs-pwm",
+		.driver_data = MXS_PWM,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver mxs_pwm_driver = {
+	.driver		= {
+		.name	= "mxs-pwm",
+	},
+	.id_table	= mxs_pwm_devtype,
+	.probe		= mxs_pwm_probe,
+	.remove		= __devexit_p(mxs_pwm_remove),
+};
+
+static int __init mxs_pwm_init(void)
+{
+	return platform_driver_register(&mxs_pwm_driver);
+}
+arch_initcall(mxs_pwm_init);
+
+static void __exit mxs_pwm_exit(void)
+{
+	platform_driver_unregister(&mxs_pwm_driver);
+}
+module_exit(mxs_pwm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
-- 
1.7.5.3

[PATCH 3/3] pwm: Add a i.MX23/28 pwm driver

[Arnd Bergmann]

On Thursday 30 June 2011, Sascha Hauer wrote:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Hi Sascha,

The probing looks good to me now. I would have added extra code to avoid
ioremapping the same page multiple times, but I see this as a matter of
different preferences, so I'm fine with that.

You are still missing a description of the driver, both in the changelog
above and in the Kconfig patch. A small sentence to spell out PWM
in the changelog and to list the devices that will use this driver
would be helpful to the causal reader.

Provided that you add that:

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 93c1052..5694574 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -2,11 +2,15 @@ menuconfig PWM
>  	bool "PWM Support"
>  	help
>  	  This enables PWM support through the generic PWM framework.
> -	  You only need to enable this, if you also want to enable
> +	  You only need to enable this if you also want to enable
>  	  one or more of the PWM drivers below.
>  
>  	  If unsure, say N.
>  

I think this hunk should have been in the other first patch.

	Arnd

[PATCH 3/3] pwm: Add a i.MX23/28 pwm driver

[Sascha Hauer]

On Thu, Jun 30, 2011 at 01:42:35PM +0200, Arnd Bergmann wrote:
> On Thursday 30 June 2011, Sascha Hauer wrote:
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Hi Sascha,
> 
> The probing looks good to me now. I would have added extra code to avoid
> ioremapping the same page multiple times, but I see this as a matter of
> different preferences, so I'm fine with that.
> 
> You are still missing a description of the driver, both in the changelog
> above and in the Kconfig patch. A small sentence to spell out PWM
> in the changelog and to list the devices that will use this driver
> would be helpful to the causal reader.

Ok, will add.

> 
> Provided that you add that:
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 93c1052..5694574 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -2,11 +2,15 @@ menuconfig PWM
> >  	bool "PWM Support"
> >  	help
> >  	  This enables PWM support through the generic PWM framework.
> > -	  You only need to enable this, if you also want to enable
> > +	  You only need to enable this if you also want to enable
> >  	  one or more of the PWM drivers below.
> >  
> >  	  If unsure, say N.
> >  
> 
> I think this hunk should have been in the other first patch.

Indeed, will fix.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |