[RFC][PATCH v3 0/3] Power sequences with PWM and DT support

[RFC][PATCH v3 0/3] Power sequences with PWM and DT support

[Alexandre Courbot]

Widening audience for this version.

The issue addressed is that the power sequences for backlights (and probably
other devices), which were implemented using board-specific callbacks so far,
could not be used with the device tree. This series of patches adds a small
power sequence interpreter that allows to acquire and control regulators, GPIOs,
and PWMs during sequences defined in the device tree or as platform data. It is
easy to use, low-footprint, and takes care of managing the resources it
acquires. This should help making the kernel less board-dependant.

Comments are especially welcome about the DT bindings proposal, which most
certainly needs to be refined, if not completely redesigned. Documentation is
still incomplete and will consolidate once the implementation reaches a stable
state.

The tegra device tree nodes of patch 3 are just here as an example usage.

Changelog:

v3:
- Move to driver to base/
- Use sub-nodes to describe steps in the DT as suggested by Simon Glass
- Write some documentation
- Make the DT support optional

Alexandre Courbot (3):
  runtime interpreted power sequences
  pwm_backlight: use power sequences
  tegra: add pwm backlight device tree nodes

 .../bindings/video/backlight/pwm-backlight.txt     |  55 +++-
 Documentation/power/power_seq.txt                  | 120 +++++++++
 arch/arm/boot/dts/tegra20-ventana.dts              |  53 ++++
 arch/arm/boot/dts/tegra20.dtsi                     |   2 +-
 drivers/base/Kconfig                               |   4 +
 drivers/base/Makefile                              |   1 +
 drivers/base/power_seq.c                           | 300 +++++++++++++++++++++
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 213 +++++++++++----
 include/linux/power_seq.h                          | 139 ++++++++++
 include/linux/pwm_backlight.h                      |  18 +-
 11 files changed, 843 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/power/power_seq.txt
 create mode 100644 drivers/base/power_seq.c
 create mode 100644 include/linux/power_seq.h

-- 
1.7.11.3

[RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alexandre Courbot]

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

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

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/power/power_seq.txt | 120 +++++++++++++++
 drivers/base/Kconfig              |   4 +
 drivers/base/Makefile             |   1 +
 drivers/base/power_seq.c          | 300 ++++++++++++++++++++++++++++++++++++++
 include/linux/power_seq.h         | 139 ++++++++++++++++++
 5 files changed, 564 insertions(+)
 create mode 100644 Documentation/power/power_seq.txt
 create mode 100644 drivers/base/power_seq.c
 create mode 100644 include/linux/power_seq.h

diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
new file mode 100644
index 0000000..aa2ceb5
--- /dev/null
+++ b/Documentation/power/power_seq.txt
@@ -0,0 +1,120 @@
+Runtime Interpreted Power Sequences
+-----------------------------------
+
+Problem
+-------
+One very common board-dependent code is the out-of-driver code that is used to
+turn a device on or off. For instance, SoC boards very commonly use a GPIO
+(abstracted to a regulator or not) to control the power supply of a backlight,
+disabling it when the backlight is not used in order to save power. The GPIO
+that should be used, however, as well as the exact power sequence that may
+involve different resources, is board-dependent and thus unknown of the driver.
+
+This has been addressed so far by using hooks in the device's platform data that
+are called whenever the state of the device might reflect a power change. This
+approach, however, introduces board-dependant code into the kernel and is not
+compatible with the device tree.
+
+The Runtime Interpreted Power Sequences (or power sequences for short) aim at
+turning this code into platform data or device tree nodes. Power sequences are
+described using a simple format and run by a simple interpreter whenever needed.
+This allows to remove the callback mechanism and makes the kernel less
+board-dependant.
+
+Sequences Format
+----------------
+Power sequences are a series of sequential steps during which an action is
+performed on a resource. The supported resources so far are:
+- GPIOs
+- Regulators
+- PWMs
+
+Each step designates a resource and the following parameters:
+- Whether the step should enable or disable the resource,
+- Delay to wait before performing the action,
+- Delay to wait after performing the action.
+
+Both new resources and parameters can be introduced, but the goal is of course
+to keep things as simple and compact as possible.
+
+The platform data is a simple array of platform_power_seq_step instances, each
+instance describing a step. The type as well as one of id or gpio members
+(depending on the type) must be specified. The last step must be of type
+POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
+identified by number. For example, the following sequence will turn on the
+"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
+
+struct platform_power_seq_step power_on_seq[] = {
+	{
+		.type = POWER_SEQ_REGULATOR,
+		.id = "power",
+		.params = {
+			.enable = 1,
+			.post_delay = 10,
+		},
+	},
+	{
+		.type = POWER_SEQ_GPIO,
+		.gpio = 110,
+		.params = {
+			.enable = 1,
+		},
+	},
+	{
+		.type = POWER_SEQ_STOP,
+	},
+};
+
+Usage by Drivers and Resources Management
+-----------------------------------------
+Power sequences make use of resources that must be properly allocated and
+managed. The power_seq_build() function takes care of resolving the resources as
+they are met in the sequence and to allocate them if needed:
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq);
+
+You will need an instance of power_seq_resources to keep track of the resources
+that are already allocated. On success, the function returns a devm allocated
+resolved sequence that is ready to be passed to power_seq_run(). In case of
+failure, and error code is returned.
+
+A resolved power sequence returned by power_seq_build can be run by
+power_run_run():
+
+int power_seq_run(struct device *dev, power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code if a
+problem occured.
+
+Finally, some resources that cannot be allocated through devm need to be freed
+manually. Therefore, be sure to call power_seq_free_resources() in your device
+remove function:
+
+void power_seq_free_resources(power_seq_resources *ress);
+
+Device tree
+-----------
+All the same, power sequences can be encoded as device tree nodes. The following
+properties and nodes are equivalent to the platform data defined previously:
+
+		power-supply = <&mydevice_reg>;
+		enable-gpio = <&gpio 6 0>;
+
+		power-on-sequence {
+			regulator@0 {
+				id = "power";
+				enable;
+				post-delay = <10>;
+			};
+			gpio@1 {
+				id = "enable-gpio";
+				enable;
+			};
+		};
+
+Note that first, the phandles of the regulator and gpio used in the sequences
+are defined as properties. Then the sequence references them through the id
+property of every step. The name of sub-properties defines the type of the step.
+Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
+sequentially.
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 08b4c52..65bebfe 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -282,4 +282,8 @@ config CMA_AREAS
 
 endif
 
+config POWER_SEQ
+	bool
+	default n
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 5aa2d70..4c498c1 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
+obj-$(CONFIG_POWER_SEQ)	+= power_seq.o
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
 obj-$(CONFIG_SOC_BUS) += soc.o
diff --git a/drivers/base/power_seq.c b/drivers/base/power_seq.c
new file mode 100644
index 0000000..6ccefa1
--- /dev/null
+++ b/drivers/base/power_seq.c
@@ -0,0 +1,300 @@
+/*
+ * power_seq.c - A simple power sequence interpreter for platform devices
+ *               and device tree.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#include <linux/power_seq.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#endif
+
+static int power_seq_step_run(struct power_seq_step *step)
+{
+	int err = 0;
+
+	if (step->params.pre_delay)
+		mdelay(step->params.pre_delay);
+
+	switch (step->resource->type) {
+#ifdef CONFIG_REGULATOR
+	case POWER_SEQ_REGULATOR:
+		if (step->params.enable)
+			err = regulator_enable(step->resource->regulator);
+		else
+			err = regulator_disable(step->resource->regulator);
+		break;
+#endif
+#ifdef CONFIG_PWM
+	case POWER_SEQ_PWM:
+		if (step->params.enable)
+			err = pwm_enable(step->resource->pwm);
+		else
+			pwm_disable(step->resource->pwm);
+		break;
+#endif
+#ifdef CONFIG_GPIOLIB
+	case POWER_SEQ_GPIO:
+		gpio_set_value_cansleep(step->resource->gpio,
+					step->params.enable);
+		break;
+#endif
+	/*
+	 * should never happen unless the sequence includes a step which
+	 * type does not have support compiled in
+	 */
+	default:
+		return -EINVAL;
+	}
+
+	if (err < 0)
+		return err;
+
+	if (step->params.post_delay)
+		mdelay(step->params.post_delay);
+
+	return 0;
+}
+
+int power_seq_run(struct device *dev, power_seq *seq)
+{
+	int err;
+
+	if (!seq) return 0;
+
+	while (seq->resource) {
+		if ((err = power_seq_step_run(seq++))) {
+			dev_err(dev, "error %d while running power sequence!\n",
+				err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_seq_run);
+
+#ifdef CONFIG_OF
+static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
+				   struct platform_power_seq_step *step)
+{
+	if (of_property_read_string(node, "id", &step->id)) {
+		dev_err(dev, "missing id property!\n");
+		return -EINVAL;
+	}
+
+	if (!strcmp(node->name, "regulator")) {
+		step->type = POWER_SEQ_REGULATOR;
+#ifdef CONFIG_OF_GPIO
+	} else if (!strcmp(node->name, "gpio")) {
+		int gpio;
+
+		step->type = POWER_SEQ_GPIO;
+		gpio = of_get_named_gpio(dev->of_node, step->id, 0);
+		if (gpio < 0) {
+			dev_err(dev, "cannot resolve gpio \"%s\"\n", step->id);
+			return gpio;
+		}
+		step->gpio = gpio;
+#endif /* CONFIG_OF_GPIO */
+	} else if (!strcmp(node->name, "pwm")) {
+		step->type = POWER_SEQ_PWM;
+	} else {
+		dev_err(dev, "invalid power seq step type!\n");
+		return -EINVAL;
+	}
+
+	if (of_find_property(node, "enable", NULL)) {
+		step->params.enable = 1;
+	} else if (!of_find_property(node, "disable", NULL)) {
+		dev_err(dev, "missing enable or disable property!\n");
+		return -EINVAL;
+	}
+
+	of_property_read_u32(node, "pre-delay", &step->params.pre_delay);
+	of_property_read_u32(node, "post-delay", &step->params.post_delay);
+
+	return 0;
+}
+
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node)
+{
+	struct device_node *child = NULL;
+	platform_power_seq *ret;
+	int cpt = 0;
+	int err;
+
+	if (!node) return NULL;
+
+	while ((child = of_get_next_child(node, child)))
+		cpt++;
+
+	/* allocate one more step to signal end of sequence */
+	ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL);
+	if (!ret)
+		return ERR_PTR(-ENOMEM);
+
+	cpt = 0;
+	while ((child = of_get_next_child(node, child))) {
+		if ((err = of_parse_power_seq_step(dev, child, &ret[cpt++])))
+			return ERR_PTR(err);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_parse_power_seq);
+#endif /* CONFIG_OF */
+
+static
+struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress,
+					struct platform_power_seq_step *res)
+{
+	struct power_seq_resource *step;
+
+	list_for_each_entry(step, ress, list) {
+		if (step->type != res->type) continue;
+		switch (res->type) {
+		case POWER_SEQ_GPIO:
+			if (step->gpio = res->gpio)
+				return step;
+			break;
+		default:
+			if (!strcmp(step->id, res->id))
+				return step;
+			break;
+		}
+	}
+
+	return NULL;
+}
+
+static int power_seq_allocate_resource(struct device *dev,
+				       struct power_seq_resource *res)
+{
+	int err;
+
+	switch (res->type) {
+#ifdef CONFIG_REGULATOR
+	case POWER_SEQ_REGULATOR:
+		res->regulator = devm_regulator_get(dev, res->id);
+		if (IS_ERR(res->regulator)) {
+			dev_err(dev, "cannot get regulator \"%s\"\n", res->id);
+			return PTR_ERR(res->regulator);
+		}
+		break;
+#endif
+#ifdef CONFIG_PWM
+	case POWER_SEQ_PWM:
+		res->pwm = pwm_get(dev, res->id);
+		if (IS_ERR(res->pwm)) {
+			dev_err(dev, "cannot get pwm \"%s\"\n", res->id);
+			return PTR_ERR(res->pwm);
+		}
+		break;
+#endif
+#ifdef CONFIG_GPIOLIB
+	case POWER_SEQ_GPIO:
+		err = devm_gpio_request_one(dev, res->gpio, GPIOF_OUT_INIT_HIGH,
+					    "backlight_gpio");
+		if (err) {
+			dev_err(dev, "cannot get gpio %d\n", res->gpio);
+			return err;
+		}
+		break;
+#endif
+	default:
+		dev_err(dev, "invalid resource type %d\n", res->type);
+		return -EINVAL;
+		break;
+	}
+
+	return 0;
+}
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq)
+{
+	struct power_seq_step *seq = NULL, *ret;
+	struct power_seq_resource *res;
+	int cpt, err;
+
+	/* first pass to count the number of steps to allocate */
+	for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
+
+	if (!cpt)
+		return seq;
+
+	/* 1 more for the STOP step */
+	ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL);
+	if (!seq)
+		return ERR_PTR(-ENOMEM);
+
+	for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) {
+		/* create resource node if not referenced already */
+		if (!(res = power_seq_find_resource(ress, pseq))) {
+			res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+			if (!res)
+				return ERR_PTR(-ENOMEM);
+			res->type = pseq->type;
+
+			if (res->type = POWER_SEQ_GPIO)
+				res->gpio = pseq->gpio;
+			else
+				res->id = pseq->id;
+
+			if ((err = power_seq_allocate_resource(dev, res)) < 0)
+				return ERR_PTR(err);
+
+			list_add(&res->list, ress);
+		}
+		seq->resource = res;
+		memcpy(&seq->params, &pseq->params, sizeof(seq->params));
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(power_seq_build);
+
+void power_seq_free_resources(power_seq_resources *ress) {
+	struct power_seq_resource *res;
+
+#ifdef CONFIG_PWM
+	list_for_each_entry(res, ress, list) {
+		if (res->type = POWER_SEQ_PWM)
+			pwm_put(res->pwm);
+	}
+#endif
+}
+EXPORT_SYMBOL_GPL(power_seq_free_resources);
+
+MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
+MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..da0593a
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,139 @@
+/*
+ * power_seq.h
+ *
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties. Initially designed for use with backlight drivers.
+ *
+ * Power sequences are designed to replace the callbacks typically used in
+ * board-specific files that implement board-specific power sequences of devices
+ * such as backlights. A power sequence is an array of resources (which can a
+ * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
+ * disable) and optional pre and post step delays. By having them interpreted
+ * instead of arbitrarily executed, it is possible to describe these in the
+ * device tree and thus remove board-specific code from the kernel.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/types.h>
+
+struct device;
+struct regulator;
+struct pwm_device;
+struct device_node;
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+typedef enum {
+	POWER_SEQ_STOP = 0,
+	POWER_SEQ_REGULATOR,
+	POWER_SEQ_PWM,
+	POWER_SEQ_GPIO,
+	POWER_SEQ_MAX,
+} power_res_type;
+
+struct power_seq_resource {
+	power_res_type type;
+	/* name to resolve for resources with a name (regulator, pwm) */
+	const char *id;
+	/* resolved resource */
+	union {
+		struct regulator *regulator;
+		struct pwm_device *pwm;
+		int gpio;
+	};
+	/* used to maintain the list of resources used by the driver */
+	struct list_head list;
+};
+typedef struct list_head power_seq_resources;
+
+struct power_step_params {
+	/* enable the resource if 1, disable if 0 */
+	bool enable;
+	/* delay (in ms) to wait before executing the step */
+	int  pre_delay;
+	/* delay (in ms) to wait after executing the step */
+	int post_delay;
+};
+
+/**
+ * Platform definition of power sequences. A sequence is an array of these,
+ * terminated by a STOP instance.
+ */
+struct platform_power_seq_step {
+	power_res_type type;
+	union {
+		/* Used by REGULATOR and PWM types to name the resource */
+		const char *id;
+		/* Used by GPIO */
+		int gpio;
+	};
+	struct power_step_params params;
+};
+typedef struct platform_power_seq_step platform_power_seq;
+
+/**
+ * Power sequence steps resolved against their resource. Built by
+ * power_seq_build and used to run the sequence.
+ */
+struct power_seq_step {
+	struct power_seq_resource *resource;
+	struct power_step_params params;
+};
+typedef struct power_seq_step power_seq;
+
+#ifdef CONFIG_OF
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * sequence is allocated using devm_kzalloc on dev.
+ */
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node);
+#else
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node)
+{
+	return NULL;
+}
+#endif
+
+/**
+ * Build a runnable power sequence from platform data, and add the resources
+ * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
+ * on dev.
+ */
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq);
+
+/**
+ * Free all the resources previously allocated by power_seq_allocate_resources.
+ */
+void power_seq_free_resources(power_seq_resources *ress);
+
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(struct device *dev, power_seq *seq);
+
+#endif
-- 
1.7.11.3

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Greg Kroah-Hartman]

On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
> 
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  Documentation/power/power_seq.txt | 120 +++++++++++++++
>  drivers/base/Kconfig              |   4 +
>  drivers/base/Makefile             |   1 +
>  drivers/base/power_seq.c          | 300 ++++++++++++++++++++++++++++++++++++++
>  include/linux/power_seq.h         | 139 ++++++++++++++++++

What's wrong with drivers/power/?  I sure don't want to maintain this
code, and it seems to not be part of the "driver core" infrastructure.

thanks,

greg k-h

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/28/2012 03:19 AM, Greg Kroah-Hartman wrote:
> On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote:
>> Some device drivers (panel backlights especially) need to follow precise
>> sequences for powering on and off, involving gpios, regulators, PWMs
>> with a precise powering order and delays to respect between each steps.
>> These sequences are board-specific, and do not belong to a particular
>> driver - therefore they have been performed by board-specific hook
>> functions to far.
>>
>> With the advent of the device tree and of ARM kernels that are not
>> board-tied, we cannot rely on these board-specific hooks anymore but
>> need a way to implement these sequences in a portable manner. This patch
>> introduces a simple interpreter that can execute such power sequences
>> encoded either as platform data or within the device tree.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>   Documentation/power/power_seq.txt | 120 +++++++++++++++
>>   drivers/base/Kconfig              |   4 +
>>   drivers/base/Makefile             |   1 +
>>   drivers/base/power_seq.c          | 300 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/power_seq.h         | 139 ++++++++++++++++++
>
> What's wrong with drivers/power/?  I sure don't want to maintain this
> code, and it seems to not be part of the "driver core" infrastructure.

I thought about drivers/power/ initially, but quickly realized it was 
only about the power supply class and its drivers - so I felt like it 
would be out of place there, as the power sequences have nothing to do 
with power supply but instead control gpios, regulators and pwms.

On the other hand I have just noticed that the apparently unrelated 
Adaptive Voltage Scaling driver just appeared in drivers/power/avs. So 
if Anton and David are ok with this, maybe I could put the power 
sequences code in its own subdirectory within drivers/power.

Thanks,
Alex.

Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runtime

[Anton Vorontsov]

On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote:
[...]
> On the other hand I have just noticed that the apparently unrelated
> Adaptive Voltage Scaling driver just appeared in drivers/power/avs.
> So if Anton and David are ok with this, maybe I could put the power
> sequences code in its own subdirectory within drivers/power.

Well, currently drivers/power/ is indeed just for power supply class
subsystem and drivers. But if the trend is to gather power management
("policy") stuff under one directory, i.e.

drivers/
  power/
    supplies/    <- former "power supply class and drivers"
    regulators/
    idle/
    cpuidle/
    cpufreq/
    devfreq/
    avs/
    ...

That would probably make sense, we could easily see the big picture.
But if we're not going to do this long-term, I would suggest to stick
to just a new directory under drivers (and move drivers/power/avs/ to
drivers/avs).

Cc'ing some more people...

Thanks,

p.s. Jean, why am I the last person who discovers drivers/power/avs/?
Would be nice to Cc me on such patches; by moving AVS under
drivers/power/ you effectively nominated me as its maintainer. :-)

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runt

[함명주]

PiBPbiBNb24sIEp1bCAzMCwgMjAxMiBhdCAxMDo1MTo0MkFNICswOTAwLCBBbGV4IENvdXJib3Qg
d3JvdGU6DQo+IFsuLi5dDQo+ID4gT24gdGhlIG90aGVyIGhhbmQgSSBoYXZlIGp1c3Qgbm90aWNl
ZCB0aGF0IHRoZSBhcHBhcmVudGx5IHVucmVsYXRlZA0KPiA+IEFkYXB0aXZlIFZvbHRhZ2UgU2Nh
bGluZyBkcml2ZXIganVzdCBhcHBlYXJlZCBpbiBkcml2ZXJzL3Bvd2VyL2F2cy4NCj4gPiBTbyBp
ZiBBbnRvbiBhbmQgRGF2aWQgYXJlIG9rIHdpdGggdGhpcywgbWF5YmUgSSBjb3VsZCBwdXQgdGhl
IHBvd2VyDQo+ID4gc2VxdWVuY2VzIGNvZGUgaW4gaXRzIG93biBzdWJkaXJlY3Rvcnkgd2l0aGlu
IGRyaXZlcnMvcG93ZXIuDQo+IA0KPiBXZWxsLCBjdXJyZW50bHkgZHJpdmVycy9wb3dlci8gaXMg
aW5kZWVkIGp1c3QgZm9yIHBvd2VyIHN1cHBseSBjbGFzcw0KPiBzdWJzeXN0ZW0gYW5kIGRyaXZl
cnMuIEJ1dCBpZiB0aGUgdHJlbmQgaXMgdG8gZ2F0aGVyIHBvd2VyIG1hbmFnZW1lbnQNCj4gKCJw
b2xpY3kiKSBzdHVmZiB1bmRlciBvbmUgZGlyZWN0b3J5LCBpLmUuDQo+IA0KPiBkcml2ZXJzLw0K
PiAgIHBvd2VyLw0KPiAgICAgc3VwcGxpZXMvICAgIDwtIGZvcm1lciAicG93ZXIgc3VwcGx5IGNs
YXNzIGFuZCBkcml2ZXJzIg0KPiAgICAgcmVndWxhdG9ycy8NCj4gICAgIGlkbGUvDQo+ICAgICBj
cHVpZGxlLw0KPiAgICAgY3B1ZnJlcS8NCj4gICAgIGRldmZyZXEvDQo+ICAgICBhdnMvDQo+ICAg
ICAuLi4NCj4gDQo+IFRoYXQgd291bGQgcHJvYmFibHkgbWFrZSBzZW5zZSwgd2UgY291bGQgZWFz
aWx5IHNlZSB0aGUgYmlnIHBpY3R1cmUuDQo+IEJ1dCBpZiB3ZSdyZSBub3QgZ29pbmcgdG8gZG8g
dGhpcyBsb25nLXRlcm0sIEkgd291bGQgc3VnZ2VzdCB0byBzdGljaw0KPiB0byBqdXN0IGEgbmV3
IGRpcmVjdG9yeSB1bmRlciBkcml2ZXJzIChhbmQgbW92ZSBkcml2ZXJzL3Bvd2VyL2F2cy8gdG8N
Cj4gZHJpdmVycy9hdnMpLg0KPiANCj4gQ2MnaW5nIHNvbWUgbW9yZSBwZW9wbGUuLi4NCj4gDQo+
IFRoYW5rcywNCj4gDQo+IHAucy4gSmVhbiwgd2h5IGFtIEkgdGhlIGxhc3QgcGVyc29uIHdobyBk
aXNjb3ZlcnMgZHJpdmVycy9wb3dlci9hdnMvPw0KPiBXb3VsZCBiZSBuaWNlIHRvIENjIG1lIG9u
IHN1Y2ggcGF0Y2hlczsgYnkgbW92aW5nIEFWUyB1bmRlcg0KPiBkcml2ZXJzL3Bvd2VyLyB5b3Ug
ZWZmZWN0aXZlbHkgbm9taW5hdGVkIG1lIGFzIGl0cyBtYWludGFpbmVyLiA6LSkNCj4gDQo+IC0t
IA0KPiBBbnRvbiBWb3JvbnRzb3YNCj4gRW1haWw6IGNib3VhdG1haWxydUBnbWFpbC5jb20NCj4g
DQoNCkhhdmluZyBwb3dlci1zdXBwbGllcyBhdCAvZHJpdmVycy9wb3dlci8gYW5kIG90aGVyIHBv
d2VyLXJlbGF0ZWQgZHJpdmVycyBzcHJlYWQgYXQgL2RyaXZlcnMvIGhhdmUgYmVlbiBidWdnaW5n
IG1lIGFzIHdlbGwuIEknZCBsaWtlIHRvIHNlZSB0aGUgY2hhbmdlIHlvdSd2ZSBzdWdnZXN0ZWQg
dGhvdWdoIEknbSBub3Qgc3VyZSBob3cgc2lnbmlmaWNhbnQgdGhlIHNpZGUtZWZmZWN0IHdpbGwg
YmUgYXQgdGhpcyBwb2ludC4NCg0KR2VuZXJhbGx5IHNwZWFraW5nLCB5ZXMsIEkgYWxzbyB0aGlu
ayB0aGUgcHJvcG9zYWwgaXMgbW9yZSByZWFzb25hYmxlIHRoYW4gdGhlIGN1cnJlbnQgc3RydWN0
dXJlLg0KDQoNCg0KQ2hlZXJzIQ0KTXl1bmdKb28NCg0KDQo

Re: Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runt

[Rafael J. Wysocki]

On Monday, July 30, 2012, Anton Vorontsov wrote:
> On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote:
> [...]
> > On the other hand I have just noticed that the apparently unrelated
> > Adaptive Voltage Scaling driver just appeared in drivers/power/avs.
> > So if Anton and David are ok with this, maybe I could put the power
> > sequences code in its own subdirectory within drivers/power.
> 
> Well, currently drivers/power/ is indeed just for power supply class
> subsystem and drivers. But if the trend is to gather power management
> ("policy") stuff under one directory, i.e.
> 
> drivers/
>   power/
>     supplies/    <- former "power supply class and drivers"
>     regulators/
>     idle/
>     cpuidle/
>     cpufreq/
>     devfreq/
>     avs/
>     ...
> 
> That would probably make sense, we could easily see the big picture.
> But if we're not going to do this long-term,

Yes, we may do this eventually, but surely not right now.

Thanks,
Rafael

Re: Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runt

[Anton Vorontsov]

On Mon, Jul 30, 2012 at 10:59:39PM +0200, Rafael J. Wysocki wrote:
[...]
> > Well, currently drivers/power/ is indeed just for power supply class
> > subsystem and drivers. But if the trend is to gather power management
> > ("policy") stuff under one directory, i.e.
> > 
> > drivers/
> >   power/
> >     supplies/    <- former "power supply class and drivers"
> >     regulators/
> >     idle/
> >     cpuidle/
> >     cpufreq/
> >     devfreq/
> >     avs/
> >     ...
> > 
> > That would probably make sense, we could easily see the big picture.
> > But if we're not going to do this long-term,
> 
> Yes, we may do this eventually, but surely not right now.

OK, great! Then I'll probably start thinking about moving power supply
drivers into its own directory, maybe in 3.7.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runt

[Pihet-XID, Jean]

Hi Anton,

Sorry for the late reply. I was away and back now.

On Mon, Jul 30, 2012 at 4:40 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote:
> [...]
>> On the other hand I have just noticed that the apparently unrelated
>> Adaptive Voltage Scaling driver just appeared in drivers/power/avs.
>> So if Anton and David are ok with this, maybe I could put the power
>> sequences code in its own subdirectory within drivers/power.
>
> Well, currently drivers/power/ is indeed just for power supply class
> subsystem and drivers. But if the trend is to gather power management
> ("policy") stuff under one directory, i.e.
>
> drivers/
>   power/
>     supplies/    <- former "power supply class and drivers"
>     regulators/
>     idle/
>     cpuidle/
>     cpufreq/
>     devfreq/
>     avs/
>     ...
>
> That would probably make sense, we could easily see the big picture.
> But if we're not going to do this long-term, I would suggest to stick
> to just a new directory under drivers (and move drivers/power/avs/ to
> drivers/avs).
>
> Cc'ing some more people...
>
> Thanks,
>
> p.s. Jean, why am I the last person who discovers drivers/power/avs/?
> Would be nice to Cc me on such patches; by moving AVS under
> drivers/power/ you effectively nominated me as its maintainer. :-)
Oops, I am really sorry about that ;-( . I wrongly assumed Rafael and
Greg KH were the contact persons for drivers/power and so I contacted
them before moving the code.

Thanks for letting me know. Are you ok with the changes? Let me know
if some more changes are needed.

Regards,
Jean

>
> --
> Anton Vorontsov
> Email: cbouatmailru@gmail.com

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Greg Kroah-Hartman]

On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote:
> +++ b/include/linux/power_seq.h
> @@ -0,0 +1,139 @@
> +/*
> + * power_seq.h
> + *
> + * Simple interpreter for defining power sequences as platform data or device
> + * tree properties. Initially designed for use with backlight drivers.
> + *
> + * Power sequences are designed to replace the callbacks typically used in
> + * board-specific files that implement board-specific power sequences of devices
> + * such as backlights. A power sequence is an array of resources (which can a
> + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
> + * disable) and optional pre and post step delays. By having them interpreted
> + * instead of arbitrarily executed, it is possible to describe these in the
> + * device tree and thus remove board-specific code from the kernel.
> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.

As I always say:
	Unless you want to track the office movements of the FSF for the
	next 40 years, and keep this file up to date, drop that last
	paragraph, it's pointless.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Simon Glass]

Hi,

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

This all looks very reasonable to me, just a few comments.

>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  Documentation/power/power_seq.txt | 120 +++++++++++++++
>  drivers/base/Kconfig              |   4 +
>  drivers/base/Makefile             |   1 +
>  drivers/base/power_seq.c          | 300 ++++++++++++++++++++++++++++++++++++++
>  include/linux/power_seq.h         | 139 ++++++++++++++++++
>  5 files changed, 564 insertions(+)
>  create mode 100644 Documentation/power/power_seq.txt
>  create mode 100644 drivers/base/power_seq.c
>  create mode 100644 include/linux/power_seq.h
>
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> new file mode 100644
> index 0000000..aa2ceb5
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,120 @@
> +Runtime Interpreted Power Sequences
> +-----------------------------------
> +
> +Problem
> +-------
> +One very common board-dependent code is the out-of-driver code that is used to
> +turn a device on or off. For instance, SoC boards very commonly use a GPIO
> +(abstracted to a regulator or not) to control the power supply of a backlight,
> +disabling it when the backlight is not used in order to save power. The GPIO
> +that should be used, however, as well as the exact power sequence that may
> +involve different resources, is board-dependent and thus unknown of the driver.
> +
> +This has been addressed so far by using hooks in the device's platform data that
> +are called whenever the state of the device might reflect a power change. This
> +approach, however, introduces board-dependant code into the kernel and is not
> +compatible with the device tree.
> +
> +The Runtime Interpreted Power Sequences (or power sequences for short) aim at
> +turning this code into platform data or device tree nodes. Power sequences are
> +described using a simple format and run by a simple interpreter whenever needed.
> +This allows to remove the callback mechanism and makes the kernel less
> +board-dependant.
> +
> +Sequences Format
> +----------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:
> +- GPIOs
> +- Regulators
> +- PWMs
> +
> +Each step designates a resource and the following parameters:
> +- Whether the step should enable or disable the resource,
> +- Delay to wait before performing the action,
> +- Delay to wait after performing the action.
> +
> +Both new resources and parameters can be introduced, but the goal is of course
> +to keep things as simple and compact as possible.
> +
> +The platform data is a simple array of platform_power_seq_step instances, each
> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
> +identified by number. For example, the following sequence will turn on the
> +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:

For the delay, I think milliseconds is reasonable. I suppose there is
no reasonable need for microseconds?

> +
> +struct platform_power_seq_step power_on_seq[] = {
> +       {
> +               .type = POWER_SEQ_REGULATOR,
> +               .id = "power",
> +               .params = {
> +                       .enable = 1,
> +                       .post_delay = 10,
> +               },
> +       },
> +       {
> +               .type = POWER_SEQ_GPIO,
> +               .gpio = 110,
> +               .params = {
> +                       .enable = 1,
> +               },
> +       },
> +       {
> +               .type = POWER_SEQ_STOP,
> +       },
> +};
> +
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function takes care of resolving the resources as
> +they are met in the sequence and to allocate them if needed:
> +
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> +                          platform_power_seq *pseq);
> +
> +You will need an instance of power_seq_resources to keep track of the resources
> +that are already allocated. On success, the function returns a devm allocated
> +resolved sequence that is ready to be passed to power_seq_run(). In case of
> +failure, and error code is returned.
> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> +int power_seq_run(struct device *dev, power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
> +
> +Finally, some resources that cannot be allocated through devm need to be freed
> +manually. Therefore, be sure to call power_seq_free_resources() in your device
> +remove function:
> +
> +void power_seq_free_resources(power_seq_resources *ress);
> +
> +Device tree
> +-----------
> +All the same, power sequences can be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined previously:
> +
> +               power-supply = <&mydevice_reg>;
> +               enable-gpio = <&gpio 6 0>;
> +
> +               power-on-sequence {
> +                       regulator@0 {
> +                               id = "power";

Is there a reason not to put the phandle here, like:

                                   id = <&mydevice_reg>;

(or maybe 'device' instead of id?)

> +                               enable;
> +                               post-delay = <10>;
> +                       };
> +                       gpio@1 {
> +                               id = "enable-gpio";
> +                               enable;
> +                       };
> +               };
> +
> +Note that first, the phandles of the regulator and gpio used in the sequences
> +are defined as properties. Then the sequence references them through the id
> +property of every step. The name of sub-properties defines the type of the step.
> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
> +sequentially.

For the regulator and gpio types I think you only have an enable. For
the pwm, what is the intended binding? Is that documented elsewhere?

Also it might be worth mentioning how you get a struct power_seq from
an fdt node, and perhaps given an example of a device which has an
attached node, so we can see how it is referenced from the device
(of_parse_power_seq I think). Do put the sequence inside the device
node or reference it with a phandle?

Finally, should you use typedefs?

Regards,
Simon

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

Hi Simon,

On 07/30/2012 08:00 PM, Simon Glass wrote:
> For the delay, I think milliseconds is reasonable. I suppose there is
> no reasonable need for microseconds?

I don't see any need for microseconds myself - anybody sees use for 
finer-grained delays?

Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.

>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> +               power-supply = <&mydevice_reg>;
>> +               enable-gpio = <&gpio 6 0>;
>> +
>> +               power-on-sequence {
>> +                       regulator@0 {
>> +                               id = "power";
>
> Is there a reason not to put the phandle here, like:
>
>                                     id = <&mydevice_reg>;
>
> (or maybe 'device' instead of id?)

There is one reason, but it might be a bad one. On Tegra, PWM phandle 
uses an extra cell to encode the duty-cycle the PWM should have when we 
call get_pwm(). This makes it possible to address the same PWM with 
different phandles (and different duty cycles), which causes an issue to 
know whether a PWM is already used in a sequence (potentially resulting 
in multiple get_pwm calls on the same PWM, and also opens the door to 
ambiguities in behavior (what is the correct duty-cycle to use if 
several different values are passed?)

Maybe the problem lies in how PWM phandles are handled - if duty-cycle 
was not part of the information, we would not have this problem.

>> +Note that first, the phandles of the regulator and gpio used in the sequences
>> +are defined as properties. Then the sequence references them through the id
>> +property of every step. The name of sub-properties defines the type of the step.
>> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
>> +sequentially.
>
> For the regulator and gpio types I think you only have an enable. For
> the pwm, what is the intended binding? Is that documented elsewhere?

Same thing, enable/disable which would call pwm_enable and pwm_disable. 
One could also image an additional property to set the duty cycle if it 
can be taken off the phandle.

> Also it might be worth mentioning how you get a struct power_seq from
> an fdt node, and perhaps given an example of a device which has an
> attached node, so we can see how it is referenced from the device
> (of_parse_power_seq I think). Do put the sequence inside the device
> node or reference it with a phandle?

Yes, this definitely needs more documentation - especially the DT part.

Thanks,
Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 05:37:07PM +0900, Alex Courbot wrote:
> Hi Simon,
> 
> On 07/30/2012 08:00 PM, Simon Glass wrote:
> >For the delay, I think milliseconds is reasonable. I suppose there is
> >no reasonable need for microseconds?
> 
> I don't see any need for microseconds myself - anybody sees use for
> finer-grained delays?
> 
> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.

You might want to take a look at Documentation/timers/timers-howto.txt.
msleep() isn't very accurate for periods shorter than 20 ms.

> >>+Device tree
> >>+-----------
> >>+All the same, power sequences can be encoded as device tree nodes. The following
> >>+properties and nodes are equivalent to the platform data defined previously:
> >>+
> >>+               power-supply = <&mydevice_reg>;
> >>+               enable-gpio = <&gpio 6 0>;
> >>+
> >>+               power-on-sequence {
> >>+                       regulator@0 {
> >>+                               id = "power";
> >
> >Is there a reason not to put the phandle here, like:
> >
> >                                    id = <&mydevice_reg>;
> >
> >(or maybe 'device' instead of id?)
> 
> There is one reason, but it might be a bad one. On Tegra, PWM
> phandle uses an extra cell to encode the duty-cycle the PWM should
> have when we call get_pwm().

This is not only the case on Tegra, but it is the default unless a
driver specifically overrides it. The second cell specifies the index of
the PWM device within the PWM chip. The third cell doesn't specify the
duty cycle but the period of the PWM.

> This makes it possible to address the
> same PWM with different phandles (and different duty cycles),

How so? A phandle will always refer to a PWM chip. Paired with the
second cell, of_pwm_request() will resolve that into the correct PWM
device.

> which
> causes an issue to know whether a PWM is already used in a sequence
> (potentially resulting in multiple get_pwm calls on the same PWM,
> and also opens the door to ambiguities in behavior (what is the
> correct duty-cycle to use if several different values are passed?)

You can't request a PWM multiple times. The second call well return
-EBUSY, or rather ERR_PTR(-EBUSY).

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 06:13 PM, Thierry Reding wrote:
>> I don't see any need for microseconds myself - anybody sees use for
>> finer-grained delays?
>>
>> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
>
> You might want to take a look at Documentation/timers/timers-howto.txt.
> msleep() isn't very accurate for periods shorter than 20 ms.

Ok, looks like usleep_range is the way to go here. In that case it would 
probably not hurt to specify delays in microseconds in the DT and 
platform data as well.

>>>> +Device tree
>>>> +-----------
>>>> +All the same, power sequences can be encoded as device tree nodes. The following
>>>> +properties and nodes are equivalent to the platform data defined previously:
>>>> +
>>>> +               power-supply = <&mydevice_reg>;
>>>> +               enable-gpio = <&gpio 6 0>;
>>>> +
>>>> +               power-on-sequence {
>>>> +                       regulator@0 {
>>>> +                               id = "power";
>>>
>>> Is there a reason not to put the phandle here, like:
>>>
>>>                                     id = <&mydevice_reg>;
>>>
>>> (or maybe 'device' instead of id?)
>>
>> There is one reason, but it might be a bad one. On Tegra, PWM
>> phandle uses an extra cell to encode the duty-cycle the PWM should
>> have when we call get_pwm().
>
> This is not only the case on Tegra, but it is the default unless a
> driver specifically overrides it. The second cell specifies the index of
> the PWM device within the PWM chip.  The third cell doesn't specify the
> duty cycle but the period of the PWM.

Then I think there is a mistake in 
Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt:

"the second cell is the duty cycle in nanoseconds."

>> This makes it possible to address the
>> same PWM with different phandles (and different duty cycles),
>
> How so? A phandle will always refer to a PWM chip. Paired with the
> second cell, of_pwm_request() will resolve that into the correct PWM
> device.

For tegra, we can only address PWMs this way IIRC:

pwm = <&pwm 2 5000000>;

If we had <&pwm 2>, I agree that there would be no problem. But here the 
period of the PWM is also given - and in practice, we can request the 
same PWM using different phandles. For instance, if the above property 
was part of the power-on sequence, and the following

pwm = <&pwm 2 0>;

was part of power-off, how can I know that these two different phandles 
refer to the same PWM without calling pwm_get a second time and getting 
-EBUSY?

Of course if the same period is specified for both, I will not have this 
issue as the phandles will be identical, but the possibility remains 
open that we are given a faulty tree here.

More generally speaking, wouldn't it make more sense to have the 
period/duty cycle of a PWM encoded into separate properties when needed 
and have the phandle just reference the PWM instance? This also seems to 
stand from an API point of view, since the period is not specified when 
invoking pwm_request or pwm_get, but set by its own pwm_set_period function?

On an unrelated note, I also don't understand why the period is also a 
parameter of pwm_config and why pwm_set_period does not do anything 
beyond setting a struct member that is returned by pwm_get_period (but 
maybe I am missing something here).

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 07:11:41PM +0900, Alex Courbot wrote:
> On 07/31/2012 06:13 PM, Thierry Reding wrote:
> >>I don't see any need for microseconds myself - anybody sees use for
> >>finer-grained delays?
> >>
> >>Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
> >
> >You might want to take a look at Documentation/timers/timers-howto.txt.
> >msleep() isn't very accurate for periods shorter than 20 ms.
> 
> Ok, looks like usleep_range is the way to go here. In that case it
> would probably not hurt to specify delays in microseconds in the DT
> and platform data as well.


> 
> >>>>+Device tree
> >>>>+-----------
> >>>>+All the same, power sequences can be encoded as device tree nodes. The following
> >>>>+properties and nodes are equivalent to the platform data defined previously:
> >>>>+
> >>>>+               power-supply = <&mydevice_reg>;
> >>>>+               enable-gpio = <&gpio 6 0>;
> >>>>+
> >>>>+               power-on-sequence {
> >>>>+                       regulator@0 {
> >>>>+                               id = "power";
> >>>
> >>>Is there a reason not to put the phandle here, like:
> >>>
> >>>                                    id = <&mydevice_reg>;
> >>>
> >>>(or maybe 'device' instead of id?)
> >>
> >>There is one reason, but it might be a bad one. On Tegra, PWM
> >>phandle uses an extra cell to encode the duty-cycle the PWM should
> >>have when we call get_pwm().
> >
> >This is not only the case on Tegra, but it is the default unless a
> >driver specifically overrides it. The second cell specifies the index of
> >the PWM device within the PWM chip.  The third cell doesn't specify the
> >duty cycle but the period of the PWM.
> 
> Then I think there is a mistake in
> Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt:
> 
> "the second cell is the duty cycle in nanoseconds."

Yes, that's a mistake. =\

> >>This makes it possible to address the
> >>same PWM with different phandles (and different duty cycles),
> >
> >How so? A phandle will always refer to a PWM chip. Paired with the
> >second cell, of_pwm_request() will resolve that into the correct PWM
> >device.
> 
> For tegra, we can only address PWMs this way IIRC:
> 
> pwm = <&pwm 2 5000000>;
> 
> If we had <&pwm 2>, I agree that there would be no problem. But here
> the period of the PWM is also given - and in practice, we can
> request the same PWM using different phandles. For instance, if the
> above property was part of the power-on sequence, and the following
> 
> pwm = <&pwm 2 0>;
> 
> was part of power-off, how can I know that these two different
> phandles refer to the same PWM without calling pwm_get a second time
> and getting -EBUSY?

You should specify the same period regardless of the sequence. But you
are right, you still cannot request the device twice.

> Of course if the same period is specified for both, I will not have
> this issue as the phandles will be identical, but the possibility
> remains open that we are given a faulty tree here.

I think the phandle is in fact only the reference to the PWM chip, that
is: &pwm. The second cell, the PWM index, is part of the PWM specifier.

However the issue doesn't go away if you drop the period cell because
you still won't be able to request the PWM device a second time. How is
this solved for regulators and GPIOs? At least for GPIOs I'm pretty sure
that you can't request them more than once either.

> More generally speaking, wouldn't it make more sense to have the
> period/duty cycle of a PWM encoded into separate properties when
> needed and have the phandle just reference the PWM instance? This
> also seems to stand from an API point of view, since the period is
> not specified when invoking pwm_request or pwm_get, but set by its
> own pwm_set_period function?

The problem with specifying the period in a separate property is how to
map them to the correct PWM device. From a hardware description point of
view, making the period part of the specifier makes a lot of sense and
hardware description is what DT is about.

> On an unrelated note, I also don't understand why the period is also
> a parameter of pwm_config and why pwm_set_period does not do
> anything beyond setting a struct member that is returned by
> pwm_get_period (but maybe I am missing something here).

pwm_config() is the legacy API that we need to support for compatibility
reasons. Eventually this interface should probably be changed.
pwm_get_period() and pwm_set_period() were merely introduced to support
DT, but I could imagine them becoming the canonical way for configuring
PWM devices in the future, perhaps with a complementary
pwm_set_duty_cycle().

But first we need to convert drivers and users.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 05:37:07PM +0900, Alex Courbot wrote:
> On 07/30/2012 08:00 PM, Simon Glass wrote:

> >For the delay, I think milliseconds is reasonable. I suppose there is
> >no reasonable need for microseconds?

> I don't see any need for microseconds myself - anybody sees use for
> finer-grained delays?

Bouncing reset lines, often the time required for reset is down in the
microseconds.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
> 
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  Documentation/power/power_seq.txt | 120 +++++++++++++++
>  drivers/base/Kconfig              |   4 +
>  drivers/base/Makefile             |   1 +
>  drivers/base/power_seq.c          | 300 ++++++++++++++++++++++++++++++++++++++
>  include/linux/power_seq.h         | 139 ++++++++++++++++++
>  5 files changed, 564 insertions(+)
>  create mode 100644 Documentation/power/power_seq.txt
>  create mode 100644 drivers/base/power_seq.c
>  create mode 100644 include/linux/power_seq.h
> 
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> new file mode 100644
> index 0000000..aa2ceb5
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,120 @@
> +Runtime Interpreted Power Sequences
> +-----------------------------------
> +
> +Problem
> +-------
> +One very common board-dependent code is the out-of-driver code that is used to
> +turn a device on or off. For instance, SoC boards very commonly use a GPIO
> +(abstracted to a regulator or not) to control the power supply of a backlight,
> +disabling it when the backlight is not used in order to save power. The GPIO
> +that should be used, however, as well as the exact power sequence that may
> +involve different resources, is board-dependent and thus unknown of the driver.
> +
> +This has been addressed so far by using hooks in the device's platform data that
> +are called whenever the state of the device might reflect a power change. This
> +approach, however, introduces board-dependant code into the kernel and is not
> +compatible with the device tree.
> +
> +The Runtime Interpreted Power Sequences (or power sequences for short) aim at
> +turning this code into platform data or device tree nodes. Power sequences are
> +described using a simple format and run by a simple interpreter whenever needed.
> +This allows to remove the callback mechanism and makes the kernel less
> +board-dependant.
> +
> +Sequences Format
> +----------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:
> +- GPIOs
> +- Regulators
> +- PWMs
> +
> +Each step designates a resource and the following parameters:
> +- Whether the step should enable or disable the resource,
> +- Delay to wait before performing the action,
> +- Delay to wait after performing the action.
> +
> +Both new resources and parameters can be introduced, but the goal is of course
> +to keep things as simple and compact as possible.
> +
> +The platform data is a simple array of platform_power_seq_step instances, each
> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
> +identified by number. For example, the following sequence will turn on the
> +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
> +
> +struct platform_power_seq_step power_on_seq[] = {
> +	{
> +		.type = POWER_SEQ_REGULATOR,
> +		.id = "power",
> +		.params = {
> +			.enable = 1,
> +			.post_delay = 10,
> +		},
> +	},
> +	{
> +		.type = POWER_SEQ_GPIO,
> +		.gpio = 110,
> +		.params = {
> +			.enable = 1,
> +		},
> +	},
> +	{
> +		.type = POWER_SEQ_STOP,
> +	},
> +};
> +
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function takes care of resolving the resources as
> +they are met in the sequence and to allocate them if needed:
> +
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> +			   platform_power_seq *pseq);
> +
> +You will need an instance of power_seq_resources to keep track of the resources
> +that are already allocated. On success, the function returns a devm allocated
> +resolved sequence that is ready to be passed to power_seq_run(). In case of
> +failure, and error code is returned.

I don't quite understand why the struct power_seq_resources is needed.
Can this not be stored within power_seq?

> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> +int power_seq_run(struct device *dev, power_seq *seq);

Why is the struct device required here? It already is passed during the
call to pwm_seq_build(), so perhaps you should keep a reference to it
within struct power_seq?

> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
> +
> +Finally, some resources that cannot be allocated through devm need to be freed
> +manually. Therefore, be sure to call power_seq_free_resources() in your device
> +remove function:
> +
> +void power_seq_free_resources(power_seq_resources *ress);

Could this not also be handled by a managed version? If a power_seq is
always managed, then I would assume that it also takes care of freeing
the resources, even if the resources have no managed equivalents.

Perhaps it would also make sense to provide non-managed version of these
functions. I think that would make the managed versions easier and more
canonical to implement.

> +
> +Device tree
> +-----------
> +All the same, power sequences can be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined previously:
> +
> +		power-supply = <&mydevice_reg>;
> +		enable-gpio = <&gpio 6 0>;
> +
> +		power-on-sequence {
> +			regulator@0 {
> +				id = "power";
> +				enable;
> +				post-delay = <10>;
> +			};
> +			gpio@1 {
> +				id = "enable-gpio";
> +				enable;
> +			};
> +		};
> +
> +Note that first, the phandles of the regulator and gpio used in the sequences
> +are defined as properties. Then the sequence references them through the id
> +property of every step. The name of sub-properties defines the type of the step.
> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
> +sequentially.

I think there has been quite some discussion regarding the naming of
subnodes and the conclusion seems to have been to name them uniformly
after what they represent. As such the power-on-sequence subnodes should
be called step@0, step@1, etc. However, that will require the addition
of a property to define the type of resource.

Also, is there some way we can make the id property for GPIOs not
require the -gpio suffix? If the resource type is already GPIO, then it
seems redundant to add -gpio to the ID.

> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 08b4c52..65bebfe 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -282,4 +282,8 @@ config CMA_AREAS
>  
>  endif
>  
> +config POWER_SEQ
> +	bool
> +	default n
> +

"default n" is already the default, so you can drop that line.

>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 5aa2d70..4c498c1 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
>  ifeq ($(CONFIG_SYSFS),y)
>  obj-$(CONFIG_MODULES)	+= module.o
>  endif
> +obj-$(CONFIG_POWER_SEQ)	+= power_seq.o
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
>  obj-$(CONFIG_REGMAP)	+= regmap/
>  obj-$(CONFIG_SOC_BUS) += soc.o
> diff --git a/drivers/base/power_seq.c b/drivers/base/power_seq.c
> new file mode 100644
> index 0000000..6ccefa1
> --- /dev/null
> +++ b/drivers/base/power_seq.c
> @@ -0,0 +1,300 @@
> +/*
> + * power_seq.c - A simple power sequence interpreter for platform devices
> + *               and device tree.
> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#include <linux/power_seq.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/pwm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#endif

I think you don't need the CONFIG_OF guard around these. Both of.h and
of_gpio.h can be included unconditionally and actually contain dummy
definitions for the public functions in the !OF case.

> +
> +static int power_seq_step_run(struct power_seq_step *step)
> +{
> +	int err = 0;
> +
> +	if (step->params.pre_delay)
> +		mdelay(step->params.pre_delay);
> +
> +	switch (step->resource->type) {
> +#ifdef CONFIG_REGULATOR
> +	case POWER_SEQ_REGULATOR:
> +		if (step->params.enable)
> +			err = regulator_enable(step->resource->regulator);
> +		else
> +			err = regulator_disable(step->resource->regulator);
> +		break;
> +#endif
> +#ifdef CONFIG_PWM
> +	case POWER_SEQ_PWM:
> +		if (step->params.enable)
> +			err = pwm_enable(step->resource->pwm);
> +		else
> +			pwm_disable(step->resource->pwm);
> +		break;
> +#endif
> +#ifdef CONFIG_GPIOLIB
> +	case POWER_SEQ_GPIO:
> +		gpio_set_value_cansleep(step->resource->gpio,
> +					step->params.enable);
> +		break;
> +#endif

This kind of #ifdef'ery is quite ugly. I don't know if adding separate
*_run() functions for each type of resource would be any better, though.
Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
GPIOLIB symbols to side-step the issue completely?

> +	/*
> +	 * should never happen unless the sequence includes a step which
> +	 * type does not have support compiled in
> +	 */
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (err < 0)
> +		return err;
> +
> +	if (step->params.post_delay)
> +		mdelay(step->params.post_delay);
> +
> +	return 0;
> +}
> +
> +int power_seq_run(struct device *dev, power_seq *seq)
> +{
> +	int err;
> +
> +	if (!seq) return 0;

I don't think this is acceptable according to the coding style. Also,
perhaps returning -EINVAL would be more meaningful?

> +
> +	while (seq->resource) {

Perhaps this should check for POWER_SEQ_STOP instead?

> +		if ((err = power_seq_step_run(seq++))) {
> +			dev_err(dev, "error %d while running power sequence!\n",
> +				err);

For this kind of diagnostics it could be useful to have a name
associated with the power sequence. But I'm not sure that making the
power sequence code output an error here is the best solution. I find it
to be annoying when core code starts outputting too many error codes. In
this case it's particularily easy to catch the errors in the caller.

> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_run);
> +
> +#ifdef CONFIG_OF
> +static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
> +				   struct platform_power_seq_step *step)
> +{
> +	if (of_property_read_string(node, "id", &step->id)) {
> +		dev_err(dev, "missing id property!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!strcmp(node->name, "regulator")) {
> +		step->type = POWER_SEQ_REGULATOR;
> +#ifdef CONFIG_OF_GPIO
> +	} else if (!strcmp(node->name, "gpio")) {
> +		int gpio;
> +
> +		step->type = POWER_SEQ_GPIO;
> +		gpio = of_get_named_gpio(dev->of_node, step->id, 0);
> +		if (gpio < 0) {
> +			dev_err(dev, "cannot resolve gpio \"%s\"\n", step->id);
> +			return gpio;
> +		}
> +		step->gpio = gpio;
> +#endif /* CONFIG_OF_GPIO */
> +	} else if (!strcmp(node->name, "pwm")) {
> +		step->type = POWER_SEQ_PWM;
> +	} else {
> +		dev_err(dev, "invalid power seq step type!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_find_property(node, "enable", NULL)) {
> +		step->params.enable = 1;
> +	} else if (!of_find_property(node, "disable", NULL)) {
> +		dev_err(dev, "missing enable or disable property!\n");
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(node, "pre-delay", &step->params.pre_delay);
> +	of_property_read_u32(node, "post-delay", &step->params.post_delay);
> +
> +	return 0;
> +}
> +
> +platform_power_seq *of_parse_power_seq(struct device *dev,
> +				       struct device_node *node)
> +{
> +	struct device_node *child = NULL;
> +	platform_power_seq *ret;
> +	int cpt = 0;
> +	int err;
> +
> +	if (!node) return NULL;

Again, this should probably be split across two lines.

> +
> +	while ((child = of_get_next_child(node, child)))
> +		cpt++;

for_each_child_of_node()?

> +
> +	/* allocate one more step to signal end of sequence */
> +	ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL);
> +	if (!ret)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cpt = 0;
> +	while ((child = of_get_next_child(node, child))) {

Here as well.

> +		if ((err = of_parse_power_seq_step(dev, child, &ret[cpt++])))
> +			return ERR_PTR(err);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(of_parse_power_seq);
> +#endif /* CONFIG_OF */
> +
> +static
> +struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress,
> +					struct platform_power_seq_step *res)
> +{
> +	struct power_seq_resource *step;
> +
> +	list_for_each_entry(step, ress, list) {
> +		if (step->type != res->type) continue;
> +		switch (res->type) {
> +		case POWER_SEQ_GPIO:
> +			if (step->gpio == res->gpio)
> +				return step;
> +			break;
> +		default:
> +			if (!strcmp(step->id, res->id))
> +				return step;
> +			break;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static int power_seq_allocate_resource(struct device *dev,
> +				       struct power_seq_resource *res)
> +{
> +	int err;
> +
> +	switch (res->type) {
> +#ifdef CONFIG_REGULATOR
> +	case POWER_SEQ_REGULATOR:
> +		res->regulator = devm_regulator_get(dev, res->id);
> +		if (IS_ERR(res->regulator)) {
> +			dev_err(dev, "cannot get regulator \"%s\"\n", res->id);
> +			return PTR_ERR(res->regulator);
> +		}
> +		break;
> +#endif
> +#ifdef CONFIG_PWM
> +	case POWER_SEQ_PWM:
> +		res->pwm = pwm_get(dev, res->id);
> +		if (IS_ERR(res->pwm)) {
> +			dev_err(dev, "cannot get pwm \"%s\"\n", res->id);
> +			return PTR_ERR(res->pwm);
> +		}
> +		break;
> +#endif
> +#ifdef CONFIG_GPIOLIB
> +	case POWER_SEQ_GPIO:
> +		err = devm_gpio_request_one(dev, res->gpio, GPIOF_OUT_INIT_HIGH,
> +					    "backlight_gpio");
> +		if (err) {
> +			dev_err(dev, "cannot get gpio %d\n", res->gpio);
> +			return err;
> +		}
> +		break;
> +#endif
> +	default:
> +		dev_err(dev, "invalid resource type %d\n", res->type);
> +		return -EINVAL;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> +			   platform_power_seq *pseq)
> +{
> +	struct power_seq_step *seq = NULL, *ret;
> +	struct power_seq_resource *res;
> +	int cpt, err;
> +
> +	/* first pass to count the number of steps to allocate */
> +	for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);

Wouldn't it be easier to pass around the number of steps in the sequence
instead of having to count in various places? This would be more along
the lines of how struct platform_device defines associated resources.

> +
> +	if (!cpt)
> +		return seq;

Perhaps this should return an error-code as well? I find it nice to not
have to handle NULL specially when using ERR_PTR et al.

> +
> +	/* 1 more for the STOP step */
> +	ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL);
> +	if (!seq)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) {
> +		/* create resource node if not referenced already */
> +		if (!(res = power_seq_find_resource(ress, pseq))) {
> +			res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> +			if (!res)
> +				return ERR_PTR(-ENOMEM);
> +			res->type = pseq->type;
> +
> +			if (res->type == POWER_SEQ_GPIO)
> +				res->gpio = pseq->gpio;
> +			else
> +				res->id = pseq->id;
> +
> +			if ((err = power_seq_allocate_resource(dev, res)) < 0)
> +				return ERR_PTR(err);
> +
> +			list_add(&res->list, ress);
> +		}
> +		seq->resource = res;
> +		memcpy(&seq->params, &pseq->params, sizeof(seq->params));
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_build);
> +
> +void power_seq_free_resources(power_seq_resources *ress) {

The brace needs to go on a line by itself.

> +	struct power_seq_resource *res;
> +
> +#ifdef CONFIG_PWM
> +	list_for_each_entry(res, ress, list) {
> +		if (res->type == POWER_SEQ_PWM)
> +			pwm_put(res->pwm);
> +	}
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(power_seq_free_resources);
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> new file mode 100644
> index 0000000..da0593a
> --- /dev/null
> +++ b/include/linux/power_seq.h
> @@ -0,0 +1,139 @@
> +/*
> + * power_seq.h
> + *
> + * Simple interpreter for defining power sequences as platform data or device
> + * tree properties. Initially designed for use with backlight drivers.
> + *
> + * Power sequences are designed to replace the callbacks typically used in
> + * board-specific files that implement board-specific power sequences of devices
> + * such as backlights. A power sequence is an array of resources (which can a
> + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
> + * disable) and optional pre and post step delays. By having them interpreted
> + * instead of arbitrarily executed, it is possible to describe these in the
> + * device tree and thus remove board-specific code from the kernel.
> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#ifndef __LINUX_POWER_SEQ_H
> +#define __LINUX_POWER_SEQ_H
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct regulator;
> +struct pwm_device;
> +struct device_node;
> +
> +/**
> + * The different kinds of resources that can be controlled during the sequences.
> + */
> +typedef enum {
> +	POWER_SEQ_STOP = 0,
> +	POWER_SEQ_REGULATOR,
> +	POWER_SEQ_PWM,
> +	POWER_SEQ_GPIO,
> +	POWER_SEQ_MAX,
> +} power_res_type;

Maybe the prefix power_seq should be used here as well, so:
power_seq_res_type.

> +
> +struct power_seq_resource {
> +	power_res_type type;
> +	/* name to resolve for resources with a name (regulator, pwm) */
> +	const char *id;
> +	/* resolved resource */
> +	union {
> +		struct regulator *regulator;
> +		struct pwm_device *pwm;
> +		int gpio;
> +	};
> +	/* used to maintain the list of resources used by the driver */
> +	struct list_head list;
> +};
> +typedef struct list_head power_seq_resources;

No type definitions like this, please. Also, why define this particular
type globally?

> +
> +struct power_step_params {
> +	/* enable the resource if 1, disable if 0 */
> +	bool enable;
> +	/* delay (in ms) to wait before executing the step */
> +	int  pre_delay;
> +	/* delay (in ms) to wait after executing the step */
> +	int post_delay;

unsigned int for the delays?

> +};
> +
> +/**
> + * Platform definition of power sequences. A sequence is an array of these,
> + * terminated by a STOP instance.
> + */
> +struct platform_power_seq_step {
> +	power_res_type type;
> +	union {
> +		/* Used by REGULATOR and PWM types to name the resource */
> +		const char *id;
> +		/* Used by GPIO */
> +		int gpio;
> +	};
> +	struct power_step_params params;
> +};
> +typedef struct platform_power_seq_step platform_power_seq;

Why are the parameters kept in a separate structure? What are the
disadvantages of keeping the in the sequence step structure directly?

> +
> +/**
> + * Power sequence steps resolved against their resource. Built by
> + * power_seq_build and used to run the sequence.
> + */
> +struct power_seq_step {
> +	struct power_seq_resource *resource;
> +	struct power_step_params params;
> +};
> +typedef struct power_seq_step power_seq;

Would it make sense to make the struct power_seq opaque? I don't see why
anyone but the power_seq code should access the internals. For resource
managing it might also be easier to separate struct power_seq_step and
struct power_seq, making the power_seq basically something like:

	struct power_seq {
		struct power_seq_step *steps;
		unsigned int num_steps;
	};

Perhaps a name field can be included for diagnostic purposes.

> +
> +#ifdef CONFIG_OF
> +/**
> + * Build a platform data sequence from a device tree node. Memory for the
> + * sequence is allocated using devm_kzalloc on dev.
> + */
> +platform_power_seq *of_parse_power_seq(struct device *dev,
> +				       struct device_node *node);
> +#else
> +platform_power_seq *of_parse_power_seq(struct device *dev,
> +				       struct device_node *node)
> +{
> +	return NULL;
> +}
> +#endif
> +
> +/**
> + * Build a runnable power sequence from platform data, and add the resources
> + * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
> + * on dev.
> + */
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> +			   platform_power_seq *pseq);

I already mentioned this above: I fail to see why the ress parameter is
needed here. It is an internal implementation detail of the power
sequence code. Maybe a better place would be to include it within the
struct power_seq.

> +/**
> + * Free all the resources previously allocated by power_seq_allocate_resources.
> + */
> +void power_seq_free_resources(power_seq_resources *ress);
> +
> +/**
> + * Run the given power sequence. Returns 0 on success, error code in case of
> + * failure.
> + */
> +int power_seq_run(struct device *dev, power_seq *seq);

I think the API is too fine grained here. From a user's point of view,
I'd expect a sequence like this:

	seq = power_seq_build(dev, sequence);
	...
	power_seq_run(seq);
	...
	power_seq_free(seq);

Perhaps with managed variants where the power_seq_free() is executed
automatically:

	seq = devm_power_seq_build(dev, sequence);
	...
	power_seq_run(seq);

Generally I really like where this is going.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/30/2012 08:33 PM, Thierry Reding wrote:
>> +You will need an instance of power_seq_resources to keep track of the resources
>> +that are already allocated. On success, the function returns a devm allocated
>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>> +failure, and error code is returned.
>
> I don't quite understand why the struct power_seq_resources is needed.
> Can this not be stored within power_seq?

power_seq_resources serves two purposes:
1) When parsing sequences, it keeps track of the resources we have 
already allocated to avoid getting the same resource twice
2) On cleanup, it cleans the resources that needs to be freed (i.e. 
those that are not devm-handled).

2) can certainly be removed either by enforcing use of devm, or by doing 
reference counting. 1) seems more difficult to avoid - we need to keep 
track of the resources we already own between calls to 
power_seq_build(). I'd certainly be glad to remove that structure from 
public view and simplify the code if that is possible though.

>> +
>> +A resolved power sequence returned by power_seq_build can be run by
>> +power_run_run():
>> +
>> +int power_seq_run(struct device *dev, power_seq *seq);
>
> Why is the struct device required here? It already is passed during the
> call to pwm_seq_build(), so perhaps you should keep a reference to it
> within struct power_seq?

The device is only needed for printing error messages. But as you point 
later, maybe messages should not be printed there at all. I will try to 
remove that parameter.

>> +It returns 0 if the sequence has successfully been run, or an error code if a
>> +problem occured.
>> +
>> +Finally, some resources that cannot be allocated through devm need to be freed
>> +manually. Therefore, be sure to call power_seq_free_resources() in your device
>> +remove function:
>> +
>> +void power_seq_free_resources(power_seq_resources *ress);
>
> Could this not also be handled by a managed version? If a power_seq is
> always managed, then I would assume that it also takes care of freeing
> the resources, even if the resources have no managed equivalents.

Right.

> Perhaps it would also make sense to provide non-managed version of these
> functions. I think that would make the managed versions easier and more
> canonical to implement.

A power_seq is a single block of memory, so that should be reasonnably 
doable indeed. Let me think a little bit more about that.

>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> +             power-supply = <&mydevice_reg>;
>> +             enable-gpio = <&gpio 6 0>;
>> +
>> +             power-on-sequence {
>> +                     regulator@0 {
>> +                             id = "power";
>> +                             enable;
>> +                             post-delay = <10>;
>> +                     };
>> +                     gpio@1 {
>> +                             id = "enable-gpio";
>> +                             enable;
>> +                     };
>> +             };
>> +
>> +Note that first, the phandles of the regulator and gpio used in the sequences
>> +are defined as properties. Then the sequence references them through the id
>> +property of every step. The name of sub-properties defines the type of the step.
>> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
>> +sequentially.
>
> I think there has been quite some discussion regarding the naming of
> subnodes and the conclusion seems to have been to name them uniformly
> after what they represent. As such the power-on-sequence subnodes should
> be called step@0, step@1, etc. However, that will require the addition
> of a property to define the type of resource.

That's fine I guess - just adds some footprint to the DT, but nothing crazy.

> Also, is there some way we can make the id property for GPIOs not
> require the -gpio suffix? If the resource type is already GPIO, then it
> seems redundant to add -gpio to the ID.

There is unfortunately an inconsistency between the way regulators and 
GPIOs are gotten by name. regulator_get(id) will expect to find a 
property named "id-supply", while gpio_request_one(id) expects a 
property named exactly "id". To workaround this we could sprintf the 
correct property name from a non-suffixed property name within the 
driver, but I think this actually speaks more in favor of having 
phandles directly into the sequences.

>> +config POWER_SEQ
>> +     bool
>> +     default n
>> +
>
> "default n" is already the default, so you can drop that line.

Did that, thanks.

>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#endif
>
> I think you don't need the CONFIG_OF guard around these. Both of.h and
> of_gpio.h can be included unconditionally and actually contain dummy
> definitions for the public functions in the !OF case.

Fixed, thanks.

>> +static int power_seq_step_run(struct power_seq_step *step)
>> +{
>> +     int err = 0;
>> +
>> +     if (step->params.pre_delay)
>> +             mdelay(step->params.pre_delay);
>> +
>> +     switch (step->resource->type) {
>> +#ifdef CONFIG_REGULATOR
>> +     case POWER_SEQ_REGULATOR:
>> +             if (step->params.enable)
>> +                     err = regulator_enable(step->resource->regulator);
>> +             else
>> +                     err = regulator_disable(step->resource->regulator);
>> +             break;
>> +#endif
>> +#ifdef CONFIG_PWM
>> +     case POWER_SEQ_PWM:
>> +             if (step->params.enable)
>> +                     err = pwm_enable(step->resource->pwm);
>> +             else
>> +                     pwm_disable(step->resource->pwm);
>> +             break;
>> +#endif
>> +#ifdef CONFIG_GPIOLIB
>> +     case POWER_SEQ_GPIO:
>> +             gpio_set_value_cansleep(step->resource->gpio,
>> +                                     step->params.enable);
>> +             break;
>> +#endif
>
> This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> *_run() functions for each type of resource would be any better, though.
> Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> GPIOLIB symbols to side-step the issue completely?

If it is not realistic to consider a kernel built without regulator, pwm 
or gpiolib support, then we might as well do that. But isn't that a 
possibility?

>> +     if (!seq) return 0;
>
> I don't think this is acceptable according to the coding style. Also,
> perhaps returning -EINVAL would be more meaningful?

I neglected running checkpatch before submitting, apologies for that. 
The return value seems correct to me, a NULL sequence has no effect.

>> +
>> +     while (seq->resource) {
>
> Perhaps this should check for POWER_SEQ_STOP instead?

There is no resource for POWER_SEQ_STOP - therefore, a NULL resource is 
used instead.

>> +             if ((err = power_seq_step_run(seq++))) {
>> +                     dev_err(dev, "error %d while running power sequence!\n",
>> +                             err);
>
> For this kind of diagnostics it could be useful to have a name
> associated with the power sequence. But I'm not sure that making the
> power sequence code output an error here is the best solution. I find it
> to be annoying when core code starts outputting too many error codes. In
> this case it's particularily easy to catch the errors in the caller.

Giving names to power sequences sounds like a good idea. Let me see how 
this can be done. It might require some more data structuring.

>> +
>> +     while ((child = of_get_next_child(node, child)))
>> +             cpt++;
>
> for_each_child_of_node()?
>
>> +
>> +     /* allocate one more step to signal end of sequence */
>> +     ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL);
>> +     if (!ret)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     cpt = 0;
>> +     while ((child = of_get_next_child(node, child))) {
>
> Here as well.

Ah, didn't know that. Thanks.

>> +     /* first pass to count the number of steps to allocate */
>> +     for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
>
> Wouldn't it be easier to pass around the number of steps in the sequence
> instead of having to count in various places? This would be more along
> the lines of how struct platform_device defines associated resources.

My goal was to limit the number of data structures, but if we add a name 
to power sequences, we can add a steps count as well.

>> +
>> +     if (!cpt)
>> +             return seq;
>
> Perhaps this should return an error-code as well? I find it nice to not
> have to handle NULL specially when using ERR_PTR et al.

Agreed.


>> +typedef enum {
>> +     POWER_SEQ_STOP = 0,
>> +     POWER_SEQ_REGULATOR,
>> +     POWER_SEQ_PWM,
>> +     POWER_SEQ_GPIO,
>> +     POWER_SEQ_MAX,
>> +} power_res_type;
>
> Maybe the prefix power_seq should be used here as well, so:
> power_seq_res_type.

Definitely.

>> +typedef struct list_head power_seq_resources;
>
> No type definitions like this, please. Also, why define this particular
> type globally?

I will move that into a proper structure with a name and number of steps.

>> +
>> +struct power_step_params {
>> +     /* enable the resource if 1, disable if 0 */
>> +     bool enable;
>> +     /* delay (in ms) to wait before executing the step */
>> +     int  pre_delay;
>> +     /* delay (in ms) to wait after executing the step */
>> +     int post_delay;
>
> unsigned int for the delays?

Yup.

>> +typedef struct platform_power_seq_step platform_power_seq;
>
> Why are the parameters kept in a separate structure? What are the
> disadvantages of keeping the in the sequence step structure directly?

This ensures the same parameters are used for the platform data and 
resolved sequences, and also ensures they are all copied correctly using 
memcpy. But maybe I am just making something complex out of something 
that ought to be simpler.

>> +struct power_seq_step {
>> +     struct power_seq_resource *resource;
>> +     struct power_step_params params;
>> +};
>> +typedef struct power_seq_step power_seq;
>
> Would it make sense to make the struct power_seq opaque? I don't see why
> anyone but the power_seq code should access the internals.

I would like to do that actually. The issue is that it did not work go 
well with the legacy pwm_backlight behavior: a power sequence needs to 
be constructed out of a PWM obtained through pwm_request(int pwm_id, 
char *label) and this behavior cannot be emulated using the new platform 
data interface (which only works with pwm_get()). But if I remove this 
old behavior, then I could make power_seq opaque. I don't think many 
drivers are using it. What do you think?

> For resource
> managing it might also be easier to separate struct power_seq_step and
> struct power_seq, making the power_seq basically something like:
>
>          struct power_seq {
>                  struct power_seq_step *steps;
>                  unsigned int num_steps;
>          };
>
> Perhaps a name field can be included for diagnostic purposes.

Yes, looks like we are going in that direction. If this can be made 
private then the number of public data structures will not be too 
confusing (platform data only, basically).

>> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
>> +                        platform_power_seq *pseq);
>
> I already mentioned this above: I fail to see why the ress parameter is
> needed here. It is an internal implementation detail of the power
> sequence code. Maybe a better place would be to include it within the
> struct power_seq.

Problem is that I need to track which resources are already allocated 
between calls to power_seq_build(). Even if I attach the resources into 
struct power_seq, they won't be attainable by the next call. So I'm 
afraid we are bound to pass a tracking structure at least to 
power_seq_build.

>> +/**
>> + * Free all the resources previously allocated by power_seq_allocate_resources.
>> + */
>> +void power_seq_free_resources(power_seq_resources *ress);
>> +
>> +/**
>> + * Run the given power sequence. Returns 0 on success, error code in case of
>> + * failure.
>> + */
>> +int power_seq_run(struct device *dev, power_seq *seq);
>
> I think the API is too fine grained here. From a user's point of view,
> I'd expect a sequence like this:
>
>          seq = power_seq_build(dev, sequence);
>          ...
>          power_seq_run(seq);
>          ...
>          power_seq_free(seq);
>
> Perhaps with managed variants where the power_seq_free() is executed
> automatically:
>
>          seq = devm_power_seq_build(dev, sequence);
>          ...
>          power_seq_run(seq);

I agree. On top of that, of_parse_power_seq() should directly return a 
resolved power sequence, not the platform data.

> Generally I really like where this is going.

Thanks - I really appreciate your review.

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
> On 07/30/2012 08:33 PM, Thierry Reding wrote:
> >>+You will need an instance of power_seq_resources to keep track of the resources
> >>+that are already allocated. On success, the function returns a devm allocated
> >>+resolved sequence that is ready to be passed to power_seq_run(). In case of
> >>+failure, and error code is returned.
> >
> >I don't quite understand why the struct power_seq_resources is needed.
> >Can this not be stored within power_seq?
> 
> power_seq_resources serves two purposes:
> 1) When parsing sequences, it keeps track of the resources we have
> already allocated to avoid getting the same resource twice
> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
> those that are not devm-handled).
> 
> 2) can certainly be removed either by enforcing use of devm, or by
> doing reference counting. 1) seems more difficult to avoid - we need
> to keep track of the resources we already own between calls to
> power_seq_build(). I'd certainly be glad to remove that structure
> from public view and simplify the code if that is possible though.

I still don't see the problem. Managing the resources should be part of
the power_seq core and shouldn't be visible to users. Maybe what you are
worried about is that you may need the same resource both for a power-up
and a power-down sequence? I can see how that would require a global
list of resources.

However I still think it would be easier to encapsulate that completely.
Maybe another level of abstraction is required. You could for example
add another type to encapsulate several power sequences and that could
keep a list of used resources. I can't think of a good name, but maybe
the following DT snippet clarifies what I mean:

	power-sequences {
		#address-cells = <1>;
		#size-cells = <0>;

		sequence@0 {
			name = "up";

			#address-cells = <1>;
			#size-cells = <0>;

			step@0 {
				...
			};

			...
		};

		sequence@1 {
			name = "down";

			#address-cells = <1>;
			#size-cells = <0>;

			step@0 {
				...
			};

			...
		};
	};

If you add a name property like this, you could extend the API to
support running a named sequence:

	power_seq_run(seq, "up");
	...
	power_seq_run(seq, "down);

> >Also, is there some way we can make the id property for GPIOs not
> >require the -gpio suffix? If the resource type is already GPIO, then it
> >seems redundant to add -gpio to the ID.
> 
> There is unfortunately an inconsistency between the way regulators
> and GPIOs are gotten by name. regulator_get(id) will expect to find
> a property named "id-supply", while gpio_request_one(id) expects a
> property named exactly "id". To workaround this we could sprintf the
> correct property name from a non-suffixed property name within the
> driver, but I think this actually speaks more in favor of having
> phandles directly into the sequences.

Yes, if it can be made to work by specifying the phandle directly that
is certainly better.

> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+     int err = 0;
> >>+
> >>+     if (step->params.pre_delay)
> >>+             mdelay(step->params.pre_delay);
> >>+
> >>+     switch (step->resource->type) {
> >>+#ifdef CONFIG_REGULATOR
> >>+     case POWER_SEQ_REGULATOR:
> >>+             if (step->params.enable)
> >>+                     err = regulator_enable(step->resource->regulator);
> >>+             else
> >>+                     err = regulator_disable(step->resource->regulator);
> >>+             break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+     case POWER_SEQ_PWM:
> >>+             if (step->params.enable)
> >>+                     err = pwm_enable(step->resource->pwm);
> >>+             else
> >>+                     pwm_disable(step->resource->pwm);
> >>+             break;
> >>+#endif
> >>+#ifdef CONFIG_GPIOLIB
> >>+     case POWER_SEQ_GPIO:
> >>+             gpio_set_value_cansleep(step->resource->gpio,
> >>+                                     step->params.enable);
> >>+             break;
> >>+#endif
> >
> >This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> >*_run() functions for each type of resource would be any better, though.
> >Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> >GPIOLIB symbols to side-step the issue completely?
> 
> If it is not realistic to consider a kernel built without regulator,
> pwm or gpiolib support, then we might as well do that. But isn't
> that a possibility?

I'd say that anything complex enough to make use of power-sequencing
probably has all of these enabled anyway. But maybe I'm not very
qualified to judge.

> 
> >>+     if (!seq) return 0;
> >
> >I don't think this is acceptable according to the coding style. Also,
> >perhaps returning -EINVAL would be more meaningful?
> 
> I neglected running checkpatch before submitting, apologies for
> that. The return value seems correct to me, a NULL sequence has no
> effect.

But seq == NULL should never happen anyway, right?

> 
> >>+
> >>+     while (seq->resource) {
> >
> >Perhaps this should check for POWER_SEQ_STOP instead?
> 
> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource
> is used instead.

Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of
a sequence, so intuitively I'd be looking for that as a stop condition.

> >>+typedef struct platform_power_seq_step platform_power_seq;
> >
> >Why are the parameters kept in a separate structure? What are the
> >disadvantages of keeping the in the sequence step structure directly?
> 
> This ensures the same parameters are used for the platform data and
> resolved sequences, and also ensures they are all copied correctly
> using memcpy. But maybe I am just making something complex out of
> something that ought to be simpler.
> 
> >>+struct power_seq_step {
> >>+     struct power_seq_resource *resource;
> >>+     struct power_step_params params;
> >>+};
> >>+typedef struct power_seq_step power_seq;
> >
> >Would it make sense to make the struct power_seq opaque? I don't see why
> >anyone but the power_seq code should access the internals.
> 
> I would like to do that actually. The issue is that it did not work
> go well with the legacy pwm_backlight behavior: a power sequence
> needs to be constructed out of a PWM obtained through
> pwm_request(int pwm_id, char *label) and this behavior cannot be
> emulated using the new platform data interface (which only works
> with pwm_get()). But if I remove this old behavior, then I could
> make power_seq opaque. I don't think many drivers are using it. What
> do you think?

I don't see how that is relevant here, since this power-sequencing code
is supposed to be generic and not tied to any specific implementation.
Can you explain further?

In any case you shouldn't be using pwm_request() in new code.

> >For resource
> >managing it might also be easier to separate struct power_seq_step and
> >struct power_seq, making the power_seq basically something like:
> >
> >         struct power_seq {
> >                 struct power_seq_step *steps;
> >                 unsigned int num_steps;
> >         };
> >
> >Perhaps a name field can be included for diagnostic purposes.
> 
> Yes, looks like we are going in that direction. If this can be made
> private then the number of public data structures will not be too
> confusing (platform data only, basically).

Yes, that sounds like a much cleaner approach.

> 
> >>+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> >>+                        platform_power_seq *pseq);
> >
> >I already mentioned this above: I fail to see why the ress parameter is
> >needed here. It is an internal implementation detail of the power
> >sequence code. Maybe a better place would be to include it within the
> >struct power_seq.
> 
> Problem is that I need to track which resources are already
> allocated between calls to power_seq_build(). Even if I attach the
> resources into struct power_seq, they won't be attainable by the
> next call. So I'm afraid we are bound to pass a tracking structure
> at least to power_seq_build.

I think this could be solved nicely by what I proposed earlier.

> >>+/**
> >>+ * Free all the resources previously allocated by power_seq_allocate_resources.
> >>+ */
> >>+void power_seq_free_resources(power_seq_resources *ress);
> >>+
> >>+/**
> >>+ * Run the given power sequence. Returns 0 on success, error code in case of
> >>+ * failure.
> >>+ */
> >>+int power_seq_run(struct device *dev, power_seq *seq);
> >
> >I think the API is too fine grained here. From a user's point of view,
> >I'd expect a sequence like this:
> >
> >         seq = power_seq_build(dev, sequence);
> >         ...
> >         power_seq_run(seq);
> >         ...
> >         power_seq_free(seq);
> >
> >Perhaps with managed variants where the power_seq_free() is executed
> >automatically:
> >
> >         seq = devm_power_seq_build(dev, sequence);
> >         ...
> >         power_seq_run(seq);
> 
> I agree. On top of that, of_parse_power_seq() should directly return
> a resolved power sequence, not the platform data.

I'm not sure. The idiom seems to be to use DT as an alternative source
for platform data, which I guess is due to most drivers already using
platform data. But it has the advantage that after you have the platform
data, be it from DT or directly specified, the subsequent code remains
the same.

Of course you could provide a separate of_build_power_seq() that wraps
both steps for convenience.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 07:19 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
>> On 07/30/2012 08:33 PM, Thierry Reding wrote:
>>>> +You will need an instance of power_seq_resources to keep track of the resources
>>>> +that are already allocated. On success, the function returns a devm allocated
>>>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>>>> +failure, and error code is returned.
>>>
>>> I don't quite understand why the struct power_seq_resources is needed.
>>> Can this not be stored within power_seq?
>>
>> power_seq_resources serves two purposes:
>> 1) When parsing sequences, it keeps track of the resources we have
>> already allocated to avoid getting the same resource twice
>> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
>> those that are not devm-handled).
>>
>> 2) can certainly be removed either by enforcing use of devm, or by
>> doing reference counting. 1) seems more difficult to avoid - we need
>> to keep track of the resources we already own between calls to
>> power_seq_build(). I'd certainly be glad to remove that structure
>> from public view and simplify the code if that is possible though.
>
> I still don't see the problem. Managing the resources should be part of
> the power_seq core and shouldn't be visible to users. Maybe what you are
> worried about is that you may need the same resource both for a power-up
> and a power-down sequence? I can see how that would require a global
> list of resources.

