* [PATCH v4 0/3] Runtime Interpreted Power Sequences
@ 2012-08-16 6:08 Alexandre Courbot
2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot
` (3 more replies)
0 siblings, 4 replies; 33+ messages in thread
From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw)
To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann
Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc, Alexandre Courbot
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
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 6:08 [PATCH v4 0/3] Runtime Interpreted Power Sequences Alexandre Courbot @ 2012-08-16 6:08 ` Alexandre Courbot 2012-08-16 7:42 ` Thierry Reding ` (3 more replies) 2012-08-16 6:08 ` [PATCH v4 2/3] pwm_backlight: use power sequences Alexandre Courbot ` (2 subsequent siblings) 3 siblings, 4 replies; 33+ messages in thread From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw) To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc, Alexandre Courbot Some device drivers (panel backlights especially) need to follow precise sequences for powering on and off, involving gpios, regulators, PWMs with a precise powering order and delays to respect between each steps. These sequences are board-specific, and do not belong to a particular driver - therefore they have been performed by board-specific hook functions to far. With the advent of the device tree and of ARM kernels that are not board-tied, we cannot rely on these board-specific hooks anymore but need a way to implement these sequences in a portable manner. This patch introduces a simple interpreter that can execute such power sequences encoded either as platform data or within the device tree. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- .../devicetree/bindings/power_seq/power_seq.txt | 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 <acourbot@nvidia.com> + * + * Copyright (c) 2012 NVIDIA Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#include <linux/power_seq.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/device.h> +#include <linux/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 <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..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 <acourbot@nvidia.com> + * + * Copyright (c) 2012 NVIDIA Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#ifndef __LINUX_POWER_SEQ_H +#define __LINUX_POWER_SEQ_H + +#include <linux/types.h> + +struct device; +struct regulator; +struct pwm_device; +struct device_node; + +/** + * The different kinds of resources that can be controlled during the sequences. + */ +enum power_seq_res_type { + POWER_SEQ_DELAY, + POWER_SEQ_REGULATOR, + POWER_SEQ_PWM, + POWER_SEQ_GPIO, + POWER_SEQ_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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot @ 2012-08-16 7:42 ` Thierry Reding 2012-08-16 9:19 ` Alex Courbot 2012-08-16 14:14 ` Mark Brown ` (2 subsequent siblings) 3 siblings, 1 reply; 33+ messages in thread From: Thierry Reding @ 2012-08-16 7:42 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 31667 bytes --] On Thu, Aug 16, 2012 at 03:08:55PM +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> > --- > .../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 "named such that"? > +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: I think the "currently" can be dropped. > +- "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. I find this part slightly confusing. It doesn't seem quite obvious from the text that "delay", "regulator", "pwm" and "gpio" are also actual property names. It might also be worth saying that the "enable" and "disable" properties are boolean in nature and don't need a value. Then again this becomes much clearer with the example below, so maybe I'm just being too picky here. > + > +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. "unknown to the driver"? > + > +This was previously addressed by having hooks in the device's platform data that > +are called whenever the state of the device might reflect a power change. This > +approach, however, introduces board-dependant code into the kernel and is not > +compatible with the device tree. > + > +The Runtime Interpreted Power Sequences (or power sequences for short) aims at "... Sequences [...] aim"? > +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: Again, I don't see a need for "so far" here. It implies that new types may be added. While it is quite possible and maybe even likely to happen the new types can be added to the documentation at the same time. > +- 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 "each of its steps" > +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, I think num_steps would be more canonical. > + .steps = { > + { > + .type = POWER_SEQ_REGULATOR, > + .regulator.regulator = "power", > + .regulator.enable = 1, > + }, This may be easier to read as: .type = POWER_SEQ_REGULATOR, .regulator { .regulator = "power", .enable = 1, } Also, why not rename the .regulator field to .name? That describes better what it is and removes the redundancy of having the structure named regulator and a regulator field within. > + { > + .type = POWER_SEQ_DELAY, > + .delay.delay_us = 10000, > + }, > + { > + .type = POWER_SEQ_GPIO, > + .gpio.gpio = 110, > + .gpio.enable = 1, > + }, Same comments as for the regulator step above. Also, since enable is a boolean field, maybe you should use true and false instead. > + }, > +}; > + > +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. 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. > 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 <acourbot@nvidia.com> > + * > + * Copyright (c) 2012 NVIDIA Corporation. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + */ > + > +#include <linux/power_seq.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/device.h> > +#include <linux/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 const? > + > +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. 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. > + */ > + default: > + return -EINVAL; > + } > + > + if (err < 0) > + return err; > + > + return 0; This can probably be collapsed to just "return err;", can't it? > +} > + > +int power_seq_run(struct power_seq *seq) > +{ > + struct device *dev = seq->dev; > + int err, cpt; Any reason why you call the loop variable cpt instead of something more canonical such as i? Also it should be of type unsigned int. > + > + 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) Maybe rename this to of_power_seq_parse_enable_properties() to make it more obvious that it is actually parsing data. It's an awfully long name for a function, though. > +{ > + 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++) { Maybe this should be renamed to POWER_SEQ_MAX_TYPE, POWER_SEQ_TYPE_MAX, POWER_SEQ_NUM_TYPES or some such. I had assumed POWER_SEQ_MAX would mean the maximum length of a power sequence. > + 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); I'm not sure if there's a limit on the length of DT property names, but maybe using kasprintf would be a better idea here. > + 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; > +} Given the size of this function, I think it might become more readable if it were split into separate parse functions for each type of resource. It will also allow you to get rid of this #ifdef within the function. > + > +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; Shouldn't the first term be sizeof(*pseq)? > + 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; kstrtouint()? Also this is somewhat ugly. Perhaps adding #address-cells and #size-cells properties isn't that bad after all. > + > + 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) I think it is customary to put the return value type on the same line as the static modifier. > +{ > + 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 <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..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 <acourbot@nvidia.com> > + * > + * Copyright (c) 2012 NVIDIA Corporation. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + */ > + > +#ifndef __LINUX_POWER_SEQ_H > +#define __LINUX_POWER_SEQ_H > + > +#include <linux/types.h> > + > +struct device; > +struct regulator; > +struct pwm_device; > +struct device_node; > + > +/** > + * The different kinds of resources that can be controlled during the sequences. > + */ > +enum power_seq_res_type { > + POWER_SEQ_DELAY, > + POWER_SEQ_REGULATOR, > + POWER_SEQ_PWM, > + POWER_SEQ_GPIO, > + POWER_SEQ_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); I believe kernel-doc comments should precede the implementation, not the prototype. Also the kernel-doc comment doesn't correspond to what is described in Documentation/kernel-doc-nano-HOWTO.txt. Thierry > + > +/** > + * 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 > > > [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 7:42 ` Thierry Reding @ 2012-08-16 9:19 ` Alex Courbot 2012-08-16 9:52 ` Thierry Reding 0 siblings, 1 reply; 33+ messages in thread From: Alex Courbot @ 2012-08-16 9:19 UTC (permalink / raw) To: Thierry Reding Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc 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: >> Some device drivers (panel backlights especially) need to follow precise >> sequences for powering on and off, involving gpios, regulators, PWMs >> with a precise powering order and delays to respect between each steps. >> These sequences are board-specific, and do not belong to a particular >> driver - therefore they have been performed by board-specific hook >> functions to far. >> >> With the advent of the device tree and of ARM kernels that are not >> board-tied, we cannot rely on these board-specific hooks anymore but >> need a way to implement these sequences in a portable manner. This patch >> introduces a simple interpreter that can execute such power sequences >> encoded either as platform data or within the device tree. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> --- >> .../devicetree/bindings/power_seq/power_seq.txt | 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 > > "named such that"? > >> +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: > > I think the "currently" can be dropped. > >> +- "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. > > I find this part slightly confusing. It doesn't seem quite obvious from > the text that "delay", "regulator", "pwm" and "gpio" are also actual > property names. > > It might also be worth saying that the "enable" and "disable" properties > are boolean in nature and don't need a value. > > Then again this becomes much clearer with the example below, so maybe > I'm just being too picky here. This could clearly be improved. I have added a sentence before that says that every step must define one property named after a supported resource type - that should help. Then, as you said, the example should clear all remaining doubts. > >> + >> +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. > > "unknown to the driver"? > >> + >> +This was previously addressed by having hooks in the device's platform data that >> +are called whenever the state of the device might reflect a power change. This >> +approach, however, introduces board-dependant code into the kernel and is not >> +compatible with the device tree. >> + >> +The Runtime Interpreted Power Sequences (or power sequences for short) aims at > > "... Sequences [...] aim"? > >> +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: > > Again, I don't see a need for "so far" here. It implies that new types > may be added. While it is quite possible and maybe even likely to happen > the new types can be added to the documentation at the same time. > >> +- 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 > > "each of its steps" > >> +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, > > I think num_steps would be more canonical. > >> + .steps = { >> + { >> + .type = POWER_SEQ_REGULATOR, >> + .regulator.regulator = "power", >> + .regulator.enable = 1, >> + }, > > This may be easier to read as: > > .type = POWER_SEQ_REGULATOR, > .regulator { > .regulator = "power", > .enable = 1, > } > > Also, why not rename the .regulator field to .name? That describes > better what it is and removes the redundancy of having the structure > named regulator and a regulator field within. That's right - this is a remain from the time the union was not used to differenciate the resource types. > >> + { >> + .type = POWER_SEQ_DELAY, >> + .delay.delay_us = 10000, >> + }, >> + { >> + .type = POWER_SEQ_GPIO, >> + .gpio.gpio = 110, >> + .gpio.enable = 1, >> + }, > > Same comments as for the regulator step above. Also, since enable is a > boolean field, maybe you should use true and false instead. > >> + }, >> +}; >> + >> +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. > > 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. 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. >> 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 <acourbot@nvidia.com> >> + * >> + * Copyright (c) 2012 NVIDIA Corporation. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; version 2 of the License. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + */ >> + >> +#include <linux/power_seq.h> >> +#include <linux/module.h> >> +#include <linux/err.h> >> +#include <linux/device.h> >> +#include <linux/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 const? > >> + >> +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. > >> + */ >> + default: >> + return -EINVAL; >> + } >> + >> + if (err < 0) >> + return err; >> + >> + return 0; > > This can probably be collapsed to just "return err;", can't it? Totally. > >> +} >> + >> +int power_seq_run(struct power_seq *seq) >> +{ >> + struct device *dev = seq->dev; >> + int err, cpt; > > Any reason why you call the loop variable cpt instead of something more > canonical such as i? Also it should be of type unsigned int. Fixed. > >> + >> + 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) > > Maybe rename this to of_power_seq_parse_enable_properties() to make it > more obvious that it is actually parsing data. It's an awfully long name > for a function, though. > >> +{ >> + 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++) { > > Maybe this should be renamed to POWER_SEQ_MAX_TYPE, POWER_SEQ_TYPE_MAX, > POWER_SEQ_NUM_TYPES or some such. I had assumed POWER_SEQ_MAX would mean > the maximum length of a power sequence. > >> + 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); > > I'm not sure if there's a limit on the length of DT property names, but > maybe using kasprintf would be a better idea here. According to the regulator code (from which this is inspired) this is the case - I try to limit dynamic memory allocations as much as possible. > >> + 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; >> +} > > Given the size of this function, I think it might become more readable > if it were split into separate parse functions for each type of > resource. It will also allow you to get rid of this #ifdef within the > function. Right. > >> + >> +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; > > Shouldn't the first term be sizeof(*pseq)? Ouch. >_< Thanks. > >> + 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; > > kstrtouint()? Also this is somewhat ugly. Perhaps adding #address-cells > and #size-cells properties isn't that bad after all. I really prefer this solution to #address-cells and #size-cells personally. Using these makes sense when you need to refer to the DT node through a phandle, which is definitely not the case here. Having them would just be confusing and error-prone. > >> + >> + 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) > > I think it is customary to put the return value type on the same line as > the static modifier. > >> +{ >> + 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 <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..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 <acourbot@nvidia.com> >> + * >> + * Copyright (c) 2012 NVIDIA Corporation. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; version 2 of the License. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + */ >> + >> +#ifndef __LINUX_POWER_SEQ_H >> +#define __LINUX_POWER_SEQ_H >> + >> +#include <linux/types.h> >> + >> +struct device; >> +struct regulator; >> +struct pwm_device; >> +struct device_node; >> + >> +/** >> + * The different kinds of resources that can be controlled during the sequences. >> + */ >> +enum power_seq_res_type { >> + POWER_SEQ_DELAY, >> + POWER_SEQ_REGULATOR, >> + POWER_SEQ_PWM, >> + POWER_SEQ_GPIO, >> + POWER_SEQ_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); > > I believe kernel-doc comments should precede the implementation, not the > prototype. Also the kernel-doc comment doesn't correspond to what is > described in Documentation/kernel-doc-nano-HOWTO.txt. Right, fixed it. I never really understood why we don't document the prototype, since that's usually what the user of a function will look at first. I have also addressed all the typos and naming issues you mentioned. Thanks! Alex. > > Thierry > >> + >> +/** >> + * 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 >> >> >> > > * Unknown Key > * 0x7F3EB3A1 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 9:19 ` Alex Courbot @ 2012-08-16 9:52 ` Thierry Reding 2012-08-16 10:33 ` Alex Courbot 0 siblings, 1 reply; 33+ messages in thread From: Thierry Reding @ 2012-08-16 9:52 UTC (permalink / raw) To: Alex Courbot Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 6392 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 9:52 ` Thierry Reding @ 2012-08-16 10:33 ` Alex Courbot 2012-08-16 10:52 ` Thierry Reding 0 siblings, 1 reply; 33+ messages in thread From: Alex Courbot @ 2012-08-16 10:33 UTC (permalink / raw) To: Thierry Reding Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 10:33 ` Alex Courbot @ 2012-08-16 10:52 ` Thierry Reding 0 siblings, 0 replies; 33+ messages in thread From: Thierry Reding @ 2012-08-16 10:52 UTC (permalink / raw) To: Alex Courbot Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 6396 bytes --] On Thu, Aug 16, 2012 at 07:33:27PM +0900, Alex Courbot wrote: > 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. I see. Perhaps I'm just bugged by the interface being a simple list. If it was something just a little more sophisticated, like a very primitive resource manager attached to one device, I would be appeased. Maybe an opaque structure that carries the list and hides it for drivers would do as well. > >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. I understand. You could use an idr (include/linux/idr.h) for this purpose. However I don't know if this would be any better than the above. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot 2012-08-16 7:42 ` Thierry Reding @ 2012-08-16 14:14 ` Mark Brown 2012-08-16 18:38 ` Stephen Warren 2012-08-21 7:44 ` Tomi Valkeinen 3 siblings, 0 replies; 33+ messages in thread From: Mark Brown @ 2012-08-16 14:14 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote: > + power-off-sequence { > + step0 { > + gpio = "enable"; > + disable; I'd change the name to "reset" or something in the example - avoids any confusion with the action. > +#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; The regulator and GPIO APIs should stub themselves out adequately to not need the ifdefs at least for regulator I'd use the stubs since... > + /* > + * should never happen unless the sequence includes a step which > + * type does not have support compiled in > + */ > + default: > + return -EINVAL; ...this probably isn't what's meant. It might also be nice to have support for bulk_enable() but that's definitely something that could be added later. > + err = power_seq_step_run(&seq->steps[cpt]); > + if (err) { > + dev_err(dev, "error %d while running power sequence!\n", > + err); > + return err; I'd also log at least the step number, it'll make diagnostics easier. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot 2012-08-16 7:42 ` Thierry Reding 2012-08-16 14:14 ` Mark Brown @ 2012-08-16 18:38 ` Stephen Warren 2012-08-16 18:47 ` Mark Brown ` (3 more replies) 2012-08-21 7:44 ` Tomi Valkeinen 3 siblings, 4 replies; 33+ messages in thread From: Stephen Warren @ 2012-08-16 18:38 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On 08/16/2012 12:08 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. > diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt > +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. Device tree bindings shouldn't reference Linux documentation; the bindings are supposed to be OS-agnostic. > +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. That's a little roundabout. That might be better as simply: Valid power sequence names are defined by each device's binding. For a power sequence named "foo", a node named "foo-power-sequence" defines that sequence. > +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. Node names shouldn't be interpreted by the code; nodes should all be named after the type of object the represent. Hence, every step should be named just "step" for example. The node's unit address (@0) should be used to distinguish the nodes. This requires reg properties within each node to match the unit address, and hence #address-cells and #size-cells properties in the power sequence itself. Note that this somewhat conflicts with accessing the top-level power sequence by name too; perhaps that should be re-thought too. I must admit this DT rule kinda sucks. > +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. I think you need to add a property that indicates what type of resource each step applies to. Sure, this is implicit in that if a "gpio" property exists, the step is a GPIO step, but in order to make that work, you'd have to search each node for each possible resource type's property name. It'd be far better to read a single type="gpio" property, then parse the step based on that. > +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; > + }; > + }; > + }; I notice that for clocks, pwms, and interrupts, the initial step of the lookup is via a single property that lists all know resources of that type. Regulators and GPIOs don't follow this style though. Using the same mechanism for power-sequences would yield something like the following, which would avoid the "node names must be significant" issue I mention above, although it does make everything rather more wordy. [start my proposal] > backlight { > compatible = "pwm-backlight"; > > /* resources used by the sequences */ > pwms = <&pwm 2 5000000>; > pwm-names = "backlight"; > power-supply = <&backlight_reg>; > bl-enable-gpio = <&gpio 28 0>; > pwr-seqs = <&bl_on &bl_off>; > pwr-seq-names = "on", "off"; > > #address-cells = <1>; > #size-cells = <0>; > > bl_on: pwr-seq@0 { > reg = <0>; > #address-cells = <1>; > #size-cells = <0>; > step@0 { > reg = <0>; > type = "regulator"; > regulator = "power"; > enable; > }; > step@1 { > reg = <1>; > type = "delay"; > delay = <10000>; > }; > step@2 { > reg = <2>; > type = "pwm"; > pwm = "backlight"; > enable; > }; > step@3 { > reg = <3>; > type = "gpio"; > gpio = "bl-enable"; > enable; > }; > }; > > bl_off: pwr-seq@1 { > reg = <1>; > #address-cells = <1>; > #size-cells = <0>; > step@0 { > reg = <0>; > type = "gpio"; > gpio = "bl-enable"; > disable; > }; > step@1 { > reg = <1>; > type = "pwm"; > pwm = "backlight"; > disable; > }; > step@2 { > reg = <2>; > type = "delay"; > delay = <10000>; > }; > step@3 { > reg = <3>; > type = "regulator"; > regulator = "power"; > disable; > }; > }; > }; > [end my proposal] > diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > +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. I see in other parts of the thread, there has been discussion re: needing the separate ress parameter, and requiring the driver to pass this in to multiple power_seq_build calls. The way the pinctrl subsystem solved this was to have a single function that parsed all pinctrl setting (c.f. all power sequences) at once, and return a single handle. Later, the driver passes this handle pinctrl_lookup_state(), along with the requested state (c.f. sequence name) to search for, and finally passes that handle to pinctrl_select_state(). Doing something similar here would result in: struct power_seqs *power_seq_get(struct device *dev); void power_seq_put(struct power_seqs *seqs); struct power_seq *power_seq_lookup(struct power_seqs *seqs, const char *name); int power_seq_executestruct power_seq *seq); struct power_seqs *devm_power_seq_get(struct device *dev); void devm_power_seq_put(struct power_seqs *seqs); > +On success, the function returns a devm allocated resolved sequence that is Perhaps the function should be named devm_power_seq_build(), in order to make this obvious to people who only read the client code, and not this documentation. > +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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 18:38 ` Stephen Warren @ 2012-08-16 18:47 ` Mark Brown 2012-08-16 18:57 ` Stephen Warren 2012-08-16 19:49 ` Thierry Reding ` (2 subsequent siblings) 3 siblings, 1 reply; 33+ messages in thread From: Mark Brown @ 2012-08-16 18:47 UTC (permalink / raw) To: Stephen Warren Cc: Alexandre Courbot, Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: > Note that this somewhat conflicts with accessing the top-level power > sequence by name too; perhaps that should be re-thought too. I must > admit this DT rule kinda sucks. Given that currently the information there is useless and ignored is there any reason we can't just change the rule? It'd be rather more constructive... ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 18:47 ` Mark Brown @ 2012-08-16 18:57 ` Stephen Warren 2012-08-16 19:35 ` Stephen Warren 0 siblings, 1 reply; 33+ messages in thread From: Stephen Warren @ 2012-08-16 18:57 UTC (permalink / raw) To: Mark Brown Cc: Alexandre Courbot, Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On 08/16/2012 12:47 PM, Mark Brown wrote: > On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: > >> Note that this somewhat conflicts with accessing the top-level power >> sequence by name too; perhaps that should be re-thought too. I must >> admit this DT rule kinda sucks. > > Given that currently the information there is useless and ignored is > there any reason we can't just change the rule? It'd be rather more > constructive... I'll start a new thread on that topic and see. I would simplify matters a lot... ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 18:57 ` Stephen Warren @ 2012-08-16 19:35 ` Stephen Warren 0 siblings, 0 replies; 33+ messages in thread From: Stephen Warren @ 2012-08-16 19:35 UTC (permalink / raw) To: Mark Brown Cc: linux-fbdev, Stephen Warren, linux-doc, linux-kernel, Rob Herring, Anton Vorontsov, Alexandre Courbot, linux-tegra, David Woodhouse, devicetree-discuss On 08/16/2012 12:57 PM, Stephen Warren wrote: > On 08/16/2012 12:47 PM, Mark Brown wrote: >> On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: >> >>> Note that this somewhat conflicts with accessing the top-level power >>> sequence by name too; perhaps that should be re-thought too. I must >>> admit this DT rule kinda sucks. >> >> Given that currently the information there is useless and ignored is >> there any reason we can't just change the rule? It'd be rather more >> constructive... > > I'll start a new thread on that topic and see. I would simplify matters > a lot... In case anyone wants to follow it, it's at: https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-August/018502.html ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 18:38 ` Stephen Warren 2012-08-16 18:47 ` Mark Brown @ 2012-08-16 19:49 ` Thierry Reding 2012-08-16 21:10 ` Mitch Bradley 2012-08-17 8:52 ` Alex Courbot 3 siblings, 0 replies; 33+ messages in thread From: Thierry Reding @ 2012-08-16 19:49 UTC (permalink / raw) To: Stephen Warren Cc: Alexandre Courbot, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 2258 bytes --] On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: > On 08/16/2012 12:08 AM, Alexandre Courbot wrote: > > diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > > > +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. > > I see in other parts of the thread, there has been discussion re: > needing the separate ress parameter, and requiring the driver to pass > this in to multiple power_seq_build calls. > > The way the pinctrl subsystem solved this was to have a single function > that parsed all pinctrl setting (c.f. all power sequences) at once, and > return a single handle. Later, the driver passes this handle > pinctrl_lookup_state(), along with the requested state (c.f. sequence > name) to search for, and finally passes that handle to > pinctrl_select_state(). Doing something similar here would result in: > > struct power_seqs *power_seq_get(struct device *dev); > void power_seq_put(struct power_seqs *seqs); > struct power_seq *power_seq_lookup(struct power_seqs *seqs, > const char *name); > int power_seq_executestruct power_seq *seq); > > struct power_seqs *devm_power_seq_get(struct device *dev); > void devm_power_seq_put(struct power_seqs *seqs); Hehe, this looks very much like what I had in mind when I proposed this during the last version of this series. The nice thing about this is that the API is very much in line with how other subsystems work. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 18:38 ` Stephen Warren 2012-08-16 18:47 ` Mark Brown 2012-08-16 19:49 ` Thierry Reding @ 2012-08-16 21:10 ` Mitch Bradley 2012-08-17 23:04 ` Mark Brown 2012-08-17 8:52 ` Alex Courbot 3 siblings, 1 reply; 33+ messages in thread From: Mitch Bradley @ 2012-08-16 21:10 UTC (permalink / raw) To: Stephen Warren Cc: Alexandre Courbot, linux-fbdev, Mark Brown, Stephen Warren, linux-doc, linux-kernel, Rob Herring, Anton Vorontsov, linux-tegra, David Woodhouse, devicetree-discuss On 8/16/2012 8:38 AM, Stephen Warren wrote: > On 08/16/2012 12:08 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. > >> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt > >> +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. > > Device tree bindings shouldn't reference Linux documentation; the > bindings are supposed to be OS-agnostic. While it is true that bindings should try to be OS-agnostic, there is the practical matter of where to put documentation so that it is widely accessible. The Linux source tree is one of the most accessible things there is, considering how widely it is replicated. As the original instigator of the policy that the device tree should describe the hardware "OS-neutrally", I personally don't have a problem with bindings referring to Linux documentation. I wouldn't like references to proprietary and inaccessible documentation. > >> +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. > > That's a little roundabout. That might be better as simply: > > Valid power sequence names are defined by each device's binding. For a > power sequence named "foo", a node named "foo-power-sequence" defines > that sequence. > >> +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. > > Node names shouldn't be interpreted by the code; nodes should all be > named after the type of object the represent. Hence, every step should > be named just "step" for example. > > The node's unit address (@0) should be used to distinguish the nodes. > This requires reg properties within each node to match the unit address, > and hence #address-cells and #size-cells properties in the power > sequence itself. > > Note that this somewhat conflicts with accessing the top-level power > sequence by name too; perhaps that should be re-thought too. I must > admit this DT rule kinda sucks. > >> +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. > > I think you need to add a property that indicates what type of resource > each step applies to. Sure, this is implicit in that if a "gpio" > property exists, the step is a GPIO step, but in order to make that > work, you'd have to search each node for each possible resource type's > property name. It'd be far better to read a single type="gpio" property, > then parse the step based on that. > >> +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; >> + }; >> + }; >> + }; > > I notice that for clocks, pwms, and interrupts, the initial step of the > lookup is via a single property that lists all know resources of that > type. Regulators and GPIOs don't follow this style though. Using the > same mechanism for power-sequences would yield something like the > following, which would avoid the "node names must be significant" issue > I mention above, although it does make everything rather more wordy. > > [start my proposal] >> backlight { >> compatible = "pwm-backlight"; >> >> /* resources used by the sequences */ >> pwms = <&pwm 2 5000000>; >> pwm-names = "backlight"; >> power-supply = <&backlight_reg>; >> bl-enable-gpio = <&gpio 28 0>; >> pwr-seqs = <&bl_on &bl_off>; >> pwr-seq-names = "on", "off"; >> >> #address-cells = <1>; >> #size-cells = <0>; >> >> bl_on: pwr-seq@0 { >> reg = <0>; >> #address-cells = <1>; >> #size-cells = <0>; >> step@0 { >> reg = <0>; >> type = "regulator"; >> regulator = "power"; >> enable; >> }; >> step@1 { >> reg = <1>; >> type = "delay"; >> delay = <10000>; >> }; >> step@2 { >> reg = <2>; >> type = "pwm"; >> pwm = "backlight"; >> enable; >> }; >> step@3 { >> reg = <3>; >> type = "gpio"; >> gpio = "bl-enable"; >> enable; >> }; >> }; >> >> bl_off: pwr-seq@1 { >> reg = <1>; >> #address-cells = <1>; >> #size-cells = <0>; >> step@0 { >> reg = <0>; >> type = "gpio"; >> gpio = "bl-enable"; >> disable; >> }; >> step@1 { >> reg = <1>; >> type = "pwm"; >> pwm = "backlight"; >> disable; >> }; >> step@2 { >> reg = <2>; >> type = "delay"; >> delay = <10000>; >> }; >> step@3 { >> reg = <3>; >> type = "regulator"; >> regulator = "power"; >> disable; >> }; >> }; >> }; >> > [end my proposal] > >> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > >> +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. > > I see in other parts of the thread, there has been discussion re: > needing the separate ress parameter, and requiring the driver to pass > this in to multiple power_seq_build calls. > > The way the pinctrl subsystem solved this was to have a single function > that parsed all pinctrl setting (c.f. all power sequences) at once, and > return a single handle. Later, the driver passes this handle > pinctrl_lookup_state(), along with the requested state (c.f. sequence > name) to search for, and finally passes that handle to > pinctrl_select_state(). Doing something similar here would result in: > > struct power_seqs *power_seq_get(struct device *dev); > void power_seq_put(struct power_seqs *seqs); > struct power_seq *power_seq_lookup(struct power_seqs *seqs, > const char *name); > int power_seq_executestruct power_seq *seq); > > struct power_seqs *devm_power_seq_get(struct device *dev); > void devm_power_seq_put(struct power_seqs *seqs); > >> +On success, the function returns a devm allocated resolved sequence that is > > Perhaps the function should be named devm_power_seq_build(), in order to > make this obvious to people who only read the client code, and not this > documentation. > >> +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. > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 21:10 ` Mitch Bradley @ 2012-08-17 23:04 ` Mark Brown 0 siblings, 0 replies; 33+ messages in thread From: Mark Brown @ 2012-08-17 23:04 UTC (permalink / raw) To: Mitch Bradley Cc: Stephen Warren, Alexandre Courbot, linux-fbdev, Stephen Warren, linux-doc, linux-kernel, Rob Herring, Anton Vorontsov, linux-tegra, David Woodhouse, devicetree-discuss [-- Attachment #1: Type: text/plain, Size: 1206 bytes --] On Thu, Aug 16, 2012 at 11:10:30AM -1000, Mitch Bradley wrote: > On 8/16/2012 8:38 AM, Stephen Warren wrote: > > Device tree bindings shouldn't reference Linux documentation; the > > bindings are supposed to be OS-agnostic. > While it is true that bindings should try to be OS-agnostic, there is > the practical matter of where to put documentation so that it is widely > accessible. The Linux source tree is one of the most accessible things > there is, considering how widely it is replicated. > As the original instigator of the policy that the device tree should > describe the hardware "OS-neutrally", I personally don't have a problem > with bindings referring to Linux documentation. I wouldn't like > references to proprietary and inaccessible documentation. OS agnosticness isn't the only issue here - the other problem with using Linux documentation is that except for things that are specifically userspace interfaces and the DT bindings nothing is intended to be stable so bindings defined in terms of Linux documentation may randomly change. We're not doing an awesome job of that with DT right now but we should try and so we ought to avoid including non-ABI things in ABIs like this. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 18:38 ` Stephen Warren ` (2 preceding siblings ...) 2012-08-16 21:10 ` Mitch Bradley @ 2012-08-17 8:52 ` Alex Courbot 3 siblings, 0 replies; 33+ messages in thread From: Alex Courbot @ 2012-08-17 8:52 UTC (permalink / raw) To: Stephen Warren Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On 08/17/2012 03:38 AM, Stephen Warren wrote: > On 08/16/2012 12:08 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. > >> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt > >> +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. > > Device tree bindings shouldn't reference Linux documentation; the > bindings are supposed to be OS-agnostic. Ok, I should be able to do without this reference anyway. > >> +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. > > That's a little roundabout. That might be better as simply: > > Valid power sequence names are defined by each device's binding. For a > power sequence named "foo", a node named "foo-power-sequence" defines > that sequence. > >> +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. > > Node names shouldn't be interpreted by the code; nodes should all be > named after the type of object the represent. Hence, every step should > be named just "step" for example. > > The node's unit address (@0) should be used to distinguish the nodes. > This requires reg properties within each node to match the unit address, > and hence #address-cells and #size-cells properties in the power > sequence itself. While this logic is perfectly suitable and adapted for devices, I think we should keep in mind that power sequences and their steps are not devices, but just arbitrary bits of information. Having adress cells and reg properties is useful when one needs to reference a node through a phandle, but this is *never* going to happen with sequence steps (unless we go insane and decide to allow goto-like statements :P). So having #address-cells, #size-cells, and reg serve absolutely no purpose here but cluttering the DT. If there is a hard rule that needs to be enforced, then let that be, but the other discussion you started (thanks for that by the way) does not seem to suggest so. > Note that this somewhat conflicts with accessing the top-level power > sequence by name too; perhaps that should be re-thought too. I must > admit this DT rule kinda sucks. > >> +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. > > I think you need to add a property that indicates what type of resource > each step applies to. Sure, this is implicit in that if a "gpio" > property exists, the step is a GPIO step, but in order to make that > work, you'd have to search each node for each possible resource type's > property name. It'd be far better to read a single type="gpio" property, > then parse the step based on that. Indeed right now all resource types must be checked. Having a type property would make that easier. I like to keep the DT as compact and expressive as possible, but I guess one more property per step would not hurt and is perhaps easier to understand too. >> +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; >> + }; >> + }; >> + }; > > I notice that for clocks, pwms, and interrupts, the initial step of the > lookup is via a single property that lists all know resources of that > type. Regulators and GPIOs don't follow this style though. Using the > same mechanism for power-sequences would yield something like the > following, which would avoid the "node names must be significant" issue > I mention above, although it does make everything rather more wordy. > > [start my proposal] >> backlight { >> compatible = "pwm-backlight"; >> >> /* resources used by the sequences */ >> pwms = <&pwm 2 5000000>; >> pwm-names = "backlight"; >> power-supply = <&backlight_reg>; >> bl-enable-gpio = <&gpio 28 0>; >> pwr-seqs = <&bl_on &bl_off>; >> pwr-seq-names = "on", "off"; >> >> #address-cells = <1>; >> #size-cells = <0>; >> >> bl_on: pwr-seq@0 { >> reg = <0>; >> #address-cells = <1>; >> #size-cells = <0>; >> step@0 { >> reg = <0>; >> type = "regulator"; >> regulator = "power"; >> enable; >> }; >> step@1 { >> reg = <1>; >> type = "delay"; >> delay = <10000>; >> }; >> step@2 { >> reg = <2>; >> type = "pwm"; >> pwm = "backlight"; >> enable; >> }; >> step@3 { >> reg = <3>; >> type = "gpio"; >> gpio = "bl-enable"; >> enable; >> }; >> }; >> >> bl_off: pwr-seq@1 { >> reg = <1>; >> #address-cells = <1>; >> #size-cells = <0>; >> step@0 { >> reg = <0>; >> type = "gpio"; >> gpio = "bl-enable"; >> disable; >> }; >> step@1 { >> reg = <1>; >> type = "pwm"; >> pwm = "backlight"; >> disable; >> }; >> step@2 { >> reg = <2>; >> type = "delay"; >> delay = <10000>; >> }; >> step@3 { >> reg = <3>; >> type = "regulator"; >> regulator = "power"; >> disable; >> }; >> }; >> }; >> > [end my proposal] Mmmm, so looking at this, what strikes me is that the amount of unused/redundant information is actually higher than the amount of information the driver will effectively use. If these conventions can be ignored here, we definitely should do that. > >> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > >> +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. > > I see in other parts of the thread, there has been discussion re: > needing the separate ress parameter, and requiring the driver to pass > this in to multiple power_seq_build calls. > > The way the pinctrl subsystem solved this was to have a single function > that parsed all pinctrl setting (c.f. all power sequences) at once, and > return a single handle. Later, the driver passes this handle > pinctrl_lookup_state(), along with the requested state (c.f. sequence > name) to search for, and finally passes that handle to > pinctrl_select_state(). Doing something similar here would result in: > > struct power_seqs *power_seq_get(struct device *dev); > void power_seq_put(struct power_seqs *seqs); > struct power_seq *power_seq_lookup(struct power_seqs *seqs, > const char *name); > int power_seq_executestruct power_seq *seq); > > struct power_seqs *devm_power_seq_get(struct device *dev); > void devm_power_seq_put(struct power_seqs *seqs); Well, if both of you bring this then I have no choice but seriously consider that. :) If other subsystems follow the same scheme then this is an additional point for this. > >> +On success, the function returns a devm allocated resolved sequence that is > > Perhaps the function should be named devm_power_seq_build(), in order to > make this obvious to people who only read the client code, and not this > documentation. Agreed. Thanks, Alex. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot ` (2 preceding siblings ...) 2012-08-16 18:38 ` Stephen Warren @ 2012-08-21 7:44 ` Tomi Valkeinen 2012-08-21 8:22 ` Alex Courbot 3 siblings, 1 reply; 33+ messages in thread From: Tomi Valkeinen @ 2012-08-21 7:44 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 1387 bytes --] Hi, On Thu, 2012-08-16 at 15:08 +0900, Alexandre Courbot 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? Tomi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 7:44 ` Tomi Valkeinen @ 2012-08-21 8:22 ` Alex Courbot 2012-08-21 8:33 ` Thierry Reding 0 siblings, 1 reply; 33+ messages in thread From: Alex Courbot @ 2012-08-21 8:22 UTC (permalink / raw) To: Tomi Valkeinen Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc 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? Alex. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 8:22 ` Alex Courbot @ 2012-08-21 8:33 ` Thierry Reding 2012-08-21 8:53 ` Alex Courbot 2012-08-21 8:57 ` Tomi Valkeinen 0 siblings, 2 replies; 33+ messages in thread From: Thierry Reding @ 2012-08-21 8:33 UTC (permalink / raw) To: Alex Courbot Cc: Tomi Valkeinen, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 3880 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 8:33 ` Thierry Reding @ 2012-08-21 8:53 ` Alex Courbot 2012-08-21 8:57 ` Tomi Valkeinen 1 sibling, 0 replies; 33+ messages in thread From: Alex Courbot @ 2012-08-21 8:53 UTC (permalink / raw) To: Thierry Reding Cc: Tomi Valkeinen, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On Tuesday 21 August 2012 16:33:30 Thierry Reding wrote: > 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. Yes, I can imagine what a mess it would become it we had one driver for every panel out there which sole purpose would be to define power sequences over more generic drivers. That reassures me about the usefulness of this work. Another (small) benefit of power sequences over specific drivers is that being defined in the DT, they would allow an old kernel to operate a newer device if the base driver is the same. Alex. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 8:33 ` Thierry Reding 2012-08-21 8:53 ` Alex Courbot @ 2012-08-21 8:57 ` Tomi Valkeinen 2012-08-21 9:13 ` Thierry Reding 1 sibling, 1 reply; 33+ messages in thread From: Tomi Valkeinen @ 2012-08-21 8:57 UTC (permalink / raw) To: Thierry Reding Cc: Alex Courbot, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 1912 bytes --] On Tue, 2012-08-21 at 10:33 +0200, Thierry Reding wrote: > 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. Right. I think my main concern is the use of DT data, not power sequences as such. I've been going back and forth in my mind with this issue with OMAP also. The question is: what stuff belongs to DT data and what belongs to the kernel? I've been trying to go to the direction where DT is used to describe the HW connections of different IP blocks and to pass board specific configuration. Everything else is in the driver. This doesn't mean that we'd have a separate driver for each device. For example, we have a generic panel driver in OMAP, which contains a kind of small panel database. The panel database contains the name of the panel as a key, and panel specific configuration as a value. This configuration could also contain some kind of generic power sequence. I'd like to require the board developer to only fill in to the DT data what panel he is using, and how it's connected on his board. Not panel's internal functionality. The one benefit I see with DT based approach is that if we have, say, 10000 panels, we'd have quite a big database in kernel memory and a board may only need one or two of those. But perhaps that could be helped with the use of __initdata. Tomi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 8:57 ` Tomi Valkeinen @ 2012-08-21 9:13 ` Thierry Reding 2012-08-21 9:54 ` Tomi Valkeinen 0 siblings, 1 reply; 33+ messages in thread From: Thierry Reding @ 2012-08-21 9:13 UTC (permalink / raw) To: Tomi Valkeinen Cc: Alex Courbot, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 2921 bytes --] On Tue, Aug 21, 2012 at 11:57:45AM +0300, Tomi Valkeinen wrote: > On Tue, 2012-08-21 at 10:33 +0200, Thierry Reding wrote: > > > 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. > > Right. I think my main concern is the use of DT data, not power > sequences as such. I've been going back and forth in my mind with this > issue with OMAP also. > > The question is: what stuff belongs to DT data and what belongs to the > kernel? I've been trying to go to the direction where DT is used to > describe the HW connections of different IP blocks and to pass board > specific configuration. Everything else is in the driver. > > This doesn't mean that we'd have a separate driver for each device. For > example, we have a generic panel driver in OMAP, which contains a kind > of small panel database. The panel database contains the name of the > panel as a key, and panel specific configuration as a value. This > configuration could also contain some kind of generic power sequence. I see. I do like the idea of it, because it is more straightforward than representing the whole sequence in DT. Matching could be done using the standard compatible property. However this also means we'll essentially just be moving the board code. Being in a central location it would be easier to refactor commonalities though. > I'd like to require the board developer to only fill in to the DT data > what panel he is using, and how it's connected on his board. Not panel's > internal functionality. The amount of work required by the board developer would largely depend on whether support for the panel is already present or not. For new panels this would mean that a new driver needs to be written, while representing the power sequence in DT might be easier to do, and readily available from some datasheet. > The one benefit I see with DT based approach is that if we have, say, > 10000 panels, we'd have quite a big database in kernel memory and a > board may only need one or two of those. But perhaps that could be > helped with the use of __initdata. I haven't worked with many different panels, so maybe I can't judge this too well, but if panel drivers were kept in a central location, then the number can be reduced by generalizing parts of existing drivers. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 9:13 ` Thierry Reding @ 2012-08-21 9:54 ` Tomi Valkeinen 2012-08-21 16:57 ` Mark Brown 2012-08-24 9:24 ` Alex Courbot 0 siblings, 2 replies; 33+ messages in thread From: Tomi Valkeinen @ 2012-08-21 9:54 UTC (permalink / raw) To: Thierry Reding Cc: Alex Courbot, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 2466 bytes --] On Tue, 2012-08-21 at 11:13 +0200, Thierry Reding wrote: > On Tue, Aug 21, 2012 at 11:57:45AM +0300, Tomi Valkeinen wrote: > > This doesn't mean that we'd have a separate driver for each device. For > > example, we have a generic panel driver in OMAP, which contains a kind > > of small panel database. The panel database contains the name of the > > panel as a key, and panel specific configuration as a value. This > > configuration could also contain some kind of generic power sequence. > > I see. I do like the idea of it, because it is more straightforward than > representing the whole sequence in DT. Matching could be done using the > standard compatible property. Right. > 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. 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. > Being in a central location it would be easier to refactor commonalities > though. > > > I'd like to require the board developer to only fill in to the DT data > > what panel he is using, and how it's connected on his board. Not panel's > > internal functionality. > > The amount of work required by the board developer would largely depend > on whether support for the panel is already present or not. For new > panels this would mean that a new driver needs to be written, while > representing the power sequence in DT might be easier to do, and readily > available from some datasheet. That's true. However, if we already have a generic driver for that type of panel, (which we would need anyway for the DT based approach), the developer only needs to add the name of the panel and the data for the power sequence to the panel driver's database, which is about the same amount of work as with DT. So it's really just a question of where to put the data in question, DT or driver. Both contain the same data, and the data structure may also be the same. In DT's case it just needs to be parsed first, whereas in database case you'll enter the data in structs. Tomi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 9:54 ` Tomi Valkeinen @ 2012-08-21 16:57 ` Mark Brown 2012-08-22 5:42 ` Thierry Reding 2012-08-24 9:24 ` Alex Courbot 1 sibling, 1 reply; 33+ messages in thread From: Mark Brown @ 2012-08-21 16:57 UTC (permalink / raw) To: Tomi Valkeinen Cc: Thierry Reding, Alex Courbot, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 873 bytes --] On Tue, Aug 21, 2012 at 12:54:20PM +0300, Tomi Valkeinen wrote: > However, if we already have a generic driver for that type of panel, > (which we would need anyway for the DT based approach), the developer > only needs to add the name of the panel and the data for the power > sequence to the panel driver's database, which is about the same amount > of work as with DT. > So it's really just a question of where to put the data in question, DT > or driver. Both contain the same data, and the data structure may also > be the same. In DT's case it just needs to be parsed first, whereas in > database case you'll enter the data in structs. I think the device tree idiomatic way of doing this is to have a bunch of .dtsi files for the panels which then get included by reference in the board files. This isn't helpful to people working on non-DT architectures though. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 16:57 ` Mark Brown @ 2012-08-22 5:42 ` Thierry Reding 0 siblings, 0 replies; 33+ messages in thread From: Thierry Reding @ 2012-08-22 5:42 UTC (permalink / raw) To: Mark Brown Cc: Tomi Valkeinen, Alex Courbot, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 1230 bytes --] On Tue, Aug 21, 2012 at 05:57:38PM +0100, Mark Brown wrote: > On Tue, Aug 21, 2012 at 12:54:20PM +0300, Tomi Valkeinen wrote: > > > However, if we already have a generic driver for that type of panel, > > (which we would need anyway for the DT based approach), the developer > > only needs to add the name of the panel and the data for the power > > sequence to the panel driver's database, which is about the same amount > > of work as with DT. > > > So it's really just a question of where to put the data in question, DT > > or driver. Both contain the same data, and the data structure may also > > be the same. In DT's case it just needs to be parsed first, whereas in > > database case you'll enter the data in structs. > > I think the device tree idiomatic way of doing this is to have a bunch > of .dtsi files for the panels which then get included by reference in > the board files. This isn't helpful to people working on non-DT > architectures though. One problem that's likely to crop up here again is that the panels won't be properly describable in the DT. Typically there is no device that can be addressed by the CPU. This is the same as for backlights and fixed regulators. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-21 9:54 ` Tomi Valkeinen 2012-08-21 16:57 ` Mark Brown @ 2012-08-24 9:24 ` Alex Courbot 2012-08-24 10:34 ` Tomi Valkeinen 1 sibling, 1 reply; 33+ messages in thread From: Alex Courbot @ 2012-08-24 9:24 UTC (permalink / raw) To: Tomi Valkeinen Cc: Thierry Reding, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences 2012-08-24 9:24 ` Alex Courbot @ 2012-08-24 10:34 ` Tomi Valkeinen 0 siblings, 0 replies; 33+ messages in thread From: Tomi Valkeinen @ 2012-08-24 10:34 UTC (permalink / raw) To: Alex Courbot Cc: Thierry Reding, Stephen Warren, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc [-- Attachment #1: Type: text/plain, Size: 3518 bytes --] 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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 2/3] pwm_backlight: use power sequences 2012-08-16 6:08 [PATCH v4 0/3] Runtime Interpreted Power Sequences Alexandre Courbot 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot @ 2012-08-16 6:08 ` Alexandre Courbot 2012-08-16 18:42 ` Stephen Warren 2012-08-16 6:08 ` [PATCH v4 3/3] tegra: add pwm backlight device tree nodes Alexandre Courbot 2012-08-16 21:47 ` [PATCH v4 0/3] Runtime Interpreted Power Sequences Rafael J. Wysocki 3 siblings, 1 reply; 33+ messages in thread From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw) To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc, Alexandre Courbot Make use of the power sequences specified in the device tree or platform data to control how the backlight is powered on and off. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- .../bindings/video/backlight/pwm-backlight.txt | 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 2/3] pwm_backlight: use power sequences 2012-08-16 6:08 ` [PATCH v4 2/3] pwm_backlight: use power sequences Alexandre Courbot @ 2012-08-16 18:42 ` Stephen Warren 0 siblings, 0 replies; 33+ messages in thread From: Stephen Warren @ 2012-08-16 18:42 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On 08/16/2012 12:08 AM, Alexandre Courbot wrote: > Make use of the power sequences specified in the device tree or platform > data to control how the backlight is powered on and off. > +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > 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 There's probably no need to move that entry just to change it? > 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 I don't think these really warrant mentioning here; such properties are part of the standard regulator and GPIO bindings, and are required as a side-effect of a power sequence using a resource of those types, rather than being something actively defined or needed directly by the pwm-backlight binding. > + - 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 For these two, I would personally simply say that the pwm-backlight binding requires that two power sequences named "on" and "off" must exists, and are to defined according to [1]. > [0]: Documentation/devicetree/bindings/pwm/pwm.txt > +[1]: Documentation/devicetree/bindings/power_seq/power_seq.txt ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 3/3] tegra: add pwm backlight device tree nodes 2012-08-16 6:08 [PATCH v4 0/3] Runtime Interpreted Power Sequences Alexandre Courbot 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot 2012-08-16 6:08 ` [PATCH v4 2/3] pwm_backlight: use power sequences Alexandre Courbot @ 2012-08-16 6:08 ` Alexandre Courbot 2012-08-16 18:45 ` Stephen Warren 2012-08-16 21:47 ` [PATCH v4 0/3] Runtime Interpreted Power Sequences Rafael J. Wysocki 3 siblings, 1 reply; 33+ messages in thread From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw) To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc, Alexandre Courbot Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 3/3] tegra: add pwm backlight device tree nodes 2012-08-16 6:08 ` [PATCH v4 3/3] tegra: add pwm backlight device tree nodes Alexandre Courbot @ 2012-08-16 18:45 ` Stephen Warren 0 siblings, 0 replies; 33+ messages in thread From: Stephen Warren @ 2012-08-16 18:45 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On 08/16/2012 12:08 AM, Alexandre Courbot wrote: > +++ b/arch/arm/boot/dts/tegra20-ventana.dts > + 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; > + }; Rather than add that as a separate node at the top-level, I think just add another sub-node to the "regulators" node. Oh, in fact it's already there in next-20120816; you just need to add a label. > +++ b/arch/arm/boot/dts/tegra20.dtsi > - pwm { > + pwm: pwm { > compatible = "nvidia,tegra20-pwm"; > reg = <0x7000a000 0x100>; It's pretty trivial I know, but I'd almost be tempted to make that a separate patch so that it could be cherry-picked somewhere without a tegra20-ventana.dts. But, perhaps that's silly. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 0/3] Runtime Interpreted Power Sequences 2012-08-16 6:08 [PATCH v4 0/3] Runtime Interpreted Power Sequences Alexandre Courbot ` (2 preceding siblings ...) 2012-08-16 6:08 ` [PATCH v4 3/3] tegra: add pwm backlight device tree nodes Alexandre Courbot @ 2012-08-16 21:47 ` Rafael J. Wysocki 2012-08-17 8:54 ` Alex Courbot 3 siblings, 1 reply; 33+ messages in thread From: Rafael J. Wysocki @ 2012-08-16 21:47 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On Thursday, August 16, 2012, Alexandre Courbot wrote: > 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 May I ask that the next version of this patchset be CCed to linux-pm@vger.kernel.org? Thanks, Rafael ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 0/3] Runtime Interpreted Power Sequences 2012-08-16 21:47 ` [PATCH v4 0/3] Runtime Interpreted Power Sequences Rafael J. Wysocki @ 2012-08-17 8:54 ` Alex Courbot 0 siblings, 0 replies; 33+ messages in thread From: Alex Courbot @ 2012-08-17 8:54 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely, Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann, Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss, linux-doc On 08/17/2012 06:47 AM, Rafael J. Wysocki wrote: > On Thursday, August 16, 2012, Alexandre Courbot wrote: >> 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 > > May I ask that the next version of this patchset be CCed to > linux-pm@vger.kernel.org? Will do. Alex. ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2012-08-24 10:34 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-16 6:08 [PATCH v4 0/3] Runtime Interpreted Power Sequences Alexandre Courbot 2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot 2012-08-16 7:42 ` Thierry Reding 2012-08-16 9:19 ` Alex Courbot 2012-08-16 9:52 ` Thierry Reding 2012-08-16 10:33 ` Alex Courbot 2012-08-16 10:52 ` Thierry Reding 2012-08-16 14:14 ` Mark Brown 2012-08-16 18:38 ` Stephen Warren 2012-08-16 18:47 ` Mark Brown 2012-08-16 18:57 ` Stephen Warren 2012-08-16 19:35 ` Stephen Warren 2012-08-16 19:49 ` Thierry Reding 2012-08-16 21:10 ` Mitch Bradley 2012-08-17 23:04 ` Mark Brown 2012-08-17 8:52 ` Alex Courbot 2012-08-21 7:44 ` Tomi Valkeinen 2012-08-21 8:22 ` Alex Courbot 2012-08-21 8:33 ` Thierry Reding 2012-08-21 8:53 ` Alex Courbot 2012-08-21 8:57 ` Tomi Valkeinen 2012-08-21 9:13 ` Thierry Reding 2012-08-21 9:54 ` Tomi Valkeinen 2012-08-21 16:57 ` Mark Brown 2012-08-22 5:42 ` Thierry Reding 2012-08-24 9:24 ` Alex Courbot 2012-08-24 10:34 ` Tomi Valkeinen 2012-08-16 6:08 ` [PATCH v4 2/3] pwm_backlight: use power sequences Alexandre Courbot 2012-08-16 18:42 ` Stephen Warren 2012-08-16 6:08 ` [PATCH v4 3/3] tegra: add pwm backlight device tree nodes Alexandre Courbot 2012-08-16 18:45 ` Stephen Warren 2012-08-16 21:47 ` [PATCH v4 0/3] Runtime Interpreted Power Sequences Rafael J. Wysocki 2012-08-17 8:54 ` Alex Courbot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).