linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/4] Runtime Interpreted Power Sequences
@ 2012-08-31 11:34 Alexandre Courbot
  2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Alexandre Courbot @ 2012-08-31 11:34 UTC (permalink / raw)
  To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann
  Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss, linux-pm, linux-doc, Alexandre Courbot

New revision taking (hopefully) all the feedback received from the previous
version into account. It's funny how a 30 lines patch to switch my backlight
turned into a 1500 lines patchset introducing a new framework. And in the end
my backlight switches just the same.

So anyway, the main updates are:

* Types of DT steps are now determined by a "type" property and not by the
  presence of a specific property.
* Power sequences and their resources are now encapsulated into a "set"
  structure. I was reluctant to this idea but have to admit now that it is way
  cleaner.
* GPIO steps now refer the GPIO phandle directly in the DT instead of by name.
  The GPIO framework does not work like regulator or PWM in that GPIOs cannot be
  accessed by name, so it did not make sense anyway. And thanks to that we now
  have perfect matching between the platform data members and the DT properties,
  which makes everything more consistent.
* Moved the implementations of resources into their own file (directly included
  from the main file) and added an "ops" structure to abstract them. This
  clearly separates the framework from the resources implementations and should
  make it easier to add new resources types.

Alexandre Courbot (4):
  Runtime Interpreted Power Sequences
  pwm_backlight: use power sequences
  tegra: dt: add label to tegra20's PWM
  tegra: ventana: add pwm backlight DT nodes

 .../devicetree/bindings/power_seq/power_seq.txt    | 117 ++++++
 .../bindings/video/backlight/pwm-backlight.txt     |  67 +++-
 Documentation/power/power_seq.txt                  | 225 +++++++++++
 arch/arm/boot/dts/tegra20-ventana.dts              |  59 ++-
 arch/arm/boot/dts/tegra20.dtsi                     |   2 +-
 drivers/power/Kconfig                              |   1 +
 drivers/power/Makefile                             |   1 +
 drivers/power/power_seq/Kconfig                    |   2 +
 drivers/power/power_seq/Makefile                   |   1 +
 drivers/power/power_seq/power_seq.c                | 446 +++++++++++++++++++++
 drivers/power/power_seq/power_seq_delay.c          |  51 +++
 drivers/power/power_seq/power_seq_gpio.c           |  81 ++++
 drivers/power/power_seq/power_seq_pwm.c            |  85 ++++
 drivers/power/power_seq/power_seq_regulator.c      |  86 ++++
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 179 ++++++---
 include/linux/power_seq.h                          | 174 ++++++++
 include/linux/pwm_backlight.h                      |  15 +-
 18 files changed, 1537 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
 create mode 100644 Documentation/power/power_seq.txt
 create mode 100644 drivers/power/power_seq/Kconfig
 create mode 100644 drivers/power/power_seq/Makefile
 create mode 100644 drivers/power/power_seq/power_seq.c
 create mode 100644 drivers/power/power_seq/power_seq_delay.c
 create mode 100644 drivers/power/power_seq/power_seq_gpio.c
 create mode 100644 drivers/power/power_seq/power_seq_pwm.c
 create mode 100644 drivers/power/power_seq/power_seq_regulator.c
 create mode 100644 include/linux/power_seq.h

-- 
1.7.12


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-08-31 11:34 [PATCH v5 0/4] Runtime Interpreted Power Sequences Alexandre Courbot
@ 2012-08-31 11:34 ` Alexandre Courbot
  2012-09-05 17:19   ` Stephen Warren
  2012-09-06 14:14   ` Heiko Stübner
  2012-08-31 11:34 ` [PATCH v5 2/4] pwm_backlight: use power sequences Alexandre Courbot
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 16+ messages in thread
From: Alexandre Courbot @ 2012-08-31 11:34 UTC (permalink / raw)
  To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann
  Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss, linux-pm, linux-doc, Alexandre Courbot

Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.

With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../devicetree/bindings/power_seq/power_seq.txt    | 117 ++++++
 Documentation/power/power_seq.txt                  | 225 +++++++++++
 drivers/power/Kconfig                              |   1 +
 drivers/power/Makefile                             |   1 +
 drivers/power/power_seq/Kconfig                    |   2 +
 drivers/power/power_seq/Makefile                   |   1 +
 drivers/power/power_seq/power_seq.c                | 446 +++++++++++++++++++++
 drivers/power/power_seq/power_seq_delay.c          |  51 +++
 drivers/power/power_seq/power_seq_gpio.c           |  81 ++++
 drivers/power/power_seq/power_seq_pwm.c            |  85 ++++
 drivers/power/power_seq/power_seq_regulator.c      |  86 ++++
 include/linux/power_seq.h                          | 174 ++++++++
 12 files changed, 1270 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
 create mode 100644 Documentation/power/power_seq.txt
 create mode 100644 drivers/power/power_seq/Kconfig
 create mode 100644 drivers/power/power_seq/Makefile
 create mode 100644 drivers/power/power_seq/power_seq.c
 create mode 100644 drivers/power/power_seq/power_seq_delay.c
 create mode 100644 drivers/power/power_seq/power_seq_gpio.c
 create mode 100644 drivers/power/power_seq/power_seq_pwm.c
 create mode 100644 drivers/power/power_seq/power_seq_regulator.c
 create mode 100644 include/linux/power_seq.h

diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 0000000..d3e3f6a
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,117 @@
+Runtime Interpreted Power Sequences
+===================================
+
+Power sequences are sequential descriptions of actions to be performed on
+power-related resources. Having these descriptions in a precise data format
+allows us to take much of the board-specific power control code out of the
+kernel and place it into the device tree instead, making kernels less
+board-dependant.
+
+In the device tree, power sequences are grouped into a set. The set is always
+declared as the "power-sequences" sub-node of the device node. Power sequences
+may reference resources declared by that device.
+
+Power Sequences Structure
+-------------------------
+Every device that makes use of power sequences must have a "power-sequences"
+sub-node. Power sequences are sub-nodes of this set node, and their node name
+indicates the id of the sequence.
+
+Every power sequence in turn contains its steps as sub-nodes of itself. Step
+must be named sequentially, with the first step named step0, the second step1,
+etc. Failure to follow this rule will result in a parsing error.
+
+Power Sequences Steps
+---------------------
+Step of a sequence describes an action to be performed on a resource. They
+always include a "type" property which indicates what kind of resource this
+step works on. Depending on the resource type, additional properties are defined
+to control the action to be performed.
+
+"delay" type required properties:
+  - delay_us: delay to wait in microseconds
+
+"regulator" type required properties:
+  - id: name of the regulator to use. Regulator is obtained by
+        regulator_get(dev, id)
+  - enable / disable: one of these two empty properties must be present to
+                      enable or disable the resource
+
+"pwm" type required properties:
+  - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
+  - enable / disable: one of these two empty properties must be present to
+                      enable or disable the resource
+
+"gpio" type required properties:
+  - number: phandle of the GPIO to use.
+  - enable / disable: one of these two empty properties must be present to
+                      enable or disable the resource
+
+Example
+-------
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+	backlight {
+		compatible = "pwm-backlight";
+		...
+
+		/* resources used by the power sequences */
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+
+		power-sequences {
+			power-on {
+				step0 {
+					type = "regulator";
+					id = "power";
+					enable;
+				};
+				step1 {
+					type = "delay";
+					delay_us = <10000>;
+				};
+				step2 {
+					type = "pwm";
+					id = "backlight";
+					enable;
+				};
+				step3 {
+					type = "gpio";
+					number = <&gpio 28 0>;
+					enable;
+				};
+			};
+
+			power-off {
+				step0 {
+					type = "gpio";
+					number = <&gpio 28 0>;
+					disable;
+				};
+				step1 {
+					type = "pwm";
+					id = "backlight";
+					disable;
+				};
+				step2 {
+					type = "delay";
+					delay_us = <10000>;
+				};
+				step3 {
+					type = "regulator";
+					id = "power";
+					disable;
+				};
+			};
+		};
+	};
+
+The first part lists the PWM and regulator resources used by the sequences.
+These resources will be requested on behalf of the backlight device when the
+sequences are built and are declared according to their own framework in a way
+that makes them accessible by name.
+
+After the resources declaration, two sequences follow for powering the backlight
+on and off. Their names are specified by the pwm-backlight driver.
diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
new file mode 100644
index 0000000..48d1f6b
--- /dev/null
+++ b/Documentation/power/power_seq.txt
@@ -0,0 +1,225 @@
+Runtime Interpreted Power Sequences
+===================================
+
+Problem
+-------
+Very commonly, boards need the help of out-of-driver code to turn some of their
+devices on and off. For instance, SoC boards very commonly use a GPIO
+(abstracted to a regulator or not) to control the power supply of a backlight,
+disabling it when the backlight is not used in order to save power. The GPIO
+that should be used, however, as well as the exact power sequence that may
+also involve other resources, is board-dependent and thus unknown to the driver.
+
+This was previously addressed by having hooks in the device's platform data that
+are called whenever the state of the device might reflect a power change. This
+approach, however, introduces board-dependant code into the kernel and is not
+compatible with the device tree.
+
+The Runtime Interpreted Power Sequences (or power sequences for short) aim at
+turning this code into platform data or device tree nodes. Power sequences are
+described using a simple format and run by a lightweight interpreter whenever
+needed. This allows to remove the callback mechanism and makes the kernel less
+board-dependant.
+
+What are Power Sequences?
+-------------------------
+Power sequences are a series of sequential steps during which an action is
+performed on a resource. The supported resources and actions operations are:
+- delay (just wait for a given number of microseconds)
+- GPIO (enable or disable)
+- regulator (enable or disable)
+- PWM (enable or disable)
+
+When a power sequence is run, each of its steps is executed sequentially until
+one step fails or the end of the sequence is reached.
+
+Power sequences are grouped in "sets" and declared per-device. Every sequence
+must be attributed a name that can be used to retrieve it from its set when it
+is needed.
+
+Power sequences can be declared as platform data or in the device tree.
+
+Platform Data Format
+--------------------
+All relevant data structures for declaring power sequences are located in
+include/linux/power_seq.h.
+
+The platform data for a given device is an instance of platform_power_seq_set
+which points to instances of platform_power_seq. Every platform_power_seq is a
+single power sequence, and is itself composed of a variable length array of
+steps.
+
+A step is a union of all the step structures. Which one is to be used depends on
+the type of the step. Step structures are documented in the
+include/linux/power_seq.h file ; please refer to it for all details, but the
+following example will probably make it clear how power sequences should be
+defined. It defines two power sequences named "power_on" and "power_off". The
+"power_on" sequence enables a regulator called "power" (retrieved from the
+device using regulator_get()), waits for 10ms, and then enabled GPIO 110.
+"power_off" does the opposite.
+
+static struct platform_power_seq power_on_seq = {
+	.id = "power_on",
+	.num_steps = 3,
+	.steps = {
+		{
+			.type = POWER_SEQ_REGULATOR,
+			.regulator = {
+				.id = "power",
+				.enable = true,
+			},
+		},
+		{
+			.type = POWER_SEQ_DELAY,
+			.delay = {
+				.delay_us = 10000,
+			},
+		},
+		{
+			.type = POWER_SEQ_GPIO,
+			.gpio = {
+				.number = 110,
+				.enable = true,
+			},
+		},
+	},
+};
+
+static struct platform_power_seq power_off_seq = {
+	.id = "power_off",
+	.num_steps = 3,
+	.steps = {
+		{
+			.type = POWER_SEQ_GPIO,
+			.gpio = {
+				.number = 110,
+				.enable = false,
+			},
+		},
+		{
+			.type = POWER_SEQ_DELAY,
+			.delay = {
+				.delay_us = 10000,
+			},
+		},
+		{
+			.type = POWER_SEQ_REGULATOR,
+			.regulator = {
+				.id = "power",
+				.enable = false,
+			},
+		},
+	},
+};
+
+static struct platform_power_seq_set power_sequences = {
+	.num_seqs = 2,
+	.seqs = {
+		&power_on_seq,
+		&power_off_seq,
+	},
+};
+
+Device Tree
+-----------
+Power sequences can also be encoded as device tree nodes. The following
+properties and nodes are equivalent to the platform data defined previously:
+
+power-supply = <&power_reg>;
+
+power-sequences {
+	power-on {
+		step0 {
+			type = "regulator";
+			id = "power";
+			enable;
+		};
+		step1 {
+			type = "delay";
+			delay = <10000>;
+		};
+		step2 {
+			type = "gpio";
+			number = <&gpio 110 0>;
+			enable;
+		};
+	}
+	power-off {
+		step0 {
+			type = "gpio";
+			number = <&gpio 110 0>;
+			disable;
+		};
+		step1 {
+			type = "delay";
+			delay = <10000>;
+		};
+		step2 {
+			type = "regulator";
+			id = "power";
+			disable;
+		};
+	}
+};
+
+See Documentation/devicetree/bindings/power_seq/power_seq.txt for the complete
+syntax of the bindings.
+
+Usage by Drivers and Resources Management
+-----------------------------------------
+Power sequences make use of resources that must be properly allocated and
+managed. The devm_power_seq_set_build() function builds a power sequence set
+from platform data. It also takes care of resolving and allocating the resources
+referenced by the sequence:
+
+  struct power_seq_set *devm_power_seq_set_build(struct device *dev,
+					   struct platform_power_seq_set *pseq);
+
+As its name states, all memory and resources are devm-allocated. The 'dev'
+argument is the device in the name of which the resources are to be allocated.
+
+On success, the function returns a devm allocated resolved sequences set for
+which all the resources are allocated. In case of failure, an error code is
+returned.
+
+Power sequences can then be retrieved by their name using power_seq_lookup:
+
+  struct power_seq *power_seq_lookup(struct power_seq_set *seqs,
+				     const char *id);
+
+A power sequence can be executed by power_seq_run:
+
+  int power_seq_run(struct power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code if a
+problem occured.
+
+Sometimes, you may want to browse the list of resources allocated by a sequence,
+to for instance ensure that a resource of a given type is present. The
+power_seq_set_resources() function returns a list head that can be used with
+the power_seq_for_each_resource() macro to browse all the resources of a set:
+
+  struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
+  power_seq_for_each_resource(pos, seqs)
+
+Here "pos" will be of type struct power_seq_resource. This structure contains a
+"pdata" pointer that can be used to explore the platform data of this resource,
+as well as the resolved resource, if applicable.
+
+Finally, users of the device tree can build the platform data corresponding to
+the tree node using this function:
+
+  struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);
+
+As the device tree syntax unambiguously states the name of the node containing
+the power sequences, it only needs a pointer to the device to work. The result
+can then be passed to devm_power_seq_set_build() in order to get a set of
+runnable sequences.
+
+devm_of_parse_power_seq_set allocates its memory using devm, but the platform
+data becomes unneeded after devm_power_seq_set_build() is called on it and can
+thus be freed. Be aware though that one allocation is performed for the set and
+for every sequence. The devm_power_seq_platform_data_free() function takes care
+of freeing the memory properly:
+
+  void devm_platform_power_seq_set_free(struct platform_power_seq_set *pseq);
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index fcc1bb0..5fdfd84 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -312,3 +312,4 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
 endif # POWER_SUPPLY
 
 source "drivers/power/avs/Kconfig"
+source "drivers/power/power_seq/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index ee58afb..d3c893b 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
 obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_POWER_AVS)		+= avs/
 obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
