[PATCH v4 00/10] Add PWM framework and device tree support.

[PATCH v4 00/10] Add PWM framework and device tree support.

[Thierry Reding]

This patch series adds very rudimentary device-tree support for PWM
devices. 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-bl
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 (the Blackfin and PXA drivers have been ported to
the framework for reference). Currently a global namespace is still
provided to keep backwards-compatibility with the legacy PWM API. In
order to achieve this, the number of global PWM devices is limited to
1024. However, patch 2 introduces per-chip indexing of PWM devices in
the core and adds an API to request a PWM device given the PWM chip or
the struct device associated with a PWM chip. They are supposed to
eventually replace pwm_request() and allow to get rid of the global
namespace. The device tree support code does not use the global
namespace.

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-bl driver to find the
PWM device that it should be using. If the no corresponding PWM chip is
available yet, the code returns EPROBE_DEFER to have users automatically
probed again later when the PWM provider may have become available.
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. Some cleanup of the
clock registration for Tegra is done in patch 5 because a subsequent
patch will instantiate one PWFM controller device 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. Device tree based
probing of the driver is implemented in patch 7.

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.
Sascha already has patches that port all in-tree drivers to the
framework and will rebase them on top of this series when it is ready.

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). Switching power to the backlight via
GPIOs is not supported yet. The DT binding also deviates from the
platform data in that it requires a list of brightness levels to be
specified instead of assuming a linearily spaced range from 0 to a given
maximum brightness.

The whole series is based on the linux-next tree from 20120314. I think
I've addressed all of the concerns raised in the first three versions.

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.
  pwm: Add device tree support
  ARM: tegra: Provide clock for only one PWM controller
  pwm: Add NVIDIA Tegra SoC support
  pwm: tegra: Add device tree support
  pwm: Add Blackfin support
  pwm: Add PXA support
  pwm-backlight: Add rudimentary device tree support

 Documentation/devicetree/bindings/pwm/pwm.txt      |   48 ++
 .../devicetree/bindings/pwm/tegra-pwm.txt          |   18 +
 .../bindings/video/backlight/pwm-backlight         |   19 +
 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             |    3 +
 arch/arm/mach-tegra/tegra2_clocks.c                |   37 +-
 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/pwm/Kconfig                                |   40 ++
 drivers/pwm/Makefile                               |    4 +
 drivers/pwm/core.c                                 |  557 ++++++++++++++++++++
 drivers/pwm/pwm-bfin.c                             |  164 ++++++
 drivers/pwm/pwm-pxa.c                              |  244 +++++++++
 drivers/pwm/pwm-tegra.c                            |  267 ++++++++++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |  136 ++++-
 include/linux/of_pwm.h                             |   36 ++
 include/linux/pwm.h                                |   97 ++++
 include/linux/pwm_backlight.h                      |    2 +
 29 files changed, 1732 insertions(+), 442 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/tegra-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/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

-- 
1.7.9.4

[PATCH v4 01/10] PWM: add pwm framework support

[Thierry Reding]

From: Sascha Hauer <s.hauer@pengutronix.de>

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

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

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

Why another framework?

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

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

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Matthias Kaehlcke <matthias@kaehlcke.net>
---
 Documentation/pwm.txt |   56 +++++++++++++
 MAINTAINERS           |    6 ++
 drivers/Kconfig       |    2 +
 drivers/Makefile      |    1 +
 drivers/pwm/Kconfig   |   12 +++
 drivers/pwm/Makefile  |    1 +
 drivers/pwm/core.c    |  220 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h   |   37 +++++++++
 8 files changed, 335 insertions(+)
 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 b9a1fb31..fd2c297 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5403,6 +5403,12 @@ S:	Maintained
 F:	Documentation/video4linux/README.pvrusb2
 F:	drivers/media/video/pvrusb2/
 
+PPWM core
+M:	Sascha Hauer <s.hauer@pengutronix.de>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	drivers/pwm/
+
 PXA2xx/PXA3xx SUPPORT
 M:	Eric Miao <eric.y.miao@gmail.com>
 M:	Russell King <linux@arm.linux.org.uk>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index b3ae9e5..ec094b5 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -142,4 +142,6 @@ source "drivers/devfreq/Kconfig"
 
 source "drivers/modem_shm/Kconfig"
 
+source "drivers/pwm/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 4855a1a..52fe956 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@pengutronix.de>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2, or (at your option)
+ *  any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; see the file COPYING.  If not, write to
+ *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/module.h>
+#include <linux/pwm.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+
+struct pwm_device {
+	struct			pwm_chip *chip;
+	const char		*label;
+	unsigned long		flags;
+#define FLAG_REQUESTED	0
+#define FLAG_ENABLED	1
+	struct list_head	node;
+};
+
+static LIST_HEAD(pwm_list);
+
+static DEFINE_MUTEX(pwm_lock);
+
+static struct pwm_device *_find_pwm(int pwm_id)
+{
+	struct pwm_device *pwm;
+
+	list_for_each_entry(pwm, &pwm_list, node) {
+		if (pwm->chip->pwm_id == pwm_id)
+			return pwm;
+	}
+
+	return NULL;
+}
+
+/**
+ * pwmchip_add() - register a new pwm
+ * @chip: the pwm
+ *
+ * register a new pwm. pwm->pwm_id must be initialized. if pwm_id < 0 then
+ * a dynamically assigned id will be used, otherwise the id specified,
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+	struct pwm_device *pwm;
+	int ret = 0;
+
+	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->chip = chip;
+
+	mutex_lock(&pwm_lock);
+
+	if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	list_add_tail(&pwm->node, &pwm_list);
+out:
+	mutex_unlock(&pwm_lock);
+
+	if (ret)
+		kfree(pwm);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_add);
+
+/**
+ * pwmchip_remove() - remove a pwm
+ * @chip: the pwm
+ *
+ * remove a pwm. This function may return busy if the pwm is still requested.
+ */
+int pwmchip_remove(struct pwm_chip *chip)
+{
+	struct pwm_device *pwm;
+	int ret = 0;
+
+	mutex_lock(&pwm_lock);
+
+	pwm = _find_pwm(chip->pwm_id);
+	if (!pwm) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	list_del(&pwm->node);
+
+	kfree(pwm);
+out:
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_remove);
+
+/*
+ * pwm_request - request a PWM device
+ */
+struct pwm_device *pwm_request(int pwm_id, const char *label)
+{
+	struct pwm_device *pwm;
+	int ret;
+
+	mutex_lock(&pwm_lock);
+
+	pwm = _find_pwm(pwm_id);
+	if (!pwm) {
+		pwm = ERR_PTR(-ENOENT);
+		goto out;
+	}
+
+	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
+		pwm = ERR_PTR(-EBUSY);
+		goto out;
+	}
+
+	if (!try_module_get(pwm->chip->ops->owner)) {
+		pwm = ERR_PTR(-ENODEV);
+		goto out;
+	}
+
+	if (pwm->chip->ops->request) {
+		ret = pwm->chip->ops->request(pwm->chip);
+		if (ret) {
+			pwm = ERR_PTR(ret);
+			goto out_put;
+		}
+	}
+
+	pwm->label = label;
+	set_bit(FLAG_REQUESTED, &pwm->flags);
+
+	goto out;
+
+out_put:
+	module_put(pwm->chip->ops->owner);
+out:
+	mutex_unlock(&pwm_lock);
+
+	return pwm;
+}
+EXPORT_SYMBOL_GPL(pwm_request);
+
+/*
+ * pwm_free - free a PWM device
+ */
+void pwm_free(struct pwm_device *pwm)
+{
+	mutex_lock(&pwm_lock);
+
+	if (!test_and_clear_bit(FLAG_REQUESTED, &pwm->flags)) {
+		pr_warning("PWM device already freed\n");
+		goto out;
+	}
+
+	pwm->label = NULL;
+
+	module_put(pwm->chip->ops->owner);
+out:
+	mutex_unlock(&pwm_lock);
+}
+EXPORT_SYMBOL_GPL(pwm_free);
+
+/*
+ * pwm_config - change a PWM device configuration
+ */
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+	return pwm->chip->ops->config(pwm->chip, duty_ns, period_ns);
+}
+EXPORT_SYMBOL_GPL(pwm_config);
+
+/*
+ * pwm_enable - start a PWM output toggling
+ */
+int pwm_enable(struct pwm_device *pwm)
+{
+	if (!test_and_set_bit(FLAG_ENABLED, &pwm->flags))
+		return pwm->chip->ops->enable(pwm->chip);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwm_enable);
+
+/*
+ * pwm_disable - stop a PWM output toggling
+ */
+void pwm_disable(struct pwm_device *pwm)
+{
+	if (test_and_clear_bit(FLAG_ENABLED, &pwm->flags))
+		pwm->chip->ops->disable(pwm->chip);
+}
+EXPORT_SYMBOL_GPL(pwm_disable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 7c77575..df9681b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -28,4 +28,41 @@ int pwm_enable(struct pwm_device *pwm);
  */
 void pwm_disable(struct pwm_device *pwm);
 
+#ifdef CONFIG_PWM
+struct pwm_chip;
+
+/**
+ * struct pwm_ops - PWM operations
+ * @request: optional hook for requesting a PWM
+ * @free: optional hook for freeing a PWM
+ * @config: configure duty cycles and period length for this PWM
+ * @enable: enable PWM output toggling
+ * @disable: disable PWM output toggling
+ */
+struct pwm_ops {
+	int			(*request)(struct pwm_chip *chip);
+	void			(*free)(struct pwm_chip *chip);
+	int			(*config)(struct pwm_chip *chip, int duty_ns,
+						int period_ns);
+	int			(*enable)(struct pwm_chip *chip);
+	void			(*disable)(struct pwm_chip *chip);
+	struct module		*owner;
+};
+
+/**
+ * struct pwm_chip - abstract a PWM
+ * @label: for diagnostics
+ * @owner: helps prevent removal of modules exporting active PWMs
+ * @ops: The callbacks for this PWM
+ */
+struct pwm_chip {
+	int			pwm_id;
+	const char		*label;
+	struct pwm_ops		*ops;
+};
+
+int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_remove(struct pwm_chip *chip);
+#endif
+
 #endif /* __LINUX_PWM_H */
-- 
1.7.9.4

Re: [PATCH v4 01/10] PWM: add pwm framework support

[Lars-Peter Clausen]

On 03/14/2012 04:56 PM, Thierry Reding wrote:
> [...]
> +struct pwm_chip {
> +	int			pwm_id;
> +	const char		*label;
> +	struct pwm_ops		*ops;

Nothing major, but if you are going for another round it would be good to see
ops made const.

> +};

Re: [PATCH v4 01/10] PWM: add pwm framework support

[Thierry Reding]

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

* Lars-Peter Clausen wrote:
> On 03/14/2012 04:56 PM, Thierry Reding wrote:
> > [...]
> > +struct pwm_chip {
> > +	int			pwm_id;
> > +	const char		*label;
> > +	struct pwm_ops		*ops;
> 
> Nothing major, but if you are going for another round it would be good to see
> ops made const.

I'll go for another round because I just noticed that I still have two open
TODO items that I forgot to fix from last round's review.

I'll take a note to make it const.

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: [PATCH v4 01/10] PWM: add pwm framework support

[Shawn Guo]

On Wed, Mar 14, 2012 at 04:56:24PM +0100, Thierry Reding wrote:
...
> +PPWM core

s/PPWM/PWM?

> +M:	Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> +L:	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	drivers/pwm/
> +

-- 
Regards,
Shawn

Re: [PATCH v4 01/10] PWM: add pwm framework support

[Thierry Reding]

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

* Shawn Guo wrote:
> On Wed, Mar 14, 2012 at 04:56:24PM +0100, Thierry Reding wrote:
> ...
> > +PPWM core
> 
> s/PPWM/PWM?

Yes, I noticed that as well just yesterday.

Thanks,
Thierry

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

Re: [PATCH v4 01/10] PWM: add pwm framework support

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> This patch adds framework support for PWM (pulse width modulation) devices.
...
> 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 anywhere in the code that implements "if pwm_id < 0 then a
dynamically assigned id will be used".

Re: [PATCH v4 01/10] PWM: add pwm framework support

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> > From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > 
> > This patch adds framework support for PWM (pulse width modulation) devices.
> ...
> > 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 anywhere in the code that implements "if pwm_id < 0 then a
> dynamically assigned id will be used".

The second patch in the series fixes up this comment when the functionality
is properly implemented. I still wanted to go over the first patch and fix up
some minor things, so maybe I'll fix this up as I go along.

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

[PATCH v4 02/10] pwm: Allow chips to support multiple PWMs.

[Thierry Reding]

Many PWM controllers provide access to more than a single PWM output and
may even share some resource among them. Allowing a PWM chip to provide
multiple PWM devices enables better sharing of those resources. As a
side-effect this change allows easy integration with the device tree
where a given PWM can be looked up based on the PWM chip's phandle and a
corresponding index.

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 1024 while
the data is actually stored in a radix tree, thus saving resources if
not all of them are used.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v4:
  - remove OF-specific code to preserve bisectability
  - add pwm_request_from_device() and pwm_request_from_chip() functions
  - refactor PWM requesting code
  - store period in struct pwm_device and get rid of the unused struct
    pwm_spec

Changes in v3:
  - get rid of pwm_desc structure and keep only struct pwm_device
  - keep a list of pwm_chip structures for fast and easy lookup
  - pass struct pwm_device directly to pwm_ops
  - add debugfs file

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

 drivers/pwm/core.c  |  376 +++++++++++++++++++++++++++++++++++++++------------
 include/linux/pwm.h |   78 +++++++++--
 2 files changed, 358 insertions(+), 96 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 71de479..74dd295 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -2,6 +2,7 @@
  * Generic pwmlib implementation
  *
  * Copyright (C) 2011 Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ * Copyright (C) 2011-2012 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
@@ -17,105 +18,207 @@
  *  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/radix-tree.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.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);
+#define MAX_PWMS 1024
 
 static DEFINE_MUTEX(pwm_lock);
+static LIST_HEAD(pwm_chips);
+static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
+static RADIX_TREE(pwm_tree, GFP_KERNEL);
 
-static struct pwm_device *_find_pwm(int pwm_id)
+static struct pwm_device *pwm_to_device(unsigned int pwm)
 {
-	struct pwm_device *pwm;
+	return radix_tree_lookup(&pwm_tree, pwm);
+}
+
+static int alloc_pwms(int pwm, unsigned int count)
+{
+	unsigned int from = 0;
+	unsigned int start;
+
+	if (pwm >= MAX_PWMS)
+		return -EINVAL;
+
+	if (pwm >= 0)
+		from = pwm;
+
+	start = bitmap_find_next_zero_area(allocated_pwms, MAX_PWMS, from,
+					   count, 0);
+
+	if (pwm >= 0 && start != pwm)
+		return -EEXIST;
+
+	if (start + count > MAX_PWMS)
+		return -ENOSPC;
+
+	return start;
+}
+
+static void free_pwms(struct pwm_chip *chip)
+{
+	unsigned int i;
+
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+		radix_tree_delete(&pwm_tree, pwm->pwm);
+	}
+
+	bitmap_clear(allocated_pwms, chip->base, chip->npwm);
+
+	kfree(chip->pwms);
+	chip->pwms = NULL;
+}
+
+static struct pwm_chip *device_to_pwmchip(struct device *dev)
+{
+	struct pwm_chip *chip;
+
+	mutex_lock(&pwm_lock);
+
+	list_for_each_entry(chip, &pwm_chips, list)
+		if (chip->dev == dev) {
+			mutex_unlock(&pwm_lock);
+			return chip;
+		}
+
+	mutex_unlock(&pwm_lock);
+
+	return ERR_PTR(-ENODEV);
+}
+
+static int pwm_device_request(struct pwm_device *pwm, const char *label)
+{
+	int err;
+
+	if (test_bit(PWMF_REQUESTED, &pwm->flags))
+		return -EBUSY;
+
+	if (!try_module_get(pwm->chip->ops->owner))
+		return -ENODEV;
 
-	list_for_each_entry(pwm, &pwm_list, node) {
-		if (pwm->chip->pwm_id == pwm_id)
-			return pwm;
+	if (pwm->chip->ops->request) {
+		err = pwm->chip->ops->request(pwm->chip, pwm);
+		if (err) {
+			module_put(pwm->chip->ops->owner);
+			return err;
+		}
 	}
 
-	return NULL;
+	set_bit(PWMF_REQUESTED, &pwm->flags);
+	pwm->label = label;
+
+	return 0;
+}
+
+/**
+ * pwm_set_chip_data - set private chip data for a PWM
+ * @pwm: PWM device
+ * @data: pointer to chip-specific data
+ */
+int pwm_set_chip_data(struct pwm_device *pwm, void *data)
+{
+	if (!pwm)
+		return -EINVAL;
+
+	pwm->chip_data = data;
+
+	return 0;
 }
 
 /**
- * pwmchip_add() - register a new pwm
- * @chip: the pwm
+ * pwm_get_chip_data - get private chip data for a PWM
+ * @pwm: PWM device
+ */
+void *pwm_get_chip_data(struct pwm_device *pwm)
+{
+	return pwm ? pwm->chip_data : NULL;
+}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
  *
- * 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 chip->base < 0 then a dynamically assigned base
+ * will be used.
  */
 int pwmchip_add(struct pwm_chip *chip)
 {
 	struct pwm_device *pwm;
-	int ret = 0;
+	unsigned int i;
+	int ret;
 
-	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
-	if (!pwm)
+	mutex_lock(&pwm_lock);
+
+	ret = alloc_pwms(chip->base, chip->npwm);
+	if (ret < 0)
+		goto out;
+
+	chip->pwms = kzalloc(chip->npwm * sizeof(*pwm), GFP_KERNEL);
+	if (!chip->pwms)
 		return -ENOMEM;
 
-	pwm->chip = chip;
+	chip->base = ret;
 
-	mutex_lock(&pwm_lock);
+	for (i = 0; i < chip->npwm; i++) {
+		pwm = &chip->pwms[i];
 
-	if (chip->pwm_id >= 0 && _find_pwm(chip->pwm_id)) {
-		ret = -EBUSY;
-		goto out;
+		pwm->chip = chip;
+		pwm->pwm = chip->base + i;
+		pwm->hwpwm = i;
+
+		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 
-	list_add_tail(&pwm->node, &pwm_list);
-out:
-	mutex_unlock(&pwm_lock);
+	bitmap_set(allocated_pwms, chip->base, chip->npwm);
 
-	if (ret)
-		kfree(pwm);
+	INIT_LIST_HEAD(&chip->list);
+	list_add(&chip->list, &pwm_chips);
 
+out:
+	mutex_unlock(&pwm_lock);
 	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 = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
 
-	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
-		ret = -EBUSY;
-		goto out;
+		if (test_bit(PWMF_REQUESTED, &pwm->flags)) {
+			ret = -EBUSY;
+			goto out;
+		}
 	}
 
-	list_del(&pwm->node);
+	list_del_init(&chip->list);
+	free_pwms(chip);
 
-	kfree(pwm);
 out:
 	mutex_unlock(&pwm_lock);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
@@ -123,50 +226,89 @@ 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_request(int pwm, const char *label)
 {
-	struct pwm_device *pwm;
-	int ret;
+	struct pwm_device *dev;
+	int err;
+
+	if (pwm < 0 || pwm >= MAX_PWMS)
+		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&pwm_lock);
 
-	pwm = _find_pwm(pwm_id);
-	if (!pwm) {
-		pwm = ERR_PTR(-ENOENT);
+	dev = pwm_to_device(pwm);
+	if (!dev) {
+		dev = ERR_PTR(-ENODEV);
 		goto out;
 	}
 
-	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
-		pwm = ERR_PTR(-EBUSY);
-		goto out;
-	}
+	err = pwm_device_request(dev, label);
+	if (err < 0)
+		dev = ERR_PTR(err);
 
-	if (!try_module_get(pwm->chip->ops->owner)) {
-		pwm = ERR_PTR(-ENODEV);
-		goto out;
-	}
+out:
+	mutex_unlock(&pwm_lock);
 
-	if (pwm->chip->ops->request) {
-		ret = pwm->chip->ops->request(pwm->chip);
-		if (ret) {
-			pwm = ERR_PTR(ret);
-			goto out_put;
-		}
-	}
+	return dev;
+}
+EXPORT_SYMBOL_GPL(pwm_request);
 
-	pwm->label = label;
-	set_bit(FLAG_REQUESTED, &pwm->flags);
+/**
+ * pwm_request_from_device() - request a PWM device relative to a device
+ * @dev: device representing a PWM chip
+ * @index: per-chip index of the PWM to request
+ * @label: a literal description string of this PWM
+ *
+ * Returns the PWM at the given index of the PWM chip represented by the
+ * given device. A negative error code is returned if the device does not
+ * represent a PWM chip, the index is not valid for the specified PWM chip
+ * or if the PWM device cannot be requested.
+ */
+struct pwm_device *pwm_request_from_device(struct device *dev,
+					   unsigned int index,
+					   const char *label)
+{
+	struct pwm_chip *chip;
 
-	goto out;
+	chip = device_to_pwmchip(dev);
+	if (IS_ERR(chip))
+		return ERR_CAST(chip);
 
-out_put:
-	module_put(pwm->chip->ops->owner);
-out:
-	mutex_unlock(&pwm_lock);
+	return pwm_request_from_chip(chip, index, label);
+}
+EXPORT_SYMBOL_GPL(pwm_request_from_device);
+
+/**
+ * pwm_request_from_chip() - request a PWM device relative to a PWM chip
+ * @chip: PWM chip
+ * @index: per-chip index of the PWM to request
+ * @label: a literal description string of this PWM
+ *
+ * Returns the PWM at the given index of the given PWM chip. A negative error
+ * code is returned if the index is not valid for the specified PWM chip or
+ * if the PWM device cannot be requested.
+ */
+struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
+					 unsigned int index,
+					 const char *label)
+{
+	struct pwm_device *pwm;
+	int err;
+
+	if (index >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&pwm_lock);
+	pwm = &chip->pwms[index];
+
+	err = pwm_device_request(pwm, label);
+	if (err < 0)
+		pwm = ERR_PTR(err);
 
+	mutex_unlock(&pwm_lock);
 	return pwm;
 }
-EXPORT_SYMBOL_GPL(pwm_request);
+EXPORT_SYMBOL_GPL(pwm_request_from_chip);
 
 /*
  * pwm_free - free a PWM device
@@ -175,11 +317,14 @@ void pwm_free(struct pwm_device *pwm)
 {
 	mutex_lock(&pwm_lock);
 
-	if (!test_and_clear_bit(FLAG_REQUESTED, &pwm->flags)) {
+	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
 		pr_warning("PWM device already freed\n");
 		goto out;
 	}
 
+	if (pwm->chip->ops->free)
+		pwm->chip->ops->free(pwm->chip, pwm);
+
 	pwm->label = NULL;
 
 	module_put(pwm->chip->ops->owner);
@@ -193,7 +338,7 @@ 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);
+	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
 }
 EXPORT_SYMBOL_GPL(pwm_config);
 
@@ -202,8 +347,8 @@ 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);
+	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags))
+		return pwm->chip->ops->enable(pwm->chip, pwm);
 
 	return 0;
 }
@@ -214,7 +359,72 @@ 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);
+	if (test_and_clear_bit(PWMF_ENABLED, &pwm->flags))
+		pwm->chip->ops->disable(pwm->chip, pwm);
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
+
+#ifdef CONFIG_DEBUG_FS
+static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
+{
+	unsigned int i;
+
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+
+		seq_printf(s, " pwm-%-3d (%-20.20s):", i, pwm->label);
+
+		if (test_bit(PWMF_REQUESTED, &pwm->flags))
+			seq_printf(s, " requested");
+
+		if (test_bit(PWMF_ENABLED, &pwm->flags))
+			seq_printf(s, " enabled");
+
+		seq_printf(s, "\n");
+	}
+}
+
+static int pwm_show(struct seq_file *s, void *unused)
+{
+	const char *prefix = "";
+	struct pwm_chip *chip;
+
+	list_for_each_entry(chip, &pwm_chips, list) {
+		struct device *dev = chip->dev;
+
+		seq_printf(s, "%s%s/%s, %d PWM devices\n", prefix,
+			   dev->bus ? dev->bus->name : "no-bus",
+			   dev_name(dev), chip->npwm);
+
+		if (chip->ops->dbg_show)
+			chip->ops->dbg_show(chip, s);
+		else
+			pwm_dbg_show(chip, s);
+
+		prefix = "\n";
+	}
+
+	return 0;
+}
+
+static int pwm_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pwm_show, NULL);
+}
+
+static const struct file_operations pwm_ops = {
+	.open = pwm_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int __init pwm_debugfs_init(void)
+{
+	debugfs_create_file("pwm", S_IFREG | S_IRUGO, NULL, NULL, &pwm_ops);
+
+	return 0;
+}
+
+subsys_initcall(pwm_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index df9681b..7261911 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -31,38 +31,90 @@ void pwm_disable(struct pwm_device *pwm);
 #ifdef CONFIG_PWM
 struct pwm_chip;
 
+enum {
+	PWMF_REQUESTED = 1 << 0,
+	PWMF_ENABLED = 1 << 1,
+};
+
+struct pwm_device {
+	const char		*label;
+	unsigned long		flags;
+	unsigned int		hwpwm;
+	unsigned int		pwm;
+	struct pwm_chip		*chip;
+	void			*chip_data;
+
+	unsigned int		period; /* in nanoseconds */
+};
+
+static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
+{
+	if (pwm)
+		pwm->period = period;
+}
+
+static inline unsigned int pwm_get_period(struct pwm_device *pwm)
+{
+	return pwm ? pwm->period : 0;
+}
+
 /**
- * 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
+ * @dbg_show: optional routine to show contents in debugfs
+ * @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 period_ns);
-	int			(*enable)(struct pwm_chip *chip);
-	void			(*disable)(struct pwm_chip *chip);
+	int			(*request)(struct pwm_chip *chip,
+					   struct pwm_device *pwm);
+	void			(*free)(struct pwm_chip *chip,
+					struct pwm_device *pwm);
+	int			(*config)(struct pwm_chip *chip,
+					  struct pwm_device *pwm,
+					  int duty_ns, int period_ns);
+	int			(*enable)(struct pwm_chip *chip,
+					  struct pwm_device *pwm);
+	void			(*disable)(struct pwm_chip *chip,
+					   struct pwm_device *pwm);
+#ifdef CONFIG_DEBUG_FS
+	void			(*dbg_show)(struct pwm_chip *chip,
+					    struct seq_file *s);
+#endif
 	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 list_head	list;
 	struct pwm_ops		*ops;
+	int			base;
+	unsigned int		npwm;
+
+	struct pwm_device	*pwms;
 };
 
+int pwm_set_chip_data(struct pwm_device *pwm, void *data);
+void *pwm_get_chip_data(struct pwm_device *pwm);
+
 int pwmchip_add(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
+struct pwm_device *pwm_request_from_device(struct device *dev,
+					   unsigned int index,
+					   const char *label);
+struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
+					 unsigned int index,
+					 const char *label);
 #endif
 
 #endif /* __LINUX_PWM_H */