Yes, that is precisely my concern. Sorry for not stating that more clearly.

> However I still think it would be easier to encapsulate that completely.
> Maybe another level of abstraction is required. You could for example
> add another type to encapsulate several power sequences and that could
> keep a list of used resources. I can't think of a good name, but maybe
> the following DT snippet clarifies what I mean:
>
>          power-sequences {
>                  #address-cells = <1>;
>                  #size-cells = <0>;
>
>                  sequence@0 {
>                          name = "up";
>
>                          #address-cells = <1>;
>                          #size-cells = <0>;
>
>                          step@0 {
>                                  ...
>                          };
>
>                          ...
>                  };
>
>                  sequence@1 {
>                          name = "down";
>
>                          #address-cells = <1>;
>                          #size-cells = <0>;
>
>                          step@0 {
>                                  ...
>                          };
>
>                          ...
>                  };
>          };
>
> If you add a name property like this, you could extend the API to
> support running a named sequence:
>
>          power_seq_run(seq, "up");
>          ...
>          power_seq_run(seq, "down);

Mmm, that's something to consider. Forcing power sequences to be grouped 
within a "power-sequences" node would also make parsing easier from the 
driver side since it would not have to explicitly parse every sequence. 
We could even imagine some tighter integration with the device subsystem 
to automatically run specifically-named sequences during suspend/resume. 
But maybe I'm thinking too much here.

>
>>> Also, is there some way we can make the id property for GPIOs not
>>> require the -gpio suffix? If the resource type is already GPIO, then it
>>> seems redundant to add -gpio to the ID.
>>
>> There is unfortunately an inconsistency between the way regulators
>> and GPIOs are gotten by name. regulator_get(id) will expect to find
>> a property named "id-supply", while gpio_request_one(id) expects a
>> property named exactly "id". To workaround this we could sprintf the
>> correct property name from a non-suffixed property name within the
>> driver, but I think this actually speaks more in favor of having
>> phandles directly into the sequences.
>
> Yes, if it can be made to work by specifying the phandle directly that
> is certainly better.

Let's do that then - as for the PWM issue I had, let's address that by 
clearly stating in the documentation that phandles referring to a same 
device *must* be identical.

>>>> +     if (!seq) return 0;
>>>
>>> I don't think this is acceptable according to the coding style. Also,
>>> perhaps returning -EINVAL would be more meaningful?
>>
>> I neglected running checkpatch before submitting, apologies for
>> that. The return value seems correct to me, a NULL sequence has no
>> effect.
>
> But seq = NULL should never happen anyway, right?

It could if you are parsing a NULL node. It seems safe to me to consider 
that a NULL sequence is an empty sequence, but if I go for your solution 
involving another data structure to encapsulate the sequence, then this 
might change.

>>> Perhaps this should check for POWER_SEQ_STOP instead?
>>
>> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource
>> is used instead.
>
> Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of
> a sequence, so intuitively I'd be looking for that as a stop condition.

That is for platform data - resolved sequences get their type from their 
resource, and a STOP sequence does not have a resource. But the STOP 
type will go away too since we will have a steps count in the platform 
data instead.

>> I would like to do that actually. The issue is that it did not work
>> go well with the legacy pwm_backlight behavior: a power sequence
>> needs to be constructed out of a PWM obtained through
>> pwm_request(int pwm_id, char *label) and this behavior cannot be
>> emulated using the new platform data interface (which only works
>> with pwm_get()). But if I remove this old behavior, then I could
>> make power_seq opaque. I don't think many drivers are using it. What
>> do you think?
>
> I don't see how that is relevant here, since this power-sequencing code
> is supposed to be generic and not tied to any specific implementation.
> Can you explain further?
>
> In any case you shouldn't be using pwm_request() in new code.

Power sequences only rely on pwm_get, and never call pwm_request since 
it is, as you stated, deprecated. However there are still boards that 
use the old pwm_id member of the pwm_backlight_platform_data. For these, 
we must call pwm_request from the pwm_backlight driver in order to 
resolve the PWM (see pwm_backlight_legacy_probe of the seconds patch). 
As the PWM is being resolved by the backlight driver, and not within the 
power sequences parser, the resolved data structure must be visible to 
pwm_backlight so it can construct it. There are two ways to solve this 
and keep the power sequences structure private:

1) Add a way to resolve a PWM by id using pwm_request in the power 
sequences (we probably should not do that)
2) Port the old platform pwm_request code to use pwm_add_table and 
pwm_get instead.