+obj-$(CONFIG_POWER_SEQ)		+= power_seq/
diff --git a/drivers/power/power_seq/Kconfig b/drivers/power/power_seq/Kconfig
new file mode 100644
index 0000000..3bff26e
--- /dev/null
+++ b/drivers/power/power_seq/Kconfig
@@ -0,0 +1,2 @@
+config POWER_SEQ
+	bool
diff --git a/drivers/power/power_seq/Makefile b/drivers/power/power_seq/Makefile
new file mode 100644
index 0000000..f77a359
--- /dev/null
+++ b/drivers/power/power_seq/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_POWER_SEQ)		+= power_seq.o
diff --git a/drivers/power/power_seq/power_seq.c b/drivers/power/power_seq/power_seq.c
new file mode 100644
index 0000000..e4d482c
--- /dev/null
+++ b/drivers/power/power_seq/power_seq.c
@@ -0,0 +1,446 @@
+/*
+ * power_seq.c - A simple power sequence interpreter for platform devices
+ *               and device tree.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/power_seq.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+
+#include <linux/of.h>
+
+struct power_seq_set {
+	struct device *dev;
+	struct list_head resources;
+	struct list_head sequences;
+};
+
+struct power_seq_step {
+	/* Copy of the platform data */
+	struct platform_power_seq_step pdata;
+	/* Resolved resource */
+	struct power_seq_resource *resource;
+};
+
+struct power_seq {
+	/* Set this sequence belongs to */
+	struct power_seq_set *parent_set;
+	const char *id;
+	/* To thread into power_seqs structure */
+	struct list_head list;
+	unsigned int num_steps;
+	struct power_seq_step steps[];
+};
+
+#define power_seq_err(dev, seq, step_nbr, format, ...)			     \
+	dev_err(dev, "%s[%d]: " format, seq->id, step_nbr, ##__VA_ARGS__);
+
+/**
+ * struct power_seq_res_type - operators for power sequences resources
+ * @name:		Name of the resource type. Set to null when a resource
+ *			type support is not compiled in
+ * @need_resource:	Whether a resource needs to be allocated when steps of
+ *			this kind are met. If set to false, res_compare and
+ *			res_alloc need not be set
+ * @of_parse:		Parse a step for this kind of resource from a device
+ *			tree node. The result of parsing must be written into
+ *			step step_nbr of seq
+ * @step_run:		Run a step for this kind of resource
+ * @res_compare:	Return true if the resource used by both steps is the
+ *			same, false otherwise
+ * @res_alloc:		Resolve and allocate the resource passed from seq
+ *			Return error code if the resource cannot be allocated
+ */
+struct power_seq_res_ops {
+	const char *name;
+	bool need_resource;
+	int (*of_parse)(struct device *dev, struct device_node *node,
+			struct platform_power_seq *seq, unsigned int step_nbr);
+	int (*step_run)(struct power_seq_step *step);
+	bool (*res_compare)(struct platform_power_seq_step *step1,
+			    struct platform_power_seq_step *step2);
+	int (*res_alloc)(struct device *dev, struct power_seq_resource *seq);
+};
+
+static const struct power_seq_res_ops power_seq_types[POWER_SEQ_NUM_TYPES];
+
+#ifdef CONFIG_OF
+static int of_power_seq_parse_enable_properties(struct device *dev,
+						struct device_node *node,
+						struct platform_power_seq *seq,
+						unsigned int step_nbr,
+						bool *enable)
+{
+	if (of_find_property(node, "enable", NULL)) {
+		*enable = true;
+	} else if (of_find_property(node, "disable", NULL)) {
+		*enable = false;
+	} else {
+		power_seq_err(dev, seq, step_nbr, 
+			      "missing enable or disable property\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int of_power_seq_parse_step(struct device *dev, struct device_node *node,
+				   struct platform_power_seq *seq,
+				   unsigned int step_nbr)
+{
+	struct platform_power_seq_step *step = &seq->steps[step_nbr];
+	const char *type;
+	int i, err;
+
+	err = of_property_read_string(node, "type", &type);
+	if (err < 0) {
+		power_seq_err(dev, seq, step_nbr,
+			      "cannot read type property\n");
+		return err;
+	}
+	for (i = 0; i < POWER_SEQ_NUM_TYPES; i++) {
+		if (power_seq_types[i].name == NULL)
+			continue;
+		if (!strcmp(type, power_seq_types[i].name))
+			break;
+	}
+	if (i >= POWER_SEQ_NUM_TYPES) {
+		power_seq_err(dev, seq, step_nbr, "unknown type %s\n", type);
+		return -EINVAL;
+	}
+	step->type = i;
+	err = power_seq_types[step->type].of_parse(dev, node, seq, step_nbr);
+
+	return err;
+}
+
+static struct platform_power_seq *of_parse_power_seq(struct device *dev,
+						     struct device_node *node)
+{
+	struct device_node *child = NULL;
+	struct platform_power_seq *pseq;
+	int num_steps, sz;
+	int err;
+
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	num_steps = of_get_child_count(node);
+	sz = sizeof(*pseq) + sizeof(pseq->steps[0]) * num_steps;
+	pseq = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!pseq)
+		return ERR_PTR(-ENOMEM);
+	pseq->num_steps = num_steps;
+	pseq->id = node->name;
+
+	for_each_child_of_node(node, child) {
+		unsigned int pos;
+
+		/* Check that the name's format is correct and within bounds */
+		if (strncmp("step", child->name, 4)) {
+			err = -EINVAL;
+			goto parse_error;
+		}
+
+		err = kstrtouint(child->name + 4, 10, &pos);
+		if (err < 0)
+			goto parse_error;
+
+		if (pos >= num_steps || pseq->steps[pos].type != 0) {
+			err = -EINVAL;
+			goto parse_error;
+		}
+
+		err = of_power_seq_parse_step(dev, child, pseq, pos);
+		if (err)
+			return ERR_PTR(err);
+	}
+
+	return pseq;
+
+parse_error:
+	dev_err(dev, "%s: invalid power step name %s!\n", pseq->id,
+		child->name);
+	return ERR_PTR(err);
+}
+
+/**
+ * of_parse_power_seq_set() - build platform data corresponding to a DT node
+ * @dev:	Device on behalf of which the sequence is to be built
+ *
+ * Sequences must be contained into a subnode named "power-sequences" of the
+ * device root node.
+ *
+ * Memory for the platform sequence is allocated using devm_kzalloc on dev and
+ * can be freed by devm_kfree after power_seq_set_build returned. Beware that on
+ * top of the set itself, platform data for individual sequences should also be
+ * freed.
+ *
+ * Returns the built power sequence set on success, or an error code in case of
+ * failure.
+ */
+struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev)
+{
+	struct platform_power_seq_set *seqs;
+	struct device_node *root = dev->of_node;
+	struct device_node *seq;
+	int num_seqs, sz, i = 0;
+
+	if (!root)
+		return NULL;
+
+	root = of_find_node_by_name(root, "power-sequences");
+	if (!root)
+		return NULL;
+
+	num_seqs = of_get_child_count(root);
+	sz = sizeof(*seqs) + sizeof(seqs->seqs[0]) * num_seqs;
+	seqs = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!seqs)
+		return ERR_PTR(-ENOMEM);
+	seqs->num_seqs = num_seqs;
+
+	for_each_child_of_node(root, seq) {
+		struct platform_power_seq *pseq;
+
+		pseq = of_parse_power_seq(dev, seq);
+		if (IS_ERR(pseq))
+			return (void *)pseq;
+
+		seqs->seqs[i++] = pseq;
+	}
+
+	return seqs;
+}
+EXPORT_SYMBOL_GPL(devm_of_parse_power_seq_set);
+#endif /* CONFIG_OF */
+
+/**
+ * devm_platform_power_seq_set_free() - free data allocated by of_parse_power_seq_set
+ * @pseq:	Platform data to free
+ *
+ * This function can be called *only* on data returned by of_parse_power_seq_set
+ * and *after* devm_power_seq_set_build has been called on it.
+ */
+void devm_platform_power_seq_set_free(struct device *dev,
+				      struct platform_power_seq_set *pseq)
+{
+	int i;
+
+	for (i = 0; i < pseq->num_seqs; i++)
+		devm_kfree(dev, pseq->seqs[i]);
+
+	devm_kfree(dev, pseq);
+}
+EXPORT_SYMBOL_GPL(devm_platform_power_seq_set_free);
+
+static struct power_seq_resource *
+power_seq_find_resource(struct list_head *ress,
+			struct platform_power_seq_step *step)
+{
+	struct power_seq_resource *res;
+
+	list_for_each_entry(res, ress, list) {
+		struct platform_power_seq_step *pdata = res->pdata;
+
+		if (pdata->type != step->type)
+			continue;
+
+		if (power_seq_types[pdata->type].res_compare(pdata, step))
+			return res;
+	}
+
+	return NULL;
+}
+
+static struct power_seq *power_seq_build_one(struct device *dev,
+					     struct power_seq_set *seqs,
+					     struct platform_power_seq *pseq)
+{
+	struct power_seq *seq;
+	struct power_seq_resource *res;
+	int i, err;
+
+	seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(seq->steps[0]) *
+			   pseq->num_steps, GFP_KERNEL);
+	if (!seq)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&seq->list);
+	seq->parent_set = seqs;
+	seq->num_steps = pseq->num_steps;
+	seq->id = pseq->id;
+
+	for (i = 0; i < seq->num_steps; i++) {
+		struct platform_power_seq_step *pstep = &pseq->steps[i];
+		struct power_seq_step *step = &seq->steps[i];
+
+		if (pstep->type >= POWER_SEQ_NUM_TYPES ||
+		    power_seq_types[pstep->type].name == NULL) {
+			power_seq_err(dev, seq, i,
+				      "invalid power sequence type %d!",
+		 		      pstep->type);
+			return ERR_PTR(-EINVAL);
+		}
+
+		memcpy(&step->pdata, pstep, sizeof(step->pdata));
+
+		/* Steps without resource need not to continue */
+		if (!power_seq_types[pstep->type].need_resource)
+			continue;
+
+		/* create resource node if not referenced already */
+		res = power_seq_find_resource(&seqs->resources, pstep);
+		if (!res) {
+			res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+			if (!res)
+				return ERR_PTR(-ENOMEM);
+
+			res->pdata = &step->pdata;
+
+			err = power_seq_types[res->pdata->type].res_alloc(dev, res);
+			if (err < 0) {
+				power_seq_err(dev, seq, i,
+					      "error building sequence\n");
+				return ERR_PTR(err);
+			}
+
+			list_add_tail(&res->list, &seqs->resources);
+		}
+		step->resource = res;
+	}
+
+	return seq;
+}
+
+/**
+ * power_seq_set_build() - build a set of runnable sequences from platform data
+ * @dev:	Device that will use the power sequences. All resources will be
+ *		devm-allocated against it
+ * @pseq:	Platform data for the power sequences. It can be freed after
+ *		this function returns
+ *
+ * All memory and resources (regulators, GPIOs, etc.) are allocated using devm
+ * functions.
+ *
+ * Returns the built sequence on success, an error code in case or failure.
+ */
+struct power_seq_set *devm_power_seq_set_build(struct device *dev,
+					    struct platform_power_seq_set *pseq)
+{
+	struct power_seq_set *seqs;
+	int i;
+
+	seqs = devm_kzalloc(dev, sizeof(*seqs), GFP_KERNEL);
+
+	if (!seqs)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&seqs->resources);
+	INIT_LIST_HEAD(&seqs->sequences);
+	for (i = 0; i < pseq->num_seqs; i++) {
+		struct power_seq *seq;
+
+		seq = power_seq_build_one(dev, seqs, pseq->seqs[i]);
+		if (IS_ERR(seq))
+			return (void *)seq;
+
+		list_add_tail(&seq->list, &seqs->sequences);
+	}
+
+	return seqs;
+}
+EXPORT_SYMBOL_GPL(devm_power_seq_set_build);
+
+/**
+ * power_seq_lookup - Lookup a power sequence by name from a set
+ * @seqs:	The set to look in
+ * @id:		Name to look after
+ *
+ * Returns a matching power sequence if it exists, NULL if it does not.
+ */
+struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char *id)
+{
+	struct power_seq *seq;
+
+	list_for_each_entry(seq, &seqs->sequences, list) {
+		if (!strcmp(seq->id, id))
+			return seq;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(power_seq_lookup);
+
+/**
+ * power_seq_set_resources - return a list of all the resources used by a set
+ * @seqs:	Power sequences set we are interested in getting the resources
+ *
+ * The returned list can be parsed using the power_seq_for_each_resource macro.
+ */
+struct list_head *power_seq_set_resources(struct power_seq_set *seqs)
+{
+	return &seqs->resources;
+}
+EXPORT_SYMBOL_GPL(power_seq_set_resources);
+
+/**
+ * power_seq_run() - run a power sequence
+ * @seq:	The power sequence to run
+ *
+ * Returns 0 on success, error code in case of failure.
+ */
+int power_seq_run(struct power_seq *seq)
+{
+	unsigned int i;
+	int err;
+
+	if (!seq)
+		return 0;
+
+	for (i = 0; i < seq->num_steps; i++) {
+		unsigned int type = seq->steps[i].pdata.type;
+
+		err = power_seq_types[type].step_run(&seq->steps[i]);
+		if (err) {
+			power_seq_err(seq->parent_set->dev, seq, i,
+				"error %d while running power sequence step\n",
+				err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_seq_run);
+
+#include "power_seq_delay.c"
+#include "power_seq_regulator.c"
+#include "power_seq_pwm.c"
+#include "power_seq_gpio.c"
+
+static const struct power_seq_res_ops power_seq_types[POWER_SEQ_NUM_TYPES] = {
+	[POWER_SEQ_DELAY] = POWER_SEQ_DELAY_TYPE,
+	[POWER_SEQ_REGULATOR] = POWER_SEQ_REGULATOR_TYPE,
+	[POWER_SEQ_PWM] = POWER_SEQ_PWM_TYPE,
+	[POWER_SEQ_GPIO] = POWER_SEQ_GPIO_TYPE,
+};
+
+MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
+MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
+MODULE_LICENSE("GPL");
diff --git a/drivers/power/power_seq/power_seq_delay.c b/drivers/power/power_seq/power_seq_delay.c
new file mode 100644
index 0000000..072bf50
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_delay.c
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/delay.h>
+
+#ifdef CONFIG_OF
+static int of_power_seq_parse_delay(struct device *dev,
+				    struct device_node *node,
+				    struct platform_power_seq *seq,
+				    unsigned int step_nbr)
+{
+	struct platform_power_seq_step *step = &seq->steps[step_nbr];
+	int err;
+
+	err = of_property_read_u32(node, "delay_us",
+				   &step->delay.delay_us);
+	if (err < 0)
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading delay_us property\n");
+
+	return err;
+}
+#else
+#define of_power_seq_parse_delay NULL
+#endif
+
+static int power_seq_step_run_delay(struct power_seq_step *step)
+{
+	usleep_range(step->pdata.delay.delay_us,
+		     step->pdata.delay.delay_us + 1000);
+
+	return 0;
+}
+
+#define POWER_SEQ_DELAY_TYPE {			\
+	.name = "delay",			\
+	.need_resource = false,			\
+	.of_parse = of_power_seq_parse_delay,	\
+	.step_run = power_seq_step_run_delay,	\
+}
diff --git a/drivers/power/power_seq/power_seq_gpio.c b/drivers/power/power_seq/power_seq_gpio.c
new file mode 100644
index 0000000..2e9a49f
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_gpio.c
@@ -0,0 +1,81 @@
+/*
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#ifdef CONFIG_OF
+static int power_seq_of_parse_gpio(struct device *dev,
+				   struct device_node *node,
+				   struct platform_power_seq *seq,
+				   unsigned int step_nbr)
+{
+	struct platform_power_seq_step *step = &seq->steps[step_nbr];
+	int gpio;
+	int err;
+
+	gpio = of_get_named_gpio(node, "number", 0);
+	if (gpio < 0) {
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading number property\n");
+		return gpio;
+	}
+	step->gpio.number = gpio;
+
+	err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
+					     &step->gpio.enable);
+
+	return err;
+}
+#else
+#define of_power_seq_parse_gpio NULL
+#endif
+
+static bool power_seq_res_compare_gpio(struct platform_power_seq_step *step1,
+				       struct platform_power_seq_step *step2)
+{
+	return step1->gpio.number == step2->gpio.number;
+}
+
+static int power_seq_res_alloc_gpio(struct device *dev,
+					struct power_seq_resource *res)
+{
+	int err;
+
+	err = devm_gpio_request_one(dev, res->pdata->gpio.number,
+				    GPIOF_OUT_INIT_LOW, dev_name(dev));
+	if (err) {
+		dev_err(dev, "cannot get gpio %d\n", res->pdata->gpio.number);
+		return err;
+	}
+
+	return 0;
+}
+
+static int power_seq_step_run_gpio(struct power_seq_step *step)
+{
+	gpio_set_value_cansleep(step->pdata.gpio.number,
+				step->pdata.gpio.enable);
+
+	return 0;
+}
+
+#define POWER_SEQ_GPIO_TYPE {					\
+	.name = "gpio",					\
+	.need_resource = true,				\
+	.of_parse = power_seq_of_parse_gpio,		\
+	.step_run = power_seq_step_run_gpio,		\
+	.res_compare = power_seq_res_compare_gpio,	\
+	.res_alloc = power_seq_res_alloc_gpio,		\
+}
diff --git a/drivers/power/power_seq/power_seq_pwm.c b/drivers/power/power_seq/power_seq_pwm.c
new file mode 100644
index 0000000..a80514f
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_pwm.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#ifdef CONFIG_PWM
+
+#include <linux/pwm.h>
+
+#ifdef CONFIG_OF
+static int power_seq_of_parse_pwm(struct device *dev,
+				  struct device_node *node,
+				  struct platform_power_seq *seq,
+				  unsigned int step_nbr)
+{
+	struct platform_power_seq_step *step = &seq->steps[step_nbr];
+	int err;
+
+	err = of_property_read_string(node, "id",
+				      &step->pwm.id);
+	if (err) {
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading id property\n");
+		return err;
+	}
+
+	err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
+						   &step->pwm.enable);
+	return err;
+}
+#else
+#define of_power_seq_parse_pwm NULL
+#endif
+
+static bool power_seq_res_compare_pwm(struct platform_power_seq_step *step1,
+				      struct platform_power_seq_step *step2)
+{
+	return (!strcmp(step1->pwm.id, step2->pwm.id));
+}
+
+static int power_seq_res_alloc_pwm(struct device *dev,
+				  struct power_seq_resource *res)
+{
+	res->pwm = devm_pwm_get(dev, res->pdata->pwm.id);
+	if (IS_ERR(res->pwm)) {
+		dev_err(dev, "cannot get pwm \"%s\"\n", res->pdata->pwm.id);
+		return PTR_ERR(res->pwm);
+	}
+
+	return 0;
+}
+
+static int power_seq_step_run_pwm(struct power_seq_step *step)
+{
+	if (step->pdata.gpio.enable) {
+		return pwm_enable(step->resource->pwm);
+	} else {
+		pwm_disable(step->resource->pwm);
+		return 0;
+	}
+}
+
+#define POWER_SEQ_PWM_TYPE {				\
+	.name = "pwm",					\
+	.need_resource = true,				\
+	.of_parse = power_seq_of_parse_pwm,		\
+	.step_run = power_seq_step_run_pwm,		\
+	.res_compare = power_seq_res_compare_pwm,	\
+	.res_alloc = power_seq_res_alloc_pwm,		\
+}
+
+#else
+
+#define POWER_SEQ_PWM_TYPE {}
+
+#endif
diff --git a/drivers/power/power_seq/power_seq_regulator.c b/drivers/power/power_seq/power_seq_regulator.c
new file mode 100644
index 0000000..915eac1
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_regulator.c
@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#ifdef CONFIG_REGULATOR
+
+#include <linux/regulator/consumer.h>
+
+/* TODO change "which" */
+#ifdef CONFIG_OF
+static int power_seq_of_parse_regulator(struct device *dev,
+					struct device_node *node,
+					struct platform_power_seq *seq,
+					unsigned int step_nbr)
+{
+	struct platform_power_seq_step *step = &seq->steps[step_nbr];
+	int err;
+
+	err = of_property_read_string(node, "id",
+				      &step->regulator.id);
+	if (err) {
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading id property\n");
+		return err;
+	}
+
+	err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
+						   &step->regulator.enable);
+	return err;
+}
+#else
+#define of_power_seq_parse_regulator NULL
+#endif
+
+static bool
+power_seq_res_compare_regulator(struct platform_power_seq_step *step1,
+				struct platform_power_seq_step *step2)
+{
+	return (!strcmp(step1->regulator.id, step2->regulator.id));
+}
+
+static int power_seq_res_alloc_regulator(struct device *dev,
+					struct power_seq_resource *res)
+{
+	res->regulator = devm_regulator_get(dev, res->pdata->regulator.id);
+	if (IS_ERR(res->regulator)) {
+		dev_err(dev, "cannot get regulator \"%s\"\n",
+			res->pdata->regulator.id);
+		return PTR_ERR(res->regulator);
+	}
+
+	return 0;
+}
+
+static int power_seq_step_run_regulator(struct power_seq_step *step)
+{
+	if (step->pdata.regulator.enable)
+		return regulator_enable(step->resource->regulator);
+	else
+		return regulator_disable(step->resource->regulator);
+}
+
+#define POWER_SEQ_REGULATOR_TYPE {			\
+	.name = "regulator",				\
+	.need_resource = true,				\
+	.of_parse = power_seq_of_parse_regulator,	\
+	.step_run = power_seq_step_run_regulator,	\
+	.res_compare = power_seq_res_compare_regulator,	\
+	.res_alloc = power_seq_res_alloc_regulator,	\
+}
+
+#else
+
+#define POWER_SEQ_REGULATOR_TYPE {}
+
+#endif
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..78e8d77
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,174 @@
+/*
+ * power_seq.h
+ *
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties.
+ *
+ * Power sequences are designed to replace the callbacks typically used in
+ * board-specific files that implement board-specific power sequences of devices
+ * such as backlights. A power sequence is an array of resources (which can a
+ * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
+ * disable) and optional pre and post step delays. By having them interpreted
+ * instead of arbitrarily executed, it is possible to describe these in the
+ * device tree and thus remove board-specific code from the kernel.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/types.h>
+
+struct device;
+struct regulator;
+struct pwm_device;
+struct device_node;
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+enum power_seq_res_type {
+	POWER_SEQ_DELAY,
+	POWER_SEQ_REGULATOR,
+	POWER_SEQ_PWM,
+	POWER_SEQ_GPIO,
+	POWER_SEQ_NUM_TYPES,
+};
+
+/**
+ * struct platform_power_seq_delay_step - platform data for delay steps
+ * @delay_us:	Amount of time to wait, in microseconds.
+ */
+struct platform_power_seq_delay_step {
+	unsigned int delay_us;
+};
+
+/**
+ * struct platform_power_seq_regulator_step - platform data for regulator steps
+ * @id:		Name of the regulator to use. The regulator will be obtained
+ *		using devm_regulator_get(dev, name)
+ * @enable:	Whether to enable or disable the regulator during this step
+ */
+struct platform_power_seq_regulator_step {
+	const char *id;
+	bool enable;
+};
+
+/**
+ * struct platform_power_seq_pwm_step - platform data for PWM steps
+ * @id:		Name of the pwm to use. The PWM will be obtained using
+ *		devm_pwm_get(dev, name)
+ * @enable:	Whether to enable or disable the PWM during this step
+ */
+struct platform_power_seq_pwm_step {
+	const char *id;
+	bool enable;
+};
+
+/**
+ * struct platform_power_seq_gpio_step - platform data for GPIO steps
+ * @number:	Number of the GPIO to use. The GPIO will be obtained using
+ *		devm_gpio_request_one(dev, number)
+ * @enable:	Whether to enable or disable the GPIO during this step
+ */
+struct platform_power_seq_gpio_step {
+	int number;
+	bool enable;
+};
+
+/**
+ * struct platform_power_seq_step - platform data for power sequences steps
+ * @type:	The type of this step. This decides which member of the union is
+ *		valid for this step.
+ * @delay:	Used if type == POWER_SEQ_DELAY
+ * @regulator:	Used if type == POWER_SEQ_REGULATOR
+ * @pwm:	Used if type == POWER_SEQ_PWN
+ * @gpio:	Used if type == POWER_SEQ_GPIO
+ */
+struct platform_power_seq_step {
+	enum power_seq_res_type type;
+	union {
+		struct platform_power_seq_delay_step delay;
+		struct platform_power_seq_regulator_step regulator;
+		struct platform_power_seq_pwm_step pwm;
+		struct platform_power_seq_gpio_step gpio;
+	};
+};
+
+/**
+ * struct platform_power_seq - platform data for power sequences
+ * @id:		Name through which this sequence is refered
+ * @num_steps:	Number of steps in that sequence
+ * @steps:	Array of num_steps steps describing the sequence
+ */
+struct platform_power_seq {
+	const char *id;
+	unsigned int num_steps;
+	struct platform_power_seq_step steps[];
+};
+
+/**
+ * struct platform_power_seq_set - platform data for sets of sequences
+ * @num_seqs:	Number of sequences in this set
+ * @seqs:	Array of pointers to individual sequences
+ */
+struct platform_power_seq_set {
+	unsigned int num_seqs;
+	struct platform_power_seq* seqs[];
+};
+
+/**
+ * struct power_seq_resource - resource used by a power sequence set
+ * @pdata:	Pointer to the platform data used to resolve this resource
+ * @regulator:	Resolved regulator if of type POWER_SEQ_REGULATOR
+ * @pwm:	Resolved PWM if of type POWER_SEQ_PWM
+ * @list:	Used to link resources together
+ */
+struct power_seq_resource {
+	/* relevant for resolving the resource and knowing its type */
+	struct platform_power_seq_step *pdata;
+	/* resolved resource (if any) */
+	union {
+		struct regulator *regulator;
+		struct pwm_device *pwm;
+	};
+	struct list_head list;
+};
+#define power_seq_for_each_resource(pos, seqs)				\
+	list_for_each_entry(pos, power_seq_set_resources(seqs), list)
+
+struct power_seq_resource;
+struct power_seq;
+struct power_seq_set;
+
+#ifdef CONFIG_OF
+struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);
+#else
+inline struct platform_power_seq_set *of_parse_power_seq_set(struct device *dev)
+{
+	return NULL;
+}
+#endif
+void devm_platform_power_seq_set_free(struct device *dev,
+				      struct platform_power_seq_set *pseq);
+
+struct power_seq_set *devm_power_seq_set_build(struct device *dev,
+					   struct platform_power_seq_set *pseq);
+struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
+struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char *id);
+int power_seq_run(struct power_seq *seq);
+
+#endif
-- 
1.7.12


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 2/4] pwm_backlight: use power sequences
  2012-08-31 11:34 [PATCH v5 0/4] Runtime Interpreted Power Sequences Alexandre Courbot
  2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot
@ 2012-08-31 11:34 ` Alexandre Courbot
  2012-09-05 17:25   ` Stephen Warren
  2012-08-31 11:34 ` [PATCH v5 3/4] tegra: dt: add label to tegra20's PWM Alexandre Courbot
  2012-08-31 11:34 ` [PATCH v5 4/4] tegra: ventana: add pwm backlight DT nodes Alexandre Courbot
  3 siblings, 1 reply; 16+ messages in thread
From: Alexandre Courbot @ 2012-08-31 11:34 UTC (permalink / raw)
  To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann
  Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss, linux-pm, linux-doc, Alexandre Courbot

Make use of the power sequences specified in the device tree or platform
data to control how the backlight is powered on and off.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/video/backlight/pwm-backlight.txt     |  67 +++++++-
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 179 +++++++++++++++------
 include/linux/pwm_backlight.h                      |  15 +-
 4 files changed, 208 insertions(+), 54 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..873b993 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,8 @@ pwm-backlight bindings
 
 Required properties:
   - compatible: "pwm-backlight"
-  - pwms: OF device-tree PWM specification (see PWM binding[0])
+  - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+      must be specified
   - brightness-levels: Array of distinct brightness levels. Typically these
       are in the range from 0 to 255, but any range starting at 0 will do.
       The actual brightness level (PWM duty cycle) will be interpolated
@@ -12,17 +13,73 @@ Required properties:
       array defined by the "brightness-levels" property)
 
 Optional properties:
-  - pwm-names: a list of names for the PWM devices specified in the
-               "pwms" property (see PWM binding[0])
+  - pwm-names: name for the PWM device specified in the "pwms" property (see PWM
+      binding[0]). Necessary if power sequences are used
+  - power-sequences: Power sequences (see Power sequences[1]) used to bring the
+      backlight on and off. If this property is present, then two power
+      sequences named "power-on" and "power-off" must be defined to control how
+      the backlight is to be powered on and off. These sequences must reference
+      the PWM specified in the pwms property by its name, and can also reference
+      other resources supported by the power sequences mechanism
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
+[1]: Documentation/devicetree/bindings/power_seq/power_seq.txt
 
 Example:
 
 	backlight {
 		compatible = "pwm-backlight";
-		pwms = <&pwm 0 5000000>;
-
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+
+		/* resources used by the sequences */
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+
+		power-sequences {
+			power-on {
+				step0 {
+					type = "regulator";
+					id = "power";
+					enable;
+				};
+				step1 {
+					type = "delay";
+					delay_us = <10000>;
+				};
+				step2 {
+					type = "pwm";
+					id = "backlight";
+					enable;
+				};
+				step3 {
+					type = "gpio";
+					number = <&gpio 28 0>;
+					enable;
+				};
+			};
+
+			power-off {
+				step0 {
+					type = "gpio";
+					number = <&gpio 28 0>;
+					disable;
+				};
+				step1 {
+					type = "pwm";
+					id = "backlight";
+					disable;
+				};
+				step2 {
+					type = "delay";
+					delay_us = <10000>;
+				};
+				step3 {
+					type = "regulator";
+					id = "power";
+					disable;
+				};
+			};
+		};
 	};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index cf28276..6fb8aa3 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -246,6 +246,7 @@ config BACKLIGHT_CARILLO_RANCH
 config BACKLIGHT_PWM
 	tristate "Generic PWM based Backlight Driver"
 	depends on PWM
+	select POWER_SEQ
 	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 995f016..45d6edd 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,12 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	bool			enabled;
+	bool			use_power_seqs;
+	struct power_seq	*power_on_seq;
+	struct power_seq	*power_off_seq;
+
+	/* Legacy callbacks */
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -35,6 +41,49 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static void pwm_backlight_on(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (pb->enabled)
+		return;
+
+	/* Legacy framework? */
+	if (!pb->use_power_seqs) {
+		pwm_config(pb->pwm, 0, pb->period);
+		pwm_disable(pb->pwm);
+		return;
+	}
+
+	ret = power_seq_run(pb->power_on_seq);
+	if (ret < 0)
+		dev_err(&bl->dev, "cannot run power on sequence\n");
+
+	pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (!pb->enabled)
+		return;
+
+	/* Legacy framework? */
+	if (!pb->use_power_seqs) {
+		pwm_enable(pb->pwm);
+		return;
+	}
+
+	ret = power_seq_run(pb->power_off_seq);
+	if (ret < 0)
+		dev_err(&bl->dev, "cannot run power off sequence\n");
+
+	pb->enabled = false;
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +100,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		brightness = pb->notify(pb->dev, brightness);
 
 	if (brightness == 0) {
-		pwm_config(pb->pwm, 0, pb->period);
-		pwm_disable(pb->pwm);
+		pwm_backlight_off(bl);
 	} else {
 		int duty_cycle;
 
@@ -66,7 +114,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		duty_cycle = pb->lth_brightness +
 		     (duty_cycle * (pb->period - pb->lth_brightness) / max);
 		pwm_config(pb->pwm, duty_cycle, pb->period);
-		pwm_enable(pb->pwm);
+		pwm_backlight_on(bl);
 	}
 
 	if (pb->notify_after)
@@ -145,11 +193,10 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		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.
-	 */
+	/* parse power sequences and extract those we will use */
+	data->power_seqs = devm_of_parse_power_seq_set(dev);
+	if (IS_ERR(data->power_seqs))
+		return PTR_ERR(data->power_seqs);
 
 	return 0;
 }
