[PATCH v6 0/4] Runtime Interpreted Power Sequences

[PATCH v6 0/4] Runtime Interpreted Power Sequences

[Alexandre Courbot]

New revision of the power sequences, taking as usual the feedback that was
kindly provided about the last version.

I think now is a good time to discuss integrating this and to start looking for
a maintainer who would be willing to merge this into his/her tree (I am
especially thinking about the power framework maintainers, since this is where
the code is right now. The second patch in this series enables the pwm_backlight
driver to be used with the device tree, without relying on board-dependent
callbacks to support complex power sequences. We also plan to use power
sequences in other Tegra drivers, and other people have expressed interest in
this work during earlier reviews. See for instance

https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-August/018532.html

and

https://lkml.org/lkml/2012/9/6/270

There is probably some more details to fix and improve, but the current shape
should be enough to know if we want this and where - therefore any sign from
a maintainer would be greatly appreciated!

Changes since v5:
* Removed pointers to platform data from resource structure for better code
  clarity
* devm_power_seq_set_build() now automatically checks the DT if no platform
  data is present, making it possible to remove the explicit DT parsing function
* Lots of fixes in the documentation which should be clearer now (thanks
  Stephen!)

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    | 122 ++++++
 .../bindings/video/backlight/pwm-backlight.txt     |  65 ++-
 Documentation/power/power_seq.txt                  | 215 ++++++++++
 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           |  91 +++++
 drivers/power/power_seq/power_seq_pwm.c            |  87 ++++
 drivers/power/power_seq/power_seq_regulator.c      |  87 ++++
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 180 ++++++---
 include/linux/power_seq.h                          | 172 ++++++++
 include/linux/pwm_backlight.h                      |  15 +-
 18 files changed, 1543 insertions(+), 55 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

[PATCH v6 1/4] Runtime Interpreted Power Sequences

[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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/power_seq/power_seq.txt    | 122 ++++++
 Documentation/power/power_seq.txt                  | 215 ++++++++++
 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           |  91 +++++
 drivers/power/power_seq/power_seq_pwm.c            |  87 ++++
 drivers/power/power_seq/power_seq_regulator.c      |  87 ++++
 include/linux/power_seq.h                          | 172 ++++++++
 12 files changed, 1276 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..2b394eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,122 @@
+Runtime Interpreted Power Sequences
+===================================
+
+Power sequences are sequential descriptions of actions to be performed on
+power-related resources. Having these descriptions in a well-defined 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.
+
+A device typically makes use of multiple power sequences, for different purposes
+such as powering on and off. All the power sequences of a given device are
+grouped into a set. In the device tree, this set is a sub-node of the device
+node named "power-sequences".
+
+Power Sequences Structure
+-------------------------
+Every device that makes use of power sequences must have a "power-sequences"
+node into which individual power sequences are declared as sub-nodes. The name
+of the node becomes the name of the sequence within the power sequences
+framework.
+
+Similarly, each power sequence declares its steps as sub-nodes of itself. 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
+---------------------
+Steps of a sequence describe 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: 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:
+  - gpio: phandle of the GPIO to use.
+  - value: value this GPIO should take. Must be 0 or 1.
+
+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 = <10000>;
+				};
+				step2 {
+					type = "pwm";
+					id = "backlight";
+					enable;
+				};
+				step3 {
+					type = "gpio";
+					gpio = <&gpio 28 0>;
+					value = <1>;
+				};
+			};
+
+			power-off {
+				step0 {
+					type = "gpio";
+					gpio = <&gpio 28 0>;
+					value = <0>;
+				};
+				step1 {
+					type = "pwm";
+					id = "backlight";
+					disable;
+				};
+				step2 {
+					type = "delay";
+					delay = <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 (for
+instance, regulators and pwms are resolved by name using regulator_get() or
+pwm_get()).
+
+After the resources declaration, two sequences follow for powering the backlight
+on and off. Their names are specified by the pwm-backlight device bindings. Once
+the sequences are built by calling devm_power_seq_set_build() with a NULL pseq
+argument, their names can be passed to power_seq_lookup() to retrieve an actual
+sequence.
diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
new file mode 100644
index 0000000..7f3f2d2
--- /dev/null
+++ b/Documentation/power/power_seq.txt
@@ -0,0 +1,215 @@
+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 device drivers to work without power callbacks and makes the
+kernel less board-dependant.
+
+What are Power Sequences?
+-------------------------
+A power sequence is a suite of sequential steps each describing an action to be
+performed on a resource. The supported resources and actions operations are:
+- delay (just wait for a given number of microseconds)
+- GPIO (set to 0 or 1)
+- regulator (enable or disable)
+- PWM (enable or disable)
+
+When a power sequence is run, each of its steps is executed one after the other
+until one step fails or the end of the sequence is reached.
+
+Power sequences are named, and grouped into "sets" which correspond to all the
+sequences of a device as well as the resources they use.
+
+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 device may include an instance of platform_power_seq_set
+which references instances of platform_power_seq. Each platform_power_seq is a
+single power sequence, and is itself made of a variable length array of steps.
+
+A step is a union of all the platform step structures, which are defined and
+documented in include/linux/power_seq.h. Which one is to be used depends on the
+type of the step. This might sound complicated, but the following example should
+make it clear how the platform data for power sequences is defined. It declares
+two power sequences named "power-on" and "power-off". The
+"power-on" sequence enables a regulator named "power" (retrieved using
+regulator_get()), waits for 10ms, and then set GPIO 110 to 1. "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 = 10000,
+			},
+		},
+		{
+			.type = POWER_SEQ_GPIO,
+			.gpio = {
+				.gpio = 110,
+				.value = 1,
+			},
+		},
+	},
+};
+
+static struct platform_power_seq power_off_seq = {
+	.id = "power-off",
+	.num_steps = 3,
+	.steps = {
+		{
+			.type = POWER_SEQ_GPIO,
+			.gpio = {
+				.gpio = 110,
+				.value = 0,
+			},
+		},
+		{
+			.type = POWER_SEQ_DELAY,
+			.delay = {
+				.delay = 10000,
+			},
+		},
+		{
+			.type = POWER_SEQ_REGULATOR,
+			.regulator = {
+				.id = "power",
+				.enable = false,
+			},
+		},
+	},
+};
+
+static struct platform_power_seq_set platform_power_sequences = {
+	.num_seqs = 2,
+	.seqs = {
+		&power_on_seq,
+		&power_off_seq,
+	},
+};
+
+"platform_power_sequences" can then be passed to devm_power_seq_set_build() in
+order to build the corresponding sequences set and allocate all the necessary
+resources. More on this later in this document.
+
+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";
+			gpio = <&gpio 110 0>;
+			value = <1>;
+		};
+	}
+	power-off {
+		step0 {
+			type = "gpio";
+			gpio = <&gpio 110 0>;
+			value = <0>;
+		};
+		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 DT 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.
+
+If the pseq argument is NULL, then the power sequences are built from the device
+tree data, if any.
+
+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. If no set can be built because no platform data is passed and the
+device tree node for the device contains no power sequence, the function returns
+NULL.
+
+Once the set is built, power sequences can be retrieved by their name using
+power_seq_lookup:
+
+  struct power_seq *power_seq_lookup(struct power_seq_set *seqs,
+				     const char *id);
+
+A retrieved power sequence can then 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 occurred.
+
+Sometimes, you may want to browse the list of resources allocated by a sequence,
+for instance to 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 a pointer to a struct power_seq_resource. This structure
+contains the type of the resource, the information used for identifying it, and
+the resolved resource itself.
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..0924a03
--- /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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
+ *
+ * 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 the resource is the
+ *			same as the one referenced by the step, false otherwise.
+ * @res_alloc:		Resolve and allocate the resource used by pstep into
+ *			seq. The memory for seq is already allocated and its
+ *			type member is already set when this function is called.
+ *			Return error code if the resource cannot be allocated, 0
+ *			if allocation went successfully.
+ */
+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 power_seq_resource *res,
+			    struct platform_power_seq_step *step);
+	int (*res_alloc)(struct device *dev,
+			 struct platform_power_seq_step *pstep,
+			 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);
+}
+
+/*
+ * build power sequences platform data from a device tree node.
+ *
+ * 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.
+ */
+static
+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;
+}
+#endif /* CONFIG_OF */
+
+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) {
+		if (res->type != step->type)
+			continue;
+
+		if (power_seq_types[res->type].res_compare(res, 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->type = step->pdata.type;
+
+			err = power_seq_types[res->type].res_alloc(dev,
+							     &step->pdata, res);
+			if (err < 0)
+				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 or device tree
+ * @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. If this parameter is null, power
+ *		sequences are looked up in the device tree.
+ *
+ * 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,
+ * and NULL if neither platform data or device tree nodes are defined.
+ */
+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;
+	bool use_dt = false;
+
+	if (!pseq) {
+		use_dt = true;
+		pseq = devm_of_parse_power_seq_set(dev);
+	}
+
+	if (!pseq)
+		return NULL;
+
+	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);
+	}
+
+	/* if we used the DT, free the temporarily built platform data */
+	if (use_dt) {
+		for (i = 0; i < pseq->num_seqs; i++)
+			devm_kfree(dev, pseq->seqs[i]);
+
+		devm_kfree(dev, pseq);
+	}
+
+	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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+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..ae94131
--- /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",
+				   &step->delay.delay);
+	if (err < 0)
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading delay 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,
+		     step->pdata.delay.delay + 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..4f83d4c
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_gpio.c
@@ -0,0 +1,91 @@
+/*
+ * 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, "gpio", 0);
+	if (gpio < 0) {
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading gpio property\n");
+		return gpio;
+	}
+	step->gpio.gpio = gpio;
+
+	err = of_property_read_u32(node, "value", &step->gpio.value);
+	if (err < 0) {
+		power_seq_err(dev, seq, step_nbr,
+			      "error reading value property\n");
+	} else if (step->gpio.value < 0 || step->gpio.value > 1) {
+		power_seq_err(dev, seq, step_nbr,
+			      "value out of range (must be 0 or 1)\n");
+		err = -EINVAL;
+	}
+
+	return err;
+}
+#else
+#define of_power_seq_parse_gpio NULL
+#endif
+
+static bool power_seq_res_compare_gpio(struct power_seq_resource *res,
+				       struct platform_power_seq_step *step)
+{
+	return res->gpio.gpio == step->gpio.gpio;
+}
+
+static int power_seq_res_alloc_gpio(struct device *dev,
+				    struct platform_power_seq_step *pstep,
+				    struct power_seq_resource *res)
+{
+	int err;
+
+	err = devm_gpio_request_one(dev, pstep->gpio.gpio,
+				    GPIOF_OUT_INIT_LOW, dev_name(dev));
+	if (err) {
+		dev_err(dev, "cannot get gpio %d\n", pstep->gpio.gpio);
+		return err;
+	}
+
+	res->gpio.gpio = pstep->gpio.gpio;
+
+	return 0;
+}
+
+static int power_seq_step_run_gpio(struct power_seq_step *step)
+{
+	gpio_set_value_cansleep(step->resource->gpio.gpio,
+				step->pdata.gpio.value);
+
+	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..14f21e1
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_pwm.c
@@ -0,0 +1,87 @@
+/*
+ * 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 power_seq_resource *res,
+				      struct platform_power_seq_step *step)
+{
+	return !strcmp(res->pwm.id, step->pwm.id);
+}
+
+static int power_seq_res_alloc_pwm(struct device *dev,
+				   struct platform_power_seq_step *pstep,
+				   struct power_seq_resource *res)
+{
+	res->pwm.pwm = devm_pwm_get(dev, pstep->pwm.id);
+	if (IS_ERR(res->pwm.pwm)) {
+		dev_err(dev, "cannot get pwm \"%s\"\n", pstep->pwm.id);
+		return PTR_ERR(res->pwm.pwm);
+	}
+	res->pwm.id = pstep->pwm.id;
+
+	return 0;
+}
+
+static int power_seq_step_run_pwm(struct power_seq_step *step)
+{
+	if (step->pdata.pwm.enable) {
+		return pwm_enable(step->resource->pwm.pwm);
+	} else {
+		pwm_disable(step->resource->pwm.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..2356578
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_regulator.c
@@ -0,0 +1,87 @@
+/*
+ * 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>
+
+#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 power_seq_resource *res,
+				struct platform_power_seq_step *step)
+{
+	return !strcmp(res->regulator.id, step->regulator.id);
+}
+
+static int power_seq_res_alloc_regulator(struct device *dev,
+					 struct platform_power_seq_step *pstep,
+					 struct power_seq_resource *res)
+{
+	res->regulator.regulator = devm_regulator_get(dev, pstep->regulator.id);
+	if (IS_ERR(res->regulator.regulator)) {
+		dev_err(dev, "cannot get regulator \"%s\"\n",
+			pstep->regulator.id);
+		return PTR_ERR(res->regulator.regulator);
+	}
+	res->regulator.id = pstep->regulator.id;
+
+	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.regulator);
+	else
+		return regulator_disable(step->resource->regulator.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..437a61a
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,172 @@
+/*
+ * 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
+ *
+ * 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>
+#include <net/irda/parameters.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:	Amount of time to wait, in microseconds.
+ */
+struct platform_power_seq_delay_step {
+	unsigned int delay;
+};
+
+/**
+ * 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
+ * @gpio:	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 gpio;
+	int value;
+};
+
+/**
+ * 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 {
+	enum power_seq_res_type type;
+	/* resolved resource and identifier */
+	union {
+		struct {
+			struct regulator *regulator;
+			const char *id;
+		} regulator;
+		struct {
+			struct pwm_device *pwm;
+			const char *id;
+		} pwm;
+		struct {
+			int gpio;
+		} gpio;
+	};
+	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;
+
+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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 09/12/2012 03:57 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.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt

> +Sometimes, you may want to browse the list of resources allocated by a sequence,
> +for instance to 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);

I don't think you need to include that prototype here?

> +  power_seq_for_each_resource(pos, seqs)
> +
> +Here "pos" will be a pointer to a struct power_seq_resource. This structure
> +contains the type of the resource, the information used for identifying it, and
> +the resolved resource itself.

> 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

Don't you need to compile all the power_seq_*.c too?

Oh, I see the following in power_seq.c:

> +#include "power_seq_delay.c"
> +#include "power_seq_regulator.c"
> +#include "power_seq_pwm.c"
> +#include "power_seq_gpio.c"

It's probably better just to compile them separately and link them.

> diff --git a/drivers/power/power_seq/power_seq.c b/drivers/power/power_seq/power_seq.c

> +struct power_seq_step {
> +	/* Copy of the platform data */
> +	struct platform_power_seq_step pdata;

I'd reword the comment to "Copy of the step", and name the field "step".

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

Ah, I see why you're using #include now.

> +MODULE_LICENSE("GPL");

s/GPL/GPL v2/ given the license header.

> diff --git a/drivers/power/power_seq/power_seq_gpio.c b/drivers/power/power_seq/power_seq_gpio.c

> +static int power_seq_res_alloc_gpio(struct device *dev,
> +				    struct platform_power_seq_step *pstep,
> +				    struct power_seq_resource *res)
> +{
> +	int err;
> +
> +	err = devm_gpio_request_one(dev, pstep->gpio.gpio,
> +				    GPIOF_OUT_INIT_LOW, dev_name(dev));

Hmm. The INIT_LOW part of that might be somewhat presumptive. I would
suggest simply requesting the GPIO here, and using
gpio_direction_output() in power_seq_step_run_gpio(), thus deferring the
decision of what value to set the GPIO to until a real sequence is
actually run.

> diff --git a/drivers/power/power_seq/power_seq_pwm.c b/drivers/power/power_seq/power_seq_pwm.c

> diff --git a/drivers/power/power_seq/power_seq_regulator.c b/drivers/power/power_seq/power_seq_regulator.c

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

> +#include <net/irda/parameters.h>

That looks out of place.

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

I think that kerneldoc is stale.

> +struct power_seq_resource {
> +	enum power_seq_res_type type;
> +	/* resolved resource and identifier */
> +	union {
> +		struct {
> +			struct regulator *regulator;
> +			const char *id;
> +		} regulator;
> +		struct {
> +			struct pwm_device *pwm;
> +			const char *id;
> +		} pwm;
> +		struct {
> +			int gpio;
> +		} gpio;
> +	};
> +	struct list_head list;
> +};

Aside from those minor issues, this all looks reasonable to me, so,
Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 06:07:13 Stephen Warren wrote:
> On 09/12/2012 03:57 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.
> > 
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> > 
> > diff --git a/Documentation/power/power_seq.txt
> > b/Documentation/power/power_seq.txt
> > 
> > +Sometimes, you may want to browse the list of resources allocated by a
> > sequence, +for instance to 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);
> 
> I don't think you need to include that prototype here?

Why not? I thought it was customary to include the prototypes in the 
documentation, and this seems to be the right place for this function.

> > +  power_seq_for_each_resource(pos, seqs)
> > +
> > +Here "pos" will be a pointer to a struct power_seq_resource. This
> > structure +contains the type of the resource, the information used for
> > identifying it, and +the resolved resource itself.
> > 
> > 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
> 
> Don't you need to compile all the power_seq_*.c too?
> 
> Oh, I see the following in power_seq.c:
> > +#include "power_seq_delay.c"
> > +#include "power_seq_regulator.c"
> > +#include "power_seq_pwm.c"
> > +#include "power_seq_gpio.c"
> 
> It's probably better just to compile them separately and link them.
> 
> > diff --git a/drivers/power/power_seq/power_seq.c
> > b/drivers/power/power_seq/power_seq.c
> > 
> > +struct power_seq_step {
> > +	/* Copy of the platform data */
> > +	struct platform_power_seq_step pdata;
> 
> I'd reword the comment to "Copy of the step", and name the field "step".

That would make a step within a step - doesn't pdata make it more explicit 
what this member is for (containing the platform data for this step)?

> > +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,
> > +};
> 
> Ah, I see why you're using #include now.

We could also go with something more dynamic and compile these files 
separately, but that would require some registration mechanism which I don't 
think is needed for such a simple feature.

> 
> > +MODULE_LICENSE("GPL");
> 
> s/GPL/GPL v2/ given the license header.
> 
> > diff --git a/drivers/power/power_seq/power_seq_gpio.c
> > b/drivers/power/power_seq/power_seq_gpio.c
> > 
> > +static int power_seq_res_alloc_gpio(struct device *dev,
> > +				    struct platform_power_seq_step *pstep,
> > +				    struct power_seq_resource *res)
> > +{
> > +	int err;
> > +
> > +	err = devm_gpio_request_one(dev, pstep->gpio.gpio,
> > +				    GPIOF_OUT_INIT_LOW, dev_name(dev));
> 
> Hmm. The INIT_LOW part of that might be somewhat presumptive. I would
> suggest simply requesting the GPIO here, and using
> gpio_direction_output() in power_seq_step_run_gpio(), thus deferring the
> decision of what value to set the GPIO to until a real sequence is
> actually run.
> 
> > diff --git a/drivers/power/power_seq/power_seq_pwm.c
> > b/drivers/power/power_seq/power_seq_pwm.c
> > 
> > diff --git a/drivers/power/power_seq/power_seq_regulator.c
> > b/drivers/power/power_seq/power_seq_regulator.c
> > 
> > diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> > 
> > +#include <net/irda/parameters.h>
> 
> That looks out of place.

Totally, thanks. I don't even understand how it landed there in the first 
place.

> 
> > +/**
> > + * 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
> > + */
> 
> I think that kerneldoc is stale.
> 
> > +struct power_seq_resource {
> > +	enum power_seq_res_type type;
> > +	/* resolved resource and identifier */
> > +	union {
> > +		struct {
> > +			struct regulator *regulator;
> > +			const char *id;
> > +		} regulator;
> > +		struct {
> > +			struct pwm_device *pwm;
> > +			const char *id;
> > +		} pwm;
> > +		struct {
> > +			int gpio;
> > +		} gpio;
> > +	};
> > +	struct list_head list;
> > +};
> 
> Aside from those minor issues, this all looks reasonable to me, so,
> Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>

Thanks!

Alex.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 09/13/2012 12:02 AM, Alex Courbot wrote:
> On Thursday 13 September 2012 06:07:13 Stephen Warren wrote:
>> On 09/12/2012 03:57 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.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> diff --git a/Documentation/power/power_seq.txt
>>> b/Documentation/power/power_seq.txt
>>>
>>> +Sometimes, you may want to browse the list of resources allocated by a
>>> sequence, +for instance to 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 a pointer to a struct power_seq_resource. This
>>> structure +contains the type of the resource, the information used for
>>> identifying it, and +the resolved resource itself.
>>
>> I don't think you need to include that [power_seq_set_resources] prototype
>> here?
> 
> Why not? I thought it was customary to include the prototypes in the 
> documentation, and this seems to be the right place for this function.

It's something used internally to the macro; what the user cares about
is which macro to use for the functionality you're describing, not any
prototypes needed by the internal implementation of the macro, which are
always provided by the appropriate header.

>>> diff --git a/drivers/power/power_seq/power_seq.c
>>> b/drivers/power/power_seq/power_seq.c
>>>
>>> +struct power_seq_step {
>>> +	/* Copy of the platform data */
>>> +	struct platform_power_seq_step pdata;
>>
>> I'd reword the comment to "Copy of the step", and name the field "step".
> 
> That would make a step within a step - doesn't pdata make it more explicit 
> what this member is for (containing the platform data for this step)?

Well, it's not always platform data; it could come from device tree.
Sorry for bike-shedding slightly, but how about just "data",
"step_data", "config", or "step_config"?

>>> +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,
>>> +};
>>
>> Ah, I see why you're using #include now.
> 
> We could also go with something more dynamic and compile these files 
> separately, but that would require some registration mechanism which I don't 
> think is needed for such a simple feature.

Sure. There are already examples in the kernel of basically what you're
doing anyway, and it's not like it'd be hard to change this if we want
to do something different in the future too.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Wed, 2012-09-12 at 18:57 +0900, 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.

The sequences are not board-specific, they are device (backlight, etc.)
specific. The sequences have been handled in board-specific hook
functions so far because there hasn't been proper drivers for the
devices.

If I were to take the same panel (and backlight) you have and install it
on my board, I would need the same power sequence.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 13:45:39 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
> 
> On Wed, 2012-09-12 at 18:57 +0900, 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.
> 
> 
> The sequences are not board-specific, they are device (backlight, etc.)
> specific. The sequences have been handled in board-specific hook
> functions so far because there hasn't been proper drivers for the
> devices.
> 
> If I were to take the same panel (and backlight) you have and install it
> on my board, I would need the same power sequence.

You could also have power sequences that control a set of GPIOs for an 
external interface (and would then be more board-specific), but you are right 
that for most of the case power seqs apply to devices and this statement is 
misleading. I will fix that in the commit message and wherever this might 
appear.

Alex.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 15:08 +0900, Alex Courbot wrote:
> On Thursday 13 September 2012 13:45:39 Tomi Valkeinen wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, 2012-09-12 at 18:57 +0900, 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.
> > 
> > 
> > The sequences are not board-specific, they are device (backlight, etc.)
> > specific. The sequences have been handled in board-specific hook
> > functions so far because there hasn't been proper drivers for the
> > devices.
> > 
> > If I were to take the same panel (and backlight) you have and install it
> > on my board, I would need the same power sequence.
> 
> You could also have power sequences that control a set of GPIOs for an 
> external interface (and would then be more board-specific), but you are right 

What do you mean with "external interface"?

But it's true that there can always be interesting board specific
hardware designs, and they truly are board specific. In my experience
these are quite rare, though, but perhaps not so rare that we wouldn't
need to care about them.

However, I fear these board specific things may be quite a bit anything,
so it may well be pwm, gpios and regulators are not enough for them. For
example, there could be an FPGA on the board which requires some
configuration to accomplish the task at hand. It could be rather
difficult to handle it with a generic power sequence.

So I guess what I'm saying is that mostly these issues are device
specific, and when they are not, they may be rather complex/strange and
require c code.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
> 
> On Thu, 2012-09-13 at 15:08 +0900, Alex Courbot wrote:
> 
> > On Thursday 13 September 2012 13:45:39 Tomi Valkeinen wrote:
> > 
> > > > Old Signed by an unknown key
> > > 
> > > 
> > > On Wed, 2012-09-12 at 18:57 +0900, 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.
> > > 
> > > 
> > > 
> > > The sequences are not board-specific, they are device (backlight, etc.)
> > > specific. The sequences have been handled in board-specific hook
> > > functions so far because there hasn't been proper drivers for the
> > > devices.
> > > 
> > > If I were to take the same panel (and backlight) you have and install
> > > it
> > > on my board, I would need the same power sequence.
> > 
> > 
> > You could also have power sequences that control a set of GPIOs for an 
> > external interface (and would then be more board-specific), but you are
> > right 
> 
> What do you mean with "external interface"?

Any crazy circuit design that would make the regular power sequence not usable 
on a specific board. Sorry, I don't have any concrete example in mind, the 
above is just speculation.

> But it's true that there can always be interesting board specific
> hardware designs, and they truly are board specific. In my experience
> these are quite rare, though, but perhaps not so rare that we wouldn't
> need to care about them.
> 
> However, I fear these board specific things may be quite a bit anything,
> so it may well be pwm, gpios and regulators are not enough for them. For
> example, there could be an FPGA on the board which requires some
> configuration to accomplish the task at hand. It could be rather
> difficult to handle it with a generic power sequence.

Right. Note that this framework is supposed to be extended - I would like to 
at least add regulator voltage setting, and maybe even support for clocks and 
pinmux (but that might be out of place).

> So I guess what I'm saying is that mostly these issues are device
> specific, and when they are not, they may be rather complex/strange and
> require c code.

You're definitely right about the powering issue being a device issue 99% of 
the time. For the rest I do not have enough insight to emit an opinion.

Alex.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
>  
> > However, I fear these board specific things may be quite a bit anything,
> > so it may well be pwm, gpios and regulators are not enough for them. For
> > example, there could be an FPGA on the board which requires some
> > configuration to accomplish the task at hand. It could be rather
> > difficult to handle it with a generic power sequence.
> 
> Right. Note that this framework is supposed to be extended - I would like to 
> at least add regulator voltage setting, and maybe even support for clocks and 
> pinmux (but that might be out of place).

Yes, that's one concern of mine... I already can imagine someone
suggesting adding conditionals to the power sequence data. Perhaps also
direct memory read/writes so you can twiddle registers directly. And so
on. Where's the limit what it should contain? Can we soon write full
drivers with the DT data? =)

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Sascha Hauer]

