Add PWM framework and device tree support.

+};

include/linux/pwm.h int pwm_enable(struct pwm_device *pwm); · line 61

Quoted 5 lines · Thierry Reding

[...]

include/linux/pwm.h +4 -0

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

+PPWM core

MAINTAINERS +5 -0

+M:	Sascha Hauer <[email protected]>
+L:	linux-kernel at vger.kernel.org
+S:	Maintained
+F:	drivers/pwm/
+

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

Quoted 1 line

+PPWM core

s/PPWM/PWM?

From: Sascha Hauer <[email protected]>

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

drivers/pwm/core.c +0 -0

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

Quoted 3 lines · Thierry Reding

From: Sascha Hauer <[email protected]>

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

...

Quoted 1 line

drivers/pwm/core.c +0 -0

...

Quoted 7 lines

+/**
+ * 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".

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 <[email protected]>

---

include/linux/pwm.h +0 -0

index df9681b..7261911 100644

/**
- * 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 {

include/linux/pwm.h +5 -6

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

On Wednesday, March 14, 2012 8:56 AM, Thierry Reding wrote:

include/linux/pwm.h void pwm_disable(struct pwm_device *pwm); · line 86

Quoted 4 lines

include/linux/pwm.h +4 -0

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

Quoted 1 line · H Hartley Sweeten

On Wednesday, March 14, 2012 8:56 AM, Thierry Reding wrote:

[...]

Quoted 7 lines

include/linux/pwm.h void pwm_disable(struct pwm_device *pwm); · line 86

Quoted 4 lines

include/linux/pwm.h +4 -0

+#ifdef CONFIG_DEBUG_FS
+	void			(*dbg_show)(struct pwm_chip *chip,
+					    struct seq_file *s);
+#endif

This doesn't compile...

Quoted 2 lines

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.

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 <[email protected]>

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

Documentation/devicetree/bindings/pwm/pwm.txt new file +48 -0

new file mode 100644
index 0000000..9421fe7
@@ -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 at 7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};

drivers/of/Kconfig +6 -0

index 6ea51dc..d47b8ee 100644
@@ -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

drivers/pwm/core.c +127 -0

index 74dd295..c600606 100644
@@ -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)
 {

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

new file mode 100644
index 0000000..a3a3da7
@@ -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;
+

include/linux/of_pwm.h line 29

Quoted 3 lines · Thierry Reding

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

+static inline struct pwm_device *of_pwm_request(struct device_node *np,
+						const char *propname,
+						int index);

                                                         ^^^

No semicolon here please.

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 <[email protected]>

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

drivers/of/Kconfig +0 -0

index 6ea51dc..d47b8ee 100644
@@ -57,6 +57,12 @@ config OF_GPIO
 	help
 	  OpenFirmware GPIO accessors

drivers/of/Kconfig +7 -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);
+

drivers/of/Kconfig +10 -0

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

drivers/of/Kconfig +11 -0

+#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.
+ */