@@ -172,33 +219,96 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
 	struct platform_pwm_backlight_data defdata;
+	struct power_seq_resource *res;
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
+	bool use_dt = false;
 	unsigned int max;
 	int ret;
 
+	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
+	if (!pb) {
+		dev_err(&pdev->dev, "no memory for state\n");
+		return -ENOMEM;
+	}
+
+	/* using new interface or device tree */
 	if (!data) {
+		/* build platform data from device tree */
 		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
-		if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to find platform data\n");
 			return ret;
 		}
-
 		data = &defdata;
+		use_dt = true;
+	}
+
+	/* using legacy interface? */
+	if (!data->power_seqs) {
+		pb->pwm = devm_pwm_get(&pdev->dev, NULL);
+		if (IS_ERR(pb->pwm)) {
+			dev_err(&pdev->dev,
+				"unable to request PWM, trying legacy API\n");
+
+			pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
+			if (IS_ERR(pb->pwm)) {
+				dev_err(&pdev->dev,
+					"unable to request legacy PWM\n");
+				return PTR_ERR(pb->pwm);
+			}
+		}
+
+		/*
+		* The DT case will set the pwm_period_ns field to 0 and store
+		* the period, parsed from the DT, in the PWM device. For the
+		* non-DT case, set the period from platform data.
+		*/
+		if (data->pwm_period_ns > 0)
+			pwm_set_period(pb->pwm, data->pwm_period_ns);
+	} else {
+		/* build power sequences from platform data */
+		struct power_seq_set *seqs;
+
+		seqs = devm_power_seq_set_build(&pdev->dev, data->power_seqs);
+		if (IS_ERR(seqs))
+			return PTR_ERR(seqs);
+
+		if (use_dt)
+			devm_platform_power_seq_set_free(&pdev->dev,
+							 data->power_seqs);
+
+		pb->power_on_seq = power_seq_lookup(seqs, "power-on");
+		pb->power_off_seq = power_seq_lookup(seqs, "power-off");
+
+		/* we must have exactly one PWM for this driver */
+		power_seq_for_each_resource(res, seqs) {
+			if (res->pdata->type != POWER_SEQ_PWM)
+				continue;
+			if (pb->pwm) {
+				dev_err(&pdev->dev, "more than one PWM used\n");
+				return -EINVAL;
+			}
+			/* keep the pwm at hand */
+			pb->pwm = res->pwm;
+		}
+
+		pb->use_power_seqs = true;
 	}
 
 	if (data->init) {
 		ret = data->init(&pdev->dev);
 		if (ret < 0)
-			return ret;
+			goto err;
 	}
 
-	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
-	if (!pb) {
-		dev_err(&pdev->dev, "no memory for state\n");
-		ret = -ENOMEM;
-		goto err_alloc;
+	/* from here we should have a PWM */
+	if (!pb->pwm) {
+		dev_err(&pdev->dev, "no PWM defined!\n");
+		return -EINVAL;
 	}
 
 	if (data->levels) {
@@ -213,28 +323,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 
-	pb->pwm = pwm_get(&pdev->dev, NULL);
-	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
-		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-		if (IS_ERR(pb->pwm)) {
-			dev_err(&pdev->dev, "unable to request legacy PWM\n");
-			ret = PTR_ERR(pb->pwm);
-			goto err_alloc;
-		}
-	}
-
-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
-	/*
-	 * The DT case will set the pwm_period_ns field to 0 and store the
-	 * period, parsed from the DT, in the PWM device. For the non-DT case,
-	 * set the period from platform data.
-	 */
-	if (data->pwm_period_ns > 0)
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-
 	pb->period = pwm_get_period(pb->pwm);
 	pb->lth_brightness = data->lth_brightness * (pb->period / max);
 
@@ -246,18 +334,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (IS_ERR(bl)) {
 		dev_err(&pdev->dev, "failed to register backlight\n");
 		ret = PTR_ERR(bl);
-		goto err_bl;
+		goto err;
 	}
 
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);
+
 	return 0;
 
-err_bl:
-	pwm_put(pb->pwm);
-err_alloc:
+err:
 	if (data->exit)
 		data->exit(&pdev->dev);
 	return ret;
@@ -269,9 +356,8 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
 	backlight_device_unregister(bl);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-	pwm_put(pb->pwm);
+	pwm_backlight_off(bl);
+
 	if (pb->exit)
 		pb->exit(&pdev->dev);
 	return 0;
@@ -285,8 +371,7 @@ static int pwm_backlight_suspend(struct device *dev)
 
 	if (pb->notify)
 		pb->notify(pb->dev, 0);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
+	pwm_backlight_off(bl);
 	if (pb->notify_after)
 		pb->notify_after(pb->dev, 0);
 	return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..70d22f2 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,25 @@
 #define __LINUX_PWM_BACKLIGHT_H
 
 #include <linux/backlight.h>
+#include <linux/power_seq.h>
 
 struct platform_pwm_backlight_data {
-	int pwm_id;
 	unsigned int max_brightness;
 	unsigned int dft_brightness;
 	unsigned int lth_brightness;
-	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	/*
+	 * New interface using power sequences
+	 */
+	struct platform_power_seq_set *power_seqs;
+	/*
+	 * Legacy interface - use power sequences instead!
+	 *
+	 * pwm_id and pwm_period_ns need only be specified
+	 * if get_pwm(dev, NULL) would return NULL.
+	 */
+	int pwm_id;
+	unsigned int pwm_period_ns;
 	int (*init)(struct device *dev);
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
-- 
1.7.12


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 3/4] tegra: dt: add label to tegra20's PWM
  2012-08-31 11:34 [PATCH v5 0/4] Runtime Interpreted Power Sequences Alexandre Courbot
  2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot
  2012-08-31 11:34 ` [PATCH v5 2/4] pwm_backlight: use power sequences Alexandre Courbot
@ 2012-08-31 11:34 ` Alexandre Courbot
  2012-08-31 11:34 ` [PATCH v5 4/4] tegra: ventana: add pwm backlight DT nodes Alexandre Courbot
  3 siblings, 0 replies; 16+ messages in thread
From: Alexandre Courbot @ 2012-08-31 11:34 UTC (permalink / raw)
  To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann
  Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss, linux-pm, linux-doc, Alexandre Courbot

This is necessary to reference the PWM in board device trees.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 405d167..67a6cd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -123,7 +123,7 @@
 		status = "disabled";
 	};
 
-	pwm {
+	pwm: pwm {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
 		#pwm-cells = <2>;
-- 
1.7.12


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5 4/4] tegra: ventana: add pwm backlight DT nodes
  2012-08-31 11:34 [PATCH v5 0/4] Runtime Interpreted Power Sequences Alexandre Courbot
                   ` (2 preceding siblings ...)
  2012-08-31 11:34 ` [PATCH v5 3/4] tegra: dt: add label to tegra20's PWM Alexandre Courbot
@ 2012-08-31 11:34 ` Alexandre Courbot
  3 siblings, 0 replies; 16+ messages in thread
From: Alexandre Courbot @ 2012-08-31 11:34 UTC (permalink / raw)
  To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann
  Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss, linux-pm, linux-doc, Alexandre Courbot

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 59 ++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index 4ec6b4c..025928d 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -467,6 +467,63 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		/* resources used by the power sequences */
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&vdd_bl_reg>;
+
+		power-sequences {
+			power-on {
+				step0 {
+					type = "regulator";
+					id = "power";
+					enable;
+				};
+				step1 {
+					type = "delay";
+					delay_us = <10000>;
+				};
+				step2 {
+					type = "pwm";
+					id = "backlight";
+					enable;
+				};
+				step3 {
+					type = "gpio";
+					number = <&gpio 28 0>;
+					enable;
+				};
+			};
+
+			power-off {
+				step0 {
+					type = "gpio";
+					number = <&gpio 28 0>;
+					disable;
+				};
+				step1 {
+					type = "pwm";
+					id = "backlight";
+					disable;
+				};
+				step2 {
+					type = "delay";
+					delay_us = <10000>;
+				};
+				step3 {
+					type = "regulator";
+					id = "power";
+					disable;
+				};
+			};
+		};
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -510,7 +567,7 @@
 			enable-active-high;
 		};
 
