LinuxLists.cc - [PATCH v4 0/3] Runtime Interpreted Power Sequences

2012-08-16 06:07:44

Subject: [PATCH v4 0/3] Runtime Interpreted Power Sequences

Overdue revision of this new feature, some changes required additional thought
and rework.

The most important change is in the way power sequences are expressed in the
device tree. In order to avoid having to specify #address-cells, #size-cells and
reg properties, the @ notation in the step names is dropped, and instead a
fixed, sequential naming is adopted. The type of the resource used by a step is
decided by the presence of some recognized properties:

power-on-sequence {
step0 {
regulator = "power";
enable;
};
step1 {
delay = <10000>;
};
step2 {
pwm = "backlight";
enable;
};
...

To me this looks safe, clear and close to the platform data representation, but
needs approval from DT experts.

Resources are still referenced by name instead of having their phandles defined
directly inside the sequences, as previous discussion came to the conclusion
that doing so would require controversial changes to the regulator and PWM
frameworks, and that having the resources declared at the device level was
making sense logically speaking.

Other changes/fixes since last revision:
* Move to drivers/power/ (hope this is ok with the maintainers?)
* Use microseconds for delay
* Use devm for PWM resources and remove cleanup function as all resources are
devm-managed
* Remove "-gpio" suffix for GPIO reference in the driver
* Remove params structure
* Make power_seq structure private
* Number of steps in a sequence is explicitly stated instead of resorting to a
"stop" sequence step
* Delays are a step instead of being a step parameter
* Use flexible member arrays to limit number of memory allocations
* Add documentation to DT bindings

There was a lot of feedback on the previous version (thanks!) so if I forgot
to address some important point, please bring it to my attention again.

Alexandre Courbot (3):
Runtime Interpreted Power Sequences
pwm_backlight: use power sequences
tegra: add pwm backlight device tree nodes

.../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
.../bindings/video/backlight/pwm-backlight.txt | 62 ++-
Documentation/power/power_seq.txt | 129 +++++++
arch/arm/boot/dts/tegra20-ventana.dts | 58 +++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +++++++++++++++++++++
drivers/video/backlight/Kconfig | 1 +
drivers/video/backlight/pwm_bl.c | 192 +++++++---
include/linux/power_seq.h | 142 +++++++
include/linux/pwm_backlight.h | 16 +-
12 files changed, 1071 insertions(+), 56 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h

--
1.7.11.4

2012-08-16 06:07:57

by Alexandre Courbot

[permalink] [raw]

Subject: [PATCH v4 1/3] Runtime Interpreted Power Sequences

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 <[email protected]>
---
.../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
Documentation/power/power_seq.txt | 129 +++++++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +++++++++++++++++++++
include/linux/power_seq.h | 142 +++++++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h

diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 0000000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+=============================================
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-------------------------
+Power sequences are sub-nodes that are named such as the device driver can find
+them. The driver's documentation should list the sequence names it recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in a
+parsing error.
+
+Power Sequences Steps
+---------------------
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
+- "delay", which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- "regulator" contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either "enable" or "disable", must be
+ present to control whether the regulator should be enabled or disabled during
+ that step.
+- "pwm" is set to the name of a PWM acquired using pwm_get(). As with regulator,
+ an additional "enable" or "disable" property is required.
+- "gpio" contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ "-gpio" to the given name and looking for a device property with a GPIO
+ phandle.
+
+Example
+-------
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = "pwm-backlight";
+ ...
+
+ /* resources used by the sequences */
+ pwms = <&pwm 2 5000000>;
+ pwm-names = "backlight";
+ power-supply = <&backlight_reg>;
+ enable-gpio = <&gpio 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm = "backlight";
+ disable;
+ };
+ step2 {
+ delay = <10000>;
+ };
+ step3 {
+ regulator = "power";
+ disable;
+ };
+ };
+ };
+
+The first part lists the PWM, regulator, and GPIO resources used by the
+sequences. These resources will be requested on behalf of the backlight device
+when the sequences are built and are declared according to their own framework
+in a way that makes them accessible by name.
+
+After the resources declaration, two sequences follow for powering the backlight
+on and off. Their names are specified by the pwm-backlight driver. Every step
+uses one of the "delay", "regulator", "pwm" or "gpio" properties to reference a
+previously-declared resource. Additional "enable" or "disable" properties are
+also used as needed.
diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
new file mode 100644
index 0000000..3ab4f93
--- /dev/null
+++ b/Documentation/power/power_seq.txt
@@ -0,0 +1,129 @@
+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
+also involve other resources, is board-dependent and thus unknown of the driver.
+
+This was previously addressed by having hooks in the device's platform data that
+are called whenever the state of the device might reflect a power change. This
+approach, however, introduces board-dependant code into the kernel and is not
+compatible with the device tree.
+
+The Runtime Interpreted Power Sequences (or power sequences for short) aims 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.
+
+What are Power Sequences?
+-------------------------
+Power sequences are a series of sequential steps during which an action is
+performed on a resource. The supported resources so far are:
+- delay (just wait for the delay given in microseconds)
+- GPIO (enable or disable)
+- regulator (enable or disable)
+- PWM (enable or disable)
+
+Every step designates a resource type and parameters that are relevant to it.
+For instance, GPIO and PWMs can be enabled or disabled.
+
+When a power sequence is run, each of its step is executed sequentially until
+one step fails or the end of the sequence is reached.
+
+Power sequences can be declared as platform data or in the device tree.
+
+Platform Data Format
+--------------------
+All relevant data structures for declaring power sequences are located in
+include/linux/power_seq.h.
+
+The platform data is a static instance of 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:
+
+static struct platform_power_seq power_on_seq = {
+ .nb_steps = 3,
+ .steps = {
+ {
+ .type = POWER_SEQ_REGULATOR,
+ .regulator.regulator = "power",
+ .regulator.enable = 1,
+ },
+ {
+ .type = POWER_SEQ_DELAY,
+ .delay.delay_us = 10000,
+ },
+ {
+ .type = POWER_SEQ_GPIO,
+ .gpio.gpio = 110,
+ .gpio.enable = 1,
+ },
+ },
+};
+
+Device Tree
+-----------
+Power sequences can also be encoded as device tree nodes. The following
+properties and nodes are equivalent to the platform data defined previously:
+
+ power-supply = <&power_reg>;
+ switch-gpio = <&gpio 110 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ gpio = "switch";
+ enable;
+ };
+ };
+
+See Documentation/devicetree/bindings/power_seq/power_seq.txt for the complete
+syntax of the bindings.
+
+Usage by Drivers and Resources Management
+-----------------------------------------
+Power sequences make use of resources that must be properly allocated and
+managed. The power_seq_build() function builds a power sequence from the
+platform data. It also takes care of resolving and allocating the resources
+referenced by the sequence if needed:
+
+ struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
+ struct platform_power_seq *pseq);
+
+The 'dev' argument is the device in the name of which the resources are to be
+allocated.
+
+The 'ress' argument is a list to which the resolved resources are appended. This
+avoids allocating a resource referenced in several power sequences multiple
+times.
+
+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(power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code if a
+problem occured.
+
+There is no need to explicitly free the resources used by the sequence as they
+are devm-allocated.
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index c1892f3..4172fe4 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -312,4 +312,7 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
thermistor connected on BATCTRL ADC.
endif # POWER_SUPPLY

+config POWER_SEQ
+ bool
+
source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index ee58afb..828859c 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
obj-$(CONFIG_POWER_AVS) += avs/
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
+obj-$(CONFIG_POWER_SEQ) += power_seq.o
diff --git a/drivers/power/power_seq.c b/drivers/power/power_seq.c
new file mode 100644
index 0000000..1dcdbe0
--- /dev/null
+++ b/drivers/power/power_seq.c
@@ -0,0 +1,420 @@
+/*
+ * power_seq.c - A simple power sequence interpreter for platform devices
+ * and device tree.
+ *
+ * Author: Alexandre Courbot <[email protected]>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include <linux/power_seq.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+
+struct power_seq_step {
+ /* Copy of the platform data */
+ struct platform_power_seq_step pdata;
+ /* Resolved resource */
+ struct power_seq_resource *resource;
+};
+
+struct power_seq {
+ struct device *dev;
+ unsigned int nb_steps;
+ struct power_seq_step steps[];
+};
+
+static char *res_names[POWER_SEQ_MAX] = {
+ [POWER_SEQ_DELAY] = "delay",
+ [POWER_SEQ_REGULATOR] = "regulator",
+ [POWER_SEQ_GPIO] = "gpio",
+ [POWER_SEQ_PWM] = "pwm",
+};
+
+static int power_seq_step_run(struct power_seq_step *step)
+{
+ struct platform_power_seq_step *pdata = &step->pdata;
+ int err = 0;
+
+ switch (pdata->type) {
+ case POWER_SEQ_DELAY:
+ usleep_range(pdata->delay.delay_us,
+ pdata->delay.delay_us + 1000);
+ break;
+#ifdef CONFIG_REGULATOR
+ case POWER_SEQ_REGULATOR:
+ if (pdata->regulator.enable)
+ err = regulator_enable(step->resource->regulator);
+ else
+ err = regulator_disable(step->resource->regulator);
+ break;
+#endif
+#ifdef CONFIG_PWM
+ case POWER_SEQ_PWM:
+ if (pdata->gpio.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(pdata->gpio.gpio, pdata->gpio.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;
+
+ return 0;
+}
+
+int power_seq_run(struct power_seq *seq)
+{
+ struct device *dev = seq->dev;
+ int err, cpt;
+
+ if (!seq)
+ return 0;
+
+ for (cpt = 0; cpt < seq->nb_steps; cpt++) {
+ err = power_seq_step_run(&seq->steps[cpt]);
+ if (err) {
+ 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_power_seq_enable_properties(struct device *dev,
+ struct device_node *node,
+ bool *enable)
+{
+ if (of_find_property(node, "enable", NULL)) {
+ *enable = true;
+ } else if (of_find_property(node, "disable", NULL)) {
+ *enable = false;
+ } else {
+ dev_err(dev, "missing enable or disable property!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
+ struct platform_power_seq_step *step)
+{
+ struct property *res_id = NULL;
+ int i, err;
+
+ /* Try to find a meaningful name property */
+ for (i = 0; i < POWER_SEQ_MAX; i++) {
+ struct property *mprop;
+
+ mprop = of_find_property(node, res_names[i], NULL);
+ if (mprop) {
+ if (res_id) {
+ dev_err(dev,
+ "more than one resource in step!\n");
+ return -EINVAL;
+ }
+ step->type = i;
+ res_id = mprop;
+ }
+ }
+ if (!res_id) {
+ dev_err(dev, "missing resource property for power seq step!\n");
+ return -EINVAL;
+ }
+
+ /* Now parse resource-specific properties */
+ switch (step->type) {
+ case POWER_SEQ_DELAY:
+ err = of_property_read_u32(node, res_id->name,
+ &step->delay.delay_us);
+ if (err)
+ goto err_read_property;
+
+ break;
+
+ case POWER_SEQ_REGULATOR:
+ err = of_property_read_string(node, res_id->name,
+ &step->regulator.regulator);
+ if (err)
+ goto err_read_property;
+
+ err = of_power_seq_enable_properties(dev, node,
+ &step->regulator.enable);
+ if (err)
+ return err;
+
+ break;
+
+ case POWER_SEQ_PWM:
+ err = of_property_read_string(node, res_id->name,
+ &step->pwm.pwm);
+ if (err)
+ goto err_read_property;
+
+ err = of_power_seq_enable_properties(dev, node,
+ &step->pwm.enable);
+ if (err)
+ return err;
+
+ break;
+
+#ifdef CONFIG_OF_GPIO
+ case POWER_SEQ_GPIO:
+ {
+ char prop_name[32]; /* max size of property name */
+ const char *gpio_name;
+ int gpio;
+
+ err = of_property_read_string(node, res_id->name, &gpio_name);
+ if (err)
+ goto err_read_property;
+
+ /* Resolve the GPIO name */
+ snprintf(prop_name, 32, "%s-gpio", gpio_name);
+ gpio = of_get_named_gpio(dev->of_node, prop_name, 0);
+ if (gpio < 0) {
+ dev_err(dev, "cannot resolve gpio \"%s\"\n", gpio_name);
+ return gpio;
+ }
+ step->gpio.gpio = gpio;
+
+ err = of_power_seq_enable_properties(dev, node,
+ &step->gpio.enable);
+ if (err)
+ return err;
+
+ break;
+ }
+#endif /* CONFIG_OF_GPIO */
+
+ default:
+ dev_err(dev, "unhandled power sequence step type %s\n",
+ res_names[step->type]);
+ return -EINVAL;
+ }
+
+ return 0;
+
+err_read_property:
+ dev_err(dev, "cannot read %s property!", res_names[step->type]);
+ return -EINVAL;
+}
+
+struct platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node)
+{
+ struct device_node *child = NULL;
+ struct platform_power_seq *pseq;
+ int nb_steps = 0, size;
+ int err;
+
+ if (!node)
+ return ERR_PTR(-EINVAL);
+
+ nb_steps = of_get_child_count(node);
+ size = sizeof(pseq) + sizeof(struct platform_power_seq_step) * nb_steps;
+ pseq = devm_kzalloc(dev, size, GFP_KERNEL);
+ if (!pseq)
+ return ERR_PTR(-ENOMEM);
+ pseq->nb_steps = nb_steps;
+
+ for_each_child_of_node(node, child) {
+ unsigned int pos;
+
+ /* Check that the name's format is correct and within bounds */
+ if (strncmp("step", child->name, 4)) {
+ err = -EINVAL;
+ goto parse_error;
+ }
+
+ err = kstrtoint(child->name + 4, 10, &pos);
+ if (err < 0)
+ goto parse_error;
+
+ if (pos >= nb_steps || pseq->steps[pos].type != 0) {
+ err = -EINVAL;
+ goto parse_error;
+ }
+
+ err = of_parse_power_seq_step(dev, child, &pseq->steps[pos]);
+ if (err)
+ return ERR_PTR(err);
+ }
+
+ return pseq;
+
+parse_error:
+ dev_err(dev, "invalid power step name %s!\n", child->name);
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(of_parse_power_seq);
+#endif /* CONFIG_OF */
+
+static
+struct power_seq_resource *power_seq_find_resource(struct list_head *ress,
+ struct platform_power_seq_step *step)
+{
+ struct power_seq_resource *res;
+
+ list_for_each_entry(res, ress, list) {
+ struct platform_power_seq_step *pdata = res->pdata;
+
+ if (pdata->type != step->type)
+ continue;
+
+ switch (pdata->type) {
+ case POWER_SEQ_REGULATOR:
+ if (!strcmp(pdata->regulator.regulator,
+ step->regulator.regulator))
+ return res;
+ break;
+ case POWER_SEQ_PWM:
+ if (!strcmp(pdata->pwm.pwm, step->pwm.pwm))
+ return res;
+ break;
+ case POWER_SEQ_GPIO:
+ if (pdata->gpio.gpio == step->gpio.gpio)
+ return res;
+ break;
+ default:
+ break;
+ }
+ }
+
+ return NULL;
+}
+
+static int power_seq_allocate_resource(struct device *dev,
+ struct power_seq_resource *res)
+{
+ struct platform_power_seq_step *pdata = res->pdata;
+ int err;
+
+ switch (pdata->type) {
+ case POWER_SEQ_DELAY:
+ break;
+#ifdef CONFIG_REGULATOR
+ case POWER_SEQ_REGULATOR:
+ res->regulator = devm_regulator_get(dev,
+ pdata->regulator.regulator);
+ if (IS_ERR(res->regulator)) {
+ dev_err(dev, "cannot get regulator \"%s\"\n",
+ pdata->regulator.regulator);
+ return PTR_ERR(res->regulator);
+ }
+ break;
+#endif
+#ifdef CONFIG_PWM
+ case POWER_SEQ_PWM:
+ res->pwm = devm_pwm_get(dev, pdata->pwm.pwm);
+ if (IS_ERR(res->pwm)) {
+ dev_err(dev, "cannot get pwm \"%s\"\n", pdata->pwm.pwm);
+ return PTR_ERR(res->pwm);
+ }
+ break;
+#endif
+#ifdef CONFIG_GPIOLIB
+ case POWER_SEQ_GPIO:
+ err = devm_gpio_request_one(dev, pdata->gpio.gpio,
+ GPIOF_OUT_INIT_HIGH, "backlight_gpio");
+ if (err) {
+ dev_err(dev, "cannot get gpio %d\n", pdata->gpio.gpio);
+ return err;
+ }
+ break;
+#endif
+ default:
+ dev_err(dev, "invalid resource type %d\n", pdata->type);
+ return -EINVAL;
+ break;
+ }
+
+ return 0;
+}
+
+struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
+ struct platform_power_seq *pseq)
+{
+ struct power_seq *seq;
+ struct power_seq_resource *res;
+ int cpt, err;
+
+ seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(struct power_seq_step) *
+ pseq->nb_steps, GFP_KERNEL);
+ if (!seq)
+ return ERR_PTR(-ENOMEM);
+
+ seq->dev = dev;
+ seq->nb_steps = pseq->nb_steps;
+
+ for (cpt = 0; cpt < seq->nb_steps; cpt++) {
+ struct platform_power_seq_step *pstep = &pseq->steps[cpt];
+ struct power_seq_step *step = &seq->steps[cpt];
+
+ memcpy(&step->pdata, pstep, sizeof(step->pdata));
+
+ /* Delay steps have no resource */
+ if (pstep->type == POWER_SEQ_DELAY)
+ continue;
+
+ /* create resource node if not referenced already */
+ res = power_seq_find_resource(ress, pstep);
+ if (!res) {
+ res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return ERR_PTR(-ENOMEM);
+ res->pdata = &step->pdata;
+
+ err = power_seq_allocate_resource(dev, res);
+ if (err < 0)
+ return ERR_PTR(err);
+
+ list_add(&res->list, ress);
+ }
+ step->resource = res;
+ }
+
+ return seq;
+}
+EXPORT_SYMBOL_GPL(power_seq_build);
+
+MODULE_AUTHOR("Alexandre Courbot <[email protected]>");
+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..d9dd277
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,142 @@
+/*
+ * power_seq.h
+ *
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties.
+ *
+ * Power sequences are designed to replace the callbacks typically used in
+ * board-specific files that implement board-specific power sequences of devices
+ * such as backlights. A power sequence is an array of resources (which can a
+ * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
+ * disable) and optional pre and post step delays. By having them interpreted
+ * instead of arbitrarily executed, it is possible to describe these in the
+ * device tree and thus remove board-specific code from the kernel.
+ *
+ * Author: Alexandre Courbot <[email protected]>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/types.h>
+
+struct device;
+struct regulator;
+struct pwm_device;
+struct device_node;
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+enum power_seq_res_type {
+ POWER_SEQ_DELAY,
+ POWER_SEQ_REGULATOR,
+ POWER_SEQ_PWM,
+ POWER_SEQ_GPIO,
+ POWER_SEQ_MAX,
+};
+
+struct platform_power_seq_delay_step {
+ unsigned int delay_us;
+};
+
+struct platform_power_seq_regulator_step {
+ const char *regulator;
+ bool enable;
+};
+
+struct platform_power_seq_pwm_step {
+ const char *pwm;
+ bool enable;
+};
+
+struct platform_power_seq_gpio_step {
+ int gpio;
+ bool enable;
+};
+
+/**
+ * Platform definition of power sequences. A sequence is an array of these,
+ * terminated by a STOP instance.
+ */
+struct platform_power_seq_step {
+ enum power_seq_res_type type;
+ union {
+ struct platform_power_seq_delay_step delay;
+ struct platform_power_seq_regulator_step regulator;
+ struct platform_power_seq_pwm_step pwm;
+ struct platform_power_seq_gpio_step gpio;
+ };
+};
+
+struct platform_power_seq {
+ unsigned int nb_steps;
+ struct platform_power_seq_step steps[];
+};
+
+/**
+ * We maintain a list of these to monitor which resources have already
+ * been met and allocated while building the sequences.
+ */
+struct power_seq_resource {
+ /* relevant for resolving the resource and knowing its type */
+ struct platform_power_seq_step *pdata;
+ /* resolved resource (if any) */
+ union {
+ struct regulator *regulator;
+ struct pwm_device *pwm;
+ };
+ struct list_head list;
+};
+
+struct power_seq_resource;
+struct power_seq;
+
+#ifdef CONFIG_OF
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * platform sequence is allocated using devm_kzalloc on dev and can be freed
+ * by devm_kfree after power_seq_build is called.
+ */
+struct platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node);
+#else
+struct platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node)
+{
+ return ERR_PTR(-EINVAL);
+}
+#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.
+ * @dev device that will use the power sequence. All resources will be
+ * devm-allocated against it.
+ * @ress list that holds the power_seq_resources already used by this device.
+ * Resources newly met in the sequence will be added to it.
+ * @pseq power sequence in platform format.
+ */
+struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
+ struct platform_power_seq *pseq);
+
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(struct power_seq *seq);
+
+#endif
--
1.7.11.4

2012-08-16 06:08:22

by Alexandre Courbot

[permalink] [raw]

Subject: [PATCH v4 3/3] tegra: add pwm backlight device tree nodes

Signed-off-by: Alexandre Courbot <[email protected]>
---
arch/arm/boot/dts/tegra20-ventana.dts | 58 +++++++++++++++++++++++++++++++++++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..b253697 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,64 @@
bus-width = <8>;
};

+ backlight {
+ compatible = "pwm-backlight";
+ brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+ default-brightness-level = <12>;
+
+ /* resources used by the power sequences */
+ pwms = <&pwm 2 5000000>;
+ pwm-names = "backlight";
+ power-supply = <&backlight_reg>;
+ enable-gpio = <&gpio 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm = "backlight";
+ disable;
+ };
+ step2 {
+ delay = <10000>;
+ };
+ step3 {
+ regulator = "power";
+ disable;
+ };
+ };
+ };
+
+ 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.4