-- 
1.7.9.4

RE: [PATCH v4 02/10] pwm: Allow chips to support multiple PWMs.

[H Hartley Sweeten]

On Wednesday, March 14, 2012 8:56 AM, Thierry Reding wrote:
> 
> Many PWM controllers provide access to more than a single PWM output and
> may even share some resource among them. Allowing a PWM chip to provide
> multiple PWM devices enables better sharing of those resources. As a
> side-effect this change allows easy integration with the device tree
> where a given PWM can be looked up based on the PWM chip's phandle and a
> corresponding index.
>
> 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 1024 while
> the data is actually stored in a radix tree, thus saving resources if
> not all of them are used.
>
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---

<snip>

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index df9681b..7261911 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h

<snip>

>  /**
> - * 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
> + * @dbg_show: optional routine to show contents in debugfs
> + * @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 period_ns);
> -	int			(*enable)(struct pwm_chip *chip);
> -	void			(*disable)(struct pwm_chip *chip);
> +	int			(*request)(struct pwm_chip *chip,
> +					   struct pwm_device *pwm);
> +	void			(*free)(struct pwm_chip *chip,
> +					struct pwm_device *pwm);
> +	int			(*config)(struct pwm_chip *chip,
> +					  struct pwm_device *pwm,
> +					  int duty_ns, int period_ns);
> +	int			(*enable)(struct pwm_chip *chip,
> +					  struct pwm_device *pwm);
> +	void			(*disable)(struct pwm_chip *chip,
> +					   struct pwm_device *pwm);
> +#ifdef CONFIG_DEBUG_FS
> +	void			(*dbg_show)(struct pwm_chip *chip,
> +					    struct seq_file *s);
> +#endif

This doesn't compile...

include/linux/pwm.h:87: warning: 'struct seq_file' declared inside parameter list
include/linux/pwm.h:87: warning: its scope is only this definition or declaration, which is probably not what you want

Regards,
Hartley

Re: [PATCH v4 02/10] pwm: Allow chips to support multiple PWMs.

[Thierry Reding]

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

* H Hartley Sweeten wrote:
> On Wednesday, March 14, 2012 8:56 AM, Thierry Reding wrote:
[...]
> > +#ifdef CONFIG_DEBUG_FS
> > +	void			(*dbg_show)(struct pwm_chip *chip,
> > +					    struct seq_file *s);
> > +#endif
> 
> This doesn't compile...
> 
> include/linux/pwm.h:87: warning: 'struct seq_file' declared inside parameter list
> include/linux/pwm.h:87: warning: its scope is only this definition or declaration, which is probably not what you want

Okay, pwm.h probably needs a seq_file.h include. Thanks for noticing.

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: [PATCH v4 02/10] pwm: Allow chips to support multiple PWMs.

[H Hartley Sweeten]

On Wednesday, March 14, 2012 1:50 PM, Thierry Reding wrote:
> * H Hartley Sweeten wrote:
>> On Wednesday, March 14, 2012 8:56 AM, Thierry Reding wrote:
> [...]
>>> +#ifdef CONFIG_DEBUG_FS
>>> +	void			(*dbg_show)(struct pwm_chip *chip,
>>> +					    struct seq_file *s);
>>> +#endif
>> 
>> This doesn't compile...
>> 
>> include/linux/pwm.h:87: warning: 'struct seq_file' declared inside parameter list
>> include/linux/pwm.h:87: warning: its scope is only this definition or declaration, which is probably not what you want
>
> Okay, pwm.h probably needs a seq_file.h include. Thanks for noticing.

Or just a:

struct seq_file;

Regards,
Hartley

[PATCH v4 03/10] pwm: Add device tree support

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

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v4:
  - add OF-specific code removed from patch 2
  - make of_node_to_pwmchip() and of_pwm_simple_xlate() static
  - rename of_get_named_pwm() to of_pwm_request(), return a struct
    pwm_device, get rid of the now unused spec parameter and use the
    device_node.name field as the PWM's name
  - return a requested struct pwm_device from pwm_chip.of_xlate and
    drop the now unused spec parameter
  - move OF support code into drivers/pwm/core.c
  - used deferred probing if a PWM chip is not available yet

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

 Documentation/devicetree/bindings/pwm/pwm.txt |   48 ++++++++++
 drivers/of/Kconfig                            |    6 ++
 drivers/pwm/core.c                            |  127 +++++++++++++++++++++++++
 include/linux/of_pwm.h                        |   36 +++++++
 include/linux/pwm.h                           |    8 ++
 5 files changed, 225 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm.txt
 create mode 100644 include/linux/of_pwm.h

diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
new file mode 100644
index 0000000..9421fe7
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -0,0 +1,48 @@
+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@7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 6ea51dc..d47b8ee 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -57,6 +57,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/pwm/core.c b/drivers/pwm/core.c
index 74dd295..c600606 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -20,6 +20,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/of_pwm.h>
 #include <linux/pwm.h>
 #include <linux/radix-tree.h>
 #include <linux/list.h>
@@ -121,6 +122,75 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	return 0;
 }
 
+#ifdef CONFIG_OF_PWM
+static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
+{
+	struct pwm_chip *chip;
+
+	mutex_lock(&pwm_lock);
+
+	list_for_each_entry(chip, &pwm_chips, list)
+		if (chip->dev && chip->dev->of_node == np) {
+			mutex_unlock(&pwm_lock);
+			return chip;
+		}
+
+	mutex_unlock(&pwm_lock);
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static struct pwm_device *of_pwm_simple_xlate(struct pwm_chip *pc,
+					      const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (pc->of_pwm_n_cells < 2)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args_count < pc->of_pwm_n_cells)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= pc->npwm)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_device(pc->dev, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return ERR_PTR(-ENODEV);
+
+	pwm_set_period(pwm, args->args[1]);
+
+	return pwm;
+}
+
+void of_pwmchip_add(struct pwm_chip *chip)
+{
+	if (!chip->dev || !chip->dev->of_node)
+		return;
+
+	if (!chip->of_xlate) {
+		chip->of_xlate = of_pwm_simple_xlate;
+		chip->of_pwm_n_cells = 2;
+	}
+
+	of_node_get(chip->dev->of_node);
+}
+
+void of_pwmchip_remove(struct pwm_chip *chip)
+{
+	if (chip->dev && chip->dev->of_node)
+		of_node_put(chip->dev->of_node);
+}
+#else
+static inline void of_pwmchip_add(struct pwm_chip *pc)
+{
+}
+
+static inline void of_pwmchip_remove(struct pwm_chip *pc)
+{
+}
+#endif /* CONFIG_OF_PWM */
+
 /**
  * pwm_set_chip_data - set private chip data for a PWM
  * @pwm: PWM device
@@ -184,6 +254,7 @@ int pwmchip_add(struct pwm_chip *chip)
 
 	INIT_LIST_HEAD(&chip->list);
 	list_add(&chip->list, &pwm_chips);
+	of_pwmchip_add(chip);
 
 out:
 	mutex_unlock(&pwm_lock);
@@ -215,6 +286,7 @@ int pwmchip_remove(struct pwm_chip *chip)
 	}
 
 	list_del_init(&chip->list);
+	of_pwmchip_remove(chip);
 	free_pwms(chip);
 
 out:
@@ -364,6 +436,61 @@ void pwm_disable(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
 
+#ifdef CONFIG_OF
+/**
+ * of_pwm_request() - request a PWM via the PWM framework
+ * @np: device node to get the PWM from
+ * @propname: property name containing PWM specifier(s)
+ * @index: index of the PWM
+ *
+ * Returns the PWM device parsed from the phandle specified in the given
+ * property of a device tree node or NULL on failure. Values parsed from
+ * the device tree are stored in the returned PWM device object.
+ */
+struct pwm_device *of_pwm_request(struct device_node *np,
+				  const char *propname, int index)
+{
+	struct pwm_device *pwm = NULL;
+	struct of_phandle_args args;
+	struct pwm_chip *pc;
+	int err;
+
+	err = of_parse_phandle_with_args(np, propname, "#pwm-cells", index,
+					 &args);
+	if (err) {
+		pr_debug("%s(): can't parse property \"%s\"\n", __func__,
+			 propname);
+		return ERR_PTR(err);
+	}
+
+	pc = of_node_to_pwmchip(args.np);
+	if (IS_ERR(pc)) {
+		pr_debug("%s(): PWM chip not found\n", __func__);
+		pwm = ERR_CAST(pc);
+		goto put;
+	}
+
+	if (args.args_count != pc->of_pwm_n_cells) {
+		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
+			 args.np->full_name);
+		pwm = ERR_PTR(-EINVAL);
+		goto put;
+	}
+
+	pwm = pc->of_xlate(pc, &args);
+	if (IS_ERR(pwm))
+		goto put;
+
+	pwm->label = np->name;
+
+put:
+	of_node_put(args.np);
+
+	return pwm;
+}
+EXPORT_SYMBOL(of_pwm_request);
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 {
diff --git a/include/linux/of_pwm.h b/include/linux/of_pwm.h
new file mode 100644
index 0000000..a3a3da7
--- /dev/null
+++ b/include/linux/of_pwm.h
@@ -0,0 +1,36 @@
+/*
+ * OF helpers for the PWM API
+ *
+ * Copyright (c) 2011-2012 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/err.h>
+#include <linux/pwm.h>
+
+struct device_node;
+
+#ifdef CONFIG_OF_PWM
+
+struct pwm_device *of_pwm_request(struct device_node *np,
+				  const char *propname, int index);
+
+#else
+
+static inline struct pwm_device *of_pwm_request(struct device_node *np,
+						const char *propname,
+						int index);
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+#endif /* CONFIG_OF_PWM */
+
+#endif /* __LINUX_OF_PWM_H */
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 7261911..7b42336 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -1,6 +1,8 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/of.h>
+
 struct pwm_device;
 
 /*
@@ -102,6 +104,12 @@ struct pwm_chip {
 	unsigned int		npwm;
 
 	struct pwm_device	*pwms;
+
+#ifdef CONFIG_OF_PWM
+	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
+					    const struct of_phandle_args *args);
+	unsigned int		of_pwm_n_cells;
+#endif
 };
 
 int pwm_set_chip_data(struct pwm_device *pwm, void *data);
-- 
1.7.9.4

Re: [PATCH v4 03/10] pwm: Add device tree support

[Sascha Hauer]

On Wed, Mar 14, 2012 at 04:56:26PM +0100, Thierry Reding wrote:
> This patch adds helpers to support device tree bindings for the generic
> PWM API. Device tree binding documentation for PWM controllers is also
> provided.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
> Changes in v4:
>   - add OF-specific code removed from patch 2
>   - make of_node_to_pwmchip() and of_pwm_simple_xlate() static
>   - rename of_get_named_pwm() to of_pwm_request(), return a struct
>     pwm_device, get rid of the now unused spec parameter and use the
>     device_node.name field as the PWM's name
>   - return a requested struct pwm_device from pwm_chip.of_xlate and
>     drop the now unused spec parameter
>   - move OF support code into drivers/pwm/core.c
>   - used deferred probing if a PWM chip is not available yet
> 
> Changes in v2:
>   - add device tree binding documentation
>   - add of_xlate to parse controller-specific PWM-specifier
> 
>  Documentation/devicetree/bindings/pwm/pwm.txt |   48 ++++++++++
>  drivers/of/Kconfig                            |    6 ++
>  drivers/pwm/core.c                            |  127 +++++++++++++++++++++++++
>  include/linux/of_pwm.h                        |   36 +++++++
>  include/linux/pwm.h                           |    8 ++
>  5 files changed, 225 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm.txt
>  create mode 100644 include/linux/of_pwm.h
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
> new file mode 100644
> index 0000000..9421fe7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> @@ -0,0 +1,48 @@
> +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@7000a000 {
> +		compatible = "nvidia,tegra20-pwm";
> +		reg = <0x7000a000 0x100>;
> +		#pwm-cells = <2>;
> +	};
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 6ea51dc..d47b8ee 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -57,6 +57,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/pwm/core.c b/drivers/pwm/core.c
> index 74dd295..c600606 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/of_pwm.h>
>  #include <linux/pwm.h>
>  #include <linux/radix-tree.h>
>  #include <linux/list.h>
> @@ -121,6 +122,75 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF_PWM
> +static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> +{
> +	struct pwm_chip *chip;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	list_for_each_entry(chip, &pwm_chips, list)
> +		if (chip->dev && chip->dev->of_node == np) {
> +			mutex_unlock(&pwm_lock);
> +			return chip;
> +		}
> +
> +	mutex_unlock(&pwm_lock);
> +
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +
> +static struct pwm_device *of_pwm_simple_xlate(struct pwm_chip *pc,
> +					      const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	if (pc->of_pwm_n_cells < 2)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (args->args_count < pc->of_pwm_n_cells)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (args->args[0] >= pc->npwm)
> +		return ERR_PTR(-EINVAL);
> +
> +	pwm = pwm_request_from_device(pc->dev, args->args[0], NULL);
> +	if (IS_ERR(pwm))
> +		return ERR_PTR(-ENODEV);
> +
> +	pwm_set_period(pwm, args->args[1]);
> +
> +	return pwm;
> +}
> +
> +void of_pwmchip_add(struct pwm_chip *chip)
> +{
> +	if (!chip->dev || !chip->dev->of_node)
> +		return;
> +
> +	if (!chip->of_xlate) {
> +		chip->of_xlate = of_pwm_simple_xlate;
> +		chip->of_pwm_n_cells = 2;
> +	}
> +
> +	of_node_get(chip->dev->of_node);
> +}
> +
> +void of_pwmchip_remove(struct pwm_chip *chip)
> +{
> +	if (chip->dev && chip->dev->of_node)
> +		of_node_put(chip->dev->of_node);
> +}
> +#else
> +static inline void of_pwmchip_add(struct pwm_chip *pc)
> +{
> +}
> +
> +static inline void of_pwmchip_remove(struct pwm_chip *pc)
> +{
> +}
> +#endif /* CONFIG_OF_PWM */
> +
>  /**
>   * pwm_set_chip_data - set private chip data for a PWM
>   * @pwm: PWM device
> @@ -184,6 +254,7 @@ int pwmchip_add(struct pwm_chip *chip)
>  
>  	INIT_LIST_HEAD(&chip->list);
>  	list_add(&chip->list, &pwm_chips);
> +	of_pwmchip_add(chip);
>  
>  out:
>  	mutex_unlock(&pwm_lock);
> @@ -215,6 +286,7 @@ int pwmchip_remove(struct pwm_chip *chip)
>  	}
>  
>  	list_del_init(&chip->list);
> +	of_pwmchip_remove(chip);
>  	free_pwms(chip);
>  
>  out:
> @@ -364,6 +436,61 @@ void pwm_disable(struct pwm_device *pwm)
>  }
>  EXPORT_SYMBOL_GPL(pwm_disable);
>  
> +#ifdef CONFIG_OF
> +/**
> + * of_pwm_request() - request a PWM via the PWM framework
> + * @np: device node to get the PWM from
> + * @propname: property name containing PWM specifier(s)
> + * @index: index of the PWM
> + *
> + * Returns the PWM device parsed from the phandle specified in the given
> + * property of a device tree node or NULL on failure. Values parsed from
> + * the device tree are stored in the returned PWM device object.
> + */
> +struct pwm_device *of_pwm_request(struct device_node *np,
> +				  const char *propname, int index)
> +{
> +	struct pwm_device *pwm = NULL;
> +	struct of_phandle_args args;
> +	struct pwm_chip *pc;
> +	int err;
> +
> +	err = of_parse_phandle_with_args(np, propname, "#pwm-cells", index,
> +					 &args);
> +	if (err) {
> +		pr_debug("%s(): can't parse property \"%s\"\n", __func__,
> +			 propname);
> +		return ERR_PTR(err);
> +	}
> +
> +	pc = of_node_to_pwmchip(args.np);
> +	if (IS_ERR(pc)) {
> +		pr_debug("%s(): PWM chip not found\n", __func__);
> +		pwm = ERR_CAST(pc);
> +		goto put;
> +	}
> +
> +	if (args.args_count != pc->of_pwm_n_cells) {
> +		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
> +			 args.np->full_name);
> +		pwm = ERR_PTR(-EINVAL);
> +		goto put;
> +	}
> +
> +	pwm = pc->of_xlate(pc, &args);
> +	if (IS_ERR(pwm))
> +		goto put;
> +
> +	pwm->label = np->name;
> +
> +put:
> +	of_node_put(args.np);
> +
> +	return pwm;
> +}
> +EXPORT_SYMBOL(of_pwm_request);
> +#endif
> +
>  #ifdef CONFIG_DEBUG_FS
>  static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  {
> diff --git a/include/linux/of_pwm.h b/include/linux/of_pwm.h
> new file mode 100644
> index 0000000..a3a3da7
> --- /dev/null
> +++ b/include/linux/of_pwm.h
> @@ -0,0 +1,36 @@
> +/*
> + * OF helpers for the PWM API
> + *
> + * Copyright (c) 2011-2012 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/err.h>
> +#include <linux/pwm.h>
> +
> +struct device_node;
> +
> +#ifdef CONFIG_OF_PWM
> +
> +struct pwm_device *of_pwm_request(struct device_node *np,
> +				  const char *propname, int index);
> +
> +#else
> +
> +static inline struct pwm_device *of_pwm_request(struct device_node *np,
> +						const char *propname,
> +						int index);
                                                         ^^^

No semicolon here please.

Sascha


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

Re: [PATCH v4 03/10] pwm: Add device tree support

[Thierry Reding]

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

* Sascha Hauer wrote:
> On Wed, Mar 14, 2012 at 04:56:26PM +0100, Thierry Reding wrote:
> > +static inline struct pwm_device *of_pwm_request(struct device_node *np,
> > +						const char *propname,
> > +						int index);
>                                                          ^^^
> 
> No semicolon here please.

Ugh. I've been doing extra compile tests to make sure the series stays
bisectible, but I guess I need to add further tests with !PWM and !OF
configurations.

Thanks,
Thierry

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

Re: [PATCH v4 03/10] pwm: Add device tree support

[Arnd Bergmann]

On Wednesday 14 March 2012, Thierry Reding wrote:
> This patch adds helpers to support device tree bindings for the generic
> PWM API. Device tree binding documentation for PWM controllers is also
> provided.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
> Changes in v4:
>   - add OF-specific code removed from patch 2
>   - make of_node_to_pwmchip() and of_pwm_simple_xlate() static
>   - rename of_get_named_pwm() to of_pwm_request(), return a struct
>     pwm_device, get rid of the now unused spec parameter and use the
>     device_node.name field as the PWM's name
>   - return a requested struct pwm_device from pwm_chip.of_xlate and
>     drop the now unused spec parameter
>   - move OF support code into drivers/pwm/core.c
>   - used deferred probing if a PWM chip is not available yet
> 
> Changes in v2:
>   - add device tree binding documentation
>   - add of_xlate to parse controller-specific PWM-specifier

Looks very cool now, and much simpler!

> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 6ea51dc..d47b8ee 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -57,6 +57,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

You might want to remove this one now and just use CONFIG_OF in the driver,
unless there is  a reason to keep it here.

> +#ifdef CONFIG_OF_PWM
> +static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> +{
> +	struct pwm_chip *chip;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	list_for_each_entry(chip, &pwm_chips, list)
> +		if (chip->dev && chip->dev->of_node == np) {
> +			mutex_unlock(&pwm_lock);
> +			return chip;
> +		}
> +
> +	mutex_unlock(&pwm_lock);
> +
> +	return ERR_PTR(-EPROBE_DEFER);
> +}

EPROBE_DEFER doesn't exist yet in my kernel, and I wonder if it's actually
safe to be used this way, because it can result in arbitrary drivers using
this to be put on the defered probe list. It certainly sounds like the
right thing to do in the long run though.

> +#else
> +static inline void of_pwmchip_add(struct pwm_chip *pc)
> +{
> +}
> +
> +static inline void of_pwmchip_remove(struct pwm_chip *pc)
> +{
> +}
> +#endif /* CONFIG_OF_PWM */
> +
>  /**
>   * pwm_set_chip_data - set private chip data for a PWM
>   * @pwm: PWM device
> @@ -184,6 +254,7 @@ int pwmchip_add(struct pwm_chip *chip)
>  
>  	INIT_LIST_HEAD(&chip->list);
>  	list_add(&chip->list, &pwm_chips);
> +	of_pwmchip_add(chip);
>  
>  out:
>  	mutex_unlock(&pwm_lock);

You could express the same thing using 

	if (IS_ENABLED(CONFIG_OF_PWM))
		of_pwmchip_add(chip);

The advantage of this is that you always get compile coverage for the
entire file, but the function is automatically discarded by the
compiler when the condition is false at compile time. That obviously
doesn't work for exported functions.

> +#ifdef CONFIG_OF
> +/**
> + * of_pwm_request() - request a PWM via the PWM framework
> + * @np: device node to get the PWM from
> + * @propname: property name containing PWM specifier(s)
> + * @index: index of the PWM
> + *
> + * Returns the PWM device parsed from the phandle specified in the given
> + * property of a device tree node or NULL on failure. Values parsed from
> + * the device tree are stored in the returned PWM device object.
> + */
> +struct pwm_device *of_pwm_request(struct device_node *np,
> +				  const char *propname, int index)
> +{
> +	struct pwm_device *pwm = NULL;
> +	struct of_phandle_args args;
> +	struct pwm_chip *pc;
> +	int err;
> +
> +	err = of_parse_phandle_with_args(np, propname, "#pwm-cells", index,
> +					 &args);

Wow, I just looked up what of_parse_phandle_with_args() does and that is
extremely helpful indeed.

>  /*
> @@ -102,6 +104,12 @@ struct pwm_chip {
>  	unsigned int		npwm;
>  
>  	struct pwm_device	*pwms;
> +
> +#ifdef CONFIG_OF_PWM
> +	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
> +					    const struct of_phandle_args *args);
> +	unsigned int		of_pwm_n_cells;
> +#endif
>  };
>  
>  int pwm_set_chip_data(struct pwm_device *pwm, void *data);

If you use IS_ENABLED() as I suggested above, this #ifdef will have to go
away, too, which is a very small size overhead.

	Arnd

Re: [PATCH v4 03/10] pwm: Add device tree support

[Mark Brown]

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

On Thu, Mar 15, 2012 at 08:40:42AM +0000, Arnd Bergmann wrote:
> On Wednesday 14 March 2012, Thierry Reding wrote:

> > +static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> > +{

> > +	return ERR_PTR(-EPROBE_DEFER);
> > +}

> EPROBE_DEFER doesn't exist yet in my kernel, and I wonder if it's actually
> safe to be used this way, because it can result in arbitrary drivers using
> this to be put on the defered probe list. It certainly sounds like the
> right thing to do in the long run though.

Similar code is going in for regulators in 3.4 along with the core
-EPROBE_DEFER change (though not OF specific) and I sent a similar patch
for GPIOs too, hopefully Grant will ack it in time for it to make it in.

My theory is that since you need to explicitly know that the thing
you're requesting is there in order to request it (eg, have a PWM number
or DT link) the overwhemlingly common case for a failure to request will
be that the provider didn't register yet which is exactly the case where
deferral is desired.  It therefore seems sensible to have the framework
default the drivers to retrying rather than have almost every individual
driver look at the failure, figure out if it was a missing provider, and
then retry.  Drivers that have a good reason to fail can always check
for -EPROBE_DEFER and override it.

The result should be that we can take advantage of probe deferral over
large areas of the kernel without having to go and explicitly modify so
many drivers - if the frameworks like GPIO, clk and regulator can do
this that ought to cover 90% of the cases where probe deferral will be
needed without having to do anything more than have good error handling
paths on probe which is a good idea anyway.

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

Re: [PATCH v4 03/10] pwm: Add device tree support

[Arnd Bergmann]

On Thursday 15 March 2012, Mark Brown wrote:
> Similar code is going in for regulators in 3.4 along with the core
> -EPROBE_DEFER change (though not OF specific) and I sent a similar patch
> for GPIOs too, hopefully Grant will ack it in time for it to make it in.
> 
> My theory is that since you need to explicitly know that the thing
> you're requesting is there in order to request it (eg, have a PWM number
> or DT link) the overwhemlingly common case for a failure to request will
> be that the provider didn't register yet which is exactly the case where
> deferral is desired.  It therefore seems sensible to have the framework
> default the drivers to retrying rather than have almost every individual
> driver look at the failure, figure out if it was a missing provider, and
> then retry.  Drivers that have a good reason to fail can always check
> for -EPROBE_DEFER and override it.
> 
> The result should be that we can take advantage of probe deferral over
> large areas of the kernel without having to go and explicitly modify so
> many drivers - if the frameworks like GPIO, clk and regulator can do
> this that ought to cover 90% of the cases where probe deferral will be
> needed without having to do anything more than have good error handling
> paths on probe which is a good idea anyway.

Ok, makes sense.

Thanks,
	
	Arnd

Re: [PATCH v4 03/10] pwm: Add device tree support

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> This patch adds helpers to support device tree bindings for the generic
> PWM API. Device tree binding documentation for PWM controllers is also
> provided.
...
> +static struct pwm_device *of_pwm_simple_xlate(struct pwm_chip *pc,
> +					      const struct of_phandle_args *args)
...
> +	if (args->args_count < pc->of_pwm_n_cells)
> +		return ERR_PTR(-EINVAL);

I think you can drop that error-check given the code quoted below?

(and if not, shouldn't it be != not >= ?)

> +struct pwm_device *of_pwm_request(struct device_node *np,
> +				  const char *propname, int index)
...
> +	if (args.args_count != pc->of_pwm_n_cells) {
> +		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
> +			 args.np->full_name);
> +		pwm = ERR_PTR(-EINVAL);
> +		goto put;
> +	}
> +
> +	pwm = pc->of_xlate(pc, &args);

Re: [PATCH v4 03/10] pwm: Add device tree support

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> > This patch adds helpers to support device tree bindings for the generic
> > PWM API. Device tree binding documentation for PWM controllers is also
> > provided.
> ...
> > +static struct pwm_device *of_pwm_simple_xlate(struct pwm_chip *pc,
> > +					      const struct of_phandle_args *args)
> ...
> > +	if (args->args_count < pc->of_pwm_n_cells)
> > +		return ERR_PTR(-EINVAL);
> 
> I think you can drop that error-check given the code quoted below?
> 
> (and if not, shouldn't it be != not >= ?)
> 
> > +struct pwm_device *of_pwm_request(struct device_node *np,
> > +				  const char *propname, int index)
> ...
> > +	if (args.args_count != pc->of_pwm_n_cells) {
> > +		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
> > +			 args.np->full_name);
> > +		pwm = ERR_PTR(-EINVAL);
> > +		goto put;
> > +	}
> > +
> > +	pwm = pc->of_xlate(pc, &args);

Yes, you're right. It is completely redundant.

Thierry

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

[PATCH v4 04/10] 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).

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Signed-off-by: Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v4:
  - reuse PWM flag from Tegra 30 clock programming
  - mask out bit 31 explicitly

 arch/arm/mach-tegra/tegra2_clocks.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c
index 592a4ee..64735bd 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_PWM_MASK	(7<<28)
+#define PERIPH_CLK_SOURCE_PWM_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,20 @@ 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;
+	u32 mask;
+
+	if (c->flags & MUX_PWM) {
+		shift = PERIPH_CLK_SOURCE_PWM_SHIFT;
+		mask = PERIPH_CLK_SOURCE_PWM_MASK;
+	} else {
+		shift = PERIPH_CLK_SOURCE_SHIFT;
+		mask = PERIPH_CLK_SOURCE_MASK;
+	}
+
 	if (c->flags & MUX) {
 		for (sel = c->inputs; sel->input != NULL; sel++) {
-			if (val >> PERIPH_CLK_SOURCE_SHIFT == sel->value)
+			if ((val & mask) >> shift == sel->value)
 				mux = sel;
 		}
 		BUG_ON(!mux);
@@ -1023,12 +1036,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 & MUX_PWM) {
+		shift = PERIPH_CLK_SOURCE_PWM_SHIFT;
+		mask = PERIPH_CLK_SOURCE_PWM_MASK;
+	} else {
+		shift = PERIPH_CLK_SOURCE_SHIFT;
+		mask = PERIPH_CLK_SOURCE_MASK;
+	}
+
 	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);
@@ -2146,7 +2170,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 | MUX_PWM),
 	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.9.4

Re: [PATCH v4 04/10] ARM: tegra: Fix PWM clock programming

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> 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).
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Signed-off-by: Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Simon Que <sque-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

[PATCH v4 05/10] ARM: tegra: Provide clock for only one PWM controller

[Thierry Reding]

A subsequent patch will add a generic PWM API driver for the Tegra 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>
---
 arch/arm/mach-tegra/tegra2_clocks.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c
index 64735bd..efbe2ed 100644
--- a/arch/arm/mach-tegra/tegra2_clocks.c
+++ b/arch/arm/mach-tegra/tegra2_clocks.c
@@ -2269,10 +2269,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.9.4

Re: [PATCH v4 05/10] ARM: tegra: Provide clock for only one PWM controller

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> A subsequent patch will add a generic PWM API driver for the Tegra 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>
...
> -	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),

I thought harder about this: Why not delete /all/ the CLK_DUPLICATE()s
for PWM, and change the second parameter to PERIPH_CLK for PWM from
"pwm" to "tegra-pwm"?

Re: [PATCH v4 05/10] ARM: tegra: Provide clock for only one PWM controller

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> > A subsequent patch will add a generic PWM API driver for the Tegra 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>
> ...
> > -	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),
> 
> I thought harder about this: Why not delete /all/ the CLK_DUPLICATE()s
> for PWM, and change the second parameter to PERIPH_CLK for PWM from
> "pwm" to "tegra-pwm"?

I think that should work. I'll retest and will update the patch accordingly.
I assume that you're Acked-by will remain valid after those changes?

Thierry

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

Re: [PATCH v4 05/10] ARM: tegra: Provide clock for only one PWM controller

[Stephen Warren]

On 03/20/2012 02:44 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/14/2012 09:56 AM, Thierry Reding wrote:
>>> A subsequent patch will add a generic PWM API driver for the Tegra 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>
>> ...
>>> -	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),
>>
>> I thought harder about this: Why not delete /all/ the CLK_DUPLICATE()s
>> for PWM, and change the second parameter to PERIPH_CLK for PWM from
>> "pwm" to "tegra-pwm"?
> 
> I think that should work. I'll retest and will update the patch accordingly.
> I assume that you're Acked-by will remain valid after those changes?

Yes in this case, assuming the patch looks like what I think it will:-)

[PATCH v4 06/10] pwm: Add NVIDIA Tegra SoC support

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

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v4:
  - merge patch from ChromiumOS kernel to fix a rounding issue
  - move DT code to patch 7

Changes in v3:
  - use pwm_device.hwpwm instead of recomputing it
  - update pwm_ops for changes in patch 2

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 |  256 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 267 insertions(+)
 create mode 100644 drivers/pwm/pwm-tegra.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 93c1052..bda6f23 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
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..19540fc
--- /dev/null
+++ b/drivers/pwm/pwm-tegra.c
@@ -0,0 +1,256 @@
+/*
+ * 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
+
+#define NUM_PWM 4
+
+struct tegra_pwm_chip {
+	struct pwm_chip		chip;
+	struct device		*dev;
+
+	struct clk		*clk;
+
+	int			enable[NUM_PWM];
+	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 num,
+		unsigned long val)
+{
+	unsigned long offset = num << 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, struct pwm_device *pwm,
+		int duty_ns, int period_ns)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+	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 and make sure to round to the
+	 * nearest integer during division.
+	 */
+	c = duty_ns * ((1 << PWM_DUTY_WIDTH) - 1) + period_ns / 2;
+	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);
+
+	/* If the PWM channel is enabled, keep it enabled */
+	if (pc->enable[pwm->hwpwm])
+		val |= PWM_ENABLE;
+
+	return pwm_writel(pc, pwm->hwpwm, val);
+}
+
+static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+	int rc = 0;
+
+	if (!pc->enable[pwm->hwpwm]) {
+		rc = clk_enable(pc->clk);
+		if (!rc) {
+			unsigned long offset = pwm->hwpwm << 4;
+			u32 val;
+
+			val = readl(pc->mmio_base + offset);
+			val |= PWM_ENABLE;
+			writel(val, pc->mmio_base + offset);
+
+			pc->enable[pwm->hwpwm] = 1;
+		}
+	}
+
+	return 0;
+}
+
+static void tegra_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+
+	if (pc->enable[pwm->hwpwm]) {
+		unsigned long offset = pwm->hwpwm << 4;
+		u32 val;
+
+		val = readl(pc->mmio_base + offset);
+		val &= ~PWM_ENABLE;
+		writel(val, pc->mmio_base + offset);
+
+		clk_disable(pc->clk);
+		pc->enable[pwm->hwpwm] = 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.dev = &pdev->dev;
+	pwm->chip.ops = &tegra_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = NUM_PWM;
+
+	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;
+
+	pwmchip_remove(&pwm->chip);
+
+	for (i = 0; i < NUM_PWM; i++) {
+		pwm_writel(pwm, i, 0);
+
+		if (pwm->enable[i])
+			clk_disable(pwm->clk);
+	}
+
+	clk_put(pwm->clk);
+
+	return 0;
+}
+
+static struct platform_driver tegra_pwm_driver = {
+	.driver = {
+		.name = "tegra-pwm",
+	},
+	.probe = tegra_pwm_probe,
+	.remove = __devexit_p(tegra_pwm_remove),
+};
+
+module_platform_driver(tegra_pwm_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("NVIDIA Corporation");
-- 
1.7.9.4

Re: [PATCH v4 06/10] pwm: Add NVIDIA Tegra SoC support

[Shawn Guo]

On Wed, Mar 14, 2012 at 04:56:29PM +0100, Thierry Reding wrote:
> 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).
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
> Changes in v4:
>   - merge patch from ChromiumOS kernel to fix a rounding issue
>   - move DT code to patch 7
> 
> Changes in v3:
>   - use pwm_device.hwpwm instead of recomputing it
>   - update pwm_ops for changes in patch 2
> 
> 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 |  256 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 267 insertions(+)
>  create mode 100644 drivers/pwm/pwm-tegra.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 93c1052..bda6f23 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -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
> 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..19540fc
> --- /dev/null
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -0,0 +1,256 @@
> +/*
> + * 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
> +
> +#define NUM_PWM 4
> +
> +struct tegra_pwm_chip {
> +	struct pwm_chip		chip;
> +	struct device		*dev;
> +
> +	struct clk		*clk;
> +
> +	int			enable[NUM_PWM];
> +	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 num,
> +		unsigned long val)
> +{
> +	unsigned long offset = num << 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, struct pwm_device *pwm,
> +		int duty_ns, int period_ns)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	unsigned long long c;
> +	unsigned long rate, hz;
> +	u32 val = 0;
> +

Every pwm driver I have been looking at has some validation on
parameters here, something like:

	if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
		return -EINVAL;

It's not needed for tegra, or the check on those platforms is
unnecessary?

> +	/*
> +	 * Convert from duty_ns / period_ns to a fixed number of duty ticks
> +	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
> +	 * nearest integer during division.
> +	 */
> +	c = duty_ns * ((1 << PWM_DUTY_WIDTH) - 1) + period_ns / 2;
> +	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);
> +
> +	/* If the PWM channel is enabled, keep it enabled */
> +	if (pc->enable[pwm->hwpwm])
> +		val |= PWM_ENABLE;
> +
> +	return pwm_writel(pc, pwm->hwpwm, val);
> +}
> +
> +static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	int rc = 0;
> +
> +	if (!pc->enable[pwm->hwpwm]) {
> +		rc = clk_enable(pc->clk);
> +		if (!rc) {
> +			unsigned long offset = pwm->hwpwm << 4;
> +			u32 val;
> +
> +			val = readl(pc->mmio_base + offset);
> +			val |= PWM_ENABLE;
> +			writel(val, pc->mmio_base + offset);
> +
> +			pc->enable[pwm->hwpwm] = 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void tegra_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +
> +	if (pc->enable[pwm->hwpwm]) {
> +		unsigned long offset = pwm->hwpwm << 4;
> +		u32 val;
> +
> +		val = readl(pc->mmio_base + offset);
> +		val &= ~PWM_ENABLE;
> +		writel(val, pc->mmio_base + offset);
> +
> +		clk_disable(pc->clk);
> +		pc->enable[pwm->hwpwm] = 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;
> +	}
> +

The helper devm_request_and_ioremap() can help here?

> +	platform_set_drvdata(pdev, pwm);
> +
> +	pwm->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return PTR_ERR(pwm->clk);
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &tegra_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = NUM_PWM;
> +
> +	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;
> +
> +	pwmchip_remove(&pwm->chip);
> +
> +	for (i = 0; i < NUM_PWM; i++) {
> +		pwm_writel(pwm, i, 0);
> +
> +		if (pwm->enable[i])
> +			clk_disable(pwm->clk);
> +	}
> +
> +	clk_put(pwm->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tegra_pwm_driver = {
> +	.driver = {
> +		.name = "tegra-pwm",
> +	},
> +	.probe = tegra_pwm_probe,
> +	.remove = __devexit_p(tegra_pwm_remove),
> +};
> +
I would remove this blank line.

> +module_platform_driver(tegra_pwm_driver);

Regards,
Shawn

> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("NVIDIA Corporation");
> -- 
> 1.7.9.4
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v4 06/10] pwm: Add NVIDIA Tegra SoC support

[Thierry Reding]

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

* Shawn Guo wrote:
> On Wed, Mar 14, 2012 at 04:56:29PM +0100, Thierry Reding wrote:
> > 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).
> > 
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> > ---
> > Changes in v4:
> >   - merge patch from ChromiumOS kernel to fix a rounding issue
> >   - move DT code to patch 7
> > 
> > Changes in v3:
> >   - use pwm_device.hwpwm instead of recomputing it
> >   - update pwm_ops for changes in patch 2
> > 
> > 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 |  256 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 267 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-tegra.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 93c1052..bda6f23 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -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
> > 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..19540fc
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -0,0 +1,256 @@
> > +/*
> > + * 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@pengutronix.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 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
> > +
> > +#define NUM_PWM 4
> > +
> > +struct tegra_pwm_chip {
> > +	struct pwm_chip		chip;
> > +	struct device		*dev;
> > +
> > +	struct clk		*clk;
> > +
> > +	int			enable[NUM_PWM];
> > +	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 num,
> > +		unsigned long val)
> > +{
> > +	unsigned long offset = num << 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, struct pwm_device *pwm,
> > +		int duty_ns, int period_ns)
> > +{
> > +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> > +	unsigned long long c;
> > +	unsigned long rate, hz;
> > +	u32 val = 0;
> > +
> 
> Every pwm driver I have been looking at has some validation on
> parameters here, something like:
> 
> 	if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
> 		return -EINVAL;
> 
> It's not needed for tegra, or the check on those platforms is
> unnecessary?

Yes, I should add similar checks for the Tegra driver. On the other hand
maybe the checks should be pushed into the core. At least those checks that
are truly general sanity checks. Off the top of my head, I can think of the
following general preconditions:

	pwm != NULL
	period_ns > 0
	duty_ns >= 0
	duty_ns <= period_ns

Of course duty_ns >= 0 could be done away with by just making the duty_ns and
period_ns parameters unsigned. But such changes were actually supposed to be
added incrementally once the framework had been merged.

> > +	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;
> > +	}
> > +
> 
> The helper devm_request_and_ioremap() can help here?

Yes, absolutely.

> > +static struct platform_driver tegra_pwm_driver = {
> > +	.driver = {
> > +		.name = "tegra-pwm",
> > +	},
> > +	.probe = tegra_pwm_probe,
> > +	.remove = __devexit_p(tegra_pwm_remove),
> > +};
> > +
> I would remove this blank line.
> 
> > +module_platform_driver(tegra_pwm_driver);

I don't know; I think it makes the code cluttered.

Thierry

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

Re: [PATCH v4 06/10] pwm: Add NVIDIA Tegra SoC support

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> 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)
...
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
...
> +static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	int rc = 0;
> +
> +	if (!pc->enable[pwm->hwpwm]) {

IIRC, the new PWM core only calls the enable() op for a disabled ->
enabled transition, so this driver probably doesn't need to check the
same thing, and you can get rid of tegra_pwm_chip.enable[] and have
tegra_pwm_config() read the enable flag from the core pwm device instead.

> +		rc = clk_enable(pc->clk);
> +		if (!rc) {
> +			unsigned long offset = pwm->hwpwm << 4;
> +			u32 val;
> +
> +			val = readl(pc->mmio_base + offset);
> +			val |= PWM_ENABLE;
> +			writel(val, pc->mmio_base + offset);

It seems a little of for the driver to define a pwm_writel() function
but only use it in some places but not others. I guess it's because in
some places the code knows the clock is on, so doesn't need the
clk_enable()/disable() helper in pwm_writel(), but I'd tend towards
always using pwm_readl()/pwm_writel() throughout the driver, and lifting
the clock management out of those low-level functions myself.

> +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;
> +
> +	pwmchip_remove(&pwm->chip);
> +
> +	for (i = 0; i < NUM_PWM; i++) {
> +		pwm_writel(pwm, i, 0);
> +
> +		if (pwm->enable[i])
> +			clk_disable(pwm->clk);

Should the core call the disable() op before the remove() op so drivers
don't need to check this? I'm a little in two minds about this; if this
decision is deferred to drivers (as the code above assumes), then I
suppose the whole remove() path might be marginally more efficient.

[PATCH v4 07/10] pwm: tegra: Add device tree support

[Thierry Reding]

Add auxdata to instantiate the PWFM controller from a device tree,
include the corresponding nodes in the dtsi files for Tegra 20 and
Tegra 30 and add binding documentation.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v4:
  - add DT binding documentation
  - allow nvidia,tegra20-pwm as fallback for nvidia,tegra30-pwm
  - export OF device table (for module auto-loading)

Changes in v3:
  - add missing include for mach/iomap.h

 .../devicetree/bindings/pwm/tegra-pwm.txt          |   18 ++++++++++++++++++
 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             |    3 +++
 drivers/pwm/pwm-tegra.c                            |   11 +++++++++++
 6 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/tegra-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/tegra-pwm.txt b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
new file mode 100644
index 0000000..bbbeedb
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
@@ -0,0 +1,18 @@
+Tegra SoC PWFM controller
+
+Required properties:
+- compatible: should be one of:
+  - "nvidia,tegra20-pwm"
+  - "nvidia,tegra30-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: On Tegra the number of cells used to specify a PWM is 2. The
+  first cell specifies the per-chip index of the PWM to use and the second
+  cell is the duty cycle in nanoseconds.
+
+Example:
+
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 108e894..55b28fd 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -154,6 +154,12 @@
 		interrupts = < 0 91 0x04 >;
 	};
 
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 	emc@7000f400 {
 		#address-cells = <1>;
 		#size-cells = <0>;
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 62a7b39..3fa5211 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -154,6 +154,12 @@
 		interrupts = < 0 91 0x04 >;
 	};
 
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 	sdhci@78000000 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000000 0x200>;
diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c
index e20b419..2180954 100644
--- a/arch/arm/mach-tegra/board-dt-tegra20.c
+++ b/arch/arm/mach-tegra/board-dt-tegra20.c
@@ -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),
 	{}
 };
 
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c
index 5f7c03e..5e1180f 100644
--- a/arch/arm/mach-tegra/board-dt-tegra30.c
+++ b/arch/arm/mach-tegra/board-dt-tegra30.c
@@ -33,6 +33,8 @@
 #include <asm/mach/arch.h>
 #include <asm/hardware/gic.h>
 
+#include <mach/iomap.h>
+
 #include "board.h"
 #include "clock.h"
 
@@ -51,6 +53,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,tegra30-pwm", TEGRA_PWFM_BASE, "tegra-pwm", NULL),
 	{}
 };
 
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 19540fc..19e96c6 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -242,9 +242,20 @@ static int __devexit tegra_pwm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id tegra_pwm_of_match[] = {
+	{ .compatible = "nvidia,tegra20-pwm" },
+	{ .compatible = "nvidia,tegra30-pwm" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
+#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),
-- 
1.7.9.4

Re: [PATCH v4 07/10] pwm: tegra: Add device tree support

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> Add auxdata to instantiate the PWFM controller from a device tree,
> include the corresponding nodes in the dtsi files for Tegra 20 and
> Tegra 30 and add binding documentation.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>

> +++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt

Can this please be named nvidia,tegra20-pwm.txt so that if we need
different bindings for any future Tegra PWM, we won't have any filename
conflicts?

Aside from that,

Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

Re: [PATCH v4 07/10] pwm: tegra: Add device tree support

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> > Add auxdata to instantiate the PWFM controller from a device tree,
> > include the corresponding nodes in the dtsi files for Tegra 20 and
> > Tegra 30 and add binding documentation.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> 
> > +++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
> 
> Can this please be named nvidia,tegra20-pwm.txt so that if we need
> different bindings for any future Tegra PWM, we won't have any filename
> conflicts?

That file already documents the bindings for Tegra 20 and Tegra 30, that's
why I chose the move generic name. Could this perhaps be documented within
the file instead? Or should I rather add a nvidia,tegra30-pwm.txt with a
reference to the nvidia,tegra20-pwm.txt?

Thierry

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

Re: [PATCH v4 07/10] pwm: tegra: Add device tree support

[Stephen Warren]

On 03/20/2012 02:48 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/14/2012 09:56 AM, Thierry Reding wrote:
>>> Add auxdata to instantiate the PWFM controller from a device tree,
>>> include the corresponding nodes in the dtsi files for Tegra 20 and
>>> Tegra 30 and add binding documentation.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>
>>> +++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
>>
>> Can this please be named nvidia,tegra20-pwm.txt so that if we need
>> different bindings for any future Tegra PWM, we won't have any filename
>> conflicts?
> 
> That file already documents the bindings for Tegra 20 and Tegra 30, that's
> why I chose the move generic name. Could this perhaps be documented within
> the file instead? Or should I rather add a nvidia,tegra30-pwm.txt with a
> reference to the nvidia,tegra20-pwm.txt?

I think it's fine for a file of name nvidia,tegra20-pwm.txt to document
both Tegra20 and Tegra30; the file is named after the first/earliest
compatible value it documents. The issue is more that /if/ say TegraNN
needs a different binding, I want the naming consistent (e.g.
nvidia,tegraNN-pwm.txt) rather than having the old file with one style
and the new file named using a different style.

Re: [PATCH v4 07/10] pwm: tegra: Add device tree support

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/20/2012 02:48 AM, Thierry Reding wrote:
> > * Stephen Warren wrote:
> >> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> >>> Add auxdata to instantiate the PWFM controller from a device tree,
> >>> include the corresponding nodes in the dtsi files for Tegra 20 and
> >>> Tegra 30 and add binding documentation.
> >>>
> >>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> >>
> >>> +++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
> >>
> >> Can this please be named nvidia,tegra20-pwm.txt so that if we need
> >> different bindings for any future Tegra PWM, we won't have any filename
> >> conflicts?
> > 
> > That file already documents the bindings for Tegra 20 and Tegra 30, that's
> > why I chose the move generic name. Could this perhaps be documented within
> > the file instead? Or should I rather add a nvidia,tegra30-pwm.txt with a
> > reference to the nvidia,tegra20-pwm.txt?
> 
> I think it's fine for a file of name nvidia,tegra20-pwm.txt to document
> both Tegra20 and Tegra30; the file is named after the first/earliest
> compatible value it documents. The issue is more that /if/ say TegraNN
> needs a different binding, I want the naming consistent (e.g.
> nvidia,tegraNN-pwm.txt) rather than having the old file with one style
> and the new file named using a different style.

Understood. I'll rename it for the next version.

Thierry

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

Re: [PATCH v4 07/10] pwm: tegra: Add device tree support

[Shawn Guo]

On Mon, Mar 19, 2012 at 08:42:34PM -0600, Stephen Warren wrote:
> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> > Add auxdata to instantiate the PWFM controller from a device tree,
> > include the corresponding nodes in the dtsi files for Tegra 20 and
> > Tegra 30 and add binding documentation.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> 
> > +++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
> 
> Can this please be named nvidia,tegra20-pwm.txt so that if we need

Is it necessary to have even the vendor prefix in the file name?  The
file will look exceptional among all these binding documents under
Documentation/devicetree/bindings.

> different bindings for any future Tegra PWM, we won't have any filename
> conflicts?

-- 
Regards,
Shawn

Re: [PATCH v4 07/10] pwm: tegra: Add device tree support

[Stephen Warren]

On 04/04/2012 01:04 AM, Shawn Guo wrote:
> On Mon, Mar 19, 2012 at 08:42:34PM -0600, Stephen Warren wrote:
>> On 03/14/2012 09:56 AM, Thierry Reding wrote:
>>> Add auxdata to instantiate the PWFM controller from a device tree,
>>> include the corresponding nodes in the dtsi files for Tegra 20 and
>>> Tegra 30 and add binding documentation.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>
>>> +++ b/Documentation/devicetree/bindings/pwm/tegra-pwm.txt
>>
>> Can this please be named nvidia,tegra20-pwm.txt so that if we need
> 
> Is it necessary to have even the vendor prefix in the file name?  The
> file will look exceptional among all these binding documents under
> Documentation/devicetree/bindings.

It would look exceptional now, but I've had it in the back of my mind to
suggest renaming all the binding docs in line with their full compatible
value. Then, it'd just look consistent ;-P.

[PATCH v4 08/10] pwm: Add Blackfin support

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v3:
  - update PWM ops for changes in patch 2

 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

diff --git a/arch/blackfin/Kconfig b/arch/blackfin/Kconfig
index c1269a1..e08f39b 100644
--- a/arch/blackfin/Kconfig
+++ b/arch/blackfin/Kconfig
@@ -950,16 +950,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
diff --git a/arch/blackfin/kernel/Makefile b/arch/blackfin/kernel/Makefile
index 1f88edd..03f60a6 100644
--- a/arch/blackfin/kernel/Makefile
+++ b/arch/blackfin/kernel/Makefile
@@ -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
diff --git a/arch/blackfin/kernel/pwm.c b/arch/blackfin/kernel/pwm.c
deleted file mode 100644
index 33f5942..0000000
--- a/arch/blackfin/kernel/pwm.c
+++ /dev/null
@@ -1,100 +0,0 @@
-/*
- * Blackfin Pulse Width Modulation (PWM) core
- *
- * Copyright (c) 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
- */
-
-#include <linux/module.h>
-#include <linux/pwm.h>
-#include <linux/slab.h>
-
-#include <asm/gptimers.h>
-#include <asm/portmux.h>
-
-struct pwm_device {
-	unsigned id;
-	unsigned short pin;
-};
-
-static const unsigned short pwm_to_gptimer_per[] = {
-	P_TMR0, P_TMR1, P_TMR2, P_TMR3, P_TMR4, P_TMR5,
-	P_TMR6, P_TMR7, P_TMR8, P_TMR9, P_TMR10, P_TMR11,
-};
-
-struct pwm_device *pwm_request(int pwm_id, const char *label)
-{
-	struct pwm_device *pwm;
-	int ret;
-
-	/* XXX: pwm_id really should be unsigned */
-	if (pwm_id < 0)
-		return NULL;
-
-	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
-	if (!pwm)
-		return pwm;
-
-	pwm->id = pwm_id;
-	if (pwm->id >= ARRAY_SIZE(pwm_to_gptimer_per))
-		goto err;
-
-	pwm->pin = pwm_to_gptimer_per[pwm->id];
-	ret = peripheral_request(pwm->pin, label);
-	if (ret)
-		goto err;
-
-	return pwm;
- err:
-	kfree(pwm);
-	return NULL;
-}
-EXPORT_SYMBOL(pwm_request);
-
-void pwm_free(struct pwm_device *pwm)
-{
-	peripheral_free(pwm->pin);
-	kfree(pwm);
-}
-EXPORT_SYMBOL(pwm_free);
-
-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	unsigned long period, duty;
-	unsigned long long val;
-
-	if (duty_ns < 0 || duty_ns > period_ns)
-		return -EINVAL;
-
-	val = (unsigned long long)get_sclk() * period_ns;
-	do_div(val, NSEC_PER_SEC);
-	period = val;
-
-	val = (unsigned long long)period * duty_ns;
-	do_div(val, period_ns);
-	duty = period - val;
-
-	if (duty >= period)
-		duty = period - 1;
-
-	set_gptimer_config(pwm->id, TIMER_MODE_PWM | TIMER_PERIOD_CNT);
-	set_gptimer_pwidth(pwm->id, duty);
-	set_gptimer_period(pwm->id, period);
-
-	return 0;
-}
-EXPORT_SYMBOL(pwm_config);
-
-int pwm_enable(struct pwm_device *pwm)
-{
-	enable_gptimer(pwm->id);
-	return 0;
-}
-EXPORT_SYMBOL(pwm_enable);
-
-void pwm_disable(struct pwm_device *pwm)
-{
-	disable_gptimer(pwm->id);
-}
-EXPORT_SYMBOL(pwm_disable);
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index bda6f23..eb54042 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 12300f5..251b8d2 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
diff --git a/drivers/pwm/pwm-bfin.c b/drivers/pwm/pwm-bfin.c
new file mode 100644
index 0000000..a0c6bf9
--- /dev/null
+++ b/drivers/pwm/pwm-bfin.c
@@ -0,0 +1,164 @@
+/*
+ * Blackfin Pulse Width Modulation (PWM) core
+ *
+ * Copyright (c) 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#include <asm/gptimers.h>
+#include <asm/portmux.h>
+
+struct bfin_pwm_chip {
+	struct pwm_chip chip;
+};
+
+struct bfin_pwm {
+	unsigned short pin;
+};
+
+static const unsigned short pwm_to_gptimer_per[] = {
+	P_TMR0, P_TMR1, P_TMR2, P_TMR3, P_TMR4, P_TMR5,
+	P_TMR6, P_TMR7, P_TMR8, P_TMR9, P_TMR10, P_TMR11,
+};
+
+static int bfin_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct bfin_pwm *priv;
+	int ret;
+
+	if (pwm->hwpwm >= ARRAY_SIZE(pwm_to_gptimer_per))
+		return -EINVAL;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pin = pwm_to_gptimer_per[pwm->hwpwm];
+
+	ret = peripheral_request(priv->pin, NULL);
+	if (ret) {
+		kfree(priv);
+		return ret;
+	}
+
+	pwm_set_chip_data(pwm, priv);
+
+	return 0;
+}
+
+static void bfin_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct bfin_pwm *priv = pwm_get_chip_data(pwm);
+
+	if (priv) {
+		peripheral_free(priv->pin);
+		kfree(priv);
+	}
+}
+
+static int bfin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+		int duty_ns, int period_ns)
+{
+	struct bfin_pwm *priv = pwm_get_chip_data(pwm);
+	unsigned long period, duty;
+	unsigned long long val;
+
+	if (duty_ns < 0 || duty_ns > period_ns)
+		return -EINVAL;
+
+	val = (unsigned long long)get_sclk() * period_ns;
+	do_div(val, NSEC_PER_SEC);
+	period = val;
+
+	val = (unsigned long long)period * duty_ns;
+	do_div(val, period_ns);
+	duty = period - val;
+
+	if (duty >= period)
+		duty = period - 1;
+
+	set_gptimer_config(priv->pin, TIMER_MODE_PWM | TIMER_PERIOD_CNT);
+	set_gptimer_pwidth(priv->pin, duty);
+	set_gptimer_period(priv->pin, period);
+
+	return 0;
+}
+
+static int bfin_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct bfin_pwm *priv = pwm_get_chip_data(pwm);
+
+	enable_gptimer(priv->pin);
+
+	return 0;
+}
+
+static void bfin_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct bfin_pwm *priv = pwm_get_chip_data(pwm);
+
+	disable_gptimer(priv->pin);
+}
+
+static struct pwm_ops bfin_pwm_ops = {
+	.request = bfin_pwm_request,
+	.free = bfin_pwm_free,
+	.config = bfin_pwm_config,
+	.enable = bfin_pwm_enable,
+	.disable = bfin_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int bfin_pwm_probe(struct platform_device *pdev)
+{
+	struct bfin_pwm_chip *pwm;
+	int ret;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &bfin_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = 12;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devexit bfin_pwm_remove(struct platform_device *pdev)
+{
+	struct bfin_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&pwm->chip);
+
+	return 0;
+}
+
+static struct platform_driver bfin_pwm_driver = {
+	.driver = {
+		.name = "bfin-pwm",
+	},
+	.probe = bfin_pwm_probe,
+	.remove = __devexit_p(bfin_pwm_remove),
+};
+
+module_platform_driver(bfin_pwm_driver);
+
+MODULE_LICENSE("GPL");
-- 
1.7.9.4

[PATCH v4 09/10] pwm: Add PXA support

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v3:
  - update PWM ops for changes in patch 2

 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      |  244 +++++++++++++++++++++++++++++++++++
 5 files changed, 254 insertions(+), 305 deletions(-)
 delete mode 100644 arch/arm/plat-pxa/pwm.c
 create mode 100644 drivers/pwm/pwm-pxa.c

diff --git a/arch/arm/plat-pxa/Makefile b/arch/arm/plat-pxa/Makefile
index f302d04..af8e484 100644
--- a/arch/arm/plat-pxa/Makefile
+++ b/arch/arm/plat-pxa/Makefile
@@ -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
diff --git a/arch/arm/plat-pxa/pwm.c b/arch/arm/plat-pxa/pwm.c
deleted file mode 100644
index ef32686..0000000
--- a/arch/arm/plat-pxa/pwm.c
+++ /dev/null
@@ -1,304 +0,0 @@
-/*
- * linux/arch/arm/mach-pxa/pwm.c
- *
- * simple driver for PWM (Pulse Width Modulator) controller
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * 2008-02-13	initial version
- * 		eric miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
- */
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/err.h>
-#include <linux/clk.h>
-#include <linux/io.h>
-#include <linux/pwm.h>
-
-#include <asm/div64.h>
-
-#define HAS_SECONDARY_PWM	0x10
-#define PWM_ID_BASE(d)		((d) & 0xf)
-
-static const struct platform_device_id pwm_id_table[] = {
-	/*   PWM    has_secondary_pwm? */
-	{ "pxa25x-pwm", 0 },
-	{ "pxa27x-pwm", 0 | HAS_SECONDARY_PWM },
-	{ "pxa168-pwm", 1 },
-	{ "pxa910-pwm", 1 },
-	{ },
-};
-MODULE_DEVICE_TABLE(platform, pwm_id_table);
-
-/* PWM registers and bits definitions */
-#define PWMCR		(0x00)
-#define PWMDCR		(0x04)
-#define PWMPCR		(0x08)
-
-#define PWMCR_SD	(1 << 6)
-#define PWMDCR_FD	(1 << 10)
-
-struct pwm_device {
-	struct list_head	node;
-	struct pwm_device	*secondary;
-	struct platform_device	*pdev;
-
-	const char	*label;
-	struct clk	*clk;
-	int		clk_enabled;
-	void __iomem	*mmio_base;
-
-	unsigned int	use_count;
-	unsigned int	pwm_id;
-};
-
-/*
- * period_ns = 10^9 * (PRESCALE + 1) * (PV + 1) / PWM_CLK_RATE
- * duty_ns   = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
- */
-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	unsigned long long c;
-	unsigned long period_cycles, prescale, pv, dc;
-
-	if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
-		return -EINVAL;
-
-	c = clk_get_rate(pwm->clk);
-	c = c * period_ns;
-	do_div(c, 1000000000);
-	period_cycles = c;
-
-	if (period_cycles < 1)
-		period_cycles = 1;
-	prescale = (period_cycles - 1) / 1024;
-	pv = period_cycles / (prescale + 1) - 1;
-
-	if (prescale > 63)
-		return -EINVAL;
-
-	if (duty_ns == period_ns)
-		dc = PWMDCR_FD;
-	else
-		dc = (pv + 1) * duty_ns / period_ns;
-
-	/* NOTE: the clock to PWM has to be enabled first
-	 * before writing to the registers
-	 */
-	clk_enable(pwm->clk);
-	__raw_writel(prescale, pwm->mmio_base + PWMCR);
-	__raw_writel(dc, pwm->mmio_base + PWMDCR);
-	__raw_writel(pv, pwm->mmio_base + PWMPCR);
-	clk_disable(pwm->clk);
-
-	return 0;
-}
-EXPORT_SYMBOL(pwm_config);
-
-int pwm_enable(struct pwm_device *pwm)
-{
-	int rc = 0;
-
-	if (!pwm->clk_enabled) {
-		rc = clk_enable(pwm->clk);
-		if (!rc)
-			pwm->clk_enabled = 1;
-	}
-	return rc;
-}
-EXPORT_SYMBOL(pwm_enable);
-
-void pwm_disable(struct pwm_device *pwm)
-{
-	if (pwm->clk_enabled) {
-		clk_disable(pwm->clk);
-		pwm->clk_enabled = 0;
-	}
-}
-EXPORT_SYMBOL(pwm_disable);
-
-static DEFINE_MUTEX(pwm_lock);
-static LIST_HEAD(pwm_list);
-
-struct pwm_device *pwm_request(int pwm_id, const char *label)
-{
-	struct pwm_device *pwm;
-	int found = 0;
-
-	mutex_lock(&pwm_lock);
-
-	list_for_each_entry(pwm, &pwm_list, node) {
-		if (pwm->pwm_id == pwm_id) {
-			found = 1;
-			break;
-		}
-	}
-
-	if (found) {
-		if (pwm->use_count == 0) {
-			pwm->use_count++;
-			pwm->label = label;
-		} else
-			pwm = ERR_PTR(-EBUSY);
-	} else
-		pwm = ERR_PTR(-ENOENT);
-
-	mutex_unlock(&pwm_lock);
-	return pwm;
-}
-EXPORT_SYMBOL(pwm_request);
-
-void pwm_free(struct pwm_device *pwm)
-{
-	mutex_lock(&pwm_lock);
-
-	if (pwm->use_count) {
-		pwm->use_count--;
-		pwm->label = NULL;
-	} else
-		pr_warning("PWM device already freed\n");
-
-	mutex_unlock(&pwm_lock);
-}
-EXPORT_SYMBOL(pwm_free);
-
-static inline void __add_pwm(struct pwm_device *pwm)
-{
-	mutex_lock(&pwm_lock);
-	list_add_tail(&pwm->node, &pwm_list);
-	mutex_unlock(&pwm_lock);
-}
-
-static int __devinit pwm_probe(struct platform_device *pdev)
-{
-	const struct platform_device_id *id = platform_get_device_id(pdev);
-	struct pwm_device *pwm, *secondary = NULL;
-	struct resource *r;
-	int ret = 0;
-
-	pwm = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
-	if (pwm == NULL) {
-		dev_err(&pdev->dev, "failed to allocate memory\n");
-		return -ENOMEM;
-	}
-
-	pwm->clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pwm->clk)) {
-		ret = PTR_ERR(pwm->clk);
-		goto err_free;
-	}
-	pwm->clk_enabled = 0;
-
-	pwm->use_count = 0;
-	pwm->pwm_id = PWM_ID_BASE(id->driver_data) + pdev->id;
-	pwm->pdev = pdev;
-
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
-		dev_err(&pdev->dev, "no memory resource defined\n");
-		ret = -ENODEV;
-		goto err_free_clk;
-	}
-
-	r = request_mem_region(r->start, resource_size(r), pdev->name);
-	if (r == NULL) {
-		dev_err(&pdev->dev, "failed to request memory resource\n");
-		ret = -EBUSY;
-		goto err_free_clk;
-	}
-
-	pwm->mmio_base = ioremap(r->start, resource_size(r));
-	if (pwm->mmio_base == NULL) {
-		dev_err(&pdev->dev, "failed to ioremap() registers\n");
-		ret = -ENODEV;
-		goto err_free_mem;
-	}
-
-	if (id->driver_data & HAS_SECONDARY_PWM) {
-		secondary = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
-		if (secondary == NULL) {
-			ret = -ENOMEM;
-			goto err_free_mem;
-		}
-
-		*secondary = *pwm;
-		pwm->secondary = secondary;
-
-		/* registers for the second PWM has offset of 0x10 */
-		secondary->mmio_base = pwm->mmio_base + 0x10;
-		secondary->pwm_id = pdev->id + 2;
-	}
-
-	__add_pwm(pwm);
-	if (secondary)
-		__add_pwm(secondary);
-
-	platform_set_drvdata(pdev, pwm);
-	return 0;
-
-err_free_mem:
-	release_mem_region(r->start, resource_size(r));
-err_free_clk:
-	clk_put(pwm->clk);
-err_free:
-	kfree(pwm);
-	return ret;
-}
-
-static int __devexit pwm_remove(struct platform_device *pdev)
-{
-	struct pwm_device *pwm;
-	struct resource *r;
-
-	pwm = platform_get_drvdata(pdev);
-	if (pwm == NULL)
-		return -ENODEV;
-
-	mutex_lock(&pwm_lock);
-
-	if (pwm->secondary) {
-		list_del(&pwm->secondary->node);
-		kfree(pwm->secondary);
-	}
-
-	list_del(&pwm->node);
-	mutex_unlock(&pwm_lock);
-
-	iounmap(pwm->mmio_base);
-
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(r->start, resource_size(r));
-
-	clk_put(pwm->clk);
-	kfree(pwm);
-	return 0;
-}
-
-static struct platform_driver pwm_driver = {
-	.driver		= {
-		.name	= "pxa25x-pwm",
-		.owner	= THIS_MODULE,
-	},
-	.probe		= pwm_probe,
-	.remove		= __devexit_p(pwm_remove),
-	.id_table	= pwm_id_table,
-};
-
-static int __init pwm_init(void)
-{
-	return platform_driver_register(&pwm_driver);
-}
-arch_initcall(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&pwm_driver);
-}
-module_exit(pwm_exit);
-
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index eb54042..0ef4f30 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 251b8d2..e859c51 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
new file mode 100644
index 0000000..0f3ef5c
--- /dev/null
+++ b/drivers/pwm/pwm-pxa.c
@@ -0,0 +1,244 @@
+/*
+ * linux/arch/arm/mach-pxa/pwm.c
+ *
+ * simple driver for PWM (Pulse Width Modulator) controller
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 2008-02-13	initial version
+ * 		eric miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+
+#include <asm/div64.h>
+
+#define HAS_SECONDARY_PWM	0x10
+#define PWM_ID_BASE(d)		((d) & 0xf)
+
+static const struct platform_device_id pwm_id_table[] = {
+	/*   PWM    has_secondary_pwm? */
+	{ "pxa25x-pwm", 0 },
+	{ "pxa27x-pwm", 0 | HAS_SECONDARY_PWM },
+	{ "pxa168-pwm", 1 },
+	{ "pxa910-pwm", 1 },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, pwm_id_table);
+
+/* PWM registers and bits definitions */
+#define PWMCR		(0x00)
+#define PWMDCR		(0x04)
+#define PWMPCR		(0x08)
+
+#define PWMCR_SD	(1 << 6)
+#define PWMDCR_FD	(1 << 10)
+
+struct pxa_pwm_chip {
+	struct pwm_chip	chip;
+	struct device	*dev;
+
+	struct clk	*clk;
+	int		clk_enabled;
+	void __iomem	*mmio_base;
+};
+
+static inline struct pxa_pwm_chip *to_pxa_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct pxa_pwm_chip, chip);
+}
+
+/*
+ * period_ns = 10^9 * (PRESCALE + 1) * (PV + 1) / PWM_CLK_RATE
+ * duty_ns   = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
+ */
+static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			  int duty_ns, int period_ns)
+{
+	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
+	unsigned long long c;
+	unsigned long period_cycles, prescale, pv, dc;
+	unsigned long offset;
+
+	if (period_ns == 0 || duty_ns > period_ns)
+		return -EINVAL;
+
+	offset = pwm->hwpwm ? 0x10 : 0;
+
+	c = clk_get_rate(pc->clk);
+	c = c * period_ns;
+	do_div(c, 1000000000);
+	period_cycles = c;
+
+	if (period_cycles < 1)
+		period_cycles = 1;
+	prescale = (period_cycles - 1) / 1024;
+	pv = period_cycles / (prescale + 1) - 1;
+
+	if (prescale > 63)
+		return -EINVAL;
+
+	if (duty_ns == period_ns)
+		dc = PWMDCR_FD;
+	else
+		dc = (pv + 1) * duty_ns / period_ns;
+
+	/* NOTE: the clock to PWM has to be enabled first
+	 * before writing to the registers
+	 */
+	clk_enable(pc->clk);
+	__raw_writel(prescale, pc->mmio_base + offset + PWMCR);
+	__raw_writel(dc, pc->mmio_base + offset + PWMDCR);
+	__raw_writel(pv, pc->mmio_base + offset + PWMPCR);
+	clk_disable(pc->clk);
+
+	return 0;
+}
+
+static int pxa_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
+	int rc = 0;
+
+	if (!pc->clk_enabled) {
+		rc = clk_enable(pc->clk);
+		if (!rc)
+			pc->clk_enabled++;
+	}
+	return rc;
+}
+
+static void pxa_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
+
+	if (pc->clk_enabled) {
+		clk_disable(pc->clk);
+		pc->clk_enabled--;
+	}
+}
+
+static struct pwm_ops pxa_pwm_ops = {
+	.config = pxa_pwm_config,
+	.enable = pxa_pwm_enable,
+	.disable = pxa_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int __devinit pwm_probe(struct platform_device *pdev)
+{
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+	struct pxa_pwm_chip *pwm;
+	struct resource *r;
+	int ret = 0;
+
+	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
+	if (pwm == NULL) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	pwm->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pwm->clk)) {
+		ret = PTR_ERR(pwm->clk);
+		goto err_free;
+	}
+	pwm->clk_enabled = 0;
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &pxa_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		dev_err(&pdev->dev, "no memory resource defined\n");
+		ret = -ENODEV;
+		goto err_free_clk;
+	}
+
+	r = request_mem_region(r->start, resource_size(r), pdev->name);
+	if (r == NULL) {
+		dev_err(&pdev->dev, "failed to request memory resource\n");
+		ret = -EBUSY;
+		goto err_free_clk;
+	}
+
+	pwm->mmio_base = ioremap(r->start, resource_size(r));
+	if (pwm->mmio_base == NULL) {
+		dev_err(&pdev->dev, "failed to ioremap() registers\n");
+		ret = -ENODEV;
+		goto err_free_mem;
+	}
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+	return 0;
+
+err_free_mem:
+	release_mem_region(r->start, resource_size(r));
+err_free_clk:
+	clk_put(pwm->clk);
+err_free:
+	kfree(pwm);
+	return ret;
+}
+
+static int __devexit pwm_remove(struct platform_device *pdev)
+{
+	struct pxa_pwm_chip *chip;
+	struct resource *r;
+
+	chip = platform_get_drvdata(pdev);
+	if (chip == NULL)
+		return -ENODEV;
+
+	pwmchip_remove(&chip->chip);
+
+	iounmap(chip->mmio_base);
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(r->start, resource_size(r));
+
+	clk_put(chip->clk);
+	kfree(chip);
+	return 0;
+}
+
+static struct platform_driver pwm_driver = {
+	.driver		= {
+		.name	= "pxa25x-pwm",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= pwm_probe,
+	.remove		= __devexit_p(pwm_remove),
+	.id_table	= pwm_id_table,
+};
+
+static int __init pwm_init(void)
+{
+	return platform_driver_register(&pwm_driver);
+}
+arch_initcall(pwm_init);
+
+static void __exit pwm_exit(void)
+{
+	platform_driver_unregister(&pwm_driver);
+}
+module_exit(pwm_exit);
+
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.4

Re: [PATCH v4 09/10] pwm: Add PXA support

[Ryan Mallon]

On 15/03/12 02:56, Thierry Reding wrote:

> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
> Changes in v3:
>   - update PWM ops for changes in patch 2

Couple of quick notes, mostly for future work.

> +	/* NOTE: the clock to PWM has to be enabled first
> +	 * before writing to the registers
> +	 */
> +	clk_enable(pc->clk);


Should be fixed to also call clk_prepare (and clk_unprepare after
clk_disable). Could be done in a follow up patch.

> +	__raw_writel(prescale, pc->mmio_base + offset + PWMCR);
> +	__raw_writel(dc, pc->mmio_base + offset + PWMDCR);
> +	__raw_writel(pv, pc->mmio_base + offset + PWMPCR);


Should we fix this driver to use readl/writel instead of the __raw
variants? The memory is properly ioremaped, and to my understanding the
__raw memory accessors should be avoided outside of core code. This
could be done in a follow up patch if you want to keep this patch as
mostly just a move of the code.

~Ryan

Re: [PATCH v4 09/10] pwm: Add PXA support

[Thierry Reding]

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

* Ryan Mallon wrote:
> On 15/03/12 02:56, Thierry Reding wrote:
> 
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> > ---
> > Changes in v3:
> >   - update PWM ops for changes in patch 2
> 
> Couple of quick notes, mostly for future work.
> 
> > +	/* NOTE: the clock to PWM has to be enabled first
> > +	 * before writing to the registers
> > +	 */
> > +	clk_enable(pc->clk);
> 
> 
> Should be fixed to also call clk_prepare (and clk_unprepare after
> clk_disable). Could be done in a follow up patch.
> 
> > +	__raw_writel(prescale, pc->mmio_base + offset + PWMCR);
> > +	__raw_writel(dc, pc->mmio_base + offset + PWMDCR);
> > +	__raw_writel(pv, pc->mmio_base + offset + PWMPCR);
> 
> 
> Should we fix this driver to use readl/writel instead of the __raw
> variants? The memory is properly ioremaped, and to my understanding the
> __raw memory accessors should be avoided outside of core code. This
> could be done in a follow up patch if you want to keep this patch as
> mostly just a move of the code.

Actually I wasn't planning on keeping this patch at all. Sascha already has
the existing PWM providers converted to his original framework and offered to
rebase them onto this series once the dust settles. I only used them as
testbed to see how the driver interface works out for different hardware. But
I also think that if Sascha hasn't cleaned the driver up yet it should either
be done in his conversion patches or as follow ups.

Sascha: how do you plan on going forward with this? It seems like the driver
interface is pretty much done now and I expect the next round to be the last,
unless I forget to properly work through the TODO list again. If you are busy
with other stuff I can probably find some time to help with porting your
converted drivers.

I'm also wondering which tree this will go in through. Does it make sense to
have an extra tree just for the PWM framework or can it go in via some other
general purpose tree? Who do I need to prod?

Thierry

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

Re: [PATCH v4 09/10] pwm: Add PXA support

[Sascha Hauer]

On Thu, Mar 15, 2012 at 07:56:31AM +0100, Thierry Reding wrote:
> * Ryan Mallon wrote:
> > On 15/03/12 02:56, Thierry Reding wrote:
> > 
> > > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> > > ---
> > > Changes in v3:
> > >   - update PWM ops for changes in patch 2
> > 
> > Couple of quick notes, mostly for future work.
> > 
> > > +	/* NOTE: the clock to PWM has to be enabled first
> > > +	 * before writing to the registers
> > > +	 */
> > > +	clk_enable(pc->clk);
> > 
> > 
> > Should be fixed to also call clk_prepare (and clk_unprepare after
> > clk_disable). Could be done in a follow up patch.
> > 
> > > +	__raw_writel(prescale, pc->mmio_base + offset + PWMCR);
> > > +	__raw_writel(dc, pc->mmio_base + offset + PWMDCR);
> > > +	__raw_writel(pv, pc->mmio_base + offset + PWMPCR);
> > 
> > 
> > Should we fix this driver to use readl/writel instead of the __raw
> > variants? The memory is properly ioremaped, and to my understanding the
> > __raw memory accessors should be avoided outside of core code. This
> > could be done in a follow up patch if you want to keep this patch as
> > mostly just a move of the code.
> 
> Actually I wasn't planning on keeping this patch at all. Sascha already has
> the existing PWM providers converted to his original framework and offered to
> rebase them onto this series once the dust settles. I only used them as
> testbed to see how the driver interface works out for different hardware. But
> I also think that if Sascha hasn't cleaned the driver up yet it should either
> be done in his conversion patches or as follow ups.

All I have is a simple conversion to the new framework, nothing more.  I
rebased my patches yesterday onto this series (unfortunately due to the
additional argument pwm_device the patches do not look like just moving
the drivers anymore). I also skipped the PXA patch since I saw that you
already have this one. I just posted the patches to the list.

> 
> Sascha: how do you plan on going forward with this? It seems like the driver
> interface is pretty much done now and I expect the next round to be the last,
> unless I forget to properly work through the TODO list again. If you are busy
> with other stuff I can probably find some time to help with porting your
> converted drivers.

I haven't done drivers/mfd/twl6030-pwm.c. This one needs a bit more work
as it currently does not register itself as a subdriver to the twl6030
but just uses a globally available twl_i2c_write_u8() function. So if
you have some time to spare it would be great if you could do this.

> 
> I'm also wondering which tree this will go in through. Does it make sense to
> have an extra tree just for the PWM framework or can it go in via some other
> general purpose tree? Who do I need to prod?

Good question, I don't know.

Sascha

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

Re: [PATCH v4 09/10] pwm: Add PXA support

[Thierry Reding]

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

* Sascha Hauer wrote:
> On Thu, Mar 15, 2012 at 07:56:31AM +0100, Thierry Reding wrote:
> > Actually I wasn't planning on keeping this patch at all. Sascha already has
> > the existing PWM providers converted to his original framework and offered to
> > rebase them onto this series once the dust settles. I only used them as
> > testbed to see how the driver interface works out for different hardware. But
> > I also think that if Sascha hasn't cleaned the driver up yet it should either
> > be done in his conversion patches or as follow ups.
> 
> All I have is a simple conversion to the new framework, nothing more.  I
> rebased my patches yesterday onto this series (unfortunately due to the
> additional argument pwm_device the patches do not look like just moving
> the drivers anymore). I also skipped the PXA patch since I saw that you
> already have this one. I just posted the patches to the list.

Okay, I'll keep my Blackfin and PXA converted drivers then and will fix the
issues with the PXA driver brought up by Ryan. Would you be okay if I took
your conversion patches and added them to my series? I was trying to avoid
the additional work but I think it might make things easier if they are part
of the same series.

> > Sascha: how do you plan on going forward with this? It seems like the driver
> > interface is pretty much done now and I expect the next round to be the last,
> > unless I forget to properly work through the TODO list again. If you are busy
> > with other stuff I can probably find some time to help with porting your
> > converted drivers.
> 
> I haven't done drivers/mfd/twl6030-pwm.c. This one needs a bit more work
> as it currently does not register itself as a subdriver to the twl6030
> but just uses a globally available twl_i2c_write_u8() function. So if
> you have some time to spare it would be great if you could do this.

Okay, I'll have a look. I guess the next round won't be the last after all.
:-)

> > I'm also wondering which tree this will go in through. Does it make sense to
> > have an extra tree just for the PWM framework or can it go in via some other
> > general purpose tree? Who do I need to prod?
> 
> Good question, I don't know.

Okay, maybe Arnd can comment on it.

Thierry

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

Re: [PATCH v4 09/10] pwm: Add PXA support

[Sascha Hauer]

On Thu, Mar 15, 2012 at 10:21:34AM +0100, Thierry Reding wrote:
> * Sascha Hauer wrote:
> > On Thu, Mar 15, 2012 at 07:56:31AM +0100, Thierry Reding wrote:
> > > Actually I wasn't planning on keeping this patch at all. Sascha already has
> > > the existing PWM providers converted to his original framework and offered to
> > > rebase them onto this series once the dust settles. I only used them as
> > > testbed to see how the driver interface works out for different hardware. But
> > > I also think that if Sascha hasn't cleaned the driver up yet it should either
> > > be done in his conversion patches or as follow ups.
> > 
> > All I have is a simple conversion to the new framework, nothing more.  I
> > rebased my patches yesterday onto this series (unfortunately due to the
> > additional argument pwm_device the patches do not look like just moving
> > the drivers anymore). I also skipped the PXA patch since I saw that you
> > already have this one. I just posted the patches to the list.
> 
> Okay, I'll keep my Blackfin and PXA converted drivers then and will fix the
> issues with the PXA driver brought up by Ryan. Would you be okay if I took
> your conversion patches and added them to my series?

Sure, go ahead.

Sascha

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

Re: [PATCH v4 09/10] pwm: Add PXA support

[Shawn Guo]

On Wed, Mar 14, 2012 at 04:56:32PM +0100, Thierry Reding wrote:
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
> Changes in v3:
>   - update PWM ops for changes in patch 2
> 
>  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      |  244 +++++++++++++++++++++++++++++++++++
>  5 files changed, 254 insertions(+), 305 deletions(-)
>  delete mode 100644 arch/arm/plat-pxa/pwm.c
>  create mode 100644 drivers/pwm/pwm-pxa.c
> 
The patch should be generated with "git format-patch -M", so that we
can see the diff like below. 

--- a/arch/arm/plat-pxa/pwm.c
+++ b/drivers/pwm/pwm-pxa.c

-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+                         int duty_ns, int period_ns)
 {
+       struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
        unsigned long long c;
        unsigned long period_cycles, prescale, pv, dc;
+       unsigned long offset;

-       if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
+       if (period_ns == 0 || duty_ns > period_ns)
                return -EINVAL;

Then I will have a question why "pwm == NULL" check is removed?

Regards,
Shawn

Re: [PATCH v4 09/10] pwm: Add PXA support

[Thierry Reding]

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

* Shawn Guo wrote:
> On Wed, Mar 14, 2012 at 04:56:32PM +0100, Thierry Reding wrote:
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> > ---
> > Changes in v3:
> >   - update PWM ops for changes in patch 2
> > 
> >  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      |  244 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 254 insertions(+), 305 deletions(-)
> >  delete mode 100644 arch/arm/plat-pxa/pwm.c
> >  create mode 100644 drivers/pwm/pwm-pxa.c
> > 
> The patch should be generated with "git format-patch -M", so that we
> can see the diff like below. 
> 
> --- a/arch/arm/plat-pxa/pwm.c
> +++ b/drivers/pwm/pwm-pxa.c

Yes, Arnd already mentioned that in the last round but I forgot. It usually
takes me a couple of weeks to prepare the next version and I try to keep a
list of TODOs but that doesn't seem to be efficient enough yet.

> -int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +                         int duty_ns, int period_ns)
>  {
> +       struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip);
>         unsigned long long c;
>         unsigned long period_cycles, prescale, pv, dc;
> +       unsigned long offset;
> 
> -       if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
> +       if (period_ns == 0 || duty_ns > period_ns)
>                 return -EINVAL;
> 
> Then I will have a question why "pwm == NULL" check is removed?