-		regulator@4 {
+		vdd_bl_reg: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
 			regulator-name = "vdd_bl";
-- 
1.7.12


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot
@ 2012-09-05 17:19   ` Stephen Warren
  2012-09-07  8:21     ` Alex Courbot
  2012-09-06 14:14   ` Heiko Stübner
  1 sibling, 1 reply; 16+ messages in thread
From: Stephen Warren @ 2012-09-05 17:19 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss, linux-pm, linux-doc

On 08/31/2012 05:34 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
> 
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.

> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt

> +Runtime Interpreted Power Sequences
> +===================================
> +
> +Power sequences are sequential descriptions of actions to be performed on
> +power-related resources. Having these descriptions in a precise data format
> +allows us to take much of the board-specific power control code out of the
> +kernel and place it into the device tree instead, making kernels less
> +board-dependant.
> +

> +In the device tree, power sequences are grouped into a set. The set is always
> +declared as the "power-sequences" sub-node of the device node. Power sequences
> +may reference resources declared by that device.

I had to read that a few times to realize this was talking about a
device with multiple power sequences, and not talking about the steps in
a sequence. I think if you add the following sentence at the start of
that paragraph, it will be clearer:

A device may support multiple power sequences, for different events such
as powering on and off.

Also, perhaps add "these" at the first comma, so the above would read:

In the device tree, these power sequences are...

> +Power Sequences Structure
> +-------------------------
> +Every device that makes use of power sequences must have a "power-sequences"
> +sub-node. Power sequences are sub-nodes of this set node, and their node name
> +indicates the id of the sequence.
> +
> +Every power sequence in turn contains its steps as sub-nodes of itself. Step

The last word on that line should be "Steps".

> +must be named sequentially, with the first step named step0, the second step1,
> +etc. Failure to follow this rule will result in a parsing error.
> +
> +Power Sequences Steps
> +---------------------
> +Step of a sequence describes an action to be performed on a resource. They

s/Step/Steps/, s/describes/describe/.

> +always include a "type" property which indicates what kind of resource this
> +step works on. Depending on the resource type, additional properties are defined
> +to control the action to be performed.
> +
> +"delay" type required properties:
> +  - delay_us: delay to wait in microseconds

DT property name should use "-" not "_" to separate words, so "delay-us".

> +"regulator" type required properties:
> +  - id: name of the regulator to use. Regulator is obtained by
> +        regulator_get(dev, id)
> +  - enable / disable: one of these two empty properties must be present to
> +                      enable or disable the resource
> +
> +"pwm" type required properties:
> +  - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
> +  - enable / disable: one of these two empty properties must be present to
> +                      enable or disable the resource

For those two, would "name" be a better property name than "id"?

I wonder if we should have "regulator-enable" and "regulator-disable"
step types, rather than a "regulator" step type, with an "enable" or
"disable" property within it. I don't feel strongly though; this is
probably fine.

> +"gpio" type required properties:
> +  - number: phandle of the GPIO to use.

Naming the property "gpio" would seem more consistent with our GPIO
properties.

> +  - enable / disable: one of these two empty properties must be present to
> +                      enable or disable the resource

You can't really enable or disable a GPIO (well, perhaps you can, but
I'd consider that to affect tri-state rather than value); it's more that
you're setting the output value to 0 or 1. I think a "value" or
"set-value" property with value <0> or <1> would be better.

...
> +After the resources declaration, two sequences follow for powering the backlight
> +on and off. Their names are specified by the pwm-backlight driver.

Not the driver, but the binding for the device.

Overall, the general structure of the bindings looks reasonable to me.

> +++ b/Documentation/power/power_seq.txt

...
> +Platform Data Format
> +--------------------
> +All relevant data structures for declaring power sequences are located in
> +include/linux/power_seq.h.
> +
> +The platform data for a given device is an instance of platform_power_seq_set
> +which points to instances of platform_power_seq. Every platform_power_seq is a
> +single power sequence, and is itself composed of a variable length array of
> +steps.

I don't think you can mandate that the entire platform data structure
for a device is a platform_power_seq_set. Instead, I think you should
say that "The platform data for a device may contain an instance of
platform_power_seq_set...".

...
> +A power sequence can be executed by power_seq_run:
> +
> +  int power_seq_run(struct power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.

s/occured/occurred/

> +Sometimes, you may want to browse the list of resources allocated by a sequence,
> +to for instance ensure that a resource of a given type is present. The
> +power_seq_set_resources() function returns a list head that can be used with
> +the power_seq_for_each_resource() macro to browse all the resources of a set:
> +
> +  struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
> +  power_seq_for_each_resource(pos, seqs)
> +
> +Here "pos" will be of type struct power_seq_resource. This structure contains a
> +"pdata" pointer that can be used to explore the platform data of this resource,
> +as well as the resolved resource, if applicable.

I'm not sure what "pdata" is supposed to point at; platform data applies
to the original "struct device", not one of the resources used by the
power sequences.

> +Finally, users of the device tree can build the platform data corresponding to
> +the tree node using this function:
> +
> +  struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);

Hmmm. It'd be nice not to need separate functions for the non-DT and DT
cases. That would require that devm_power_seq_set_build() be able to
find the power sequence definitions somewhere other than platform data
in the non-DT case - that's exactly why the regulator and pinctrl
subsystems represent the device<->data mapping table separately from the
device's platform data.

> +++ b/drivers/power/power_seq/Kconfig

> +config POWER_SEQ
> +	bool

Some kind of help text might be useful?

I didn't review any of the .c files.

> +++ b/include/linux/power_seq.h

> +/**
> + * struct platform_power_seq_step - platform data for power sequences steps
> + * @type:	The type of this step. This decides which member of the union is
> + *		valid for this step.
> + * @delay:	Used if type == POWER_SEQ_DELAY
> + * @regulator:	Used if type == POWER_SEQ_REGULATOR
> + * @pwm:	Used if type == POWER_SEQ_PWN
> + * @gpio:	Used if type == POWER_SEQ_GPIO

In those last 4 line, I think s/type/@type/ since you're referencing
another parameter?

> +struct power_seq_resource {
> +	/* relevant for resolving the resource and knowing its type */
> +	struct platform_power_seq_step *pdata;

Aha. So this isn't really platform data for the resource, but actually a
step definition that referenced it. I think it'd be better to rename
this field "step", and amend the documentation above not to refer to
"pdata" but explicitly talk about a step definition; the step may have
been defined in pdata, but isn't in the DT case.

Alternatively, why not just copy the step type enum here, rather than
referencing the step definition?


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 2/4] pwm_backlight: use power sequences
  2012-08-31 11:34 ` [PATCH v5 2/4] pwm_backlight: use power sequences Alexandre Courbot
@ 2012-09-05 17:25   ` Stephen Warren
  2012-09-07  8:28     ` Alex Courbot
  0 siblings, 1 reply; 16+ messages in thread
From: Stephen Warren @ 2012-09-05 17:25 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, linux-fbdev, linux-pm, devicetree-discuss,
	linux-doc, linux-kernel, linux-tegra

On 08/31/2012 05:34 AM, Alexandre Courbot wrote:
> Make use of the power sequences specified in the device tree or platform
> data to control how the backlight is powered on and off.

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

>  Optional properties:
> -  - pwm-names: a list of names for the PWM devices specified in the
> -               "pwms" property (see PWM binding[0])
> +  - pwm-names: name for the PWM device specified in the "pwms" property (see PWM
> +      binding[0]). Necessary if power sequences are used

So this implies that power sequence are completely optional in the pwm
binding...

> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig

>  config BACKLIGHT_PWM
>  	tristate "Generic PWM based Backlight Driver"
>  	depends on PWM
> +	select POWER_SEQ

... but that implies they're basically mandatory.

Briefly looking at the code, power sequences don't appear to be
optional, at least for the DT case, so perhaps you just need to update
the documentation to make "pwm-names" non-optional?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot
  2012-09-05 17:19   ` Stephen Warren
@ 2012-09-06 14:14   ` Heiko Stübner
  2012-09-07  8:04     ` Alex Courbot
  1 sibling, 1 reply; 16+ messages in thread
From: Heiko Stübner @ 2012-09-06 14:14 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss, linux-pm, linux-doc

Hi Alexander,

Am Freitag, 31. August 2012, 13:34:03 schrieb Alexandre Courbot:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
> 
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

I really like this idea, also because I'll have to solve similar problems on 
the way to dt for my tinker-platform.

For your power_seq_run function you write that it simply returns an error code 
on failure and looking through it I also just found the error return 
statement. This would leave a device half turned on.

So I'm wondering, if it shouldn't turn off all the things it turned on until 
the step that produced the error. All your possible step types (execpt the 
delay) are booleans, so it should be possible to simply negate them when 
backtracking through the previous steps.


Heiko


> ---
>  .../devicetree/bindings/power_seq/power_seq.txt    | 117 ++++++
>  Documentation/power/power_seq.txt                  | 225 +++++++++++
>  drivers/power/Kconfig                              |   1 +
>  drivers/power/Makefile                             |   1 +
>  drivers/power/power_seq/Kconfig                    |   2 +
>  drivers/power/power_seq/Makefile                   |   1 +
>  drivers/power/power_seq/power_seq.c                | 446
> +++++++++++++++++++++ drivers/power/power_seq/power_seq_delay.c          |
>  51 +++
>  drivers/power/power_seq/power_seq_gpio.c           |  81 ++++
>  drivers/power/power_seq/power_seq_pwm.c            |  85 ++++
>  drivers/power/power_seq/power_seq_regulator.c      |  86 ++++
>  include/linux/power_seq.h                          | 174 ++++++++
>  12 files changed, 1270 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/power_seq/power_seq.txt create mode
> 100644 Documentation/power/power_seq.txt
>  create mode 100644 drivers/power/power_seq/Kconfig
>  create mode 100644 drivers/power/power_seq/Makefile
>  create mode 100644 drivers/power/power_seq/power_seq.c
>  create mode 100644 drivers/power/power_seq/power_seq_delay.c
>  create mode 100644 drivers/power/power_seq/power_seq_gpio.c
>  create mode 100644 drivers/power/power_seq/power_seq_pwm.c
>  create mode 100644 drivers/power/power_seq/power_seq_regulator.c
>  create mode 100644 include/linux/power_seq.h
> 
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
> b/Documentation/devicetree/bindings/power_seq/power_seq.txt new file mode
> 100644
> index 0000000..d3e3f6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> @@ -0,0 +1,117 @@
> +Runtime Interpreted Power Sequences
> +===================================
> +
> +Power sequences are sequential descriptions of actions to be performed on
> +power-related resources. Having these descriptions in a precise data
> format +allows us to take much of the board-specific power control code
> out of the +kernel and place it into the device tree instead, making
> kernels less +board-dependant.
> +
> +In the device tree, power sequences are grouped into a set. The set is
> always +declared as the "power-sequences" sub-node of the device node.
> Power sequences +may reference resources declared by that device.
> +
> +Power Sequences Structure
> +-------------------------
> +Every device that makes use of power sequences must have a
> "power-sequences" +sub-node. Power sequences are sub-nodes of this set
> node, and their node name +indicates the id of the sequence.
> +
> +Every power sequence in turn contains its steps as sub-nodes of itself.
> Step +must be named sequentially, with the first step named step0, the
> second step1, +etc. Failure to follow this rule will result in a parsing
> error.
> +
> +Power Sequences Steps
> +---------------------
> +Step of a sequence describes an action to be performed on a resource. They
> +always include a "type" property which indicates what kind of resource
> this +step works on. Depending on the resource type, additional properties
> are defined +to control the action to be performed.
> +
> +"delay" type required properties:
> +  - delay_us: delay to wait in microseconds
> +
> +"regulator" type required properties:
> +  - id: name of the regulator to use. Regulator is obtained by
> +        regulator_get(dev, id)
> +  - enable / disable: one of these two empty properties must be present to
> +                      enable or disable the resource
> +
> +"pwm" type required properties:
> +  - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
> +  - enable / disable: one of these two empty properties must be present to
> +                      enable or disable the resource
> +
> +"gpio" type required properties:
> +  - number: phandle of the GPIO to use.
> +  - enable / disable: one of these two empty properties must be present to
> +                      enable or disable the resource
> +
> +Example
> +-------
> +Here are example sequences declared within a backlight device that use all
> the +supported resources types:
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		...
> +
> +		/* resources used by the power sequences */
> +		pwms = <&pwm 2 5000000>;
> +		pwm-names = "backlight";
> +		power-supply = <&backlight_reg>;
> +
> +		power-sequences {
> +			power-on {
> +				step0 {
> +					type = "regulator";
> +					id = "power";
> +					enable;
> +				};
> +				step1 {
> +					type = "delay";
> +					delay_us = <10000>;
> +				};
> +				step2 {
> +					type = "pwm";
> +					id = "backlight";
> +					enable;
> +				};
> +				step3 {
> +					type = "gpio";
> +					number = <&gpio 28 0>;
> +					enable;
> +				};
> +			};
> +
> +			power-off {
> +				step0 {
> +					type = "gpio";
> +					number = <&gpio 28 0>;
> +					disable;
> +				};
> +				step1 {
> +					type = "pwm";
> +					id = "backlight";
> +					disable;
> +				};
> +				step2 {
> +					type = "delay";
> +					delay_us = <10000>;
> +				};
> +				step3 {
> +					type = "regulator";
> +					id = "power";
> +					disable;
> +				};
> +			};
> +		};
> +	};
> +
> +The first part lists the PWM and regulator resources used by the
> sequences. +These resources will be requested on behalf of the backlight
> device when the +sequences are built and are declared according to their
> own framework in a way +that makes them accessible by name.
> +
> +After the resources declaration, two sequences follow for powering the
> backlight +on and off. Their names are specified by the pwm-backlight
> driver. diff --git a/Documentation/power/power_seq.txt
> b/Documentation/power/power_seq.txt new file mode 100644
> index 0000000..48d1f6b
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,225 @@
> +Runtime Interpreted Power Sequences
> +===================================
> +
> +Problem
> +-------
> +Very commonly, boards need the help of out-of-driver code to turn some of
> their +devices on and off. For instance, SoC boards very commonly use a
> GPIO +(abstracted to a regulator or not) to control the power supply of a
> backlight, +disabling it when the backlight is not used in order to save
> power. The GPIO +that should be used, however, as well as the exact power
> sequence that may +also involve other resources, is board-dependent and
> thus unknown to the driver. +
> +This was previously addressed by having hooks in the device's platform
> data that +are called whenever the state of the device might reflect a
> power change. This +approach, however, introduces board-dependant code
> into the kernel and is not +compatible with the device tree.
> +
> +The Runtime Interpreted Power Sequences (or power sequences for short) aim
> at +turning this code into platform data or device tree nodes. Power
> sequences are +described using a simple format and run by a lightweight
> interpreter whenever +needed. This allows to remove the callback mechanism
> and makes the kernel less +board-dependant.
> +
> +What are Power Sequences?
> +-------------------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources and actions operations
> are: +- delay (just wait for a given number of microseconds)
> +- GPIO (enable or disable)
> +- regulator (enable or disable)
> +- PWM (enable or disable)
> +
> +When a power sequence is run, each of its steps is executed sequentially
> until +one step fails or the end of the sequence is reached.
> +
> +Power sequences are grouped in "sets" and declared per-device. Every
> sequence +must be attributed a name that can be used to retrieve it from
> its set when it +is needed.
> +
> +Power sequences can be declared as platform data or in the device tree.
> +
> +Platform Data Format
> +--------------------
> +All relevant data structures for declaring power sequences are located in
> +include/linux/power_seq.h.
> +
> +The platform data for a given device is an instance of
> platform_power_seq_set +which points to instances of platform_power_seq.
> Every platform_power_seq is a +single power sequence, and is itself
> composed of a variable length array of +steps.
> +
> +A step is a union of all the step structures. Which one is to be used
> depends on +the type of the step. Step structures are documented in the
> +include/linux/power_seq.h file ; please refer to it for all details, but
> the +following example will probably make it clear how power sequences
> should be +defined. It defines two power sequences named "power_on" and
> "power_off". The +"power_on" sequence enables a regulator called "power"
> (retrieved from the +device using regulator_get()), waits for 10ms, and
> then enabled GPIO 110. +"power_off" does the opposite.
> +
> +static struct platform_power_seq power_on_seq = {
> +	.id = "power_on",
> +	.num_steps = 3,
> +	.steps = {
> +		{
> +			.type = POWER_SEQ_REGULATOR,
> +			.regulator = {
> +				.id = "power",
> +				.enable = true,
> +			},
> +		},
> +		{
> +			.type = POWER_SEQ_DELAY,
> +			.delay = {
> +				.delay_us = 10000,
> +			},
> +		},
> +		{
> +			.type = POWER_SEQ_GPIO,
> +			.gpio = {
> +				.number = 110,
> +				.enable = true,
> +			},
> +		},
> +	},
> +};
> +
> +static struct platform_power_seq power_off_seq = {
> +	.id = "power_off",
> +	.num_steps = 3,
> +	.steps = {
> +		{
> +			.type = POWER_SEQ_GPIO,
> +			.gpio = {
> +				.number = 110,
> +				.enable = false,
> +			},
> +		},
> +		{
> +			.type = POWER_SEQ_DELAY,
> +			.delay = {
> +				.delay_us = 10000,
> +			},
> +		},
> +		{
> +			.type = POWER_SEQ_REGULATOR,
> +			.regulator = {
> +				.id = "power",
> +				.enable = false,
> +			},
> +		},
> +	},
> +};
> +
> +static struct platform_power_seq_set power_sequences = {
> +	.num_seqs = 2,
> +	.seqs = {
> +		&power_on_seq,
> +		&power_off_seq,
> +	},
> +};
> +
> +Device Tree
> +-----------
> +Power sequences can also be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined
> previously: +
> +power-supply = <&power_reg>;
> +
> +power-sequences {
> +	power-on {
> +		step0 {
> +			type = "regulator";
> +			id = "power";
> +			enable;
> +		};
> +		step1 {
> +			type = "delay";
> +			delay = <10000>;
> +		};
> +		step2 {
> +			type = "gpio";
> +			number = <&gpio 110 0>;
> +			enable;
> +		};
> +	}
> +	power-off {
> +		step0 {
> +			type = "gpio";
> +			number = <&gpio 110 0>;
> +			disable;
> +		};
> +		step1 {
> +			type = "delay";
> +			delay = <10000>;
> +		};
> +		step2 {
> +			type = "regulator";
> +			id = "power";
> +			disable;
> +		};
> +	}
> +};
> +
> +See Documentation/devicetree/bindings/power_seq/power_seq.txt for the
> complete +syntax of the bindings.
> +
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The devm_power_seq_set_build() function builds a power sequence
> set +from platform data. It also takes care of resolving and allocating
> the resources +referenced by the sequence:
> +
> +  struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> +					   struct platform_power_seq_set *pseq);
> +
> +As its name states, all memory and resources are devm-allocated. The 'dev'
> +argument is the device in the name of which the resources are to be
> allocated. +
> +On success, the function returns a devm allocated resolved sequences set
> for +which all the resources are allocated. In case of failure, an error
> code is +returned.
> +
> +Power sequences can then be retrieved by their name using
> power_seq_lookup: +
> +  struct power_seq *power_seq_lookup(struct power_seq_set *seqs,
> +				     const char *id);
> +
> +A power sequence can be executed by power_seq_run:
> +
> +  int power_seq_run(struct power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code
> if a +problem occured.
> +
> +Sometimes, you may want to browse the list of resources allocated by a
> sequence, +to for instance ensure that a resource of a given type is
> present. The +power_seq_set_resources() function returns a list head that
> can be used with +the power_seq_for_each_resource() macro to browse all
> the resources of a set: +
> +  struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
> +  power_seq_for_each_resource(pos, seqs)
> +
> +Here "pos" will be of type struct power_seq_resource. This structure
> contains a +"pdata" pointer that can be used to explore the platform data
> of this resource, +as well as the resolved resource, if applicable.
> +
> +Finally, users of the device tree can build the platform data
> corresponding to +the tree node using this function:
> +
> +  struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device
> *dev); +
> +As the device tree syntax unambiguously states the name of the node
> containing +the power sequences, it only needs a pointer to the device to
> work. The result +can then be passed to devm_power_seq_set_build() in
> order to get a set of +runnable sequences.
> +
> +devm_of_parse_power_seq_set allocates its memory using devm, but the
> platform +data becomes unneeded after devm_power_seq_set_build() is called
> on it and can +thus be freed. Be aware though that one allocation is
> performed for the set and +for every sequence. The
> devm_power_seq_platform_data_free() function takes care +of freeing the
> memory properly:
> +
> +  void devm_platform_power_seq_set_free(struct platform_power_seq_set
> *pseq); diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index fcc1bb0..5fdfd84 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -312,3 +312,4 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
>  endif # POWER_SUPPLY
> 
>  source "drivers/power/avs/Kconfig"
> +source "drivers/power/power_seq/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ee58afb..d3c893b 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
>  obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>  obj-$(CONFIG_POWER_AVS)		+= avs/
>  obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
> +obj-$(CONFIG_POWER_SEQ)		+= power_seq/
> diff --git a/drivers/power/power_seq/Kconfig
> b/drivers/power/power_seq/Kconfig new file mode 100644
> index 0000000..3bff26e
> --- /dev/null
> +++ b/drivers/power/power_seq/Kconfig
> @@ -0,0 +1,2 @@
> +config POWER_SEQ
> +	bool
> diff --git a/drivers/power/power_seq/Makefile
> b/drivers/power/power_seq/Makefile new file mode 100644
> index 0000000..f77a359
> --- /dev/null
> +++ b/drivers/power/power_seq/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_POWER_SEQ)		+= power_seq.o
> diff --git a/drivers/power/power_seq/power_seq.c
> b/drivers/power/power_seq/power_seq.c new file mode 100644
> index 0000000..e4d482c
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq.c
> @@ -0,0 +1,446 @@
> +/*
> + * power_seq.c - A simple power sequence interpreter for platform devices
> + *               and device tree.
> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * 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; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/power_seq.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +
> +#include <linux/of.h>
> +
> +struct power_seq_set {
> +	struct device *dev;
> +	struct list_head resources;
> +	struct list_head sequences;
> +};
> +
> +struct power_seq_step {
> +	/* Copy of the platform data */
> +	struct platform_power_seq_step pdata;
> +	/* Resolved resource */
> +	struct power_seq_resource *resource;
> +};
> +
> +struct power_seq {
> +	/* Set this sequence belongs to */
> +	struct power_seq_set *parent_set;
> +	const char *id;
> +	/* To thread into power_seqs structure */
> +	struct list_head list;
> +	unsigned int num_steps;
> +	struct power_seq_step steps[];
> +};
> +
> +#define power_seq_err(dev, seq, step_nbr, format, ...)			     \
> +	dev_err(dev, "%s[%d]: " format, seq->id, step_nbr, ##__VA_ARGS__);
> +
> +/**
> + * struct power_seq_res_type - operators for power sequences resources
> + * @name:		Name of the resource type. Set to null when a resource
> + *			type support is not compiled in
> + * @need_resource:	Whether a resource needs to be allocated when steps of
> + *			this kind are met. If set to false, res_compare and
> + *			res_alloc need not be set
> + * @of_parse:		Parse a step for this kind of resource from a device
> + *			tree node. The result of parsing must be written into
> + *			step step_nbr of seq
> + * @step_run:		Run a step for this kind of resource
> + * @res_compare:	Return true if the resource used by both steps is the
> + *			same, false otherwise
> + * @res_alloc:		Resolve and allocate the resource passed from seq
> + *			Return error code if the resource cannot be allocated
> + */
> +struct power_seq_res_ops {
> +	const char *name;
> +	bool need_resource;
> +	int (*of_parse)(struct device *dev, struct device_node *node,
> +			struct platform_power_seq *seq, unsigned int step_nbr);
> +	int (*step_run)(struct power_seq_step *step);
> +	bool (*res_compare)(struct platform_power_seq_step *step1,
> +			    struct platform_power_seq_step *step2);
> +	int (*res_alloc)(struct device *dev, struct power_seq_resource *seq);
> +};
> +
> +static const struct power_seq_res_ops
> power_seq_types[POWER_SEQ_NUM_TYPES]; +
> +#ifdef CONFIG_OF
> +static int of_power_seq_parse_enable_properties(struct device *dev,
> +						struct device_node *node,
> +						struct platform_power_seq *seq,
> +						unsigned int step_nbr,
> +						bool *enable)
> +{
> +	if (of_find_property(node, "enable", NULL)) {
> +		*enable = true;
> +	} else if (of_find_property(node, "disable", NULL)) {
> +		*enable = false;
> +	} else {
> +		power_seq_err(dev, seq, step_nbr,
> +			      "missing enable or disable property\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_power_seq_parse_step(struct device *dev, struct device_node
> *node, +				   struct platform_power_seq *seq,
> +				   unsigned int step_nbr)
> +{
> +	struct platform_power_seq_step *step = &seq->steps[step_nbr];
> +	const char *type;
> +	int i, err;
> +
> +	err = of_property_read_string(node, "type", &type);
> +	if (err < 0) {
> +		power_seq_err(dev, seq, step_nbr,
> +			      "cannot read type property\n");
> +		return err;
> +	}
> +	for (i = 0; i < POWER_SEQ_NUM_TYPES; i++) {
> +		if (power_seq_types[i].name == NULL)
> +			continue;
> +		if (!strcmp(type, power_seq_types[i].name))
> +			break;
> +	}
> +	if (i >= POWER_SEQ_NUM_TYPES) {
> +		power_seq_err(dev, seq, step_nbr, "unknown type %s\n", type);
> +		return -EINVAL;
> +	}
> +	step->type = i;
> +	err = power_seq_types[step->type].of_parse(dev, node, seq, step_nbr);
> +
> +	return err;
> +}
> +
> +static struct platform_power_seq *of_parse_power_seq(struct device *dev,
> +						     struct device_node *node)
> +{
> +	struct device_node *child = NULL;
> +	struct platform_power_seq *pseq;
> +	int num_steps, sz;
> +	int err;
> +
> +	if (!node)
> +		return ERR_PTR(-EINVAL);
> +
> +	num_steps = of_get_child_count(node);
> +	sz = sizeof(*pseq) + sizeof(pseq->steps[0]) * num_steps;
> +	pseq = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!pseq)
> +		return ERR_PTR(-ENOMEM);
> +	pseq->num_steps = num_steps;
> +	pseq->id = node->name;
> +
> +	for_each_child_of_node(node, child) {
> +		unsigned int pos;
> +
> +		/* Check that the name's format is correct and within bounds */
> +		if (strncmp("step", child->name, 4)) {
> +			err = -EINVAL;
> +			goto parse_error;
> +		}
> +
> +		err = kstrtouint(child->name + 4, 10, &pos);
> +		if (err < 0)
> +			goto parse_error;
> +
> +		if (pos >= num_steps || pseq->steps[pos].type != 0) {
> +			err = -EINVAL;
> +			goto parse_error;
> +		}
> +
> +		err = of_power_seq_parse_step(dev, child, pseq, pos);
> +		if (err)
> +			return ERR_PTR(err);
> +	}
> +
> +	return pseq;
> +
> +parse_error:
> +	dev_err(dev, "%s: invalid power step name %s!\n", pseq->id,
> +		child->name);
> +	return ERR_PTR(err);
> +}
> +
> +/**
> + * of_parse_power_seq_set() - build platform data corresponding to a DT
> node + * @dev:	Device on behalf of which the sequence is to be built
> + *
> + * Sequences must be contained into a subnode named "power-sequences" of
> the + * device root node.
> + *
> + * Memory for the platform sequence is allocated using devm_kzalloc on dev
> and + * can be freed by devm_kfree after power_seq_set_build returned.
> Beware that on + * top of the set itself, platform data for individual
> sequences should also be + * freed.
> + *
> + * Returns the built power sequence set on success, or an error code in
> case of + * failure.
> + */
> +struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device
> *dev) +{
> +	struct platform_power_seq_set *seqs;
> +	struct device_node *root = dev->of_node;
> +	struct device_node *seq;
> +	int num_seqs, sz, i = 0;
> +
> +	if (!root)
> +		return NULL;
> +
> +	root = of_find_node_by_name(root, "power-sequences");
> +	if (!root)
> +		return NULL;
> +
> +	num_seqs = of_get_child_count(root);
> +	sz = sizeof(*seqs) + sizeof(seqs->seqs[0]) * num_seqs;
> +	seqs = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!seqs)
> +		return ERR_PTR(-ENOMEM);
> +	seqs->num_seqs = num_seqs;
> +
> +	for_each_child_of_node(root, seq) {
> +		struct platform_power_seq *pseq;
> +
> +		pseq = of_parse_power_seq(dev, seq);
> +		if (IS_ERR(pseq))
> +			return (void *)pseq;
> +
> +		seqs->seqs[i++] = pseq;
> +	}
> +
> +	return seqs;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_parse_power_seq_set);
> +#endif /* CONFIG_OF */
> +
> +/**
> + * devm_platform_power_seq_set_free() - free data allocated by
> of_parse_power_seq_set + * @pseq:	Platform data to free
> + *
> + * This function can be called *only* on data returned by
> of_parse_power_seq_set + * and *after* devm_power_seq_set_build has been
> called on it.
> + */
> +void devm_platform_power_seq_set_free(struct device *dev,
> +				      struct platform_power_seq_set *pseq)
> +{
> +	int i;
> +
> +	for (i = 0; i < pseq->num_seqs; i++)
> +		devm_kfree(dev, pseq->seqs[i]);
> +
> +	devm_kfree(dev, pseq);
> +}
> +EXPORT_SYMBOL_GPL(devm_platform_power_seq_set_free);
> +
> +static struct power_seq_resource *
> +power_seq_find_resource(struct list_head *ress,
> +			struct platform_power_seq_step *step)
> +{
> +	struct power_seq_resource *res;
> +
> +	list_for_each_entry(res, ress, list) {
> +		struct platform_power_seq_step *pdata = res->pdata;
> +
> +		if (pdata->type != step->type)
> +			continue;
> +
> +		if (power_seq_types[pdata->type].res_compare(pdata, step))
> +			return res;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct power_seq *power_seq_build_one(struct device *dev,
> +					     struct power_seq_set *seqs,
> +					     struct platform_power_seq *pseq)
> +{
> +	struct power_seq *seq;
> +	struct power_seq_resource *res;
> +	int i, err;
> +
> +	seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(seq->steps[0]) *
> +			   pseq->num_steps, GFP_KERNEL);
> +	if (!seq)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&seq->list);
> +	seq->parent_set = seqs;
> +	seq->num_steps = pseq->num_steps;
> +	seq->id = pseq->id;
> +
> +	for (i = 0; i < seq->num_steps; i++) {
> +		struct platform_power_seq_step *pstep = &pseq->steps[i];
> +		struct power_seq_step *step = &seq->steps[i];
> +
> +		if (pstep->type >= POWER_SEQ_NUM_TYPES ||
> +		    power_seq_types[pstep->type].name == NULL) {
> +			power_seq_err(dev, seq, i,
> +				      "invalid power sequence type %d!",
> +		 		      pstep->type);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		memcpy(&step->pdata, pstep, sizeof(step->pdata));
> +
> +		/* Steps without resource need not to continue */
> +		if (!power_seq_types[pstep->type].need_resource)
> +			continue;
> +
> +		/* create resource node if not referenced already */
> +		res = power_seq_find_resource(&seqs->resources, pstep);
> +		if (!res) {
> +			res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> +			if (!res)
> +				return ERR_PTR(-ENOMEM);
> +
> +			res->pdata = &step->pdata;
> +
> +			err = power_seq_types[res->pdata->type].res_alloc(dev, res);
> +			if (err < 0) {
> +				power_seq_err(dev, seq, i,
> +					      "error building sequence\n");
> +				return ERR_PTR(err);
> +			}
> +
> +			list_add_tail(&res->list, &seqs->resources);
> +		}
> +		step->resource = res;
> +	}
> +
> +	return seq;
> +}
> +
> +/**
> + * power_seq_set_build() - build a set of runnable sequences from platform
> data + * @dev:	Device that will use the power sequences. All resources
> will be + *		devm-allocated against it
> + * @pseq:	Platform data for the power sequences. It can be freed after
> + *		this function returns
> + *
> + * All memory and resources (regulators, GPIOs, etc.) are allocated using
> devm + * functions.
> + *
> + * Returns the built sequence on success, an error code in case or
> failure. + */
> +struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> +					    struct platform_power_seq_set *pseq)
> +{
> +	struct power_seq_set *seqs;
> +	int i;
> +
> +	seqs = devm_kzalloc(dev, sizeof(*seqs), GFP_KERNEL);
> +
> +	if (!seqs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&seqs->resources);
> +	INIT_LIST_HEAD(&seqs->sequences);
> +	for (i = 0; i < pseq->num_seqs; i++) {
> +		struct power_seq *seq;
> +
> +		seq = power_seq_build_one(dev, seqs, pseq->seqs[i]);
> +		if (IS_ERR(seq))
> +			return (void *)seq;
> +
> +		list_add_tail(&seq->list, &seqs->sequences);
> +	}
> +
> +	return seqs;
> +}
> +EXPORT_SYMBOL_GPL(devm_power_seq_set_build);
> +
> +/**
> + * power_seq_lookup - Lookup a power sequence by name from a set
> + * @seqs:	The set to look in
> + * @id:		Name to look after
> + *
> + * Returns a matching power sequence if it exists, NULL if it does not.
> + */
> +struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char
> *id) +{
> +	struct power_seq *seq;
> +
> +	list_for_each_entry(seq, &seqs->sequences, list) {
> +		if (!strcmp(seq->id, id))
> +			return seq;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_lookup);
> +
> +/**
> + * power_seq_set_resources - return a list of all the resources used by a
> set + * @seqs:	Power sequences set we are interested in getting the
> resources + *
> + * The returned list can be parsed using the power_seq_for_each_resource
> macro. + */
> +struct list_head *power_seq_set_resources(struct power_seq_set *seqs)
> +{
> +	return &seqs->resources;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_set_resources);
> +
> +/**
> + * power_seq_run() - run a power sequence
> + * @seq:	The power sequence to run
> + *
> + * Returns 0 on success, error code in case of failure.
> + */
> +int power_seq_run(struct power_seq *seq)
> +{
> +	unsigned int i;
> +	int err;
> +
> +	if (!seq)
> +		return 0;
> +
> +	for (i = 0; i < seq->num_steps; i++) {
> +		unsigned int type = seq->steps[i].pdata.type;
> +
> +		err = power_seq_types[type].step_run(&seq->steps[i]);
> +		if (err) {
> +			power_seq_err(seq->parent_set->dev, seq, i,
> +				"error %d while running power sequence step\n",
> +				err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_run);
> +
> +#include "power_seq_delay.c"
> +#include "power_seq_regulator.c"
> +#include "power_seq_pwm.c"
> +#include "power_seq_gpio.c"
> +
> +static const struct power_seq_res_ops power_seq_types[POWER_SEQ_NUM_TYPES]
> = { +	[POWER_SEQ_DELAY] = POWER_SEQ_DELAY_TYPE,
> +	[POWER_SEQ_REGULATOR] = POWER_SEQ_REGULATOR_TYPE,
> +	[POWER_SEQ_PWM] = POWER_SEQ_PWM_TYPE,
> +	[POWER_SEQ_GPIO] = POWER_SEQ_GPIO_TYPE,
> +};
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/power/power_seq/power_seq_delay.c
> b/drivers/power/power_seq/power_seq_delay.c new file mode 100644
> index 0000000..072bf50
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_delay.c
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * 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; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/delay.h>
> +
> +#ifdef CONFIG_OF
> +static int of_power_seq_parse_delay(struct device *dev,
> +				    struct device_node *node,
> +				    struct platform_power_seq *seq,
> +				    unsigned int step_nbr)
> +{
> +	struct platform_power_seq_step *step = &seq->steps[step_nbr];
> +	int err;
> +
> +	err = of_property_read_u32(node, "delay_us",
> +				   &step->delay.delay_us);
> +	if (err < 0)
> +		power_seq_err(dev, seq, step_nbr,
> +			      "error reading delay_us property\n");
> +
> +	return err;
> +}
> +#else
> +#define of_power_seq_parse_delay NULL
> +#endif
> +
> +static int power_seq_step_run_delay(struct power_seq_step *step)
> +{
> +	usleep_range(step->pdata.delay.delay_us,
> +		     step->pdata.delay.delay_us + 1000);
> +
> +	return 0;
> +}
> +
> +#define POWER_SEQ_DELAY_TYPE {			\
> +	.name = "delay",			\
> +	.need_resource = false,			\
> +	.of_parse = of_power_seq_parse_delay,	\
> +	.step_run = power_seq_step_run_delay,	\
> +}
> diff --git a/drivers/power/power_seq/power_seq_gpio.c
> b/drivers/power/power_seq/power_seq_gpio.c new file mode 100644
> index 0000000..2e9a49f
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_gpio.c
> @@ -0,0 +1,81 @@
> +/*
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * 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; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +
> +#ifdef CONFIG_OF
> +static int power_seq_of_parse_gpio(struct device *dev,
> +				   struct device_node *node,
> +				   struct platform_power_seq *seq,
> +				   unsigned int step_nbr)
> +{
> +	struct platform_power_seq_step *step = &seq->steps[step_nbr];
> +	int gpio;
> +	int err;
> +
> +	gpio = of_get_named_gpio(node, "number", 0);
> +	if (gpio < 0) {
> +		power_seq_err(dev, seq, step_nbr,
> +			      "error reading number property\n");
> +		return gpio;
> +	}
> +	step->gpio.number = gpio;
> +
> +	err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
> +					     &step->gpio.enable);
> +
> +	return err;
> +}
> +#else
> +#define of_power_seq_parse_gpio NULL
> +#endif
> +
> +static bool power_seq_res_compare_gpio(struct platform_power_seq_step
> *step1, +				       struct platform_power_seq_step *step2)
> +{
> +	return step1->gpio.number == step2->gpio.number;
> +}
> +
> +static int power_seq_res_alloc_gpio(struct device *dev,
> +					struct power_seq_resource *res)
> +{
> +	int err;
> +
> +	err = devm_gpio_request_one(dev, res->pdata->gpio.number,
> +				    GPIOF_OUT_INIT_LOW, dev_name(dev));
> +	if (err) {
> +		dev_err(dev, "cannot get gpio %d\n", res->pdata->gpio.number);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int power_seq_step_run_gpio(struct power_seq_step *step)
> +{
> +	gpio_set_value_cansleep(step->pdata.gpio.number,
> +				step->pdata.gpio.enable);
> +
> +	return 0;
> +}
> +
> +#define POWER_SEQ_GPIO_TYPE {					\
> +	.name = "gpio",					\
> +	.need_resource = true,				\
> +	.of_parse = power_seq_of_parse_gpio,		\
> +	.step_run = power_seq_step_run_gpio,		\
> +	.res_compare = power_seq_res_compare_gpio,	\
> +	.res_alloc = power_seq_res_alloc_gpio,		\
> +}
> diff --git a/drivers/power/power_seq/power_seq_pwm.c
> b/drivers/power/power_seq/power_seq_pwm.c new file mode 100644
> index 0000000..a80514f
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_pwm.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * 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; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#ifdef CONFIG_PWM
> +
> +#include <linux/pwm.h>
> +
> +#ifdef CONFIG_OF
> +static int power_seq_of_parse_pwm(struct device *dev,
> +				  struct device_node *node,
> +				  struct platform_power_seq *seq,
> +				  unsigned int step_nbr)
> +{
> +	struct platform_power_seq_step *step = &seq->steps[step_nbr];
> +	int err;
> +
> +	err = of_property_read_string(node, "id",
> +				      &step->pwm.id);
> +	if (err) {
> +		power_seq_err(dev, seq, step_nbr,
> +			      "error reading id property\n");
> +		return err;
> +	}
> +
> +	err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
> +						   &step->pwm.enable);
> +	return err;
> +}
> +#else
> +#define of_power_seq_parse_pwm NULL
> +#endif
> +
> +static bool power_seq_res_compare_pwm(struct platform_power_seq_step
> *step1, +				      struct platform_power_seq_step *step2)
> +{
> +	return (!strcmp(step1->pwm.id, step2->pwm.id));
> +}
> +
> +static int power_seq_res_alloc_pwm(struct device *dev,
> +				  struct power_seq_resource *res)
> +{
> +	res->pwm = devm_pwm_get(dev, res->pdata->pwm.id);
> +	if (IS_ERR(res->pwm)) {
> +		dev_err(dev, "cannot get pwm \"%s\"\n", res->pdata->pwm.id);
> +		return PTR_ERR(res->pwm);
> +	}
> +
> +	return 0;
> +}
> +
> +static int power_seq_step_run_pwm(struct power_seq_step *step)
> +{
> +	if (step->pdata.gpio.enable) {
> +		return pwm_enable(step->resource->pwm);
> +	} else {
> +		pwm_disable(step->resource->pwm);
> +		return 0;
> +	}
> +}
> +
> +#define POWER_SEQ_PWM_TYPE {				\
> +	.name = "pwm",					\
> +	.need_resource = true,				\
> +	.of_parse = power_seq_of_parse_pwm,		\
> +	.step_run = power_seq_step_run_pwm,		\
> +	.res_compare = power_seq_res_compare_pwm,	\
> +	.res_alloc = power_seq_res_alloc_pwm,		\
> +}
> +
> +#else
> +
> +#define POWER_SEQ_PWM_TYPE {}
> +
> +#endif
> diff --git a/drivers/power/power_seq/power_seq_regulator.c
> b/drivers/power/power_seq/power_seq_regulator.c new file mode 100644
> index 0000000..915eac1
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_regulator.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * 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; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#ifdef CONFIG_REGULATOR
> +
> +#include <linux/regulator/consumer.h>
> +
> +/* TODO change "which" */
> +#ifdef CONFIG_OF
> +static int power_seq_of_parse_regulator(struct device *dev,
> +					struct device_node *node,
> +					struct platform_power_seq *seq,
> +					unsigned int step_nbr)
> +{
> +	struct platform_power_seq_step *step = &seq->steps[step_nbr];
> +	int err;
> +
> +	err = of_property_read_string(node, "id",
> +				      &step->regulator.id);
> +	if (err) {
> +		power_seq_err(dev, seq, step_nbr,
> +			      "error reading id property\n");
> +		return err;
> +	}
> +
> +	err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
> +						   &step->regulator.enable);
> +	return err;
> +}
> +#else
> +#define of_power_seq_parse_regulator NULL
> +#endif
> +
> +static bool
> +power_seq_res_compare_regulator(struct platform_power_seq_step *step1,
> +				struct platform_power_seq_step *step2)
> +{
> +	return (!strcmp(step1->regulator.id, step2->regulator.id));
> +}
> +
> +static int power_seq_res_alloc_regulator(struct device *dev,
> +					struct power_seq_resource *res)
> +{
> +	res->regulator = devm_regulator_get(dev, res->pdata->regulator.id);
> +	if (IS_ERR(res->regulator)) {
> +		dev_err(dev, "cannot get regulator \"%s\"\n",
> +			res->pdata->regulator.id);
> +		return PTR_ERR(res->regulator);
> +	}
> +
> +	return 0;
> +}
> +
> +static int power_seq_step_run_regulator(struct power_seq_step *step)
> +{
> +	if (step->pdata.regulator.enable)
> +		return regulator_enable(step->resource->regulator);
> +	else
> +		return regulator_disable(step->resource->regulator);
> +}
> +
> +#define POWER_SEQ_REGULATOR_TYPE {			\
> +	.name = "regulator",				\
> +	.need_resource = true,				\
> +	.of_parse = power_seq_of_parse_regulator,	\
> +	.step_run = power_seq_step_run_regulator,	\
> +	.res_compare = power_seq_res_compare_regulator,	\
> +	.res_alloc = power_seq_res_alloc_regulator,	\
> +}
> +
> +#else
> +
> +#define POWER_SEQ_REGULATOR_TYPE {}
> +
> +#endif
> diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> new file mode 100644
> index 0000000..78e8d77
> --- /dev/null
> +++ b/include/linux/power_seq.h
> @@ -0,0 +1,174 @@
> +/*
> + * power_seq.h
> + *
> + * Simple interpreter for defining power sequences as platform data or
> device + * tree properties.
> + *
> + * Power sequences are designed to replace the callbacks typically used in
> + * board-specific files that implement board-specific power sequences of
> devices + * such as backlights. A power sequence is an array of resources
> (which can a + * regulator, a GPIO, a PWM, ...) with an action to perform
> on it (enable or + * disable) and optional pre and post step delays. By
> having them interpreted + * instead of arbitrarily executed, it is
> possible to describe these in the + * device tree and thus remove
> board-specific code from the kernel. + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * 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; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __LINUX_POWER_SEQ_H
> +#define __LINUX_POWER_SEQ_H
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct regulator;
> +struct pwm_device;
> +struct device_node;
> +
> +/**
> + * The different kinds of resources that can be controlled during the
> sequences. + */
> +enum power_seq_res_type {
> +	POWER_SEQ_DELAY,
> +	POWER_SEQ_REGULATOR,
> +	POWER_SEQ_PWM,
> +	POWER_SEQ_GPIO,
> +	POWER_SEQ_NUM_TYPES,
> +};
> +
> +/**
> + * struct platform_power_seq_delay_step - platform data for delay steps
> + * @delay_us:	Amount of time to wait, in microseconds.
> + */
> +struct platform_power_seq_delay_step {
> +	unsigned int delay_us;
> +};
> +
> +/**
> + * struct platform_power_seq_regulator_step - platform data for regulator
> steps + * @id:		Name of the regulator to use. The regulator will be
> obtained + *		using devm_regulator_get(dev, name)
> + * @enable:	Whether to enable or disable the regulator during this step
> + */
> +struct platform_power_seq_regulator_step {
> +	const char *id;
> +	bool enable;
> +};
> +
> +/**
> + * struct platform_power_seq_pwm_step - platform data for PWM steps
> + * @id:		Name of the pwm to use. The PWM will be obtained using
> + *		devm_pwm_get(dev, name)
> + * @enable:	Whether to enable or disable the PWM during this step
> + */
> +struct platform_power_seq_pwm_step {
> +	const char *id;
> +	bool enable;
> +};
> +
> +/**
> + * struct platform_power_seq_gpio_step - platform data for GPIO steps
> + * @number:	Number of the GPIO to use. The GPIO will be obtained using
> + *		devm_gpio_request_one(dev, number)
> + * @enable:	Whether to enable or disable the GPIO during this step
> + */
> +struct platform_power_seq_gpio_step {
> +	int number;
> +	bool enable;
> +};
> +
> +/**
> + * struct platform_power_seq_step - platform data for power sequences
> steps + * @type:	The type of this step. This decides which member of the
> union is + *		valid for this step.
> + * @delay:	Used if type == POWER_SEQ_DELAY
> + * @regulator:	Used if type == POWER_SEQ_REGULATOR
> + * @pwm:	Used if type == POWER_SEQ_PWN
> + * @gpio:	Used if type == POWER_SEQ_GPIO
> + */
> +struct platform_power_seq_step {
> +	enum power_seq_res_type type;
> +	union {
> +		struct platform_power_seq_delay_step delay;
> +		struct platform_power_seq_regulator_step regulator;
> +		struct platform_power_seq_pwm_step pwm;
> +		struct platform_power_seq_gpio_step gpio;
> +	};
> +};
> +
> +/**
> + * struct platform_power_seq - platform data for power sequences
> + * @id:		Name through which this sequence is refered
> + * @num_steps:	Number of steps in that sequence
> + * @steps:	Array of num_steps steps describing the sequence
> + */
> +struct platform_power_seq {
> +	const char *id;
> +	unsigned int num_steps;
> +	struct platform_power_seq_step steps[];
> +};
> +
> +/**
> + * struct platform_power_seq_set - platform data for sets of sequences
> + * @num_seqs:	Number of sequences in this set
> + * @seqs:	Array of pointers to individual sequences
> + */
> +struct platform_power_seq_set {
> +	unsigned int num_seqs;
> +	struct platform_power_seq* seqs[];
> +};
> +
> +/**
> + * struct power_seq_resource - resource used by a power sequence set
> + * @pdata:	Pointer to the platform data used to resolve this resource
> + * @regulator:	Resolved regulator if of type POWER_SEQ_REGULATOR
> + * @pwm:	Resolved PWM if of type POWER_SEQ_PWM
> + * @list:	Used to link resources together
> + */
> +struct power_seq_resource {
> +	/* relevant for resolving the resource and knowing its type */
> +	struct platform_power_seq_step *pdata;
> +	/* resolved resource (if any) */
> +	union {
> +		struct regulator *regulator;
> +		struct pwm_device *pwm;
> +	};
> +	struct list_head list;
> +};
> +#define power_seq_for_each_resource(pos, seqs)				\
> +	list_for_each_entry(pos, power_seq_set_resources(seqs), list)
> +
> +struct power_seq_resource;
> +struct power_seq;
> +struct power_seq_set;
> +
> +#ifdef CONFIG_OF
> +struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device
> *dev); +#else
> +inline struct platform_power_seq_set *of_parse_power_seq_set(struct device
> *dev) +{
> +	return NULL;
> +}
> +#endif
> +void devm_platform_power_seq_set_free(struct device *dev,
> +				      struct platform_power_seq_set *pseq);
> +
> +struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> +					   struct platform_power_seq_set *pseq);
> +struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
> +struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char
> *id); +int power_seq_run(struct power_seq *seq);
> +
> +#endif


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-09-06 14:14   ` Heiko Stübner
@ 2012-09-07  8:04     ` Alex Courbot
  2012-09-07  8:15       ` Mark Brown
  2012-09-07  9:08       ` Heiko Stübner
  0 siblings, 2 replies; 16+ messages in thread
From: Alex Courbot @ 2012-09-07  8:04 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss, linux-pm, linux-doc

Hi Heiko,

On Thursday 06 September 2012 22:14:53 Heiko Stübner wrote:
> Hi Alexander,
> 
> Am Freitag, 31. August 2012, 13:34:03 schrieb Alexandre Courbot:
> > Some device drivers (panel backlights especially) need to follow precise
> > sequences for powering on and off, involving gpios, regulators, PWMs
> > with a precise powering order and delays to respect between each steps.
> > These sequences are board-specific, and do not belong to a particular
> > driver - therefore they have been performed by board-specific hook
> > functions to far.
> > 
> > With the advent of the device tree and of ARM kernels that are not
> > board-tied, we cannot rely on these board-specific hooks anymore but
> > need a way to implement these sequences in a portable manner. This patch
> > introduces a simple interpreter that can execute such power sequences
> > encoded either as platform data or within the device tree.
> > 
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> I really like this idea, also because I'll have to solve similar problems on
> the way to dt for my tinker-platform.

Glad to read that! :)

> For your power_seq_run function you write that it simply returns an error
> code on failure and looking through it I also just found the error return
> statement. This would leave a device half turned on.
> 
> So I'm wondering, if it shouldn't turn off all the things it turned on until
> the step that produced the error. All your possible step types (execpt the
> delay) are booleans, so it should be possible to simply negate them when
> backtracking through the previous steps.

Indeed, I think you raised an important point. Right now all step types are 
invertible, but we cannot rely on that statement to be true forever. For 
instance, one short-term improvement will be to allow finer regulator control, 
like voltage setting. In this case, how can we go back to the initial state 
without recording it?

If e.g. the power on sequence fails at step N (of M steps for that sequence), 
one could try playing the corresponding power off sequence (either completely 
of from step M - N), but then again we cannot rely on sequences to be 
perfectly symetrical. Maybe this is more something for the calling driver to 
check for and control?

Alex.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-09-07  8:04     ` Alex Courbot
@ 2012-09-07  8:15       ` Mark Brown
  2012-09-07  9:08       ` Heiko Stübner
  1 sibling, 0 replies; 16+ messages in thread
From: Mark Brown @ 2012-09-07  8:15 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Heiko Stübner, Stephen Warren, Thierry Reding, Simon Glass,
	Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss, linux-pm, linux-doc

On Fri, Sep 07, 2012 at 05:04:24PM +0900, Alex Courbot wrote:

> If e.g. the power on sequence fails at step N (of M steps for that sequence), 
> one could try playing the corresponding power off sequence (either completely 
> of from step M - N), but then again we cannot rely on sequences to be 
> perfectly symetrical. Maybe this is more something for the calling driver to 
> check for and control?

That had been my thought too - depending on what the sequence is for it
may be that the corrective action is something very different to
reversing the sequence, for example a device reset may be required.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-09-05 17:19   ` Stephen Warren
@ 2012-09-07  8:21     ` Alex Courbot
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Courbot @ 2012-09-07  8:21 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss, linux-pm, linux-doc

Hi Stephen,

Skipping the typos and rephrasing issues (which will all be addressed, 
thanks!), these issues caught my attention more particularly:

On Thursday 06 September 2012 01:19:45 Stephen Warren wrote:
> > +"regulator" type required properties:
> > +  - id: name of the regulator to use. Regulator is obtained by
> > +        regulator_get(dev, id)
> > +  - enable / disable: one of these two empty properties must be present
> > to
> > +                      enable or disable the resource
> > +
> > +"pwm" type required properties:
> > +  - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
> > +  - enable / disable: one of these two empty properties must be present
> > to
> > +                      enable or disable the resource
> 
> For those two, would "name" be a better property name than "id"?

IIRC "name" is a reserved property name in the DT (I remember seeing an error 
message when compiling nodes with a "name" property that did not match the 
node's name). "id" was the second best candidate in my mind.

> > +"gpio" type required properties:
> > +  - number: phandle of the GPIO to use.
> 
> Naming the property "gpio" would seem more consistent with our GPIO
> properties.

Ok, although "gpio.gpio" might look strange in the platform data.

> > +  - enable / disable: one of these two empty properties must be present
> > to
> > +                      enable or disable the resource
> 
> You can't really enable or disable a GPIO (well, perhaps you can, but
> I'd consider that to affect tri-state rather than value); it's more that
> you're setting the output value to 0 or 1. I think a "value" or
> "set-value" property with value <0> or <1> would be better.

Right - will fix that.

> Overall, the general structure of the bindings looks reasonable to me.

Great, maybe we are finally headed to something stable!

> I'm not sure what "pdata" is supposed to point at; platform data applies
> to the original "struct device", not one of the resources used by the
> power sequences.

Right - more on this later.

> Hmmm. It'd be nice not to need separate functions for the non-DT and DT
> cases. That would require that devm_power_seq_set_build() be able to
> find the power sequence definitions somewhere other than platform data
> in the non-DT case - that's exactly why the regulator and pinctrl
> subsystems represent the device<->data mapping table separately from the
> device's platform data.

Oh, that sounds better indeed - I was still mentally stuck in the previous 
scheme where power sequences were not accessible from a fixed node of the 
device. Thanks.

> > +++ b/drivers/power/power_seq/Kconfig
> > 
> > +config POWER_SEQ
> > +	bool
> 
> Some kind of help text might be useful?

As this option was not user-visible but automatically enabled by drivers, I 
did not judge it to be necessary - but after reading your other mail we might 
need to make it visible and documented after all.

> > +/**
> > + * struct platform_power_seq_step - platform data for power sequences
> > steps + * @type:	The type of this step. This decides which member of 
the
> > union is + *		valid for this step.
> > + * @delay:	Used if type == POWER_SEQ_DELAY
> > + * @regulator:	Used if type == POWER_SEQ_REGULATOR
> > + * @pwm:	Used if type == POWER_SEQ_PWN
> > + * @gpio:	Used if type == POWER_SEQ_GPIO
> 
> In those last 4 line, I think s/type/@type/ since you're referencing
> another parameter?

Absolutely.

> > +struct power_seq_resource {
> > +	/* relevant for resolving the resource and knowing its type */
> > +	struct platform_power_seq_step *pdata;
> 
> Aha. So this isn't really platform data for the resource, but actually a
> step definition that referenced it. I think it'd be better to rename
> this field "step", and amend the documentation above not to refer to
> "pdata" but explicitly talk about a step definition; the step may have
> been defined in pdata, but isn't in the DT case.

This is a little bit confusing, isn't it? The resource identifier is duplicated 
in the platform data by every step that uses it. To avoid copying that 
information again into the resource structure, I just use this pointer to the 
first step that uses this resource. So a step not only contains step 
information, but also part of it might be used by a resource instance. Ugh, 
that's horribly confusing, actually.

> Alternatively, why not just copy the step type enum here, rather than
> referencing the step definition?

On top of the type we will also need the identifier of the resource (string for 
pwm and regulator, number for GPIO) so we can compare the resource against 
platform data when building the sequence to avoid allocating it twice. That's 
a little bit of extra memory, but the gain in clarity is probably worth it.

Alex.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 2/4] pwm_backlight: use power sequences
  2012-09-05 17:25   ` Stephen Warren
@ 2012-09-07  8:28     ` Alex Courbot
  2012-09-07  8:29       ` Mark Brown
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Courbot @ 2012-09-07  8:28 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, linux-fbdev, linux-pm, devicetree-discuss,
	linux-doc, linux-kernel, linux-tegra

On Thursday 06 September 2012 01:25:27 Stephen Warren wrote:
> On 08/31/2012 05:34 AM, Alexandre Courbot wrote:
> > Make use of the power sequences specified in the device tree or platform
> > data to control how the backlight is powered on and off.
> > 
> > +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > 
> >  Optional properties:
> > -  - pwm-names: a list of names for the PWM devices specified in the
> > -               "pwms" property (see PWM binding[0])
> > +  - pwm-names: name for the PWM device specified in the "pwms" property
> > (see PWM +      binding[0]). Necessary if power sequences are used
> 
> So this implies that power sequence are completely optional in the pwm
> binding...
> 
> > diff --git a/drivers/video/backlight/Kconfig
> > b/drivers/video/backlight/Kconfig> 
> >  config BACKLIGHT_PWM
> >  
> >  	tristate "Generic PWM based Backlight Driver"
> >  	depends on PWM
> > 
> > +	select POWER_SEQ
> 
> ... but that implies they're basically mandatory.
> 
> Briefly looking at the code, power sequences don't appear to be
> optional, at least for the DT case, so perhaps you just need to update
> the documentation to make "pwm-names" non-optional?

This has to do with how power sequences are enabled during the build. Instead 
of providing yet-another-kernel-option, I thought it would be better to make 
it invisible and let drivers that take advantage of power seqs enable the 
option by themselves when they are selected. That's why power sequences are 
unconditionally compiled when pwm-backlight is selected.

But on the other hand, pwm-backlight already has a DT interface that does not 
use power sequences, and its current users are still relying on the legacy 
platform data interface (with one PWM and some callback functions). Making the 
power sequences mandatory in the DT bindings would make it impossible to use 
that legacy interface.

We could make power sequences an option of its own and add #ifdefs to drivers 
that use it to lift this ambiguity, but I like the transparency of the current 
way. It also seems hard (illegal?) to get rid of the legacy DT interface.

Alex.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 2/4] pwm_backlight: use power sequences
  2012-09-07  8:28     ` Alex Courbot
@ 2012-09-07  8:29       ` Mark Brown
  2012-09-07  8:34         ` Alex Courbot
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2012-09-07  8:29 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Stephen Warren, Stephen Warren, Thierry Reding, Simon Glass,
	Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, linux-fbdev, linux-pm, devicetree-discuss,
	linux-doc, linux-kernel, linux-tegra

On Fri, Sep 07, 2012 at 05:28:17PM +0900, Alex Courbot wrote:

> We could make power sequences an option of its own and add #ifdefs to drivers 
> that use it to lift this ambiguity, but I like the transparency of the current 
> way. It also seems hard (illegal?) to get rid of the legacy DT interface.

If you're doing this I'd suggest using stubs rather than ifdefs in the
users, otherwise it's just going to cause lots of annoyance from
randconfig build.  Is the code likely to big enough to worry about,
though?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 2/4] pwm_backlight: use power sequences
  2012-09-07  8:29       ` Mark Brown
@ 2012-09-07  8:34         ` Alex Courbot
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Courbot @ 2012-09-07  8:34 UTC (permalink / raw)
  To: Mark Brown
  Cc: Stephen Warren, Stephen Warren, Thierry Reding, Simon Glass,
	Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, linux-fbdev, linux-pm, devicetree-discuss,
	linux-doc, linux-kernel, linux-tegra