2012-08-16 06:08:37

by Alexandre Courbot

[permalink] [raw]

Subject: [PATCH v4 2/3] pwm_backlight: use power sequences

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

Signed-off-by: Alexandre Courbot <[email protected]>
---
.../bindings/video/backlight/pwm-backlight.txt | 62 ++++++-
drivers/video/backlight/Kconfig | 1 +
drivers/video/backlight/pwm_bl.c | 192 +++++++++++++++------
include/linux/pwm_backlight.h | 16 +-
4 files changed, 216 insertions(+), 55 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..51e814d 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,19 +9,72 @@ 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

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
+ - pwm-names: name for the PWM device specified in the "pwms" property (see PWM
+ binding[0]). Necessary if power sequences are used
+ - power-on-sequence: Power sequence (see Power sequences[1]) used to bring the
+ backlight on. This sequence must reference the PWM specified in the pwms
+ property by its name. It can also reference other resources supported by
+ the power sequences mechanism
+ - power-off-sequence: Power sequence (see Power sequences[1]) used to bring
+ the backlight off. This sequence must reference the PWM specified in the
+ pwms property by its name. It can also reference other resources supported
+ by the power sequences mechanism

[0]: Documentation/devicetree/bindings/pwm/pwm.txt
+[1]: Documentation/devicetree/bindings/power_seq/power_seq.txt