/*

drivers/of/Kconfig +0 -0

@@ -102,6 +104,12 @@ struct pwm_chip {
 	unsigned int		npwm;
 

Quoted 2 lines · Thierry Reding

(quoted diff) +2 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

drivers/pwm/core.c +2 -0

+static struct pwm_device *of_pwm_simple_xlate(struct pwm_chip *pc,
+					      const struct of_phandle_args *args)

drivers/pwm/core.c +2 -0

+struct pwm_device *of_pwm_request(struct device_node *np,
+				  const char *propname, int index)

drivers/pwm/core.c +8 -0

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

Quoted 3 lines · 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.

...

Quoted 2 lines

drivers/pwm/core.c +2 -0

+static struct pwm_device *of_pwm_simple_xlate(struct pwm_chip *pc,
+					      const struct of_phandle_args *args)

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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 >= ?)

Quoted 2 lines

drivers/pwm/core.c +2 -0

+struct pwm_device *of_pwm_request(struct device_node *np,
+				  const char *propname, int index)

...

drivers/pwm/core.c void pwm_disable(struct pwm_device *pwm) · line 480

Quoted 8 lines

drivers/pwm/core.c +8 -0

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

From: Simon Que <[email protected]>

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

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

Signed-off-by Thierry Reding <[email protected]>

Signed-off-by Bill Huang <[email protected]>

Signed-off-by Simon Que <[email protected]>

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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

Quoted 6 lines · 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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

...

arch/arm/mach-tegra/tegra2_clocks.c static struct clk_duplicate tegra_clk_duplicates[] = { · line 2272

Quoted 5 lines

arch/arm/mach-tegra/tegra2_clocks.c +1 -4

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

Quoted 8 lines · Stephen Warren

Quoted 6 lines · 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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

...

Quoted 9 lines

arch/arm/mach-tegra/tegra2_clocks.c static struct clk_duplicate tegra_clk_duplicates[] = { · line 2272

Quoted 5 lines

arch/arm/mach-tegra/tegra2_clocks.c +1 -4

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

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 <[email protected]>

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

drivers/pwm/Kconfig +10 -0

index 93c1052..bda6f23 100644
@@ -9,4 +9,14 @@ menuconfig PWM
 
 if PWM
 
+config PWM_TEGRA
+	tristate "NVIDIA Tegra PWM support"
+	depends on ARCH_TEGRA
+	help
+	  Generic PWM framework driver for the PWFM controller found on NVIDIA
+	  Tegra SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-tegra.
+
 endif

drivers/pwm/Makefile +1 -0

index 3469c3d..12300f5 100644
@@ -1 +1,2 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o

drivers/pwm/pwm-tegra.c new file +69 -0

new file mode 100644
index 0000000..19540fc
@@ -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 <[email protected]>
+ *
+ * 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;

drivers/pwm/pwm-tegra.c new file +112 -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) {

drivers/pwm/pwm-tegra.c new file +41 -0

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

drivers/pwm/pwm-tegra.c new file +3 -0

+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("NVIDIA Corporation");

Signature · 6 lines

1.7.9.4

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 132 lines · 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 <[email protected]>

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

drivers/pwm/Kconfig +10 -0

index 93c1052..bda6f23 100644
@@ -9,4 +9,14 @@ menuconfig PWM
 
 if PWM
 
+config PWM_TEGRA
+	tristate "NVIDIA Tegra PWM support"
+	depends on ARCH_TEGRA
+	help
+	  Generic PWM framework driver for the PWFM controller found on NVIDIA
+	  Tegra SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-tegra.
+
 endif

drivers/pwm/Makefile +1 -0

index 3469c3d..12300f5 100644
@@ -1 +1,2 @@
 obj-$(CONFIG_PWM)		+= core.o
+obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o

drivers/pwm/pwm-tegra.c new file +69 -0

new file mode 100644
index 0000000..19540fc
@@ -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 <[email protected]>
+ *
+ * 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;

drivers/pwm/pwm-tegra.c line 79

Quoted 10 lines

drivers/pwm/pwm-tegra.c new file +10 -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?

Quoted 7 lines

drivers/pwm/pwm-tegra.c new file +7 -0

+	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;
+	}
+

drivers/pwm/pwm-tegra.c line 201

Quoted 6 lines

drivers/pwm/pwm-tegra.c new file +6 -0

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

drivers/pwm/pwm-tegra.c line 252

Quoted 8 lines

drivers/pwm/pwm-tegra.c new file +8 -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.

Quoted 1 line

drivers/pwm/pwm-tegra.c new file +1 -0

+module_platform_driver(tegra_pwm_driver);

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)

drivers/pwm/pwm-tegra.c +0 -0

drivers/pwm/pwm-tegra.c +5 -0

+static int __devexit tegra_pwm_remove(struct platform_device *pdev)
+{
+	struct tegra_pwm_chip *pwm = platform_get_drvdata(pdev);
+	int i;
+

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 <[email protected]>

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

Quoted 4 lines · 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.

Quoted 3 lines

Signed-off-by Thierry Reding <[email protected]>

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

Quoted 5 lines · Stephen Warren

Quoted 4 lines · 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.

Quoted 7 lines

Quoted 3 lines

Signed-off-by Thierry Reding <[email protected]>

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

Quoted 14 lines · Thierry Reding

Quoted 5 lines · Stephen Warren

Quoted 4 lines · 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.

Quoted 7 lines

Quoted 3 lines

Signed-off-by Thierry Reding <[email protected]>

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

Quoted 4 lines

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.

Quoted 7 lines · 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 <[email protected]>

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

Quoted 7 lines · Stephen Warren

Quoted 5 lines · 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 <[email protected]>

Quoted 3 lines

Quoted 1 line

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

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- update PWM ops for changes in patch 2

Quoted 4 lines · Thierry Reding

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- update PWM ops for changes in patch 2

Couple of quick notes, mostly for future work.

drivers/pwm/pwm-pxa.c line 98

Quoted 4 lines

drivers/pwm/pwm-pxa.c new file +4 -0

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

drivers/pwm/pwm-pxa.c line 101

Quoted 3 lines

drivers/pwm/pwm-pxa.c new file +3 -0

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

Quoted 18 lines · Ryan Mallon

Quoted 4 lines · Thierry Reding

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- update PWM ops for changes in patch 2

Couple of quick notes, mostly for future work.

drivers/pwm/pwm-pxa.c line 98

Quoted 4 lines

drivers/pwm/pwm-pxa.c new file +4 -0

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

Quoted 10 lines

drivers/pwm/pwm-pxa.c line 101

Quoted 3 lines

drivers/pwm/pwm-pxa.c new file +3 -0

+	__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?

Quoted 6 lines · Thierry Reding

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 8 lines · Sascha Hauer

Quoted 6 lines · Thierry Reding

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.

Quoted 5 lines

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?

Signed-off-by Thierry Reding <[email protected]>

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

drivers/pwm/pwm-pxa.c renamed from arch/arm/plat-pxa/pwm.c +5 -2

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

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

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

drivers/video/backlight/pwm_bl.c +4 -0

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

static int pwm_backlight_probe(struct platform_device *pdev)

drivers/video/backlight/pwm_bl.c +0 -6

-	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

Quoted 8 lines · 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.

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

Documentation/devicetree/bindings/video/backlight/pwm-backlight line 7

Quoted 7 lines

Documentation/devicetree/bindings/video/backlight/pwm-backlight new file +7 -0

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

Quoted 1 line

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

...

Quoted 6 lines

drivers/video/backlight/pwm_bl.c +4 -0

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

...

drivers/video/backlight/pwm_bl.c static const struct backlight_ops pwm_backlight_ops = { · line 138

Quoted 4 lines

drivers/video/backlight/pwm_bl.c +4 -0

+		ret = of_property_read_u32(node, "default-brightness-level",
+					   &value);
+		if (ret < 0)
+			goto free;

Range-check that against max_brightness?

Quoted 1 line

static int pwm_backlight_probe(struct platform_device *pdev)

...

Quoted 13 lines

drivers/video/backlight/pwm_bl.c +6 -7

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

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 228

Quoted 3 lines

drivers/video/backlight/pwm_bl.c +3 -0

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

Quoted 7 lines · Stephen Warren

Quoted 6 lines · 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.

Quoted 15 lines

Quoted 1 line

static int pwm_backlight_probe(struct platform_device *pdev)

...

Quoted 13 lines

drivers/video/backlight/pwm_bl.c +6 -7

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

Quoted 8 lines

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 228

Quoted 3 lines

drivers/video/backlight/pwm_bl.c +3 -0

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

Quoted 8 lines · Thierry Reding

Quoted 7 lines · Stephen Warren

Quoted 6 lines · 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.

...

Quoted 24 lines

Quoted 23 lines

Quoted 1 line

static int pwm_backlight_probe(struct platform_device *pdev)

...

Quoted 6 lines

drivers/video/backlight/pwm_bl.c +0 -6

-	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

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 228

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +9 -1

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

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 1 line · Stephen Warren

On 03/20/2012 02:39 AM, Thierry Reding wrote:

Quoted 4 lines

Quoted 3 lines

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.

Quoted 1 line

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?

Quoted 1 line · Thierry Reding

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)

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