On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> >  
> > > However, I fear these board specific things may be quite a bit anything,
> > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > example, there could be an FPGA on the board which requires some
> > > configuration to accomplish the task at hand. It could be rather
> > > difficult to handle it with a generic power sequence.
> > 
> > Right. Note that this framework is supposed to be extended - I would like to 
> > at least add regulator voltage setting, and maybe even support for clocks and 
> > pinmux (but that might be out of place).
> 
> Yes, that's one concern of mine... I already can imagine someone
> suggesting adding conditionals to the power sequence data. Perhaps also
> direct memory read/writes so you can twiddle registers directly. And so
> on. Where's the limit what it should contain? Can we soon write full
> drivers with the DT data? =)

I have this concern aswell, that's why I'm sceptical about this patch
set. But what are the alternatives? Adding power code to the drivers and
thus adding board specific code to them is backwards.

Sascha

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > >  
> > > > However, I fear these board specific things may be quite a bit anything,
> > > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > > example, there could be an FPGA on the board which requires some
> > > > configuration to accomplish the task at hand. It could be rather
> > > > difficult to handle it with a generic power sequence.
> > > 
> > > Right. Note that this framework is supposed to be extended - I would like to 
> > > at least add regulator voltage setting, and maybe even support for clocks and 
> > > pinmux (but that might be out of place).
> > 
> > Yes, that's one concern of mine... I already can imagine someone
> > suggesting adding conditionals to the power sequence data. Perhaps also
> > direct memory read/writes so you can twiddle registers directly. And so
> > on. Where's the limit what it should contain? Can we soon write full
> > drivers with the DT data? =)
> 
> I have this concern aswell, that's why I'm sceptical about this patch
> set. But what are the alternatives? Adding power code to the drivers and
> thus adding board specific code to them is backwards.

As was pointed out in earlier posts in this thread, these are almost
always device specific, not board specific.

Do you have examples of board specific power sequences or such?

 Tomi


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Sascha Hauer]

On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > >  
> > > > > However, I fear these board specific things may be quite a bit anything,
> > > > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > > > example, there could be an FPGA on the board which requires some
> > > > > configuration to accomplish the task at hand. It could be rather
> > > > > difficult to handle it with a generic power sequence.
> > > > 
> > > > Right. Note that this framework is supposed to be extended - I would like to 
> > > > at least add regulator voltage setting, and maybe even support for clocks and 
> > > > pinmux (but that might be out of place).
> > > 
> > > Yes, that's one concern of mine... I already can imagine someone
> > > suggesting adding conditionals to the power sequence data. Perhaps also
> > > direct memory read/writes so you can twiddle registers directly.

These memory writes can be avoided when these registers are abstracted
as a regular gpio/regulator/pwm driver.

> > > And so
> > > on. Where's the limit what it should contain? Can we soon write full
> > > drivers with the DT data? =)
> > 
> > I have this concern aswell, that's why I'm sceptical about this patch
> > set. But what are the alternatives? Adding power code to the drivers and
> > thus adding board specific code to them is backwards.
> 
> As was pointed out in earlier posts in this thread, these are almost
> always device specific, not board specific.
> 
> Do you have examples of board specific power sequences or such?