Example:

backlight {
compatible = "pwm-backlight";
- pwms = <&pwm 0 5000000>;
-
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+
+ /* resources used by the sequences */
+ pwms = <&pwm 2 5000000>;
+ pwm-names = "backlight";
+ power-supply = <&backlight_reg>;
+ enable-gpio = <&gpio 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm = "backlight";
+ disable;
+ };
+ step2 {
+ delay = <10000>;
+ };
+ step3 {
+ regulator = "power";
+ disable;
+ };
+ };
};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index cf28276..6fb8aa3 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -246,6 +246,7 @@ config BACKLIGHT_CARILLO_RANCH
config BACKLIGHT_PWM
tristate "Generic PWM based Backlight Driver"
depends on PWM
+ select POWER_SEQ
help
If you have a LCD backlight adjustable by PWM, say Y to enable
this driver.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 995f016..f456a71 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,13 @@ struct pwm_bl_data {
unsigned int period;
unsigned int lth_brightness;
unsigned int *levels;
+ bool enabled;
+ bool use_power_seqs;
+ struct list_head resources;
+ struct power_seq *power_on_seq;
+ struct power_seq *power_off_seq;
+
+ /* Legacy callbacks */
int (*notify)(struct device *,
int brightness);
void (*notify_after)(struct device *,
@@ -35,6 +42,49 @@ struct pwm_bl_data {
void (*exit)(struct device *);
};

+static void pwm_backlight_on(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+ int ret;
+
+ if (pb->enabled)
+ return;
+
+ /* Legacy framework? */
+ if (!pb->use_power_seqs) {
+ pwm_config(pb->pwm, 0, pb->period);
+ pwm_disable(pb->pwm);
+ return;
+ }
+
+ ret = power_seq_run(pb->power_on_seq);
+ if (ret < 0)
+ dev_err(&bl->dev, "cannot run power on sequence\n");
+
+ pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+ int ret;
+
+ if (!pb->enabled)
+ return;
+
+ /* Legacy framework? */
+ if (!pb->use_power_seqs) {
+ pwm_enable(pb->pwm);
+ return;
+ }
+
+ ret = power_seq_run(pb->power_off_seq);
+ if (ret < 0)
+ dev_err(&bl->dev, "cannot run power off sequence\n");
+
+ pb->enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +101,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 +115,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)
@@ -98,7 +147,7 @@ static const struct backlight_ops pwm_backlight_ops = {
static int pwm_backlight_parse_dt(struct device *dev,
struct platform_pwm_backlight_data *data)
{
- struct device_node *node = dev->of_node;
+ struct device_node *node = dev->of_node, *pseq_node;
struct property *prop;
int length;
u32 value;
@@ -145,11 +194,18 @@ 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.
- */
+ /* convert power sequences to platform data, if any */
+ pseq_node = of_find_node_by_name(node, "power-on-sequence");
+ if (pseq_node)
+ data->power_on_seq = of_parse_power_seq(dev, pseq_node);
+ if (IS_ERR(data->power_on_seq))
+ return PTR_ERR(data->power_on_seq);
+
+ pseq_node = of_find_node_by_name(node, "power-off-sequence");
+ if (pseq_node)
+ data->power_off_seq = of_parse_power_seq(dev, pseq_node);
+ if (IS_ERR(data->power_off_seq))
+ return PTR_ERR(data->power_off_seq);

return 0;
}
@@ -172,33 +228,98 @@ 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;
}

+ /* using legacy interface? */
+ if (!data->power_on_seq && !data->power_off_seq) {
+ pb->pwm = devm_pwm_get(&pdev->dev, NULL);
+ if (IS_ERR(pb->pwm)) {
+ dev_err(&pdev->dev,
+ "unable to request PWM, trying legacy API\n");
+
+ pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
+ if (IS_ERR(pb->pwm)) {
+ dev_err(&pdev->dev,
+ "unable to request legacy PWM\n");
+ return PTR_ERR(pb->pwm);
+ }
+ }
+
+ /*
+ * The DT case will set the pwm_period_ns field to 0 and store
+ * the period, parsed from the DT, in the PWM device. For the
+ * non-DT case, set the period from platform data.
+ */
+ if (data->pwm_period_ns > 0)
+ pwm_set_period(pb->pwm, data->pwm_period_ns);
+ } else {
+ /* build 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->pdata->type != POWER_SEQ_PWM)
+ continue;
+ if (pb->pwm) {
+ dev_err(&pdev->dev, "more than one PWM used\n");
+ return -EINVAL;
+ }
+ /* keep the pwm at hand */
+ pb->pwm = res->pwm;
+ }
+
+ pb->use_power_seqs = true;
+ }
+
if (data->init) {
ret = data->init(&pdev->dev);
if (ret < 0)
- return ret;
+ goto err;
}

- pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
- if (!pb) {
- dev_err(&pdev->dev, "no memory for state\n");
- ret = -ENOMEM;
- goto err_alloc;
+ /* from here we should have a PWM */
+ if (!pb->pwm) {
+ dev_err(&pdev->dev, "no PWM defined!\n");
+ return -EINVAL;
}

if (data->levels) {
@@ -213,28 +334,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->exit = data->exit;
pb->dev = &pdev->dev;

- pb->pwm = pwm_get(&pdev->dev, NULL);
- if (IS_ERR(pb->pwm)) {
- dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
- pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
- if (IS_ERR(pb->pwm)) {
- dev_err(&pdev->dev, "unable to request legacy PWM\n");
- ret = PTR_ERR(pb->pwm);
- goto err_alloc;
- }
- }
-
- dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
- /*
- * The DT case will set the pwm_period_ns field to 0 and store the
- * period, parsed from the DT, in the PWM device. For the non-DT case,
- * set the period from platform data.
- */
- if (data->pwm_period_ns > 0)
- pwm_set_period(pb->pwm, data->pwm_period_ns);
-
pb->period = pwm_get_period(pb->pwm);
pb->lth_brightness = data->lth_brightness * (pb->period / max);

@@ -246,18 +345,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(bl)) {
dev_err(&pdev->dev, "failed to register backlight\n");
ret = PTR_ERR(bl);
- goto err_bl;
+ goto err;
}

bl->props.brightness = data->dft_brightness;
backlight_update_status(bl);

platform_set_drvdata(pdev, bl);
+
return 0;

-err_bl:
- pwm_put(pb->pwm);
-err_alloc:
+err:
if (data->exit)
data->exit(&pdev->dev);
return ret;
@@ -269,9 +367,8 @@ static int pwm_backlight_remove(struct platform_device *pdev)
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);

backlight_device_unregister(bl);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
- pwm_put(pb->pwm);
+ pwm_backlight_off(bl);
+
if (pb->exit)
pb->exit(&pdev->dev);
return 0;
@@ -285,8 +382,7 @@ static int pwm_backlight_suspend(struct device *dev)

if (pb->notify)
pb->notify(pb->dev, 0);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_off(bl);
if (pb->notify_after)
pb->notify_after(pb->dev, 0);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..5bc5e39 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,26 @@
#define __LINUX_PWM_BACKLIGHT_H

#include <linux/backlight.h>
+#include <linux/power_seq.h>

struct platform_pwm_backlight_data {
- int pwm_id;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;
- unsigned int pwm_period_ns;
unsigned int *levels;
+ /*
+ * New interface using power sequences
+ */
+ struct platform_power_seq *power_on_seq;
+ struct platform_power_seq *power_off_seq;
+ /*
+ * Legacy interface - use power sequences instead!
+ *
+ * pwm_id and pwm_period_ns need only be specified
+ * if get_pwm(dev, NULL) would return NULL.
+ */
+ int pwm_id;
+ unsigned int pwm_period_ns;
int (*init)(struct device *dev);
int (*notify)(struct device *dev, int brightness);
void (*notify_after)(struct device *dev, int brightness);
--
1.7.11.4

2012-08-16 07:42:47

On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
> On 08/16/2012 04:42 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
[...]
> >>+Usage by Drivers and Resources Management
> >>+-----------------------------------------
> >>+Power sequences make use of resources that must be properly allocated and
> >>+managed. The power_seq_build() function builds a power sequence from the
> >>+platform data. It also takes care of resolving and allocating the resources
> >>+referenced by the sequence if needed:
> >>+
> >>+ struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> >>+ struct platform_power_seq *pseq);
> >>+
> >>+The 'dev' argument is the device in the name of which the resources are to be
> >>+allocated.
> >>+
> >>+The 'ress' argument is a list to which the resolved resources are appended. This
> >>+avoids allocating a resource referenced in several power sequences multiple
> >>+times.
> >>+
> >>+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(power_seq *seq);
> >>+
> >>+It returns 0 if the sequence has successfully been run, or an error code if a
> >>+problem occured.
> >>+
> >>+There is no need to explicitly free the resources used by the sequence as they
> >>+are devm-allocated.
> >
> >I had some comments about this particular interface for creating
> >sequences in the last series. My point was that explicitly requiring
> >drivers to manage a list of already allocated resources may be too much
> >added complexity. Power sequences should be easy to use, and I find the
> >requirement for a separately managed list of resources cumbersome.
> >
> >What I proposed last time was to collect all power sequences under a
> >common parent object, which in turn would take care of managing the
> >resources.
>
> Yes, I remember that. While I see why you don't like this list,
> having a common parent object to all sequences will not reduce the
> number of arguments to pass to power_seq_build() (which is the only
> function that has to handle this list now). Also having the list of
> resources at hand is needed for some drivers: for instance,
> pwm-backlight needs to check that exactly one PWM has been
> allocated, and takes a reference to it from this list in order to
> control the brightness.

I'm not complaining about the additional argument to power_seq_build()
but about the missing encapsulation. I just think that keeping a list
external to the power sequencing code is error-prone. Drivers could do
just about anything with it between calls to power_seq_build(). If you
do all of this internally, then you don't depend on the driver at all
and power sequencing code can just do the right thing.

Obtaining a reference to the PWM, or any other resource for that matter,
from the power sequence could be done via an explicit API.

> Ideally we could embed the list into the device structure, but I
> don't see how we can do that without modifying it (and we don't want
> to modify it). Another solution would be to keep a static mapping
> table that associates a device to its power_seq related resources
> within power_seq.c. If we protect it for concurrent access this
> should make it possible to make resources management transparent.
> How does this sound? Only drawback I see is that we would need to
> explicitly clean it up through a dedicated function when the driver
> exits.

