[RFC 0/7] Add PWM device-tree support.

[RFC 0/7] Add PWM device-tree support.

[Thierry Reding]

This patch series adds very rudimentary device-tree support for PWM devices.
I fully realize that this is early work and it should not be merged as is.
However I wanted to post these patches for review to make sure I'm not on a
wild-goose chase.

With all of these patches applied (plus one board-specific patch that is not
included), I'm able to control the backlight on the device I'm working on
using the sysfs interface provided by the pwm-backlight driver and the
backlight class.

This series is based on Sascha Hauer's series of patches[0] to add a generic
PWM framework. The first patch in this series is takes from Sascha's branch,
while the second patch enables each PWM chip to provide multiple PWM devices
(none of the drivers in Sascha's branch are converted yet).

Patch 3 adds some code to lookup a PWM chip given its device-tree handle. This
code will be used later on by the pwm-backlight driver to find the PWM device
that it should be using. I did not include any binding documentation yet since
I wasn't sure the DT binding was at all sensible.

Patch 4 was taken from the Chromium tree and is required to provide proper
clocking of the Tegra2 PWFM controller. All Chromium-specific tags have been
removed from the commit message.

Patch 5 cleans up the clock registration for Tegra2 because patch 6 will only
instantiate one device for the PWFM controller instead of four.

Patch 6 adds a generic PWM framework driver for the Tegra2 PWFM controller.
The code is taken from the Chromium tree with some adjustments to integrate it
with the PWM framework.

Patch 7 implements DT-based probing in the pwm-backlight driver. Note that
this code only handles the "pwm" property (by looking up the PWM device via
the new PWM DT binding) and the "default-" and "max-brightness" properties.
Switching power to the backlight via GPIOs is not supported yet.

[0]: http://git.pengutronix.de/?p=imx/linux-2.6.git;a=shortlog;h=refs/heads/pwmlib

Sascha Hauer (1):
  PWM: add pwm framework support

Simon Que (1):
  arm: tegra: Fix PWM clock programming

Thierry Reding (5):
  pwm: Allow chips to support multiple PWMs.
  of: Add PWM support.
  arm: tegra: Provide clock for only one PWM controller.
  pwm: Add Tegra2 SoC support
  pwm-backlight: Add rudimentary device-tree support

 .../bindings/video/backlight/pwm-backlight         |   16 +
 Documentation/pwm.txt                              |   56 ++++
 MAINTAINERS                                        |    6 +
 arch/arm/boot/dts/tegra20.dtsi                     |    6 +
 arch/arm/mach-tegra/board-dt.c                     |    1 +
 arch/arm/mach-tegra/clock.h                        |    1 +
 arch/arm/mach-tegra/devices.c                      |   15 +
 arch/arm/mach-tegra/devices.h                      |    1 +
 arch/arm/mach-tegra/tegra2_clocks.c                |   33 ++-
 drivers/Kconfig                                    |    2 +
 drivers/Makefile                                   |    1 +
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/pwm.c                                   |   88 ++++++
 drivers/pwm/Kconfig                                |   17 ++
 drivers/pwm/Makefile                               |    2 +
 drivers/pwm/core.c                                 |  301 ++++++++++++++++++++
 drivers/pwm/pwm-tegra.c                            |  274 ++++++++++++++++++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |   70 +++++-
 include/linux/of_pwm.h                             |   41 +++
 include/linux/pwm.h                                |   49 ++++
 22 files changed, 976 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/pwm-backlight
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/of/pwm.c
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/core.c
 create mode 100644 drivers/pwm/pwm-tegra.c
 create mode 100644 include/linux/of_pwm.h

-- 
1.7.8

[RFC 1/7] PWM: add pwm framework support

[Thierry Reding]

From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Acked-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Reviewed-by: Matthias Kaehlcke <matthias-RprLehDfhQ3k1uMJSBkQmQ@public.gmane.org>
---
 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 4475602..6f7b382 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5315,6 +5315,12 @@ S:	Maintained
 F:	Documentation/video4linux/README.pvrusb2
 F:	drivers/media/video/pvrusb2/
 
+PPWM core
+M:	Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+L:	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	drivers/pwm/
+
 PXA2xx/PXA3xx SUPPORT
 M:	Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
 M:	Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index b5e6f24..29252dc 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -136,4 +136,6 @@ source "drivers/hv/Kconfig"
 
 source "drivers/devfreq/Kconfig"
 
+source "drivers/pwm/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 1b31421..991a40f 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -8,6 +8,7 @@
 # GPIO must come after pinctrl as gpios may need to mux pins etc
 obj-y				+= pinctrl/
 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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ *
+ *  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.8

[RFC 2/7] pwm: Allow chips to support multiple PWMs.

[Thierry Reding]

This commit modifies the PWM core to support multiple PWMs per struct
pwm_chip. It achieves this in a similar way to how gpiolib works, by
allowing PWM ranges to be requested dynamically (pwm_chip.base == -1) or
starting at a given offset (pwm_chip.base >= 0). A chip specifies how
many PWMs it controls using the npwm member. Each of the functions in
the pwm_ops structure gets an additional argument that specified the PWM
number (it can be converted to a per-chip index by subtracting the
chip's base).

The total maximum number of PWM devices is currently fixed to 64, but
can easily be made configurable via Kconfig.

The patch is incomplete in that it doesn't convert any existing drivers
that are now broken.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/pwm/core.c  |  213 +++++++++++++++++++++++++++++++++++----------------
 include/linux/pwm.h |   20 ++++--
 2 files changed, 161 insertions(+), 72 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 71de479..b4c22a9 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -17,6 +17,7 @@
  *  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>
@@ -25,63 +26,106 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 
+#define NR_PWMS 64
+#define PWM_BITMAP_BITS NR_PWMS
+
 struct pwm_device {
-	struct			pwm_chip *chip;
-	const char		*label;
+	const char *label;
+	unsigned int pwm;
+};
+
+struct pwm_desc {
+	struct pwm_chip		*chip;
 	unsigned long		flags;
 #define FLAG_REQUESTED	0
 #define FLAG_ENABLED	1
-	struct list_head	node;
+	struct pwm_device	pwm;
 };
 
-static LIST_HEAD(pwm_list);
-
 static DEFINE_MUTEX(pwm_lock);
+static DECLARE_BITMAP(allocated_pwms, NR_PWMS);
+static struct pwm_desc pwm_desc[NR_PWMS];
+
+static struct pwm_desc *pwm_to_desc(unsigned int pwm)
+{
+	return (pwm < NR_PWMS) ? &pwm_desc[pwm] : NULL;
+}
 
-static struct pwm_device *_find_pwm(int pwm_id)
+static void free_pwm(unsigned int pwm)
 {
-	struct pwm_device *pwm;
+	struct pwm_desc *desc = pwm_to_desc(pwm);
 
-	list_for_each_entry(pwm, &pwm_list, node) {
-		if (pwm->chip->pwm_id == pwm_id)
-			return pwm;
+	desc->pwm.label = NULL;
+	desc->flags = 0;
+	desc->chip = NULL;
+}
+
+static int __alloc_pwms(int pwm, unsigned int from, unsigned int count)
+{
+	unsigned int start;
+
+	if (pwm >= 0) {
+		if (from > pwm)
+			return -EINVAL;
+
+		from = pwm;
 	}
 
-	return NULL;
+	start = bitmap_find_next_zero_area(allocated_pwms, NR_PWMS, from,
+			count, 0);
+
+	if ((pwm >= 0) && (start != pwm))
+		return -EEXIST;
+
+	if (start + count > NR_PWMS)
+		return -ENOSPC;
+
+	bitmap_set(allocated_pwms, start, count);
+
+	return start;
+}
+
+static void __free_pwms(unsigned int from, unsigned int count)
+{
+	unsigned int i;
+
+	for (i = 0; i < count; i++)
+		free_pwm(from + i);
+
+	bitmap_clear(allocated_pwms, from, count);
 }
 
 /**
- * pwmchip_add() - register a new pwm
- * @chip: the pwm
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip
  *
- * 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,
+ * Register a new PWM chip. If pwm->base < 0 then a dynamically assigned base
+ * will be used.
  */
 int pwmchip_add(struct pwm_chip *chip)
 {
-	struct pwm_device *pwm;
+	unsigned int i;
 	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;
+	ret = __alloc_pwms(chip->base, 0, chip->npwm);
+	if (ret < 0)
 		goto out;
+
+	chip->base = ret;
+
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_desc *desc = pwm_to_desc(chip->base + i);
+
+		desc->chip = chip;
+		desc->flags = 0;
 	}
 
-	list_add_tail(&pwm->node, &pwm_list);
+	ret = 0;
+
 out:
 	mutex_unlock(&pwm_lock);
-
-	if (ret)
-		kfree(pwm);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pwmchip_add);
@@ -94,77 +138,106 @@ EXPORT_SYMBOL_GPL(pwmchip_add);
  */
 int pwmchip_remove(struct pwm_chip *chip)
 {
-	struct pwm_device *pwm;
+	unsigned int i;
 	int ret = 0;
 
 	mutex_lock(&pwm_lock);
 
-	pwm = _find_pwm(chip->pwm_id);
-	if (!pwm) {
-		ret = -ENOENT;
-		goto out;
-	}
+	for (i = chip->base; i < chip->base + chip->npwm; i++) {
+		struct pwm_desc *desc = pwm_to_desc(i);
 
-	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
-		ret = -EBUSY;
-		goto out;
+		if (test_bit(FLAG_REQUESTED, &desc->flags)) {
+			ret = -EBUSY;
+			goto out;
+		}
 	}
 
-	list_del(&pwm->node);
+	__free_pwms(chip->base, chip->npwm);
 
-	kfree(pwm);
 out:
 	mutex_unlock(&pwm_lock);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
+/**
+ * pwmchip_find() - iterator for locating a specific pwm_chip
+ * @data: data to pass to match function
+ * @match: callback function to check pwm_chip
+ */
+struct pwm_chip *pwmchip_find(void *data, int (*match)(struct pwm_chip *chip,
+						       void *data))
+{
+	struct pwm_chip *chip = NULL;
+	unsigned int i;
+
+	mutex_lock(&pwm_lock);
+
+	for (i = 0; i < NR_PWMS; i++) {
+		struct pwm_desc *desc = pwm_to_desc(i);
+
+		if (!desc->chip)
+			continue;
+
+		if (match(desc->chip, data)) {
+			chip = desc->chip;
+			break;
+		}
+	}
+
+	mutex_unlock(&pwm_lock);
+
+	return chip;
+}
+EXPORT_SYMBOL_GPL(pwmchip_find);
+
 /*
  * pwm_request - request a PWM device
  */
-struct pwm_device *pwm_request(int pwm_id, const char *label)
+struct pwm_device *pwm_request(int pwm, const char *label)
 {
-	struct pwm_device *pwm;
+	struct pwm_device *dev;
+	struct pwm_desc *desc;
 	int ret;
 
+	if ((pwm < 0) || (pwm >= NR_PWMS))
+		return ERR_PTR(-ENOENT);
+
 	mutex_lock(&pwm_lock);
 
-	pwm = _find_pwm(pwm_id);
-	if (!pwm) {
-		pwm = ERR_PTR(-ENOENT);
-		goto out;
-	}
+	desc = pwm_to_desc(pwm);
+	dev = &desc->pwm;
 
-	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
-		pwm = ERR_PTR(-EBUSY);
+	if (test_bit(FLAG_REQUESTED, &desc->flags)) {
+		dev = ERR_PTR(-EBUSY);
 		goto out;
 	}
 
-	if (!try_module_get(pwm->chip->ops->owner)) {
-		pwm = ERR_PTR(-ENODEV);
+	if (!try_module_get(desc->chip->ops->owner)) {
+		dev = ERR_PTR(-ENODEV);
 		goto out;
 	}
 
-	if (pwm->chip->ops->request) {
-		ret = pwm->chip->ops->request(pwm->chip);
+	if (desc->chip->ops->request) {
+		ret = desc->chip->ops->request(desc->chip, pwm);
 		if (ret) {
-			pwm = ERR_PTR(ret);
+			dev = ERR_PTR(ret);
 			goto out_put;
 		}
 	}
 
-	pwm->label = label;
-	set_bit(FLAG_REQUESTED, &pwm->flags);
+	dev->label = label;
+	dev->pwm = pwm;
+	set_bit(FLAG_REQUESTED, &desc->flags);
 
 	goto out;
 
 out_put:
-	module_put(pwm->chip->ops->owner);
+	module_put(desc->chip->ops->owner);
 out:
 	mutex_unlock(&pwm_lock);
 
-	return pwm;
+	return dev;
 }
 EXPORT_SYMBOL_GPL(pwm_request);
 
@@ -173,16 +246,18 @@ EXPORT_SYMBOL_GPL(pwm_request);
  */
 void pwm_free(struct pwm_device *pwm)
 {
+	struct pwm_desc *desc = container_of(pwm, struct pwm_desc, pwm);
+
 	mutex_lock(&pwm_lock);
 
-	if (!test_and_clear_bit(FLAG_REQUESTED, &pwm->flags)) {
+	if (!test_and_clear_bit(FLAG_REQUESTED, &desc->flags)) {
 		pr_warning("PWM device already freed\n");
 		goto out;
 	}
 
 	pwm->label = NULL;
 
-	module_put(pwm->chip->ops->owner);
+	module_put(desc->chip->ops->owner);
 out:
 	mutex_unlock(&pwm_lock);
 }
@@ -193,7 +268,9 @@ EXPORT_SYMBOL_GPL(pwm_free);
  */
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 {
-	return pwm->chip->ops->config(pwm->chip, duty_ns, period_ns);
+	struct pwm_desc *desc = container_of(pwm, struct pwm_desc, pwm);
+	return desc->chip->ops->config(desc->chip, pwm->pwm, duty_ns,
+			period_ns);
 }
 EXPORT_SYMBOL_GPL(pwm_config);
 
@@ -202,8 +279,10 @@ EXPORT_SYMBOL_GPL(pwm_config);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (!test_and_set_bit(FLAG_ENABLED, &pwm->flags))
-		return pwm->chip->ops->enable(pwm->chip);
+	struct pwm_desc *desc = container_of(pwm, struct pwm_desc, pwm);
+
+	if (!test_and_set_bit(FLAG_ENABLED, &desc->flags))
+		return desc->chip->ops->enable(desc->chip, pwm->pwm);
 
 	return 0;
 }
@@ -214,7 +293,9 @@ EXPORT_SYMBOL_GPL(pwm_enable);
  */
 void pwm_disable(struct pwm_device *pwm)
 {
-	if (test_and_clear_bit(FLAG_ENABLED, &pwm->flags))
-		pwm->chip->ops->disable(pwm->chip);
+	struct pwm_desc *desc = container_of(pwm, struct pwm_desc, pwm);
+
+	if (test_and_clear_bit(FLAG_ENABLED, &desc->flags))
+		desc->chip->ops->disable(desc->chip, pwm->pwm);
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index df9681b..01c0153 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -40,12 +40,17 @@ struct pwm_chip;
  * @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			(*request)(struct pwm_chip *chip,
+						unsigned int pwm);
+	void			(*free)(struct pwm_chip *chip,
+						unsigned int pwm);
+	int			(*config)(struct pwm_chip *chip,
+						unsigned int pwm, int duty_ns,
 						int period_ns);
-	int			(*enable)(struct pwm_chip *chip);
-	void			(*disable)(struct pwm_chip *chip);
+	int			(*enable)(struct pwm_chip *chip,
+						unsigned int pwm);
+	void			(*disable)(struct pwm_chip *chip,
+						unsigned int pwm);
 	struct module		*owner;
 };
 
@@ -56,13 +61,16 @@ struct pwm_ops {
  * @ops: The callbacks for this PWM
  */
 struct pwm_chip {
-	int			pwm_id;
 	const char		*label;
 	struct pwm_ops		*ops;
+	int			base;
+	unsigned int		npwm;
 };
 
 int pwmchip_add(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
+struct pwm_chip *pwmchip_find(void *data, int (*match)(struct pwm_chip *chip,
+						       void *data));
 #endif
 
 #endif /* __LINUX_PWM_H */
-- 
1.7.8

[RFC 3/7] of: Add PWM support.

[Thierry Reding]

This patch adds helpers to support device-tree bindings for the generic
PWM API.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/of/Kconfig     |    6 +++
 drivers/of/Makefile    |    1 +
 drivers/of/pwm.c       |   88 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pwm.h |   41 ++++++++++++++++++++++
 include/linux/pwm.h    |    4 ++
 5 files changed, 140 insertions(+), 0 deletions(-)
 create mode 100644 drivers/of/pwm.c
 create mode 100644 include/linux/of_pwm.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index cac63c9..62a2073 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -47,6 +47,12 @@ config OF_GPIO
 	help
 	  OpenFirmware GPIO accessors
 
+config OF_PWM
+	def_bool y
+	depends on PWM
+	help
+	  OpenFirmware PWM accessors
+
 config OF_I2C
 	def_tristate I2C
 	depends on I2C && !SPARC
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index dccb117..c3acf27 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_OF_ADDRESS)  += address.o
 obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
+obj-$(CONFIG_OF_PWM)	+= pwm.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
 obj-$(CONFIG_OF_SPI)	+= of_spi.o
diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c
new file mode 100644
index 0000000..c0ef302
--- /dev/null
+++ b/drivers/of/pwm.c
@@ -0,0 +1,88 @@
+/*
+ * OF helpers for the PWM API
+ *
+ * Copyright (c) 2011 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+
+static int of_pwmchip_is_match(struct pwm_chip *chip, void *data)
+{
+	return chip->of_node == data;
+}
+
+/**
+ * of_node_to_pwmchip() - finds the PWM chip associated with a device node
+ * @np:		device node of the PWM chip
+ *
+ * Returns a pointer to the PWM chip associated with the specified device
+ * node or NULL if the device node doesn't represent a PWM chip.
+ */
+struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
+{
+	return pwmchip_find(np, of_pwmchip_is_match);
+}
+
+/**
+ * of_get_named_pwm() - get a PWM number and period to use with the PWM API
+ * @np:		device node to get the PWM from
+ * @propname:	property name containing PWM specifier(s)
+ * @index:	index of the PWM
+ * @period_ns:	a pointer to the PWM period (in ns) to fill in
+ *
+ * Returns PWM number to use with the Linux generic PWM API or a negative
+ * error code on failure. If @period_ns is not NULL the function fills in
+ * the value parsed from the period-ns property.
+ */
+int of_get_named_pwm(struct device_node *np, const char *propname,
+		     int index, unsigned int *period_ns)
+{
+	struct device_node *pwm_np;
+	const void *pwm_spec;
+	struct pwm_chip *pc;
+	const __be32 *spec;
+	u32 cells;
+	u32 pwm;
+	int ret;
+
+	ret = of_parse_phandles_with_args(np, propname, "#pwm-cells", index,
+					  &pwm_np, &pwm_spec);
+	if (ret) {
+		pr_debug("%s(): can't parse pwms property\n", __func__);
+		goto out;
+	}
+
+	pc = of_node_to_pwmchip(pwm_np);
+	if (!pc) {
+		ret = -ENODEV;
+		goto put;
+	}
+
+	ret = of_property_read_u32(pwm_np, "#pwm-cells", &cells);
+	if (ret < 0)
+		goto put;
+
+	spec = (const __be32 *)pwm_spec;
+	pwm = be32_to_cpup(pwm_spec);
+
+	if ((cells > 1) && period_ns)
+		*period_ns = be32_to_cpu(spec[1]);
+
+	ret = pc->base + pwm;
+
+put:
+	of_node_put(pwm_np);
+out:
+	pr_debug("%s() exited with status %d\n", __func__, ret);
+	return ret;
+}
+EXPORT_SYMBOL(of_get_named_pwm);
diff --git a/include/linux/of_pwm.h b/include/linux/of_pwm.h
new file mode 100644
index 0000000..8d6d569
--- /dev/null
+++ b/include/linux/of_pwm.h
@@ -0,0 +1,41 @@
+/*
+ * OF helpers for the PWM API
+ *
+ * Copyright (c) 2011 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_OF_PWM_H
+#define __LINUX_OF_PWM_H
+
+#include <linux/pwm.h>
+
+struct device_node;
+
+#ifdef CONFIG_OF_PWM
+
+struct pwm_chip *of_node_to_pwmchip(struct device_node *np);
+int of_get_named_pwm(struct device_node *np, const char *propname,
+		     int index, unsigned int *period_ns);
+
+#else
+
+static inline struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
+{
+	return NULL;
+}
+
+static inline int of_get_named_pwm(struct device_node *np,
+				   const char *propname, int index,
+				   unsigned int *period_ns)
+{
+	return -ENOSYS;
+}
+
+#endif /* CONFIG_OF_PWM */
+
+#endif /* __LINUX_OF_PWM_H */
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 01c0153..d6be4e4 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -65,6 +65,10 @@ struct pwm_chip {
 	struct pwm_ops		*ops;
 	int			base;
 	unsigned int		npwm;
+
+#ifdef CONFIG_OF_PWM
+	struct device_node	*of_node;
+#endif
 };
 
 int pwmchip_add(struct pwm_chip *chip);
-- 
1.7.8

[RFC 4/7] arm: tegra: Fix PWM clock programming

[Thierry Reding]

From: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

PWM clock source registers in Tegra 2 have different clock source selection bit
fields than other registers.  PWM clock source bits in CLK_SOURCE_PWM_0 register
are located at bit field bit[30:28] while others are at bit field bit[31:30] in
their respective clock source register.

This patch updates the clock programming to correctly reflect that, by adding a
flag to indicate the alternate bit field format and checking for it when
selecting a clock source (parent clock).

Also, adjusts for the frequency divider being offset by 1.

Signed-off-by: Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 arch/arm/mach-tegra/clock.h         |    1 +
 arch/arm/mach-tegra/tegra2_clocks.c |   28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/clock.h b/arch/arm/mach-tegra/clock.h
index 688316a..ebf4bcd 100644
--- a/arch/arm/mach-tegra/clock.h
+++ b/arch/arm/mach-tegra/clock.h
@@ -39,6 +39,7 @@
 #define PERIPH_MANUAL_RESET	(1 << 12)
 #define PLL_ALT_MISC_REG	(1 << 13)
 #define PLLU			(1 << 14)
+#define PERIPH_SOURCE_CLK_4BIT	(1 << 15)
 #define ENABLE_ON_INIT		(1 << 28)
 
 struct clk;
diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c
index 371869d..21729fa 100644
--- a/arch/arm/mach-tegra/tegra2_clocks.c
+++ b/arch/arm/mach-tegra/tegra2_clocks.c
@@ -69,6 +69,8 @@
 
 #define PERIPH_CLK_SOURCE_MASK		(3<<30)
 #define PERIPH_CLK_SOURCE_SHIFT		30
+#define PERIPH_CLK_SOURCE_4BIT_MASK	(7<<28)
+#define PERIPH_CLK_SOURCE_4BIT_SHIFT	28
 #define PERIPH_CLK_SOURCE_ENABLE	(1<<28)
 #define PERIPH_CLK_SOURCE_DIVU71_MASK	0xFF
 #define PERIPH_CLK_SOURCE_DIVU16_MASK	0xFFFF
@@ -920,9 +922,16 @@ static void tegra2_periph_clk_init(struct clk *c)
 	u32 val = clk_readl(c->reg);
 	const struct clk_mux_sel *mux = NULL;
 	const struct clk_mux_sel *sel;
+	u32 shift;
+
+	if (c->flags & PERIPH_SOURCE_CLK_4BIT)
+		shift = PERIPH_CLK_SOURCE_4BIT_SHIFT;
+	else
+		shift = PERIPH_CLK_SOURCE_SHIFT;
+
 	if (c->flags & MUX) {
 		for (sel = c->inputs; sel->input != NULL; sel++) {
-			if (val >> PERIPH_CLK_SOURCE_SHIFT == sel->value)
+			if (val >> shift == sel->value)
 				mux = sel;
 		}
 		BUG_ON(!mux);
@@ -1035,12 +1044,23 @@ static int tegra2_periph_clk_set_parent(struct clk *c, struct clk *p)
 {
 	u32 val;
 	const struct clk_mux_sel *sel;
+	u32 mask, shift;
+
 	pr_debug("%s: %s %s\n", __func__, c->name, p->name);
+
+	if (c->flags & PERIPH_SOURCE_CLK_4BIT) {
+		mask = PERIPH_CLK_SOURCE_4BIT_MASK;
+		shift = PERIPH_CLK_SOURCE_4BIT_SHIFT;
+	} else {
+		mask = PERIPH_CLK_SOURCE_MASK;
+		shift = PERIPH_CLK_SOURCE_SHIFT;
+	}
+
 	for (sel = c->inputs; sel->input != NULL; sel++) {
 		if (sel->input == p) {
 			val = clk_readl(c->reg);
-			val &= ~PERIPH_CLK_SOURCE_MASK;
-			val |= (sel->value) << PERIPH_CLK_SOURCE_SHIFT;
+			val &= ~mask;
+			val |= (sel->value) << shift;
 
 			if (c->refcnt)
 				clk_enable(p);
@@ -2133,7 +2153,7 @@ static struct clk tegra_list_clks[] = {
 	PERIPH_CLK("i2s2",	"tegra-i2s.1",		NULL,	18,	0x104,	26000000,  mux_pllaout0_audio2x_pllp_clkm,	MUX | DIV_U71),
 	PERIPH_CLK("spdif_out",	"spdif_out",		NULL,	10,	0x108,	100000000, mux_pllaout0_audio2x_pllp_clkm,	MUX | DIV_U71),
 	PERIPH_CLK("spdif_in",	"spdif_in",		NULL,	10,	0x10c,	100000000, mux_pllp_pllc_pllm,		MUX | DIV_U71),
-	PERIPH_CLK("pwm",	"pwm",			NULL,	17,	0x110,	432000000, mux_pllp_pllc_audio_clkm_clk32,	MUX | DIV_U71),
+	PERIPH_CLK("pwm",	"pwm",			NULL,	17,	0x110,	432000000, mux_pllp_pllc_audio_clkm_clk32,	MUX | DIV_U71 | PERIPH_SOURCE_CLK_4BIT),
 	PERIPH_CLK("spi",	"spi",			NULL,	43,	0x114,	40000000,  mux_pllp_pllc_pllm_clkm,	MUX | DIV_U71),
 	PERIPH_CLK("xio",	"xio",			NULL,	45,	0x120,	150000000, mux_pllp_pllc_pllm_clkm,	MUX | DIV_U71),
 	PERIPH_CLK("twc",	"twc",			NULL,	16,	0x12c,	150000000, mux_pllp_pllc_pllm_clkm,	MUX | DIV_U71),
-- 
1.7.8

[RFC 5/7] arm: tegra: Provide clock for only one PWM controller.

[Thierry Reding]

A subsequent patch will add a generic PWM API driver for the Tegra2 PWFM
controller, supporting all four PWM devices with a single PWM chip. The
device will be named tegra-pwm and only one clock needs to be provided.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 arch/arm/mach-tegra/tegra2_clocks.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c
index 21729fa..a5fb09a 100644
--- a/arch/arm/mach-tegra/tegra2_clocks.c
+++ b/arch/arm/mach-tegra/tegra2_clocks.c
@@ -2252,10 +2252,7 @@ static struct clk_duplicate tegra_clk_duplicates[] = {
 	CLK_DUPLICATE("usbd", "tegra-otg", NULL),
 	CLK_DUPLICATE("hdmi", "tegradc.0", "hdmi"),
 	CLK_DUPLICATE("hdmi", "tegradc.1", "hdmi"),
-	CLK_DUPLICATE("pwm", "tegra_pwm.0", NULL),
-	CLK_DUPLICATE("pwm", "tegra_pwm.1", NULL),
-	CLK_DUPLICATE("pwm", "tegra_pwm.2", NULL),
-	CLK_DUPLICATE("pwm", "tegra_pwm.3", NULL),
+	CLK_DUPLICATE("pwm", "tegra-pwm", NULL),
 	CLK_DUPLICATE("host1x", "tegra_grhost", "host1x"),
 	CLK_DUPLICATE("2d", "tegra_grhost", "gr2d"),
 	CLK_DUPLICATE("3d", "tegra_grhost", "gr3d"),
-- 
1.7.8

[RFC 6/7] pwm: Add Tegra2 SoC support

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 arch/arm/boot/dts/tegra20.dtsi |    6 +
 arch/arm/mach-tegra/board-dt.c |    1 +
 arch/arm/mach-tegra/devices.c  |   15 +++
 arch/arm/mach-tegra/devices.h  |    1 +
 drivers/pwm/Kconfig            |    5 +
 drivers/pwm/Makefile           |    1 +
 drivers/pwm/pwm-tegra.c        |  274 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 303 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-tegra.c

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 168545e..1c58dce 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -122,6 +122,12 @@
 		interrupts = < 0 91 0x04 >;
 	};
 
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 	sdhci@c8000000 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000000 0x200>;
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 2fa599d..387146c 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -81,6 +81,7 @@ struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
 		       &tegra_ehci2_device.dev.platform_data),
 	OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",
 		       &tegra_ehci3_device.dev.platform_data),
+	OF_DEV_AUXDATA("nvidia,tegra20-pwm", TEGRA_PWFM_BASE, "tegra-pwm", NULL),
 	{}
 };
 
diff --git a/arch/arm/mach-tegra/devices.c b/arch/arm/mach-tegra/devices.c
index 7a2a02d..c67d85d 100644
--- a/arch/arm/mach-tegra/devices.c
+++ b/arch/arm/mach-tegra/devices.c
@@ -704,3 +704,18 @@ struct platform_device tegra_pcm_device = {
 	.name = "tegra-pcm-audio",
 	.id = -1,
 };
+
+static struct resource tegra_pwm_resources[] = {
+	[0] = {
+		.start = TEGRA_PWFM_BASE,
+		.end = TEGRA_PWFM_BASE + TEGRA_PWFM_SIZE - 1,
+		.flags = IORESOURCE_MEM,
+	},
+};
+
+struct platform_device tegra_pwm_device = {
+	.name = "tegra-pwm",
+	.id = -1,
+	.num_resources = ARRAY_SIZE(tegra_pwm_resources),
+	.resource = tegra_pwm_resources,
+};
diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
index 873ecb2..a8a5e25 100644
--- a/arch/arm/mach-tegra/devices.h
+++ b/arch/arm/mach-tegra/devices.h
@@ -48,5 +48,6 @@ extern struct platform_device tegra_i2s_device1;
 extern struct platform_device tegra_i2s_device2;
 extern struct platform_device tegra_das_device;
 extern struct platform_device tegra_pcm_device;
+extern struct platform_device tegra_pwm_device;
 
 #endif
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 93c1052..7c6a137 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -9,4 +9,9 @@ menuconfig PWM
 
 if PWM
 
+config PWM_TEGRA
+	tristate "NVIDIA Tegra PWM support"
+	depends on ARCH_TEGRA
+	default n
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 3469c3d..12300f5 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
new file mode 100644
index 0000000..fd2f075
--- /dev/null
+++ b/drivers/pwm/pwm-tegra.c
@@ -0,0 +1,274 @@
+/*
+ * drivers/pwm/pwm-tegra.c
+ *
+ * Tegra pulse-width-modulation controller driver
+ *
+ * Copyright (c) 2010, NVIDIA Corporation.
+ * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define PWM_ENABLE	(1 << 31)
+#define PWM_DUTY_WIDTH	8
+#define PWM_DUTY_SHIFT	16
+#define PWM_SCALE_WIDTH	13
+#define PWM_SCALE_SHIFT	0
+
+struct tegra_pwm_chip {
+	struct pwm_chip		chip;
+	struct device		*dev;
+
+	struct clk		*clk;
+
+	int			clk_enb[4];
+	void __iomem		*mmio_base;
+};
+
+static inline struct tegra_pwm_chip *to_tegra_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct tegra_pwm_chip, chip);
+}
+
+static inline int pwm_writel(struct tegra_pwm_chip *chip, unsigned int pwm,
+		unsigned long val)
+{
+	unsigned long offset = pwm << 4;
+	int rc;
+
+	rc = clk_enable(chip->clk);
+	if (WARN_ON(rc))
+		return rc;
+
+	writel(val, chip->mmio_base + offset);
+	clk_disable(chip->clk);
+
+	return 0;
+}
+
+static int tegra_pwm_config(struct pwm_chip *chip, unsigned int pwm,
+		int duty_ns, int period_ns)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+	unsigned int num = pwm - chip->base;
+	unsigned long long c;
+	unsigned long rate, hz;
+	u32 val = 0;
+
+	/* convert from duty_ns / period_ns to a fixed number of duty
+	 * ticks per (1 << PWM_DUTY_WIDTH) cycles.
+	 */
+	c = duty_ns * ((1 << PWM_DUTY_WIDTH) - 1);
+	do_div(c, period_ns);
+
+	val = (u32)c << PWM_DUTY_SHIFT;
+
+	/* compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
+	 * cycles at the PWM clock rate will take period_ns nanoseconds.
+	 */
+	rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+	hz = 1000000000ul / period_ns;
+
+	rate = (rate + (hz / 2)) / hz;
+
+	/* Since the actual PWM divider is the register's frequency divider
+	 * field minus 1, we need to decrement to get the correct value to write
+	 * to the register.
+	 */
+	if (rate > 0)
+		rate--;
+
+	/* Make sure that the rate will fit in the register's frequency divider
+	 * field.
+	 */
+	if (rate >> PWM_SCALE_WIDTH)
+		return -EINVAL;
+
+	val |= (rate << PWM_SCALE_SHIFT);
+
+	/* the struct clk may be shared across multiple PWM devices, so
+	 * only enable the PWM if this device has been enabled
+	 */
+	if (pc->clk_enb[num])
+		val |= PWM_ENABLE;
+
+	return pwm_writel(pc, num, val);
+}
+
+static int tegra_pwm_enable(struct pwm_chip *chip, unsigned int pwm)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+	unsigned int num = pwm - chip->base;
+	int rc = 0;
+
+	if (!pc->clk_enb[num]) {
+		rc = clk_enable(pc->clk);
+		if (!rc) {
+			unsigned long offset = num << 4;
+			u32 val;
+
+			val = readl(pc->mmio_base + offset);
+			val |= PWM_ENABLE;
+			writel(val, pc->mmio_base + offset);
+
+			pc->clk_enb[num] = 1;
+		}
+	}
+
+	return 0;
+}
+
+static void tegra_pwm_disable(struct pwm_chip *chip, unsigned int pwm)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+	unsigned int num = pwm - chip->base;
+
+	if (pc->clk_enb[num]) {
+		unsigned long offset = num << 4;
+		u32 val;
+
+		val = readl(pc->mmio_base + offset);
+		val &= ~PWM_ENABLE;
+		writel(val, pc->mmio_base + offset);
+
+		clk_disable(pc->clk);
+		pc->clk_enb[num] = 0;
+	}
+}
+
+static struct pwm_ops tegra_pwm_ops = {
+	.config = tegra_pwm_config,
+	.enable = tegra_pwm_enable,
+	.disable = tegra_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int tegra_pwm_probe(struct platform_device *pdev)
+{
+	struct tegra_pwm_chip *pwm;
+	struct resource *r;
+	int ret;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	pwm->dev = &pdev->dev;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "no memory resources defined\n");
+		return -ENODEV;
+	}
+
+	r = devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
+			pdev->name);
+	if (!r) {
+		dev_err(&pdev->dev, "failed to request memory\n");
+		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() region\n");
+		return -ENODEV;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+	pwm->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return PTR_ERR(pwm->clk);
+
+	pwm->chip.label = "tegra-pwm";
+	pwm->chip.ops = &tegra_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = 4;
+
+#ifdef CONFIG_OF_PWM
+	pwm->chip.of_node = pdev->dev.of_node;
+#endif
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		clk_put(pwm->clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devexit tegra_pwm_remove(struct platform_device *pdev)
+{
+	struct tegra_pwm_chip *pwm = platform_get_drvdata(pdev);
+	int i;
+
+	if (WARN_ON(!pwm))
+		return -ENODEV;
+
+	for (i = 0; i < 4; i++) {
+		pwm_writel(pwm, i, 0);
+
+		if (pwm->clk_enb[i])
+			clk_disable(pwm->clk);
+	}
+
+	clk_put(pwm->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id tegra_pwm_of_match[] = {
+	{ .compatible = "nvidia,tegra20-pwm" },
+	{ }
+};
+#endif
+
+static struct platform_driver tegra_pwm_driver = {
+	.driver		= {
+		.name		= "tegra-pwm",
+		.of_match_table	= of_match_ptr(tegra_pwm_of_match),
+	},
+	.probe		= tegra_pwm_probe,
+	.remove		= __devexit_p(tegra_pwm_remove),
+};
+
+static int __init tegra_pwm_init(void)
+{
+	return platform_driver_register(&tegra_pwm_driver);
+}
+subsys_initcall(tegra_pwm_init);
+
+static void __exit tegra_pwm_exit(void)
+{
+	platform_driver_unregister(&tegra_pwm_driver);
+}
+module_exit(tegra_pwm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("NVIDIA Corporation");
-- 
1.7.8

[RFC 7/7] pwm-backlight: Add rudimentary device-tree support

[Thierry Reding]

This commit adds very basic support for device-tree probing. Currently,
only a PWM and maximum and default brightness values can be specified.
Enabling or disabling backlight power via GPIOs is not yet supported.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 .../bindings/video/backlight/pwm-backlight         |   16 +++++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |   70 ++++++++++++++++++-
 3 files changed, 83 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/pwm-backlight

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
new file mode 100644
index 0000000..ce65280
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
@@ -0,0 +1,16 @@
+pwm-backlight bindings
+
+Required properties:
+  - compatible: "pwm-backlight"
+  - default-brightness: the default brightness setting
+  - max-brightness: the maximum brightness setting
+  - pwm: OF device-tree PWM specification
+
+Example:
+
+	backlight {
+		compatible = "pwm-backlight";
+		default-brightness = <224>;
+		max-brightness = <255>;
+		pwm = <&pwm 0 5000000>;
+	};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 278aeaa..51c3642 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -233,7 +233,7 @@ config BACKLIGHT_CARILLO_RANCH
 
 config BACKLIGHT_PWM
 	tristate "Generic PWM based Backlight Driver"
-	depends on HAVE_PWM
+	depends on HAVE_PWM || PWM
 	help
 	  If you have a LCD backlight adjustable by PWM, say Y to enable
 	  this driver.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 8b5b2a4..742934c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -17,6 +17,7 @@
 #include <linux/fb.h>
 #include <linux/backlight.h>
 #include <linux/err.h>
+#include <linux/of_pwm.h>
 #include <linux/pwm.h>
 #include <linux/pwm_backlight.h>
 #include <linux/slab.h>
@@ -83,17 +84,77 @@ static const struct backlight_ops pwm_backlight_ops = {
 	.check_fb	= pwm_backlight_check_fb,
 };
 
+#ifdef CONFIG_OF
+static int pwm_backlight_parse_dt(struct device *dev,
+				  struct platform_pwm_backlight_data *data)
+{
+	struct device_node *node = dev->of_node;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	memset(data, 0, sizeof(*data));
+
+	ret = of_get_named_pwm(node, "pwm", 0, &data->pwm_period_ns);
+	if (ret < 0)
+		return ret;
+
+	data->pwm_id = ret;
+
+	ret = of_property_read_u32(node, "default-brightness", &value);
+	if (ret < 0)
+		return ret;
+
+	data->dft_brightness = value;
+
+	ret = of_property_read_u32(node, "max-brightness", &value);
+	if (ret < 0)
+		return ret;
+
+	data->max_brightness = value;
+
+	/*
+	 * TODO: Most users of this driver use a number of GPIOs to control
+	 *       backlight power. Support for specifying these needs to be
+	 *       added.
+	 */
+
+	return 0;
+}
+
+static struct of_device_id pwm_backlight_of_match[] = {
+	{ .compatible = "pwm-backlight" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);
+#else
+static int pwm_backlight_parse_dt(struct device *dev,
+				  struct platform_pwm_backlight_data *data)
+{
+	return -ENODEV;
+}
+#endif
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct backlight_properties props;
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+	struct platform_pwm_backlight_data defdata;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
 	int ret;
 
 	if (!data) {
-		dev_err(&pdev->dev, "failed to find platform data\n");
-		return -EINVAL;
+		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to find platform data\n");
+			return ret;
+		}
+
+		data = &defdata;
 	}
 
 	if (data->init) {
@@ -198,8 +259,9 @@ static int pwm_backlight_resume(struct platform_device *pdev)
 
 static struct platform_driver pwm_backlight_driver = {
 	.driver		= {
-		.name	= "pwm-backlight",
-		.owner	= THIS_MODULE,
+		.name		= "pwm-backlight",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(pwm_backlight_of_match),
 	},
 	.probe		= pwm_backlight_probe,
 	.remove		= pwm_backlight_remove,
-- 
1.7.8

Re: [RFC 0/7] Add PWM device-tree support.

[Rob Herring]

On 12/20/2011 04:32 AM, Thierry Reding wrote:
> This patch series adds very rudimentary device-tree support for PWM devices.
> I fully realize that this is early work and it should not be merged as is.
> However I wanted to post these patches for review to make sure I'm not on a
> wild-goose chase.
> 
> With all of these patches applied (plus one board-specific patch that is not
> included), I'm able to control the backlight on the device I'm working on
> using the sysfs interface provided by the pwm-backlight driver and the
> backlight class.
> 
> This series is based on Sascha Hauer's series of patches[0] to add a generic
> PWM framework. The first patch in this series is takes from Sascha's branch,
> while the second patch enables each PWM chip to provide multiple PWM devices
> (none of the drivers in Sascha's branch are converted yet).
> 
> Patch 3 adds some code to lookup a PWM chip given its device-tree handle. This
> code will be used later on by the pwm-backlight driver to find the PWM device
> that it should be using. I did not include any binding documentation yet since
> I wasn't sure the DT binding was at all sensible.
> 
> Patch 4 was taken from the Chromium tree and is required to provide proper
> clocking of the Tegra2 PWFM controller. All Chromium-specific tags have been
> removed from the commit message.
> 
> Patch 5 cleans up the clock registration for Tegra2 because patch 6 will only
> instantiate one device for the PWFM controller instead of four.
> 
> Patch 6 adds a generic PWM framework driver for the Tegra2 PWFM controller.
> The code is taken from the Chromium tree with some adjustments to integrate it
> with the PWM framework.
> 
> Patch 7 implements DT-based probing in the pwm-backlight driver. Note that
> this code only handles the "pwm" property (by looking up the PWM device via
> the new PWM DT binding) and the "default-" and "max-brightness" properties.
> Switching power to the backlight via GPIOs is not supported yet.
> 
> [0]: http://git.pengutronix.de/?p=imx/linux-2.6.git;a=shortlog;h=refs/heads/pwmlib
> 
> Sascha Hauer (1):
>   PWM: add pwm framework support
> 
> Simon Que (1):
>   arm: tegra: Fix PWM clock programming
> 
> Thierry Reding (5):
>   pwm: Allow chips to support multiple PWMs.
>   of: Add PWM support.
>   arm: tegra: Provide clock for only one PWM controller.
>   pwm: Add Tegra2 SoC support
>   pwm-backlight: Add rudimentary device-tree support
> 
>  .../bindings/video/backlight/pwm-backlight         |   16 +
>  Documentation/pwm.txt                              |   56 ++++
>  MAINTAINERS                                        |    6 +
>  arch/arm/boot/dts/tegra20.dtsi                     |    6 +
>  arch/arm/mach-tegra/board-dt.c                     |    1 +
>  arch/arm/mach-tegra/clock.h                        |    1 +
>  arch/arm/mach-tegra/devices.c                      |   15 +
>  arch/arm/mach-tegra/devices.h                      |    1 +
>  arch/arm/mach-tegra/tegra2_clocks.c                |   33 ++-
>  drivers/Kconfig                                    |    2 +
>  drivers/Makefile                                   |    1 +
>  drivers/of/Kconfig                                 |    6 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/pwm.c                                   |   88 ++++++
>  drivers/pwm/Kconfig                                |   17 ++
>  drivers/pwm/Makefile                               |    2 +
>  drivers/pwm/core.c                                 |  301 ++++++++++++++++++++
>  drivers/pwm/pwm-tegra.c                            |  274 ++++++++++++++++++
>  drivers/video/backlight/Kconfig                    |    2 +-
>  drivers/video/backlight/pwm_bl.c                   |   70 +++++-
>  include/linux/of_pwm.h                             |   41 +++
>  include/linux/pwm.h                                |   49 ++++
>  22 files changed, 976 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/pwm-backlight
>  create mode 100644 Documentation/pwm.txt
>  create mode 100644 drivers/of/pwm.c
>  create mode 100644 drivers/pwm/Kconfig
>  create mode 100644 drivers/pwm/Makefile
>  create mode 100644 drivers/pwm/core.c
>  create mode 100644 drivers/pwm/pwm-tegra.c
>  create mode 100644 include/linux/of_pwm.h

You need binding documentation for the pwm portion.

Rob

Re: [RFC 0/7] Add PWM device-tree support.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 247 bytes --]

* Rob Herring wrote:
> You need binding documentation for the pwm portion.

Okay. I was going to add the documentation once this had seen some
discussion, but I guess you'd rather see the documentation first and discuss
it then, correct?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

RE: [RFC 1/7] PWM: add pwm framework support

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This patch adds framework support for PWM (pulse width modulation) devices.
...

> diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt

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

pwmchip_reserve() doesn't seem to exist in this patch.

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c

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

I don't see any code that assigns pwm_id if it's negative.

> + */
> +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);

There have been discussions before re: GPIOs and interrupts that creating
a global namespace for them was a mistake. This new PWM API does the same
thing. Is there an issue here?

I suppose that coming at it purely from a device tree perspective, the
numbering scheme doesn't matter, since it can be dynamically assigned
in the same way that global GPIO and IRQ IDs are with device tree. Still,
there's plenty of non-device tree stuff around, so this is probably worth
thinking about.

Is 0 a valid ID? Given previous GPIO and IRQ discussions, we should
disallow 0 explicitly. The rationale is that such an invalid ID can be
used to indicate "no PWM", and 0 is a good value for that, since it's
what uninitialized global data gets set to.

-- 
nvpublic

Re: [RFC 6/7] pwm: Add Tegra2 SoC support

[Olof Johansson]

Hi,

On Tue, Dec 20, 2011 at 2:32 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>

If the code is copyright nvidia without a signed-off-by from them,
then they at least deserve mentioning of original authorship in the
change log.

It's also proving itself easier to do the driver and the device tree
and board-dt update separately since they tend to have different merge
paths.

Care to split up as two patches, one for drivers/ and one for arch/arm/?


Thanks!

-Olof


> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> index 2fa599d..387146c 100644
> --- a/arch/arm/mach-tegra/board-dt.c
> +++ b/arch/arm/mach-tegra/board-dt.c
> @@ -81,6 +81,7 @@ struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
>                       &tegra_ehci2_device.dev.platform_data),
>        OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",
>                       &tegra_ehci3_device.dev.platform_data),
> +       OF_DEV_AUXDATA("nvidia,tegra20-pwm", TEGRA_PWFM_BASE, "tegra-pwm", NULL),
>        {}
>  };
>
> diff --git a/arch/arm/mach-tegra/devices.c b/arch/arm/mach-tegra/devices.c
> index 7a2a02d..c67d85d 100644
> --- a/arch/arm/mach-tegra/devices.c
> +++ b/arch/arm/mach-tegra/devices.c
> @@ -704,3 +704,18 @@ struct platform_device tegra_pcm_device = {
>        .name = "tegra-pcm-audio",
>        .id = -1,
>  };
> +
> +static struct resource tegra_pwm_resources[] = {
> +       [0] = {
> +               .start = TEGRA_PWFM_BASE,
> +               .end = TEGRA_PWFM_BASE + TEGRA_PWFM_SIZE - 1,
> +               .flags = IORESOURCE_MEM,
> +       },
> +};
> +
> +struct platform_device tegra_pwm_device = {
> +       .name = "tegra-pwm",
> +       .id = -1,
> +       .num_resources = ARRAY_SIZE(tegra_pwm_resources),
> +       .resource = tegra_pwm_resources,
> +};
> diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
> index 873ecb2..a8a5e25 100644
> --- a/arch/arm/mach-tegra/devices.h
> +++ b/arch/arm/mach-tegra/devices.h
> @@ -48,5 +48,6 @@ extern struct platform_device tegra_i2s_device1;
>  extern struct platform_device tegra_i2s_device2;
>  extern struct platform_device tegra_das_device;
>  extern struct platform_device tegra_pcm_device;
> +extern struct platform_device tegra_pwm_device;
>
>  #endif

I don't think you need the platform_device at all, since all platforms
should probe this device via DT (no legacy board file users in-tree).

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 93c1052..7c6a137 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -9,4 +9,9 @@ menuconfig PWM
>
>  if PWM
>
> +config PWM_TEGRA
> +       tristate "NVIDIA Tegra PWM support"
> +       depends on ARCH_TEGRA
> +       default n

No need to specify default n, since that's the default default. :)


-Olof

RE: [RFC 2/7] pwm: Allow chips to support multiple PWMs.

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This commit modifies the PWM core to support multiple PWMs per struct
> pwm_chip. It achieves this in a similar way to how gpiolib works, by
> allowing PWM ranges to be requested dynamically (pwm_chip.base == -1) or
> starting at a given offset (pwm_chip.base >= 0). A chip specifies how

Echoing my previous comment: >0 would be better than >=0 here.

> many PWMs it controls using the npwm member. Each of the functions in
> the pwm_ops structure gets an additional argument that specified the PWM
> number (it can be converted to a per-chip index by subtracting the
> chip's base).

I think it'd be much more convenient for drivers if the PWM core passed
the device-relative PWM ID to the driver functions instead of the global
PWM ID; the common case is going to be for the driver to want to index
into some local array using the value, which means all drivers will have
to subtract the chip base. I doubt many drivers will care about the global
ID at all, and if they do, they can add the base back on.

> The total maximum number of PWM devices is currently fixed to 64, but
> can easily be made configurable via Kconfig.

GPIO (and IRQ?) have static sized arrays for this kind of purpose too,
and I'm pretty sure I've seen discussions that this was a mistake, or
at least something that will hopefully be reworked. pinctrl was similar,
but it was requested this be reworked, and now I think the pin ID ->
pin descriptor table is a radix tree.

In other words, can you do away with NR_PWM, and make it completely
dynamic?

-- 
nvpublic

RE: [RFC 3/7] of: Add PWM support.

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This patch adds helpers to support device-tree bindings for the generic
> PWM API.

> diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c

> +/**
> + * of_get_named_pwm() - get a PWM number and period to use with the PWM API
> + * @np:		device node to get the PWM from
> + * @propname:	property name containing PWM specifier(s)
> + * @index:	index of the PWM
> + * @period_ns:	a pointer to the PWM period (in ns) to fill in
> + *
> + * Returns PWM number to use with the Linux generic PWM API or a negative
> + * error code on failure. If @period_ns is not NULL the function fills in
> + * the value parsed from the period-ns property.
> + */
> +int of_get_named_pwm(struct device_node *np, const char *propname,
> +		     int index, unsigned int *period_ns)
> +{
...
> +	if ((cells > 1) && period_ns)
> +		*period_ns = be32_to_cpu(spec[1]);

What if the client needs to know period_ns, yet the DT doesn't provide
it?

I think a better approach would be to use an "of_xlate" function like
GPIO and IRQ do. This way, the PWM device's own bindings get to define
what the extra cells mean, and the definition of of_xlate can be such
that it must return a period_ns value in all cases; in some cases, the
driver may return a hard-coded value if the HW doesn't support the
feature, whereas in most cases the value would be parsed from the
extra cells.

Without seeing the complete binding documentation and an example, it's
difficult to think about whether it's a good idea to include period_ns
in the PWM specifier or not. An alternative might be a property in the
PWM node itself.

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h

> @@ -65,6 +65,10 @@ struct pwm_chip {

> +#ifdef CONFIG_OF_PWM
> +	struct device_node	*of_node;
> +#endif
>  };

Can we remove the conditionals here? That would allow a PWM driver to
unconditionally assign this field, rather than having to wrap the 
assignment in ifdefs.

Actually, even better would be for struct pwm_chip to contain a struct
device * instead. That'd allow the PWM core to use that for e.g. dev_err
calls, and it includes the of_node for use in the match function above.

-- 
nvpublic

RE: [RFC 4/7] arm: tegra: Fix PWM clock programming

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> From: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> PWM clock source registers in Tegra 2 have different clock source selection bit
> fields than other registers.  PWM clock source bits in CLK_SOURCE_PWM_0 register
> are located at bit field bit[30:28] while others are at bit field bit[31:30] in
> their respective clock source register.
> 
> This patch updates the clock programming to correctly reflect that, by adding a
> flag to indicate the alternate bit field format and checking for it when
> selecting a clock source (parent clock).
> 
> Also, adjusts for the frequency divider being offset by 1.

That last line applies to the original patch in the ChromeOS tree, but
not to the patch you posted (the edit to arch/arm/mach-tegra/pwm.c that
was in the original patch isn't part of this patch).

-- 
nvpublic

RE: [RFC 5/7] arm: tegra: Provide clock for only one PWM controller.

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> A subsequent patch will add a generic PWM API driver for the Tegra2 PWFM
> controller, supporting all four PWM devices with a single PWM chip. The
> device will be named tegra-pwm and only one clock needs to be provided.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>

Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

-- 
nvpublic

RE: [RFC 6/7] pwm: Add Tegra2 SoC support

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>

This seems reasonable, besides Olof's comments. I made a few comments
below, although I understand most were present in the original patch
you're upstreaming.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig

> +config PWM_TEGRA
> +	tristate "NVIDIA Tegra PWM support"
> +	depends on ARCH_TEGRA
> +	default n

I think you need some help text there.

> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c

> +struct tegra_pwm_chip {
> +	struct pwm_chip		chip;
> +	struct device		*dev;
> +
> +	struct clk		*clk;
> +
> +	int			clk_enb[4];

s/clk_enb/enable/? This is really more about whether the PWM channel is
enabled than the clock; the fact the clock has to be enabled for the PWM
to run is an implementation detail.

> +static int tegra_pwm_config(struct pwm_chip *chip, unsigned int pwm,
> +		int duty_ns, int period_ns)
...
> +	/* the struct clk may be shared across multiple PWM devices, so
> +	 * only enable the PWM if this device has been enabled
> +	 */
> +	if (pc->clk_enb[num])
> +		val |= PWM_ENABLE;

That comment doesn't describe what the code is doing. Perhaps if a comment
is needed at all:

/* If the PWM channel is enabled, keep it enabled */

> +static int tegra_pwm_probe(struct platform_device *pdev)
...
> +	pwm->chip.label = "tegra-pwm";
> +	pwm->chip.ops = &tegra_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = 4;

I mentioned in an earlier patch it'd be a good idea to store a struct
device * in pcm_chip. That'd also avoid allow the label field to be
removed, since you can just use dev_name(pwm_chip->dev) then.

> +static int __init tegra_pwm_init(void)
> +{
> +	return platform_driver_register(&tegra_pwm_driver);
> +}
> +subsys_initcall(tegra_pwm_init);
> +
> +static void __exit tegra_pwm_exit(void)
> +{
> +	platform_driver_unregister(&tegra_pwm_driver);
> +}
> +module_exit(tegra_pwm_exit);

Can you use module_init() for tegra_pwm_init() instead. That had better
be possible, since the Kconfig allows building this as a module. If so,
you can replace that quoted block with:

module_platform_driver(tegra_pwm_driver);

> +MODULE_LICENSE("GPL v2");

The license header says GPLv2+, so this should just be "GPL" according
to the meanings in include/linux/module.h.

-- 
nvpublic

RE: [RFC 7/7] pwm-backlight: Add rudimentary device-tree support

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> This commit adds very basic support for device-tree probing. Currently,
> only a PWM and maximum and default brightness values can be specified.
> Enabling or disabling backlight power via GPIOs is not yet supported.

> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight

> +Required properties:
> +  - compatible: "pwm-backlight"
> +  - default-brightness: the default brightness setting
> +  - max-brightness: the maximum brightness setting

What are the units of those two properties? Percentage seems like a
reasonable choice, although that's not what the patch implements.

> +  - pwm: OF device-tree PWM specification
> +
> +Example:
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		default-brightness = <224>;
> +		max-brightness = <255>;
> +		pwm = <&pwm 0 5000000>;
> +	};

This may be fine, but I'm not sure that representing the backlight as a
standalone object is correct; I wonder if you want to represent a complete
LCD display complex, including backlight, various GPIOs, and other display
properties, all in the one node? That said, I suppose you could easily
layer this as follows:

reg: regulator {
    // GPIO regulator
};

bl: backlight {
    compatible = "pwm-backlight";
    default-brightness = <224>;
    max-brightness = <255>;
    pwm = <&pwm 0 5000000>;
    power-supply = <&reg>;
};

lcd@x {
    backlight = <&bl>;
    ...
};

so this probably is OK.

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
...
> +static int pwm_backlight_parse_dt(struct device *dev,
> +				  struct platform_pwm_backlight_data *data)
...
> +	/*
> +	 * TODO: Most users of this driver use a number of GPIOs to control
> +	 *       backlight power. Support for specifying these needs to be
> +	 *       added.
> +	 */

At least for the power GPIO, this should probably modeled as a GPIO-based
fixed voltage regulator. Are there other GPIOs that are directly related
to a backlight rather than an LCD complex?

-- 
nvpublic

Re: [RFC 1/7] PWM: add pwm framework support

[Mark Brown]

On Tue, Dec 20, 2011 at 02:13:38PM -0800, Stephen Warren wrote:

> There have been discussions before re: GPIOs and interrupts that creating
> a global namespace for them was a mistake. This new PWM API does the same
> thing. Is there an issue here?

It is an issue with the PWM API but as the PWM API already exists
(without a generic implementation) it's kind of out of scope to fix that
- it'd be a big churn to do along with providing standard code.

Re: [RFC 6/7] pwm: Add Tegra2 SoC support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 958 bytes --]

* Olof Johansson wrote:
> Hi,
> 
> On Tue, Dec 20, 2011 at 2:32 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> 
> If the code is copyright nvidia without a signed-off-by from them,
> then they at least deserve mentioning of original authorship in the
> change log.

Yes, I forgot to add a comment to that effect. Alternatively I could try to
get both Gary King (original author) and Simon Que (Chromium) to sign-off on
the current patch if that is preferable.

> It's also proving itself easier to do the driver and the device tree
> and board-dt update separately since they tend to have different merge
> paths.
> 
> Care to split up as two patches, one for drivers/ and one for arch/arm/?

No problem. I will also address the other comments in the next version.

Thanks,
Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC 2/7] pwm: Allow chips to support multiple PWMs.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1839 bytes --]

* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> > many PWMs it controls using the npwm member. Each of the functions in
> > the pwm_ops structure gets an additional argument that specified the PWM
> > number (it can be converted to a per-chip index by subtracting the
> > chip's base).
> 
> I think it'd be much more convenient for drivers if the PWM core passed
> the device-relative PWM ID to the driver functions instead of the global
> PWM ID; the common case is going to be for the driver to want to index
> into some local array using the value, which means all drivers will have
> to subtract the chip base. I doubt many drivers will care about the global
> ID at all, and if they do, they can add the base back on.

That was my initial plan as well, but then I looked to gpiolib for
inspiration and decided to go the same way. But from what I hear now that
wasn't so good. If there is a concensus that gpiolib has some flaws in this
respect, then I think we should try to avoid them in the PWM framework.

> > The total maximum number of PWM devices is currently fixed to 64, but
> > can easily be made configurable via Kconfig.
> 
> GPIO (and IRQ?) have static sized arrays for this kind of purpose too,
> and I'm pretty sure I've seen discussions that this was a mistake, or
> at least something that will hopefully be reworked. pinctrl was similar,
> but it was requested this be reworked, and now I think the pin ID ->
> pin descriptor table is a radix tree.
> 
> In other words, can you do away with NR_PWM, and make it completely
> dynamic?

IRQ can be configured to use a radix tree if CONFIG_SPARSE_IRQ=y. I guess it
doesn't hurt to always use a radix tree for PWM, so I'll read up on it and
will try to address that in the next version.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC 3/7] of: Add PWM support.

[Thierry Reding]

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

* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> > This patch adds helpers to support device-tree bindings for the generic
> > PWM API.
> 
> > diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c
> 
> > +/**
> > + * of_get_named_pwm() - get a PWM number and period to use with the PWM API
> > + * @np:		device node to get the PWM from
> > + * @propname:	property name containing PWM specifier(s)
> > + * @index:	index of the PWM
> > + * @period_ns:	a pointer to the PWM period (in ns) to fill in
> > + *
> > + * Returns PWM number to use with the Linux generic PWM API or a negative
> > + * error code on failure. If @period_ns is not NULL the function fills in
> > + * the value parsed from the period-ns property.
> > + */
> > +int of_get_named_pwm(struct device_node *np, const char *propname,
> > +		     int index, unsigned int *period_ns)
> > +{
> ...
> > +	if ((cells > 1) && period_ns)
> > +		*period_ns = be32_to_cpu(spec[1]);
> 
> What if the client needs to know period_ns, yet the DT doesn't provide
> it?
> 
> I think a better approach would be to use an "of_xlate" function like
> GPIO and IRQ do. This way, the PWM device's own bindings get to define
> what the extra cells mean, and the definition of of_xlate can be such
> that it must return a period_ns value in all cases; in some cases, the
> driver may return a hard-coded value if the HW doesn't support the
> feature, whereas in most cases the value would be parsed from the
> extra cells.
> 
> Without seeing the complete binding documentation and an example, it's
> difficult to think about whether it's a good idea to include period_ns
> in the PWM specifier or not. An alternative might be a property in the
> PWM node itself.

I like the "of_xlate" alternative better. Adding a property to the PWM node
would hard-code the period regardless of the user. That doesn't really
reflect the PWM API.

I will play around with it a bit and make sure to include PWM binding
documentation in the next version.

> Actually, even better would be for struct pwm_chip to contain a struct
> device * instead. That'd allow the PWM core to use that for e.g. dev_err
> calls, and it includes the of_node for use in the match function above.

I like that idea. I'll address that in the next version.

Thierry

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

Re: [RFC 4/7] arm: tegra: Fix PWM clock programming

[Thierry Reding]

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

* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> > From: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > PWM clock source registers in Tegra 2 have different clock source selection bit
> > fields than other registers.  PWM clock source bits in CLK_SOURCE_PWM_0 register
> > are located at bit field bit[30:28] while others are at bit field bit[31:30] in
> > their respective clock source register.
> > 
> > This patch updates the clock programming to correctly reflect that, by adding a
> > flag to indicate the alternate bit field format and checking for it when
> > selecting a clock source (parent clock).
> > 
> > Also, adjusts for the frequency divider being offset by 1.
> 
> That last line applies to the original patch in the ChromeOS tree, but
> not to the patch you posted (the edit to arch/arm/mach-tegra/pwm.c that
> was in the original patch isn't part of this patch).

Right, I've adjusted the commit message to take that into account. I assume
the commit now also requires my Signed-off-by because I actually modified the
patch? This would be true even in the previous version because I had to make
some small adjustments.

Thierry

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

Re: [RFC 7/7] pwm-backlight: Add rudimentary device-tree support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2573 bytes --]

* Stephen Warren wrote:
> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
[...]
> > +  - default-brightness: the default brightness setting
> > +  - max-brightness: the maximum brightness setting
> 
> What are the units of those two properties? Percentage seems like a
> reasonable choice, although that's not what the patch implements.

That's just the way the pwm-backlight driver works. Typically the maximum
brightness is set to 255. I think you can set these values to pretty much
anything, and the driver will convert the brightness set via sysfs to the
range from 0 to max-brightness.

> > +Example:
> > +
> > +	backlight {
> > +		compatible = "pwm-backlight";
> > +		default-brightness = <224>;
> > +		max-brightness = <255>;
> > +		pwm = <&pwm 0 5000000>;
> > +	};
> 
> This may be fine, but I'm not sure that representing the backlight as a
> standalone object is correct;

I don't think there really is any other way because we need the device node in
order to have the corresponding platform device instantiated.

> I wonder if you want to represent a complete
> LCD display complex, including backlight, various GPIOs, and other display
> properties, all in the one node? That said, I suppose you could easily
> layer this as follows:
> 
> reg: regulator {
>     // GPIO regulator
> };
> 
> bl: backlight {
>     compatible = "pwm-backlight";
>     default-brightness = <224>;
>     max-brightness = <255>;
>     pwm = <&pwm 0 5000000>;
>     power-supply = <&reg>;
> };
> 
> lcd@x {
>     backlight = <&bl>;
>     ...
> };
> 
> so this probably is OK.

This looks pretty reasonable. I actually like it. I don't think there's any
code to resolve the &bl reference from the lcd driver yet, but that should be
rather easy to do.

> > +	/*
> > +	 * TODO: Most users of this driver use a number of GPIOs to control
> > +	 *       backlight power. Support for specifying these needs to be
> > +	 *       added.
> > +	 */
> 
> At least for the power GPIO, this should probably modeled as a GPIO-based
> fixed voltage regulator. Are there other GPIOs that are directly related
> to a backlight rather than an LCD complex?

Currently some platforms seem to use more than a single GPIO for the power.
PXA/Magician has two, depending on the brightness. Viper for example takes a
shortcut and controls both the backlight power and LCD enable from the
pwm-backlight callbacks. I guess if/when those machines are converted, they
can use a complete LCD complex as you described.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC 2/7] pwm: Allow chips to support multiple PWMs.

[Thierry Reding]

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

* Thierry Reding wrote:
> * Stephen Warren wrote:
> > In other words, can you do away with NR_PWM, and make it completely
> > dynamic?
> 
> IRQ can be configured to use a radix tree if CONFIG_SPARSE_IRQ=y. I guess it
> doesn't hurt to always use a radix tree for PWM, so I'll read up on it and
> will try to address that in the next version.

I guess something like idr/ida can be used to dynamically assign a PWM ID,
which would allow us to get rid of the bitmap. Then again, ida itself is not
much more than a bitmap either. It would complicate things a little in that
the ID assignment could no longer be assumed to be sequential for one given
PWM chip, so the lookup (or rather mapping the ID to a chip-relative number)
will be trickier to do.

I'm not sure that it's worth it. Perhaps I should keep the bitmap for ID
allocation and just set the number of bits to something sufficiently large,
say 1024? Then use a radix tree to store the actual descriptors.

pinctrl doesn't solve this because it uses statically allocated pin numbers.
Interestingly though it uses per-device numbering as well, which would be
fine for PWM as well if we had only device tree based probing. In order to
support other devices, we'll still need a global namespace.

Perhaps we can keep the global namespace using the bitmap as is for the time
being and introduce a per-chip API and move all users to that eventually? As
Mark already noted this will cause a lot of churn. Still, there aren't that
many users of the API yet (from Linus' latest tree):

	$ git grep -c pwm_request
	arch/arm/mach-s3c2440/mach-rx1950.c:1
	arch/arm/mach-vt8500/pwm.c:2
	arch/arm/plat-mxc/pwm.c:2
	arch/arm/plat-pxa/pwm.c:2
	arch/arm/plat-samsung/pwm.c:2
	arch/blackfin/kernel/pwm.c:2
	arch/mips/jz4740/pwm.c:1
	arch/unicore32/kernel/pwm.c:2
	drivers/input/misc/pwm-beeper.c:1
	drivers/leds/leds-pwm.c:1
	drivers/mfd/twl6030-pwm.c:2
	drivers/misc/ab8500-pwm.c:2
	drivers/video/backlight/pwm_bl.c:1
	include/linux/pwm.h:2

However, that would require a way to pass the providing PWM chip to the
driver (in addition to the PWM ID). I'm thinking that we should do this step
by step and use a global namespace for now, with a given maximum number of
PWM devices (with the current API there is only a very limited number of
devices anyway) and modify or extend the API subsequently.

Thierry

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

RE: [RFC 4/7] arm: tegra: Fix PWM clock programming

[Stephen Warren]

Thierry Reding wrote at Wednesday, December 21, 2011 2:12 AM:
> * Stephen Warren wrote:
> > Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> > > From: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > >
> > > PWM clock source registers in Tegra 2 have different clock source selection bit
...
> > > Also, adjusts for the frequency divider being offset by 1.
> >
> > That last line applies to the original patch in the ChromeOS tree, but
> > not to the patch you posted (the edit to arch/arm/mach-tegra/pwm.c that
> > was in the original patch isn't part of this patch).
> 
> Right, I've adjusted the commit message to take that into account. I assume
> the commit now also requires my Signed-off-by because I actually modified the
> patch? This would be true even in the previous version because I had to make
> some small adjustments.

All patches you send need you S-o-b line. This is true whether you
modified the patch or not.

When modifying a patch someone else wrote, it's typical to include some
notes on what you changed, e.g.:

Signed-off-by: Original author <...>
[swarren: Fixed checkpatch warnings]
Signed-off-by: You <...>

Or perhaps for larger changes that don't inline between the S-o-b very
well, put it above all the S-o-b.

-- 
nvpbulic

RE: [RFC 2/7] pwm: Allow chips to support multiple PWMs.

[Stephen Warren]

Thierry Reding wrote at Wednesday, December 21, 2011 7:10 AM:
> * Thierry Reding wrote:
> > * Stephen Warren wrote:
> > > In other words, can you do away with NR_PWM, and make it completely
> > > dynamic?
> >
> > IRQ can be configured to use a radix tree if CONFIG_SPARSE_IRQ=y. I guess it
> > doesn't hurt to always use a radix tree for PWM, so I'll read up on it and
> > will try to address that in the next version.
> 
> I guess something like idr/ida can be used to dynamically assign a PWM ID,
> which would allow us to get rid of the bitmap. Then again, ida itself is not
> much more than a bitmap either. It would complicate things a little in that
> the ID assignment could no longer be assumed to be sequential for one given
> PWM chip, so the lookup (or rather mapping the ID to a chip-relative number)
> will be trickier to do.

You can support both dynamic assignment of IDs, and assigning each PWM
chip's IDs in a contiguous block. Just search for n contiguous free IDs
instead of looping n times looking for 1 free ID.

...
> pinctrl doesn't solve this because it uses statically allocated pin numbers.

Well, they're per-device IDs, so the issue doesn't really come up.

> Interestingly though it uses per-device numbering as well, which would be
> fine for PWM as well if we had only device tree based probing. In order to
> support other devices, we'll still need a global namespace.

Yes, that's probably true. I guess a global namespace is reasonable for
now. If you do plan to rework the API though, the sooner the better since
the tree will grow fewer users before it's done:-)

-- 
nvpublic

RE: [RFC 7/7] pwm-backlight: Add rudimentary device-tree support

[Stephen Warren]

Thierry Reding wrote at Wednesday, December 21, 2011 2:33 AM:
> * Stephen Warren wrote:
> > Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> [...]
> > > +  - default-brightness: the default brightness setting
> > > +  - max-brightness: the maximum brightness setting
> >
> > What are the units of those two properties? Percentage seems like a
> > reasonable choice, although that's not what the patch implements.
> 
> That's just the way the pwm-backlight driver works. Typically the maximum
> brightness is set to 255. I think you can set these values to pretty much
> anything, and the driver will convert the brightness set via sysfs to the
> range from 0 to max-brightness.

It sounds like this numbering scheme is some SW abstraction then and not
directly related to HW. It's questionable that this should be represented
in DT, although if it's not I'm not sure where else it could be.

I think from a pure HW perspective, you need to represent:
* PWM period. nS is fine here.
* Minimum PWM duty cycle. nS or percentage is probably fine here. I don't
  know which is more likely to be specified in data sheets.
  (This is missing in the current patch, but supported in pwm_bl.c, so
  I assume there's a need for this, and hence it should be added to DT)
* Maximum PWM duty cycle. nS or percentage is probably fine here. I don't
  know which is more likely to be specified in data sheets.
  (I think this is assumed to be 100% duty cycle by pwm_bl.c, so perhaps
  there isn't a need to represent this in DT?)

Now, the Linux PWM driver then needs to know a range to map those min/max
HW duty cycles to. These are the values where it's unclear to me if/how
DT should represent them. It looks like the SW min value is always 0, and
the SW max value is what max-brightness is in the patch. Is there any
particular reason that max-brightness has to be a particular value, or
even defined in the DT at all? Could we hard-code it to 255 in the DT
parsing code, and not store it in the DT at all; would anything break
if we did that?

If we do need to represent it in DT, given it's a Linux-specific value,
perhaps the property name should have a "linux," or "linux-pwm-bl,"
prefix?

Thinking some more, perhaps the concept of "number of distinct useful
brightness steps" is a reasonable pure HW concept. If we rename max-
brightness to something representing that concept, we can still use the
value as max_brightness in the driver (well, subtract 1) and everyone's
happy? Perhaps "num-brightness-levels"?

If we store a "default brightness" in the DT, perhaps we should represent
it in the same way as the min/max PWM duty cycle values, and convert it
to the 0..max_brighness range when parsing the DT. I'm not sure what we'd
do if the selected value didn't align with a value obtainable using one
of the "num-brightness-steps" though. I guess that implies that the default
should be an integer step ID (as it is in your patch), not a duty cycle?

-- 
nvpublic

Re: [RFC 7/7] pwm-backlight: Add rudimentary device-tree support

[Mitch Bradley]

On 12/21/2011 8:20 AM, Stephen Warren wrote:
> Thierry Reding wrote at Wednesday, December 21, 2011 2:33 AM:
>> * Stephen Warren wrote:
>>> Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
>> [...]
>>>> +  - default-brightness: the default brightness setting
>>>> +  - max-brightness: the maximum brightness setting
>>>
>>> What are the units of those two properties? Percentage seems like a
>>> reasonable choice, although that's not what the patch implements.
>>
>> That's just the way the pwm-backlight driver works. Typically the maximum
>> brightness is set to 255. I think you can set these values to pretty much
>> anything, and the driver will convert the brightness set via sysfs to the
>> range from 0 to max-brightness.
>
> It sounds like this numbering scheme is some SW abstraction then and not
> directly related to HW. It's questionable that this should be represented
> in DT, although if it's not I'm not sure where else it could be.
>
> I think from a pure HW perspective, you need to represent:
> * PWM period. nS is fine here.
> * Minimum PWM duty cycle. nS or percentage is probably fine here. I don't
>    know which is more likely to be specified in data sheets.
>    (This is missing in the current patch, but supported in pwm_bl.c, so
>    I assume there's a need for this, and hence it should be added to DT)
> * Maximum PWM duty cycle. nS or percentage is probably fine here. I don't
>    know which is more likely to be specified in data sheets.
>    (I think this is assumed to be 100% duty cycle by pwm_bl.c, so perhaps
>    there isn't a need to represent this in DT?)
>
> Now, the Linux PWM driver then needs to know a range to map those min/max
> HW duty cycles to. These are the values where it's unclear to me if/how
> DT should represent them. It looks like the SW min value is always 0, and
> the SW max value is what max-brightness is in the patch. Is there any
> particular reason that max-brightness has to be a particular value, or
> even defined in the DT at all? Could we hard-code it to 255 in the DT
> parsing code, and not store it in the DT at all; would anything break
> if we did that?
>
> If we do need to represent it in DT, given it's a Linux-specific value,
> perhaps the property name should have a "linux," or "linux-pwm-bl,"
> prefix?
>
> Thinking some more, perhaps the concept of "number of distinct useful
> brightness steps" is a reasonable pure HW concept.

On the PWM-controlled backlights that I have used, there is no such 
quantization at the hardware level.  The light (LED or fluorescent tube) 
sees an analog signal, typically a low-pass-filtered PWM waveform, and 
responds accordingly.  Human factors may establish a limit on the number 
of useful distinct levels, but there is no way to determine that from a 
data sheet, and thus no definitive hardware-defined value to assign to a 
property.

The objective hardware parameters are often

a) clock frequency choices for the PWM input
b) counter choices for base period
c) counter choices for on phase

There is usually wide latitude for choosing (a) and (b).  Depending on 
the analog filtering and the response characteristics of the light 
source, some choices may result in flickering.  So it would be 
reasonable to report frequency and base period settings that, on the 
hardware in question, result in a reasonable number of flicker-free 
brightness steps.

To further complicate matters, the relationship between PWM duty cycle 
and perceived brightness is usually nonlinear, so equally-spaced duty 
cycle percentages might not result in the best perceived brightness ramp.

One useful way to describe a given hardware system would be to have 
properties reporting values that work well for that hardware.  You would 
need to report a good base clock frequency, a good base period, and an 
array of N values that give a good perceived brightness ramp.

The backlight control on my PC laptop has 16 levels, 0 to 15.  That 
seems adequate to me.

> If we rename max-
> brightness to something representing that concept, we can still use the
> value as max_brightness in the driver (well, subtract 1) and everyone's
> happy? Perhaps "num-brightness-levels"?
>
> If we store a "default brightness" in the DT, perhaps we should represent
> it in the same way as the min/max PWM duty cycle values, and convert it
> to the 0..max_brighness range when parsing the DT. I'm not sure what we'd
> do if the selected value didn't align with a value obtainable using one
> of the "num-brightness-steps" though. I guess that implies that the default
> should be an integer step ID (as it is in your patch), not a duty cycle?
>

Re: [RFC 2/7] pwm: Allow chips to support multiple PWMs.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2204 bytes --]

* Stephen Warren wrote:
> Thierry Reding wrote at Wednesday, December 21, 2011 7:10 AM:
> > * Thierry Reding wrote:
> > > * Stephen Warren wrote:
> > > > In other words, can you do away with NR_PWM, and make it completely
> > > > dynamic?
> > >
> > > IRQ can be configured to use a radix tree if CONFIG_SPARSE_IRQ=y. I guess it
> > > doesn't hurt to always use a radix tree for PWM, so I'll read up on it and
> > > will try to address that in the next version.
> > 
> > I guess something like idr/ida can be used to dynamically assign a PWM ID,
> > which would allow us to get rid of the bitmap. Then again, ida itself is not
> > much more than a bitmap either. It would complicate things a little in that
> > the ID assignment could no longer be assumed to be sequential for one given
> > PWM chip, so the lookup (or rather mapping the ID to a chip-relative number)
> > will be trickier to do.
> 
> You can support both dynamic assignment of IDs, and assigning each PWM
> chip's IDs in a contiguous block. Just search for n contiguous free IDs
> instead of looping n times looking for 1 free ID.

Yes, but that doesn't buy us anything over using a bitmap like we're doing
now, since ida is really only a bitmap with a different API on top (it is
also limited to 1024 entries). For our purposes, using a bitmap should work
just as well.

> > Interestingly though it uses per-device numbering as well, which would be
> > fine for PWM as well if we had only device tree based probing. In order to
> > support other devices, we'll still need a global namespace.
> 
> Yes, that's probably true. I guess a global namespace is reasonable for
> now. If you do plan to rework the API though, the sooner the better since
> the tree will grow fewer users before it's done:-)

Right =) If you think it better to do it all in one go, perhaps we should
rather get the API rework done now. I was hoping it could be avoided, but
perhaps it would be just as well to bite the bullet now. Perhaps Sascha or
Arnd (Cc'ed) can comment on this. One of the reasons why Sascha's solution
was preferred over Bill Gatliff's proposal was that it doesn't change the
existing API.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC 7/7] pwm-backlight: Add rudimentary device-tree support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1703 bytes --]

* Mitch Bradley wrote:
> To further complicate matters, the relationship between PWM duty
> cycle and perceived brightness is usually nonlinear, so
> equally-spaced duty cycle percentages might not result in the best
> perceived brightness ramp.

Yes, I've seen that on numerous devices as well.

> One useful way to describe a given hardware system would be to have
> properties reporting values that work well for that hardware.  You
> would need to report a good base clock frequency, a good base
> period, and an array of N values that give a good perceived
> brightness ramp.

I'm not sure I quite understand what you mean by base period. Would the
base period not be simply the inverse of the frequency? Or should the base
period be the "step size" to multiply each element of the brightness levels
with?

> The backlight control on my PC laptop has 16 levels, 0 to 15.  That
> seems adequate to me.

I have a laptop that uses 8 level, 0 to 7. Combining that with the above it
should be easy to represent in the DT:

	bl: backlight {
		pwms = <&pwm 0 5000000>;
		base-period = <...>;
		brightness-levels = <...>;
	};

However, that would entail some major modifications to the pwm-backlight
driver to make it compute the actual duty cycle based on the array of
brightness levels.

This also raises a more general question: in a lot of drivers the DT binding
is used to provide an alternative source for platform data. With that
assumption, is it still reasonable to describe data in DT is such a different
way from the platform data? I mean in this particular case, there will be no
way to convert the DT data to the corresponding platform data because there
simply is no correspondence.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss