[PATCHv9 0/3] Runtime Interpreted Power Sequences

[PATCHv9 0/3] Runtime Interpreted Power Sequences

[Alexandre Courbot]

Apologies for sending two patchsets in two days - the main purpose
of this new revision is to add the linux-arm-kernel list to the
audience. A few suggestions from v8 have also been added.

Changelog from v8:
- Compile resource support into different compilation units
- Check that resource support is compiled in when resolving sequences
- Now compilable as a module
- Renamed source files to avoid repeated power_seq in their path
- Add linux-arm-kernel list to audience as suggested by Mark Rutland

Alexandre Courbot (3):
  Runtime Interpreted Power Sequences
  pwm_backlight: use power sequences
  Take maintainership of power sequences

 .../devicetree/bindings/power/power_seq.txt        | 121 +++++++
 .../bindings/video/backlight/pwm-backlight.txt     |  63 +++-
 Documentation/power/power_seq.txt                  | 253 ++++++++++++++
 MAINTAINERS                                        |  10 +
 drivers/power/Kconfig                              |   1 +
 drivers/power/Makefile                             |   1 +
 drivers/power/power_seq/Kconfig                    |   2 +
 drivers/power/power_seq/Makefile                   |   2 +
 drivers/power/power_seq/core.c                     | 362 +++++++++++++++++++++
 drivers/power/power_seq/delay.c                    |  66 ++++
 drivers/power/power_seq/gpio.c                     |  95 ++++++
 drivers/power/power_seq/power_seq_priv.h           |  56 ++++
 drivers/power/power_seq/pwm.c                      |  85 +++++
 drivers/power/power_seq/regulator.c                |  87 +++++
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 160 +++++++--
 include/linux/power_seq.h                          | 203 ++++++++++++
 include/linux/pwm_backlight.h                      |  18 +-
 18 files changed, 1546 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/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/core.c
 create mode 100644 drivers/power/power_seq/delay.c
 create mode 100644 drivers/power/power_seq/gpio.c
 create mode 100644 drivers/power/power_seq/power_seq_priv.h
 create mode 100644 drivers/power/power_seq/pwm.c
 create mode 100644 drivers/power/power_seq/regulator.c
 create mode 100644 include/linux/power_seq.h

-- 
1.8.0

[PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alexandre Courbot]

Some device drivers (e.g. panel or backlights) need to follow precise
sequences for powering on and off, involving GPIOs, regulators, PWMs
and other power-related resources, with a defined powering order and
delays to respect between steps. These sequences are device-specific,
and do not belong to a particular driver - therefore they have been
performed by board-specific hook functions so 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. It also
introduces first support for regulator, GPIO and PWM resources.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>
Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
Tested-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 .../devicetree/bindings/power/power_seq.txt        | 121 +++++++
 Documentation/power/power_seq.txt                  | 253 ++++++++++++++
 drivers/power/Kconfig                              |   1 +
 drivers/power/Makefile                             |   1 +
 drivers/power/power_seq/Kconfig                    |   2 +
 drivers/power/power_seq/Makefile                   |   2 +
 drivers/power/power_seq/core.c                     | 362 +++++++++++++++++++++
 drivers/power/power_seq/delay.c                    |  66 ++++
 drivers/power/power_seq/gpio.c                     |  95 ++++++
 drivers/power/power_seq/power_seq_priv.h           |  56 ++++
 drivers/power/power_seq/pwm.c                      |  85 +++++
 drivers/power/power_seq/regulator.c                |  87 +++++
 include/linux/power_seq.h                          | 203 ++++++++++++
 13 files changed, 1334 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/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/core.c
 create mode 100644 drivers/power/power_seq/delay.c
 create mode 100644 drivers/power/power_seq/gpio.c
 create mode 100644 drivers/power/power_seq/power_seq_priv.h
 create mode 100644 drivers/power/power_seq/pwm.c
 create mode 100644 drivers/power/power_seq/regulator.c
 create mode 100644 include/linux/power_seq.h

diff --git a/Documentation/devicetree/bindings/power/power_seq.txt b/Documentation/devicetree/bindings/power/power_seq.txt
new file mode 100644
index 0000000..7880a6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/power_seq.txt
@@ -0,0 +1,121 @@
+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- or device- 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.
+  - 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.
+  - 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 bindings (for
+instance, regulators and pwms are resolved by name - note though that name
+declaration is done differently by the two frameworks).
+
+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_of_parse_power_seq_set() on the
+backlight device, they can be added to a set using
+power_seq_set_add_sequences().
diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
new file mode 100644
index 0000000..8be0570
--- /dev/null
+++ b/Documentation/power/power_seq.txt
@@ -0,0 +1,253 @@
+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 might use a GPIO (abstracted to a
+regulator or not) to control the power supply of a backlight. 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 need a power status 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 an array of sequential steps 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, 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 contain 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 all the power sequences used for a device. The power sequences
+reference resources in their steps, and setup the union member that corresponds
+to the resource's type. Resources, similarly, have a union which relevant member
+depends on their type.
+
+Note that the only "platform data" per se here is platform_power_seq_set. Other
+structures (power_seq and power_seq_resource) will be used at runtime and thus
+*must* survive initialization, so do not declare them with the __initdata
+attribute.
+
+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" for a backlight device. The "power-on" sequence enables the "power"
+regulator of the device, waits for 10ms, and then enables PWM "backlight" and
+set GPIO 28 to 1. "power-off" does the opposite.
+
+struct power_seq_resource reg_res = {
+	.type = POWER_SEQ_REGULATOR,
+	.regulator.id = "power",
+};
+
+struct power_seq_resource gpio_res = {
+	.type = POWER_SEQ_GPIO,
+	.gpio.gpio = 28,
+};
+
+struct power_seq_resource pwm_res = {
+	.type = POWER_SEQ_PWM,
+	.pwm.id = "backlight",
+};
+
+struct power_seq_resource delay_res = {
+	.type = POWER_SEQ_DELAY,
+};
+
+struct power_seq power_on_seq = {
+	.id = "power-on",
+	.num_steps = 4,
+	.steps = {
+		{
+			.resource = &reg_res,
+			.regulator.enable = true,
+		}, {
+			.resource = &delay_res,
+			.delay.delay = 10000,
+		}, {
+			.resource = &pwm_res,
+			.pwm.enable = true,
+		}, {
+			.resource = &gpio_res,
+			.gpio.value = 1,
+		},
+	},
+};
+
+struct power_seq power_off_seq = {
+	.id = "power-off",
+	.num_steps = 4,
+	.steps = {
+		{
+			.resource = &gpio_res,
+			.gpio.value = 0,
+		}, {
+			.resource = &pwm_res,
+			.pwm.enable = false,
+		}, {
+			.resource = &delay_res,
+			.delay.delay = 10000,
+		}, {
+			.resource = &reg_res,
+			.regulator.enable = false,
+		},
+	},
+};
+
+struct platform_power_seq_set backlight_power_seqs __initdata = {
+	.num_seqs = 2,
+	.seqs = {
+		&power_on_seq,
+		&power_off_seq,
+	},
+};
+
+"backlight_power_seqs" can then be passed to power_seq_set_add_sequences() in
+order to add the sequences to a 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:
+
+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;
+		};
+	};
+};
+
+See Documentation/devicetree/bindings/power/power_seq.txt for the complete
+syntax of the DT bindings.
+
+Use by Drivers and Resources Management
+---------------------------------------
+Power sequences make use of resources that must be properly allocated and
+managed. The power_seq_set structure manages the sequences and resources for a
+particular device. A driver willing to use power sequences will thus declare one
+instance of power_seq_set per device and initialize it at probe time:
+
+struct my_device_data {
+	struct device *dev;
+	...
+	struct power_set_set power_seqs;
+	...
+};
+
+power_seq_set_init(&my_device->power_seqs, my_device->dev);
+
+The power_seq_set_add_sequence() and power_seq_set_add_sequences() functions are
+then used to add one or several sequences to a set. These functions will also
+allocate all the resources used by the sequence(s) and make sure they are ready
+to be run. All resources are allocated through devm and will thus be freed when
+the set's device is removed.
+
+  int power_seq_set_add_sequence(struct power_seq_set *set,
+			         struct power_seq *seq);
+  int power_seq_set_add_sequences(struct power_seq_set *set,
+				  struct platform_power_seq_set *seqs);
+
+Power sequences added to a set can then be resolved by their name using
+power_seq_lookup():
+
+  struct power_seq *power_seq_lookup(struct power_seq_set *seqs,
+				     const char *id);
+
+power_seq_lookup() returns a ready-to-run pointer to the power sequence which
+name matches the id parameter.
+
+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 for the
+sequences of a device, for instance to ensure that a resource of a given type is
+present. The power_seq_for_each_resource() macro does this:
+
+  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.
+
+Finally, users of the device tree can obtain a platform_power_seq_set structure
+built from the device's node using devm_of_parse_power_seq_set:
+
+  struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);
+
+The power sequences must be declared under a "power-sequences" node directly
+declared under the device's node. Detailed syntax contained in Documentation/devicetree/bindings/power/power_seq.txt. As the function name
+states, all memory is allocated through devm. The returned
+platform_power_seq_set can be freed after being added to a set, but the
+sequences themselves must be preserved until they are freed by devm.
\ No newline at end of file
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 49a8939..f20d449 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -338,3 +338,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 b949cf8..883ad4d 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -49,3 +49,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..0ece819
--- /dev/null
+++ b/drivers/power/power_seq/Kconfig
@@ -0,0 +1,2 @@
+config POWER_SEQ
+	tristate
diff --git a/drivers/power/power_seq/Makefile b/drivers/power/power_seq/Makefile
new file mode 100644
index 0000000..964b91e
--- /dev/null
+++ b/drivers/power/power_seq/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_POWER_SEQ)		+= power_seq.o
+power_seq-y			:= core.o delay.o regulator.o gpio.o pwm.o
diff --git a/drivers/power/power_seq/core.c b/drivers/power/power_seq/core.c
new file mode 100644
index 0000000..d51b9b8
--- /dev/null
+++ b/drivers/power/power_seq/core.c
@@ -0,0 +1,362 @@
+/*
+ * core.c - power sequence interpreter for platform devices and device tree
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include <linux/power_seq.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/of.h>
+
+#include "power_seq_priv.h"
+
+static const struct power_seq_res_ops *power_seq_ops[POWER_SEQ_NUM_TYPES];
+
+#ifdef CONFIG_OF
+int of_power_seq_parse_enable_properties(struct device_node *node,
+					 struct 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(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 power_seq *seq,
+				   unsigned int step_nbr,
+				   struct list_head *resources)
+{
+	struct power_seq_step *step = &seq->steps[step_nbr];
+	struct power_seq_resource res, *res2;
+	const char *type;
+	unsigned int i;
+	int err;
+
+	err = of_property_read_string(node, "type", &type);
+	if (err < 0) {
+		power_seq_err(seq, step_nbr, "cannot read type property\n");
+		return err;
+	}
+	for (i = 0; i < POWER_SEQ_NUM_TYPES; i++) {
+		if (power_seq_ops[i]->name == NULL)
+			continue;
+		if (!strcmp(type, power_seq_ops[i]->name))
+			break;
+	}
+	if (i >= POWER_SEQ_NUM_TYPES) {
+		power_seq_err(seq, step_nbr, "unknown type %s\n", type);
+		return -EINVAL;
+	}
+	memset(&res, 0, sizeof(res));
+	res.type = i;
+	err = power_seq_ops[res.type]->of_parse(node, seq, step_nbr, &res);
+	if (err < 0)
+		return err;
+
+	/* Use the same instance of the resource if met before */
+	list_for_each_entry(res2, resources, list) {
+		if (res.type == res2->type &&
+		    power_seq_ops[res.type]->res_compare(&res, res2))
+			break;
+	}
+	/* Resource never met before, create it */
+	if (&res2->list == resources) {
+		res2 = devm_kzalloc(dev, sizeof(*res2), GFP_KERNEL);
+		if (!res2)
+			return -ENOMEM;
+		memcpy(res2, &res, sizeof(res));
+		list_add_tail(&res2->list, resources);
+	}
+	step->resource = res2;
+
+	return 0;
+}
+
+static struct power_seq *of_parse_power_seq(struct device *dev,
+					    struct device_node *node,
+					    struct list_head *resources)
+{
+	struct device_node *child = NULL;
+	struct power_seq *pseq;
+	unsigned int sz;
+	int num_steps;
+	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->id = node->name;
+	pseq->num_steps = num_steps;
+
+	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;
+
+		/* Invalid step index or step already parsed? */
+		if (pos >= num_steps || pseq->steps[pos].resource != NULL) {
+			err = -EINVAL;
+			goto parse_error;
+		}
+
+		err = of_power_seq_parse_step(dev, child, pseq, pos, resources);
+		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);
+}
+
+/**
+ * devm_of_parse_power_seq_set - build a power_seq_set from the device tree
+ * @dev:	Device to parse the power sequences of
+ *
+ * Sequences must be contained into a subnode named "power-sequences" of the
+ * device root node.
+ *
+ * Memory for the sequence is allocated using devm_kzalloc on dev. The returned
+ * platform_power_seq_set can be freed by devm_kfree after the sequences have
+ * been added, but the sequences themselves must be preserved.
+ *
+ * Returns the built set on success, or an error code in case of failure.
+ */
+struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev)
+{
+	struct platform_power_seq_set *set;
+	struct device_node *root = dev->of_node;
+	struct device_node *seq;
+	struct list_head resources;
+	unsigned int sz;
+	int n;
+
+	if (!root)
+		return NULL;
+
+	root = of_find_node_by_name(root, "power-sequences");
+	if (!root)
+		return NULL;
+
+	n = of_get_child_count(root);
+	sz = sizeof(*set) + sizeof(struct power_seq *) * n;
+	set = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!set)
+		return ERR_PTR(-ENOMEM);
+	set->num_seqs = n;
+
+	n = 0;
+	INIT_LIST_HEAD(&resources);
+	for_each_child_of_node(root, seq) {
+		struct power_seq *pseq;
+
+		pseq = of_parse_power_seq(dev, seq, &resources);
+		if (IS_ERR(pseq))
+			return (void *)pseq;
+
+		set->seqs[n++] = pseq;
+	}
+
+	return set;
+}
+EXPORT_SYMBOL_GPL(devm_of_parse_power_seq_set);
+#endif /* CONFIG_OF */
+
+/**
+ * power_seq_set_init - initialize a power_seq_set
+ * @set:	Set to initialize
+ * @dev:	Device this set is going to belong to
+ */
+void power_seq_set_init(struct power_seq_set *set, struct device *dev)
+{
+	set->dev = dev;
+	INIT_LIST_HEAD(&set->resources);
+	INIT_LIST_HEAD(&set->seqs);
+}
+EXPORT_SYMBOL_GPL(power_seq_set_init);
+
+/**
+ * power_seq_add_sequence - add a power sequence to a set
+ * @set:	Set to add the sequence to
+ * @seq:	Sequence to add
+ *
+ * This step will check that all the resources used by the sequence are
+ * allocated. If they are not, an attempt to allocate them is made. This
+ * operation can fail and and return an error code.
+ *
+ * Returns 0 on success, error code if a resource initialization failed.
+ */
+int power_seq_add_sequence(struct power_seq_set *set, struct power_seq *seq)
+{
+	struct power_seq_resource *res;
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < seq->num_steps; i++) {
+		struct power_seq_step *step = &seq->steps[i];
+		struct power_seq_resource *step_res = step->resource;
+
+		/* support for resource type not compiled in? */
+		if (power_seq_ops[step_res->type]->name == NULL) {
+			power_seq_err(seq, i, "unsupported resource type\n");
+			return -EINVAL;
+		}
+
+		/* resource already allocated from a previous step ? */
+		list_for_each_entry(res, &set->resources, list) {
+			if (res == step_res)
+				break;
+		}
+		/* resource not allocated yet, allocate and add it */
+		if (&res->list == &set->resources) {
+			err = power_seq_ops[step_res->type]->res_alloc(set->dev,
+								      step_res);
+			if (err)
+				return err;
+			list_add_tail(&step->resource->list, &set->resources);
+		}
+	}
+
+	list_add_tail(&seq->list, &set->seqs);
+	seq->set = set;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_seq_add_sequence);
+
+/**
+ * power_seq_add_sequences - add power sequences defined as platform data
+ * @set:	Set to add the sequences to
+ * @seqs:	Sequences to add
+ *
+ * See power_seq_add_sequence for more details.
+ *
+ * Returns 0 on success, error code if a resource initialization failed.
+ */
+int power_seq_set_add_sequences(struct power_seq_set *set,
+				struct platform_power_seq_set *seqs)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < seqs->num_seqs; i++) {
+		ret = power_seq_add_sequence(set, seqs->seqs[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_seq_set_add_sequences);
+
+/**
+ * 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 *set, const char *id)
+{
+	struct power_seq *seq;
+
+	list_for_each_entry(seq, &set->seqs, list) {
+		if (!strcmp(seq->id, id))
+			return seq;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(power_seq_lookup);
+
+/**
+ * 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;
+
+	if (!seq->set) {
+		pr_err("cannot run a sequence not added to a set");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < seq->num_steps; i++) {
+		unsigned int type = seq->steps[i].resource->type;
+
+		err = power_seq_ops[type]->step_run(&seq->steps[i]);
+		if (err) {
+			power_seq_err(seq, i,
+				"error %d while running power sequence step\n",
+				err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_seq_run);
+
+/* defined in power_seq_*.c files */
+extern const struct power_seq_res_ops power_seq_delay_ops;
+extern const struct power_seq_res_ops power_seq_gpio_ops;
+extern const struct power_seq_res_ops power_seq_regulator_ops;
+extern const struct power_seq_res_ops power_seq_pwm_ops;
+
+static const struct power_seq_res_ops *power_seq_ops[POWER_SEQ_NUM_TYPES] = {
+	[POWER_SEQ_DELAY] = &power_seq_delay_ops,
+	[POWER_SEQ_REGULATOR] = &power_seq_regulator_ops,
+	[POWER_SEQ_PWM] = &power_seq_gpio_ops,
+	[POWER_SEQ_GPIO] = &power_seq_pwm_ops,
+};
+
+MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
+MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/power/power_seq/delay.c b/drivers/power/power_seq/delay.c
new file mode 100644
index 0000000..de6810b
--- /dev/null
+++ b/drivers/power/power_seq/delay.c
@@ -0,0 +1,66 @@
+/*
+ * 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 "power_seq_priv.h"
+#include <linux/delay.h>
+
+#ifdef CONFIG_OF
+static int of_power_seq_parse_delay(struct device_node *node,
+				    struct power_seq *seq,
+				    unsigned int step_nbr,
+				    struct power_seq_resource *res)
+{
+	struct 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(seq, step_nbr, "error reading delay property\n");
+
+	return err;
+}
+#else
+#define of_power_seq_parse_delay NULL
+#endif
+
+static bool power_seq_res_compare_delay(struct power_seq_resource *res,
+					struct power_seq_resource *res2)
+{
+	/* Delay resources are just here to hold the type of steps, so they are
+	 * all equivalent. */
+	return true;
+}
+
+static int power_seq_res_alloc_delay(struct device *dev,
+				     struct power_seq_resource *res)
+{
+	return 0;
+}
+
+static int power_seq_step_run_delay(struct power_seq_step *step)
+{
+	usleep_range(step->delay.delay,
+		     step->delay.delay + 1000);
+
+	return 0;
+}
+
+const struct power_seq_res_ops power_seq_delay_ops = {
+	.name = "delay",
+	.of_parse = of_power_seq_parse_delay,
+	.step_run = power_seq_step_run_delay,
+	.res_compare = power_seq_res_compare_delay,
+	.res_alloc = power_seq_res_alloc_delay,
+};
diff --git a/drivers/power/power_seq/gpio.c b/drivers/power/power_seq/gpio.c
new file mode 100644
index 0000000..0cd143a
--- /dev/null
+++ b/drivers/power/power_seq/gpio.c
@@ -0,0 +1,95 @@
+/*
+ * 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 "power_seq_priv.h"
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#ifdef CONFIG_OF
+static int of_power_seq_parse_gpio(struct device_node *node,
+				   struct power_seq *seq,
+				   unsigned int step_nbr,
+				   struct power_seq_resource *res)
+{
+	struct 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(seq, step_nbr, "error reading gpio property\n");
+		return gpio;
+	}
+	res->gpio.gpio = gpio;
+
+	err = of_property_read_u32(node, "value", &step->gpio.value);
+	if (err < 0) {
+		power_seq_err(seq, step_nbr, "error reading value property\n");
+	} else if (step->gpio.value < 0 || step->gpio.value > 1) {
+		power_seq_err(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 power_seq_resource *res2)
+{
+	return res->gpio.gpio == res2->gpio.gpio;
+}
+
+static int power_seq_res_alloc_gpio(struct device *dev,
+				    struct power_seq_resource *res)
+{
+	int err;
+
+	err = devm_gpio_request(dev, res->gpio.gpio, dev_name(dev));
+	if (err) {
+		dev_err(dev, "cannot get gpio %d\n", res->gpio.gpio);
+		return err;
+	}
+
+	return 0;
+}
+
+static int power_seq_step_run_gpio(struct power_seq_step *step)
+{
+	struct power_seq_resource *res = step->resource;
+
+	/* set the GPIO direction at first use */
+	if (!res->gpio.is_set) {
+		int err = gpio_direction_output(res->gpio.gpio,
+						step->gpio.value);
+		if (err)
+			return err;
+		res->gpio.is_set = true;
+	} else {
+		gpio_set_value_cansleep(res->gpio.gpio, step->gpio.value);
+	}
+
+	return 0;
+}
+
+const struct power_seq_res_ops power_seq_gpio_ops = {
+	.name = "gpio",
+	.of_parse = of_power_seq_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_priv.h b/drivers/power/power_seq/power_seq_priv.h
new file mode 100644
index 0000000..fa56e21
--- /dev/null
+++ b/drivers/power/power_seq/power_seq_priv.h
@@ -0,0 +1,56 @@
+/*
+ * 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 POWER_SEQ_PRIV_H
+#define POWER_SEQ_PRIV_H
+
+#include <linux/device.h>
+#include <linux/power_seq.h>
+#include <linux/of.h>
+
+#define power_seq_err(seq, step_nbr, format, ...)			\
+	dev_err(seq->set->dev, "%s[%d]: " format, seq->id, step_nbr,	\
+	##__VA_ARGS__);
+
+#ifdef CONFIG_OF
+int of_power_seq_parse_enable_properties(struct device_node *node,
+					 struct power_seq *seq,
+					 unsigned int step_nbr, bool *enable);
+#endif
+
+/**
+ * struct power_seq_res_ops - operators for power sequences resources
+ * @name:		Name of the resource type. Set to null when a resource
+ *			type support is not compiled in
+ * @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 a resource. Return error code if
+ *			the resource cannot be allocated, 0 otherwise
+ */
+struct power_seq_res_ops {
+	const char *name;
+	int (*of_parse)(struct device_node *node, struct power_seq *seq,
+			unsigned int step_nbr, struct power_seq_resource *res);
+	int (*step_run)(struct power_seq_step *step);
+	bool (*res_compare)(struct power_seq_resource *res,
+			    struct power_seq_resource *res2);
+	int (*res_alloc)(struct device *dev,
+			 struct power_seq_resource *res);
+};
+
+#endif
diff --git a/drivers/power/power_seq/pwm.c b/drivers/power/power_seq/pwm.c
new file mode 100644
index 0000000..a74b768
--- /dev/null
+++ b/drivers/power/power_seq/pwm.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include "power_seq_priv.h"
+
+#ifdef CONFIG_PWM
+
+#include <linux/pwm.h>
+
+#ifdef CONFIG_OF
+static int of_power_seq_parse_pwm(struct device_node *node,
+				  struct power_seq *seq,
+				  unsigned int step_nbr,
+				  struct power_seq_resource *res)
+{
+	struct power_seq_step *step = &seq->steps[step_nbr];
+	int err;
+
+	err = of_property_read_string(node, "id", &res->pwm.id);
+	if (err) {
+		power_seq_err(seq, step_nbr, "error reading id property\n");
+		return err;
+	}
+
+	err = of_power_seq_parse_enable_properties(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 power_seq_resource *res2)
+{
+	return !strcmp(res->pwm.id, res2->pwm.id);
+}
+
+static int power_seq_res_alloc_pwm(struct device *dev,
+				   struct power_seq_resource *res)
+{
+	res->pwm.pwm = devm_pwm_get(dev, res->pwm.id);
+	if (IS_ERR(res->pwm.pwm)) {
+		dev_err(dev, "cannot get pwm \"%s\"\n", res->pwm.id);
+		return PTR_ERR(res->pwm.pwm);
+	}
+
+	return 0;
+}
+
+static int power_seq_step_run_pwm(struct power_seq_step *step)
+{
+	if (step->pwm.enable) {
+		return pwm_enable(step->resource->pwm.pwm);
+	} else {
+		pwm_disable(step->resource->pwm.pwm);
+		return 0;
+	}
+}
+
+const struct power_seq_res_ops power_seq_pwm_ops = {
+	.name = "pwm",
+	.of_parse = of_power_seq_parse_pwm,
+	.step_run = power_seq_step_run_pwm,
+	.res_compare = power_seq_res_compare_pwm,
+	.res_alloc = power_seq_res_alloc_pwm,
+};
+
+#else
+
+const struct power_seq_res_ops power_seq_pwm_ops = {
+};
+
+#endif
diff --git a/drivers/power/power_seq/regulator.c b/drivers/power/power_seq/regulator.c
new file mode 100644
index 0000000..4663dbc
--- /dev/null
+++ b/drivers/power/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.
+ *
+ */
+
+#include "power_seq_priv.h"
+
+#ifdef CONFIG_REGULATOR
+
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
+
+#ifdef CONFIG_OF
+static int of_power_seq_parse_regulator(struct device_node *node,
+					struct power_seq *seq,
+					unsigned int step_nbr,
+					struct power_seq_resource *res)
+{
+	struct power_seq_step *step = &seq->steps[step_nbr];
+	int err;
+
+	err = of_property_read_string(node, "id",
+				      &res->regulator.id);
+	if (err) {
+		power_seq_err(seq, step_nbr, "error reading id property\n");
+		return err;
+	}
+
+	err = of_power_seq_parse_enable_properties(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 power_seq_resource *res2)
+{
+	return !strcmp(res->regulator.id, res2->regulator.id);
+}
+
+static int power_seq_res_alloc_regulator(struct device *dev,
+					 struct power_seq_resource *res)
+{
+	res->regulator.regulator = devm_regulator_get(dev, res->regulator.id);
+	if (IS_ERR(res->regulator.regulator)) {
+		dev_err(dev, "cannot get regulator \"%s\"\n",
+			res->regulator.id);
+		return PTR_ERR(res->regulator.regulator);
+	}
+
+	return 0;
+}
+
+static int power_seq_step_run_regulator(struct power_seq_step *step)
+{
+	if (step->regulator.enable)
+		return regulator_enable(step->resource->regulator.regulator);
+	else
+		return regulator_disable(step->resource->regulator.regulator);
+}
+
+const struct power_seq_res_ops power_seq_regulator_ops = {
+	.name = "regulator",
+	.of_parse = of_power_seq_parse_regulator,
+	.step_run = power_seq_step_run_regulator,
+	.res_compare = power_seq_res_compare_regulator,
+	.res_alloc = power_seq_res_alloc_regulator,
+};
+
+#else
+
+const struct power_seq_res_ops power_seq_regulator_ops = {
+};
+
+#endif
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..21b95b6
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,203 @@
+/*
+ * power_seq.h
+ *
+ * Simple interpreter for power sequences defined as platform data or device
+ * tree properties.
+ *
+ * Power sequences are designed to replace the callbacks typically used in
+ * board-specific files that implement board- or device- specific power
+ * sequences (such as those of backlights). A power sequence is an array of
+ * steps referencing resources (regulators, GPIOs, PWMs, ...) with an action to
+ * perform on them. By having the power sequences interpreted, it becomes
+ * possible to describe them in the device tree and thus to remove
+ * board-specific files from the kernel.
+ *
+ * See Documentation/power/power_seqs.txt for detailed information.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/types.h>
+#include <linux/list.h>
+
+struct device;
+struct regulator;
+struct pwm_device;
+
+/**
+ * The different kinds of resources that can be controlled by the sequences
+ */
+enum power_seq_res_type {
+	POWER_SEQ_DELAY,
+	POWER_SEQ_REGULATOR,
+	POWER_SEQ_PWM,
+	POWER_SEQ_GPIO,
+	POWER_SEQ_NUM_TYPES,
+};
+
+/**
+ * struct power_seq_regulator_resource
+ * @id:		name of the regulator
+ * @regulator:	resolved regulator. Written during resource resolution.
+ */
+struct power_seq_regulator_resource {
+	const char *id;
+	struct regulator *regulator;
+};
+
+/**
+ * struct power_seq_pwm_resource
+ * @id:		name of the PWM
+ * @regulator:	resolved PWM. Written during resource resolution.
+ */
+struct power_seq_pwm_resource {
+	const char *id;
+	struct pwm_device *pwm;
+};
+
+/**
+ * struct power_seq_gpio_resource
+ * @gpio:	number of the GPIO
+ * @is_set:	track GPIO state to set its direction at first use
+ */
+struct power_seq_gpio_resource {
+	int gpio;
+	bool is_set;
+};
+
+/**
+ * struct power_seq_resource - resource used by power sequences
+ * @type:	type of the resource. This decides which member of the union is
+ *		used for this resource
+ * @list:	link resources together in power_seq_set
+ * @regulator:	used if @type == POWER_SEQ_REGULATOR
+ * @pwm:	used if @type == POWER_SEQ_PWM
+ * @gpio:	used if @type == POWER_SEQ_GPIO
+ */
+struct power_seq_resource {
+	enum power_seq_res_type type;
+	struct list_head list;
+	union {
+		struct power_seq_regulator_resource regulator;
+		struct power_seq_pwm_resource pwm;
+		struct power_seq_gpio_resource gpio;
+	};
+};
+#define power_seq_for_each_resource(pos, set)			\
+	list_for_each_entry(pos, &(set)->resources, list)
+
+/**
+ * struct power_seq_delay_step - action data for delay steps
+ * @delay:	amount of time to wait, in microseconds
+ */
+struct power_seq_delay_step {
+	unsigned int delay;
+};
+
+/**
+ * struct power_seq_regulator_step - platform data for regulator steps
+ * @enable:	whether to enable or disable the regulator during this step
+ */
+struct power_seq_regulator_step {
+	bool enable;
+};
+
+/**
+ * struct power_seq_pwm_step - action data for PWM steps
+ * @enable:	whether to enable or disable the PWM during this step
+ */
+struct power_seq_pwm_step {
+	bool enable;
+};
+
+/**
+ * struct power_seq_gpio_step - action data for GPIO steps
+ * @enable:	whether to enable or disable the GPIO during this step
+ */
+struct power_seq_gpio_step {
+	int value;
+};
+
+/**
+ * struct power_seq_step - data for power sequences steps
+ * @resource:	resource used by this step
+ * @delay:	used if resource->type == POWER_SEQ_DELAY
+ * @regulator:	used if resource->type == POWER_SEQ_REGULATOR
+ * @pwm:	used if resource->type == POWER_SEQ_PWN
+ * @gpio:	used if resource->type == POWER_SEQ_GPIO
+ */
+struct power_seq_step {
+	struct power_seq_resource *resource;
+	union {
+		struct power_seq_delay_step delay;
+		struct power_seq_regulator_step regulator;
+		struct power_seq_pwm_step pwm;
+		struct power_seq_gpio_step gpio;
+	};
+};
+
+struct power_seq_set;
+
+/**
+ * struct power_seq - single power sequence
+ * @id:		name of this sequence
+ * @list:	link sequences together in power_seq_set. Leave as-is
+ * @set:	set this sequence belongs to. Written when added to a set
+ * @num_steps:	number of steps in the sequence
+ * @steps:	array of steps that make the sequence
+ */
+struct power_seq {
+	const char *id;
+	struct list_head list;
+	struct power_seq_set *set;
+	unsigned int num_steps;
+	struct power_seq_step steps[];
+};
+
+/**
+ * struct power_seq_set - power sequences and resources used by a device
+ * @dev:	device this set belongs to
+ * @resources:	list of resources used by power sequences
+ * @seqs:	list of power sequences
+ */
+struct power_seq_set {
+	struct device *dev;
+	struct list_head resources;
+	struct list_head seqs;
+};
+
+/**
+ * struct platform_power_seq_set - define power sequences as platform data
+ * @num_seqs:	number of sequences defined
+ * @seqs:	array of num_seqs power sequences
+ */
+struct platform_power_seq_set {
+	unsigned int num_seqs;
+	struct power_seq *seqs[];
+};
+
+struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);
+void power_seq_set_init(struct power_seq_set *set, struct device *dev);
+int power_seq_set_add_sequence(struct power_seq_set *set,
+			       struct power_seq *seq);
+int power_seq_set_add_sequences(struct power_seq_set *set,
+				struct platform_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.8.0

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Anton Vorontsov]

On Sat, Nov 17, 2012 at 07:55:45PM +0900, Alexandre Courbot wrote:
> Some device drivers (e.g. panel or backlights) need to follow precise
> sequences for powering on and off, involving GPIOs, regulators, PWMs
> and other power-related resources, with a defined powering order and
> delays to respect between steps. These sequences are device-specific,
> and do not belong to a particular driver - therefore they have been
> performed by board-specific hook functions so 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. It also
> introduces first support for regulator, GPIO and PWM resources.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>
> Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---

This looks almost perfect!

Just a few final notes, again mostly about the build system bits.

[...]
> diff --git a/drivers/power/power_seq/Kconfig b/drivers/power/power_seq/Kconfig
> new file mode 100644
> index 0000000..0ece819
> --- /dev/null
> +++ b/drivers/power/power_seq/Kconfig
> @@ -0,0 +1,2 @@
> +config POWER_SEQ
> +	tristate

This really needs a proper Kconfig description and a help text, shortly
describing the purpose of the subsystem.

> diff --git a/drivers/power/power_seq/Makefile b/drivers/power/power_seq/Makefile
> new file mode 100644
> index 0000000..964b91e
> --- /dev/null
> +++ b/drivers/power/power_seq/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_POWER_SEQ)		+= power_seq.o
> +power_seq-y			:= core.o delay.o regulator.o gpio.o pwm.o
> diff --git a/drivers/power/power_seq/core.c b/drivers/power/power_seq/core.c
> new file mode 100644
> index 0000000..d51b9b8
> --- /dev/null
> +++ b/drivers/power/power_seq/core.c
> @@ -0,0 +1,362 @@
> +/*
> + * core.c - power sequence interpreter for platform devices and device tree

We usually don't write file names in the comments.

> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
[...]
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_run);
> +
> +/* defined in power_seq_*.c files */

Why not place the decls into the _priv.h file?.. I understand that this
might be a temporary stuff until we have something better for ops
registration, but still, I believe it belongs to the header file.

> +extern const struct power_seq_res_ops power_seq_delay_ops;
> +extern const struct power_seq_res_ops power_seq_gpio_ops;
> +extern const struct power_seq_res_ops power_seq_regulator_ops;
> +extern const struct power_seq_res_ops power_seq_pwm_ops;
> +
> +static const struct power_seq_res_ops *power_seq_ops[POWER_SEQ_NUM_TYPES] = {
> +	[POWER_SEQ_DELAY] = &power_seq_delay_ops,
> +	[POWER_SEQ_REGULATOR] = &power_seq_regulator_ops,
> +	[POWER_SEQ_PWM] = &power_seq_gpio_ops,
> +	[POWER_SEQ_GPIO] = &power_seq_pwm_ops,
> +};
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/power/power_seq/delay.c b/drivers/power/power_seq/delay.c
> new file mode 100644
> index 0000000..de6810b
> --- /dev/null
> +++ b/drivers/power/power_seq/delay.c
> @@ -0,0 +1,66 @@
> +/*
> + * 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 "power_seq_priv.h"
> +#include <linux/delay.h>

Things should not depend on _priv.h's includes. I.e. I see this file uses
of_ routines, so it should include <linux/of.h>. <linux/delay.h> for
udelay_range(), etc.

Also, usually we place "private" headers at the very end, not at the
beginning.

> +
> +#ifdef CONFIG_OF
> +static int of_power_seq_parse_delay(struct device_node *node,
> +				    struct power_seq *seq,
> +				    unsigned int step_nbr,
> +				    struct power_seq_resource *res)
> +{
> +	struct 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(seq, step_nbr, "error reading delay property\n");
> +
> +	return err;
> +}
> +#else
> +#define of_power_seq_parse_delay NULL
> +#endif

[...]
> +#define power_seq_err(seq, step_nbr, format, ...)			\
> +	dev_err(seq->set->dev, "%s[%d]: " format, seq->id, step_nbr,	\
> +	##__VA_ARGS__);
> +
> +#ifdef CONFIG_OF
> +int of_power_seq_parse_enable_properties(struct device_node *node,
> +					 struct power_seq *seq,
> +					 unsigned int step_nbr, bool *enable);

Um, I believe you don't need CONFIG_OF here. (If it's about 'struct
device_node', then as I see it in of.h, the header always declares it,
even for the !OF case.)

> +#endif
> +
> +/**
[...]
> +++ b/drivers/power/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.
> + *
> + */
> +
> +#include "power_seq_priv.h"
> +
> +#ifdef CONFIG_REGULATOR

Would be really great if you could get rid of the #ifdefs in the .c files.

To do so, in the makefile you could write something like this:

power_seq-$(CONFIG_REGULATOR) += regulator.o

And in the header file (as explained above), you'd have

#ifdef CONFIG_REGULATOR
#define ...REGULATOR_OPS &power_seq_regulator_ops
#else
#define ...REGULATOR_OPS NULL
#endif

Or something along these lines...

Thanks,
Anton.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

On Saturday 17 November 2012 19:38:50 Anton Vorontsov wrote:
> > +++ b/drivers/power/power_seq/Kconfig
> > @@ -0,0 +1,2 @@
> > +config POWER_SEQ
> > +	tristate
> 
> This really needs a proper Kconfig description and a help text, shortly
> describing the purpose of the subsystem.

Does it? The current state makes power seqs automatically selected by drivers 
that need it, and they do not appear in the configuration menu. If we add a 
description they will then become a visible item, so the logic would be to 
make it user-selectable.

If that approach is preferred, I will also have to change pwm-backlight so 
that it compiles without power sequences. Not that I mind, but I liked the 
idea of not adding yet-another-option to the config menu.

Fixed the code according to all your other comments, thanks!

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Anton Vorontsov]

On Mon, Nov 19, 2012 at 11:29:05AM +0900, Alex Courbot wrote:
> On Saturday 17 November 2012 19:38:50 Anton Vorontsov wrote:
> > > +++ b/drivers/power/power_seq/Kconfig
> > > @@ -0,0 +1,2 @@
> > > +config POWER_SEQ
> > > +	tristate
> > 
> > This really needs a proper Kconfig description and a help text, shortly
> > describing the purpose of the subsystem.
> 
> Does it? The current state makes power seqs automatically selected by drivers 
> that need it, and they do not appear in the configuration menu. If we add a 
> description they will then become a visible item, so the logic would be to 
> make it user-selectable.

Ah. OK, then yes, there is no need for the short description, but the help
text would a good idea anyway.

Thanks,
Anton.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

Hi,

On 2012-11-17 12:55, Alexandre Courbot wrote:

A few questions after looking at the documentation:

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

I guess there's a reason, but the above looks a bit inconsistent. For
gpio you define the gpio resource inside the step. For power and pwm the
resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
5000000>;" work in step2?

> +When a power sequence is run, its steps is executed one after the other until
> +one step fails or the end of the sequence is reached.

The document doesn't give any hint of what the driver should do if
running the power sequence fails. Run the "opposite" power sequence?
Will that work for all resources? I'm mainly thinking of a case where
each enable of the resource should be matched by a disable, i.e. you
can't call disable if no enable was called.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

Hi Tomi,

On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote:
> I guess there's a reason, but the above looks a bit inconsistent. For
> gpio you define the gpio resource inside the step. For power and pwm the
> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
> 5000000>;" work in step2?

That's mostly a framework issue. Most frameworks do not export a function that 
allow to dereference a phandle - they expect resources to be declared right 
under the device node and accessed by name through foo_get(device, name). So 
using phandles in power sequences would require to export these additional 
functions and also opens the door to some inconsistencies - for instance, your 
PWM phandle could be referenced a second time in the sequence with a different 
period - how do you know that these are actually referring the same PWM 
device?

> > +When a power sequence is run, its steps is executed one after the other
> > until +one step fails or the end of the sequence is reached.
> 
> The document doesn't give any hint of what the driver should do if
> running the power sequence fails. Run the "opposite" power sequence?
> Will that work for all resources? I'm mainly thinking of a case where
> each enable of the resource should be matched by a disable, i.e. you
> can't call disable if no enable was called.

We discussed that issue already (around v5 I think) and the conclusion was 
that it should be up to the driver. When we simply enable/disable resources it 
is easy to revert, but in the future non-boolean properties will likely be 
introduced, and these cannot easily be reverted. Moreover some drivers might 
have more complex recovery needs. This deserves more discussion I think, as 
I'd like to have some "generic" recovery mechanism that covers most of the 
cases.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On 2012-11-21 03:56, Alex Courbot wrote:
> Hi Tomi,
> 
> On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote:
>> I guess there's a reason, but the above looks a bit inconsistent. For
>> gpio you define the gpio resource inside the step. For power and pwm the
>> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
>> 5000000>;" work in step2?
> 
> That's mostly a framework issue. Most frameworks do not export a function that 
> allow to dereference a phandle - they expect resources to be declared right 
> under the device node and accessed by name through foo_get(device, name). So 
> using phandles in power sequences would require to export these additional 

Right, I expected something like that.

> functions and also opens the door to some inconsistencies - for instance, your 
> PWM phandle could be referenced a second time in the sequence with a different 
> period - how do you know that these are actually referring the same PWM 
> device?

This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
device, no matter where and how many times it's used?

>>> +When a power sequence is run, its steps is executed one after the other
>>> until +one step fails or the end of the sequence is reached.
>>
>> The document doesn't give any hint of what the driver should do if
>> running the power sequence fails. Run the "opposite" power sequence?
>> Will that work for all resources? I'm mainly thinking of a case where
>> each enable of the resource should be matched by a disable, i.e. you
>> can't call disable if no enable was called.
> 
> We discussed that issue already (around v5 I think) and the conclusion was 
> that it should be up to the driver. When we simply enable/disable resources it 
> is easy to revert, but in the future non-boolean properties will likely be 
> introduced, and these cannot easily be reverted. Moreover some drivers might 
> have more complex recovery needs. This deserves more discussion I think, as 
> I'd like to have some "generic" recovery mechanism that covers most of the 
> cases.

Ok. I'll need to dig up the conversation. Did you consider any examples
of how some driver could handle the error cases?

What I'm worried about is that, as far as I understand, the power
sequence is kinda like black box to the driver. The driver just does
"power-up", without knowing what really goes on in there.

And if it doesn't know what goes on in there, nor what's in "power-down"
sequence, how can it do anything when an error happens? The only option
I see is that the driver doesn't do anything, which will leave some
resources enabled, or it can run the power-down sequence, which may or
may not work.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

On Wednesday 21 November 2012 16:13:47 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
> 
> On 2012-11-21 03:56, Alex Courbot wrote:
> > Hi Tomi,
> > 
> > On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote:
> >> I guess there's a reason, but the above looks a bit inconsistent. For
> >> gpio you define the gpio resource inside the step. For power and pwm the
> >> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
> >> 5000000>;" work in step2?
> > 
> > That's mostly a framework issue. Most frameworks do not export a function
> > that allow to dereference a phandle - they expect resources to be
> > declared right under the device node and accessed by name through
> > foo_get(device, name). So using phandles in power sequences would require
> > to export these additional
> Right, I expected something like that.
> 
> > functions and also opens the door to some inconsistencies - for instance,
> > your PWM phandle could be referenced a second time in the sequence with a
> > different period - how do you know that these are actually referring the
> > same PWM device?
> 
> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
> device, no matter where and how many times it's used?
> 
> >>> +When a power sequence is run, its steps is executed one after the other
> >>> until +one step fails or the end of the sequence is reached.
> >> 
> >> The document doesn't give any hint of what the driver should do if
> >> running the power sequence fails. Run the "opposite" power sequence?
> >> Will that work for all resources? I'm mainly thinking of a case where
> >> each enable of the resource should be matched by a disable, i.e. you
> >> can't call disable if no enable was called.
> > 
> > We discussed that issue already (around v5 I think) and the conclusion was
> > that it should be up to the driver. When we simply enable/disable
> > resources it is easy to revert, but in the future non-boolean properties
> > will likely be introduced, and these cannot easily be reverted. Moreover
> > some drivers might have more complex recovery needs. This deserves more
> > discussion I think, as I'd like to have some "generic" recovery mechanism
> > that covers most of the cases.
> 
> Ok. I'll need to dig up the conversation

IIRC it was somewhere around here:

https://lkml.org/lkml/2012/9/7/662

See the parent messages too.

> Did you consider any examples
> of how some driver could handle the error cases?

For all the (limited) use cases I considered, playing the power-off sequence 
when power-on fails just works. If power-off also fails you are potentially in 
more trouble though. Maybe we could have another "run" function that does not 
stop on errors for handling such cases where you want to "stop everything you 
can".

> What I'm worried about is that, as far as I understand, the power
> sequence is kinda like black box to the driver. The driver just does
> "power-up", without knowing what really goes on in there.

The driver could always inspect the sequence, but you are right in that this 
is not how it is intended to be done.

> And if it doesn't know what goes on in there, nor what's in "power-down"
> sequence, how can it do anything when an error happens? The only option
> I see is that the driver doesn't do anything, which will leave some
> resources enabled, or it can run the power-down sequence, which may or
> may not work.

Failures might be better handled if sequences have some "recovery policy" 
about what to do when they fail, as mentioned in the link above. As you 
pointed out, the driver might not always know enough about the resources 
involved to do the right thing.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On 2012-11-21 10:32, Alex Courbot wrote:

>> Ok. I'll need to dig up the conversation
> 
> IIRC it was somewhere around here:
> 
> https://lkml.org/lkml/2012/9/7/662
> 
> See the parent messages too.

Thanks.

>> Did you consider any examples
>> of how some driver could handle the error cases?
> 
> For all the (limited) use cases I considered, playing the power-off sequence 
> when power-on fails just works. If power-off also fails you are potentially in 
> more trouble though. Maybe we could have another "run" function that does not 
> stop on errors for handling such cases where you want to "stop everything you 
> can".

If the power-off sequence disables a regulator that was supposed to be
enabled by the power-on sequence (but wasn't enabled because of an
error), the regulator_disable is still called when the driver runs the
power-off sequence, isn't it? Regulator enables and disables are ref
counted, and the enables should match the disables.

> Failures might be better handled if sequences have some "recovery policy" 
> about what to do when they fail, as mentioned in the link above. As you 
> pointed out, the driver might not always know enough about the resources 
> involved to do the right thing.

Yes, I think such recovery policy would be needed.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

On Wednesday 21 November 2012 16:48:45 Tomi Valkeinen wrote:
> If the power-off sequence disables a regulator that was supposed to be
> enabled by the power-on sequence (but wasn't enabled because of an
> error), the regulator_disable is still called when the driver runs the
> power-off sequence, isn't it? Regulator enables and disables are ref
> counted, and the enables should match the disables.

And there collapses my theory.

> > Failures might be better handled if sequences have some "recovery policy"
> > about what to do when they fail, as mentioned in the link above. As you
> > pointed out, the driver might not always know enough about the resources
> > involved to do the right thing.
> 
> Yes, I think such recovery policy would be needed.

Indeed, from your last paragraph this makes even more sense now.

Oh, and I noticed I forgot to reply to this:

> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
> device, no matter where and how many times it's used?

That's true - however when dereferencing the phandle, the underlying framework 
will try to acquire the PWM, which will result in failure if the same resource 
is referenced several times.

One could compare the phandles to avoid this, but in your example you must 
know that for PWMs the "xyz" part is not relevant for comparison.

This makes referencing of resources by name much easier to implement and more 
elegant with respect to frameworks leveraging.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Wed, Nov 21, 2012 at 10:00 AM, Alex Courbot <acourbot@nvidia.com> wrote:
> On Wednesday 21 November 2012 16:48:45 Tomi Valkeinen wrote:
>> If the power-off sequence disables a regulator that was supposed to be
>> enabled by the power-on sequence (but wasn't enabled because of an
>> error), the regulator_disable is still called when the driver runs the
>> power-off sequence, isn't it? Regulator enables and disables are ref
>> counted, and the enables should match the disables.
>
> And there collapses my theory.
>
>> > Failures might be better handled if sequences have some "recovery policy"
>> > about what to do when they fail, as mentioned in the link above. As you
>> > pointed out, the driver might not always know enough about the resources
>> > involved to do the right thing.
>>
>> Yes, I think such recovery policy would be needed.
>
> Indeed, from your last paragraph this makes even more sense now.
>
> Oh, and I noticed I forgot to reply to this:
>
>> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
>> device, no matter where and how many times it's used?
>
> That's true - however when dereferencing the phandle, the underlying framework
> will try to acquire the PWM, which will result in failure if the same resource
> is referenced several times.
>
> One could compare the phandles to avoid this, but in your example you must
> know that for PWMs the "xyz" part is not relevant for comparison.
>
> This makes referencing of resources by name much easier to implement and more
> elegant with respect to frameworks leveraging.

I would rather (at least for how the DT bindings settle out) see the
design separate the resource references from the sequence. ie. Declare
which resources are used by a device's sequences all in one place and
have the commands index into that.

g.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Some device drivers (e.g. panel or backlights) need to follow precise
> sequences for powering on and off, involving GPIOs, regulators, PWMs
> and other power-related resources, with a defined powering order and
> delays to respect between steps. These sequences are device-specific,
> and do not belong to a particular driver - therefore they have been
> performed by board-specific hook functions so far.

I must be honest, this series really makes me nervous...

> 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

This isn't strictly true. It is still perfectly fine to have board
specific code when necessary. However, there is strong encouragement to
enable that code in device drivers as much as possible and new board
files need to have very strong justification.

> 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. It also
> introduces first support for regulator, GPIO and PWM resources.

This is where I start getting nervous. Up to now I've strongly resisted
adding any kind of interpreted code to the device tree. The model is to
identify hardware, but require the driver to know how to control it. (as
compared to ACPI which is entirely designed around executable
bytecode).

While the power sequences described here certainly cannot be confused
with a Turing complete bytecode, it is a step in that direction. Plus,
there will always be that new use case that needs just a "little new
feature" to make this work for that too. Without thinking about how to
handle that ahead of time is just asking for something to turn into a
maintenance nightmare. It's just as important to specify what the limits
of this approach are and when to punt to real driver code to handle a
device.

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

I think this will get very verbose in a hurry. Already this simple
example is 45 lines long. Using the device tree structure to encode the
language doesn't look like a very good fit. Not to mention that the
order of operations is entirely based on the node name. Want to insert
an operation between step0 and step1? Need to rename step1, step2, and
step3 to do so.

This implementation also isn't very consistent. The gpio is referenced
with a phandle in step3/step0, but the regulator and pwm are referenced
by id.

As an alternative, what about something like the following?

	backlight {
		compatible = "pwm-backlight";
		...

		/* resources used by the power sequences */
		pwms = <&pwm 2 5000000>;
		pwm-names = "backlight";
		regulators = <&backlight_reg>;
		gpios = <&gpio 28 0>;

		power-on-sequence = "r0e;d10000m;p0e;g0s";
		power-off-sequence = "g0c;p0d;d10000m;r0d";
	};

I'm thinking about the scripts as trivial-to-parse ascii strings that
have a very simple set of commands. The commands use resources already
defined in the node. ie. 'g0' refers to the first entry in the gpios
property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
no means take this as the format to use, it is just an example off the
top of my head, but it is already way easier to work with than putting
each command into a node.

The trick is still to define a syntax that doesn't fall apart when it
needs to be extended. I would also like to get opinions on whether or
not conditionals or loops should be supported (ie. loop waiting for a
status to change). If they should then we need to be a lot more careful
about the design (and due to my aforementioned nervousness, somebody may
need to get me therapy).

(Mitch, I'll let you make the argument for using Forth here. To be
honest, I'm not keen on defining any kind of new language, however
simple, but neither am I keen to pull in Forth).

> +Platform Data Format
> +--------------------
> +All relevant data structures for declaring power sequences are located in
> +include/linux/power_seq.h.

Hmm... If sequences are switched to a string instead, then platform_data
should also use it. The power sequence data structures can be created at
runtime by parsing the string.

g.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Mark Brown]

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

On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote:
> On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:

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

> This isn't strictly true. It is still perfectly fine to have board
> specific code when necessary. However, there is strong encouragement to
> enable that code in device drivers as much as possible and new board
> files need to have very strong justification.

This isn't the message that's gone over, and even for device drivers
everyone seems to be taking the whole device tree thing as a move to
pull all data out of the kernel.  In some cases there are some real
practical advantages to doing this but a lot of the people making these
changes seem to view having things in DT as a goal in itself.

> I'm thinking about the scripts as trivial-to-parse ascii strings that
> have a very simple set of commands. The commands use resources already
> defined in the node. ie. 'g0' refers to the first entry in the gpios
> property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> no means take this as the format to use, it is just an example off the
> top of my head, but it is already way easier to work with than putting
> each command into a node.

It does appear to have some legibility challenges, especially with using
the indexes into the arrays of resources.  On the other hand the arrays
should be fairly small.

> > +Platform Data Format
> > +--------------------
> > +All relevant data structures for declaring power sequences are located in
> > +include/linux/power_seq.h.

> Hmm... If sequences are switched to a string instead, then platform_data
> should also use it. The power sequence data structures can be created at
> runtime by parsing the string.

Seems like a step backwards to me - why not let people store the more
parsed version of the data?  It's going to be less convenient for users
on non-DT systems.

As I said in my earlier reviews I think this is a useful thing to have
as a utility library for drivers independantly of the DT bindings, it
would allow drivers to become more data driven.  Perhaps we can rework
the series so that the DT bindings are added as a final patch?  All the
rest of the code seems OK.

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Wed, 21 Nov 2012 10:31:34 +0900, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote:
> > On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> > > 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
> 
> > This isn't strictly true. It is still perfectly fine to have board
> > specific code when necessary. However, there is strong encouragement to
> > enable that code in device drivers as much as possible and new board
> > files need to have very strong justification.
> 
> This isn't the message that's gone over, and even for device drivers
> everyone seems to be taking the whole device tree thing as a move to
> pull all data out of the kernel.  In some cases there are some real
> practical advantages to doing this but a lot of the people making these
> changes seem to view having things in DT as a goal in itself.

I know, and I do apologize for my part in not communicating and
shepherding enough. For anyone reading; please be pragmatic on putting
things into the DT. If a binding starts to look really complex, ask
whether or not it helps things.

> > I'm thinking about the scripts as trivial-to-parse ascii strings that
> > have a very simple set of commands. The commands use resources already
> > defined in the node. ie. 'g0' refers to the first entry in the gpios
> > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> > no means take this as the format to use, it is just an example off the
> > top of my head, but it is already way easier to work with than putting
> > each command into a node.
> 
> It does appear to have some legibility challenges, especially with using
> the indexes into the arrays of resources.  On the other hand the arrays
> should be fairly small.

This is merely an example off the top of my head. I'm in no way wed to
the syntax.

> > > +Platform Data Format
> > > +--------------------
> > > +All relevant data structures for declaring power sequences are located in
> > > +include/linux/power_seq.h.
> 
> > Hmm... If sequences are switched to a string instead, then platform_data
> > should also use it. The power sequence data structures can be created at
> > runtime by parsing the string.
> 
> Seems like a step backwards to me - why not let people store the more
> parsed version of the data?  It's going to be less convenient for users
> on non-DT systems.
> 
> As I said in my earlier reviews I think this is a useful thing to have
> as a utility library for drivers independantly of the DT bindings, it
> would allow drivers to become more data driven.  Perhaps we can rework
> the series so that the DT bindings are added as a final patch?  All the
> rest of the code seems OK.

I'm fine with that if the bindings don't get merged yet.

g.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Linus Walleij]

On Wed, Nov 21, 2012 at 2:31 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> As I said in my earlier reviews I think this is a useful thing to have
> as a utility library for drivers independantly of the DT bindings, it
> would allow drivers to become more data driven.  Perhaps we can rework
> the series so that the DT bindings are added as a final patch?  All the
> rest of the code seems OK.

I have the same (limited by experience) opinion. Working sort of
near som audio drivers I have understood that power sequencing
is a big issue not only for things like this backlight example, but
even more so in the area of audio to avoid clicks and pops.

Is it correct to assume that this library will be useful also for ALSA
SoC type of devices?

Then whether to couple that to DT or ACPI or SFI or whatever
is a different discussion altogether.

Yours,
Linus Walleij

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alexandre Courbot]

On Thu, Nov 22, 2012 at 5:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> I have the same (limited by experience) opinion. Working sort of
> near som audio drivers I have understood that power sequencing
> is a big issue not only for things like this backlight example, but
> even more so in the area of audio to avoid clicks and pops.
>
> Is it correct to assume that this library will be useful also for ALSA
> SoC type of devices?

There should be nothing against that - provided ALSA's needs remain
similar to the current set of actions (regulators, gpios, ...) it
should be perfectly usable. Can you think of any usage that involves
more than dumb regulator/gpio manipulation?

If power seqs are to be used this way (as opposed to through the DT),
it will become urgent to implement that gpio handlers library we were
talking about in another thread, as we don't want to have GPIO numbers
hard-coded into the sequences.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Mark Brown]

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

On Thu, Nov 22, 2012 at 09:57:22AM +0100, Linus Walleij wrote:

> Is it correct to assume that this library will be useful also for ALSA
> SoC type of devices?

ASoC has facilities for autogenerating the bulk of the sequences which
with modern devices is all that you really need.

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

Hi Grant,

On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> > 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
> 
> This isn't strictly true. It is still perfectly fine to have board
> specific code when necessary. However, there is strong encouragement to
> enable that code in device drivers as much as possible and new board
> files need to have very strong justification.

But doesn't introducing board-specific code into the kernel just defeats the 
purpose of the DT? If we extend this logic, we are heading straight back to 
board-definition files. To a lesser extent than before I agree, but the problem 
is fundamentally the same.

> > 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. It also
> > introduces first support for regulator, GPIO and PWM resources.
> 
> This is where I start getting nervous. Up to now I've strongly resisted
> adding any kind of interpreted code to the device tree. The model is to
> identify hardware, but require the driver to know how to control it. (as
> compared to ACPI which is entirely designed around executable
> bytecode).
> 
> While the power sequences described here certainly cannot be confused
> with a Turing complete bytecode, it is a step in that direction.

Technically speaking power sequences are a step towards an interpreter, but it 
is a very small one and it should not go much further than the current state. 
I understand the concern of having "code" into the DT but I really think it 
should be viewed from a different angle.

Powering sequences are special in that they can be affected by the board design 
or the devices variations. For instance hundreds of different panels with 
backlights are currently compatible with the pwm-backlight driver. The only 
thing that differenciates them is how the backlight is powered on and off. If 
you are to build a kernel that is supposed to support all these panels, you 
would need to embed all the powering sequences in the kernel even though only 
one of them will be used by one specific board. Power sequences in the DT help 
preventing that.

With that stated, it is clear that we should not need to define more than the 
short, simple sequences of actions that cannot be elegantly handled by the 
driver. Anything beyond that should be handled by the driver itself. In 
particular, here are a few things I do *not* want to see included in power 
seqs:

- conditionals/jumps (or it's not a sequence anymore).
- direct access to hardware. Resources must at least be abstracted in some 
way. You shall not e.g. access the address space directly.
- support for non-power related resources - that is out of the special case of 
powering sequences and should be done by the driver

That should keep the "grammar" simple, and the sequences short enough to that 
we can consider then as data belonging to the device, and not as code that is 
interpreted.

> I think this will get very verbose in a hurry. Already this simple
> example is 45 lines long. Using the device tree structure to encode the
> language doesn't look like a very good fit. Not to mention that the
> order of operations is entirely based on the node name. Want to insert
> an operation between step0 and step1? Need to rename step1, step2, and
> step3 to do so.

I don't like that steps numbering thing neither, but it seems to be the best 
way to do it so far.

As for the DT structure not being adapted for this - I would agree if we 
wanted to implement a complete interpreter, but that's precisely not the case. 
More about this later.

> This implementation also isn't very consistent. The gpio is referenced
> with a phandle in step3/step0, but the regulator and pwm are referenced
> by id.

Tomi made the same remark - the reason for using the phandle in GPIO is 
because GPIO framework does not support referencing GPIOs by name yet. I 
wanted to DT bindings to reflect the underlying framework as much as possible 
until we have a function like gpio_get(device, id).

However I agree that this makes things inconsistent at the moment and would 
require a bindings change. And in the case of the DT this is actually easy to 
implement (I did it in some previous versions). I'll make sure to do it.

> As an alternative, what about something like the following?
> 
> 	backlight {
> 		compatible = "pwm-backlight";
> 		...
> 
> 		/* resources used by the power sequences */
> 		pwms = <&pwm 2 5000000>;
> 		pwm-names = "backlight";
> 		regulators = <&backlight_reg>;
> 		gpios = <&gpio 28 0>;
> 
> 		power-on-sequence = "r0e;d10000m;p0e;g0s";
> 		power-off-sequence = "g0c;p0d;d10000m;r0d";
> 	};

Well, *now* it really looks like bytecode. :)

> I'm thinking about the scripts as trivial-to-parse ascii strings that
> have a very simple set of commands. The commands use resources already
> defined in the node. ie. 'g0' refers to the first entry in the gpios
> property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> no means take this as the format to use, it is just an example off the
> top of my head, but it is already way easier to work with than putting
> each command into a node.

I'd argue that this opens the door wide open towards having a complete 
interpreter in the device tree. At least DT nodes restrict what we can do 
conveniently.

> The trick is still to define a syntax that doesn't fall apart when it
> needs to be extended. I would also like to get opinions on whether or
> not conditionals or loops should be supported (ie. loop waiting for a
> status to change). If they should then we need to be a lot more careful
> about the design (and due to my aforementioned nervousness, somebody may
> need to get me therapy).

That's IMHO the main argument in favor of using DT nodes here (the syntax 
extension, not your therapy). They can be extended simply by adding properties 
to them, and I was relying on this to make power seqs extensible while keeping 
things consistent (and backward-compatible). For instance, when we want to 
support voltage setting in regulators we can just add a "voltage" property for 
that.

So to sum up the pros of the current node-base representation:
- give a "data like" representation of powering sequences (what they should 
be, actually)
- puts sanity barriers for not turning power seqs into a real code interpreter
- easily extensible

There are probably a few cons, the numbering of steps being one, but at least 
we don't need to design a new DSL and care too much about extensibility which 
is, in the nodes case, built-in and free.

I also like to make it clear that I don't want to see this going out of 
control and that any extension proposal would have to be thoroughly justified 
and reviewed - and better not contradict any of the 3 points listed above.

If that makes you feel better, maybe we can try and fix what is wrong with the 
current bindings instead of introducing a new language that will be, by 
nature, more complex to handle and difficult to extend without breaking things?

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

On 2012-11-21 06:23, Alex Courbot wrote:
> Hi Grant,
> 
> On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
>>> 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
>>
>> This isn't strictly true. It is still perfectly fine to have board
>> specific code when necessary. However, there is strong encouragement to
>> enable that code in device drivers as much as possible and new board
>> files need to have very strong justification.
> 
> But doesn't introducing board-specific code into the kernel just defeats the 
> purpose of the DT? If we extend this logic, we are heading straight back to 
> board-definition files. To a lesser extent than before I agree, but the problem 
> is fundamentally the same.

I don't think so. I'll reiterate my opinion on this subject, as a
summary and for those who haven't read the discussions of the earlier
versions of the series. And perhaps I'll even manage to say something
new =).


First about the board specific code. I think we may need some board
specific code, even if this series was merged. Let's call them board
drivers. These board drivers would only exist for boards with some weird
setups that cannot be expressed or managed with DT and normal drivers.

I think these cases would be quite rare, as I can't even come up with a
very good example. I guess most likely these cases would involve some
small trivial chips for muxing or such, for which it doesn't really make
sense to have a real driver.

Say, perhaps a board with two LCDs connected to one video output, and
only one LCD can be enabled at a time, and you need to set some mux chip
to route the signals to the correct LCD. In this case I'd see we should
have hotplug support in the display framework, and the board driver
would act on user input (sysfs file, perhaps), plugging in/out the LCD
device depending on the user input.


As for expressing device internal details in the DT data, as done in
this series, I don't like it very much. I think the DT data or the board
file should just describe which device is the one in question, and how
it's connected to other components on the board. The driver for the
device should handle everything else.

As Alex pointed out, there may be lots of devices that work the same way
when enabled, but require slightly different power-on/off sequences. We
could have these sequences in the driver itself, either as plain code,
or in a table of some sort, possibly using the power sequence framework
presented in this series. The correct code or sequence would be ran
depending on the particular model of the device.

I think this approach is correct in the sense that this is what drivers
are supposed to do: handle all the device internal matters. But this
raises the problem of bloating the kernel with possibly lots of
different power sequences, of which only a few would be used by a board,
and all the rest would just be waste of memory.

Regarding this problem I have the following questions, to which I don't
have clear answers:

- How much of this device specific data is too much? If a driver
supports 10 different models, and the sequences for each model take 100
bytes, this 1000 bytes doesn't sound too much. But where's the limit?
And even if one driver only has 1kB of this data, what if we have lots
of similar drivers?

- How many bytes does a power sequence presented in this series take, if
the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and
regulator?

- How likely is it that we'll get lots of different models? A hundred
different models for a backlight PWM with different power-on/off
sequences already sounds a really big number. If we're only going to
have each driver supporting, say, no more than 10 models, perhaps the
problem is not even an issue in practice.

- Are there ways to limit this bloat in the driver? One obvious way
would be to discard the unused sequences after driver probe, but that
only works for platform devices. Although, I guess these sequences would
mostly be used by platform drivers? If so, then the problem could be
solved by discarding the data after probe. Another would be to load the
sequences from a file as firmware, but that feels quite an awful
solution. Anybody have other ideas?

One clear positive side with in-driver approach is that it's totally
inside the kernel, and can be easily reworked in the future, or even
changed to a DT-based approach as presented in this series if that seems
like a better solution. The DT based approach, on the other hand, will
be more or less written to stone after it's merged.


So, I like the framework of expressing the sequences presented in this
series (except there's a problem with error cases, as I pointed out in
another post), as it can be used inside the drivers. But I'm not so
enthusiastic about presenting the sequences in DT.

My suggestion would be to go forward with an in-driver solution, and
look at the DT based solution later if we are seeing an increasing bloat
in the drivers.

 Tomi

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
> On 2012-11-21 06:23, Alex Courbot wrote:
> > Hi Grant,
> > 
> > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> >>> 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
> >>
> >> This isn't strictly true. It is still perfectly fine to have board
> >> specific code when necessary. However, there is strong encouragement to
> >> enable that code in device drivers as much as possible and new board
> >> files need to have very strong justification.
> > 
> > But doesn't introducing board-specific code into the kernel just defeats the 
> > purpose of the DT? If we extend this logic, we are heading straight back to 
> > board-definition files. To a lesser extent than before I agree, but the problem 
> > is fundamentally the same.
> 
> I don't think so. I'll reiterate my opinion on this subject, as a
> summary and for those who haven't read the discussions of the earlier
> versions of the series. And perhaps I'll even manage to say something
> new =).
> 
> 
> First about the board specific code. I think we may need some board
> specific code, even if this series was merged. Let's call them board
> drivers. These board drivers would only exist for boards with some weird
> setups that cannot be expressed or managed with DT and normal drivers.
> 
> I think these cases would be quite rare, as I can't even come up with a
> very good example. I guess most likely these cases would involve some
> small trivial chips for muxing or such, for which it doesn't really make
> sense to have a real driver.
> 
> Say, perhaps a board with two LCDs connected to one video output, and
> only one LCD can be enabled at a time, and you need to set some mux chip
> to route the signals to the correct LCD. In this case I'd see we should
> have hotplug support in the display framework, and the board driver
> would act on user input (sysfs file, perhaps), plugging in/out the LCD
> device depending on the user input.
> 
> 
> As for expressing device internal details in the DT data, as done in
> this series, I don't like it very much. I think the DT data or the board
> file should just describe which device is the one in question, and how
> it's connected to other components on the board. The driver for the
> device should handle everything else.
> 
> As Alex pointed out, there may be lots of devices that work the same way
> when enabled, but require slightly different power-on/off sequences. We
> could have these sequences in the driver itself, either as plain code,
> or in a table of some sort, possibly using the power sequence framework
> presented in this series. The correct code or sequence would be ran
> depending on the particular model of the device.
> 
> I think this approach is correct in the sense that this is what drivers
> are supposed to do: handle all the device internal matters. But this
> raises the problem of bloating the kernel with possibly lots of
> different power sequences, of which only a few would be used by a board,
> and all the rest would just be waste of memory.
> 
> Regarding this problem I have the following questions, to which I don't
> have clear answers:
> 
> - How much of this device specific data is too much? If a driver
> supports 10 different models, and the sequences for each model take 100
> bytes, this 1000 bytes doesn't sound too much. But where's the limit?
> And even if one driver only has 1kB of this data, what if we have lots
> of similar drivers?
> 
> - How many bytes does a power sequence presented in this series take, if
> the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and
> regulator?
> 
> - How likely is it that we'll get lots of different models? A hundred
> different models for a backlight PWM with different power-on/off
> sequences already sounds a really big number. If we're only going to
> have each driver supporting, say, no more than 10 models, perhaps the
> problem is not even an issue in practice.
> 
> - Are there ways to limit this bloat in the driver? One obvious way
> would be to discard the unused sequences after driver probe, but that
> only works for platform devices. Although, I guess these sequences would
> mostly be used by platform drivers? If so, then the problem could be
> solved by discarding the data after probe. Another would be to load the
> sequences from a file as firmware, but that feels quite an awful
> solution. Anybody have other ideas?
> 
> One clear positive side with in-driver approach is that it's totally
> inside the kernel, and can be easily reworked in the future, or even
> changed to a DT-based approach as presented in this series if that seems
> like a better solution. The DT based approach, on the other hand, will
> be more or less written to stone after it's merged.
> 
> 
> So, I like the framework of expressing the sequences presented in this
> series (except there's a problem with error cases, as I pointed out in
> another post), as it can be used inside the drivers. But I'm not so
> enthusiastic about presenting the sequences in DT.
> 
> My suggestion would be to go forward with an in-driver solution, and
> look at the DT based solution later if we are seeing an increasing bloat
> in the drivers.

Assuming we go with your approach, what's the plan? We're actually
facing this problem right now for Tegra. Basically we have a DRM driver
that can drive the panel, but we're still missing a way to hook up the
backlight and panel enabling code. So we effectively can't support any
of the LVDS devices out there without this series.

As I understand it, what you propose is similar to what ASoC does. For a
specific board, you'd have to write a driver, presumably for the new
panel/display framework, that provides code to power the panel on and
off. That means we'll have to have a driver for each panel out there
basically, or we'd need to write generic drivers that can be configured
to some degree (via platform data or DT). This is similar to how ASoC
works, where we have a driver that provides support for a specific codec
connected to the Tegra SoC. For the display framework things could be
done in a similar way I suppose, so that Tegra could have one display
driver to handle all aspects of powering on and off the various panels
for the various boards out there.

Obviously, a lot of the code will be similar for other SoCs, but maybe
that's just the way things are if we choose that approach. There's also
the potential for factoring out large chunks of common code later on
once we start to see common patterns.

One thing that's not very clear is how the backlight subsystem should be
wired up with the display framework. I have a patch on top of the Tegra
DRM driver which adds some ad-hoc display support using this power
sequences series and the pwm-backlight.

From reading the proposal for the panel/display framework, it sounds
like a lot more is planned than just enabling or disabling panels, but
it also seems like a lot of code needs to be written to support things
like DSI, DBI or other control busses.

At least for Tegra, and I think the same holds for a wide variety of
other SoCs, dumb panels would be enough for a start. In the interest of
getting a working solution for those setups, maybe we can start small
and add just enough framework to register dumb panel drivers to along
with code to wire up a backlight to light up the display. Then we could
possibly still make it to have a proper solution to support the various
LVDS panels for Tegra with 3.9.

I'm adding Laurent on Cc since he's probably busy working on a new
proposal for the panel/display framework. Maybe he can share his thought
on this.

All of the above said, I'm willing to help out with the coding if that's
what is required to reach a solution that everybody can be happy with.

Thierry

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On 2012-11-21 13:40, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:

(sorry for bouncing back and forth with my private and my @ti addresses.
I can't find an option in thunderbird to only use one sender address,
and I always forget to change it when responding...)

>> My suggestion would be to go forward with an in-driver solution, and
>> look at the DT based solution later if we are seeing an increasing bloat
>> in the drivers.
> 
> Assuming we go with your approach, what's the plan? We're actually
> facing this problem right now for Tegra. Basically we have a DRM driver
> that can drive the panel, but we're still missing a way to hook up the
> backlight and panel enabling code. So we effectively can't support any
> of the LVDS devices out there without this series.

Could you describe the hardware setup you have related to the LCD and
backlight? Is it a public board with public schematics?

I've understood that you don't have anything special in your board, just
an LCD and a backlight, and the power sequences are related to powering
up the LCD and the backlight, without anything board specific. If so,
there's no need for board specific code, but just improving the panel
and backlight drivers to support the models you use.

> As I understand it, what you propose is similar to what ASoC does. For a
> specific board, you'd have to write a driver, presumably for the new
> panel/display framework, that provides code to power the panel on and
> off. That means we'll have to have a driver for each panel out there
> basically, or we'd need to write generic drivers that can be configured
> to some degree (via platform data or DT). This is similar to how ASoC
> works, where we have a driver that provides support for a specific codec
> connected to the Tegra SoC. For the display framework things could be
> done in a similar way I suppose, so that Tegra could have one display
> driver to handle all aspects of powering on and off the various panels
> for the various boards out there.

I think we should only need the board drivers for very special cases. If
there's just a panel and a backlight, without any special dynamic muxing
or other trickery needed, I don't see a need for a board driver. I
presume this is the case for most of the boards.

> Obviously, a lot of the code will be similar for other SoCs, but maybe
> that's just the way things are if we choose that approach. There's also
> the potential for factoring out large chunks of common code later on
> once we start to see common patterns.
> 
> One thing that's not very clear is how the backlight subsystem should be
> wired up with the display framework. I have a patch on top of the Tegra
> DRM driver which adds some ad-hoc display support using this power
> sequences series and the pwm-backlight.

I think that's a separate issue: how to associate the lcd device and
backlight device together. I don't have a clear answer to this.

There are many ways the backlight may be handled. In some cases the
panel and the backlight are truly independent, and you can use the other
without using the other (not very practical, though =).

But then with some LCDs the backlight may be controlled by sending
commands to the panel, and in this case the two may be quite linked.
Changing the backlight requires the panel driver to be up and running,
and sometimes the sending the backlight commands may need to be (say,
DSI display, with backlight commands going over the DSI bus).

So my feeling is that the panel driver should know about the related
backlight device. In the first case the panel driver would just call
enable/disable in the backlight device when the panel is turned on.

In the second case of the DSI panel... I'm not sure. I've implemented it
so that the panel driver creates the backlight device, and implements
the backlight callbacks. It then sends the DSI commands from those
callbacks.

> From reading the proposal for the panel/display framework, it sounds
> like a lot more is planned than just enabling or disabling panels, but
> it also seems like a lot of code needs to be written to support things
> like DSI, DBI or other control busses.
> 
> At least for Tegra, and I think the same holds for a wide variety of
> other SoCs, dumb panels would be enough for a start. In the interest of
> getting a working solution for those setups, maybe we can start small
> and add just enough framework to register dumb panel drivers to along
> with code to wire up a backlight to light up the display. Then we could
> possibly still make it to have a proper solution to support the various
> LVDS panels for Tegra with 3.9.

Yes, we (Laurent and me) both agree that we should start simple.

However, the common panel framework is not strictly needed for this. I'm
not sure of the current architecture for Tegra, but for OMAP we already
have panel drivers (omap specific ones, though). The panel drivers may
support multiple models, (for example,
drivers/video/omap2/displays/panel-generic-dpi.c).

I don't see any problem with adding small Tegra specific panel drivers
for the time being, with the intention of converting to common panel
framework when that's available.

Of course, the DT side is an issue. If you now create DT bindings for a
temporary model, and need to change it again later, you'll have some
headaches trying managing that without breaking the old bindings... This
is why I haven't pushed DT bindings for OMAP, as I know I have to change
them in the near future.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
> On 2012-11-21 13:40, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
> 
> (sorry for bouncing back and forth with my private and my @ti addresses.
> I can't find an option in thunderbird to only use one sender address,
> and I always forget to change it when responding...)
> 
> >> My suggestion would be to go forward with an in-driver solution, and
> >> look at the DT based solution later if we are seeing an increasing bloat
> >> in the drivers.
> > 
> > Assuming we go with your approach, what's the plan? We're actually
> > facing this problem right now for Tegra. Basically we have a DRM driver
> > that can drive the panel, but we're still missing a way to hook up the
> > backlight and panel enabling code. So we effectively can't support any
> > of the LVDS devices out there without this series.
> 
> Could you describe the hardware setup you have related to the LCD and
> backlight? Is it a public board with public schematics?

I don't think any of the schematics are public. The Tamonten Evaluation
Carrier is available publicly from our website and the schematics are
available on demand as well. If required I can probably arrange to send
you a copy.

> I've understood that you don't have anything special in your board, just
> an LCD and a backlight, and the power sequences are related to powering
> up the LCD and the backlight, without anything board specific. If so,
> there's no need for board specific code, but just improving the panel
> and backlight drivers to support the models you use.

Correct. Basically we have two GPIOs that each enable the panel or the
backlight respectively and one PWM to control the brightness. Are the
panel drivers that you refer to those in drivers/video? I'm not sure if
adding more ad-hoc drivers there just to move them to a generic
framework in the next cycle is a good idea. I'd rather spend some time
on helping to get the framework right and have drivers for that instead.

From what I understand by looking at the OMAP display drivers, they also
provide the timings for the displays. Steffen's videomode helpers can be
used to represent these easily in DT, but I suppose if all of those per-
panel specifics are represented in the drivers then that won't be needed
anymore either.

> > As I understand it, what you propose is similar to what ASoC does. For a
> > specific board, you'd have to write a driver, presumably for the new
> > panel/display framework, that provides code to power the panel on and
> > off. That means we'll have to have a driver for each panel out there
> > basically, or we'd need to write generic drivers that can be configured
> > to some degree (via platform data or DT). This is similar to how ASoC
> > works, where we have a driver that provides support for a specific codec
> > connected to the Tegra SoC. For the display framework things could be
> > done in a similar way I suppose, so that Tegra could have one display
> > driver to handle all aspects of powering on and off the various panels
> > for the various boards out there.
> 
> I think we should only need the board drivers for very special cases. If
> there's just a panel and a backlight, without any special dynamic muxing
> or other trickery needed, I don't see a need for a board driver. I
> presume this is the case for most of the boards.

For Tegra ASoC, the way to provide for this is to allow a specific board
to introduce a separate compatible value to enable per-board quirks or
special handling if it cannot be supported by the generic driver and
configuration mechanisms.

> > Obviously, a lot of the code will be similar for other SoCs, but maybe
> > that's just the way things are if we choose that approach. There's also
> > the potential for factoring out large chunks of common code later on
> > once we start to see common patterns.
> > 
> > One thing that's not very clear is how the backlight subsystem should be
> > wired up with the display framework. I have a patch on top of the Tegra
> > DRM driver which adds some ad-hoc display support using this power
> > sequences series and the pwm-backlight.
> 
> I think that's a separate issue: how to associate the lcd device and
> backlight device together. I don't have a clear answer to this.
> 
> There are many ways the backlight may be handled. In some cases the
> panel and the backlight are truly independent, and you can use the other
> without using the other (not very practical, though =).

At least for DT I think we can easily wire that up. I've actually posted
a patch recently that does so. I think in most cases it makes sense to
control them together, such as on DPMS changes, where you really want to
turn both the backlight and the LCD off, independent of how they are
tied together.

> But then with some LCDs the backlight may be controlled by sending
> commands to the panel, and in this case the two may be quite linked.
> Changing the backlight requires the panel driver to be up and running,
> and sometimes the sending the backlight commands may need to be (say,
> DSI display, with backlight commands going over the DSI bus).
> 
> So my feeling is that the panel driver should know about the related
> backlight device. In the first case the panel driver would just call
> enable/disable in the backlight device when the panel is turned on.

Exactly.

> In the second case of the DSI panel... I'm not sure. I've implemented it
> so that the panel driver creates the backlight device, and implements
> the backlight callbacks. It then sends the DSI commands from those
> callbacks.

That certainly sounds like the right approach to me.

> > From reading the proposal for the panel/display framework, it sounds
> > like a lot more is planned than just enabling or disabling panels, but
> > it also seems like a lot of code needs to be written to support things
> > like DSI, DBI or other control busses.
> > 
> > At least for Tegra, and I think the same holds for a wide variety of
> > other SoCs, dumb panels would be enough for a start. In the interest of
> > getting a working solution for those setups, maybe we can start small
> > and add just enough framework to register dumb panel drivers to along
> > with code to wire up a backlight to light up the display. Then we could
> > possibly still make it to have a proper solution to support the various
> > LVDS panels for Tegra with 3.9.
> 
> Yes, we (Laurent and me) both agree that we should start simple.
> 
> However, the common panel framework is not strictly needed for this. I'm
> not sure of the current architecture for Tegra, but for OMAP we already
> have panel drivers (omap specific ones, though). The panel drivers may
> support multiple models, (for example,
> drivers/video/omap2/displays/panel-generic-dpi.c).
> 
> I don't see any problem with adding small Tegra specific panel drivers
> for the time being, with the intention of converting to common panel
> framework when that's available.

I can take a look at how such a driver could be implemented, but again,
I'm a bit reluctant to add something ad-hoc now when maybe we can have
it supported in a proper framework not too far away in the future.

> Of course, the DT side is an issue. If you now create DT bindings for a
> temporary model, and need to change it again later, you'll have some
> headaches trying managing that without breaking the old bindings... This
> is why I haven't pushed DT bindings for OMAP, as I know I have to change
> them in the near future.

We're already keeping back on this and none of the patches that define
the bindings have been merged yet. Although bindings have been known to
change every once in a while even for code that is already in mainline.

Thierry

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On 2012-11-21 15:00, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>> On 2012-11-21 13:40, Thierry Reding wrote:
>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>
>> (sorry for bouncing back and forth with my private and my @ti addresses.
>> I can't find an option in thunderbird to only use one sender address,
>> and I always forget to change it when responding...)
>>
>>>> My suggestion would be to go forward with an in-driver solution, and
>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>> in the drivers.
>>>
>>> Assuming we go with your approach, what's the plan? We're actually
>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>> that can drive the panel, but we're still missing a way to hook up the
>>> backlight and panel enabling code. So we effectively can't support any
>>> of the LVDS devices out there without this series.
>>
>> Could you describe the hardware setup you have related to the LCD and
>> backlight? Is it a public board with public schematics?
> 
> I don't think any of the schematics are public. The Tamonten Evaluation
> Carrier is available publicly from our website and the schematics are
> available on demand as well. If required I can probably arrange to send
> you a copy.

No need, I think your answer below is enough.

>> I've understood that you don't have anything special in your board, just
>> an LCD and a backlight, and the power sequences are related to powering
>> up the LCD and the backlight, without anything board specific. If so,
>> there's no need for board specific code, but just improving the panel
>> and backlight drivers to support the models you use.
> 
> Correct. Basically we have two GPIOs that each enable the panel or the
> backlight respectively and one PWM to control the brightness. Are the

The panel GPIO goes to the panel hardware device, and enables the panel?
And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
making sure there are no other components involved.

> panel drivers that you refer to those in drivers/video? I'm not sure if
> adding more ad-hoc drivers there just to move them to a generic
> framework in the next cycle is a good idea. I'd rather spend some time
> on helping to get the framework right and have drivers for that instead.

We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
sure if other platforms have their own versions of panel drivers, but
probably adding a simple panel driver system for a platform would not be
too difficult. It could even be quite hardcoded, i.e. embedded directly
into the display subsystem driver, just to get something working until
the common panel framework is available.

Yes, I agree it's not good idea to add more platform specific panel
drivers. But it's unclear when CPF will be merged, so if you need to get
the panel working now, I don't see a simple ad-hoc driver as too
horrible. But, of course, I'm not the one making the decision whether to
merge or not =).

> From what I understand by looking at the OMAP display drivers, they also
> provide the timings for the displays. Steffen's videomode helpers can be
> used to represent these easily in DT, but I suppose if all of those per-

Right. Note that I didn't present omap panel drivers as perfect
examples, just examples =).

> panel specifics are represented in the drivers then that won't be needed
> anymore either.

Yes, for most panels with just one native mode and nothing else, the
panel driver can contain the timings.

However, this subject has been discussed earlier a few times. If the
panel in question doesn't need any special power-on/off sequences, just,
perhaps, one gpio or such, we could still use DT video modes. This would
simplify the cases where you have lots of different very simple panels.

Obviously the same questions apply to DT video modes than to the power
sequences, and while I do think it's better to handle the timings inside
the driver, I'm not too much against video timings in DT. The reason
being that the video modes are quite clear, simple and stable data,
versus the much more complex and open-ended power sequences.

>> I don't see any problem with adding small Tegra specific panel drivers
>> for the time being, with the intention of converting to common panel
>> framework when that's available.
> 
> I can take a look at how such a driver could be implemented, but again,

Don't look at the mainline omap panel drivers, at least not too closely
=). They contain hackery that will be cleaned up with CPF.

I think there are two methods to implements simple panel drivers:

The hardcoded one, where the display subsystem driver manages a few
different panel models. This is obviously not very expandable or
"correct", but should probably work just fine for a few models, until
CPF is usable.

Something like CPF will have: have the panel device/driver as a platform
device/driver, which will register itself to the display subsystem. And
with "itself" I mean some kind of struct panel_entity, with a few ops
implemented by the panel driver.

Well, this goes a bit out of subject. If you want to discuss panel
drivers more, please start a new thread. Laurent's upcoming CPF v2
should give you good ideas what the model will be.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alexandre Courbot]

Mmmm so maybe I am misinterpreting things, but it looks like we
have just buried the power sequences here, haven't we?

Alex.

On Wed, Nov 21, 2012 at 10:32 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-21 15:00, Thierry Reding wrote:
>> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>>> On 2012-11-21 13:40, Thierry Reding wrote:
>>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>>
>>> (sorry for bouncing back and forth with my private and my @ti addresses.
>>> I can't find an option in thunderbird to only use one sender address,
>>> and I always forget to change it when responding...)
>>>
>>>>> My suggestion would be to go forward with an in-driver solution, and
>>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>>> in the drivers.
>>>>
>>>> Assuming we go with your approach, what's the plan? We're actually
>>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>>> that can drive the panel, but we're still missing a way to hook up the
>>>> backlight and panel enabling code. So we effectively can't support any
>>>> of the LVDS devices out there without this series.
>>>
>>> Could you describe the hardware setup you have related to the LCD and
>>> backlight? Is it a public board with public schematics?
>>
>> I don't think any of the schematics are public. The Tamonten Evaluation
>> Carrier is available publicly from our website and the schematics are
>> available on demand as well. If required I can probably arrange to send
>> you a copy.
>
> No need, I think your answer below is enough.
>
>>> I've understood that you don't have anything special in your board, just
>>> an LCD and a backlight, and the power sequences are related to powering
>>> up the LCD and the backlight, without anything board specific. If so,
>>> there's no need for board specific code, but just improving the panel
>>> and backlight drivers to support the models you use.
>>
>> Correct. Basically we have two GPIOs that each enable the panel or the
>> backlight respectively and one PWM to control the brightness. Are the
>
> The panel GPIO goes to the panel hardware device, and enables the panel?
> And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
> making sure there are no other components involved.
>
>> panel drivers that you refer to those in drivers/video? I'm not sure if
>> adding more ad-hoc drivers there just to move them to a generic
>> framework in the next cycle is a good idea. I'd rather spend some time
>> on helping to get the framework right and have drivers for that instead.
>
> We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
> sure if other platforms have their own versions of panel drivers, but
> probably adding a simple panel driver system for a platform would not be
> too difficult. It could even be quite hardcoded, i.e. embedded directly
> into the display subsystem driver, just to get something working until
> the common panel framework is available.
>
> Yes, I agree it's not good idea to add more platform specific panel
> drivers. But it's unclear when CPF will be merged, so if you need to get
> the panel working now, I don't see a simple ad-hoc driver as too
> horrible. But, of course, I'm not the one making the decision whether to
> merge or not =).
>
>> From what I understand by looking at the OMAP display drivers, they also
>> provide the timings for the displays. Steffen's videomode helpers can be
>> used to represent these easily in DT, but I suppose if all of those per-
>
> Right. Note that I didn't present omap panel drivers as perfect
> examples, just examples =).
>
>> panel specifics are represented in the drivers then that won't be needed
>> anymore either.
>
> Yes, for most panels with just one native mode and nothing else, the
> panel driver can contain the timings.
>
> However, this subject has been discussed earlier a few times. If the
> panel in question doesn't need any special power-on/off sequences, just,
> perhaps, one gpio or such, we could still use DT video modes. This would
> simplify the cases where you have lots of different very simple panels.
>
> Obviously the same questions apply to DT video modes than to the power
> sequences, and while I do think it's better to handle the timings inside
> the driver, I'm not too much against video timings in DT. The reason
> being that the video modes are quite clear, simple and stable data,
> versus the much more complex and open-ended power sequences.
>
>>> I don't see any problem with adding small Tegra specific panel drivers
>>> for the time being, with the intention of converting to common panel
>>> framework when that's available.
>>
>> I can take a look at how such a driver could be implemented, but again,
>
> Don't look at the mainline omap panel drivers, at least not too closely
> =). They contain hackery that will be cleaned up with CPF.
>
> I think there are two methods to implements simple panel drivers:
>
> The hardcoded one, where the display subsystem driver manages a few
> different panel models. This is obviously not very expandable or
> "correct", but should probably work just fine for a few models, until
> CPF is usable.
>
> Something like CPF will have: have the panel device/driver as a platform
> device/driver, which will register itself to the display subsystem. And
> with "itself" I mean some kind of struct panel_entity, with a few ops
> implemented by the panel driver.
>
> Well, this goes a bit out of subject. If you want to discuss panel
> drivers more, please start a new thread. Laurent's upcoming CPF v2
> should give you good ideas what the model will be.
>
>  Tomi
>
>

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
> Mmmm so maybe I am misinterpreting things, but it looks like we
> have just buried the power sequences here, haven't we?

I don't think so. In fact I was just starting to think that maybe for
Tegra we could have a generic panel driver which used power sequences
to abstract away the pin differences for powering up the panel. Since
most likely that will be where the differences are there is a lot of
potential to factor things out into sequences.

Perhaps what we may want to postpone for now is the DT representation
since that's what Tomi and Grant seem to be mostly opposed to.

Thierry

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alexandre Courbot]

On Thu, Nov 22, 2012 at 12:12 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
>> Mmmm so maybe I am misinterpreting things, but it looks like we
>> have just buried the power sequences here, haven't we?
>
> I don't think so. In fact I was just starting to think that maybe for
> Tegra we could have a generic panel driver which used power sequences
> to abstract away the pin differences for powering up the panel. Since
> most likely that will be where the differences are there is a lot of
> potential to factor things out into sequences.
>
> Perhaps what we may want to postpone for now is the DT representation
> since that's what Tomi and Grant seem to be mostly opposed to.

The thing I don't understand here is why would anyone want power
sequences without the DT representation. Guys, that's the whole point! :)

If we are to implement things into drivers, then callback functions
are going to serve us just as well - even better, for they are more
flexible. All we need to do is define a dedicated ops structure and
have the driver plug the right callback functions depending on the
"compatible" property of the DT device node. We don't need a framework
for that.

And if we are not going to use power seqs that's probably what we
should do in order to get panels to work on Tegra boards for now. Then
see how things turn out with the panel framework and whether there is
a use for power seqs in it. But by the meantime, I don't see any
motivation to merge power seqs sans DT support.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Mark Brown]

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

On Thu, Nov 22, 2012 at 11:01:34AM +0900, Alexandre Courbot wrote:

> The thing I don't understand here is why would anyone want power
> sequences without the DT representation. Guys, that's the whole point! :)

> If we are to implement things into drivers, then callback functions
> are going to serve us just as well - even better, for they are more
> flexible. All we need to do is define a dedicated ops structure and
> have the driver plug the right callback functions depending on the
> "compatible" property of the DT device node. We don't need a framework
> for that.

It allows drivers (both board drivers and actual drivers) to write these
things in a semi-scripted form instead of having to open code everything
each time, it'd save a bunch of tedious stuff with resource requesting
for example.

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alexandre Courbot]

On Thu, Nov 22, 2012 at 11:06 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 22, 2012 at 11:01:34AM +0900, Alexandre Courbot wrote:
>
>> The thing I don't understand here is why would anyone want power
>> sequences without the DT representation. Guys, that's the whole point! :)
>
>> If we are to implement things into drivers, then callback functions
>> are going to serve us just as well - even better, for they are more
>> flexible. All we need to do is define a dedicated ops structure and
>> have the driver plug the right callback functions depending on the
>> "compatible" property of the DT device node. We don't need a framework
>> for that.
>
> It allows drivers (both board drivers and actual drivers) to write these
> things in a semi-scripted form instead of having to open code everything
> each time, it'd save a bunch of tedious stuff with resource requesting
> for example.

Mmm, I overlooked that point - that's fair enough. Guess I should
remove all DT support and stress that point in the documentation. Then
maybe we'll have a deal.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Wed, Nov 21, 2012 at 3:12 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
>> Mmmm so maybe I am misinterpreting things, but it looks like we
>> have just buried the power sequences here, haven't we?
>
> I don't think so. In fact I was just starting to think that maybe for
> Tegra we could have a generic panel driver which used power sequences
> to abstract away the pin differences for powering up the panel. Since
> most likely that will be where the differences are there is a lot of
> potential to factor things out into sequences.
>
> Perhaps what we may want to postpone for now is the DT representation
> since that's what Tomi and Grant seem to be mostly opposed to.

Don't give up. I'm am being cautions and trying to think of all the
implications because that is what I have to do as a maintainer. I'm
not opposed to the feature but I need a good level of confidence that
it has been fully thought out.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Laurent Pinchart]

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

Hi Thierry,

On Wednesday 21 November 2012 14:00:39 Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
> > On 2012-11-21 13:40, Thierry Reding wrote:
> > > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
> > (sorry for bouncing back and forth with my private and my @ti addresses.
> > I can't find an option in thunderbird to only use one sender address,
> > and I always forget to change it when responding...)
> > 
> > >> My suggestion would be to go forward with an in-driver solution, and
> > >> look at the DT based solution later if we are seeing an increasing
> > >> bloat in the drivers.
> > > 
> > > Assuming we go with your approach, what's the plan? We're actually
> > > facing this problem right now for Tegra. Basically we have a DRM driver
> > > that can drive the panel, but we're still missing a way to hook up the
> > > backlight and panel enabling code. So we effectively can't support any
> > > of the LVDS devices out there without this series.
> > 
> > Could you describe the hardware setup you have related to the LCD and
> > backlight? Is it a public board with public schematics?
> 
> I don't think any of the schematics are public. The Tamonten Evaluation
> Carrier is available publicly from our website and the schematics are
> available on demand as well. If required I can probably arrange to send
> you a copy.
> 
> > I've understood that you don't have anything special in your board, just
> > an LCD and a backlight, and the power sequences are related to powering
> > up the LCD and the backlight, without anything board specific. If so,
> > there's no need for board specific code, but just improving the panel
> > and backlight drivers to support the models you use.
> 
> Correct. Basically we have two GPIOs that each enable the panel or the
> backlight respectively and one PWM to control the brightness. Are the
> panel drivers that you refer to those in drivers/video? I'm not sure if
> adding more ad-hoc drivers there just to move them to a generic
> framework in the next cycle is a good idea. I'd rather spend some time
> on helping to get the framework right and have drivers for that instead.

Thanks :-) It should be pretty easy to add support for an enable GPIO to the 
DPI panel driver that I've posted as part of the CDF RFC v2.

> From what I understand by looking at the OMAP display drivers, they also
> provide the timings for the displays. Steffen's videomode helpers can be
> used to represent these easily in DT, but I suppose if all of those per-
> panel specifics are represented in the drivers then that won't be needed
> anymore either.

For DPI panels it might still make sense to provide the timings through DT or 
platform data, as the driver might grow huge otherwise. Panel drivers that 
support a couple (dozens) of different models could hardcode the timings.

> > > As I understand it, what you propose is similar to what ASoC does. For a
> > > specific board, you'd have to write a driver, presumably for the new
> > > panel/display framework, that provides code to power the panel on and
> > > off. That means we'll have to have a driver for each panel out there
> > > basically, or we'd need to write generic drivers that can be configured
> > > to some degree (via platform data or DT). This is similar to how ASoC
> > > works, where we have a driver that provides support for a specific codec
> > > connected to the Tegra SoC. For the display framework things could be
> > > done in a similar way I suppose, so that Tegra could have one display
> > > driver to handle all aspects of powering on and off the various panels
> > > for the various boards out there.
> > 
> > I think we should only need the board drivers for very special cases. If
> > there's just a panel and a backlight, without any special dynamic muxing
> > or other trickery needed, I don't see a need for a board driver. I
> > presume this is the case for most of the boards.
> 
> For Tegra ASoC, the way to provide for this is to allow a specific board
> to introduce a separate compatible value to enable per-board quirks or
> special handling if it cannot be supported by the generic driver and
> configuration mechanisms.
> 
> > > Obviously, a lot of the code will be similar for other SoCs, but maybe
> > > that's just the way things are if we choose that approach. There's also
> > > the potential for factoring out large chunks of common code later on
> > > once we start to see common patterns.
> > > 
> > > One thing that's not very clear is how the backlight subsystem should be
> > > wired up with the display framework. I have a patch on top of the Tegra
> > > DRM driver which adds some ad-hoc display support using this power
> > > sequences series and the pwm-backlight.
> > 
> > I think that's a separate issue: how to associate the lcd device and
> > backlight device together. I don't have a clear answer to this.
> > 
> > There are many ways the backlight may be handled. In some cases the
> > panel and the backlight are truly independent, and you can use the other
> > without using the other (not very practical, though =).
> 
> At least for DT I think we can easily wire that up. I've actually posted
> a patch recently that does so. I think in most cases it makes sense to
> control them together, such as on DPMS changes, where you really want to
> turn both the backlight and the LCD off, independent of how they are
> tied together.
> 
> > But then with some LCDs the backlight may be controlled by sending
> > commands to the panel, and in this case the two may be quite linked.
> > Changing the backlight requires the panel driver to be up and running,
> > and sometimes the sending the backlight commands may need to be (say,
> > DSI display, with backlight commands going over the DSI bus).
> > 
> > So my feeling is that the panel driver should know about the related
> > backlight device. In the first case the panel driver would just call
> > enable/disable in the backlight device when the panel is turned on.
> 
> Exactly.
> 
> > In the second case of the DSI panel... I'm not sure. I've implemented it
> > so that the panel driver creates the backlight device, and implements
> > the backlight callbacks. It then sends the DSI commands from those
> > callbacks.
> 
> That certainly sounds like the right approach to me.
> 
> > > From reading the proposal for the panel/display framework, it sounds
> > > like a lot more is planned than just enabling or disabling panels, but
> > > it also seems like a lot of code needs to be written to support things
> > > like DSI, DBI or other control busses.
> > > 
> > > At least for Tegra, and I think the same holds for a wide variety of
> > > other SoCs, dumb panels would be enough for a start. In the interest of
> > > getting a working solution for those setups, maybe we can start small
> > > and add just enough framework to register dumb panel drivers to along
> > > with code to wire up a backlight to light up the display. Then we could
> > > possibly still make it to have a proper solution to support the various
> > > LVDS panels for Tegra with 3.9.
> > 
> > Yes, we (Laurent and me) both agree that we should start simple.
> > 
> > However, the common panel framework is not strictly needed for this. I'm
> > not sure of the current architecture for Tegra, but for OMAP we already
> > have panel drivers (omap specific ones, though). The panel drivers may
> > support multiple models, (for example,
> > drivers/video/omap2/displays/panel-generic-dpi.c).
> > 
> > I don't see any problem with adding small Tegra specific panel drivers
> > for the time being, with the intention of converting to common panel
> > framework when that's available.
> 
> I can take a look at how such a driver could be implemented, but again,
> I'm a bit reluctant to add something ad-hoc now when maybe we can have
> it supported in a proper framework not too far away in the future.
> 
> > Of course, the DT side is an issue. If you now create DT bindings for a
> > temporary model, and need to change it again later, you'll have some
> > headaches trying managing that without breaking the old bindings... This
> > is why I haven't pushed DT bindings for OMAP, as I know I have to change
> > them in the near future.
> 
> We're already keeping back on this and none of the patches that define
> the bindings have been merged yet. Although bindings have been known to
> change every once in a while even for code that is already in mainline.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Laurent Pinchart]

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

Hi Tomi,

On Wednesday 21 November 2012 14:04:17 Tomi Valkeinen wrote:
> On 2012-11-21 13:40, Thierry Reding wrote:

[snip]

> > One thing that's not very clear is how the backlight subsystem should be
> > wired up with the display framework. I have a patch on top of the Tegra
> > DRM driver which adds some ad-hoc display support using this power
> > sequences series and the pwm-backlight.
> 
> I think that's a separate issue: how to associate the lcd device and
> backlight device together. I don't have a clear answer to this.
> 
> There are many ways the backlight may be handled. In some cases the
> panel and the backlight are truly independent, and you can use the other
> without using the other (not very practical, though =).

>From a control point of view that's always the case for DPI panels (as those 
panels have no control bus, the backlight control bus is by definition 
separate) and is the case for the two DBI panels I've seen (but I won't claim 
that's a significative number of panels).

> But then with some LCDs the backlight may be controlled by sending commands
> to the panel, and in this case the two may be quite linked. Changing the
> backlight requires the panel driver to be up and running, and sometimes the
> sending the backlight commands may need to be (say, DSI display, with
> backlight commands going over the DSI bus).

When you write "sending commands to the panel", do you mean on the same 
control bus that the panel use ? Or would that also include for instance an 
I2C backlight controller integrated inside a DSI panel module ? In the later 
case there might still be dependencies between the panel controller and the 
backlight controller (let's say for instance that the panel controller has a 
DSI-controller GPIO wired to the backlight controller reset signal), but in 
practice I don't know if that's ever the case.

> So my feeling is that the panel driver should know about the related
> backlight device. In the first case the panel driver would just call
> enable/disable in the backlight device when the panel is turned on.

That makes sense. Unless I'm mistaken a backlight is always associated with a 
panel (it would be called a light if there was no panel in front of it). We 
can thus expose backlight operations in the panel CDF (in-kernel) API. The 
panel driver would need a way to retrieve a pointer to the associated 
backlight device.

> In the second case of the DSI panel... I'm not sure. I've implemented it
> so that the panel driver creates the backlight device, and implements
> the backlight callbacks. It then sends the DSI commands from those
> callbacks.

If we decide to make the panel expose backlight operations we could get rid of 
the backlight device in this case.

> > From reading the proposal for the panel/display framework, it sounds
> > like a lot more is planned than just enabling or disabling panels, but
> > it also seems like a lot of code needs to be written to support things
> > like DSI, DBI or other control busses.
> > 
> > At least for Tegra, and I think the same holds for a wide variety of
> > other SoCs, dumb panels would be enough for a start. In the interest of
> > getting a working solution for those setups, maybe we can start small
> > and add just enough framework to register dumb panel drivers to along
> > with code to wire up a backlight to light up the display. Then we could
> > possibly still make it to have a proper solution to support the various
> > LVDS panels for Tegra with 3.9.
> 
> Yes, we (Laurent and me) both agree that we should start simple.
> 
> However, the common panel framework is not strictly needed for this. I'm
> not sure of the current architecture for Tegra, but for OMAP we already
> have panel drivers (omap specific ones, though). The panel drivers may
> support multiple models, (for example,
> drivers/video/omap2/displays/panel-generic-dpi.c).
> 
> I don't see any problem with adding small Tegra specific panel drivers
> for the time being, with the intention of converting to common panel
> framework when that's available.

I'm fine with that, but using the CDF would be even better :-)

> Of course, the DT side is an issue. If you now create DT bindings for a
> temporary model, and need to change it again later, you'll have some
> headaches trying managing that without breaking the old bindings... This
> is why I haven't pushed DT bindings for OMAP, as I know I have to change
> them in the near future.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Tomi Valkeinen]

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

On 2012-11-27 17:08, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wednesday 21 November 2012 14:04:17 Tomi Valkeinen wrote:
>> On 2012-11-21 13:40, Thierry Reding wrote:
> 
> [snip]
> 
>>> One thing that's not very clear is how the backlight subsystem should be
>>> wired up with the display framework. I have a patch on top of the Tegra
>>> DRM driver which adds some ad-hoc display support using this power
>>> sequences series and the pwm-backlight.
>>
>> I think that's a separate issue: how to associate the lcd device and
>> backlight device together. I don't have a clear answer to this.
>>
>> There are many ways the backlight may be handled. In some cases the
>> panel and the backlight are truly independent, and you can use the other
>> without using the other (not very practical, though =).
> 
> From a control point of view that's always the case for DPI panels (as those 
> panels have no control bus, the backlight control bus is by definition 
> separate) and is the case for the two DBI panels I've seen (but I won't claim 
> that's a significative number of panels).

They may have a control bus, I2C, SPI, etc. In some cases that can be
used to control the backlight. But yes, it's separate from the video bus.

>> But then with some LCDs the backlight may be controlled by sending commands
>> to the panel, and in this case the two may be quite linked. Changing the
>> backlight requires the panel driver to be up and running, and sometimes the
>> sending the backlight commands may need to be (say, DSI display, with
>> backlight commands going over the DSI bus).
> 
> When you write "sending commands to the panel", do you mean on the same 
> control bus that the panel use ? Or would that also include for instance an 
> I2C backlight controller integrated inside a DSI panel module ? In the later 

I mean the same control bus that is used to control the panel, be it
shared with video bus like DSI, or separate like I2C.

> case there might still be dependencies between the panel controller and the 
> backlight controller (let's say for instance that the panel controller has a 
> DSI-controller GPIO wired to the backlight controller reset signal), but in 
> practice I don't know if that's ever the case.
> 
>> So my feeling is that the panel driver should know about the related
>> backlight device. In the first case the panel driver would just call
>> enable/disable in the backlight device when the panel is turned on.
> 
> That makes sense. Unless I'm mistaken a backlight is always associated with a 
> panel (it would be called a light if there was no panel in front of it). We 
> can thus expose backlight operations in the panel CDF (in-kernel) API. The 
> panel driver would need a way to retrieve a pointer to the associated 
> backlight device.

I agree.

>> In the second case of the DSI panel... I'm not sure. I've implemented it
>> so that the panel driver creates the backlight device, and implements
>> the backlight callbacks. It then sends the DSI commands from those
>> callbacks.
> 
> If we decide to make the panel expose backlight operations we could get rid of 
> the backlight device in this case.

Do you mean there would be a real backlight device only when there's a
totally independent backlight for the panel?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Laurent Pinchart]

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

Hi Tomi,

On Tuesday 27 November 2012 17:19:05 Tomi Valkeinen wrote:
> On 2012-11-27 17:08, Laurent Pinchart wrote:
> > On Wednesday 21 November 2012 14:04:17 Tomi Valkeinen wrote:
> >> On 2012-11-21 13:40, Thierry Reding wrote:
> > [snip]
> > 
> >>> One thing that's not very clear is how the backlight subsystem should be
> >>> wired up with the display framework. I have a patch on top of the Tegra
> >>> DRM driver which adds some ad-hoc display support using this power
> >>> sequences series and the pwm-backlight.
> >> 
> >> I think that's a separate issue: how to associate the lcd device and
> >> backlight device together. I don't have a clear answer to this.
> >> 
> >> There are many ways the backlight may be handled. In some cases the
> >> panel and the backlight are truly independent, and you can use the other
> >> without using the other (not very practical, though =).
> > 
> > From a control point of view that's always the case for DPI panels (as
> > those panels have no control bus, the backlight control bus is by
> > definition separate) and is the case for the two DBI panels I've seen
> > (but I won't claim that's a significative number of panels).
> 
> They may have a control bus, I2C, SPI, etc. In some cases that can be
> used to control the backlight. But yes, it's separate from the video bus.
> 
> >> But then with some LCDs the backlight may be controlled by sending
> >> commands to the panel, and in this case the two may be quite linked.
> >> Changing the backlight requires the panel driver to be up and running,
> >> and sometimes the sending the backlight commands may need to be (say, DSI
> >> display, with backlight commands going over the DSI bus).
> > 
> > When you write "sending commands to the panel", do you mean on the same
> > control bus that the panel use ? Or would that also include for instance
> > an I2C backlight controller integrated inside a DSI panel module ? In the
> > later
>
> I mean the same control bus that is used to control the panel, be it shared
> with video bus like DSI, or separate like I2C.
> 
> > case there might still be dependencies between the panel controller and
> > the backlight controller (let's say for instance that the panel controller
> > has a DSI-controller GPIO wired to the backlight controller reset signal),
> > but in practice I don't know if that's ever the case.
> > 
> >> So my feeling is that the panel driver should know about the related
> >> backlight device. In the first case the panel driver would just call
> >> enable/disable in the backlight device when the panel is turned on.
> > 
> > That makes sense. Unless I'm mistaken a backlight is always associated
> > with a panel (it would be called a light if there was no panel in front
> > of it). We can thus expose backlight operations in the panel CDF
> > (in-kernel) API. The panel driver would need a way to retrieve a pointer
> > to the associated backlight device.
> 
> I agree.
> 
> >> In the second case of the DSI panel... I'm not sure. I've implemented it
> >> so that the panel driver creates the backlight device, and implements
> >> the backlight callbacks. It then sends the DSI commands from those
> >> callbacks.
> > 
> > If we decide to make the panel expose backlight operations we could get
> > rid of the backlight device in this case.
> 
> Do you mean there would be a real backlight device only when there's a
> totally independent backlight for the panel?

I'm toying with that idea.

I see two reasons for having backlight devices in the kernel right now.

- Integration with the fbdev API to control the backlight automatically on 
fbdev blank/unblank/suspend/resume. That's something I want to get rid of, 
backlight should not depend on fbdev. If the backlight in-kernel API is 
exposed by the panel through CDF operations we won't need this anymore.

- Exposing backlight control to userspace in sysfs. That's quite probably 
something we want to keep, if only for compatibility reasons. Backlight on/off 
should be controlled through the display APIs through DPMS control, but we 
have no way that I'm aware of to control the backlight brightness in the FBDEV 
and KMS APIs. It might make sense to add a backlight API to KMS for the 
future.

One possibly crazy idea I had was to replace backlight devices as we know them 
with LED devices (a LED driver IC shouldn't be supported by different APIs 
depending on whether the LEDs it drives are used as a backlight or not), and 
have the panel hook up with the associated LED device to expose backlight 
operations through the CDF. Panels with a backlight controller tied to the 
panel controller (control through DSI for instance) would implement the 
backlight operations directly without instantiating a LED device. This still 
leaves the question of the userspace backlight brightness API open.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Mark Brown]

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

On Tue, Nov 27, 2012 at 04:37:32PM +0100, Laurent Pinchart wrote:

> One possibly crazy idea I had was to replace backlight devices as we know them 
> with LED devices (a LED driver IC shouldn't be supported by different APIs 
> depending on whether the LEDs it drives are used as a backlight or not), and 
> have the panel hook up with the associated LED device to expose backlight 
> operations through the CDF. Panels with a backlight controller tied to the 
> panel controller (control through DSI for instance) would implement the 
> backlight operations directly without instantiating a LED device. This still 
> leaves the question of the userspace backlight brightness API open.

I remember looking at that before but never actually got round to it, it
seems like a sensible idea to me.

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Laurent Pinchart]

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

Hi Thierry,

Sorry for the late reply.

On Wednesday 21 November 2012 12:40:18 Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
> > On 2012-11-21 06:23, Alex Courbot wrote:
> > > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> > >>> 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
> > >> 
> > >> This isn't strictly true. It is still perfectly fine to have board
> > >> specific code when necessary. However, there is strong encouragement to
> > >> enable that code in device drivers as much as possible and new board
> > >> files need to have very strong justification.
> > > 
> > > But doesn't introducing board-specific code into the kernel just defeats
> > > the purpose of the DT? If we extend this logic, we are heading straight
> > > back to board-definition files. To a lesser extent than before I agree,
> > > but the problem is fundamentally the same.
> > 
> > I don't think so. I'll reiterate my opinion on this subject, as a
> > summary and for those who haven't read the discussions of the earlier
> > versions of the series. And perhaps I'll even manage to say something
> > new =).
> > 
> > 
> > First about the board specific code. I think we may need some board
> > specific code, even if this series was merged. Let's call them board
> > drivers. These board drivers would only exist for boards with some weird
> > setups that cannot be expressed or managed with DT and normal drivers.
> > 
> > I think these cases would be quite rare, as I can't even come up with a
> > very good example. I guess most likely these cases would involve some
> > small trivial chips for muxing or such, for which it doesn't really make
> > sense to have a real driver.
> > 
> > Say, perhaps a board with two LCDs connected to one video output, and
> > only one LCD can be enabled at a time, and you need to set some mux chip
> > to route the signals to the correct LCD. In this case I'd see we should
> > have hotplug support in the display framework, and the board driver
> > would act on user input (sysfs file, perhaps), plugging in/out the LCD
> > device depending on the user input.
> > 
> > 
> > As for expressing device internal details in the DT data, as done in
> > this series, I don't like it very much. I think the DT data or the board
> > file should just describe which device is the one in question, and how
> > it's connected to other components on the board. The driver for the
> > device should handle everything else.
> > 
> > As Alex pointed out, there may be lots of devices that work the same way
> > when enabled, but require slightly different power-on/off sequences. We
> > could have these sequences in the driver itself, either as plain code,
> > or in a table of some sort, possibly using the power sequence framework
> > presented in this series. The correct code or sequence would be ran
> > depending on the particular model of the device.
> > 
> > I think this approach is correct in the sense that this is what drivers
> > are supposed to do: handle all the device internal matters. But this
> > raises the problem of bloating the kernel with possibly lots of
> > different power sequences, of which only a few would be used by a board,
> > and all the rest would just be waste of memory.
> > 
> > Regarding this problem I have the following questions, to which I don't
> > have clear answers:
> > 
> > - How much of this device specific data is too much? If a driver
> > supports 10 different models, and the sequences for each model take 100
> > bytes, this 1000 bytes doesn't sound too much. But where's the limit?
> > And even if one driver only has 1kB of this data, what if we have lots
> > of similar drivers?
> > 
> > - How many bytes does a power sequence presented in this series take, if
> > the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and
> > regulator?
> > 
> > - How likely is it that we'll get lots of different models? A hundred
> > different models for a backlight PWM with different power-on/off
> > sequences already sounds a really big number. If we're only going to
> > have each driver supporting, say, no more than 10 models, perhaps the
> > problem is not even an issue in practice.
> > 
> > - Are there ways to limit this bloat in the driver? One obvious way
> > would be to discard the unused sequences after driver probe, but that
> > only works for platform devices. Although, I guess these sequences would
> > mostly be used by platform drivers? If so, then the problem could be
> > solved by discarding the data after probe. Another would be to load the
> > sequences from a file as firmware, but that feels quite an awful
> > solution. Anybody have other ideas?
> > 
> > One clear positive side with in-driver approach is that it's totally
> > inside the kernel, and can be easily reworked in the future, or even
> > changed to a DT-based approach as presented in this series if that seems
> > like a better solution. The DT based approach, on the other hand, will
> > be more or less written to stone after it's merged.
> > 
> > 
> > So, I like the framework of expressing the sequences presented in this
> > series (except there's a problem with error cases, as I pointed out in
> > another post), as it can be used inside the drivers. But I'm not so
> > enthusiastic about presenting the sequences in DT.
> > 
> > My suggestion would be to go forward with an in-driver solution, and
> > look at the DT based solution later if we are seeing an increasing bloat
> > in the drivers.
> 
> Assuming we go with your approach, what's the plan? We're actually
> facing this problem right now for Tegra. Basically we have a DRM driver
> that can drive the panel, but we're still missing a way to hook up the
> backlight and panel enabling code. So we effectively can't support any
> of the LVDS devices out there without this series.
> 
> As I understand it, what you propose is similar to what ASoC does. For a
> specific board, you'd have to write a driver, presumably for the new
> panel/display framework, that provides code to power the panel on and
> off. That means we'll have to have a driver for each panel out there
> basically, or we'd need to write generic drivers that can be configured
> to some degree (via platform data or DT). This is similar to how ASoC
> works, where we have a driver that provides support for a specific codec
> connected to the Tegra SoC. For the display framework things could be
> done in a similar way I suppose, so that Tegra could have one display
> driver to handle all aspects of powering on and off the various panels
> for the various boards out there.

The common display framework (CDF) includes a driver for dumb parallel panels 
called panel-dpi. It doesn't handle complex power on/off sequences yet as I 
haven't run into any need for them, but that's definitely something we could 
add, possibly using Alexandre's code to express the sequences. Similarly to 
Tomi, my preferences go for including the sequences in drivers.

> Obviously, a lot of the code will be similar for other SoCs, but maybe
> that's just the way things are if we choose that approach. There's also
> the potential for factoring out large chunks of common code later on
> once we start to see common patterns.

I don't mind duplicating some code in different drivers as a first step. 
Factoring out common code in a second step is easier (at least for me) than 
trying to figure out what the common code would look like before the first 
driver is even written.

> One thing that's not very clear is how the backlight subsystem should be
> wired up with the display framework. I have a patch on top of the Tegra
> DRM driver which adds some ad-hoc display support using this power
> sequences series and the pwm-backlight.

I've touched the backlight API a bit, from both the provider and consumer 
points of view, and my feeling is that the API will need to be reworked to 
neatly integrate with the CDF. The backlight API currently depends on fbdev, 
that's something we should aim to remove. Proposals are welcome :-) Should we 
create a separate mail thread for this ?

> From reading the proposal for the panel/display framework, it sounds
> like a lot more is planned than just enabling or disabling panels,

That's correct. CDF implements several other panel control operations (to 
retrieve the list of modes supported by the panel for instance), and more 
operations will be needed in the future. I've decided to only implement in the 
RFC operations that I need for the platforms I work on, and let other 
developers propose additional operations depending on their use cases.

> but it also seems like a lot of code needs to be written to support things
> like DSI, DBI or other control busses.

I don't think we need that much code there. The most common control busses for 
display hardware are DSI, DBI, I2C, SPI and plain GPIOs. The last three 
control busses are well supported in the Linux kernel, I've implemented DBI 
support as part of the CDF RFC, and DSI support is missing. If you look at the 
DBI bus implementation the amount of code isn't overly large.

> At least for Tegra, and I think the same holds for a wide variety of other
> SoCs, dumb panels would be enough for a start. In the interest of getting a
> working solution for those setups, maybe we can start small and add just
> enough framework to register dumb panel drivers to along with code to wire
> up a backlight to light up the display. Then we could possibly still make it
> to have a proper solution to support the various LVDS panels for Tegra with
> 3.9.
> 
> I'm adding Laurent on Cc since he's probably busy working on a new
> proposal for the panel/display framework. Maybe he can share his thought
> on this.
>
> All of the above said, I'm willing to help out with the coding if that's
> what is required to reach a solution that everybody can be happy with.

That's much appreciated. If that means a couple additional hours of sleep per 
week for me I'll be extremely thankful :-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Wed, 21 Nov 2012 13:23:06 +0900, Alex Courbot <acourbot@nvidia.com> wrote:
> Hi Grant,
> 
> On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> > > 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
> > 
> > This isn't strictly true. It is still perfectly fine to have board
> > specific code when necessary. However, there is strong encouragement to
> > enable that code in device drivers as much as possible and new board
> > files need to have very strong justification.
> 
> But doesn't introducing board-specific code into the kernel just defeats the 
> purpose of the DT? If we extend this logic, we are heading straight back to 
> board-definition files. To a lesser extent than before I agree, but the problem 
> is fundamentally the same.

There is *always* board specific code. There is always something fiddly
enough, complex enough, or just plain ugly enough that it is best
handled in C code. The trick is to make the board-specific stuff the
exception, not the rule.

When you can factor out common behavour (like you are doing here), then
it is a really good candidate for a common binding, but still please
always ask yourself the question is this going to make things better or
worse in the long run.

> > I'm thinking about the scripts as trivial-to-parse ascii strings that
> > have a very simple set of commands. The commands use resources already
> > defined in the node. ie. 'g0' refers to the first entry in the gpios
> > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> > no means take this as the format to use, it is just an example off the
> > top of my head, but it is already way easier to work with than putting
> > each command into a node.
> 
> I'd argue that this opens the door wide open towards having a complete 
> interpreter in the device tree. At least DT nodes restrict what we can do 
> conveniently.

I disagree... see below.

> > The trick is still to define a syntax that doesn't fall apart when it
> > needs to be extended. I would also like to get opinions on whether or
> > not conditionals or loops should be supported (ie. loop waiting for a
> > status to change). If they should then we need to be a lot more careful
> > about the design (and due to my aforementioned nervousness, somebody may
> > need to get me therapy).
> 
> That's IMHO the main argument in favor of using DT nodes here (the syntax 
> extension, not your therapy).

hahaha :-)

> They can be extended simply by adding properties 
> to them, and I was relying on this to make power seqs extensible while keeping 
> things consistent (and backward-compatible). For instance, when we want to 
> support voltage setting in regulators we can just add a "voltage" property for 
> that.
> 
> So to sum up the pros of the current node-base representation:
> - give a "data like" representation of powering sequences (what they should 
> be, actually)
> - puts sanity barriers for not turning power seqs into a real code interpreter
> - easily extensible
> 
> There are probably a few cons, the numbering of steps being one, but at least 
> we don't need to design a new DSL and care too much about extensibility which 
> is, in the nodes case, built-in and free.

No matter how it is encoded, this *IS* a new DSL. Using nodes and
properties doesn't change that. Extensibility is no more built-in and
free with nodes that it is with another encoding. If there aren't clear
guidelines on how to extend it then we'll get weird stuff. For example,
even with the nodes case someone might do this:

	step3 {
		type = "loop";
		count = 10;
		step1 {
			type = "gpio";
			gpio = <&gpio 1>;
			value = 1;
		};
		step2 {
			type = "delay";
			value = 10000;
		};
		step3 {
			type = "gpio";
			gpio = <&gpio 1>;
			value = 0;
		};
		step4 {
			type = "delay";
			value = 10000;
		};
	};

And even while cringing as I type the above, I also have to consider
that looping may just be a valid use case for sequences.

And even here, a very simple sequence fragment required 22 lines of
code.

Next, I'm concerned about where these will show up. Say for instance
there needs to be a power sequence added to an spi bus node. Already spi
bus child nodes have a defined meaning; they are spi slaves. How then
should the sequence be attached to the spi bus?

> If that makes you feel better, maybe we can try and fix what is wrong with the 
> current bindings instead of introducing a new language that will be, by 
> nature, more complex to handle and difficult to extend without breaking things?

Okay, here are 3 concrete objections with the proposed binding:
- The syntax concerns of it being too verbose and it effectively uses
  line numbers for ordering (Do you remember fighting with BASIC?).
- There are many devices that won't be able to use the binding because
  they already have a meaning for child nodes
- I think resources should be declared separate from the sequence based
  on the assumption that multiple steps will be operating on the same
  resource.

I do think that each sequence should be contained within a single
property, but I'm open to other suggestions.

g.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Thierry Reding]

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

On Thu, Nov 22, 2012 at 01:39:41PM +0000, Grant Likely wrote:
[...]
> I do think that each sequence should be contained within a single
> property, but I'm open to other suggestions.

IIRC a very early prototype did implement something like that. However
because of the resource issues this had to be string based, so that the
sequences looked somewhat like (Alex, correct me if I'm wrong):

	power-on = <"REGULATOR", "power", 1, "GPIO", "enable", 1>;

Instead we could possibly have something like:

	power-on = <0 &reg 1,
		    1 &gpio 42 0 1>;

Where the first cell in each entry defines the type (0 = regulator, 1 =
GPIO) and the rest would be a regular OF specifier for the given type of
resource along with some defined parameter such as enable/disable,
voltage, delay in ms, ... I don't know if that sounds any better. It
looks sort of cryptic but it is more "in the spirit of" DT, right Grant?

Writing this down, it seems to me like even that proposal was already
discussed at some point. Again, Alex may remember better.

Thierry

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

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

On Friday 23 November 2012 05:40:21 Thierry Reding wrote:
> On Thu, Nov 22, 2012 at 01:39:41PM +0000, Grant Likely wrote:
> [...]
> 
> > I do think that each sequence should be contained within a single
> > property, but I'm open to other suggestions.
> 
> IIRC a very early prototype did implement something like that. However
> because of the resource issues this had to be string based, so that the
> sequences looked somewhat like (Alex, correct me if I'm wrong):
> 
> 	power-on = <"REGULATOR", "power", 1, "GPIO", "enable", 1>;

You're right. Back when the burning sun of July was beating down a little bit 
too hard on my head.

https://lkml.org/lkml/2012/7/9/30

> Instead we could possibly have something like:
> 
> 	power-on = <0 &reg 1,
> 		    1 &gpio 42 0 1>;
> 
> Where the first cell in each entry defines the type (0 = regulator, 1 =
> GPIO) and the rest would be a regular OF specifier for the given type of
> resource along with some defined parameter such as enable/disable,
> voltage, delay in ms, ... I don't know if that sounds any better. It
> looks sort of cryptic but it is more "in the spirit of" DT, right Grant?
> 
> Writing this down, it seems to me like even that proposal was already
> discussed at some point. Again, Alex may remember better.

The idea that we had was to use preprocessor support in DTC to use macros 
instead of strings for the step type. We also thought about using phandles 
directly in there as well, but this would require some more API support.

Anyway, at the current point we are not even sure whether we want or need 
power seqs in the DT - so let's keep this topic on hold for a while. We can 
still introduce the feature without DT support, and if it eventually turns out 
during this winter that expressing power seqs in the DT makes sense, we will 
have plenty of archives to read in front of the fire.

Alex.

Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Thu, 22 Nov 2012 22:40:21 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> On Thu, Nov 22, 2012 at 01:39:41PM +0000, Grant Likely wrote:
> [...]
> > I do think that each sequence should be contained within a single
> > property, but I'm open to other suggestions.
> 
> IIRC a very early prototype did implement something like that. However
> because of the resource issues this had to be string based, so that the
> sequences looked somewhat like (Alex, correct me if I'm wrong):
> 
> 	power-on = <"REGULATOR", "power", 1, "GPIO", "enable", 1>;
> 
> Instead we could possibly have something like:
> 
> 	power-on = <0 &reg 1,
> 		    1 &gpio 42 0 1>;

Yes, that would work, although I still think it would be a good idea to
split the used resources off into the gpios/pwms/regs/etc properties.

> Where the first cell in each entry defines the type (0 = regulator, 1 =
> GPIO) and the rest would be a regular OF specifier for the given type of
> resource along with some defined parameter such as enable/disable,
> voltage, delay in ms, ... I don't know if that sounds any better. It
> looks sort of cryptic but it is more "in the spirit of" DT, right Grant?

It is still kind of a ham-handed approach, but it does fit better with
existing conventions than the hierarchy of nodes does.

g.

[PATCHv9 2/3] 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>
Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>
Tested-by: Stephen Warren <swarren@nvidia.com>
Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 .../bindings/video/backlight/pwm-backlight.txt     |  63 +++++++-
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 160 ++++++++++++++++-----
 include/linux/pwm_backlight.h                      |  18 ++-
 4 files changed, 202 insertions(+), 40 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..b20e98e 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -13,16 +13,73 @@ Required properties:
 
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
-               "pwms" property (see PWM binding[0])
+      "pwms" property (see PWM binding[0]).
+  - 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/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>;
+		low-threshold-brightness = <50>;
+
+		/* resources used by the power sequences */
+		pwms = <&pwm 0 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;
+				};
+			};
+		};
 	};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..a6b0640 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -240,6 +240,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 069983c..cfc0780 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,51 @@ 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_on_seq) {
+		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_enable(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_off_seq) {
+		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_config(pb->pwm, 0, pb->period);
+		pwm_disable(pb->pwm);
+	}
+
+	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 +102,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 +116,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		duty_cycle = pb->lth_brightness +
 		     (duty_cycle * (pb->period - pb->lth_brightness) / max);
 		pwm_config(pb->pwm, duty_cycle, pb->period);
-		pwm_enable(pb->pwm);
+		pwm_backlight_on(bl);
 	}
 
 	if (pb->notify_after)
@@ -145,11 +195,10 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		data->max_brightness--;
 	}
 
-	/*
-	 * TODO: Most users of this driver use a number of GPIOs to control
-	 *       backlight power. Support for specifying these needs to be
-	 *       added.
-	 */
+	/* read power sequences */
+	data->power_seqs = devm_of_parse_power_seq_set(dev);
+	if (IS_ERR(data->power_seqs))
+		return PTR_ERR(data->power_seqs);
 
 	return 0;
 }
@@ -172,6 +221,7 @@ 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;
@@ -180,7 +230,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 	if (!data) {
 		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;
 		}
@@ -201,6 +253,68 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	if (data->power_seqs) {
+		/* use power sequences */
+		struct power_seq_set *seqs = &pb->power_seqs;
+
+		power_seq_set_init(seqs, &pdev->dev);
+		power_seq_set_add_sequences(seqs, data->power_seqs);
+
+		/* Check that the required sequences are here */
+		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 resource 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;
+		}
+		/* from here we should have a PWM */
+		if (!pb->pwm) {
+			dev_err(&pdev->dev, "no PWM defined!\n");
+			return -EINVAL;
+		}
+	} else {
+		/* use legacy interface */
+		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");
+				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);
+	}
+
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
@@ -213,28 +327,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 
-	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");
-			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);
 
@@ -267,8 +359,7 @@ 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_backlight_off(bl);
 	if (pb->exit)
 		pb->exit(&pdev->dev);
 	return 0;
@@ -282,8 +373,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..0dcec1d 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,28 @@
 #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. Must include exactly
+	 * two power sequences named 'power-on' and 'power-off'. If NULL,
+	 * the legacy interface is used.
+	 */
+	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.8.0

[PATCHv9 3/3] Take maintainership of power sequences

[Alexandre Courbot]

Add entry for power sequences into MAINTAINERS with all the needed
contact and SCM info.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bb0b27d..e9098f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5745,6 +5745,16 @@ F:	fs/timerfd.c
 F:	include/linux/timer*
 F:	kernel/*timer*
 
+POWER SEQUENCES
+M:	Alexandre Courbot <acourbot@nvidia.com>
+S:	Supported
+W:	https://github.com/Gnurou/linux
+T:	git git://github.com/Gnurou/linux.git power-seq/for-next
+F:	Documentation/devicetree/bindings/power/power_seq.txt
+F:	Documentation/power/power_seq.txt
+F:	include/linux/power_seq.h
+F:	drivers/power/power_seq/
+
 POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
 M:	Anton Vorontsov <cbou@mail.ru>
 M:	David Woodhouse <dwmw2@infradead.org>
-- 
1.8.0

Re: [PATCHv9 0/3] Runtime Interpreted Power Sequences

[Grant Likely]

On Sat, 17 Nov 2012 19:55:44 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Apologies for sending two patchsets in two days - the main purpose
> of this new revision is to add the linux-arm-kernel list to the
> audience. A few suggestions from v8 have also been added.

Just to be clear from my previous reply, don't merge this series. The DT
binding is not good.

g.