Actually, as I mentioned in another mail the idea was to put these kinds of
checks into the core. They are still missing from the core, though.

Thierry

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

[PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Thierry Reding]

This commit adds very basic support for device tree probing. Currently,
only a PWM and a list of distinct brightness levels 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.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v4:
  - store a PWM requested with of_pwm_request() in the platform data

Changes in v3:
  - use a list of distinct brightness levels instead of a linear range
    as discussed with Stephen Warren and Mitch Bradley

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

TODO:
  - add fixed-regulator support for backlight enable/disable

 .../bindings/video/backlight/pwm-backlight         |   19 +++
 drivers/video/backlight/Kconfig                    |    2 +-
 drivers/video/backlight/pwm_bl.c                   |  136 +++++++++++++++++---
 include/linux/pwm_backlight.h                      |    2 +
 4 files changed, 141 insertions(+), 18 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..79db3dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
@@ -0,0 +1,19 @@
+pwm-backlight bindings
+
+Required properties:
+  - compatible: "pwm-backlight"
+  - pwm: OF device-tree PWM specification
+  - num-brightness-levels: number of distinct brightness levels
+  - brightness-levels: array of distinct brightness levels
+  - default-brightness-level: the default brightness level
+
+Example:
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwm = <&pwm 0 5000000>;
+
+		num-brightness-levels = <8>;
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+	};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 7ed9991..ecfe3a0 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 342b7d7..57ff91b 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>
@@ -26,11 +27,13 @@ struct pwm_bl_data {
 	struct device		*dev;
 	unsigned int		period;
 	unsigned int		lth_brightness;
+	unsigned int		*levels;
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
 					int brightness);
 	int			(*check_fb)(struct device *, struct fb_info *);
