Skip to contentSkip to conversation

Add PWM framework and device-tree support.

PATCH Superseded Thierry Reding 6 participants 22 messages Feb 6, 2012 – Feb 8, 2012

Subsystems (10)

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v2 cover Add PWM framework and device-tree support.

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 taken 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). When this series is ready I think it would be best to merge patches 1 and 2.

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. Device tree binding documentation is also provided.

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 device tree based probing for the Tegra2 PWFM controller.

Patches 8 and 9 are ports of the Blackfin PWM and the PXA PWM drivers to the PWM framework. These are only compile-tested as I do not have any hardware to test them on. They are meant as test-bed for the framework. I intend to port other drivers over to the framework for the next spin.

Patch 10 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.

The whole series is based on the linux-next tree from 20120202. I think I've addressed all of the concerns raised in the first version, and those I haven't yet were still open for discussion (or I've explicitly marked them as TODO).

Thierry

[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 (8):
pwm: Allow chips to support multiple PWMs.
of: Add PWM support.
arm/tegra: Provide clock for only one PWM controller
pwm: Add NVIDIA Tegra SoC support arm/tegra: Add PWFM controller device tree probing
pwm: Add Blackfin support
pwm: Add PXA support
pwm-backlight: Add rudimentary device tree support

 Documentation/devicetree/bindings/pwm/pwm.txt      |   50 +++
 .../bindings/video/backlight/pwm-backlight         |   16 +
 Documentation/pwm.txt                              |   56 +++
 MAINTAINERS                                        |    6 +
 arch/arm/boot/dts/tegra20.dtsi                     |    6 +
 arch/arm/boot/dts/tegra30.dtsi                     |    6 +
 arch/arm/mach-tegra/board-dt-tegra20.c             |    1 +
 arch/arm/mach-tegra/board-dt-tegra30.c             |    1 +
 arch/arm/mach-tegra/clock.h                        |    1 +
 arch/arm/mach-tegra/tegra2_clocks.c                |   33 ++-
 arch/arm/plat-pxa/Makefile                         |    1 -
 arch/arm/plat-pxa/pwm.c                            |  304 ----------------
 arch/blackfin/Kconfig                              |   10 -
 arch/blackfin/kernel/Makefile                      |    1 -
 arch/blackfin/kernel/pwm.c                         |  100 ------
 drivers/Kconfig                                    |    2 +
 drivers/Makefile                                   |    1 +
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/pwm.c                                   |  130 +++++++
 drivers/pwm/Kconfig                                |   40 +++
 drivers/pwm/Makefile                               |    4 +
 drivers/pwm/core.c                                 |  367 ++++++++++++++++++++
 drivers/pwm/pwm-bfin.c                             |  164 +++++++++
 drivers/pwm/pwm-pxa.c                              |  243 +++++++++++++
 drivers/pwm/pwm-tegra.c                            |  266 ++++++++++++++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |   81 ++++-
 include/linux/of_pwm.h                             |   51 +++
 include/linux/pwm.h                                |   67 ++++
 30 files changed, 1584 insertions(+), 433 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm.txt
 create mode 100644 Documentation/devicetree/bindings/video/backlight/pwm-backlight
 create mode 100644 Documentation/pwm.txt
 delete mode 100644 arch/arm/plat-pxa/pwm.c
 delete mode 100644 arch/blackfin/kernel/pwm.c
 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-bfin.c
 create mode 100644 drivers/pwm/pwm-pxa.c
 create mode 100644 drivers/pwm/pwm-tegra.c
 create mode 100644 include/linux/of_pwm.h
Signature · 1 line
1.7.9
#1 lore raw

PATCH v2 1/10 PWM: add pwm framework support

From: Sascha Hauer <[email protected]>

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.

---
 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
Documentation/pwm.txt new file +56 -0
new file mode 100644
index 0000000..c7c5cb1
@@ -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.
MAINTAINERS +6 -0
index eb8a518..a422dd5 100644
@@ -5356,6 +5356,12 @@ S:	Maintained
 F:	Documentation/video4linux/README.pvrusb2
 F:	drivers/media/video/pvrusb2/
 
+PPWM core
+M:	Sascha Hauer <[email protected]>
+L:	linux-kernel at vger.kernel.org
+S:	Maintained
+F:	drivers/pwm/
+
 PXA2xx/PXA3xx SUPPORT
 M:	Eric Miao <[email protected]>
 M:	Russell King <[email protected]>
drivers/Kconfig +2 -0
index 03ac866..a88882a 100644
@@ -144,4 +144,6 @@ source "drivers/devfreq/Kconfig"
 
 source "drivers/modem_shm/Kconfig"
 
+source "drivers/pwm/Kconfig"
+
 endmenu
drivers/Makefile +1 -0
index 2571dff..447ef50 100644
@@ -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/
drivers/pwm/Kconfig new file +12 -0
new file mode 100644
index 0000000..93c1052
@@ -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
drivers/pwm/Makefile new file +1 -0
new file mode 100644
index 0000000..3469c3d
@@ -0,0 +1 @@
+obj-$(CONFIG_PWM)		+= core.o
drivers/pwm/core.c new file +220 -0

220 lines not on this page · raw message

include/linux/pwm.h +37 -0
index 7c77575..df9681b 100644
@@ -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 */
Signature · 1 line
1.7.9
#2 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

In reply to 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.

---
Changes in v2:
- add 'struct device *dev' field to pwm_chip, drop label field
- use radix tree for PWM descriptors
- add pwm_set_chip_data() and pwm_get_chip_data() functions to allow storing and retrieving chip-specific per-PWM data

TODO:
- pass chip-indexed PWM number (PWM descriptor?) to drivers
- merge with Sascha's patch

 drivers/pwm/core.c  |  287 ++++++++++++++++++++++++++++++++++++++-------------
 include/linux/pwm.h |   37 +++++--
 2 files changed, 242 insertions(+), 82 deletions(-)
drivers/pwm/core.c +217 -70
index 71de479..a223bd6 100644
@@ -17,154 +17,292 @@
  *  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/of_pwm.h>
 #include <linux/pwm.h>
+#include <linux/radix-tree.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/device.h>
 
+#define MAX_PWMS 1024
+
 struct pwm_device {
-	struct			pwm_chip *chip;
-	const char		*label;
+	const char *label;
+	unsigned int pwm;
+};
+
+struct pwm_desc {
+	struct pwm_chip		*chip;
+	void			*chip_data;
 	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, MAX_PWMS);
+static RADIX_TREE(pwm_desc_tree, GFP_KERNEL);
+
+static struct pwm_desc *pwm_to_desc(unsigned int pwm)
+{
+	return radix_tree_lookup(&pwm_desc_tree, pwm);
+}
+
+static struct pwm_desc *alloc_desc(unsigned int pwm)
+{
+	struct pwm_desc *desc;
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return NULL;
 
-static struct pwm_device *_find_pwm(int pwm_id)
+	return desc;
+}
+
+static void free_desc(unsigned int pwm)
+{
+	struct pwm_desc *desc = pwm_to_desc(pwm);
+
+	radix_tree_delete(&pwm_desc_tree, pwm);
+	kfree(desc);
+}
+
+static int alloc_descs(int pwm, unsigned int from, unsigned int count)
 {
-	struct pwm_device *pwm;
+	unsigned int start;
+	unsigned int i;
+	int ret = 0;
+
+	if (pwm >= 0) {
+		if (from > pwm)
+			return -EINVAL;
+
+		from = pwm;
+	}
+
+	start = bitmap_find_next_zero_area(allocated_pwms, MAX_PWMS, from,
+			count, 0);
 
-	list_for_each_entry(pwm, &pwm_list, node) {
-		if (pwm->chip->pwm_id == pwm_id)
-			return pwm;
+	if ((pwm >= 0) && (start != pwm))
+		return -EEXIST;
+
+	if (start + count > MAX_PWMS)
+		return -ENOSPC;
+
+	for (i = start; i < start + count; i++) {
+		struct pwm_desc *desc = alloc_desc(i);
+		if (!desc) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		radix_tree_insert(&pwm_desc_tree, i, desc);
 	}
 
-	return NULL;
+	bitmap_set(allocated_pwms, start, count);
+
+	return start;
+
+err:
+	for (i--; i >= 0; i--)
+		free_desc(i);
+
+	return ret;
+}
+
+static void free_descs(unsigned int from, unsigned int count)
+{
+	unsigned int i;
+
+	for (i = from; i < from + count; i++)
+		free_desc(i);
+
+	bitmap_clear(allocated_pwms, from, count);
 }
 
 /**
- * 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,
+ * pwm_set_chip_data - set private chip data for a PWM
+ * @pwm: PWM number
+ * @data: pointer to chip-specific data
  */
-int pwmchip_add(struct pwm_chip *chip)
+int pwm_set_chip_data(unsigned int pwm, void *data)
 {
-	struct pwm_device *pwm;
-	int ret = 0;
+	struct pwm_desc *desc = pwm_to_desc(pwm);
 
-	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
-	if (!pwm)
-		return -ENOMEM;
+	if (!desc)
+		return -EINVAL;
 
-	pwm->chip = chip;
+	desc->chip_data = data;
+	return 0;
+}
+
+/**
+ * pwm_get_chip_data - get private chip data for a PWM
+ * @pwm: PWM number
+ */
+void *pwm_get_chip_data(unsigned int pwm)
+{
+	struct pwm_desc *desc = pwm_to_desc(pwm);
+
+	return desc ? desc->chip_data : NULL;
+}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If pwm->base < 0 then a dynamically assigned base
+ * will be used.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	unsigned int i;
+	int ret;
 
 	mutex_lock(&pwm_lock);
 
-	if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
-		ret = -EBUSY;
+	ret = alloc_descs(chip->base, 0, chip->npwm);
+	if (ret < 0)
 		goto out;
+
+	chip->base = ret;
+	ret = 0;
+
+	/* initialize descriptors */
+	for (i = chip->base; i < chip->base + chip->npwm; i++) {
+		struct pwm_desc *desc = pwm_to_desc(i);
+
+		if (!desc) {
+			pr_debug("pwm: descriptor %u not initialized\n", i);
+			continue;
+		}
+
+		desc->chip = chip;
 	}
 
-	list_add_tail(&pwm->node, &pwm_list);
+	of_pwmchip_add(chip);
+
 out:
 	mutex_unlock(&pwm_lock);
-
-	if (ret)
-		kfree(pwm);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
 /**
- * pwmchip_remove() - remove a pwm
- * @chip: the pwm
+ * pwmchip_remove() - remove a PWM chip
+ * @chip: the PWM chip to remove
  *
- * remove a pwm. This function may return busy if the pwm is still requested.
+ * Removes a PWM chip. This function may return busy if the PWM chip provides
+ * a PWM device that is still requested.
  */
 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_descs(chip->base, chip->npwm);
+	of_pwmchip_remove(chip);
 
-	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 long i;
+
+	mutex_lock(&pwm_lock);
+
+	for_each_set_bit(i, allocated_pwms, MAX_PWMS) {
+		struct pwm_desc *desc = pwm_to_desc(i);
+
+		if (!desc || !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 >= MAX_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 +311,19 @@ 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;
+	pwm->pwm = 0;
 
-	module_put(pwm->chip->ops->owner);
+	module_put(desc->chip->ops->owner);
 out:
 	mutex_unlock(&pwm_lock);
 }
@@ -193,7 +334,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 +345,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 +359,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);
include/linux/pwm.h +25 -12
index df9681b..0f8d105 100644
@@ -32,37 +32,50 @@ void pwm_disable(struct pwm_device *pwm);
 struct pwm_chip;
 
 /**
- * struct pwm_ops - PWM operations
+ * struct pwm_ops - PWM controller 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
+ * @owner: helps prevent removal of modules exporting active PWMs
  */
 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;
 };
 
 /**
- * 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 - abstract a PWM controller
+ * @dev: device providing the PWMs
+ * @ops: callbacks for this PWM controller
+ * @base: number of first PWM controlled by this chip
+ * @npwm: number of PWMs controlled by this chip
  */
 struct pwm_chip {
-	int			pwm_id;
-	const char		*label;
+	struct device		*dev;
 	struct pwm_ops		*ops;
+	int			base;
+	unsigned int		npwm;
 };
 
+int pwm_set_chip_data(unsigned int pwm, void *data);
+void *pwm_get_chip_data(unsigned int pwm);
+
 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 */
Signature · 1 line
1.7.9
#3 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 2 lines · Thierry Reding

This commit modifies the PWM core to support multiple PWMs per struct pwm_chip.

I think you should mention what motivates this change.

Quoted 3 lines

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

If we've learned one thing from gpiolib, I think it is that using a global index to identify a resource was a bad idea.

Quoted 8 lines

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 code says 1024.

Quoted 3 lines

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

#4 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 5 lines · Lars-Peter Clausen
Quoted 2 lines · Thierry Reding

This commit modifies the PWM core to support multiple PWMs per struct pwm_chip.

I think you should mention what motivates this change.

Okay, I can add that.

Quoted 6 lines
Quoted 3 lines

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

If we've learned one thing from gpiolib, I think it is that using a global index to identify a resource was a bad idea.

Yes, that concern has been raised several times already. The reason for it being this way is that the current API requires it. So either we keep it as is for now and change it over to indexing on a per-chip basis later (which has the advantage of not breaking anything currently in-tree) or we make that change now, in which case this patch series will probably double in size. I'm also not sure if I can reasonably keep the series bisectible that way.

One of the major problems when converting to a non-global namespace is how to represent the relationship in code. For device tree this should be easy to do because it has all the infrastructure in place. For non-device-tree devices I have no idea yet how this could be done. Perhaps by using something like the clock API and using names for lookup?

Quoted 10 lines
Quoted 8 lines

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 code says 1024.

Right, I missed that. Thanks.

Quoted 2 lines
Quoted 2 lines

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

I just noticed that this isn't completely valid anymore either since I've ported two of the drivers. However, The remaining drivers won't work properly when built against a kernel with PWM framework support because of the duplicate symbols.

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

#5 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 7 lines · Thierry Reding
Quoted 2 lines · Lars-Peter Clausen

If we've learned one thing from gpiolib, I think it is that using a global index to identify a resource was a bad idea.

Yes, that concern has been raised several times already. The reason for it being this way is that the current API requires it. So either we keep it as

Might be worth calling this out in the changelog to preempt further
discussion - it's a totally sensible approach to take but it's not
going to be immediately obvious to reviewers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120207/75c22e28/attachment.sig>

#6 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

In reply to Thierry Reding
Quoted 5 lines · Thierry Reding

One of the major problems when converting to a non-global namespace is how to represent the relationship in code. For device tree this should be easy to do because it has all the infrastructure in place. For non-device-tree devices I have no idea yet how this could be done. Perhaps by using something like the clock API and using names for lookup?

Well, the first step is to make sure that the pwm 'chip' drivers don't know anything about the global numberspace.

That limits the extent of the global numberspace to the pwm core and pwm users. You then don't have to worry about the pwm chip drivers while you work out how to solve the other half of the problem.

#7 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 7 lines · Russell King - ARM Linux
Quoted 5 lines · Thierry Reding

One of the major problems when converting to a non-global namespace is how to represent the relationship in code. For device tree this should be easy to do because it has all the infrastructure in place. For non-device-tree devices I have no idea yet how this could be done. Perhaps by using something like the clock API and using names for lookup?

Quoted 6 lines

Well, the first step is to make sure that the pwm 'chip' drivers don't know anything about the global numberspace.

That limits the extent of the global numberspace to the pwm core and pwm users. You then don't have to worry about the pwm chip drivers while you work out how to solve the other half of the problem.

I see. That would mean that pwm_request() gets a global PWM index, looks up the corresponding PWM chip and device then pass the chip-relative index to the driver's ops.

I think that can be simplified even further. For instance if the pwm_device structure contains a pointer to its parent struct pwm_chip, then that can be used directly to call into the driver's ops. And the ops could be modified to take a pwm_device instead of the global PWM index. That would be similar to how struct irq_data is used.

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

#8 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

In reply to Thierry Reding

On 07/02/12 02:19, Thierry Reding wrote:

Hi Thierry,

A few comments below,

~Ryan

Quoted 11 lines

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.

It would be better to make the code handle arbitrary numbers of PWMs. A Kconfig knob becomes annoying when you have more than one platform configured into the kernel.

Quoted 2 lines

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

Does this patch actually break the drivers in terms of not building or running? If so, can this patch series be reworked a bit to allow the old PWM framework to be used until all of the drivers are converted?

Quoted 44 lines

---
Changes in v2:
- add 'struct device *dev' field to pwm_chip, drop label field
- use radix tree for PWM descriptors
- add pwm_set_chip_data() and pwm_get_chip_data() functions to allow storing and retrieving chip-specific per-PWM data

TODO:
- pass chip-indexed PWM number (PWM descriptor?) to drivers
- merge with Sascha's patch

 drivers/pwm/core.c  |  287 ++++++++++++++++++++++++++++++++++++++-------------
 include/linux/pwm.h |   37 +++++--
 2 files changed, 242 insertions(+), 82 deletions(-)
drivers/pwm/core.c +8 -2
index 71de479..a223bd6 100644
@@ -17,154 +17,292 @@
  *  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/of_pwm.h>
 #include <linux/pwm.h>
+#include <linux/radix-tree.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/device.h>
 
+#define MAX_PWMS 1024
+
 struct pwm_device {
-	struct			pwm_chip *chip;
-	const char		*label;
+	const char *label;
+	unsigned int pwm;
+};

drivers/pwm/core.c line 45

Quoted 10 lines
drivers/pwm/core.c +5 -1
+
+struct pwm_desc {
+	struct pwm_chip		*chip;
+	void			*chip_data;
 	unsigned long		flags;
 #define FLAG_REQUESTED	0
 #define FLAG_ENABLED	1
-	struct list_head	node;
+	struct pwm_device	pwm;
 };

Do pwm_desc and pwm_device really need to be separate structs? pwm_device only has two fields, which could easily be added to pwm_desc (and rename it to pmw_device if you like). They are both opaque structures, so it makes no difference to the users of the framework.

drivers/pwm/core.c line 49

Quoted 5 lines
drivers/pwm/core.c +2 -2
-static LIST_HEAD(pwm_list);
-
 static DEFINE_MUTEX(pwm_lock);
+static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
+static RADIX_TREE(pwm_desc_tree, GFP_KERNEL);

I missed any discussion of this from v1, but why was this approach chosen? The bitmap arbitrarily limits the maximum number of PWMs and is also a little bit (nitpicky) wasteful when a platform doesn't need anywhere near 1024 PWMs.

In most cases I would expect that platforms only have a handful of PWMs (say less than 32), in which case a list lookup isn't too bad.

As someone else mentioned, it might be best to drop the global numbering scheme and have each pwm_chip have its own numbering for its PWMs. So requesting a PWM would first require getting a handle to the PWM chip it is on. If a chip has a fixed number of PWMs (e.g. set a registration time) then the PWMs within a chip can just be an array with O(1) lookup.

Quoted 18 lines
drivers/pwm/core.c +16 -1
+static struct pwm_desc *pwm_to_desc(unsigned int pwm)
+{
+	return radix_tree_lookup(&pwm_desc_tree, pwm);
+}
+
+static struct pwm_desc *alloc_desc(unsigned int pwm)
+{
+	struct pwm_desc *desc;
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return NULL;
 
-static struct pwm_device *_find_pwm(int pwm_id)
+	return desc;
+}
+
+static void free_desc(unsigned int pwm)

drivers/pwm/core.c line 134

Quoted 10 lines
drivers/pwm/core.c +8 -0
+{
+	struct pwm_desc *desc = pwm_to_desc(pwm);
+
+	radix_tree_delete(&pwm_desc_tree, pwm);
+	kfree(desc);
+}
+
+static int alloc_descs(int pwm, unsigned int from, unsigned int count)
 
 {

You don't really need the from argument. There is only one caller for alloc_descs and it passes 0 for from. So you could re-write this as:

	static int alloc_descs(int pwm, unsigned int count)
	{
		unsigned int from = 0;
		...

		if (pwm > MAX_PWMS)
			return -EINVAL;
		if (pwm >= 0)
			from = pwm;

drivers/pwm/core.c line 86

Quoted 11 lines
drivers/pwm/core.c +10 -1
-	struct pwm_device *pwm;
+	unsigned int start;
+	unsigned int i;
+	int ret = 0;
+
+	if (pwm >= 0) {
+		if (from > pwm)
+			return -EINVAL;
+
+		from = pwm;
+	}

This doesn't catch some caller errors:

	if (pwm > MAX_PWMS || from > MAX_PWMS)
		return -EINVAL;

drivers/pwm/core.c line 89

Quoted 3 lines
drivers/pwm/core.c +3 -0
+
+	start = bitmap_find_next_zero_area(allocated_pwms, MAX_PWMS, from,
+			count, 0);

Do the PWM indexes need to be contiguous for a given PWM chip? You could probably grab each bit individually. That said, I think a better solution is to have per-chip numbering.

drivers/pwm/core.c line 91

Quoted 4 lines
drivers/pwm/core.c +1 -3
-	list_for_each_entry(pwm, &pwm_list, node) {
-		if (pwm->chip->pwm_id == pwm_id)
-			return pwm;
+	if ((pwm >= 0) && (start != pwm))

Don't need the inner parens.

drivers/pwm/core.c line 95

Quoted 4 lines
drivers/pwm/core.c +4 -0
+		return -EEXIST;
+
+	if (start + count > MAX_PWMS)
+		return -ENOSPC;

Is this possible? Can bitmap_find_next_zero_area return a start position where the count would exceed the maximum?

Quoted 8 lines
drivers/pwm/core.c +8 -0
+
+	for (i = start; i < start + count; i++) {
+		struct pwm_desc *desc = alloc_desc(i);
+		if (!desc) {
+			ret = -ENOMEM;
+			goto err;
+		}
+

drivers/pwm/core.c line 112

Quoted 10 lines
drivers/pwm/core.c +7 -1
+		radix_tree_insert(&pwm_desc_tree, i, desc);
 	}
 
-	return NULL;
+	bitmap_set(allocated_pwms, start, count);
+
+	return start;
+
+err:
+	for (i--; i >= 0; i--)

Nitpick:

	while (--i >= 0)

is a bit simpler.

Quoted 17 lines
drivers/pwm/core.c +13 -1
+		free_desc(i);
+
+	return ret;
+}
+
+static void free_descs(unsigned int from, unsigned int count)
+{
+	unsigned int i;
+
+	for (i = from; i < from + count; i++)
+		free_desc(i);
+
+	bitmap_clear(allocated_pwms, from, count);
 }
 
 /**
- * pwmchip_add() - register a new pwm

drivers/pwm/core.c line 133

Quoted 10 lines
drivers/pwm/core.c +4 -5
- * @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,
+ * pwm_set_chip_data - set private chip data for a PWM
+ * @pwm: PWM number
+ * @data: pointer to chip-specific data
  */
-int pwmchip_add(struct pwm_chip *chip)
+int pwm_set_chip_data(unsigned int pwm, void *data)

This interface is a bit ugly. Shouldn't the user need to have a handle to the pwm_device in order to access the chip_data? Why should callers be able to set/grab chip data from random PWMs?

Quoted 47 lines
drivers/pwm/core.c +31 -8
 {
-	struct pwm_device *pwm;
-	int ret = 0;
+	struct pwm_desc *desc = pwm_to_desc(pwm);
 
-	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
-	if (!pwm)
-		return -ENOMEM;
+	if (!desc)
+		return -EINVAL;
 
-	pwm->chip = chip;
+	desc->chip_data = data;
+	return 0;
+}
+
+/**
+ * pwm_get_chip_data - get private chip data for a PWM
+ * @pwm: PWM number
+ */
+void *pwm_get_chip_data(unsigned int pwm)
+{
 
+	struct pwm_desc *desc = pwm_to_desc(pwm);
+
+	return desc ? desc->chip_data : NULL;
+}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If pwm->base < 0 then a dynamically assigned base
+ * will be used.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	unsigned int i;
+	int ret;
 
 	mutex_lock(&pwm_lock);
 
-	if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
-		ret = -EBUSY;
+	ret = alloc_descs(chip->base, 0, chip->npwm);
+	if (ret < 0)
 		goto out;

drivers/pwm/core.c

Quoted 10 lines
drivers/pwm/core.c +10 -0
+
+	chip->base = ret;
+	ret = 0;
+
+	/* initialize descriptors */
+	for (i = chip->base; i < chip->base + chip->npwm; i++) {
+		struct pwm_desc *desc = pwm_to_desc(i);
+
+		if (!desc) {
+			pr_debug("pwm: descriptor %u not initialized\n", i);

Should this be a WARN_ON?

Quoted 86 lines
drivers/pwm/core.c +39 -20
+			continue;
+		}
+
+		desc->chip = chip;
 	}
 
-	list_add_tail(&pwm->node, &pwm_list);
+	of_pwmchip_add(chip);
+
 out:
 	mutex_unlock(&pwm_lock);
-
-	if (ret)
-		kfree(pwm);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
 /**
- * pwmchip_remove() - remove a pwm
- * @chip: the pwm
+ * pwmchip_remove() - remove a PWM chip
+ * @chip: the PWM chip to remove
  *
- * remove a pwm. This function may return busy if the pwm is still requested.
+ * Removes a PWM chip. This function may return busy if the PWM chip provides
+ * a PWM device that is still requested.
  */
 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_descs(chip->base, chip->npwm);
+	of_pwmchip_remove(chip);
 
-	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 long i;
+
+	mutex_lock(&pwm_lock);
+
+	for_each_set_bit(i, allocated_pwms, MAX_PWMS) {
+		struct pwm_desc *desc = pwm_to_desc(i);
+
+		if (!desc || !desc->chip)
+			continue;
+
+		if (match(desc->chip, data)) {

drivers/pwm/core.c line 257

Quoted 10 lines
drivers/pwm/core.c +10 -0
+			chip = desc->chip;
+			break;
+		}
+	}
+
+	mutex_unlock(&pwm_lock);
+
+	return chip;
+}
+EXPORT_SYMBOL_GPL(pwmchip_find);

So, propreitary modules are not allowed to use PWMs?

drivers/pwm/core.c line 268

Quoted 13 lines
drivers/pwm/core.c +5 -2
+
 /*
  * 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 >= MAX_PWMS))

Don't need the inner parens.

drivers/pwm/core.c line 269

Quoted 1 line
drivers/pwm/core.c +1 -0
+		return ERR_PTR(-ENOENT);

-EINVAL maybe?

drivers/pwm/core.c line 274

Quoted 10 lines
drivers/pwm/core.c +3 -5
+
 	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;

This looks wrong. If the pwm_desc being requested doesn't exist, won't this oops?

Quoted 46 lines
drivers/pwm/core.c +12 -11
-	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);
 

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_request); · line 316

Quoted 6 lines
drivers/pwm/core.c +2 -0
@@ -173,16 +311,19 @@ 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);

pwm_lock is used to protect global addition and removal from the radix/tree bitmap. Here you are also using it to protect the fields of a specific pwm device? Maybe it would be better to have a per pwm device lock for this?

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_request); · line 324

Quoted 8 lines
drivers/pwm/core.c +2 -1
-	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;
+	pwm->pwm = 0;

Why do this? Isn't index 0 a valid PWM index?a

Quoted 109 lines
drivers/pwm/core.c +12 -6
-	module_put(pwm->chip->ops->owner);
+	module_put(desc->chip->ops->owner);
 out:
 	mutex_unlock(&pwm_lock);
 }
@@ -193,7 +334,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 +345,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 +359,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);
include/linux/pwm.h +25 -12
index df9681b..0f8d105 100644
@@ -32,37 +32,50 @@ void pwm_disable(struct pwm_device *pwm);
 struct pwm_chip;
 
 /**
- * struct pwm_ops - PWM operations
+ * struct pwm_ops - PWM controller 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
+ * @owner: helps prevent removal of modules exporting active PWMs
  */
 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;
 };
 
 /**
- * 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 - abstract a PWM controller
+ * @dev: device providing the PWMs
+ * @ops: callbacks for this PWM controller
+ * @base: number of first PWM controlled by this chip
+ * @npwm: number of PWMs controlled by this chip
  */
 struct pwm_chip {
-	int			pwm_id;
-	const char		*label;
+	struct device		*dev;
 	struct pwm_ops		*ops;
+	int			base;
+	unsigned int		npwm;
 };
 
+int pwm_set_chip_data(unsigned int pwm, void *data);
+void *pwm_get_chip_data(unsigned int pwm);
+
 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 */
#9 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 7 lines · Ryan Mallon
Quoted 2 lines · Thierry Reding

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

It would be better to make the code handle arbitrary numbers of PWMs. A Kconfig knob becomes annoying when you have more than one platform configured into the kernel.

AFAICT handling an arbitrary number of PWMs will only be possible once we get rid of the global namespace and therefore should be postponed for later. I may be wrong, though, so if anybody can point me in the right direction I'm perfectly happy to change that in this series.

Quoted 6 lines
Quoted 2 lines

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

Does this patch actually break the drivers in terms of not building or running? If so, can this patch series be reworked a bit to allow the old PWM framework to be used until all of the drivers are converted?

That sentence is misleading. Since the new framework implements the exact same API as the drivers, any unconverted drivers will still work unless they are built within the same kernel as the framework.

The intent is to get the framework into an acceptable, backwards-compatible shape and port existing drivers (i.e. PWM API providers) to the new framework to ensure everything keeps working as usual.

After that, any issues with the current implementation can be addressed (per-chip numbering, ...).

Quoted 14 lines
Quoted 14 lines

struct pwm_device {

drivers/pwm/core.c +7 -3
-	struct			pwm_chip *chip;
-	const char		*label;
+	const char *label;
+	unsigned int pwm;
+};
+
+struct pwm_desc {
+	struct pwm_chip		*chip;
+	void			*chip_data;
 	unsigned long		flags;
 #define FLAG_REQUESTED	0
 #define FLAG_ENABLED	1
-	struct list_head	node;
Quoted 8 lines

drivers/pwm/core.c line 45

Quoted 2 lines
drivers/pwm/core.c +1 -0
+	struct pwm_device	pwm;
 };

Do pwm_desc and pwm_device really need to be separate structs? pwm_device only has two fields, which could easily be added to pwm_desc (and rename it to pmw_device if you like). They are both opaque structures, so it makes no difference to the users of the framework.

This was meant to be a middle-ground between the PWM API and a central framework. The current PWM API returns a pointer to struct pwm_device when requesting a PWM but the plan was to keep this in line with gpiolib which does a lot of the same managing.

Perhaps I should elaborate some more. One thing I find particularily nice about gpiolib is how each GPIO can be addressed with just the GPIO number which is initially requested and can only be used by the requester. So the plan was to keep a set of pwm_desc structures which could be looked up via the PWM number instead of using a pointer to struct pwm_device.

In retrospect I must say that this may not have been the best idea. If indeed we want to end up with a per-chip numbering of PWM devices, a global number won't do any good. Keeping the pwm_device structure and wiring it up with the PWM chip would be a better fit.

Does that sound reasonable?

Quoted 10 lines

drivers/pwm/core.c line 49

Quoted 5 lines
drivers/pwm/core.c +2 -2
-static LIST_HEAD(pwm_list);
-
 static DEFINE_MUTEX(pwm_lock);
+static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
+static RADIX_TREE(pwm_desc_tree, GFP_KERNEL);

I missed any discussion of this from v1, but why was this approach chosen? The bitmap arbitrarily limits the maximum number of PWMs and is also a little bit (nitpicky) wasteful when a platform doesn't need anywhere near 1024 PWMs.

The initial implementation used a static array of MAX_PWMS pwm_desc structures (with MAX_PWMS being 64), but in order not to waste memory unnecessarily this was changed to a radix tree with the bitmap keeping track of free ranges.

Quoted 2 lines

In most cases I would expect that platforms only have a handful of PWMs (say less than 32), in which case a list lookup isn't too bad.

The difficulty with such a scheme is allocating a contiguous range of PWM IDs for each chip.

Quoted 5 lines

As someone else mentioned, it might be best to drop the global numbering scheme and have each pwm_chip have its own numbering for its PWMs. So requesting a PWM would first require getting a handle to the PWM chip it is on. If a chip has a fixed number of PWMs (e.g. set a registration time) then the PWMs within a chip can just be an array with O(1) lookup.

Yes, that would be the ultimate goal.

Quoted 7 lines
Quoted 3 lines

+static int alloc_descs(int pwm, unsigned int from, unsigned int count)

{

You don't really need the from argument. There is only one caller for alloc_descs and it passes 0 for from. So you could re-write this as:

Quoted 9 lines
	static int alloc_descs(int pwm, unsigned int count)
	{
		unsigned int from = 0;
		...

		if (pwm > MAX_PWMS)
			return -EINVAL;
		if (pwm >= 0)
			from = pwm;

You're right, I'll change that for the next series. Unless we decide to get rid of the whole pwm_desc structure.

Quoted 15 lines

drivers/pwm/core.c line 86

Quoted 11 lines
drivers/pwm/core.c +10 -1
-	struct pwm_device *pwm;
+	unsigned int start;
+	unsigned int i;
+	int ret = 0;
+
+	if (pwm >= 0) {
+		if (from > pwm)
+			return -EINVAL;
+
+		from = pwm;
+	}

This doesn't catch some caller errors:

Quoted 2 lines
	if (pwm > MAX_PWMS || from > MAX_PWMS)
		return -EINVAL;

I'll fix that as well.

Quoted 6 lines

drivers/pwm/core.c line 89

Quoted 3 lines
drivers/pwm/core.c +3 -0
+
+	start = bitmap_find_next_zero_area(allocated_pwms, MAX_PWMS, from,
+			count, 0);

Do the PWM indexes need to be contiguous for a given PWM chip? You could probably grab each bit individually.

Now that you mention it, I don't think so. Since there can currently only be a single provider of the PWM API there really aren't any restrictions at all since each chip will simply get the first N indices anyway.

Quoted 1 line

That said, I think a better solution is to have per-chip numbering.

Yes, I've heard that a number of times already. =)

Quoted 7 lines

drivers/pwm/core.c line 91

Quoted 4 lines
drivers/pwm/core.c +1 -3
-	list_for_each_entry(pwm, &pwm_list, node) {
-		if (pwm->chip->pwm_id == pwm_id)
-			return pwm;
+	if ((pwm >= 0) && (start != pwm))

Don't need the inner parens.

I usually like to keep it explicit but I can change it, no problem.

Quoted 8 lines

drivers/pwm/core.c line 95

Quoted 4 lines
drivers/pwm/core.c +4 -0
+		return -EEXIST;
+
+	if (start + count > MAX_PWMS)
+		return -ENOSPC;

Is this possible? Can bitmap_find_next_zero_area return a start position where the count would exceed the maximum?

It can indeed. Whenever the area doesn't fit it will return a number past the maximum (see lib/bitmap.c).

Quoted 22 lines
Quoted 8 lines
drivers/pwm/core.c +8 -0
+
+	for (i = start; i < start + count; i++) {
+		struct pwm_desc *desc = alloc_desc(i);
+		if (!desc) {
+			ret = -ENOMEM;
+			goto err;
+		}
+

drivers/pwm/core.c line 112

Quoted 10 lines
drivers/pwm/core.c +7 -1
+		radix_tree_insert(&pwm_desc_tree, i, desc);
 	}
 
-	return NULL;
+	bitmap_set(allocated_pwms, start, count);
+
+	return start;
+
+err:
+	for (i--; i >= 0; i--)

Nitpick:

Quoted 3 lines
	while (--i >= 0)

is a bit simpler.

Okay.

Quoted 24 lines
Quoted 24 lines
drivers/pwm/core.c +16 -5
+		free_desc(i);
+
+	return ret;
+}
+
+static void free_descs(unsigned int from, unsigned int count)
+{
+	unsigned int i;
+
+	for (i = from; i < from + count; i++)
+		free_desc(i);
+
+	bitmap_clear(allocated_pwms, from, count);
 }
 
 /**
- * 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,
+ * pwm_set_chip_data - set private chip data for a PWM
+ * @pwm: PWM number
+ * @data: pointer to chip-specific data
Quoted 8 lines

drivers/pwm/core.c line 133

Quoted 3 lines

*/

drivers/pwm/core.c +1 -1
-int pwmchip_add(struct pwm_chip *chip)
+int pwm_set_chip_data(unsigned int pwm, void *data)

This interface is a bit ugly. Shouldn't the user need to have a handle to the pwm_device in order to access the chip_data? Why should callers be able to set/grab chip data from random PWMs?

That interface is meant to be used by drivers to set and retrieve driver- private data for the PWM. Again, if we get rid of the radix tree and struct pwm_desc this can be done via the pwm_device structure directly.

Quoted 1 line
Quoted 1 line

+int pwmchip_add(struct pwm_chip *chip)

[...]

Quoted 9 lines

drivers/pwm/core.c line 181

Quoted 6 lines
drivers/pwm/core.c +6 -0
+	/* initialize descriptors */
+	for (i = chip->base; i < chip->base + chip->npwm; i++) {
+		struct pwm_desc *desc = pwm_to_desc(i);
+
+		if (!desc) {
+			pr_debug("pwm: descriptor %u not initialized\n", i);

Should this be a WARN_ON?

Yes. I just saw that WARN_ON can even be used within a condition, so I'll fix that up as well. Unless, of course, pwm_desc is removed.

Quoted 3 lines
Quoted 1 line

+EXPORT_SYMBOL_GPL(pwmchip_find);

So, propreitary modules are not allowed to use PWMs?

I just copied that from gpiolib. I don't know, should they?

Quoted 3 lines

drivers/pwm/core.c line 268

Quoted 1 line
drivers/pwm/core.c +1 -0
+	if ((pwm < 0) || (pwm >= MAX_PWMS))

Don't need the inner parens.

Okay.

Quoted 3 lines

drivers/pwm/core.c line 269

Quoted 1 line
drivers/pwm/core.c +1 -0
+		return ERR_PTR(-ENOENT);

-EINVAL maybe?

Yes, that's better.

Quoted 13 lines

drivers/pwm/core.c line 274

Quoted 10 lines
drivers/pwm/core.c +3 -5
+
 	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;

This looks wrong. If the pwm_desc being requested doesn't exist, won't this oops?

Yes, a check is missing here.

Quoted 10 lines

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_request); · line 316

Quoted 5 lines

void pwm_free(struct pwm_device *pwm)
{

drivers/pwm/core.c +2 -0
+	struct pwm_desc *desc = container_of(pwm, struct pwm_desc, pwm);
+
 	mutex_lock(&pwm_lock);

pwm_lock is used to protect global addition and removal from the radix/tree bitmap. Here you are also using it to protect the fields of a specific pwm device? Maybe it would be better to have a per pwm device lock for this?

I'm not sure. pwm_free() would be the sole user currently, so maybe it would be overkill at this point.

Quoted 11 lines

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_request); · line 324

Quoted 8 lines
drivers/pwm/core.c +2 -1
-	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;
+	pwm->pwm = 0;

Why do this? Isn't index 0 a valid PWM index?

I added that just to zero out all fields in the structure. And 0 is a valid index, yes. There is currently no such thing as an invalid PWM index. Perhaps that needs to be addressed as well. Alternatively this could just be handled by using the pwm_desc's flags field.

Thanks for the thorough review,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120208/e70501a8/attachment-0001.sig>

#10 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 17 lines · Thierry Reding
Quoted 7 lines · Ryan Mallon
Quoted 2 lines · Thierry Reding

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

It would be better to make the code handle arbitrary numbers of PWMs. A Kconfig knob becomes annoying when you have more than one platform configured into the kernel.

AFAICT handling an arbitrary number of PWMs will only be possible once we get rid of the global namespace and therefore should be postponed for later. I may be wrong, though, so if anybody can point me in the right direction I'm perfectly happy to change that in this series.

Quoted 3 lines
Quoted 2 lines

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

Quoted 7 lines
Quoted 3 lines

Does this patch actually break the drivers in terms of not building or running? If so, can this patch series be reworked a bit to allow the old PWM framework to be used until all of the drivers are converted?

That sentence is misleading. Since the new framework implements the exact same API as the drivers, any unconverted drivers will still work unless they are built within the same kernel as the framework.

I have a series porting all in kernel drivers to this framework. I can follow up on this series once it's clear we want to go this way. With this series the way will be free to move to a better pwm matching without a global pwm index because the drivers won't be affected by this change anymore.

Sascha

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

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 5 lines · Sascha Hauer

I have a series porting all in kernel drivers to this framework. I can follow up on this series once it's clear we want to go this way. With this series the way will be free to move to a better pwm matching without a global pwm index because the drivers won't be affected by this change anymore.

Nice. I already ported the PXA and Blackfin drivers in this series and was planning to port more in subsequent versions but if you've already done most of the work I can just skip. I should probably keep the Tegra driver in the series because it doesn't exist in the kernel tree yet.

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

#12 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

In reply to Thierry Reding
Quoted 5 lines · Thierry Reding
Quoted 4 lines · Ryan Mallon
Quoted 2 lines · Thierry Reding

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

Quoted 8 lines
Quoted 3 lines

It would be better to make the code handle arbitrary numbers of PWMs. A Kconfig knob becomes annoying when you have more than one platform configured into the kernel.

AFAICT handling an arbitrary number of PWMs will only be possible once we get rid of the global namespace and therefore should be postponed for later. I may be wrong, though, so if anybody can point me in the right direction I'm perfectly happy to change that in this series.

One solution would be to look at how genirq handles this. Eg, it uses a radix tree to store the irqdescs rather than an array, and a bitmap showing which global numbers are allocated.

#13 lore raw

PATCH v2 2/10 pwm: Allow chips to support multiple PWMs.

Quoted 10 lines · Russell King - ARM Linux
Quoted 9 lines · Thierry Reding
Quoted 7 lines · Ryan Mallon
Quoted 2 lines · Thierry Reding

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

It would be better to make the code handle arbitrary numbers of PWMs. A Kconfig knob becomes annoying when you have more than one platform configured into the kernel.

Quoted 8 lines
Quoted 4 lines

AFAICT handling an arbitrary number of PWMs will only be possible once we get rid of the global namespace and therefore should be postponed for later. I may be wrong, though, so if anybody can point me in the right direction I'm perfectly happy to change that in this series.

One solution would be to look at how genirq handles this. Eg, it uses a radix tree to store the irqdescs rather than an array, and a bitmap showing which global numbers are allocated.

That's exactly the solution implemented by this second version of the series. In fact I did turn to genirq for inspiration at the time I wrote the code. My understanding was that Ryan proposed to get rid of the bitmap altogether because it arbitrarily limits the number of PWMs. How that can be achieved with a global namespace I don't know. Thus my proposal to keep it as-is for now and get rid of it once per-chip indexing is implemented.

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

#14 lore raw

PATCH v2 3/10 of: Add PWM support.

In reply to Thierry Reding

This patch adds helpers to support device tree bindings for the generic PWM API. Device tree binding documentation for PWM controllers is also provided.

---
Changes in v2:
- add device tree binding documentation
- add of_xlate to parse controller-specific PWM-specifier

 Documentation/devicetree/bindings/pwm/pwm.txt |   50 ++++++++++
 drivers/of/Kconfig                            |    6 +
 drivers/of/Makefile                           |    1 +
 drivers/of/pwm.c                              |  130 +++++++++++++++++++++++++
 include/linux/of_pwm.h                        |   51 ++++++++++
 include/linux/pwm.h                           |   17 +++
 6 files changed, 255 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm.txt
 create mode 100644 drivers/of/pwm.c
 create mode 100644 include/linux/of_pwm.h
Documentation/devicetree/bindings/pwm/pwm.txt new file +50 -0
new file mode 100644
index 0000000..1c34e86
@@ -0,0 +1,50 @@
+Specifying PWM information for devices
+======================================
+
+1) PWM user nodes
+-----------------
+
+PWM users should specify a list of PWM devices that they want to use
+with a property containing a 'pwm-list':
+
+	pwm-list ::= <single-pwm> [pwm-list]
+	single-pwm ::= <pwm-phandle> <pwm-specifier>
+	pwm-phandle : phandle to PWM controller node
+	pwm-specifier : array of #pwm-cells specifying the given PWM
+			(controller specific)
+
+PWM properties should be named "[<name>-]pwms". Exact meaning of each
+pwms property must be documented in the device tree binding for each
+device.
+
+The following example could be used to describe a PWM-based backlight
+device:
+
+	pwm: pwm {
+		#pwm-cells = <2>;
+	};
+
+	[...]
+
+	bl: backlight {
+		pwms = <&pwm 0 5000000>;
+	};
+
+pwm-specifier typically encodes the chip-relative PWM number and the PWM
+period in nanoseconds.
+
+2) PWM controller nodes
+-----------------------
+
+PWM controller nodes must specify the number of cells used for the
+specifier using the '#pwm-cells' property.
+
+An example PWM controller might look like this:
+
+	pwm: pwm at 7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
+TODO: do we need an empty 'pwm-controller' property?
drivers/of/Kconfig +6 -0
index 268163d..9635468 100644
@@ -56,6 +56,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
drivers/of/Makefile +1 -0
index a73f5a5..3dd13e3 100644
@@ -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
drivers/of/pwm.c new file +130 -0

130 lines not on this page · raw message

include/linux/of_pwm.h new file +51 -0

51 lines not on this page · raw message

include/linux/pwm.h +17 -0
index 0f8d105..8595be4 100644
@@ -1,6 +1,8 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/of.h>
+
 struct pwm_device;
 
 /*
@@ -56,6 +58,14 @@ struct pwm_ops {
 };
 
 /**
+ * struct pwm_spec - device tree PWM specifier
+ * @period: PWM period (in nanoseconds)
+ */
+struct pwm_spec {
+	unsigned int period;
+};
+
+/**
  * struct pwm_chip - abstract a PWM controller
  * @dev: device providing the PWMs
  * @ops: callbacks for this PWM controller
@@ -67,6 +77,13 @@ struct pwm_chip {
 	struct pwm_ops		*ops;
 	int			base;
 	unsigned int		npwm;
+
+#ifdef CONFIG_OF_PWM
+	int			(*of_xlate)(struct pwm_chip *pc,
+					    const struct of_phandle_args *args,
+					    struct pwm_spec *spec);
+	unsigned int		of_pwm_n_cells;
+#endif
 };
 
 int pwm_set_chip_data(unsigned int pwm, void *data);
Signature · 1 line
1.7.9
#15 lore raw

PATCH v2 4/10 arm/tegra: Fix PWM clock programming

In reply to Thierry Reding

From: Simon Que <[email protected]>

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

---
Changes in v2:
- update commit message to account for changes

 arch/arm/mach-tegra/clock.h         |    1 +
 arch/arm/mach-tegra/tegra2_clocks.c |   28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)
arch/arm/mach-tegra/clock.h +1 -0
index bc30065..e18d77e 100644
@@ -49,6 +49,7 @@
 #define PLLM                    (1 << 20)
 #define DIV_U71_INT             (1 << 21)
 #define DIV_U71_IDLE            (1 << 22)
+#define PERIPH_SOURCE_CLK_4BIT	(1 << 23)
 #define ENABLE_ON_INIT		(1 << 28)
 #define PERIPH_ON_APB           (1 << 29)
 
arch/arm/mach-tegra/tegra2_clocks.c +24 -4
index 74d314f..379f201 100644
@@ -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
@@ -908,9 +910,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);
@@ -1023,12 +1032,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);
@@ -2126,7 +2146,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),
Signature · 1 line
1.7.9
#16 lore raw

PATCH v2 5/10 arm/tegra: Provide clock for only one PWM controller

In reply to 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.

---
 arch/arm/mach-tegra/tegra2_clocks.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
arch/arm/mach-tegra/tegra2_clocks.c +1 -4
index 379f201..6eec82d 100644
@@ -2245,10 +2245,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"),
Signature · 1 line
1.7.9
#17 lore raw

PATCH v2 6/10 pwm: Add NVIDIA Tegra SoC support

In reply to Thierry Reding

This commit adds a generic PWM framework driver for the PWFM controller found on NVIDIA Tegra SoCs. The driver is based on code from the Chromium kernel tree and was originally written by Gary King (NVIDIA) and later modified by Simon Que (Chromium).

---
Changes in v2:
- set pwm_chip.dev field
- rename clk_enb to enable
- introduce NUM_PWM macro instead of hard-coding the number of PWM devices
- update comment in tegra_pwm_config
- fix coding-style for multi-line comments
- use module_platform_driver
- change license to GPL

 drivers/pwm/Kconfig     |   10 ++
 drivers/pwm/Makefile    |    1 +
 drivers/pwm/pwm-tegra.c |  266 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 277 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-tegra.c
drivers/pwm/Kconfig +10 -0
index 93c1052..bda6f23 100644
@@ -9,4 +9,14 @@ menuconfig PWM
 
 if PWM
 
+config PWM_TEGRA
+	tristate "NVIDIA Tegra PWM support"
+	depends on ARCH_TEGRA
+	help
+	  Generic PWM framework driver for the PWFM controller found on NVIDIA
+	  Tegra SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-tegra.
+
 endif
drivers/pwm/Makefile +1 -0
index 3469c3d..12300f5 100644
@@ -1 +1,2 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
drivers/pwm/pwm-tegra.c new file +266 -0

266 lines not on this page · raw message

Signature · 1 line
1.7.9
#18 lore raw

PATCH v2 7/10 arm/tegra: Add PWFM controller device tree probing

In reply to Thierry Reding

Add auxdata to instantiate a device tree for the PWFM controller and include a corresponding node in the device tree.

---
 arch/arm/boot/dts/tegra20.dtsi         |    6 ++++++
 arch/arm/boot/dts/tegra30.dtsi         |    6 ++++++
 arch/arm/mach-tegra/board-dt-tegra20.c |    1 +
 arch/arm/mach-tegra/board-dt-tegra30.c |    1 +
 4 files changed, 14 insertions(+), 0 deletions(-)
arch/arm/boot/dts/tegra20.dtsi +6 -0
index ec1f010..c34e6c3 100644
@@ -148,6 +148,12 @@
 		interrupts = < 0 91 0x04 >;
 	};
 
+	pwm: pwm at 7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 	emc at 7000f400 {
 		#address-cells = <1>;
 		#size-cells = <0>;
arch/arm/boot/dts/tegra30.dtsi +6 -0
index ac4b75c..77bd985 100644
@@ -146,6 +146,12 @@
 		interrupts = < 0 91 0x04 >;
 	};
 
+	pwm: pwm at 7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 	sdhci at 78000000 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000000 0x200>;
arch/arm/mach-tegra/board-dt-tegra20.c +1 -0
index 7a95e0b..bad0dd1 100644
@@ -73,6 +73,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),
 	{}
 };
 
arch/arm/mach-tegra/board-dt-tegra30.c +1 -0
index b4124b1..dc7324c 100644
@@ -51,6 +51,7 @@ struct of_dev_auxdata tegra30_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("nvidia,tegra20-i2c", 0x7000C500, "tegra-i2c.2", NULL),
 	OF_DEV_AUXDATA("nvidia,tegra20-i2c", 0x7000C700, "tegra-i2c.3", NULL),
 	OF_DEV_AUXDATA("nvidia,tegra20-i2c", 0x7000D000, "tegra-i2c.4", NULL),
+	OF_DEV_AUXDATA("nvidia,tegra20-pwm", TEGRA_PWFM_BASE, "tegra-pwm", NULL),
 	{}
 };
 
Signature · 1 line
1.7.9
#19 lore raw

PATCH v2 8/10 pwm: Add Blackfin support

In reply to Thierry Reding
---
 arch/blackfin/Kconfig         |   10 ---
 arch/blackfin/kernel/Makefile |    1 -
 arch/blackfin/kernel/pwm.c    |  100 -------------------------
 drivers/pwm/Kconfig           |    9 ++
 drivers/pwm/Makefile          |    1 +
 drivers/pwm/pwm-bfin.c        |  164 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 174 insertions(+), 111 deletions(-)
 delete mode 100644 arch/blackfin/kernel/pwm.c
 create mode 100644 drivers/pwm/pwm-bfin.c
arch/blackfin/Kconfig +0 -10
index abe5a9e..3f23fd3 100644
@@ -949,16 +949,6 @@ config BFIN_GPTIMERS
 	  To compile this driver as a module, choose M here: the module
 	  will be called gptimers.
 
-config HAVE_PWM
-	tristate "Enable PWM API support"
-	depends on BFIN_GPTIMERS
-	help
-	  Enable support for the Pulse Width Modulation framework (as
-	  found in linux/pwm.h).
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called pwm.
-
 choice
 	prompt "Uncached DMA region"
 	default DMA_UNCACHED_1M
arch/blackfin/kernel/Makefile +0 -1
index 1f88edd..03f60a6 100644
@@ -21,7 +21,6 @@ obj-$(CONFIG_FUNCTION_TRACER)        += ftrace-entry.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER)  += ftrace.o
 CFLAGS_REMOVE_ftrace.o = -pg
 
-obj-$(CONFIG_HAVE_PWM)               += pwm.o
 obj-$(CONFIG_IPIPE)                  += ipipe.o
 obj-$(CONFIG_BFIN_GPTIMERS)          += gptimers.o
 obj-$(CONFIG_CPLB_INFO)              += cplbinfo.o
arch/blackfin/kernel/pwm.c deleted +0 -100

100 lines not on this page · raw message

drivers/pwm/Kconfig +9 -0
index bda6f23..eb54042 100644
@@ -9,6 +9,15 @@ menuconfig PWM
 
 if PWM
 
+config PWM_BFIN
+	tristate "Blackfin PWM support"
+	depends on BFIN_GPTIMERS
+	help
+	  Generic PWM framework driver for Blackfin.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-bfin.
+
 config PWM_TEGRA
 	tristate "NVIDIA Tegra PWM support"
 	depends on ARCH_TEGRA
drivers/pwm/Makefile +1 -0
index 12300f5..251b8d2 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
drivers/pwm/pwm-bfin.c new file +164 -0

164 lines not on this page · raw message

Signature · 1 line
1.7.9
#20 lore raw

PATCH v2 9/10 pwm: Add PXA support

In reply to Thierry Reding
---
 arch/arm/plat-pxa/Makefile |    1 -
 arch/arm/plat-pxa/pwm.c    |  304 --------------------------------------------
 drivers/pwm/Kconfig        |    9 ++
 drivers/pwm/Makefile       |    1 +
 drivers/pwm/pwm-pxa.c      |  243 +++++++++++++++++++++++++++++++++++
 5 files changed, 253 insertions(+), 305 deletions(-)
 delete mode 100644 arch/arm/plat-pxa/pwm.c
 create mode 100644 drivers/pwm/pwm-pxa.c
arch/arm/plat-pxa/Makefile +0 -1
index f302d04..af8e484 100644
@@ -8,5 +8,4 @@ obj-$(CONFIG_PXA3xx)		+= mfp.o
 obj-$(CONFIG_PXA95x)		+= mfp.o
 obj-$(CONFIG_ARCH_MMP)		+= mfp.o
 
-obj-$(CONFIG_HAVE_PWM)		+= pwm.o
 obj-$(CONFIG_PXA_SSP)		+= ssp.o
arch/arm/plat-pxa/pwm.c deleted +0 -304

304 lines not on this page · raw message

drivers/pwm/Kconfig +9 -0
index eb54042..0ef4f30 100644
@@ -18,6 +18,15 @@ config PWM_BFIN
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-bfin.
 
+config PWM_PXA
+	tristate "PXA PWM support"
+	depends on ARCH_PXA
+	help
+	  Generic PWM framework driver for PXA.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-pxa.
+
 config PWM_TEGRA
 	tristate "NVIDIA Tegra PWM support"
 	depends on ARCH_TEGRA
drivers/pwm/Makefile +1 -0
index 251b8d2..e859c51 100644
@@ -1,3 +1,4 @@
 obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
+obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
drivers/pwm/pwm-pxa.c new file +243 -0

243 lines not on this page · raw message

Signature · 1 line
1.7.9
#21 lore raw

PATCH v2 10/10 pwm-backlight: Add rudimentary device tree support

In reply to 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.

A pointer to the exit() callback is stored in the driver data to keep it around until the driver is unloaded.

---
Changes in v2:
- avoid oops by keeping a reference to the platform-specific exit() callback

 .../bindings/video/backlight/pwm-backlight         |   16 ++++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |   81 ++++++++++++++++++--
 3 files changed, 90 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/pwm-backlight
Documentation/devicetree/bindings/video/backlight/pwm-backlight new file +16 -0
new file mode 100644
index 0000000..ce65280
@@ -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>;
+	};
drivers/video/backlight/Kconfig +1 -1
index 49e7d83..37982fa 100644
@@ -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.
drivers/video/backlight/pwm_bl.c +73 -8

134 lines not on this page · raw message

Signature · 1 line
1.7.9