Sure, tons of. One board needs a gpio to be set high to enable backlight,
the next one to low, a regulator has to be enabled, and to avoid
flickering a certain timing has to be ensured. This is all highly board
specific.

Sascha

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 09:18 +0200, Sascha Hauer wrote:
> On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > > >  
> > > > > > However, I fear these board specific things may be quite a bit anything,
> > > > > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > > > > example, there could be an FPGA on the board which requires some
> > > > > > configuration to accomplish the task at hand. It could be rather
> > > > > > difficult to handle it with a generic power sequence.
> > > > > 
> > > > > Right. Note that this framework is supposed to be extended - I would like to 
> > > > > at least add regulator voltage setting, and maybe even support for clocks and 
> > > > > pinmux (but that might be out of place).
> > > > 
> > > > Yes, that's one concern of mine... I already can imagine someone
> > > > suggesting adding conditionals to the power sequence data. Perhaps also
> > > > direct memory read/writes so you can twiddle registers directly.
> 
> These memory writes can be avoided when these registers are abstracted
> as a regular gpio/regulator/pwm driver.

Only if they are gpios/regulators/pwms. Yes, I agree most of the
possible things to configure would be among those (or perhaps
pinmuxing). But there's always the odd one that's not one of those.

> > Do you have examples of board specific power sequences or such?
> 
> Sure, tons of. One board needs a gpio to be set high to enable backlight,
> the next one to low, a regulator has to be enabled, and to avoid
> flickering a certain timing has to be ensured. This is all highly board
> specific.

Okay. In my experience these have always been device specific. In the
case of backlight, the backlight device requires one gpio to be set
high, other one low, etc.

Can you share a bit more what kind of HW configuration you have that
requires this? The backlight is not a single piece of HW added to the
board (or embedded into a panel module), but consists of multiple HW
blocks integrated in a custom way to the board?

 Tomi


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 15:03:27 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
> 
> On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> 
> > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > 
> > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > 
> > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > > 
> > > >  
> > > >  
> > > > > However, I fear these board specific things may be quite a bit
> > > > > anything,
> > > > > so it may well be pwm, gpios and regulators are not enough for them.
> > > > > For
> > > > > example, there could be an FPGA on the board which requires some
> > > > > configuration to accomplish the task at hand. It could be rather
> > > > > difficult to handle it with a generic power sequence.
> > > > 
> > > > 
> > > > Right. Note that this framework is supposed to be extended - I would
> > > > like to 
 at least add regulator voltage setting, and maybe even
> > > > support for clocks and pinmux (but that might be out of place).
> > > 
> > > 
> > > Yes, that's one concern of mine... I already can imagine someone
> > > suggesting adding conditionals to the power sequence data. Perhaps also
> > > direct memory read/writes so you can twiddle registers directly. And so
> > > on. Where's the limit what it should contain? Can we soon write full
> > > drivers with the DT data? =)
> > 
> > 
> > I have this concern aswell, that's why I'm sceptical about this patch
> > set. But what are the alternatives? Adding power code to the drivers and
> > thus adding board specific code to them is backwards.
> 
> 
> As was pointed out in earlier posts in this thread, these are almost
> always device specific, not board specific.

I think the confusion comes from the fact that in practice, people just wrote 
their hooks into the board files instead of writing more "specialized" drivers 
(which I agree would have been the correct way of doing). That is why hooks 
like those of the pwm_backlight driver were "board specific code" to me too.

Alex.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > >  
> > > > > However, I fear these board specific things may be quite a bit anything,
> > > > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > > > example, there could be an FPGA on the board which requires some
> > > > > configuration to accomplish the task at hand. It could be rather
> > > > > difficult to handle it with a generic power sequence.
> > > > 
> > > > Right. Note that this framework is supposed to be extended - I would like to 
> > > > at least add regulator voltage setting, and maybe even support for clocks and 
> > > > pinmux (but that might be out of place).
> > > 
> > > Yes, that's one concern of mine... I already can imagine someone
> > > suggesting adding conditionals to the power sequence data. Perhaps also
> > > direct memory read/writes so you can twiddle registers directly. And so
> > > on. Where's the limit what it should contain? Can we soon write full
> > > drivers with the DT data? =)
> > 
> > I have this concern aswell, that's why I'm sceptical about this patch
> > set. But what are the alternatives? Adding power code to the drivers and
> > thus adding board specific code to them is backwards.
> 
> As was pointed out in earlier posts in this thread, these are almost
> always device specific, not board specific.
> 
> Do you have examples of board specific power sequences or such?

It is true that most (perhaps all) power sequences can be associated
with a specific device, but if we go and implement drivers for these
kinds of devices we will probably end up with loads of variations of
the same scheme.

Lets take display panels as an example. One of the devices that we build
has gone through two generations so far and both are slightly different
in how they control the panel backlight: one has an external backlight
controller, the other has the display controller built into the panel.
However, from the board's perspective the control of the backlight
doesn't change, because both devices get the same inputs (an enable pin
and a PWM) that map to the same pins on the SoC.

This may not be a very good example because the timing isn't relevant,
but the basic point is still valid: if we provide a driver for both
panel devices, the code will be exactly the same. So we end up having to
refactor to avoid code duplication and use the same driver for a number
of backlight/panel combinations. Which in itself isn't very bad, but it
also means that we'll probably get to see a large number of "generic"
drivers which aren't very generic after all.

Another problem, which also applies to the case of power-sequences, is
that often the panel and backlight are not the same device. So you could
have the same panel with any number of different backlight controllers
or vice-versa any number of different panels with the same backlight
controller.

Thierry

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

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

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Sascha Hauer]

On Thu, Sep 13, 2012 at 09:29:20AM +0200, Thierry Reding wrote:
> On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > > >  
> > > > > > However, I fear these board specific things may be quite a bit anything,
> > > > > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > > > > example, there could be an FPGA on the board which requires some
> > > > > > configuration to accomplish the task at hand. It could be rather
> > > > > > difficult to handle it with a generic power sequence.
> > > > > 
> > > > > Right. Note that this framework is supposed to be extended - I would like to 
> > > > > at least add regulator voltage setting, and maybe even support for clocks and 
> > > > > pinmux (but that might be out of place).
> > > > 
> > > > Yes, that's one concern of mine... I already can imagine someone
> > > > suggesting adding conditionals to the power sequence data. Perhaps also
> > > > direct memory read/writes so you can twiddle registers directly. And so
> > > > on. Where's the limit what it should contain? Can we soon write full
> > > > drivers with the DT data? =)
> > > 
> > > I have this concern aswell, that's why I'm sceptical about this patch
> > > set. But what are the alternatives? Adding power code to the drivers and
> > > thus adding board specific code to them is backwards.
> > 
> > As was pointed out in earlier posts in this thread, these are almost
> > always device specific, not board specific.
> > 
> > Do you have examples of board specific power sequences or such?
> 
> It is true that most (perhaps all) power sequences can be associated
> with a specific device, but if we go and implement drivers for these
> kinds of devices we will probably end up with loads of variations of
> the same scheme.
> 
> Lets take display panels as an example. One of the devices that we build
> has gone through two generations so far and both are slightly different
> in how they control the panel backlight: one has an external backlight
> controller, the other has the display controller built into the panel.
> However, from the board's perspective the control of the backlight
> doesn't change, because both devices get the same inputs (an enable pin
> and a PWM) that map to the same pins on the SoC.
> 
> This may not be a very good example because the timing isn't relevant,
> but the basic point is still valid: if we provide a driver for both
> panel devices, the code will be exactly the same. So we end up having to
> refactor to avoid code duplication and use the same driver for a number
> of backlight/panel combinations. Which in itself isn't very bad, but it
> also means that we'll probably get to see a large number of "generic"
> drivers which aren't very generic after all.
> 
> Another problem, which also applies to the case of power-sequences, is
> that often the panel and backlight are not the same device.

Maybe that is the problem that needs to be addressed? They *are* not the
same device, still they are handled in a single platform callback (or
now power sequence). Maybe the amount of combinations dastrically go
down if we really make them two devices.

Most of our panels have:

- A regulator (or gpio) for turning them on

And the backlights have:

- A regulator (or gpio) for turning them on
- A PWM for controlling brightness.

The power sequence for the above is clear: Turn on the panel the panel,
wait until it stabilized and afterwards turn on the backlight.

Sascha

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 15:50:37 Sascha Hauer wrote:
> On Thu, Sep 13, 2012 at 09:29:20AM +0200, Thierry Reding wrote:
> > On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> > > On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > > > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > > > > > However, I fear these board specific things may be quite a bit
> > > > > > > anything,
> > > > > > > so it may well be pwm, gpios and regulators are not enough for
> > > > > > > them. For
> > > > > > > example, there could be an FPGA on the board which requires some
> > > > > > > configuration to accomplish the task at hand. It could be rather
> > > > > > > difficult to handle it with a generic power sequence.
> > > > > > 
> > > > > > Right. Note that this framework is supposed to be extended - I
> > > > > > would like to at least add regulator voltage setting, and maybe
> > > > > > even support for clocks and pinmux (but that might be out of
> > > > > > place).
> > > > > 
> > > > > Yes, that's one concern of mine... I already can imagine someone
> > > > > suggesting adding conditionals to the power sequence data. Perhaps
> > > > > also
> > > > > direct memory read/writes so you can twiddle registers directly. And
> > > > > so
> > > > > on. Where's the limit what it should contain? Can we soon write full
> > > > > drivers with the DT data? =)
> > > > 
> > > > I have this concern aswell, that's why I'm sceptical about this patch
> > > > set. But what are the alternatives? Adding power code to the drivers
> > > > and
> > > > thus adding board specific code to them is backwards.
> > > 
> > > As was pointed out in earlier posts in this thread, these are almost
> > > always device specific, not board specific.
> > > 
> > > Do you have examples of board specific power sequences or such?
> > 
> > It is true that most (perhaps all) power sequences can be associated
> > with a specific device, but if we go and implement drivers for these
> > kinds of devices we will probably end up with loads of variations of
> > the same scheme.
> > 
> > Lets take display panels as an example. One of the devices that we build
> > has gone through two generations so far and both are slightly different
> > in how they control the panel backlight: one has an external backlight
> > controller, the other has the display controller built into the panel.
> > However, from the board's perspective the control of the backlight
> > doesn't change, because both devices get the same inputs (an enable pin
> > and a PWM) that map to the same pins on the SoC.
> > 
> > This may not be a very good example because the timing isn't relevant,
> > but the basic point is still valid: if we provide a driver for both
> > panel devices, the code will be exactly the same. So we end up having to
> > refactor to avoid code duplication and use the same driver for a number
> > of backlight/panel combinations. Which in itself isn't very bad, but it
> > also means that we'll probably get to see a large number of "generic"
> > drivers which aren't very generic after all.
> > 
> > Another problem, which also applies to the case of power-sequences, is
> > that often the panel and backlight are not the same device.
> 
> Maybe that is the problem that needs to be addressed? They *are* not the
> same device, still they are handled in a single platform callback (or
> now power sequence). Maybe the amount of combinations dastrically go
> down if we really make them two devices.
> 
> Most of our panels have:
> 
> - A regulator (or gpio) for turning them on
> 
> And the backlights have:
> 
> - A regulator (or gpio) for turning them on
> - A PWM for controlling brightness.
> 
> The power sequence for the above is clear: Turn on the panel the panel,
> wait until it stabilized and afterwards turn on the backlight.

Actually the sequence I submitted in this patchset only takes care of the 
backlight device (the panel - or LCD - should have its own). The regulator 
controls the power supply, the PWM the intensity, and on top of that it also 
has an enable GPIO. These 3 resources are exclusively for the LED - the LCD 
uses other ones. So as of now it seems that the LCD/backlight separation is 
effective and the resources needed are not so uniform across backlights (not 
even mentioning the delays).

The LCD's power sequence is even weirder - VDD must take at least 0.5ms for 
going from 10% to 90% of its power, you must wait 400ms after switching it off 
before switching it on again, and you should also transmit data for 200ms 
before switching the backlight's LED on (using its own sequence). That last 
point is interesting since it somehow makes the LCD and LED dependent on each 
other - on an unrelated note, this might be something to consider in Laurent's 
proposal for a panel framework.

Alex.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Thu, Sep 13, 2012 at 05:21:10PM +0900, Alex Courbot wrote:
> On Thursday 13 September 2012 15:50:37 Sascha Hauer wrote:
> > On Thu, Sep 13, 2012 at 09:29:20AM +0200, Thierry Reding wrote:
> > > On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> > > > On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > > > > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > > > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > > > > > > However, I fear these board specific things may be quite a bit
> > > > > > > > anything,
> > > > > > > > so it may well be pwm, gpios and regulators are not enough for
> > > > > > > > them. For
> > > > > > > > example, there could be an FPGA on the board which requires some
> > > > > > > > configuration to accomplish the task at hand. It could be rather
> > > > > > > > difficult to handle it with a generic power sequence.
> > > > > > > 
> > > > > > > Right. Note that this framework is supposed to be extended - I
> > > > > > > would like to at least add regulator voltage setting, and maybe
> > > > > > > even support for clocks and pinmux (but that might be out of
> > > > > > > place).
> > > > > > 
> > > > > > Yes, that's one concern of mine... I already can imagine someone
> > > > > > suggesting adding conditionals to the power sequence data. Perhaps
> > > > > > also
> > > > > > direct memory read/writes so you can twiddle registers directly. And
> > > > > > so
> > > > > > on. Where's the limit what it should contain? Can we soon write full
> > > > > > drivers with the DT data? =)
> > > > > 
> > > > > I have this concern aswell, that's why I'm sceptical about this patch
> > > > > set. But what are the alternatives? Adding power code to the drivers
> > > > > and
> > > > > thus adding board specific code to them is backwards.
> > > > 
> > > > As was pointed out in earlier posts in this thread, these are almost
> > > > always device specific, not board specific.
> > > > 
> > > > Do you have examples of board specific power sequences or such?
> > > 
> > > It is true that most (perhaps all) power sequences can be associated
> > > with a specific device, but if we go and implement drivers for these
> > > kinds of devices we will probably end up with loads of variations of
> > > the same scheme.
> > > 
> > > Lets take display panels as an example. One of the devices that we build
> > > has gone through two generations so far and both are slightly different
> > > in how they control the panel backlight: one has an external backlight
> > > controller, the other has the display controller built into the panel.
> > > However, from the board's perspective the control of the backlight
> > > doesn't change, because both devices get the same inputs (an enable pin
> > > and a PWM) that map to the same pins on the SoC.
> > > 
> > > This may not be a very good example because the timing isn't relevant,
> > > but the basic point is still valid: if we provide a driver for both
> > > panel devices, the code will be exactly the same. So we end up having to
> > > refactor to avoid code duplication and use the same driver for a number
> > > of backlight/panel combinations. Which in itself isn't very bad, but it
> > > also means that we'll probably get to see a large number of "generic"
> > > drivers which aren't very generic after all.
> > > 
> > > Another problem, which also applies to the case of power-sequences, is
> > > that often the panel and backlight are not the same device.
> > 
> > Maybe that is the problem that needs to be addressed? They *are* not the
> > same device, still they are handled in a single platform callback (or
> > now power sequence). Maybe the amount of combinations dastrically go
> > down if we really make them two devices.
> > 
> > Most of our panels have:
> > 
> > - A regulator (or gpio) for turning them on
> > 
> > And the backlights have:
> > 
> > - A regulator (or gpio) for turning them on
> > - A PWM for controlling brightness.
> > 
> > The power sequence for the above is clear: Turn on the panel the panel,
> > wait until it stabilized and afterwards turn on the backlight.
> 
> Actually the sequence I submitted in this patchset only takes care of the 
> backlight device (the panel - or LCD - should have its own). The regulator 
> controls the power supply, the PWM the intensity, and on top of that it also 
> has an enable GPIO. These 3 resources are exclusively for the LED - the LCD 
> uses other ones. So as of now it seems that the LCD/backlight separation is 
> effective and the resources needed are not so uniform across backlights (not 
> even mentioning the delays).
> 
> The LCD's power sequence is even weirder - VDD must take at least 0.5ms for 
> going from 10% to 90% of its power, you must wait 400ms after switching it off 
> before switching it on again, and you should also transmit data for 200ms 
> before switching the backlight's LED on (using its own sequence). That last 
> point is interesting since it somehow makes the LCD and LED dependent on each 
> other - on an unrelated note, this might be something to consider in Laurent's 
> proposal for a panel framework.

Maybe this could be solved by adding a backlight resource type and
embedding a reference to the backlight within the panel's power
sequence?

Thierry

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 09:29 +0200, Thierry Reding wrote:
> On Thu, Sep 13, 2012 at 10:03:27AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-09-13 at 09:00 +0200, Sascha Hauer wrote:
> > > On Thu, Sep 13, 2012 at 09:54:09AM +0300, Tomi Valkeinen wrote:
> > > > On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> > > > > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > > > >  
> > > > > > However, I fear these board specific things may be quite a bit anything,
> > > > > > so it may well be pwm, gpios and regulators are not enough for them. For
> > > > > > example, there could be an FPGA on the board which requires some
> > > > > > configuration to accomplish the task at hand. It could be rather
> > > > > > difficult to handle it with a generic power sequence.
> > > > > 
> > > > > Right. Note that this framework is supposed to be extended - I would like to 
> > > > > at least add regulator voltage setting, and maybe even support for clocks and 
> > > > > pinmux (but that might be out of place).
> > > > 
> > > > Yes, that's one concern of mine... I already can imagine someone
> > > > suggesting adding conditionals to the power sequence data. Perhaps also
> > > > direct memory read/writes so you can twiddle registers directly. And so
> > > > on. Where's the limit what it should contain? Can we soon write full
> > > > drivers with the DT data? =)
> > > 
> > > I have this concern aswell, that's why I'm sceptical about this patch
> > > set. But what are the alternatives? Adding power code to the drivers and
> > > thus adding board specific code to them is backwards.
> > 
> > As was pointed out in earlier posts in this thread, these are almost
> > always device specific, not board specific.
> > 
> > Do you have examples of board specific power sequences or such?
> 
> It is true that most (perhaps all) power sequences can be associated
> with a specific device, but if we go and implement drivers for these
> kinds of devices we will probably end up with loads of variations of
> the same scheme.
> 
> Lets take display panels as an example. One of the devices that we build
> has gone through two generations so far and both are slightly different
> in how they control the panel backlight: one has an external backlight
> controller, the other has the display controller built into the panel.
> However, from the board's perspective the control of the backlight
> doesn't change, because both devices get the same inputs (an enable pin
> and a PWM) that map to the same pins on the SoC.

We had something a bit similar in Nokia. First versions had an
"independent" backlight controlled via pwm. Later versions had a
backlight that is controlled by the panel IP, so it was changed by
sending DSI commands to the panel.

> This may not be a very good example because the timing isn't relevant,
> but the basic point is still valid: if we provide a driver for both
> panel devices, the code will be exactly the same. So we end up having to
> refactor to avoid code duplication and use the same driver for a number
> of backlight/panel combinations. Which in itself isn't very bad, but it
> also means that we'll probably get to see a large number of "generic"
> drivers which aren't very generic after all.
> 
> Another problem, which also applies to the case of power-sequences, is
> that often the panel and backlight are not the same device. So you could
> have the same panel with any number of different backlight controllers
> or vice-versa any number of different panels with the same backlight
> controller.

Yes, I think the backlight and the panel should be considered separate
devices. Just like, say, a touch screen and a panel may happen to be in
the same display module, a backlight and a panel can be in the same
display module. They are still separate, independent things, although
they are, of course, used together.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Thu, Sep 13, 2012 at 11:00:18AM +0300, Tomi Valkeinen wrote:
> Yes, I think the backlight and the panel should be considered separate
> devices. Just like, say, a touch screen and a panel may happen to be in
> the same display module, a backlight and a panel can be in the same
> display module. They are still separate, independent things, although
> they are, of course, used together.

Still, as Alex mentioned, there may be some dependency between the panel
and the backlight, so we may need to have some kind of connection. I
haven't had much time to look at the panel subsystem, but perhaps it
should provide for this.

I obviously don't know every panel and backlight combination out there,
but isn't the dependency more of a usability nature. What I mean is that
the panel will work even if you switch the backlight on immediately,
just that the panel might take some time to properly display data and
therefore the backlight should remain powered off so the user doesn't
see garbage. Or are there really any functional dependencies?

Thierry

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

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

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

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 14:54:09 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
> 
> On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
> 
> > On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
> > 
> >  
> >  
> > > However, I fear these board specific things may be quite a bit
> > > anything,
> > > so it may well be pwm, gpios and regulators are not enough for them.
> > > For
> > > example, there could be an FPGA on the board which requires some
> > > configuration to accomplish the task at hand. It could be rather
> > > difficult to handle it with a generic power sequence.
> > 
> > 
> > Right. Note that this framework is supposed to be extended - I would like
> > to at least add regulator voltage setting, and maybe even support for
> > clocks and pinmux (but that might be out of place).
> 
> 
> Yes, that's one concern of mine... I already can imagine someone
> suggesting adding conditionals to the power sequence data.

I took care of that when naming the feature - it is not a "sequence" anymore 
if you have conditionals. :P

> Perhaps also
> direct memory read/writes so you can twiddle registers directly. And so
> on. Where's the limit what it should contain? Can we soon write full
> drivers with the DT data? =)

I shall be satisfied the day the kernel is released as one big DT node along 
with the 5KB interpreter that runs it.

Alex.

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 09/13/2012 01:08 AM, Alex Courbot wrote:
> On Thursday 13 September 2012 14:54:09 Tomi Valkeinen wrote:
>> * PGP Signed by an unknown key
>>
>> On Thu, 2012-09-13 at 15:36 +0900, Alex Courbot wrote:
>>
>>> On Thursday 13 September 2012 14:22:57 Tomi Valkeinen wrote:
>>>
>>>  
>>>  
>>>> However, I fear these board specific things may be quite a bit
>>>> anything,
>>>> so it may well be pwm, gpios and regulators are not enough for them.
>>>> For
>>>> example, there could be an FPGA on the board which requires some
>>>> configuration to accomplish the task at hand. It could be rather
>>>> difficult to handle it with a generic power sequence.
>>>
>>>
>>> Right. Note that this framework is supposed to be extended - I would like
>>> to at least add regulator voltage setting, and maybe even support for
>>> clocks and pinmux (but that might be out of place).
>>
>>
>> Yes, that's one concern of mine... I already can imagine someone
>> suggesting adding conditionals to the power sequence data.
> 
> I took care of that when naming the feature - it is not a "sequence" anymore 
> if you have conditionals. :P
> 
>> Perhaps also
>> direct memory read/writes so you can twiddle registers directly. And so
>> on. Where's the limit what it should contain? Can we soon write full
>> drivers with the DT data? =)
> 
> I shall be satisfied the day the kernel is released as one big DT node along 
> with the 5KB interpreter that runs it.

I know you're joking, but *cough* OpenFirmware *cough*, right?

Re: [PATCH v6 1/4] Runtime Interpreted Power Sequences

[Mark Brown]

On Wed, Sep 12, 2012 at 06:57:44PM +0900, 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.

It does make me a little sad that the DT bindings need to specify the
number of steps but otherwise this looks good (modulo the minor comments
Stephen had as well):

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

I think regardless of the current discussion about some of the
applications (like pwm-backlight) there are going to be cases where this
is useful even if it ends up being more as library code for drivers than 
as something that users work with directly.

[PATCH v6 3/4] tegra: dt: add label to tegra20's PWM

[Alexandre Courbot]

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

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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

[PATCH v6 2/4] pwm_backlight: use power sequences

[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     |  65 +++++++-
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 180 +++++++++++++++------
 include/linux/pwm_backlight.h                      |  15 +-
 4 files changed, 208 insertions(+), 53 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 689c7d2..3b7e99b 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
@@ -16,17 +17,75 @@ Optional properties:
                "pwms" property (see PWM binding[0])
   - low_threshold_brightness: brightness threshold low level. (get linear
 		 scales in brightness in low end of brightness levels)
+  - 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 = <10000>;
+				};
+				step2 {
+					type = "pwm";
+					id = "backlight";
+					enable;
+				};
+				step3 {
+					type = "gpio";
+					gpio = <&gpio 28 0>;
+					value = <1>;
+				};
+			};
+
+			power-off {
+				step0 {
+					type = "gpio";
+					gpio = <&gpio 28 0>;
+					value = <0>;
+				};
+				step1 {
+					type = "pwm";
+					id = "backlight";
+					disable;
+				};
+				step2 {
+					type = "delay";
+					delay = <10000>;
+				};
+				step3 {
+					type = "regulator";
+					id = "power";
+					disable;
+				};
+			};
+		};
 	};
 
 Example for brightness_threshold_level:
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 950a131..ca86125 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -239,6 +239,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 4965408..37a23c5 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;
+	struct power_seq_set	*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,52 @@ 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;
+
+	if (pb->power_seqs) {
+		ret = power_seq_run(pb->power_on_seq);
+		if (ret < 0) {
+			dev_err(&bl->dev, "cannot run power on sequence\n");
+			return;
+		}
+	} else {
+		/* legacy framework */
+		pwm_config(pb->pwm, 0, pb->period);
+		pwm_disable(pb->pwm);
+	}
+
+	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;
+
+	if (pb->power_seqs) {
+		ret = power_seq_run(pb->power_off_seq);
+		if (ret < 0) {
+			dev_err(&bl->dev, "cannot run power off sequence\n");
+			return;
+		}
+	} else {
+		/* legacy framework */
+		pwm_enable(pb->pwm);
+		return;
+	}
+
+	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 +103,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 +117,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)
@@ -150,12 +201,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
 			data->lth_brightness = value;
 	}
 
-	/*
-	 * TODO: Most users of this driver use a number of GPIOs to control
-	 *       backlight power. Support for specifying these needs to be
-	 *       added.
-	 */
-
 	return 0;
 }
 
@@ -177,33 +222,97 @@ 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;
 	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 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;
 	}
 
+	/* using power sequences? */
+	pb->power_seqs = devm_power_seq_set_build(&pdev->dev, data->power_seqs);
+	if (pb->power_seqs) {
+		struct power_seq_set *seqs = pb->power_seqs;
+
+		if (IS_ERR(seqs))
+			return PTR_ERR(seqs);
+
+		pb->power_on_seq = power_seq_lookup(seqs, "power-on");
+		if (!pb->power_on_seq) {
+			dev_err(&pdev->dev, "missing power-on sequence\n");
+			return -EINVAL;
+		}
+		pb->power_off_seq = power_seq_lookup(seqs, "power-off");
+		if (!pb->power_off_seq) {
+			dev_err(&pdev->dev, "missing power-off sequence\n");
+			return -EINVAL;
+		}
+
+		/* we must have exactly one PWM for this driver */
+		power_seq_for_each_resource(res, seqs) {
+			if (res->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.pwm;
+		}
+	}
+	/* using legacy interface? */
+	else {
+		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);
+	}
+
 	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) {
@@ -218,28 +327,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);
 
@@ -251,18 +338,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;
@@ -274,9 +360,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;
@@ -290,8 +375,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

Re: [PATCH v6 2/4] pwm_backlight: use power sequences

[Stephen Warren]

On 09/12/2012 03:57 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.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>

[PATCH v6 4/4] tegra: ventana: add pwm backlight DT nodes

[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 3e5952f..4ca3ceb 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -469,6 +469,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 = <10000>;
+				};
+				step2 {
+					type = "pwm";
+					id = "backlight";
+					enable;
+				};
+				step3 {
+					type = "gpio";
+					gpio = <&gpio 28 0>;
+					value = <1>;
+				};
+			};
+
+			power-off {
+				step0 {
+					type = "gpio";
+					gpio = <&gpio 28 0>;
+					value = <0>;
+				};
+				step1 {
+					type = "pwm";
+					id = "backlight";
+					disable;
+				};
+				step2 {
+					type = "delay";
+					delay = <10000>;
+				};
+				step3 {
+					type = "regulator";
+					id = "power";
+					disable;
+				};
+			};
+		};
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -512,7 +569,7 @@
 			enable-active-high;
 		};
 
-		regulator@4 {
+		vdd_bl_reg: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
 			regulator-name = "vdd_bl";
-- 
1.7.12

Re: [PATCH v6 4/4] tegra: ventana: add pwm backlight DT nodes

[Stephen Warren]

On 09/12/2012 03:57 AM, Alexandre Courbot wrote:
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Both patches 3 and 4 in the series,
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

I will take those patches into the Tegra tree when (hopefully rather
than if) patches 1 and 2 are accepted.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 09/12/2012 03:57 AM, Alexandre Courbot wrote:
> New revision of the power sequences, taking as usual the feedback that was
> kindly provided about the last version.
> 
> I think now is a good time to discuss integrating this and to start looking for
> a maintainer who would be willing to merge this into his/her tree (I am
> especially thinking about the power framework maintainers, since this is where
> the code is right now.

The other alternative is for you to maintain this going forward; I
believe that would be as simple as:

* Create a patch to add yourself to MAINTAINERS for the
drivers/power/power_seq/ directory.

* Get a kernel.org account, push this patch to a branch there, and add
the branch into linux-next.

* Send a pull request to Linus at the appropriate time.

* Ongoing: Accept any patches, perform any maintenance required, etc.

Does anyone see any issue with Alexandre doing this? Nobody else has
volunteered yet:-)

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Anton Vorontsov]

On Wed, Sep 12, 2012 at 03:27:04PM -0600, Stephen Warren wrote:
> On 09/12/2012 03:57 AM, Alexandre Courbot wrote:
> > New revision of the power sequences, taking as usual the feedback that was
> > kindly provided about the last version.
> > 
> > I think now is a good time to discuss integrating this and to start looking for
> > a maintainer who would be willing to merge this into his/her tree (I am
> > especially thinking about the power framework maintainers, since this is where
> > the code is right now.
> 
> The other alternative is for you to maintain this going forward; I
> believe that would be as simple as:
> 
> * Create a patch to add yourself to MAINTAINERS for the
> drivers/power/power_seq/ directory.
> 
> * Get a kernel.org account, push this patch to a branch there, and add
> the branch into linux-next.
> 
> * Send a pull request to Linus at the appropriate time.
> 
> * Ongoing: Accept any patches, perform any maintenance required, etc.
> 
> Does anyone see any issue with Alexandre doing this? Nobody else has
> volunteered yet:-)

Yup, looks like the best way.

Anton.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 05:33:56 Anton Vorontsov wrote:
> On Wed, Sep 12, 2012 at 03:27:04PM -0600, Stephen Warren wrote:
> 
> > On 09/12/2012 03:57 AM, Alexandre Courbot wrote:
> > 
> > > New revision of the power sequences, taking as usual the feedback that
> > > was
> > > kindly provided about the last version.
> > > 
> > > I think now is a good time to discuss integrating this and to start
> > > looking for a maintainer who would be willing to merge this into
> > > his/her tree (I am especially thinking about the power framework
> > > maintainers, since this is where the code is right now.
> > 
> > 
> > The other alternative is for you to maintain this going forward; I
> > believe that would be as simple as:
> > 
> > * Create a patch to add yourself to MAINTAINERS for the
> > drivers/power/power_seq/ directory.
> > 
> > * Get a kernel.org account, push this patch to a branch there, and add
> > the branch into linux-next.
> > 
> > * Send a pull request to Linus at the appropriate time.
> > 
> > * Ongoing: Accept any patches, perform any maintenance required, etc.
> > 
> > Does anyone see any issue with Alexandre doing this? Nobody else has
> > volunteered yet:-)
> 
> 
> Yup, looks like the best way.

I am fine this way too - it will just take some time for me to get ready as I 
will need to get my GPG key signed by some kernel developers first (that's what 
I forgot to do during the last LinuxCon!). I know a few here so it should not 
be too hard to get them drunk and sign my key, but I don't expect to be ready 
for the 3.7 merge window. Would anybody be concerned by this delay? By the 
meantime I can also make a branch available somewhere.

Alex.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Wed, 2012-09-12 at 18:57 +0900, Alexandre Courbot wrote:
> New revision of the power sequences, taking as usual the feedback that was
> kindly provided about the last version.
> 
> I think now is a good time to discuss integrating this and to start looking for
> a maintainer who would be willing to merge this into his/her tree (I am
> especially thinking about the power framework maintainers, since this is where
> the code is right now. The second patch in this series enables the pwm_backlight
> driver to be used with the device tree, without relying on board-dependent
> callbacks to support complex power sequences. We also plan to use power
> sequences in other Tegra drivers, and other people have expressed interest in
> this work during earlier reviews. See for instance
> 
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-August/018532.html
> 
> and
> 
> https://lkml.org/lkml/2012/9/6/270
> 
> There is probably some more details to fix and improve, but the current shape
> should be enough to know if we want this and where - therefore any sign from
> a maintainer would be greatly appreciated!

I want to reiterate my opinion that I think power sequences in DT data
is the wrong way to go. Powering sequences are device specific issues
and should be handled in the device driver. But I also think that power
sequences inside the drivers would probably be useful.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 13:50:47 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
> 
> On Wed, 2012-09-12 at 18:57 +0900, Alexandre Courbot wrote:
> 
> > New revision of the power sequences, taking as usual the feedback that
> > was
> > kindly provided about the last version.
> > 
> > I think now is a good time to discuss integrating this and to start
> > looking for a maintainer who would be willing to merge this into his/her
> > tree (I am especially thinking about the power framework maintainers,
> > since this is where the code is right now. The second patch in this
> > series enables the pwm_backlight driver to be used with the device tree,
> > without relying on board-dependent callbacks to support complex power
> > sequences. We also plan to use power sequences in other Tegra drivers,
> > and other people have expressed interest in this work during earlier
> > reviews. See for instance
> > 
> > https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-
August/018532.html 
> > and
> > 
> > https://lkml.org/lkml/2012/9/6/270
> > 
> > There is probably some more details to fix and improve, but the current
> > shape should be enough to know if we want this and where - therefore any
> > sign from a maintainer would be greatly appreciated!
> 
> 
> I want to reiterate my opinion that I think power sequences in DT data
> is the wrong way to go. Powering sequences are device specific issues
> and should be handled in the device driver. But I also think that power
> sequences inside the drivers would probably be useful.

I understand the logic behind handling powering sequences in the device 
driver, but as we discussed for some classes of devices this might just not 
scale. I don't know how many different panels (each with different powering 
sequences) are relying on pwm_backlight, but the alternative of embedding 
support for all of them into the kernel (and bloating the kernel image) or 
having a 3 kilometers list in the kernel configuration to individually chose 
which panel to support (which would be cumbersome and make the kernel less 
portable across boards) does not look much appealing to me. With power 
sequences encoded in the DT, we could have one .dtsi file per panel that would 
be included from the board's .dts file - no bloat, no drivers explosion, 
portability preserved.

DT support is actually the main point of power sequences, as outside of the DT 
we can always work the old way and use callbacks. If we were to remove DT 
support, I am not sure this work would still be worth being merged.

Alex.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Mark Brown]

On Thu, Sep 13, 2012 at 03:23:06PM +0900, Alex Courbot wrote:

> I understand the logic behind handling powering sequences in the device 
> driver, but as we discussed for some classes of devices this might just not 
> scale. I don't know how many different panels (each with different powering 

It would be sensible to make sure that the framework is done in such a
way that drivers can use it - there will be drivers (perhaps not display
ones) that have a known power sequence and which could benefit from the
ability to use library code to implement it based on the user simply
supplying named resources.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 14:25:53 Mark Brown wrote:
> On Thu, Sep 13, 2012 at 03:23:06PM +0900, Alex Courbot wrote:
> > I understand the logic behind handling powering sequences in the device
> > driver, but as we discussed for some classes of devices this might just
> > not
> > scale. I don't know how many different panels (each with different
> > powering
> 
> It would be sensible to make sure that the framework is done in such a
> way that drivers can use it - there will be drivers (perhaps not display
> ones) that have a known power sequence and which could benefit from the
> ability to use library code to implement it based on the user simply
> supplying named resources.

Not sure I understand what you mean, but things should be working this way 
already - regulators and PWMs are acquired by name using the standard 
regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have a way to be 
referenced by name so their number is used instead.

Alex.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Mark Brown]

On Thu, Sep 13, 2012 at 03:42:11PM +0900, Alex Courbot wrote:
> On Thursday 13 September 2012 14:25:53 Mark Brown wrote:

> > It would be sensible to make sure that the framework is done in such a
> > way that drivers can use it - there will be drivers (perhaps not display
> > ones) that have a known power sequence and which could benefit from the
> > ability to use library code to implement it based on the user simply
> > supplying named resources.

> Not sure I understand what you mean, but things should be working this way 
> already - regulators and PWMs are acquired by name using the standard 
> regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have a way to be 
> referenced by name so their number is used instead.

Right, but the sequencing for enabling them is currently open coded in
each driver.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On Thursday 13 September 2012 15:19:30 Mark Brown wrote:
> On Thu, Sep 13, 2012 at 03:42:11PM +0900, Alex Courbot wrote:
> > On Thursday 13 September 2012 14:25:53 Mark Brown wrote:
> > > It would be sensible to make sure that the framework is done in such a
> > > way that drivers can use it - there will be drivers (perhaps not display
> > > ones) that have a known power sequence and which could benefit from the
> > > ability to use library code to implement it based on the user simply
> > > supplying named resources.
> > 
> > Not sure I understand what you mean, but things should be working this way
> > already - regulators and PWMs are acquired by name using the standard
> > regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have a way
> > to be referenced by name so their number is used instead.
> 
> Right, but the sequencing for enabling them is currently open coded in
> each driver.

Mmm then I'm afraid I don't see what you wanted to say initially - could you 
elaborate?

Alex.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Mark Brown]

On Thu, Sep 13, 2012 at 04:26:34PM +0900, Alex Courbot wrote:
> On Thursday 13 September 2012 15:19:30 Mark Brown wrote:
> > > On Thursday 13 September 2012 14:25:53 Mark Brown wrote:
> > > > It would be sensible to make sure that the framework is done in such a
> > > > way that drivers can use it - there will be drivers (perhaps not display
> > > > ones) that have a known power sequence and which could benefit from the
> > > > ability to use library code to implement it based on the user simply
> > > > supplying named resources.

> > > Not sure I understand what you mean, but things should be working this way
> > > already - regulators and PWMs are acquired by name using the standard
> > > regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have a way
> > > to be referenced by name so their number is used instead.

> > Right, but the sequencing for enabling them is currently open coded in
> > each driver.

> Mmm then I'm afraid I don't see what you wanted to say initially - could you 
> elaborate?

The driver knows the power sequence.  Having to type the same sequence
into the DT or platform data for each board using the device wouuld be
retarded so we need the drivers to be able to give the sequence to the
library if they're going to be able to reuse it (which is a lot of what
Tomi is talking about).

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 09/13/2012 01:29 AM, Mark Brown wrote:
> On Thu, Sep 13, 2012 at 04:26:34PM +0900, Alex Courbot wrote:
>> On Thursday 13 September 2012 15:19:30 Mark Brown wrote:
>>>> On Thursday 13 September 2012 14:25:53 Mark Brown wrote:
>>>>> It would be sensible to make sure that the framework is done in such a
>>>>> way that drivers can use it - there will be drivers (perhaps not display
>>>>> ones) that have a known power sequence and which could benefit from the
>>>>> ability to use library code to implement it based on the user simply
>>>>> supplying named resources.
> 
>>>> Not sure I understand what you mean, but things should be working this way
>>>> already - regulators and PWMs are acquired by name using the standard
>>>> regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have a way
>>>> to be referenced by name so their number is used instead.
> 
>>> Right, but the sequencing for enabling them is currently open coded in
>>> each driver.
> 
>> Mmm then I'm afraid I don't see what you wanted to say initially - could you 
>> elaborate?
> 
> The driver knows the power sequence.  Having to type the same sequence
> into the DT or platform data for each board using the device wouuld be
> retarded so we need the drivers to be able to give the sequence to the
> library if they're going to be able to reuse it (which is a lot of what
> Tomi is talking about).