On Friday 07 September 2012 16:29:03 Mark Brown wrote:
> On Fri, Sep 07, 2012 at 05:28:17PM +0900, Alex Courbot wrote:
> > We could make power sequences an option of its own and add #ifdefs to
> > drivers that use it to lift this ambiguity, but I like the transparency
> > of the current way. It also seems hard (illegal?) to get rid of the
> > legacy DT interface.
> If you're doing this I'd suggest using stubs rather than ifdefs in the
> users, otherwise it's just going to cause lots of annoyance from
> randconfig build.  Is the code likely to big enough to worry about,
> though?

I don't think is will ever become big enough to bother. Moreover if the power 
seqs way meets acceptance, new drivers/frameworks are likely to use them as 
the only option, making it really mandatory.

Alex.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-09-07  8:04     ` Alex Courbot
  2012-09-07  8:15       ` Mark Brown
@ 2012-09-07  9:08       ` Heiko Stübner
  2012-09-07 16:36         ` Stephen Warren
  1 sibling, 1 reply; 16+ messages in thread
From: Heiko Stübner @ 2012-09-07  9:08 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
	Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
	Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss, linux-pm, linux-doc

Am Freitag, 7. September 2012, 10:04:24 schrieb Alex Courbot:
> > For your power_seq_run function you write that it simply returns an error
> > code on failure and looking through it I also just found the error return
> > statement. This would leave a device half turned on.
> > 
> > So I'm wondering, if it shouldn't turn off all the things it turned on
> > until the step that produced the error. All your possible step types
> > (execpt the delay) are booleans, so it should be possible to simply
> > negate them when backtracking through the previous steps.
> 
> Indeed, I think you raised an important point. Right now all step types are
> invertible, but we cannot rely on that statement to be true forever. For
> instance, one short-term improvement will be to allow finer regulator
> control, like voltage setting. In this case, how can we go back to the
> initial state without recording it?
>
> If e.g. the power on sequence fails at step N (of M steps for that
> sequence), one could try playing the corresponding power off sequence
> (either completely of from step M - N), but then again we cannot rely on
> sequences to be perfectly symetrical. Maybe this is more something for the
> calling driver to check for and control?

Am Freitag, 7. September 2012, 10:15:03 schrieb Mark Brown:
> On Fri, Sep 07, 2012 at 05:04:24PM +0900, Alex Courbot wrote:
> > If e.g. the power on sequence fails at step N (of M steps for that
> > sequence), one could try playing the corresponding power off sequence
> > (either completely of from step M - N), but then again we cannot rely on
> > sequences to be perfectly symetrical. Maybe this is more something for
> > the calling driver to check for and control?
> 
> That had been my thought too - depending on what the sequence is for it
> may be that the corrective action is something very different to
> reversing the sequence, for example a device reset may be required.


If I understood the description correctly, the power sequence should be 
transparent to the driver, as it implements board specific actions and 
shouldn't bother the driver with it to much. Therefore my thoughts went along 
the lines how gpio_request_array handles this, always producing a sane state 
at the end.

Recording the previous state, could be done by making a copy of the current 
sequence, and just noting the previous values (including voltages etc) in the 
respective entries. And in the error case running this new sequence from the 
error point instead to power down again.


As both Alex and Mark wrote, reversing the sequence might be the action of 
choice only for some devices, but others might need to run a completely 
different powerdown sequence and still others would need special handling.

Would it be possible to encode this in the sequence definition, something like
	on-error = "reverse"

	on-error = "sequence"
	error-seq = <&other_sequence>

	on-error = "driver"