+	void			(*exit)(struct device *);
 };
 
 static int pwm_backlight_update_status(struct backlight_device *bl)
@@ -52,6 +55,11 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		pwm_config(pb->pwm, 0, pb->period);
 		pwm_disable(pb->pwm);
 	} else {
+		if (pb->levels) {
+			brightness = pb->levels[brightness];
+			max = pb->levels[max];
+		}
+
 		brightness = pb->lth_brightness +
 			(brightness * (pb->period - pb->lth_brightness) / max);
 		pwm_config(pb->pwm, brightness, pb->period);
@@ -83,17 +91,102 @@ 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;
+	struct pwm_device *pwm;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	pwm = of_pwm_request(node, "pwm", 0);
+	if (IS_ERR(pwm))
+		return PTR_ERR(pwm);
+
+	memset(data, 0, sizeof(*data));
+	data->pwm_period_ns = pwm_get_period(pwm);
+	data->pwm = pwm;
+
+	ret = of_property_read_u32(node, "num-brightness-levels", &value);
+	if (ret < 0)
+		goto free;
+
+	data->max_brightness = value;
+
+	if (data->max_brightness > 0) {
+		size_t size = sizeof(*data->levels) * data->max_brightness;
+
+		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
+		if (!data->levels) {
+			ret = -ENOMEM;
+			goto free;
+		}
+
+		ret = of_property_read_u32_array(node, "brightness-levels",
+						 data->levels,
+						 data->max_brightness);
+		if (ret < 0)
+			goto free;
+
+		ret = of_property_read_u32(node, "default-brightness-level",
+					   &value);
+		if (ret < 0)
+			goto free;
+
+		data->dft_brightness = value;
+		data->max_brightness--;
+	}
+
+	/*
+	 * 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;
+
+free:
+	pwm_free(pwm);
+
+	return ret;
+}
+
+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;
+	unsigned int max;
 	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) {
@@ -109,21 +202,30 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	if (data->levels) {
+		max = data->levels[data->max_brightness];
+		pb->levels = data->levels;
+	} else
+		max = data->max_brightness;
+
 	pb->period = data->pwm_period_ns;
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
 	pb->check_fb = data->check_fb;
-	pb->lth_brightness = data->lth_brightness *
-		(data->pwm_period_ns / data->max_brightness);
+	pb->exit = data->exit;
+	pb->lth_brightness = data->lth_brightness * (data->pwm_period_ns / max);
+	pb->pwm = data->pwm;
 	pb->dev = &pdev->dev;
 
-	pb->pwm = pwm_request(data->pwm_id, "backlight");
-	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM for backlight\n");
-		ret = PTR_ERR(pb->pwm);
-		goto err_alloc;
-	} else
-		dev_dbg(&pdev->dev, "got pwm for backlight\n");
+	if (!pb->pwm) {
+		pb->pwm = pwm_request(data->pwm_id, "backlight");
+		if (IS_ERR(pb->pwm)) {
+			dev_err(&pdev->dev, "unable to request PWM for backlight\n");
+			ret = PTR_ERR(pb->pwm);
+			goto err_alloc;
+		} else
+			dev_dbg(&pdev->dev, "got pwm for backlight\n");
+	}
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
@@ -152,7 +254,6 @@ err_alloc:
 
 static int pwm_backlight_remove(struct platform_device *pdev)
 {
-	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
 	struct backlight_device *bl = platform_get_drvdata(pdev);
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
@@ -160,8 +261,8 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	pwm_config(pb->pwm, 0, pb->period);
 	pwm_disable(pb->pwm);
 	pwm_free(pb->pwm);
-	if (data->exit)
-		data->exit(&pdev->dev);
+	if (pb->exit)
+		pb->exit(&pdev->dev);
 	return 0;
 }
 
@@ -195,11 +296,12 @@ static SIMPLE_DEV_PM_OPS(pwm_backlight_pm_ops, pwm_backlight_suspend,
 
 static struct platform_driver pwm_backlight_driver = {
 	.driver		= {
-		.name	= "pwm-backlight",
-		.owner	= THIS_MODULE,
+		.name		= "pwm-backlight",
+		.owner		= THIS_MODULE,
 #ifdef CONFIG_PM
-		.pm	= &pwm_backlight_pm_ops,
+		.pm		= &pwm_backlight_pm_ops,
 #endif
+		.of_match_table	= of_match_ptr(pwm_backlight_of_match),
 	},
 	.probe		= pwm_backlight_probe,
 	.remove		= pwm_backlight_remove,
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 63d2df4..3d1f0da 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -7,11 +7,13 @@
 #include <linux/backlight.h>
 
 struct platform_pwm_backlight_data {
+	struct pwm_device *pwm;
 	int pwm_id;
 	unsigned int max_brightness;
 	unsigned int dft_brightness;
 	unsigned int lth_brightness;
 	unsigned int pwm_period_ns;
+	unsigned int *levels;
 	int (*init)(struct device *dev);
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
-- 
1.7.9.4

Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Arnd Bergmann]

On Wednesday 14 March 2012, Thierry Reding wrote:
> +
> +       pwm = of_pwm_request(node, "pwm", 0);
> +       if (IS_ERR(pwm))
> +               return PTR_ERR(pwm);

It's interesting that the (so far) only user of of_pwm_request() doesn't
actually give a meaningful argument as the name of the pwm property.

Maybe rename that function to of_pwm_request_named() and provide a helper
that just does this:?

static inline struct pwm_device *of_pwm_request(struct device_node *np, int index)
{
	return of_pwm_request_named(np, "pwm", index);
}

Any device that just has needs one pwm or uses more than one but has no
reason to name them can just use this helper then and use the default pwm
property name.

	Arnd

Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Stephen Warren]

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> This commit adds very basic support for device tree probing. Currently,
> only a PWM and a list of distinct brightness levels 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.

> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight

> +pwm-backlight bindings
> +
> +Required properties:
> +  - compatible: "pwm-backlight"
> +  - pwm: OF device-tree PWM specification
> +  - num-brightness-levels: number of distinct brightness levels
> +  - brightness-levels: array of distinct brightness levels

I assume the values in this array are 0 (darkest/off) to 255 (max
brightness)? The doc should probably specify this.

> +  - default-brightness-level: the default brightness level

Likewise, this is an index into the default-brightness-level? Again,
it'd be best to explicitly state this.

...
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <6>;


> +static int pwm_backlight_parse_dt(struct device *dev,
> +				  struct platform_pwm_backlight_data *data)
...
> +		ret = of_property_read_u32(node, "default-brightness-level",
> +					   &value);
> +		if (ret < 0)
> +			goto free;

Range-check that against max_brightness?

>  static int pwm_backlight_probe(struct platform_device *pdev)
...
> -	pb->pwm = pwm_request(data->pwm_id, "backlight");
> -	if (IS_ERR(pb->pwm)) {
> -		dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> -		ret = PTR_ERR(pb->pwm);
> -		goto err_alloc;
> -	} else
> -		dev_dbg(&pdev->dev, "got pwm for backlight\n");
> +	if (!pb->pwm) {
> +		pb->pwm = pwm_request(data->pwm_id, "backlight");
> +		if (IS_ERR(pb->pwm)) {
> +			dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> +			ret = PTR_ERR(pb->pwm);
> +			goto err_alloc;
> +		} else
> +			dev_dbg(&pdev->dev, "got pwm for backlight\n");
> +	}

Hmmm. It'd be more consistent if pwm_backlight_parse_dt() called
something like of_pwm_get() instead of of_pwm_request(), so that this
code could always call pwm_request() on the PWM and hence operate the
same irrespective of DT vs non-DT. GPIOs work that way at least.

Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> > This commit adds very basic support for device tree probing. Currently,
> > only a PWM and a list of distinct brightness levels 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.
> 
> > +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
> 
> > +pwm-backlight bindings
> > +
> > +Required properties:
> > +  - compatible: "pwm-backlight"
> > +  - pwm: OF device-tree PWM specification
> > +  - num-brightness-levels: number of distinct brightness levels
> > +  - brightness-levels: array of distinct brightness levels
> 
> I assume the values in this array are 0 (darkest/off) to 255 (max
> brightness)? The doc should probably specify this.

Typically yes. Although I haven't tested this it should work for pretty much
any range starting from 0 because the last value will be used to interpolate
("scale") the PWM duty cycle. So it really is 0 (darkest/off) to <last item>
(max brightness). I should document this, of course.

> > +  - default-brightness-level: the default brightness level
> 
> Likewise, this is an index into the default-brightness-level? Again,
> it'd be best to explicitly state this.

Yes. Correct.

> ...
> > +		brightness-levels = <0 4 8 16 32 64 128 255>;
> > +		default-brightness-level = <6>;
> 
> 
> > +static int pwm_backlight_parse_dt(struct device *dev,
> > +				  struct platform_pwm_backlight_data *data)
> ...
> > +		ret = of_property_read_u32(node, "default-brightness-level",
> > +					   &value);
> > +		if (ret < 0)
> > +			goto free;
> 
> Range-check that against max_brightness?

Yes, that would be good. I guess logging a warning and falling back to
max_brightness would be the proper action?

> >  static int pwm_backlight_probe(struct platform_device *pdev)
> ...
> > -	pb->pwm = pwm_request(data->pwm_id, "backlight");
> > -	if (IS_ERR(pb->pwm)) {
> > -		dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> > -		ret = PTR_ERR(pb->pwm);
> > -		goto err_alloc;
> > -	} else
> > -		dev_dbg(&pdev->dev, "got pwm for backlight\n");
> > +	if (!pb->pwm) {
> > +		pb->pwm = pwm_request(data->pwm_id, "backlight");
> > +		if (IS_ERR(pb->pwm)) {
> > +			dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> > +			ret = PTR_ERR(pb->pwm);
> > +			goto err_alloc;
> > +		} else
> > +			dev_dbg(&pdev->dev, "got pwm for backlight\n");
> > +	}
> 
> Hmmm. It'd be more consistent if pwm_backlight_parse_dt() called
> something like of_pwm_get() instead of of_pwm_request(), so that this
> code could always call pwm_request() on the PWM and hence operate the
> same irrespective of DT vs non-DT. GPIOs work that way at least.

That's actually what the initial patch had. Unfortunately that's pretty much
the opposite direction of where the PWM framework is headed because it would
involve getting a global index to request the PWM. I think in the long run it
would be much better to get rid of pwm_request() altogether and unify by
having the non-DT case request the PWM device on a per-chip basis.

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: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Stephen Warren]

On 03/20/2012 02:39 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/14/2012 09:56 AM, Thierry Reding wrote:
>>> This commit adds very basic support for device tree probing. Currently,
>>> only a PWM and a list of distinct brightness levels 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.
...
>>>  static int pwm_backlight_probe(struct platform_device *pdev)
>> ...
>>> -	pb->pwm = pwm_request(data->pwm_id, "backlight");
>>> -	if (IS_ERR(pb->pwm)) {
>>> -		dev_err(&pdev->dev, "unable to request PWM for backlight\n");
>>> -		ret = PTR_ERR(pb->pwm);
>>> -		goto err_alloc;
>>> -	} else
>>> -		dev_dbg(&pdev->dev, "got pwm for backlight\n");
>>> +	if (!pb->pwm) {
>>> +		pb->pwm = pwm_request(data->pwm_id, "backlight");
>>> +		if (IS_ERR(pb->pwm)) {
>>> +			dev_err(&pdev->dev, "unable to request PWM for backlight\n");
>>> +			ret = PTR_ERR(pb->pwm);
>>> +			goto err_alloc;
>>> +		} else
>>> +			dev_dbg(&pdev->dev, "got pwm for backlight\n");
>>> +	}
>>
>> Hmmm. It'd be more consistent if pwm_backlight_parse_dt() called
>> something like of_pwm_get() instead of of_pwm_request(), so that this
>> code could always call pwm_request() on the PWM and hence operate the
>> same irrespective of DT vs non-DT. GPIOs work that way at least.
> 
> That's actually what the initial patch had. Unfortunately that's pretty much
> the opposite direction of where the PWM framework is headed because it would
> involve getting a global index to request the PWM.

Not necessarily; get() could return a controller+index pair, which could
then be passed to request().

> I think in the long run it
> would be much better to get rid of pwm_request() altogether and unify by
> having the non-DT case request the PWM device on a per-chip basis.

That might also work.

Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Thierry Reding]

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

* Stephen Warren wrote:
> On 03/20/2012 02:39 AM, Thierry Reding wrote:
> > * Stephen Warren wrote:
> >> On 03/14/2012 09:56 AM, Thierry Reding wrote:
> >>> This commit adds very basic support for device tree probing. Currently,
> >>> only a PWM and a list of distinct brightness levels 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.
> ...
> >>>  static int pwm_backlight_probe(struct platform_device *pdev)
> >> ...
> >>> -	pb->pwm = pwm_request(data->pwm_id, "backlight");
> >>> -	if (IS_ERR(pb->pwm)) {
> >>> -		dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> >>> -		ret = PTR_ERR(pb->pwm);
> >>> -		goto err_alloc;
> >>> -	} else
> >>> -		dev_dbg(&pdev->dev, "got pwm for backlight\n");
> >>> +	if (!pb->pwm) {
> >>> +		pb->pwm = pwm_request(data->pwm_id, "backlight");
> >>> +		if (IS_ERR(pb->pwm)) {
> >>> +			dev_err(&pdev->dev, "unable to request PWM for backlight\n");
> >>> +			ret = PTR_ERR(pb->pwm);
> >>> +			goto err_alloc;
> >>> +		} else
> >>> +			dev_dbg(&pdev->dev, "got pwm for backlight\n");
> >>> +	}
> >>
> >> Hmmm. It'd be more consistent if pwm_backlight_parse_dt() called
> >> something like of_pwm_get() instead of of_pwm_request(), so that this
> >> code could always call pwm_request() on the PWM and hence operate the
> >> same irrespective of DT vs non-DT. GPIOs work that way at least.
> > 
> > That's actually what the initial patch had. Unfortunately that's pretty much
> > the opposite direction of where the PWM framework is headed because it would
> > involve getting a global index to request the PWM.
> 
> Not necessarily; get() could return a controller+index pair, which could
> then be passed to request().

This is pretty much what of_pwm_request() does internally (actually via the
of_xlate() function), and it goes one step further by requesting the
corresponding PWM.

I don't really like the fact that the DT parsing code needs to store a
requested PWM device in the platform data. The only other solution would be,
as you suggest, to obtain two indices and request based on them. However that
comes with a whole lot of problems of its own (race between getting the
controller index and the actual requesting of the PWM device, ...).

One other alternative could be to not pass the PWM device via platform data
but rather return it from the pwm_backlight_parse_dt() function (either via
the return value or an output parameter).

> > I think in the long run it
> > would be much better to get rid of pwm_request() altogether and unify by
> > having the non-DT case request the PWM device on a per-chip basis.
> 
> That might also work.

What I'm not sure about is how to represent this in the non-DT case. The
problem would be how to identify the individual PWM chips. From a quick
glance, pinctrl seems to do this purely on a name lookup basis, similar to
the clock framework.

What would be a good way to represent this in platform data?

Thierry

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

Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Stephen Warren]

On 03/20/2012 09:43 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/20/2012 02:39 AM, Thierry Reding wrote:
...
>>> I think in the long run it
>>> would be much better to get rid of pwm_request() altogether and unify by
>>> having the non-DT case request the PWM device on a per-chip basis.
>>
>> That might also work.
> 
> What I'm not sure about is how to represent this in the non-DT case. The
> problem would be how to identify the individual PWM chips. From a quick
> glance, pinctrl seems to do this purely on a name lookup basis, similar to
> the clock framework.
> 
> What would be a good way to represent this in platform data?

I think most get subsystems have a get API that works for both DT and
non-DT, rather than using platform data:

# works in DT right now IIRC:
regulator_get(dev, supply_name)

# patches posted to make this work in DT. Near checkin:
p = pinctrl_get(dev); s = pinctrl_lookup_state(p, state_name)

# patches posted to make this work in DT. I'm not sure how
# near they are to check-in.
clk_get(dev, clk_name)

That way, there could be a pwm_get(dev, name) for both non-DT (use some
table registered by board files) and DT (use the OF lookup code you've
written).

Admittedly, GPIOs don't work quite this way. Interrupts kinda do via the
platform resources.

For board files, this would rely on some table which mapped from the get
parameters to the returned object. IIRC, most of these tables are
indexed by (dev_name(dev), name) and probably return the dev_name() of
the providing device (or whatever internally makes sense to identify the
device) plus some subsystem-specific data (e.g. PWM index).

Re: [PATCH v4 10/10] pwm-backlight: Add rudimentary device tree support

[Mark Brown]

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

On Tue, Mar 20, 2012 at 09:56:09AM -0600, Stephen Warren wrote:
> On 03/20/2012 09:43 AM, Thierry Reding wrote:

> > What would be a good way to represent this in platform data?

> I think most get subsystems have a get API that works for both DT and
> non-DT, rather than using platform data:

Yes, please - having to have explict handling in drivers to select a
mechanism to look up stuff in the subsystem would be very sad.

> # works in DT right now IIRC:
> regulator_get(dev, supply_name)

Yes, that's totally transparent to the device.

> For board files, this would rely on some table which mapped from the get
> parameters to the returned object. IIRC, most of these tables are
> indexed by (dev_name(dev), name) and probably return the dev_name() of
> the providing device (or whatever internally makes sense to identify the
> device) plus some subsystem-specific data (e.g. PWM index).

That's how regulator and clk do it (I think pinctrl too, and IIO also
does this though it's in staging) - the consumer works with the struct
device and the name of the thing it's requesting and the things doing
the mapping (boards, SoCs, whatever) map this onto the actual device
with a lookup table which works in terms of dev_name() for the
requesting device.  We use dev_name() because many buses don't make a
struct device available until very late in system startup.

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

RE: [PATCH v4 00/10] Add PWM framework and device tree support.

[H Hartley Sweeten]

On Wednesday, March 14, 2012 8:56 AM, Thierry Reding wrote:
> 
> This patch series adds very rudimentary device-tree support for PWM
> devices. 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-bl
> driver and the backlight class.

Hello Thierry,

Are you planning on adding a sysfs interface for generic userspace access
to the PWM devices?

I have tested your patches (with a couple fixes already mentioned) on a
ep93xx board and have converted the drivers/misc/ep93xx_pwm.c driver
to use the framework.  Everything "seems" to work but my use for the
PWM is from userspace so I can't really test anything.

I have hacked in the sysfs interface from the ep93xx_pwm driver but it
is a hack... It's not really using the framework to control the PWM since
all the sysfs knobs are directly accessing the pwm registers.

Anyway, I would be happy to convert the current driver to the new framework
and get it out of drivers/misc and into a proper location. But, before I can do
that, I need to be able to control the PWM from userspace somehow.

Maybe an interface similar to the one gpiolib provides with CONFIG_GPIO_SYSFS?

Regards,
Hartley

Re: [PATCH v4 00/10] Add PWM framework and device tree support.

[Thierry Reding]

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

* H Hartley Sweeten wrote:
> Are you planning on adding a sysfs interface for generic userspace access
> to the PWM devices?

I wasn't planning to, but really only because I have no need for it. You
could of course add one yourself.

> Anyway, I would be happy to convert the current driver to the new framework
> and get it out of drivers/misc and into a proper location. But, before I can do
> that, I need to be able to control the PWM from userspace somehow.

You should probably coordinate with Sascha Hauer who I think already has most
(if not all) of the PWM drivers converted to his first version of the
framework and said he was going to rebase them on top of this series.

> Maybe an interface similar to the one gpiolib provides with CONFIG_GPIO_SYSFS?

I think the most challenging part will be figuring out how to export PWM
devices via sysfs on a per-chip basis. Since the goal of the series is to
eventually get rid of the global namespace, using an export file like gpiolib
won't work in the long run. Perhaps something like the following would:

	/sys/class/pwm/
		pwmchipX/
			export
			unexport
			pwmY/
				label
				duty
				period

That way you get a reference to the device associated with a PWM chip and you
can use the new pwm_request_from_device() function to request and export the
PWM device.

Again, I have no direct use for this and you probably know the requirements
better than me so it's probably better if you add the interface. I'm happy to
review patches, though.

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