I don't think that's much better. Since the power sequences will be very
tightly coupled to a specific device, tying the sequences and their
resources to the device makes a lot of sense. Keeping a global list of
resources doesn't in my opinion.

> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+ struct platform_power_seq_step *pdata = &step->pdata;
> >>+ int err = 0;
> >>+
> >>+ switch (pdata->type) {
> >>+ case POWER_SEQ_DELAY:
> >>+ usleep_range(pdata->delay.delay_us,
> >>+ pdata->delay.delay_us + 1000);
> >>+ break;
> >>+#ifdef CONFIG_REGULATOR
> >>+ case POWER_SEQ_REGULATOR:
> >>+ if (pdata->regulator.enable)
> >>+ err = regulator_enable(step->resource->regulator);
> >>+ else
> >>+ err = regulator_disable(step->resource->regulator);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+ case POWER_SEQ_PWM:
> >>+ if (pdata->gpio.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(pdata->gpio.gpio, pdata->gpio.enable);
> >>+ break;
> >>+#endif
> >>+ /*
> >>+ * should never happen unless the sequence includes a step which
> >>+ * type does not have support compiled in
> >
> >I think this should be "whose type"? I also remember commenting on the
> >whole #ifdef'ery here. I really don't think it is necessary. At least
> >for regulators I know that the functions can be used even if the
> >subsystem itself isn't supported. The same seems to hold for GPIO and we
> >can probably add something similar for PWM.
>
> Actually I kept them because I don't really like the empty function
> definitions in the regulator framework. They all return 0 as if the
> function completed successfully - here we should at least warn the
> user that proper support for that resource is missing.
>
> >
> >It might also be a good idea to just skip unsupported resource types
> >when the sequence is built, accompanied by runtime warnings that the
> >type is not supported.
>
> Agreed.

If you do this, then I think the above #ifdef'ery becomes obsolete
because any errors that could potentially be hidden have already been
caught when the list was built.

Thierry

Attachments:

(No filename) (6.11 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-16 10:31:41

by Alexandre Courbot

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

On 08/16/2012 06:52 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
>> On 08/16/2012 04:42 PM, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> [...]
>>>> +Usage by Drivers and Resources Management
>>>> +-----------------------------------------
>>>> +Power sequences make use of resources that must be properly allocated and
>>>> +managed. The power_seq_build() function builds a power sequence from the
>>>> +platform data. It also takes care of resolving and allocating the resources
>>>> +referenced by the sequence if needed:
>>>> +
>>>> + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
>>>> + struct platform_power_seq *pseq);
>>>> +
>>>> +The 'dev' argument is the device in the name of which the resources are to be
>>>> +allocated.
>>>> +
>>>> +The 'ress' argument is a list to which the resolved resources are appended. This
>>>> +avoids allocating a resource referenced in several power sequences multiple
>>>> +times.
>>>> +
>>>> +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(power_seq *seq);
>>>> +
>>>> +It returns 0 if the sequence has successfully been run, or an error code if a
>>>> +problem occured.
>>>> +
>>>> +There is no need to explicitly free the resources used by the sequence as they
>>>> +are devm-allocated.
>>>
>>> I had some comments about this particular interface for creating
>>> sequences in the last series. My point was that explicitly requiring
>>> drivers to manage a list of already allocated resources may be too much
>>> added complexity. Power sequences should be easy to use, and I find the
>>> requirement for a separately managed list of resources cumbersome.
>>>
>>> What I proposed last time was to collect all power sequences under a
>>> common parent object, which in turn would take care of managing the
>>> resources.
>>
>> Yes, I remember that. While I see why you don't like this list,
>> having a common parent object to all sequences will not reduce the
>> number of arguments to pass to power_seq_build() (which is the only
>> function that has to handle this list now). Also having the list of
>> resources at hand is needed for some drivers: for instance,
>> pwm-backlight needs to check that exactly one PWM has been
>> allocated, and takes a reference to it from this list in order to
>> control the brightness.
>
> I'm not complaining about the additional argument to power_seq_build()
> but about the missing encapsulation. I just think that keeping a list
> external to the power sequencing code is error-prone. Drivers could do
> just about anything with it between calls to power_seq_build(). If you
> do all of this internally, then you don't depend on the driver at all
> and power sequencing code can just do the right thing.

On the opposite side, I am concerned about over-encapsulation. :) IIRC
you proposed to have a top structure to hold the power sequences, their
resources and the associated device. Power sequences would then have a
name and be run through a 2 arguments power_seq_run():

power_seq_run(sequences, "up");

There are two things that bother me with this solution. First is that
addressing power sequences by name looks a little bit overkill, when a
single pointer should be enough. It would also complicate the design.
Second thing is that this design would place the power sequences
structure on top of the device - in effect, you could perfectly have
several of these structures all using the same device and failing to see
each other's resources. While that would be a error from the device
driver's side, the design allows it.

>
> Obtaining a reference to the PWM, or any other resource for that matter,
> from the power sequence could be done via an explicit API.
>
>> Ideally we could embed the list into the device structure, but I
>> don't see how we can do that without modifying it (and we don't want
>> to modify it). Another solution would be to keep a static mapping
>> table that associates a device to its power_seq related resources
>> within power_seq.c. If we protect it for concurrent access this
>> should make it possible to make resources management transparent.
>> How does this sound? Only drawback I see is that we would need to
>> explicitly clean it up through a dedicated function when the driver
>> exits.
>
> I don't think that's much better. Since the power sequences will be very
> tightly coupled to a specific device, tying the sequences and their
> resources to the device makes a lot of sense. Keeping a global list of
> resources doesn't in my opinion.

That is not what would happen actually - what I proposed is to have a
mapping (hash map, or more likely binary tree) between a device and the
list_head of the resources for that device. In C++ (forgive me, this
makes the types more explicit) that would be:

static std::map<struct device *, struct list_head> device_resources;

That way you would have exactly one list per device, could keep
resource-management totally transparent without exposing the list_head,
and keep the API and design simple.

For special cases (like pwm-backlight which needs to get the PWM), the
list_head could be obtained through a dedicated API.

Alex.

2012-08-16 10:53:01

2012-08-21 07:44:45

On Tue, Aug 21, 2012 at 05:22:12PM +0900, Alex Courbot wrote:
> Hi Tomi,
>
> On Tuesday 21 August 2012 15:44:29 Tomi Valkeinen wrote:
> > > +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 +also involve other resources, is
> > > board-dependent and thus unknown of the driver. +
> > > +This was previously addressed by having hooks in the device's platform
> > > data that +are called whenever the state of the device might reflect a
> > > power change. This +approach, however, introduces board-dependant code
> > > into the kernel and is not +compatible with the device tree.
> >
> > I've been having the same problems on OMAP display related code, but the
> > problem has always been fixable by just having the driver to use a
> > common framework to do the job (a framework which may not have existed
> > at the time). The problems have never been board specific in the end,
> > but device specific.
> >
> > Can you describe your particular HW problem a bit more? In the backlight
> > case, what exactly requires the delays and the sequence you show in the
> > example to enable/disable the backlight?
>
> In the example, the sequence (including delays) is clearly stated by the
> backlight specification, which is part of the panel specification. The backlight
> uses a PWM, which makes it suitable to use the generic PWM backlight driver,
> but how to turn the backlight on and off is very backlight specific. The power
> sequences allow to replace the board-specific backlight callbacks by sequences
> in the DT.
>
> On the other hand, I saw your discussion with Laurent about the panel
> framework, and I wonder how this would fit into it. Backlights are typically
> embedded within panels. Power sequences are a good way to deal with the
> absence of specific drivers for every panels, since they allow to tailor the
> behavior of generic drivers to fit particular needs. But if every panel comes
> with its own driver (which would define the backlight device using the most
> appropriate driver), then it could just as well perform its backlight's
> sequences via regular callbacks. In this particular case, there would be no
> need for power sequences.
>
> Power sequences are supposed to go beyond backlight drivers and support all
> sort of devices (I have heard that it could be useful for modems as well), but
> I wonder how relevant they are when there is a proper driver for a device. I
> hate to question my own work but now I cannot help but think that a proper
> driver could do the same job. So what are we trying to achieve with power
> sequences? Are we trying to avoid a drivers' explosion by keeping some of the
> specifics out of them? Which approach would be preferable? Are there cases
> where a dedicated driver could not replace power sequences?

I suppose power sequences aren't needed if you have a specific driver
for every panel out there. However that also means that you'd have to
write drivers for literally every panel that requires support. In the
end this will just result in discussion down the road how the common
functionality can be refactored and we may end up with power sequences
again.

Also as you mentioned, power sequences are useful for a number of other
use-cases. Without power sequences you'll have to potentially create
extra frameworks tha reimplement parts of the power sequence code for
their specific hardware needs.

Thierry

Attachments:

(No filename) (3.72 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-21 08:51:39

by Alexandre Courbot

On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote:
> > However this also means we'll essentially just be moving the board code.
>
>
> What do you mean "just"? Wasn't the point of the whole "arm board file
> mess" to get rid of the code from the board files? If the code in the
> board file is device specific code, not board specific, then the driver
> of the device is a logical place to place it.

I think Tomi has a point here - these sequences were not belonging to the
board code in the first place. They are definitely tied to the device, hence
should have been handled by the driver all along, with the board code
assigning the correct resources to the device (like the vast majority of
device drivers do).

> And as I said, I don't have any problems with some kind of generic power
> sequences. So the code in the board file could be moved and converted to
> use the power sequences, if that is better than just plain c code.

My concern now is, provided that all drivers to their job and handle how their
devices are switched on and off, when (if at all) are encoded power sequences
better than their equivalent C code? There is the matching database size issue
that you mentionned, is it a sufficient concern to justify a new kernel feature?

On the other hand some devices like panels are typically not used in many
different appliances, so maybe it is not worth to separate them from their
board definition. As Mark mentionned, having .dtsi files for the DT (and their
equivalent .h for kernels that use platform data) might be a good middle
ground.

But the line is really tight between what is code and what is data here.

Alex.

2012-08-24 10:34:28

by Tomi Valkeinen

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

On Fri, 2012-08-24 at 18:24 +0900, Alex Courbot wrote:
> On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote:

> > And as I said, I don't have any problems with some kind of generic power
> > sequences. So the code in the board file could be moved and converted to
> > use the power sequences, if that is better than just plain c code.
>
> My concern now is, provided that all drivers to their job and handle how their
> devices are switched on and off, when (if at all) are encoded power sequences
> better than their equivalent C code? There is the matching database size issue
> that you mentionned, is it a sufficient concern to justify a new kernel feature?

Good question.

I think obviously the worst solution is to have separate .c driver files
for each panel, where the drivers do 99% the same thing.

So the question is how to represent the 1% difference the panels have.

I think it depends on the panels. If it looks like all the panel have,
say, max 2 regulators and one reset/enable gpio, and they are always
enabled in the same order (regulators first, then the gpio), it should
be easy to handle it in the driver without any power sequence framework.

If the panels require more complex setups, then the code in the panel
driver would probably start to form into a power sequence framework, and
it would make sense to have it as a separate framework.

Then again, if the panel setup is complex, it makes me wonder if it'd be
just easier to handle it with c code in a separate driver.

Also, the database size issue is a bit separate issue. There's the db
size problem with or without the framework, if we do not pass the data
from DT.

So as clarification, I see 4 different options:
- Power sequences in DT (as proposed in this series)
- Custom panel data in DT, that the driver uses to power up properly
- Power sequences in a panel database in kernel
- Custom panel data in a panel db in kernel

> On the other hand some devices like panels are typically not used in many
> different appliances, so maybe it is not worth to separate them from their

Yep, this is the reason for my concern with the database size. The DB
could contain 10k panels, of which a board uses one. The rest just waste
memory. But then again, 10k panels is probably not a realistic amount.
It's difficult to guess the amount of memory used by such a database,
though. If it uses, say, 8kB, I'm not sure if it's a reason to panic.

And, as I mentioned, it could be optimized so that the driver throws
away the unneeded panels at __init. Of course here the problem is that
the panel needs to know what panels are needed.

> board definition. As Mark mentionned, having .dtsi files for the DT (and their
> equivalent .h for kernels that use platform data) might be a good middle
> ground.

I guess it's the perfectionist in me that leans toward handling it fully
in the kernel, as I think that's architecturally correct thing to do.
The DT data should contain parameters that can be configured per board,
or nodes (like regulators) that are needed by other parts of the DT
data.

If the only reason why to do it via DT is to avoid the possible size
problem with the panel database, perhaps what we should solve is the
optimization of the database. If that seems unsolvable, then DT approach
could be used as a workaround.

And I have to say I'm not too familiar with DT, so if I'm wrong about
what DT should contain, I'm all ears =).

Tomi

Attachments:

signature.asc (836.00 B)
This is a digitally signed message part