with better names and types of course.

This would keep the power sequence transparent to most drivers and only the 
real esoteric ones would need to do their special handling on their own.


Heiko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
  2012-09-07  9:08       ` Heiko Stübner
@ 2012-09-07 16:36         ` Stephen Warren
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Warren @ 2012-09-07 16:36 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: Alex Courbot, linux-fbdev, Mark Brown, Stephen Warren, linux-pm,
	linux-doc, linux-kernel, Rob Herring, Anton Vorontsov,
	linux-tegra, David Woodhouse, devicetree-discuss

On 09/07/2012 03:08 AM, Heiko Stübner wrote:
> Am Freitag, 7. September 2012, 10:04:24 schrieb Alex Courbot:
>>> For your power_seq_run function you write that it simply returns an error
>>> code on failure and looking through it I also just found the error return
>>> statement. This would leave a device half turned on.
>>>
>>> So I'm wondering, if it shouldn't turn off all the things it turned on
>>> until the step that produced the error. All your possible step types
>>> (execpt the delay) are booleans, so it should be possible to simply
>>> negate them when backtracking through the previous steps.
>>
>> Indeed, I think you raised an important point. Right now all step types are
>> invertible, but we cannot rely on that statement to be true forever. For
>> instance, one short-term improvement will be to allow finer regulator
>> control, like voltage setting. In this case, how can we go back to the
>> initial state without recording it?
>>
>> If e.g. the power on sequence fails at step N (of M steps for that
>> sequence), one could try playing the corresponding power off sequence
>> (either completely of from step M - N), but then again we cannot rely on
>> sequences to be perfectly symetrical. Maybe this is more something for the
>> calling driver to check for and control?
> 
> Am Freitag, 7. September 2012, 10:15:03 schrieb Mark Brown:
>> On Fri, Sep 07, 2012 at 05:04:24PM +0900, Alex Courbot wrote:
>>> If e.g. the power on sequence fails at step N (of M steps for that
>>> sequence), one could try playing the corresponding power off sequence
>>> (either completely of from step M - N), but then again we cannot rely on
>>> sequences to be perfectly symetrical. Maybe this is more something for
>>> the calling driver to check for and control?
>>
>> That had been my thought too - depending on what the sequence is for it
>> may be that the corrective action is something very different to
>> reversing the sequence, for example a device reset may be required.
> 
> If I understood the description correctly, the power sequence should be 
> transparent to the driver, as it implements board specific actions and 
> shouldn't bother the driver with it to much.

Well, the contents/implementation of the sequence should be transparent
to the driver. The fact that a sequence exists and needs to be executed
obviously can't be transparent to the driver, since the driver needs to
call an API to execute the sequence.

I'd assert that requiring the driver to get back to a sane state by
executing sequence (b) if sequence (a) fails is fairly reasonable, and
doesn't give the driver any more knowledge of what the sequences are
than what it already has.

But then I start to wonder: What if the "help something went wrong"
sequence gets an error...

> Therefore my thoughts went along
> the lines how gpio_request_array handles this, always producing a sane state 
> at the end.
> 
> Recording the previous state, could be done by making a copy of the current 
> sequence, and just noting the previous values (including voltages etc) in the 
> respective entries. And in the error case running this new sequence from the 
> error point instead to power down again.
> 
> 
> As both Alex and Mark wrote, reversing the sequence might be the action of 
> choice only for some devices, but others might need to run a completely 
> different powerdown sequence and still others would need special handling.
> 
> Would it be possible to encode this in the sequence definition, something like
> 	on-error = "reverse"
> 
> 	on-error = "sequence"
> 	error-seq = <&other_sequence>
> 
> 	on-error = "driver"
> with better names and types of course.
> 
> This would keep the power sequence transparent to most drivers and only the 
> real esoteric ones would need to do their special handling on their own.

Yes, something like that sounds reasonable on the surface. I'm not sure
about the on-error="driver" case though; if the driver knows nothing
about the content of the sequences, I'm not sure how the driver could
possibly do anything other than execute some sequence to recover.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-09-07 16:36 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-31 11:34 [PATCH v5 0/4] Runtime Interpreted Power Sequences Alexandre Courbot
2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot
2012-09-05 17:19   ` Stephen Warren
2012-09-07  8:21     ` Alex Courbot
2012-09-06 14:14   ` Heiko Stübner
2012-09-07  8:04     ` Alex Courbot
2012-09-07  8:15       ` Mark Brown
2012-09-07  9:08       ` Heiko Stübner
2012-09-07 16:36         ` Stephen Warren
2012-08-31 11:34 ` [PATCH v5 2/4] pwm_backlight: use power sequences Alexandre Courbot
2012-09-05 17:25   ` Stephen Warren
2012-09-07  8:28     ` Alex Courbot
2012-09-07  8:29       ` Mark Brown
2012-09-07  8:34         ` Alex Courbot
2012-08-31 11:34 ` [PATCH v5 3/4] tegra: dt: add label to tegra20's PWM Alexandre Courbot
2012-08-31 11:34 ` [PATCH v5 4/4] tegra: ventana: add pwm backlight DT nodes Alexandre Courbot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).