I believe that's trivial to implement. The relevant function is:

struct power_seq_set *devm_power_seq_set_build(struct device *dev,
		   struct platform_power_seq_set *pseq);

It's up to the driver whether pseq comes from platform data or is
hard-coded into the driver (or not provided at all, for the DT case).
So, the only change needed to convert a "hard-coded" driver to this API
is to convert the current custom data structure (or code) that describes
the sequence into a struct platform_power_seq_set.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Mark Brown]

On Thu, Sep 13, 2012 at 09:24:53AM -0600, Stephen Warren wrote:
> On 09/13/2012 01:29 AM, Mark Brown wrote:

> > The driver knows the power sequence.  Having to type the same sequence
> > into the DT or platform data for each board using the device wouuld be
> > retarded so we need the drivers to be able to give the sequence to the
> > library if they're going to be able to reuse it (which is a lot of what
> > Tomi is talking about).

> I believe that's trivial to implement. The relevant function is:

Right, that's what I'm saying - the code is mostly there now.

> It's up to the driver whether pseq comes from platform data or is
> hard-coded into the driver (or not provided at all, for the DT case).
> So, the only change needed to convert a "hard-coded" driver to this API
> is to convert the current custom data structure (or code) that describes
> the sequence into a struct platform_power_seq_set.

The framework could still help by providing ways to avoid having to copy
the structure and fill in the blanks for GPIO numbers (and anything else
that is numbered rather than named) by hand.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Alex Courbot]

On 09/14/2012 12:24 AM, Stephen Warren wrote:
> On 09/13/2012 01:29 AM, Mark Brown wrote:
>> On Thu, Sep 13, 2012 at 04:26:34PM +0900, Alex Courbot wrote:
>>> On Thursday 13 September 2012 15:19:30 Mark Brown wrote:
>>>>> On Thursday 13 September 2012 14:25:53 Mark Brown wrote:
>>>>>> It would be sensible to make sure that the framework is done in such a
>>>>>> way that drivers can use it - there will be drivers (perhaps not display
>>>>>> ones) that have a known power sequence and which could benefit from the
>>>>>> ability to use library code to implement it based on the user simply
>>>>>> supplying named resources.
>>
>>>>> Not sure I understand what you mean, but things should be working this way
>>>>> already - regulators and PWMs are acquired by name using the standard
>>>>> regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have a way
>>>>> to be referenced by name so their number is used instead.
>>
>>>> Right, but the sequencing for enabling them is currently open coded in
>>>> each driver.
>>
>>> Mmm then I'm afraid I don't see what you wanted to say initially - could you
>>> elaborate?
>>
>> The driver knows the power sequence.  Having to type the same sequence
>> into the DT or platform data for each board using the device wouuld be
>> retarded so we need the drivers to be able to give the sequence to the
>> library if they're going to be able to reuse it (which is a lot of what
>> Tomi is talking about).
>
> I believe that's trivial to implement. The relevant function is:
>
> struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> 		   struct platform_power_seq_set *pseq);
>
> It's up to the driver whether pseq comes from platform data or is
> hard-coded into the driver (or not provided at all, for the DT case).
> So, the only change needed to convert a "hard-coded" driver to this API
> is to convert the current custom data structure (or code) that describes
> the sequence into a struct platform_power_seq_set.

If we go this way (which looks good IMO!), then maybe we should abandon 
that "platform" denomination and merge platform_power_seq* structures 
with the currently private power_seq*, and also replace the "building" 
step with a resources acquisition one. Calling these structures 
"platform" implies they are for platform data while they can be used to 
perform more flexible things as Mark mentioned. Also making the resolved 
resource visible would allow drivers to "patch" generic sequences with 
the proper GPIO numbers at runtime. We would also avoid a few memory 
copies and both design and usage would be simplified, at the cost of 
having more things exposed. How does that sound?

Alex.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 10/03/2012 02:24 AM, Alex Courbot wrote:
> On 09/14/2012 12:24 AM, Stephen Warren wrote:
>> On 09/13/2012 01:29 AM, Mark Brown wrote:
>>> On Thu, Sep 13, 2012 at 04:26:34PM +0900, Alex Courbot wrote:
>>>> On Thursday 13 September 2012 15:19:30 Mark Brown wrote:
>>>>>> On Thursday 13 September 2012 14:25:53 Mark Brown wrote:
>>>>>>> It would be sensible to make sure that the framework is done in
>>>>>>> such a
>>>>>>> way that drivers can use it - there will be drivers (perhaps not
>>>>>>> display
>>>>>>> ones) that have a known power sequence and which could benefit
>>>>>>> from the
>>>>>>> ability to use library code to implement it based on the user simply
>>>>>>> supplying named resources.
>>>
>>>>>> Not sure I understand what you mean, but things should be working
>>>>>> this way
>>>>>> already - regulators and PWMs are acquired by name using the standard
>>>>>> regulator_get() and pwm_get() functions. GPIOs do not, AFAIK, have
>>>>>> a way
>>>>>> to be referenced by name so their number is used instead.
>>>
>>>>> Right, but the sequencing for enabling them is currently open coded in
>>>>> each driver.
>>>
>>>> Mmm then I'm afraid I don't see what you wanted to say initially -
>>>> could you
>>>> elaborate?
>>>
>>> The driver knows the power sequence.  Having to type the same sequence
>>> into the DT or platform data for each board using the device wouuld be
>>> retarded so we need the drivers to be able to give the sequence to the
>>> library if they're going to be able to reuse it (which is a lot of what
>>> Tomi is talking about).
>>
>> I believe that's trivial to implement. The relevant function is:
>>
>> struct power_seq_set *devm_power_seq_set_build(struct device *dev,
>>            struct platform_power_seq_set *pseq);
>>
>> It's up to the driver whether pseq comes from platform data or is
>> hard-coded into the driver (or not provided at all, for the DT case).
>> So, the only change needed to convert a "hard-coded" driver to this API
>> is to convert the current custom data structure (or code) that describes
>> the sequence into a struct platform_power_seq_set.
> 
> If we go this way (which looks good IMO!), then maybe we should abandon
> that "platform" denomination and merge platform_power_seq* structures
> with the currently private power_seq*, and also replace the "building"
> step with a resources acquisition one. Calling these structures
> "platform" implies they are for platform data while they can be used to
> perform more flexible things as Mark mentioned.

That all seems reasonable.

> Also making the resolved
> resource visible would allow drivers to "patch" generic sequences with
> the proper GPIO numbers at runtime.

That doesn't sound like a great idea to me, but we can simply avoid
doing this even though it's technically possible.

> We would also avoid a few memory
> copies and both design and usage would be simplified, at the cost of
> having more things exposed. How does that sound?

Sounds fine to me at least.

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 15:23 +0900, Alex Courbot wrote:
> On Thursday 13 September 2012 13:50:47 Tomi Valkeinen wrote:

> > I want to reiterate my opinion that I think power sequences in DT data
> > is the wrong way to go. Powering sequences are device specific issues
> > and should be handled in the device driver. But I also think that power
> > sequences inside the drivers would probably be useful.
> 
> I understand the logic behind handling powering sequences in the device 
> driver, but as we discussed for some classes of devices this might just not 
> scale. I don't know how many different panels (each with different powering 
> sequences) are relying on pwm_backlight, but the alternative of embedding 
> support for all of them into the kernel (and bloating the kernel image) or 
> having a 3 kilometers list in the kernel configuration to individually chose 
> which panel to support (which would be cumbersome and make the kernel less 
> portable across boards) does not look much appealing to me. With power 
> sequences encoded in the DT, we could have one .dtsi file per panel that would 
> be included from the board's .dts file - no bloat, no drivers explosion, 
> portability preserved.

Yes, I see that side of the argument also. And to be honest, I don't
know what kind of data is DT supposed to contain (or if there even is a
strict definition for that).

I have my opinion because I think that's how things should be: DT tells
us what devices there are and how they connect, and the driver handles
the rest. I may be a perfectionist, though, which is not good =).

As for the kernel bloat, it's a valid issue, but I wonder if it would be
an issue in practice. I don't know how many different supported devices
we'd have, and how many bytes the data for each device would consume.
I'm not even sure what amount of bytes would be acceptable.

But I'm guessing that we wouldn't have very many devices, and if the per
device data is made compact there wouldn't be that many bytes per
device. And with non-hotpluggable platform devices the unused device
data could be discarded after init.

Anyway, having the power sequences doesn't affect me if I don't use
them, so I have nothing against them =).

> DT support is actually the main point of power sequences, as outside of the DT 
> we can always work the old way and use callbacks. If we were to remove DT 
> support, I am not sure this work would still be worth being merged.

We can't use board callbacks when running with a DT enabled kernel. What
I meant is that the driver could contain a power sequence for the device
(or multiple supported devices). So it'd essentially be the same as
getting the power sequence from the DT data.

But I haven't looked at the power sequence data structures, so I'm not
sure if they are geared for DT use. If so, they would probably need
tuning to be good for in-kernel use.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 0/4] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On Thu, 2012-09-13 at 15:23 +0900, Alex Courbot wrote:

> DT support is actually the main point of power sequences, as outside of the DT 
> we can always work the old way and use callbacks. If we were to remove DT 
> support, I am not sure this work would still be worth being merged.

Ah, I guess you meant hooks in the driver, not hooks to board files?
Yes, that would work, but if all the hooks do essentially the same
things with just minor modifications like sleep-time, it makes more
sense to have just one piece of code which gets the sequence as data.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]