Do I get points if I do 2)? :)

Thanks,
Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Wed, Aug 01, 2012 at 11:50:35AM +0900, Alex Courbot wrote:
> On 07/31/2012 07:19 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
> >>I would like to do that actually. The issue is that it did not work
> >>go well with the legacy pwm_backlight behavior: a power sequence
> >>needs to be constructed out of a PWM obtained through
> >>pwm_request(int pwm_id, char *label) and this behavior cannot be
> >>emulated using the new platform data interface (which only works
> >>with pwm_get()). But if I remove this old behavior, then I could
> >>make power_seq opaque. I don't think many drivers are using it. What
> >>do you think?
> >
> >I don't see how that is relevant here, since this power-sequencing code
> >is supposed to be generic and not tied to any specific implementation.
> >Can you explain further?
> >
> >In any case you shouldn't be using pwm_request() in new code.
> 
> Power sequences only rely on pwm_get, and never call pwm_request
> since it is, as you stated, deprecated. However there are still
> boards that use the old pwm_id member of the
> pwm_backlight_platform_data. For these, we must call pwm_request
> from the pwm_backlight driver in order to resolve the PWM (see
> pwm_backlight_legacy_probe of the seconds patch). As the PWM is
> being resolved by the backlight driver, and not within the power
> sequences parser, the resolved data structure must be visible to
> pwm_backlight so it can construct it. There are two ways to solve
> this and keep the power sequences structure private:
> 
> 1) Add a way to resolve a PWM by id using pwm_request in the power
> sequences (we probably should not do that)

No. But I think we don't need that either. If you use power sequences,
then you shouldn't be using the pwm_id member. Both methods should be
exclusive.

> 2) Port the old platform pwm_request code to use pwm_add_table and
> pwm_get instead.

In the long term, every PWM user should move to pwm_add_table() and
pwm_get(), but there are quite a lot of boards that need conversion.

> Do I get points if I do 2)? :)

Yes. =) I have a sample patch for PXA EZX, but as I said, many more
boards need conversion.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:

> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
> those that are not devm-handled).

> 2) can certainly be removed either by enforcing use of devm, or by
> doing reference counting. 1) seems more difficult to avoid - we need

I'd just add devm_ for everything that's not got it.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Rob Herring]

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

Why not? We'll always have some amount of board code. The key is to
limit parts that are just data. I'm not sure this is something that
should be in devicetree.

Perhaps what is needed is a better way to hook into the driver like
notifiers?

> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  Documentation/power/power_seq.txt | 120 +++++++++++++++
>  drivers/base/Kconfig              |   4 +
>  drivers/base/Makefile             |   1 +
>  drivers/base/power_seq.c          | 300 ++++++++++++++++++++++++++++++++++++++
>  include/linux/power_seq.h         | 139 ++++++++++++++++++
>  5 files changed, 564 insertions(+)
>  create mode 100644 Documentation/power/power_seq.txt
>  create mode 100644 drivers/base/power_seq.c
>  create mode 100644 include/linux/power_seq.h
> 
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> new file mode 100644
> index 0000000..aa2ceb5
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,120 @@
> +Runtime Interpreted Power Sequences
> +-----------------------------------
> +
> +Problem
> +-------
> +One very common board-dependent code is the out-of-driver code that is used to
> +turn a device on or off. For instance, SoC boards very commonly use a GPIO
> +(abstracted to a regulator or not) to control the power supply of a backlight,
> +disabling it when the backlight is not used in order to save power. The GPIO
> +that should be used, however, as well as the exact power sequence that may
> +involve different resources, is board-dependent and thus unknown of the driver.
> +
> +This has been addressed so far by using hooks in the device's platform data that
> +are called whenever the state of the device might reflect a power change. This
> +approach, however, introduces board-dependant code into the kernel and is not
> +compatible with the device tree.
> +
> +The Runtime Interpreted Power Sequences (or power sequences for short) aim at
> +turning this code into platform data or device tree nodes. Power sequences are
> +described using a simple format and run by a simple interpreter whenever needed.
> +This allows to remove the callback mechanism and makes the kernel less
> +board-dependant.
> +
> +Sequences Format
> +----------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:
> +- GPIOs
> +- Regulators
> +- PWMs
> +
> +Each step designates a resource and the following parameters:
> +- Whether the step should enable or disable the resource,
> +- Delay to wait before performing the action,
> +- Delay to wait after performing the action.
> +
> +Both new resources and parameters can be introduced, but the goal is of course
> +to keep things as simple and compact as possible.
> +
> +The platform data is a simple array of platform_power_seq_step instances, each
> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
> +identified by number. For example, the following sequence will turn on the
> +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
> +
> +struct platform_power_seq_step power_on_seq[] = {
> +	{
> +		.type = POWER_SEQ_REGULATOR,
> +		.id = "power",
> +		.params = {
> +			.enable = 1,
> +			.post_delay = 10,
> +		},
> +	},
> +	{
> +		.type = POWER_SEQ_GPIO,
> +		.gpio = 110,
> +		.params = {
> +			.enable = 1,
> +		},
> +	},
> +	{
> +		.type = POWER_SEQ_STOP,
> +	},
> +};
> +
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function takes care of resolving the resources as
> +they are met in the sequence and to allocate them if needed:
> +
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> +			   platform_power_seq *pseq);
> +
> +You will need an instance of power_seq_resources to keep track of the resources
> +that are already allocated. On success, the function returns a devm allocated
> +resolved sequence that is ready to be passed to power_seq_run(). In case of
> +failure, and error code is returned.
> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> +int power_seq_run(struct device *dev, power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
> +
> +Finally, some resources that cannot be allocated through devm need to be freed
> +manually. Therefore, be sure to call power_seq_free_resources() in your device
> +remove function:
> +
> +void power_seq_free_resources(power_seq_resources *ress);
> +
> +Device tree

Bindings need to documented in Documentation/devicetree

> +-----------
> +All the same, power sequences can be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined previously:
> +
> +		power-supply = <&mydevice_reg>;
> +		enable-gpio = <&gpio 6 0>;
> +
> +		power-on-sequence {
> +			regulator@0 {
> +				id = "power";
> +				enable;

What do this mean? Isn't this implied for a regulator?

> +				post-delay = <10>;
> +			};
> +			gpio@1 {
> +				id = "enable-gpio";
> +				enable;
> +			};
> +		};

This looks like you designed the platform_data structs first and then
came up with device nodes to mirror the struct.

Rob

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Mon, Jul 30, 2012 at 10:44:29AM -0500, Rob Herring wrote:
> On 07/27/2012 07:05 AM, Alexandre Courbot wrote:

> > +		power-on-sequence {
> > +			regulator@0 {
> > +				id = "power";
> > +				enable;

> What do this mean? Isn't this implied for a regulator?

I assume you might have some sequences which need some things to be
turned off for some reason; it at least seems to be something you'd want
to design for.

> This looks like you designed the platform_data structs first and then
> came up with device nodes to mirror the struct.

Judging by most of the DT I review this seems idiomatic :)

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Mon, Jul 30, 2012 at 04:47:06PM +0100, Mark Brown wrote:
> On Mon, Jul 30, 2012 at 10:44:29AM -0500, Rob Herring wrote:
> > On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
> 
> > > +		power-on-sequence {
> > > +			regulator@0 {
> > > +				id = "power";
> > > +				enable;
> 
> > What do this mean? Isn't this implied for a regulator?
> 
> I assume you might have some sequences which need some things to be
> turned off for some reason; it at least seems to be something you'd want
> to design for.

Furthermore there is the power-off-sequence equivalent, which you use
for instance when you turn off the panel. Typically they would do the
inverse of the power-on-sequence, so turning off a regulator is
definitiely required.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Stephen Warren]

On 07/30/2012 09:44 AM, Rob Herring wrote:
> On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>> Some device drivers (panel backlights especially) need to follow precise
>> sequences for powering on and off, involving gpios, regulators, PWMs
>> with a precise powering order and delays to respect between each steps.
>> These sequences are board-specific, and do not belong to a particular
>> driver - therefore they have been performed by board-specific hook
>> functions to far.
>>
>> With the advent of the device tree and of ARM kernels that are not
>> board-tied, we cannot rely on these board-specific hooks anymore but
>> need a way to implement these sequences in a portable manner. This patch
>> introduces a simple interpreter that can execute such power sequences
>> encoded either as platform data or within the device tree.
>>
> 
> Why not? We'll always have some amount of board code. The key is to
> limit parts that are just data. I'm not sure this is something that
> should be in devicetree.
> 
> Perhaps what is needed is a better way to hook into the driver like
> notifiers?

I would answer that by asking the reverse question - why should we have
to put some data in DT, and some data into board files still?

I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to
manipulate is just data.

To be honest, if we're going to have to put some parts of a board's
configuration into board files anyway, then the entirety of DT seems
useless; I'd far rather see all the configuration in one cohesive place
than arbitrarily split into two/n different locations - that would make
everything harder to maintain.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 07:26 AM, Stephen Warren wrote:
> On 07/30/2012 09:44 AM, Rob Herring wrote:
>> On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>>> Some device drivers (panel backlights especially) need to follow precise
>>> sequences for powering on and off, involving gpios, regulators, PWMs
>>> with a precise powering order and delays to respect between each steps.
>>> These sequences are board-specific, and do not belong to a particular
>>> driver - therefore they have been performed by board-specific hook
>>> functions to far.
>>>
>>> With the advent of the device tree and of ARM kernels that are not
>>> board-tied, we cannot rely on these board-specific hooks anymore but
>>> need a way to implement these sequences in a portable manner. This patch
>>> introduces a simple interpreter that can execute such power sequences
>>> encoded either as platform data or within the device tree.
>>>
>>
>> Why not? We'll always have some amount of board code. The key is to
>> limit parts that are just data. I'm not sure this is something that
>> should be in devicetree.
>>
>> Perhaps what is needed is a better way to hook into the driver like
>> notifiers?
>
> I would answer that by asking the reverse question - why should we have
> to put some data in DT, and some data into board files still?
>
> I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to
> manipulate is just data.
>
> To be honest, if we're going to have to put some parts of a board's
> configuration into board files anyway, then the entirety of DT seems
> useless; I'd far rather see all the configuration in one cohesive place
> than arbitrarily split into two/n different locations - that would make
> everything harder to maintain.

Also, having these sequences into the DT would allow an older kernel to 
boot on and correctly initialize a newer board with - which is also part 
of the DT's purpose if I am not mistaken.

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Stephen Warren]

On 07/27/2012 06:05 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.

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

> +Sequences Format
> +----------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:
> +- GPIOs
> +- Regulators
> +- PWMs
> +
> +Each step designates a resource and the following parameters:
> +- Whether the step should enable or disable the resource,

At the highest level, i.e. what's being describe here, I wouldn't even
talk about enable/disable, but rather the action to perform on the
resource. The set of legal actions may be specific to the type of
resource in question. Admittedly enable/disable are common though.

> +- Delay to wait before performing the action,
> +- Delay to wait after performing the action.

I don't see a need to have a delay both before and after an action;
except at the start of the sequence, step n's post-delay is at the same
point in the sequence as step n+1's pre-delay. Perhaps make a "delay"
step type?

> +Both new resources and parameters can be introduced, but the goal is of course
> +to keep things as simple and compact as possible.

> +The platform data is a simple array of platform_power_seq_step instances, each

Rather than jumping straight into platform data here, I'd expect an
enumeration of legal resource types, and what actions can be performed
on each, followed by a description of a sequence (very simply, just a
list of actions and their parameters). This could be followed by a
section describing the mapping of the abstract concepts to concrete
platform data representation (and concrete device tree representation).

> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP.

I'd certainly suggest having a step count rather than a sentinel value
in the list.

> Regulator and PWM resources are identified by name. GPIO are
> +identified by number.

That's a little implementation-specific. I guess it's entirely true for
a platform data representation, but not when mapping this into device tree.

> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function takes care of resolving the resources as
> +they are met in the sequence and to allocate them if needed:
> +
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> +			   platform_power_seq *pseq);
> +
> +You will need an instance of power_seq_resources to keep track of the resources
> +that are already allocated. On success, the function returns a devm allocated
> +resolved sequence that is ready to be passed to power_seq_run(). In case of
> +failure, and error code is returned.

If the result is devm-allocated, the function probably should be named
devm_power_seq_build().

I wonder if using the same structure/array as input and output would
simplify the API; the platform data would fill in the fields mentioned
above, and power_seq_build() would parse those, then set other fields in
the same structs to the looked-up handle values?

> +Finally, some resources that cannot be allocated through devm need to be freed
> +manually. Therefore, be sure to call power_seq_free_resources() in your device
> +remove function:
> +
> +void power_seq_free_resources(power_seq_resources *ress);

You can make a custom devm free routine for the power_seq_resources
itself, so the overall free'ing of the content can be triggered by devm,
but the free'ing function can then call whatever non-devm APIs it wants
for the non-devm-allocated members.

> +Device tree
> +-----------
> +All the same, power sequences can be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined previously:
> +
> +		power-supply = <&mydevice_reg>;
> +		enable-gpio = <&gpio 6 0>;
> +
> +		power-on-sequence {
> +			regulator@0 {

As Thierry mentioned, the step nodes should be named for the type of
object they are (a "step") not the type or name of resource they act
upon ("regulator" or "gpio").

If the nodes have a unit address (i.e. end in "@n"), which they will
have to if all named "step" and there's more than one of them, then they
will need a matching reg property. Equally, the parent node will need
#address-cells and #size-cells too. So, the last couple lines would be:

		power-on-sequence {
			#address-cells = <1>;
			#size-cells = <0>;
			step@0 {
				reg = <0>;

> +				id = "power";

"id" is usually a name or identifier. I think you mean "type" or perhaps
"action" here:

				type = "regulator";
				action = "enable";

or:

				action = "enable-regulator";

I wish we had integer constants in DT so we didn't have to do all this
with strings. Sigh.

> +				enable;
> +				post-delay = <10>;
> +			};
> +			gpio@1 {
> +				id = "enable-gpio";
> +				enable;
> +			};
> +		};
> +
> +Note that first, the phandles of the regulator and gpio used in the sequences
> +are defined as properties. Then the sequence references them through the id
> +property of every step. The name of sub-properties defines the type of the step.
> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
> +sequentially.

Oh I see. That's a little confusing. Why not just reference the relevant
resources directly in each step; something more like:

		gpio@1 {
			action = "enable-gpio";
			gpio = <&gpio 1 0>;
		};

I guess that might make parsing/building a little harder, since you'd
have to detect when you'd already done gpio_request() on a given GPIO
and not repeat it or something like that, but to me this makes the DT a
lot easier to comprehend.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 07:45 AM, Stephen Warren wrote:
>> +- Delay to wait before performing the action,
>> +- Delay to wait after performing the action.
>
> I don't see a need to have a delay both before and after an action;
> except at the start of the sequence, step n's post-delay is at the same
> point in the sequence as step n+1's pre-delay. Perhaps make a "delay"
> step type?

My first version used this actually - and you're right, having a "delay" 
step type would be more flexible and less redundant.

>> +Both new resources and parameters can be introduced, but the goal is of course
>> +to keep things as simple and compact as possible.
>
>> +The platform data is a simple array of platform_power_seq_step instances, each
>
> Rather than jumping straight into platform data here, I'd expect an
> enumeration of legal resource types, and what actions can be performed
> on each, followed by a description of a sequence (very simply, just a
> list of actions and their parameters). This could be followed by a
> section describing the mapping of the abstract concepts to concrete
> platform data representation (and concrete device tree representation).

Keeping that in mind for the next revision.

>> +instance describing a step. The type as well as one of id or gpio members
>> +(depending on the type) must be specified. The last step must be of type
>> +POWER_SEQ_STOP.
>
> I'd certainly suggest having a step count rather than a sentinel value
> in the list.

As Thierry did - I think I will go that way.

>> Regulator and PWM resources are identified by name. GPIO are
>> +identified by number.
>
> That's a little implementation-specific. I guess it's entirely true for
> a platform data representation, but not when mapping this into device tree.

If we can come with a way to properly use phandles within DT sequences 
(and we should), then this will only apply to platform data.

>> +You will need an instance of power_seq_resources to keep track of the resources
>> +that are already allocated. On success, the function returns a devm allocated
>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>> +failure, and error code is returned.
>
> If the result is devm-allocated, the function probably should be named
> devm_power_seq_build().

Right - more generally this needs to have both devm and non-devm variants.

> I wonder if using the same structure/array as input and output would
> simplify the API; the platform data would fill in the fields mentioned
> above, and power_seq_build() would parse those, then set other fields in
> the same structs to the looked-up handle values?

The thing is that I am not sure what happens to the platform data once 
probe() is done. Isn't it customary to mark it with __devinit and have 
it freed after probing is successful?

More generally, I think it is a good practice to have data structures 
tailored right for what they need to do - code with members that are 
meaningful only at given points of an instance's life tends to be more 
confusing.

> You can make a custom devm free routine for the power_seq_resources
> itself, so the overall free'ing of the content can be triggered by devm,
> but the free'ing function can then call whatever non-devm APIs it wants
> for the non-devm-allocated members.

That sounds good.

>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> +		power-supply = <&mydevice_reg>;
>> +		enable-gpio = <&gpio 6 0>;
>> +
>> +		power-on-sequence {
>> +			regulator@0 {
>
> As Thierry mentioned, the step nodes should be named for the type of
> object they are (a "step") not the type or name of resource they act
> upon ("regulator" or "gpio").

Will fix that.

> If the nodes have a unit address (i.e. end in "@n"), which they will
> have to if all named "step" and there's more than one of them, then they
> will need a matching reg property. Equally, the parent node will need
> #address-cells and #size-cells too. So, the last couple lines would be:
>
> 		power-on-sequence {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			step@0 {
> 				reg = <0>;

That's precisely what I would like to avoid - I don't need the steps to 
be numbered and I certainly have no use for a reg property. Isn't there 
a way to make it simpler?

>> +				id = "power";
>
> "id" is usually a name or identifier. I think you mean "type" or perhaps
> "action" here:
>
> 				type = "regulator";
> 				action = "enable";
>
> or:
>
> 				action = "enable-regulator";

Right, that was a clear misuse.

> Oh I see. That's a little confusing. Why not just reference the relevant
> resources directly in each step; something more like:
>
> 		gpio@1 {
> 			action = "enable-gpio";
> 			gpio = <&gpio 1 0>;
> 		};
>
> I guess that might make parsing/building a little harder, since you'd
> have to detect when you'd already done gpio_request() on a given GPIO
> and not repeat it or something like that, but to me this makes the DT a
> lot easier to comprehend.

You can see my reply to Thierry for the reason - the only issue with 
that is caused by PWM phandles. If we overcome this, then I agree we 
should use phandles. The code should not even get more complex as I have 
to check whether a resource is already allocated with strings as well.

Thanks,
Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
> On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >I wonder if using the same structure/array as input and output would
> >simplify the API; the platform data would fill in the fields mentioned
> >above, and power_seq_build() would parse those, then set other fields in
> >the same structs to the looked-up handle values?
> 
> The thing is that I am not sure what happens to the platform data
> once probe() is done. Isn't it customary to mark it with __devinit
> and have it freed after probing is successful?

No, platform data should stay around forever. Otherwise, consider what
would happen if your driver is built as a module and you unload and load
it again.

> More generally, I think it is a good practice to have data
> structures tailored right for what they need to do - code with
> members that are meaningful only at given points of an instance's
> life tends to be more confusing.

I agree. Furthermore the driver unload/reload would be another reason
not to reuse platform data as the output of the build() function.

But maybe what Stephen meant was more like filling a structure with data
taken from the platform data and pass that to a resolve() function which
would fill in the missing pieces like pointers to actual resources. I
imagine a managed interface would become a little trickier to do using
such an approach.

> >If the nodes have a unit address (i.e. end in "@n"), which they will
> >have to if all named "step" and there's more than one of them, then they
> >will need a matching reg property. Equally, the parent node will need
> >#address-cells and #size-cells too. So, the last couple lines would be:
> >
> >		power-on-sequence {
> >			#address-cells = <1>;
> >			#size-cells = <0>;
> >			step@0 {
> >				reg = <0>;
> 
> That's precisely what I would like to avoid - I don't need the steps
> to be numbered and I certainly have no use for a reg property. Isn't
> there a way to make it simpler?

It's not technically valid to not have the reg property. Or
#address-cells and #size-cells properties for that matter.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mitch Bradley]

On 7/31/2012 6:56 PM, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>> I wonder if using the same structure/array as input and output would
>>> simplify the API; the platform data would fill in the fields mentioned
>>> above, and power_seq_build() would parse those, then set other fields in
>>> the same structs to the looked-up handle values?
>>
>> The thing is that I am not sure what happens to the platform data
>> once probe() is done. Isn't it customary to mark it with __devinit
>> and have it freed after probing is successful?
> 
> No, platform data should stay around forever. Otherwise, consider what
> would happen if your driver is built as a module and you unload and load
> it again.
> 
>> More generally, I think it is a good practice to have data
>> structures tailored right for what they need to do - code with
>> members that are meaningful only at given points of an instance's
>> life tends to be more confusing.
> 
> I agree. Furthermore the driver unload/reload would be another reason
> not to reuse platform data as the output of the build() function.
> 
> But maybe what Stephen meant was more like filling a structure with data
> taken from the platform data and pass that to a resolve() function which
> would fill in the missing pieces like pointers to actual resources. I
> imagine a managed interface would become a little trickier to do using
> such an approach.
> 
>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>> have to if all named "step" and there's more than one of them, then they
>>> will need a matching reg property. Equally, the parent node will need
>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>
>>> 		power-on-sequence {
>>> 			#address-cells = <1>;
>>> 			#size-cells = <0>;
>>> 			step@0 {
>>> 				reg = <0>;
>>
>> That's precisely what I would like to avoid - I don't need the steps
>> to be numbered and I certainly have no use for a reg property. Isn't
>> there a way to make it simpler?
> 
> It's not technically valid to not have the reg property. Or
> #address-cells and #size-cells properties for that matter.

I'm not keen on this representation where individual steps are nodes.
That seems like it could end up being too "heavyweight" for a long sequence.


> 
> Thierry
> 
> 
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote:
> On 7/31/2012 6:56 PM, Thierry Reding wrote:
> > On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
> >> On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >>> I wonder if using the same structure/array as input and output would
> >>> simplify the API; the platform data would fill in the fields mentioned
> >>> above, and power_seq_build() would parse those, then set other fields in
> >>> the same structs to the looked-up handle values?
> >>
> >> The thing is that I am not sure what happens to the platform data
> >> once probe() is done. Isn't it customary to mark it with __devinit
> >> and have it freed after probing is successful?
> > 
> > No, platform data should stay around forever. Otherwise, consider what
> > would happen if your driver is built as a module and you unload and load
> > it again.
> > 
> >> More generally, I think it is a good practice to have data
> >> structures tailored right for what they need to do - code with
> >> members that are meaningful only at given points of an instance's
> >> life tends to be more confusing.
> > 
> > I agree. Furthermore the driver unload/reload would be another reason
> > not to reuse platform data as the output of the build() function.
> > 
> > But maybe what Stephen meant was more like filling a structure with data
> > taken from the platform data and pass that to a resolve() function which
> > would fill in the missing pieces like pointers to actual resources. I
> > imagine a managed interface would become a little trickier to do using
> > such an approach.
> > 
> >>> If the nodes have a unit address (i.e. end in "@n"), which they will
> >>> have to if all named "step" and there's more than one of them, then they
> >>> will need a matching reg property. Equally, the parent node will need
> >>> #address-cells and #size-cells too. So, the last couple lines would be:
> >>>
> >>> 		power-on-sequence {
> >>> 			#address-cells = <1>;
> >>> 			#size-cells = <0>;
> >>> 			step@0 {
> >>> 				reg = <0>;
> >>
> >> That's precisely what I would like to avoid - I don't need the steps
> >> to be numbered and I certainly have no use for a reg property. Isn't
> >> there a way to make it simpler?
> > 
> > It's not technically valid to not have the reg property. Or
> > #address-cells and #size-cells properties for that matter.
> 
> I'm not keen on this representation where individual steps are nodes.
> That seems like it could end up being too "heavyweight" for a long sequence.

The other alternative would involve using a single property to encode
one sequence. I think that was the initial proposal, though using proper
phandle encoding it could probably be enhanced a bit. However anything
that involves a single property has the problem that we need to encode
the type of resource as an integer, and that makes things very hard to
read.

So it would look something like this:

	power-on = <1  &gpio 6 0            1
		    0                   10000
		    2  &reg                 1
		    3  &pwm  0 5000000      1>;

	power-off = <3  &pwm  0 5000000      0
		     2  &reg                 0
		     0                   10000
		     1  &gpio 6 0            0>;

So the first cell would encode the type:
  0: delay
  1: gpio
  2: regulator
  3: PWM

The next n cells would be the phandle and the specifier, while the last
cell would encode a resource-specific parameter:
  delay: time in microseconds
  gpio: set level (0: low, 1: high)
  regulator: 0: disable, 1: enable
  pwm: 0: disable, 1: enable

I guess this would be more compact, but it is also very hard to read. Is
that something you would be happier with? Perhaps you were thinking of
something completely different?

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mitch Bradley]

On 7/31/2012 8:38 PM, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote:
>> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>> I wonder if using the same structure/array as input and output would
>>>>> simplify the API; the platform data would fill in the fields mentioned
>>>>> above, and power_seq_build() would parse those, then set other fields in
>>>>> the same structs to the looked-up handle values?
>>>>
>>>> The thing is that I am not sure what happens to the platform data
>>>> once probe() is done. Isn't it customary to mark it with __devinit
>>>> and have it freed after probing is successful?
>>>
>>> No, platform data should stay around forever. Otherwise, consider what
>>> would happen if your driver is built as a module and you unload and load
>>> it again.
>>>
>>>> More generally, I think it is a good practice to have data
>>>> structures tailored right for what they need to do - code with
>>>> members that are meaningful only at given points of an instance's
>>>> life tends to be more confusing.
>>>
>>> I agree. Furthermore the driver unload/reload would be another reason
>>> not to reuse platform data as the output of the build() function.
>>>
>>> But maybe what Stephen meant was more like filling a structure with data
>>> taken from the platform data and pass that to a resolve() function which
>>> would fill in the missing pieces like pointers to actual resources. I
>>> imagine a managed interface would become a little trickier to do using
>>> such an approach.
>>>
>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>>> have to if all named "step" and there's more than one of them, then they
>>>>> will need a matching reg property. Equally, the parent node will need
>>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>>
>>>>> 		power-on-sequence {
>>>>> 			#address-cells = <1>;
>>>>> 			#size-cells = <0>;
>>>>> 			step@0 {
>>>>> 				reg = <0>;
>>>>
>>>> That's precisely what I would like to avoid - I don't need the steps
>>>> to be numbered and I certainly have no use for a reg property. Isn't
>>>> there a way to make it simpler?
>>>
>>> It's not technically valid to not have the reg property. Or
>>> #address-cells and #size-cells properties for that matter.
>>
>> I'm not keen on this representation where individual steps are nodes.
>> That seems like it could end up being too "heavyweight" for a long sequence.
> 
> The other alternative would involve using a single property to encode
> one sequence. I think that was the initial proposal, though using proper
> phandle encoding it could probably be enhanced a bit. However anything
> that involves a single property has the problem that we need to encode
> the type of resource as an integer, and that makes things very hard to
> read.
> 
> So it would look something like this:
> 
> 	power-on = <1  &gpio 6 0            1
> 		    0                   10000
> 		    2  &reg                 1
> 		    3  &pwm  0 5000000      1>;
> 
> 	power-off = <3  &pwm  0 5000000      0
> 		     2  &reg                 0
> 		     0                   10000
> 		     1  &gpio 6 0            0>;
> 
> So the first cell would encode the type:
>   0: delay
>   1: gpio
>   2: regulator
>   3: PWM
> 
> The next n cells would be the phandle and the specifier, while the last
> cell would encode a resource-specific parameter:
>   delay: time in microseconds
>   gpio: set level (0: low, 1: high)
>   regulator: 0: disable, 1: enable
>   pwm: 0: disable, 1: enable
> 
> I guess this would be more compact, but it is also very hard to read. Is
> that something you would be happier with? Perhaps you were thinking of
> something completely different?


Perhaps a compact/flexible encoding could be designed, with a textual
encoding that is easy to read.  A separate tool could convert the text
encoding to the integer format, annotated with comments containing
the "source text".  A file containing that output could be #included
into the dts file.

> 
> Thierry
> 

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 09:55 PM, Mitch Bradley wrote:
> On 7/31/2012 8:38 PM, Thierry Reding wrote:
>> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote:
>>> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>>> I wonder if using the same structure/array as input and output would
>>>>>> simplify the API; the platform data would fill in the fields mentioned
>>>>>> above, and power_seq_build() would parse those, then set other fields in
>>>>>> the same structs to the looked-up handle values?
>>>>>
>>>>> The thing is that I am not sure what happens to the platform data
>>>>> once probe() is done. Isn't it customary to mark it with __devinit
>>>>> and have it freed after probing is successful?
>>>>
>>>> No, platform data should stay around forever. Otherwise, consider what
>>>> would happen if your driver is built as a module and you unload and load
>>>> it again.
>>>>
>>>>> More generally, I think it is a good practice to have data
>>>>> structures tailored right for what they need to do - code with
>>>>> members that are meaningful only at given points of an instance's
>>>>> life tends to be more confusing.
>>>>
>>>> I agree. Furthermore the driver unload/reload would be another reason
>>>> not to reuse platform data as the output of the build() function.
>>>>
>>>> But maybe what Stephen meant was more like filling a structure with data
>>>> taken from the platform data and pass that to a resolve() function which
>>>> would fill in the missing pieces like pointers to actual resources. I
>>>> imagine a managed interface would become a little trickier to do using
>>>> such an approach.
>>>>
>>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>>>> have to if all named "step" and there's more than one of them, then they
>>>>>> will need a matching reg property. Equally, the parent node will need
>>>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>>>
>>>>>> 		power-on-sequence {
>>>>>> 			#address-cells = <1>;
>>>>>> 			#size-cells = <0>;
>>>>>> 			step@0 {
>>>>>> 				reg = <0>;
>>>>>
>>>>> That's precisely what I would like to avoid - I don't need the steps
>>>>> to be numbered and I certainly have no use for a reg property. Isn't
>>>>> there a way to make it simpler?
>>>>
>>>> It's not technically valid to not have the reg property. Or
>>>> #address-cells and #size-cells properties for that matter.
>>>
>>> I'm not keen on this representation where individual steps are nodes.
>>> That seems like it could end up being too "heavyweight" for a long sequence.
>>
>> The other alternative would involve using a single property to encode
>> one sequence. I think that was the initial proposal, though using proper
>> phandle encoding it could probably be enhanced a bit. However anything
>> that involves a single property has the problem that we need to encode
>> the type of resource as an integer, and that makes things very hard to
>> read.
>>
>> So it would look something like this:
>>
>> 	power-on = <1  &gpio 6 0            1
>> 		    0                   10000
>> 		    2  &reg                 1
>> 		    3  &pwm  0 5000000      1>;
>>
>> 	power-off = <3  &pwm  0 5000000      0
>> 		     2  &reg                 0
>> 		     0                   10000
>> 		     1  &gpio 6 0            0>;
>>
>> So the first cell would encode the type:
>>    0: delay
>>    1: gpio
>>    2: regulator
>>    3: PWM
>>
>> The next n cells would be the phandle and the specifier, while the last
>> cell would encode a resource-specific parameter:
>>    delay: time in microseconds
>>    gpio: set level (0: low, 1: high)
>>    regulator: 0: disable, 1: enable
>>    pwm: 0: disable, 1: enable
>>
>> I guess this would be more compact, but it is also very hard to read. Is
>> that something you would be happier with? Perhaps you were thinking of
>> something completely different?
>
>
> Perhaps a compact/flexible encoding could be designed, with a textual
> encoding that is easy to read.  A separate tool could convert the text
> encoding to the integer format, annotated with comments containing
> the "source text".  A file containing that output could be #included
> into the dts file.

Do you mean having a external compiler that would run before dtc just 
for producing the power sequences? That sounds a little bit overkill for 
something that ough to remain simple.

Also, although I admit I don't have the whole picture of where they 
could be used, I don't expect the power sequences to grow to sizes that 
would make us bother about their footprint.

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mitch Bradley]

On 8/1/2012 9:47 AM, Alex Courbot wrote:
> On 07/31/2012 09:55 PM, Mitch Bradley wrote:
>> On 7/31/2012 8:38 PM, Thierry Reding wrote:
>>> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote:
>>>> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>>>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>>>> I wonder if using the same structure/array as input and output would
>>>>>>> simplify the API; the platform data would fill in the fields mentioned
>>>>>>> above, and power_seq_build() would parse those, then set other fields in
>>>>>>> the same structs to the looked-up handle values?
>>>>>>
>>>>>> The thing is that I am not sure what happens to the platform data
>>>>>> once probe() is done. Isn't it customary to mark it with __devinit
>>>>>> and have it freed after probing is successful?
>>>>>
>>>>> No, platform data should stay around forever. Otherwise, consider what
>>>>> would happen if your driver is built as a module and you unload and load
>>>>> it again.
>>>>>
>>>>>> More generally, I think it is a good practice to have data
>>>>>> structures tailored right for what they need to do - code with
>>>>>> members that are meaningful only at given points of an instance's
>>>>>> life tends to be more confusing.
>>>>>
>>>>> I agree. Furthermore the driver unload/reload would be another reason
>>>>> not to reuse platform data as the output of the build() function.
>>>>>
>>>>> But maybe what Stephen meant was more like filling a structure with data
>>>>> taken from the platform data and pass that to a resolve() function which
>>>>> would fill in the missing pieces like pointers to actual resources. I
>>>>> imagine a managed interface would become a little trickier to do using
>>>>> such an approach.
>>>>>
>>>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>>>>> have to if all named "step" and there's more than one of them, then they
>>>>>>> will need a matching reg property. Equally, the parent node will need
>>>>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>>>>
>>>>>>> 		power-on-sequence {
>>>>>>> 			#address-cells = <1>;
>>>>>>> 			#size-cells = <0>;
>>>>>>> 			step@0 {
>>>>>>> 				reg = <0>;
>>>>>>
>>>>>> That's precisely what I would like to avoid - I don't need the steps
>>>>>> to be numbered and I certainly have no use for a reg property. Isn't
>>>>>> there a way to make it simpler?
>>>>>
>>>>> It's not technically valid to not have the reg property. Or
>>>>> #address-cells and #size-cells properties for that matter.
>>>>
>>>> I'm not keen on this representation where individual steps are nodes.
>>>> That seems like it could end up being too "heavyweight" for a long sequence.
>>>
>>> The other alternative would involve using a single property to encode
>>> one sequence. I think that was the initial proposal, though using proper
>>> phandle encoding it could probably be enhanced a bit. However anything
>>> that involves a single property has the problem that we need to encode
>>> the type of resource as an integer, and that makes things very hard to
>>> read.
>>>
>>> So it would look something like this:
>>>
>>> 	power-on = <1  &gpio 6 0            1
>>> 		    0                   10000
>>> 		    2  &reg                 1
>>> 		    3  &pwm  0 5000000      1>;
>>>
>>> 	power-off = <3  &pwm  0 5000000      0
>>> 		     2  &reg                 0
>>> 		     0                   10000
>>> 		     1  &gpio 6 0            0>;
>>>
>>> So the first cell would encode the type:
>>>    0: delay
>>>    1: gpio
>>>    2: regulator
>>>    3: PWM
>>>
>>> The next n cells would be the phandle and the specifier, while the last
>>> cell would encode a resource-specific parameter:
>>>    delay: time in microseconds
>>>    gpio: set level (0: low, 1: high)
>>>    regulator: 0: disable, 1: enable
>>>    pwm: 0: disable, 1: enable
>>>
>>> I guess this would be more compact, but it is also very hard to read. Is
>>> that something you would be happier with? Perhaps you were thinking of
>>> something completely different?
>>
>>
>> Perhaps a compact/flexible encoding could be designed, with a textual
>> encoding that is easy to read.  A separate tool could convert the text
>> encoding to the integer format, annotated with comments containing
>> the "source text".  A file containing that output could be #included
>> into the dts file.
> 
> Do you mean having a external compiler that would run before dtc just 
> for producing the power sequences? That sounds a little bit overkill for 
> something that ough to remain simple.
> 
> Also, although I admit I don't have the whole picture of where they 
> could be used, I don't expect the power sequences to grow to sizes that 
> would make us bother about their footprint.

It is axiomatic that every "language", if it succeeds at all, eventually
grows into a complete programming language.  The more special-purpose
that it starts as, the uglier that it ends up.

> 
> Alex.
> 

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 09:22 PM, Mitch Bradley wrote:
> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>> I wonder if using the same structure/array as input and output would
>>>> simplify the API; the platform data would fill in the fields mentioned
>>>> above, and power_seq_build() would parse those, then set other fields in
>>>> the same structs to the looked-up handle values?
>>>
>>> The thing is that I am not sure what happens to the platform data
>>> once probe() is done. Isn't it customary to mark it with __devinit
>>> and have it freed after probing is successful?
>>
>> No, platform data should stay around forever. Otherwise, consider what
>> would happen if your driver is built as a module and you unload and load
>> it again.
>>
>>> More generally, I think it is a good practice to have data
>>> structures tailored right for what they need to do - code with
>>> members that are meaningful only at given points of an instance's
>>> life tends to be more confusing.
>>
>> I agree. Furthermore the driver unload/reload would be another reason
>> not to reuse platform data as the output of the build() function.
>>
>> But maybe what Stephen meant was more like filling a structure with data
>> taken from the platform data and pass that to a resolve() function which
>> would fill in the missing pieces like pointers to actual resources. I
>> imagine a managed interface would become a little trickier to do using
>> such an approach.
>>
>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>> have to if all named "step" and there's more than one of them, then they
>>>> will need a matching reg property. Equally, the parent node will need
>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>
>>>> 		power-on-sequence {
>>>> 			#address-cells = <1>;
>>>> 			#size-cells = <0>;
>>>> 			step@0 {
>>>> 				reg = <0>;
>>>
>>> That's precisely what I would like to avoid - I don't need the steps
>>> to be numbered and I certainly have no use for a reg property. Isn't
>>> there a way to make it simpler?
>>
>> It's not technically valid to not have the reg property. Or
>> #address-cells and #size-cells properties for that matter.
>
> I'm not keen on this representation where individual steps are nodes.
> That seems like it could end up being too "heavyweight" for a long sequence.

Using nodes has a big advantage though: we can use any arbitrary 
property to add extra parameters to the resource being controlled. Right 
now we only use enable/disable, but for example one can imagine an 
optional voltage setting for a regulator. It is much more future-proof 
than a design where the number of parameters would be fixed and could 
not be extended without breaking compatibility.

I experimented encoding the whole sequence within a single property. It 
works of course, but is not really flexible, hard to read, and quite 
error-prone overall. The memory footprint gain was not so obvious 
neither (although it was indeed more compact).

What bothers me is to have to specify the cells layout and reg property 
for *every single sequence*, but well, I guess we can live with that.

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 12:56:40PM +0200, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:

> > The thing is that I am not sure what happens to the platform data
> > once probe() is done. Isn't it customary to mark it with __devinit
> > and have it freed after probing is successful?

> No, platform data should stay around forever. Otherwise, consider what
> would happen if your driver is built as a module and you unload and load
> it again.

__devinit can be discarded if you disable enough kernel features,
HOTPLUG is the main one IIRC, modules might also need to go - drivers
really ought to take a copy of platform data they plan to use at
runtime, though practically speaking you have to try to trigger any
problems.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 03:13:29PM +0100, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 12:56:40PM +0200, Thierry Reding wrote:
> > On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
> 
> > > The thing is that I am not sure what happens to the platform data
> > > once probe() is done. Isn't it customary to mark it with __devinit
> > > and have it freed after probing is successful?
> 
> > No, platform data should stay around forever. Otherwise, consider what
> > would happen if your driver is built as a module and you unload and load
> > it again.
> 
> __devinit can be discarded if you disable enough kernel features,
> HOTPLUG is the main one IIRC, modules might also need to go - drivers
> really ought to take a copy of platform data they plan to use at
> runtime, though practically speaking you have to try to trigger any
> problems.

HOTPLUG is marked EXPERT and explicitly states that it should only be
disabled if you're not using modules or dynamic device discovery. I
think if you've ignored all of that you're no longer entitled to
complain.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 04:22:17PM +0200, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 03:13:29PM +0100, Mark Brown wrote:

> > __devinit can be discarded if you disable enough kernel features,
> > HOTPLUG is the main one IIRC, modules might also need to go - drivers
> > really ought to take a copy of platform data they plan to use at
> > runtime, though practically speaking you have to try to trigger any
> > problems.

> HOTPLUG is marked EXPERT and explicitly states that it should only be
> disabled if you're not using modules or dynamic device discovery. I
> think if you've ignored all of that you're no longer entitled to
> complain.

This is framework code - it doesn't have much option.  Disabling HOTPLUG
is totally reasonable on space constrained systems, there's no reason
for the code to break things for people.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 04:22:17PM +0200, Thierry Reding wrote:
> > On Tue, Jul 31, 2012 at 03:13:29PM +0100, Mark Brown wrote:
> 
> > > __devinit can be discarded if you disable enough kernel features,
> > > HOTPLUG is the main one IIRC, modules might also need to go - drivers
> > > really ought to take a copy of platform data they plan to use at
> > > runtime, though practically speaking you have to try to trigger any
> > > problems.
> 
> > HOTPLUG is marked EXPERT and explicitly states that it should only be
> > disabled if you're not using modules or dynamic device discovery. I
> > think if you've ignored all of that you're no longer entitled to
> > complain.
> 
> This is framework code - it doesn't have much option.  Disabling HOTPLUG
> is totally reasonable on space constrained systems, there's no reason
> for the code to break things for people.

Still if you use this code and disable HOTPLUG, then you shouldn't be
using modules either. I mean there is no way you can write a driver that
can gracefully handle its platform data being discarded.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote:

> > This is framework code - it doesn't have much option.  Disabling HOTPLUG
> > is totally reasonable on space constrained systems, there's no reason
> > for the code to break things for people.

> Still if you use this code and disable HOTPLUG, then you shouldn't be
> using modules either. I mean there is no way you can write a driver that

Of course.

> can gracefully handle its platform data being discarded.

Sure there is - take a copy of the platform data in probe().

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Greg Kroah-Hartman]

On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote:
> > On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote:
> 
> > > This is framework code - it doesn't have much option.  Disabling HOTPLUG
> > > is totally reasonable on space constrained systems, there's no reason
> > > for the code to break things for people.
> 
> > Still if you use this code and disable HOTPLUG, then you shouldn't be
> > using modules either. I mean there is no way you can write a driver that
> 
> Of course.
> 
> > can gracefully handle its platform data being discarded.
> 
> Sure there is - take a copy of the platform data in probe().

Ick ick ick, devdata needs to die as it's pretty much pointless these
days.

It's on my TODO list actually...

greg k-h

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 09:19:54AM -0700, Greg Kroah-Hartman wrote:
> On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote:
> > On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote:

> > > can gracefully handle its platform data being discarded.

> > Sure there is - take a copy of the platform data in probe().

> Ick ick ick, devdata needs to die as it's pretty much pointless these
> days.

Hrm?  I'm not sure I understand the direct relevance here - we're
talking about platform data.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Greg Kroah-Hartman]

On Tue, Jul 31, 2012 at 05:22:30PM +0100, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 09:19:54AM -0700, Greg Kroah-Hartman wrote:
> > On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote:
> > > On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote:
> 
> > > > can gracefully handle its platform data being discarded.
> 
> > > Sure there is - take a copy of the platform data in probe().
> 
> > Ick ick ick, devdata needs to die as it's pretty much pointless these
> > days.
> 
> Hrm?  I'm not sure I understand the direct relevance here - we're
> talking about platform data.

The platform data was marked __devdata, and you said it could be thrown
away after init so you should keep it around somehow.

Anyway, not really the main point here at all, sorry for the noise...

greg k-h

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Tue, Jul 31, 2012 at 09:42:45AM -0700, Greg Kroah-Hartman wrote:
> On Tue, Jul 31, 2012 at 05:22:30PM +0100, Mark Brown wrote:

> > Hrm?  I'm not sure I understand the direct relevance here - we're
> > talking about platform data.

> The platform data was marked __devdata, and you said it could be thrown
> away after init so you should keep it around somehow.

Ah, sorry - I was thinking __devinitdata.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote:
> On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote:
> > On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote:
> 
> > > This is framework code - it doesn't have much option.  Disabling HOTPLUG
> > > is totally reasonable on space constrained systems, there's no reason
> > > for the code to break things for people.
> 
> > Still if you use this code and disable HOTPLUG, then you shouldn't be
> > using modules either. I mean there is no way you can write a driver that
> 
> Of course.
> 
> > can gracefully handle its platform data being discarded.
> 
> Sure there is - take a copy of the platform data in probe().

Yes, but that will only work for built-in drivers. If you unload the
module and that causes the platform data to be discarded, reloading
won't work.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Wed, Aug 01, 2012 at 09:41:13AM +0200, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote:

> > Sure there is - take a copy of the platform data in probe().

> Yes, but that will only work for built-in drivers. If you unload the
> module and that causes the platform data to be discarded, reloading
> won't work.

This is why __devinit data will only be discarded when this is not
possible.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 09:41:13AM +0200, Thierry Reding wrote:
> > On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote:
> 
> > > Sure there is - take a copy of the platform data in probe().
> 
> > Yes, but that will only work for built-in drivers. If you unload the
> > module and that causes the platform data to be discarded, reloading
> > won't work.
> 
> This is why __devinit data will only be discarded when this is not
> possible.

That's exactly my point. But I seem to have miserably failed to get that
across. =)

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Wed, Aug 01, 2012 at 03:38:14PM +0200, Thierry Reding wrote:
> On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote:

> > This is why __devinit data will only be discarded when this is not
> > possible.

> That's exactly my point. But I seem to have miserably failed to get that
> across. =)

We must be talking at cross purposes then.  What I'm saying is that the
framework shouldn't rely on platform data and should assume that the
kernel might be configured so it can be discarded (by copying most
likely).

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Wed, Aug 01, 2012 at 02:55:31PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 03:38:14PM +0200, Thierry Reding wrote:
> > On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote:
> 
> > > This is why __devinit data will only be discarded when this is not
> > > possible.
> 
> > That's exactly my point. But I seem to have miserably failed to get that
> > across. =)
> 
> We must be talking at cross purposes then.  What I'm saying is that the
> framework shouldn't rely on platform data and should assume that the
> kernel might be configured so it can be discarded (by copying most
> likely).

Yes. I think this should be solved by the power_seq_build() function
which uses the platform data description of the sequence and builds it
into something for internal use that doesn't rely on anything in the
platform data.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Stephen Warren]

On 07/31/2012 04:32 AM, Alex Courbot wrote:
> On 07/31/2012 07:45 AM, Stephen Warren wrote:
...
>> If the nodes have a unit address (i.e. end in "@n"), which they will
>> have to if all named "step" and there's more than one of them, then they
>> will need a matching reg property. Equally, the parent node will need
>> #address-cells and #size-cells too. So, the last couple lines would be:
>>
>>         power-on-sequence {
>>             #address-cells = <1>;
>>             #size-cells = <0>;
>>             step@0 {
>>                 reg = <0>;
> 
> That's precisely what I would like to avoid - I don't need the steps to
> be numbered and I certainly have no use for a reg property. Isn't there
> a way to make it simpler?

You may be able to get away without using the reg values in the code.
However, to have a semantically correct device tree, you really do need
all of those properties.

That said, I think you might need to use the reg values in code. I
believe there's no guarantee of the order in which nodes enumerate in
device tree, so you need to look at the reg property in order to find
the order in which to execute the nodes/steps.

... although perhaps the desire to avoid a dependency on DT ordering
applies more to the order in which devices instantiated from DT get
probed, rather than the order of low-level node enumeration, so I may be
wrong here. I've definitely seen Grant caution people not to rely on
device probe order, but perhaps he didn't make the same comment about
node order.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 07/31/2012 07:45 AM, Stephen Warren wrote:
> Oh I see. That's a little confusing. Why not just reference the relevant
> resources directly in each step; something more like:
>
> 		gpio@1 {
> 			action = "enable-gpio";
> 			gpio = <&gpio 1 0>;
> 		};
>
> I guess that might make parsing/building a little harder, since you'd
> have to detect when you'd already done gpio_request() on a given GPIO
> and not repeat it or something like that, but to me this makes the DT a
> lot easier to comprehend.

I tried to move towards having the phandles directly in the sequences 
themselves - that reminded me why I did not do that in the first place. 
Let's say we have a sequence like this (reg property omitted on purpose):

	power-on-sequence {
		step@0 {
			regulator = <&backlight_reg>;
			enable;
		};
		step@1 {
			delay = <10000>;
		};
		step@2 {
			pwm = <&pwm 2 5000000>;
			enable;
		};
		step@3 {
			gpio = <&gpio 28 0>;
			enable;
		};
	};

The problem is, how do we turn these phandles into the resource of 
interest. The type of the resource can be infered by the name of the 
property. The hard part is resolving the resource from the phandle - it 
seems like the API just does not allow to do this. GPIO has 
of_get_named_gpio, but AFAIK there are no equivalent for regulator 
consumer and PWM: the only way to use the DT with them is through 
get_regulator and get_pwm which work at the device level.

Or is there a way that I overlooked?

Thanks,
Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >Oh I see. That's a little confusing. Why not just reference the relevant
> >resources directly in each step; something more like:
> >
> >		gpio@1 {
> >			action = "enable-gpio";
> >			gpio = <&gpio 1 0>;
> >		};
> >
> >I guess that might make parsing/building a little harder, since you'd
> >have to detect when you'd already done gpio_request() on a given GPIO
> >and not repeat it or something like that, but to me this makes the DT a
> >lot easier to comprehend.
> 
> I tried to move towards having the phandles directly in the
> sequences themselves - that reminded me why I did not do that in the
> first place. Let's say we have a sequence like this (reg property
> omitted on purpose):
> 
> 	power-on-sequence {
> 		step@0 {
> 			regulator = <&backlight_reg>;
> 			enable;
> 		};
> 		step@1 {
> 			delay = <10000>;
> 		};
> 		step@2 {
> 			pwm = <&pwm 2 5000000>;
> 			enable;
> 		};
> 		step@3 {
> 			gpio = <&gpio 28 0>;
> 			enable;
> 		};
> 	};
> 
> The problem is, how do we turn these phandles into the resource of
> interest. The type of the resource can be infered by the name of the
> property. The hard part is resolving the resource from the phandle -
> it seems like the API just does not allow to do this. GPIO has
> of_get_named_gpio, but AFAIK there are no equivalent for regulator
> consumer and PWM: the only way to use the DT with them is through
> get_regulator and get_pwm which work at the device level.
> 
> Or is there a way that I overlooked?

No, you are right. Perhaps we should add exported functions that do the
equivalent of of_pwm_request() or the regulator_dev_lookup() and
of_get_regulator() pair.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>> Oh I see. That's a little confusing. Why not just reference the relevant
>>> resources directly in each step; something more like:
>>>
>>> 		gpio@1 {
>>> 			action = "enable-gpio";
>>> 			gpio = <&gpio 1 0>;
>>> 		};
>>>
>>> I guess that might make parsing/building a little harder, since you'd
>>> have to detect when you'd already done gpio_request() on a given GPIO
>>> and not repeat it or something like that, but to me this makes the DT a
>>> lot easier to comprehend.
>>
>> I tried to move towards having the phandles directly in the
>> sequences themselves - that reminded me why I did not do that in the
>> first place. Let's say we have a sequence like this (reg property
>> omitted on purpose):
>>
>> 	power-on-sequence {
>> 		step@0 {
>> 			regulator = <&backlight_reg>;
>> 			enable;
>> 		};
>> 		step@1 {
>> 			delay = <10000>;
>> 		};
>> 		step@2 {
>> 			pwm = <&pwm 2 5000000>;
>> 			enable;
>> 		};
>> 		step@3 {
>> 			gpio = <&gpio 28 0>;
>> 			enable;
>> 		};
>> 	};
>>
>> The problem is, how do we turn these phandles into the resource of
>> interest. The type of the resource can be infered by the name of the
>> property. The hard part is resolving the resource from the phandle -
>> it seems like the API just does not allow to do this. GPIO has
>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>> consumer and PWM: the only way to use the DT with them is through
>> get_regulator and get_pwm which work at the device level.
>>
>> Or is there a way that I overlooked?
>
> No, you are right. Perhaps we should add exported functions that do the
> equivalent of of_pwm_request() or the regulator_dev_lookup() and
> of_get_regulator() pair.

How would that be looked with respect to "good DT practices"? I can 
somehow understand the wish to restrain DT access to these functions 
that integrate well with current workflows. Aren't we going to be 
frowned upon if we make more low-level functions public?

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

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

On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote:
> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> >>On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >>>Oh I see. That's a little confusing. Why not just reference the relevant
> >>>resources directly in each step; something more like:
> >>>
> >>>		gpio@1 {
> >>>			action = "enable-gpio";
> >>>			gpio = <&gpio 1 0>;
> >>>		};
> >>>
> >>>I guess that might make parsing/building a little harder, since you'd
> >>>have to detect when you'd already done gpio_request() on a given GPIO
> >>>and not repeat it or something like that, but to me this makes the DT a
> >>>lot easier to comprehend.
> >>
> >>I tried to move towards having the phandles directly in the
> >>sequences themselves - that reminded me why I did not do that in the
> >>first place. Let's say we have a sequence like this (reg property
> >>omitted on purpose):
> >>
> >>	power-on-sequence {
> >>		step@0 {
> >>			regulator = <&backlight_reg>;
> >>			enable;
> >>		};
> >>		step@1 {
> >>			delay = <10000>;
> >>		};
> >>		step@2 {
> >>			pwm = <&pwm 2 5000000>;
> >>			enable;
> >>		};
> >>		step@3 {
> >>			gpio = <&gpio 28 0>;
> >>			enable;
> >>		};
> >>	};
> >>
> >>The problem is, how do we turn these phandles into the resource of
> >>interest. The type of the resource can be infered by the name of the
> >>property. The hard part is resolving the resource from the phandle -
> >>it seems like the API just does not allow to do this. GPIO has
> >>of_get_named_gpio, but AFAIK there are no equivalent for regulator
> >>consumer and PWM: the only way to use the DT with them is through
> >>get_regulator and get_pwm which work at the device level.
> >>
> >>Or is there a way that I overlooked?
> >
> >No, you are right. Perhaps we should add exported functions that do the
> >equivalent of of_pwm_request() or the regulator_dev_lookup() and
> >of_get_regulator() pair.
> 
> How would that be looked with respect to "good DT practices"? I can
> somehow understand the wish to restrain DT access to these functions
> that integrate well with current workflows. Aren't we going to be
> frowned upon if we make more low-level functions public?

Yes, I think that's exactly what will happen.

Thierry

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

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On Thu 02 Aug 2012 05:45:41 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote:
>> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>> Oh I see. That's a little confusing. Why not just reference the relevant
>>>>> resources directly in each step; something more like:
>>>>>
>>>>> 		gpio@1 {
>>>>> 			action = "enable-gpio";
>>>>> 			gpio = <&gpio 1 0>;
>>>>> 		};
>>>>>
>>>>> I guess that might make parsing/building a little harder, since you'd
>>>>> have to detect when you'd already done gpio_request() on a given GPIO
>>>>> and not repeat it or something like that, but to me this makes the DT a
>>>>> lot easier to comprehend.
>>>>
>>>> I tried to move towards having the phandles directly in the
>>>> sequences themselves - that reminded me why I did not do that in the
>>>> first place. Let's say we have a sequence like this (reg property
>>>> omitted on purpose):
>>>>
>>>> 	power-on-sequence {
>>>> 		step@0 {
>>>> 			regulator = <&backlight_reg>;
>>>> 			enable;
>>>> 		};
>>>> 		step@1 {
>>>> 			delay = <10000>;
>>>> 		};
>>>> 		step@2 {
>>>> 			pwm = <&pwm 2 5000000>;
>>>> 			enable;
>>>> 		};
>>>> 		step@3 {
>>>> 			gpio = <&gpio 28 0>;
>>>> 			enable;
>>>> 		};
>>>> 	};
>>>>
>>>> The problem is, how do we turn these phandles into the resource of
>>>> interest. The type of the resource can be infered by the name of the
>>>> property. The hard part is resolving the resource from the phandle -
>>>> it seems like the API just does not allow to do this. GPIO has
>>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>>>> consumer and PWM: the only way to use the DT with them is through
>>>> get_regulator and get_pwm which work at the device level.
>>>>
>>>> Or is there a way that I overlooked?
>>>
>>> No, you are right. Perhaps we should add exported functions that do the
>>> equivalent of of_pwm_request() or the regulator_dev_lookup() and
>>> of_get_regulator() pair.
>>
>> How would that be looked with respect to "good DT practices"? I can
>> somehow understand the wish to restrain DT access to these functions
>> that integrate well with current workflows. Aren't we going to be
>> frowned upon if we make more low-level functions public?
>
> Yes, I think that's exactly what will happen.

Probably not worth to try this then?

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote:
> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:

> > The problem is, how do we turn these phandles into the resource of
> > interest. The type of the resource can be infered by the name of the
> > property. The hard part is resolving the resource from the phandle -
> > it seems like the API just does not allow to do this. GPIO has
> > of_get_named_gpio, but AFAIK there are no equivalent for regulator
> > consumer and PWM: the only way to use the DT with them is through
> > get_regulator and get_pwm which work at the device level.

> > Or is there a way that I overlooked?

> No, you are right. Perhaps we should add exported functions that do the
> equivalent of of_pwm_request() or the regulator_dev_lookup() and
> of_get_regulator() pair.

I missed some of the earlier bits of the thread here but why can't we do
device based lookups?

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
> On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote:
>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>
>>> The problem is, how do we turn these phandles into the resource of
>>> interest. The type of the resource can be infered by the name of the
>>> property. The hard part is resolving the resource from the phandle -
>>> it seems like the API just does not allow to do this. GPIO has
>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>>> consumer and PWM: the only way to use the DT with them is through
>>> get_regulator and get_pwm which work at the device level.
>
>>> Or is there a way that I overlooked?
>
>> No, you are right. Perhaps we should add exported functions that do the
>> equivalent of of_pwm_request() or the regulator_dev_lookup() and
>> of_get_regulator() pair.
>
> I missed some of the earlier bits of the thread here but why can't we do
> device based lookups?

That is because the phandles would not be properties of the device node 
but rather of its sub-nodes:

backlight {
     compatible = "pwm-backlight";
     ...
     power-on-sequence {
         step@0 {
             regulator = <&backlight_reg>;
             enable;
         };


So here simply using regulator_get on the backlight device would not work.

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Mark Brown]

On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:

> >I missed some of the earlier bits of the thread here but why can't we do
> >device based lookups?

> That is because the phandles would not be properties of the device
> node but rather of its sub-nodes:

> backlight {
>     compatible = "pwm-backlight";
>     ...
>     power-on-sequence {
>         step@0 {
>             regulator = <&backlight_reg>;
>             enable;
>         };
> 

> So here simply using regulator_get on the backlight device would not work.

Ah, right.  DT isn't being terribly helpful here...  I think the thing
I'd expect to work here is that you have a reference back to the supply
property of the backlight device rather than direct to the regulator so
you end up writing "enable supply X" rather than "enable regulator X".

Not quite sure how exactly you'd accomplish that - I guess if
regulator_get() would recursively follow phandles until it hits a node
that'd do the trick?

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 08/04/2012 11:12 PM, Mark Brown wrote:
> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
>
>>> I missed some of the earlier bits of the thread here but why can't we do
>>> device based lookups?
>
>> That is because the phandles would not be properties of the device
>> node but rather of its sub-nodes:
>
>> backlight {
>>      compatible = "pwm-backlight";
>>      ...
>>      power-on-sequence {
>>          step@0 {
>>              regulator = <&backlight_reg>;
>>              enable;
>>          };
>>
>
>> So here simply using regulator_get on the backlight device would not work.
>
> Ah, right.  DT isn't being terribly helpful here...  I think the thing
> I'd expect to work here is that you have a reference back to the supply
> property of the backlight device rather than direct to the regulator so
> you end up writing "enable supply X" rather than "enable regulator X".
>
> Not quite sure how exactly you'd accomplish that - I guess if
> regulator_get() would recursively follow phandles until it hits a node
> that'd do the trick?

Do you mean that regulator_get() would parse sub-nodes looking for a 
match? That seems rather dangerous and error-prone, especially if one 
has actual devices within the sub-nodes - their regulators could be 
stolen by the parent device.

I think we only have two choices for this:

1) Stick to the scheme where resources are declared at the device level, 
such as they can be referenced by name in the sub-nodes (basically what 
I did in this patch):

backlight {
      compatible = "pwm-backlight";
      ...
      backlight-supply = <&backlight_reg>;

      power-on-sequence {
          step@0 {
              regulator = "backlight";
              enable;
          };

This would translate by a get_regulator(dev, "backlight") in the code 
which would be properly resolved.

2) Export a lower-level DT API for resolving phandles directly from a 
property, similar to of_get_named_gpio. We would then have 
of_get_named_regulator and of_get_named_pwm.

If 2) is deemed acceptable, then I think we should go for it as it would 
provide the most compact and readable DT syntax. Otherwise 1) is still 
acceptable IMHO, as it should at least make sense to people already 
familiar with how the DT works.

Opinions from DT experts?

Alex.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Stephen Warren]

On 08/05/2012 08:27 PM, Alex Courbot wrote:
> On 08/04/2012 11:12 PM, Mark Brown wrote:
>> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
>>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
>>
>>>> I missed some of the earlier bits of the thread here but why can't
>>>> we do
>>>> device based lookups?
...
> I think we only have two choices for this:
> 
> 1) Stick to the scheme where resources are declared at the device level,
> such as they can be referenced by name in the sub-nodes (basically what
> I did in this patch):
> 
> backlight {
>      compatible = "pwm-backlight";
>      ...
>      backlight-supply = <&backlight_reg>;
> 
>      power-on-sequence {
>          step@0 {
>              regulator = "backlight";
>              enable;
>          };
> 
> This would translate by a get_regulator(dev, "backlight") in the code
> which would be properly resolved.

Yes, upon reflection, that scheme does make sense. I withdraw the
comments I made re: whether it'd be better to just stick the phandles
into the steps.

Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Alex Courbot]

On 08/07/2012 01:16 AM, Stephen Warren wrote:
> On 08/05/2012 08:27 PM, Alex Courbot wrote:
>> On 08/04/2012 11:12 PM, Mark Brown wrote:
>>> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
>>>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
>>>
>>>>> I missed some of the earlier bits of the thread here but why can't
>>>>> we do
>>>>> device based lookups?
> ...
>> I think we only have two choices for this:
>>
>> 1) Stick to the scheme where resources are declared at the device level,
>> such as they can be referenced by name in the sub-nodes (basically what
>> I did in this patch):
>>
>> backlight {
>>       compatible = "pwm-backlight";
>>       ...
>>       backlight-supply = <&backlight_reg>;
>>
>>       power-on-sequence {
>>           step@0 {
>>               regulator = "backlight";
>>               enable;
>>           };
>>
>> This would translate by a get_regulator(dev, "backlight") in the code
>> which would be properly resolved.
>
> Yes, upon reflection, that scheme does make sense. I withdraw the
> comments I made re: whether it'd be better to just stick the phandles
> into the steps.

Right - having the phandles directly in the sequences has its merits, 
but logically speaking resources are related to a device, so this 
declarative approach is probably closer to reality anyway.

I will revise the patch according to all the feedback received and 
submit a new version soon.

Thanks,
Alex.

[RFC][PATCH v3 2/3] pwm_backlight: use power sequences

[Alexandre Courbot]

Make use of the power sequences specified in the device tree or platform
data, if any.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/video/backlight/pwm-backlight.txt     |  55 +++++-
 drivers/video/backlight/Kconfig                    |   1 +
 drivers/video/backlight/pwm_bl.c                   | 213 +++++++++++++++------
 include/linux/pwm_backlight.h                      |  18 +-
 4 files changed, 225 insertions(+), 62 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..59abeba 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,6 @@ pwm-backlight bindings
 
 Required properties:
   - compatible: "pwm-backlight"
-  - pwms: OF device-tree PWM specification (see PWM binding[0])
   - brightness-levels: Array of distinct brightness levels. Typically these
       are in the range from 0 to 255, but any range starting at 0 will do.
       The actual brightness level (PWM duty cycle) will be interpolated
@@ -10,10 +9,22 @@ Required properties:
       last value in the array represents a 100% duty cycle (brightest).
   - default-brightness-level: the default brightness level (index into the
       array defined by the "brightness-levels" property)
+  - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+      must be specified
+  - pwm-names: a list of names for the PWM devices specified in the
+      "pwms" property (see PWM binding[0])
+  - power-on-sequence: Power sequence that will bring the backlight on. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
+  - power-off-sequence: Power sequence that will bring the backlight off. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
 
 Optional properties:
-  - pwm-names: a list of names for the PWM devices specified in the
-               "pwms" property (see PWM binding[0])
+  - *-supply: regulators used within a power sequence
+  - *-gpio: GPIOs used within a power sequence
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -21,8 +32,42 @@ Example:
 
 	backlight {
 		compatible = "pwm-backlight";
-		pwms = <&pwm 0 5000000>;
-
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+
+		pwms = <&pwm 0 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+		enable-gpio = <&gpio 6 0>;
+
+		power-on-sequence {
+			regulator@0 {
+				id = "power";
+				enable;
+				post-delay = <10>;
+			};
+			pwm@1 {
+				id = "backlight";
+				enable;
+			};
+			gpio@2 {
+				id = "enable-gpio";
+				enable;
+			};
+		};
+		power-off-sequence {
+			gpio@0 {
+				id = "enable-gpio";
+				disable;
+			};
+			pwm@1 {
+				id = "backlight";
+				disable;
+			};
+			regulator@2 {
+				id = "power";
+				disable;
+				pre-delay = <10>;
+			};
+		};
 	};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index cf28276..6fb8aa3 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -246,6 +246,7 @@ config BACKLIGHT_CARILLO_RANCH
 config BACKLIGHT_PWM
 	tristate "Generic PWM based Backlight Driver"
 	depends on PWM
+	select POWER_SEQ
 	help
 	  If you have a LCD backlight adjustable by PWM, say Y to enable
 	  this driver.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 995f016..6e3a49e 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;
+	power_seq_resources	resources;
+	power_seq		*power_on_seq;
+	power_seq		*power_off_seq;
+
+	/* Legacy callbacks */
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -35,6 +41,34 @@ 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 ((ret = power_seq_run(pb->dev, pb->power_on_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power on sequence\n");
+
+	pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (!pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->dev, pb->power_off_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power off sequence\n");
+
+	pb->enabled = false;
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +85,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 +99,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,12 +178,16 @@ 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.
-	 */
+	data->power_on_seq = of_parse_power_seq(dev, of_find_node_by_name(node,
+							"power-on-sequence"));
+	if (IS_ERR(data->power_on_seq))
+		return PTR_ERR(data->power_on_seq);
+	data->power_off_seq = of_parse_power_seq(dev, of_find_node_by_name(node,
+							"power-off-sequence"));
+	if (IS_ERR(data->power_off_seq))
+		return PTR_ERR(data->power_off_seq);
 
+	data->use_power_sequences = true;
 	return 0;
 }
 
@@ -168,73 +205,141 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+/**
+ * Construct the power sequences corresponding to the legacy platform data.
+ */
+static int pwm_backlight_legacy_probe(struct platform_device *pdev,
+				      struct pwm_bl_data *pb)
+{
+	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct power_seq_resource *res;
+	struct power_seq_step *step;
+
+	pb->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(pb->pwm)) {
+		dev_warn(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(dev, "unable to request legacy PWM\n");
+			return PTR_ERR(pb->pwm);
+		}
+		pwm_set_period(pb->pwm, data->pwm_period_ns);
+	}
+
+	pb->notify = data->notify;
+	pb->notify_after = data->notify_after;
+	pb->check_fb = data->check_fb;
+	pb->exit = data->exit;
+	pb->dev = dev;
+
+	/* Now build the resources and sequences corresponding to this PWM */
+	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+	if (!res) return -ENOMEM;
+	res->type = POWER_SEQ_PWM;
+	res->id = "pwm-backlight";
+	res->pwm = pb->pwm;
+	list_add(&res->list, &pb->resources);
+
+	/* allocate both power on and off sequences at the same time */
+	step = devm_kzalloc(dev, sizeof(*step) * 4, GFP_KERNEL);
+	if (!step) return -ENOMEM;
+	step->resource = res;
+	memcpy(&step[2], &step[0], sizeof(*step));
+	step[0].params.enable = 1;
+	pb->power_on_seq = &step[0];
+	pb->power_off_seq = &step[2];
+
+	return 0;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
 	struct platform_pwm_backlight_data defdata;
+	struct power_seq_resource *res;
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
 	unsigned int max;
 	int ret;
 
+	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
+	if (!pb) {
+		dev_err(&pdev->dev, "no memory for state\n");
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&pb->resources);
+
+	/* using new interface or device tree */
 	if (!data) {
+		/* build platform data from device tree */
 		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
-		if (ret < 0) {
+		if (ret = -EPROBE_DEFER) {
+			return ret;
+		} else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to find platform data\n");
 			return ret;
 		}
-
 		data = &defdata;
 	}
 
-	if (data->init) {
-		ret = data->init(&pdev->dev);
+	if (!data->use_power_sequences) {
+		/* using legacy interface */
+		ret = pwm_backlight_legacy_probe(pdev, pb);
 		if (ret < 0)
 			return ret;
+	} else {
+		/* build sequences and allocate resources from platform data */
+		if (data->power_on_seq) {
+			pb->power_on_seq = power_seq_build(&pdev->dev,
+							   &pb->resources,
+							   data->power_on_seq);
+			if (IS_ERR(pb->power_on_seq))
+				return PTR_ERR(pb->power_on_seq);
+		}
+		if (data->power_off_seq) {
+			pb->power_off_seq = power_seq_build(&pdev->dev,
+							    &pb->resources,
+							   data->power_off_seq);
+			if (IS_ERR(pb->power_off_seq))
+				return PTR_ERR(pb->power_off_seq);
+		}
+
+		/* we must have exactly one PWM for this driver */
+		list_for_each_entry(res, &pb->resources, list) {
+			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;
+		}
 	}
 
-	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
-	if (!pb) {
-		dev_err(&pdev->dev, "no memory for state\n");
-		ret = -ENOMEM;
-		goto err_alloc;
+	/* from here we should have a PWM */
+	if (!pb->pwm) {
+		dev_err(&pdev->dev, "no PWM defined!\n");
+		return -EINVAL;
+	}
+
+	if (data->init) {
+		ret = data->init(&pdev->dev);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
-	} else
+	} else {
 		max = data->max_brightness;
-
-	pb->notify = data->notify;
-	pb->notify_after = data->notify_after;
-	pb->check_fb = data->check_fb;
-	pb->exit = data->exit;
-	pb->dev = &pdev->dev;
-
-	pb->pwm = pwm_get(&pdev->dev, NULL);
-	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
-		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-		if (IS_ERR(pb->pwm)) {
-			dev_err(&pdev->dev, "unable to request legacy PWM\n");
-			ret = PTR_ERR(pb->pwm);
-			goto err_alloc;
-		}
 	}
 
-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
-	/*
-	 * The DT case will set the pwm_period_ns field to 0 and store the
-	 * period, parsed from the DT, in the PWM device. For the non-DT case,
-	 * set the period from platform data.
-	 */
-	if (data->pwm_period_ns > 0)
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-
 	pb->period = pwm_get_period(pb->pwm);
 	pb->lth_brightness = data->lth_brightness * (pb->period / max);
 
@@ -246,20 +351,20 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (IS_ERR(bl)) {
 		dev_err(&pdev->dev, "failed to register backlight\n");
 		ret = PTR_ERR(bl);
-		goto err_bl;
+		goto err;
 	}
 
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);
+
 	return 0;
 
-err_bl:
-	pwm_put(pb->pwm);
-err_alloc:
+err:
 	if (data->exit)
 		data->exit(&pdev->dev);
+	power_seq_free_resources(&pb->resources);
 	return ret;
 }
 
@@ -269,9 +374,9 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
 	backlight_device_unregister(bl);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-	pwm_put(pb->pwm);
+	pwm_backlight_off(bl);
+	power_seq_free_resources(&pb->resources);
+
 	if (pb->exit)
 		pb->exit(&pdev->dev);
 	return 0;
@@ -285,8 +390,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;
@@ -323,4 +427,3 @@ module_platform_driver(pwm_backlight_driver);
 MODULE_DESCRIPTION("PWM based Backlight Driver");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:pwm-backlight");
-
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..3e8bd2c 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;
+	/* Set this to true otherwise the legacy interface will be used */
+	bool use_power_sequences;
+	/*
+	 * New interface - arrays of steps terminated by a STOP instance, or
+	 * NULL if unused.
+	 */
+	struct platform_power_seq_step *power_on_seq;
+	struct platform_power_seq_step *power_off_seq;
+	/*
+	 * Legacy interface - single PWM and callback methods to control
+	 * the power sequence. pwm_id and pwm_period_ns need only be specified
+	 * if get_pwm(dev, NULL) will return NULL.
+	 */
+	int pwm_id;
+	unsigned int pwm_period_ns;
 	int (*init)(struct device *dev);
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
-- 
1.7.11.3

[RFC][PATCH v3 3/3] tegra: add pwm backlight device tree nodes

[Alexandre Courbot]

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

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..6acdb6d 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,59 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+		enable-gpio = <&gpio 28 0>;
+
+		power-on-sequence {
+			regulator@0 {
+				id = "power";
+				enable;
+				post-delay = <10>;
+			};
+			pwm@1 {
+				id = "backlight";
+				enable;
+			};
+			gpio@2 {
+				id = "enable-gpio";
+				enable;
+			};
+		};
+		power-off-sequence {
+			gpio@0 {
+				id = "enable-gpio";
+				disable;
+			};
+			pwm@1 {
+				id = "backlight";
+				disable;
+			};
+			regulator@2 {
+				id = "power";
+				disable;
+				pre-delay = <10>;
+			};
+		};
+	};
+
+	backlight_reg: fixedregulator@176 {
+		compatible = "regulator-fixed";
+		regulator-name = "backlight_regulator";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		gpio = <&gpio 176 0>;
+		startup-delay-us = <0>;
+		enable-active-high;
+		regulator-boot-off;
+	};
+
 	sound {
 		compatible = "nvidia,tegra-audio-wm8903-ventana",
 			     "nvidia,tegra-audio-wm8903";
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 405d167..67a6cd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -123,7 +123,7 @@
 		status = "disabled";
 	};
 
-	pwm {
+	pwm: pwm {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
 		#pwm-cells = <2>;
-- 
1.7.11.3